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

first draft of getBlobs #6913

Draft
wants to merge 5 commits into
base: unstable
Choose a base branch
from
Draft

first draft of getBlobs #6913

wants to merge 5 commits into from

Conversation

agnxsh
Copy link
Contributor

@agnxsh agnxsh commented Feb 10, 2025

ethereum/execution-apis#559

if EL supports engine_getBlobsV1

  • makes the block available as soon as the CL gets all the required blobs from the blob pool
  • populates blob quarantine with that info (should probably verify proofs and inclusion once, like gossip?)
  • pops from blob quarantine and add blobs to the block processor

else

  • does everything the conventional way

Note: getBlobs in nimbus-eth2 here is made to backward compatible from Electra and NOT Deneb as per spec
CI failing: because of some nim-web3 upstream incompatibilities

Copy link

github-actions bot commented Feb 10, 2025

Unit Test Results

       15 files  ±0    2 610 suites  ±0   1h 16m 15s ⏱️ +17s
  6 403 tests ±0    5 882 ✔️ ±0  521 💤 ±0  0 ±0 
44 579 runs  ±0  43 861 ✔️ ±0  718 💤 ±0  0 ±0 

Results for commit 3142f19. ± Comparison against base commit 135febc.

♻️ This comment has been updated with latest results.

@agnxsh
Copy link
Contributor Author

agnxsh commented Feb 11, 2025

for devnet testing locally, use ethpandaops/nimbus-eth2:getBlobs

@agnxsh agnxsh requested a review from tersec February 11, 2025 11:39
@@ -57,6 +57,7 @@ type
GetPayloadV3Response |
GetPayloadV4Response


Copy link
Contributor

Choose a reason for hiding this comment

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

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oops, will cleanup

when consensusFork >= ConsensusFork.Electra:
# Pull blobs and proofs from the EL blob pool
let blobsFromElOpt = await node.elManager.sendGetBlobs(forkyBlck)
debugEcho "pulled blobs from el"
Copy link
Contributor

Choose a reason for hiding this comment

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

Obligatory comment about debugEcho (but yeah, it's a draft PR and useful for now)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

just for now, until i see things are ok in the local devnet

return ok(requests[bestResponse.get()].value().blobsAndProofs)

else:
# should not reach this case
Copy link
Contributor

Choose a reason for hiding this comment

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

How "should" is this (i.e. if it's a logic bug, or a potentially buggy EL, or etc)?

return err()
let
timeout = GETBLOBS_TIMEOUT
deadline = sleepAsync(timeout)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not directly use GETBLOBS_TIMEOUT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i was considering to add some extra processing latency

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants