-
Notifications
You must be signed in to change notification settings - Fork 908
Eip 7732 containers #7807
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Eip 7732 containers #7807
Conversation
consensus/types/src/eth_spec.rs
Outdated
@@ -585,6 +612,8 @@ impl EthSpec for GnosisEthSpec { | |||
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you must have gotten confused because the spec you linked also has this as 512
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we seem to serialize to 0xABC instead of byte array when sending json over the wire
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
JSONVariant
so 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
@@ -75,6 +75,7 @@ pub trait EthSpec: 'static + Default + Sync + Send + Clone + Debug + PartialEq + | |||
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
makes total sense, updated in 76f9f83
consensus/types/src/eth_spec.rs
Outdated
@@ -585,6 +612,8 @@ impl EthSpec for GnosisEthSpec { | |||
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
JSONVariant
so 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okay so i was under the impression that the PayloadAttestation
s 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similarly here the PayloadAttestation
s 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.
ebf949f
to
a9c297e
Compare
76f9f83
to
b7bdd9c
Compare
 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// 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 |
Changes
Constants
/Misc
/Preset
/Containers
from 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 build
to compile andcargo test -p types
to be all passing in a separate PRTodo for Containers (didn't want this PR to get too big)
BeaconBlockBody
BeaconState