Skip to content
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 Deneb builder test & update mock builder #4607

Merged

Conversation

jimmygchen
Copy link
Member

@jimmygchen jimmygchen commented Aug 10, 2023

Changes

  • Add a Deneb builder flow http test, which revealed a couple of issues (see below), now we have a working happy path \o/. I'll look into adding more testes.
  • Fix an issue with builder bid signature mismatch. It turned out that the tree hash changes based on the variable list max size. So I had to use a different type alias for KzgCommitments in BlobsBundle, as the KzgCommitments in BlockBody has a different max size:
    pub type KzgCommitments<T> = VariableList<KzgCommitment, <T as EthSpec>::MaxBlobsPerBlock>;
    pub type BlockBodyKzgCommitments<T> =
    VariableList<KzgCommitment, <T as EthSpec>::MaxBlobCommitmentsPerBlock>;
  • Add missing deserialization logic for blinded block contents
  • Update mock-builder to support Deneb types from updated mev-rs and ethereum-consensus libs
  • Update shardingForkTime to cancunTime in local testnet scripts

Additional Info

  • I had to make some adjustments when running the deneb builder test in debug mode:
    • Use RUST_MIN_STACK=8388608: 4mb thread stack limit wasnt enough, looks like we really need to get @ethDreamer's eliminate blob clone PR in.
    • Increase get_header timeout: 1 second timeout wasn't enough when running in debug - I think we might even want to override this in tests via env vars, as it's likely that mock builder requires more time to return a response.
  • There is a separate branch I use for testing with mock-relay that contains expected withdrawals changes cherry-picked from [Merged by Bors] - Implement expected withdrawals endpoint #4390. I've excluded it from this PR as expected withdrawals endpoint change is still under review.

@jimmygchen jimmygchen added work-in-progress PR is a work-in-progress deneb labels Aug 10, 2023
@jimmygchen jimmygchen added ready-for-review The code is ready for review and removed work-in-progress PR is a work-in-progress labels Aug 15, 2023
@jimmygchen jimmygchen force-pushed the add-mock-builder-deneb-support branch from 66be8c2 to 6752198 Compare August 15, 2023 23:36
@jimmygchen
Copy link
Member Author

It looks like doppelganger-protection-test are now failing because of the cancunTime change, I'll revert the change in scripts/tests/genesis.json until we have a compatible el version for this test.

@jimmygchen jimmygchen changed the title Add Deneb support to mock builder and tests Add Deneb builder test & update mock builder Aug 16, 2023
Copy link
Member

@realbigsean realbigsean left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Woohoo! I reviewed Mark's blob clone PR so maybe we add that one in first

@@ -9,9 +9,22 @@ use superstruct::superstruct;
use test_random_derive::TestRandom;
use tree_hash_derive::TreeHash;

pub type KzgCommitments<T> =
pub type KzgCommitments<T> = VariableList<KzgCommitment, <T as EthSpec>::MaxBlobsPerBlock>;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hopefully this gets merged ethereum/builder-specs#87

It'd be nice not to have to introduce this type. It makes things quite confusing IMO

@realbigsean realbigsean merged commit 4898430 into sigp:deneb-free-blobs Aug 19, 2023
27 of 28 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deneb ready-for-review The code is ready for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants