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

Use engine api get-blobs for block subscriber #14513

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

terencechain
Copy link
Member

@terencechain terencechain commented Oct 7, 2024

Reference: ethereum/execution-apis#559

Background

The engine_getBlobsV1 function allows the CL to retrieve blobs and their proofs from the EL by providing the block body's KZG version hashes. If the blobs and proofs are already available in the local mempool, the EL will return them. Once the CL receives the blobs and proofs, it can import the block and run the fork choice algorithm to update the head. In the best-case scenario, this process is faster than waiting for blobs to propagate through the network. By assuming the following condition holds: $T_{BlockArrival} + \triangle_{BlobsOverEngineAPI} < T_{BlockArrival} + \triangle_{BlobsOverP2P}$ which simplifies to: $\triangle_{BlobsOverEngineAPI} < \triangle_{BlobsOverP2P}$
The benefits include:

  • Faster block import times
  • Reduced stress on nodes with limited upload bandwidth, as they don't need to burst-upload blobs in a short time frame
  • Opens discussions on potentially increasing the maximum blob count, pending production data

Prysm Implementation

Now we will discuss how Prysm may implement this optimization. Feel free to stop reading here if you're not interested in the Prysm side of things.

One way to study this is by reviewing the following task timeline:

  1. Block is received over P2P
  2. Block is validated and forwarded to peers
  3. Block is imported into core processing
  4. Block passes consensus and execution checks
  5. Block passes DA check
  6. Block can be used by fork choice

Implementation Details

  • This PR implements blob reconstruction, broadcast, and saving during step 2.
  • If the execution client doesn't support the get blobs endpoint, it will be a no-op instead of returning an error. We added exchange capabilities caching and a check beforehand.
  • Blob sidecars will not be reconstructed if they already exist in the blob database.
  • Proofs retrieved from the EL will be verified with their blobs. This verification step is fast, approximately 2ms per blob sidecar.
  • Reconstructed blob sidecars are first broadcasted all together and then imported all together to ensure IDONTWANT is released quickly.
  • New metrics track recovered blob sidecars, and new logs track processing time.

The preliminary result is documented here: https://hackmd.io/@ttsao/get-blobs-early-results

@terencechain terencechain force-pushed the get-blobs branch 5 times, most recently from f52c455 to 7f73d11 Compare October 7, 2024 15:23

// Initialize KZG hashes and retrieve blobs
kzgHashes := make([]common.Hash, len(kzgCommitments))
blobs, err := s.GetBlobs(ctx, kzgHashes)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we check if the EL supports this API (cached exchange capabilities result?) before attempting to call it? Otherwise the logs could be noisy for anyone who upgrades to this version with an EL that does not support getBlobs.

defer span.End()

result := make([]*pb.BlobAndProof, len(versionedHashes))
err := s.rpcClient.CallContext(ctx, &result, GetBlobsV1, versionedHashes)
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see the json methodset defined on this type, you probably need to define unmarshaling for the encoding to come through corrrectly.

if err != nil {
return nil, errors.Wrap(err, "could not create RO blob with root")
}
verifiedBlobs = append(verifiedBlobs, blocks.NewVerifiedROBlob(roBlob))
Copy link
Contributor

@kasey kasey Oct 7, 2024

Choose a reason for hiding this comment

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

You're calling NewVerifiedROBlob and minting VerifiedROBlobs here, rather than getting those values from a verifier. That is risky. Would it be possible to define a verifier for this? Maybe something like the initial sync verifier that can deal with a batch.

On that note, I don't see you verifying the commitment inclusion proof anywhere in this PR.

Terence reminded me we are constructing the proof so we don't have to verify it, derp.

log.WithFields(blobFields(sidecar.ROBlob)).WithError(err).Error("Failed to receive blob")
}

if err := s.cfg.p2p.BroadcastBlob(ctx, sidecar.Index, sidecar.BlobSidecar); err != nil {
Copy link
Contributor

@kasey kasey Oct 7, 2024

Choose a reason for hiding this comment

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

Would it be safe to broadcast before we call ReceiveBlob? I think it would be best to do these steps across multiple loops, like:

  • verify all of the blobs (I actually think we should do this inside ReconstructBlobSidecars since that method returns VerifiedROBlobs).
  • call BroadcastBlob for all the blobs. - this ensures we get the most benefit from idontwant. I think that calling broadcast is non-blocking; io with the individual peers happens in separate libp2p threads. don't need to worry about "rebroadcasting" blobs where we have the index on disk, because libp2p handles that.
  • Call ReceiveBlob for each blob. Do this after all the broadcasts because it will save the blobs to disk, which can block, esp if they have fsync enabled.

@terencechain terencechain force-pushed the get-blobs branch 8 times, most recently from f75b0e0 to e64a726 Compare October 16, 2024 16:54
@terencechain terencechain marked this pull request as ready for review October 16, 2024 16:54
@terencechain terencechain requested a review from a team as a code owner October 16, 2024 16:54
@terencechain terencechain force-pushed the get-blobs branch 3 times, most recently from 89e329c to 7cbbb68 Compare October 16, 2024 20:33
Debug

changelog

add proto marshal and unmarshal

Kasey's feedback
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