Eip 7732 containers#7807
Conversation
|
|
consensus/types/src/eth_spec.rs
Outdated
| type BytesPerCell = U2048; | ||
| type KzgCommitmentsInclusionProofDepth = U4; | ||
| type ProposerLookaheadSlots = U32; // Derived from (MIN_SEED_LOOKAHEAD + 1) * SLOTS_PER_EPOCH | ||
| type PTCSize = U64; // todo: verify if needs to be U512 for some reason like in Mark's OG implementation |
There was a problem hiding this comment.
@ethDreamer, just wanted to check with you on this. pretty sure all good now but wanted to flag it anyways (https://ethereum.github.io/consensus-specs/specs/_features/eip7732/beacon-chain/#misc_1)
There was a problem hiding this comment.
you must have gotten confused because the spec you linked also has this as 512?
There was a problem hiding this comment.
@ethDreamer, I'm reading
as saying: PTCSize should be of type uint64 with a constant value of 512. so I think we're ok to uses U64 instead of U512 in the eth_spec.rs?
There was a problem hiding this comment.
Ohhhh no these types are special. The type is actually its value, not the number of bits in the type. Look at the typenum crate in rust.
There was a problem hiding this comment.
ahh, I see! typenum lets you express that the type represents 512 in this case, nice
updated in latest commit 76f9f83
consensus/types/src/execution_bid.rs
Outdated
| pub parent_block_root: Hash256, | ||
| pub block_hash: ExecutionBlockHash, | ||
| #[serde(with = "serde_utils::address_hex")] | ||
| pub fee_recipient: Address, // todo(eip-7732): verify if this needs address_hex serialization |
There was a problem hiding this comment.
we seem to serialize to 0xABC instead of byte array when sending json over the wire
There was a problem hiding this comment.
So.. when gossiping payloads we always serialize with SSZ which is unaffected by the serde configuration. SSZ serialization/deserialization is achieved from Encode and Decode traits while JSON serialization/deserialization is done with the Serialize and Deserialize traits via serde.
There are two ways that consensus objects get serialized into JSON:
- The beacon API (which while use these objects directly)
- The engine API (where the objects are first converted to
JSONVariantso that the serialization matches the engine API.. side note.. perhaps I should rename all instances from JSON to Engine as that would make more sense..
I don't believe this object ever enters the Engine API so it would only need to conform to the beacon API standards for JSON encoding.
There was a problem hiding this comment.
for sure! I was familiar with Encode and Decode being for ssz, and Serialize/Deserialize being for json, but I did not know about the engine api having to convert to JSONVariant for serialization. Will keep this in mind
michaelsproul
added
gloas
consensus
ethDreamer
left a comment
ethDreamer
left a comment
There was a problem hiding this comment.
Great start! Only small changes and I need to go clarify some stuff about the spec I now realize.
consensus/types/src/eth_spec.rs
Outdated
| type EpochsPerSlashingsVector: Unsigned + Clone + Sync + Send + Debug + PartialEq; | ||
| type HistoricalRootsLimit: Unsigned + Clone + Sync + Send + Debug + PartialEq; | ||
| type ValidatorRegistryLimit: Unsigned + Clone + Sync + Send + Debug + PartialEq; | ||
| type BuilderPendingWithdrawalsLimit: Unsigned + Clone + Sync + Send + Debug + PartialEq; |
There was a problem hiding this comment.
small nit here - this is a state list length but this list appears as all phase0 state list lengths - others have been put along with their respective fork (see Electra's PendingDepositsLimit and PendingPartialWithdrawalsLimit)
consensus/types/src/eth_spec.rs
Outdated
| type BytesPerCell = U2048; | ||
| type KzgCommitmentsInclusionProofDepth = U4; | ||
| type ProposerLookaheadSlots = U32; // Derived from (MIN_SEED_LOOKAHEAD + 1) * SLOTS_PER_EPOCH | ||
| type PTCSize = U64; // todo: verify if needs to be U512 for some reason like in Mark's OG implementation |
There was a problem hiding this comment.
you must have gotten confused because the spec you linked also has this as 512?
consensus/types/src/execution_bid.rs
Outdated
| pub parent_block_root: Hash256, | ||
| pub block_hash: ExecutionBlockHash, | ||
| #[serde(with = "serde_utils::address_hex")] | ||
| pub fee_recipient: Address, // todo(eip-7732): verify if this needs address_hex serialization |
There was a problem hiding this comment.
So.. when gossiping payloads we always serialize with SSZ which is unaffected by the serde configuration. SSZ serialization/deserialization is achieved from Encode and Decode traits while JSON serialization/deserialization is done with the Serialize and Deserialize traits via serde.
There are two ways that consensus objects get serialized into JSON:
- The beacon API (which while use these objects directly)
- The engine API (where the objects are first converted to
JSONVariantso that the serialization matches the engine API.. side note.. perhaps I should rename all instances from JSON to Engine as that would make more sense..
I don't believe this object ever enters the Engine API so it would only need to conform to the beacon API standards for JSON encoding.
| #[serde(bound = "E: EthSpec", deny_unknown_fields)] | ||
| #[cfg_attr(feature = "arbitrary", arbitrary(bound = "E: EthSpec"))] | ||
| #[context_deserialize(ForkName)] | ||
| pub struct IndexedPayloadAttestation<E: EthSpec> { |
There was a problem hiding this comment.
okay so i was under the impression that the PayloadAttestations never go on chain but the existence of this object (and the latest spec) has them in the BeaconBlock.. i need to go figure out why..
| pub data: PayloadAttestationData, | ||
| pub signature: AggregateSignature, | ||
| } | ||
|
|
There was a problem hiding this comment.
Similarly here the PayloadAttestations are aggregated by the proposer.. this was the opposite of what I had understood.. I wonder if this was accidentally left in.. going to go check.
shane-moore
force-pushed
the
eip-7732-containers
branch
from
ebf949f to
a9c297e
Compare
shane-moore
force-pushed
the
eip-7732-containers
branch
from
76f9f83 to
b7bdd9c
Compare
|
 |
ethDreamer
left a comment
ethDreamer
left a comment
There was a problem hiding this comment.
looks good! just a couple of changes now
consensus/types/src/execution_bid.rs
Outdated
| #[cfg_attr(feature = "arbitrary", derive(arbitrary::Arbitrary))] | ||
| #[derivative(PartialEq, Hash)] | ||
| #[context_deserialize(ForkName)] | ||
| // This is what Potuz' spec calls an `ExecutionPayload` even though it's clearly a bid. |
There was a problem hiding this comment.
| // This is what Potuz' spec calls an `ExecutionPayload` even though it's clearly a bid. | |
| // This is what Potuz' spec calls an `ExecutionPayloadHeader` even though it's clearly a bid. | |
| // https://github.com/ethereum/consensus-specs/blob/bba2c7be148d6d921d2ca5e1cc528f5daaf456d9/specs/gloas/beacon-chain.md#executionpayloadheader |
|
continuing this work in #7923 |
4 participants
Changes
Constants/Misc/Preset/Containersfrom consensus specs while cross-referencing the old epbs branch, which already had some of the containers built out and just needed some minor tweaks per updates to the specNote: I made a couple fixes to the gloas boilerplate to get
cargo buildto compile andcargo test -p typesto be all passing in a separate PRTodo for Containers (didn't want this PR to get too big)
BeaconBlockBodyBeaconState