-
Notifications
You must be signed in to change notification settings - Fork 907
Add Gloas boilerplate #7728
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
Add Gloas boilerplate #7728
Conversation
@macladson can you add this de-duplications? |
Can we resolve conflicts on this and merge? Might be nice before your |
0cd243f
to
710dfb7
Compare
710dfb7
to
3fbbe38
Compare
b741799
to
d1ec186
Compare
I have rebased this code onto #7909 to clear up the confusion around payload versioning. I also removed the new versions of Edit: With #7909 merged this is now ready for review |
Fixes for Rust 2024
d1ec186
to
d3420f2
Compare
Some required checks have failed. Could you please take a look @macladson? 🙏 |
@@ -101,6 +101,7 @@ fn reconstruct_default_header_block<E: EthSpec>( | |||
ForkName::Deneb => ExecutionPayloadDeneb::default().into(), | |||
ForkName::Electra => ExecutionPayloadElectra::default().into(), | |||
ForkName::Fulu => ExecutionPayloadFulu::default().into(), | |||
ForkName::Gloas => ExecutionPayloadGloas::default().into(), |
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.
Note: Comment below has no impact on this PR. just something we'll have to address in epbs branches, and noting here while reviewing since it's top of mind
When loading blinded blocks from the db, we run reconstruct_default_header_block
to get a default ExecutionPayloadHeader
. If we expect the db to store blinded blocks for Gloas, then this ForkName::Gloas => ExecutionPayloadGloas::default().into()
variant will be hit for creating a default payload, but when we try let header_from_payload = ExecutionPayloadHeader::from(payload.to_ref());
right below it, ExecutionPayloadHeader
is no longer supported in gloas, so it will fail.
I haven't thought enough about how the db will be refactored, but my intuition is that we will only store Full
beacon blocks post-gloas, so we should be able to move Gloas into the variant:
ForkName::Base | ForkName::Altair => {
return Err(Error::PayloadReconstruction(format!(
"Block with fork variant {} has execution payload",
fork
))
.into())
@@ -3244,6 +3252,25 @@ pub fn generate_rand_block_and_blobs<E: EthSpec>( | |||
message.body.blob_kzg_commitments = bundle.commitments.clone(); | |||
bundle | |||
} | |||
SignedBeaconBlock::Gloas(SignedBeaconBlockGloas { |
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.
Note: Comment below has no impact on this PR. just something we'll have to address in epbs branches, and noting here while reviewing since it's top of mind
we removed the gloas variant of generate_rand_block_and_blobs
in eip-7732 branch since beacon blocks no longer contain execution payloads or blobs.
The goal of this test seems to be:
- Generate a beacon block with blob commitments
- Generate corresponding blob sidecars
- Link them together with proofs
This won't work in gloas, since the kzg commitments will be revealed along with the ExecutionPayloadEnvelope
instead of the BeaconBlock
. We should probably make a comment in the epbs branches that we'll need a new test based off this separation
}, | ||
}, | ||
}, | ||
let forks = [ |
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.
much cleaner now 🤙
LGTM, all changes seem isolated to gloas variants, so no side effects on current functionality. ofcourse could use a few more sets of 👀 |
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.
LGTM!
As Shane mentioned, there are many things in this PR that don't make total sense in the context of block payload separation. So we've been addressing them as they come up in the implementation of Gloas. It doesn't make sense to address them in this PR as it's beyond the scope and will start to cause conflicts with the changes we've already made. |
@@ -16,7 +16,7 @@ use types::*; | |||
/// | |||
/// This can be deleted once schema versions prior to V22 are no longer supported. |
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'll be able to delete this once Fulu goes live on mainnet.
i.e. we can avoid making any more Gloas-related changes to this (hopefully).
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.
LGTM, let's merge
Proposed Changes
Adds the required boilerplate code for the Gloas (Glamsterdam) hard fork. This allows PRs testing Gloas-candidate features to test fork transition.
This also includes de-duplication of post-Bellatrix readiness notifiers from #6797 (credit to @dapplion)
Additional Info
Not all tests have been updated yet.