feat(l1): add engine_getBlobsV3 rpc method#5648
feat(l1): add engine_getBlobsV3 rpc method#5648MegaRedHand merged 7 commits intolambdaclass:mainfrom
engine_getBlobsV3 rpc method#5648Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds support for the engine_getBlobsV3 RPC method, which allows partial blob data to be returned, unlike engine_getBlobsV2 which returns all blobs or nothing. The implementation refactors common logic between V2 and V3 into a shared helper function.
Key Changes:
- Introduced new
BlobsV3Requeststruct and RPC handler - Refactored blob retrieval logic into shared
get_blobs_and_proof_v2function - Added
engine_getBlobsV3to capabilities and routing
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| crates/networking/rpc/rpc.rs | Added BlobsV3Request import and routing for engine_getBlobsV3 method |
| crates/networking/rpc/engine/mod.rs | Updated capabilities array to include engine_getBlobsV3 |
| crates/networking/rpc/engine/blobs.rs | Introduced BlobsV3Request handler and refactored shared blob retrieval logic into get_blobs_and_proof_v2 function |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Thanks for your contribution! Left some comments, but should be good to merge once those are addressed |
| for blobs_bundle in context.blockchain.mempool.get_blobs_bundle_pool()? { | ||
| // Go over all blobs bundles from the blobs bundle pool. | ||
| let blobs_in_bundle = blobs_bundle.blobs; | ||
| let commitments_in_bundle = blobs_bundle.commitments; | ||
| let proofs_in_bundle = blobs_bundle.proofs; | ||
|
|
||
| // Go over all the commitments in each blobs bundle to calculate the blobs versioned hash. | ||
| for (commitment, (blob, proofs)) in commitments_in_bundle.iter().zip( | ||
| blobs_in_bundle | ||
| .iter() | ||
| .zip(proofs_in_bundle.chunks(CELLS_PER_EXT_BLOB)), | ||
| ) { | ||
| let current_versioned_hash = kzg_commitment_to_versioned_hash(commitment); | ||
| if let Some(index) = blob_versioned_hashes | ||
| .iter() | ||
| .position(|&hash| hash == current_versioned_hash) | ||
| { | ||
| // If the versioned hash is one of the requested we save its corresponding blob and proof in the returned vector. We store them in the same position as the versioned hash was received. | ||
| res[index] = Some(BlobAndProofV2 { | ||
| blob: *blob, | ||
| proofs: proofs.to_vec(), | ||
| }); | ||
| } | ||
| } | ||
| if res.iter().any(|blob| blob.is_none()) { | ||
| return Ok(Value::Null); | ||
| } |
There was a problem hiding this comment.
This looks like a lot of work to do for a single RPC call. We should probably, at the very least, hash once when we accept the blob tx to the mempool and keep that hash, rather than hash all of them each time someone calls this endpoint. Looks like a DoS vector to me.
NOTE: this is not necessarily the place to fix it, being already there, it just got it to my attention.
@MegaRedHand should we open an issue for this?
There was a problem hiding this comment.
Let's. I think we should also add a get_blobs(versioned_hashes) method for fetching the blobs with those versioned hashes only.
There was a problem hiding this comment.
I will raise a PR with the a new method once the issue is up. Could you assign it to me when you create the issue @MegaRedHand. Thanks!
| if let Some(index) = blob_versioned_hashes | ||
| .iter() | ||
| .position(|&hash| hash == current_versioned_hash) | ||
| { | ||
| // If the versioned hash is one of the requested we save its corresponding blob and proof in the returned vector. We store them in the same position as the versioned hash was received. | ||
| res[index] = Some(BlobAndProofV2 { | ||
| blob: *blob, | ||
| proofs: proofs.to_vec(), | ||
| }); | ||
| } |
There was a problem hiding this comment.
This search also looks rather worrisome in terms of performance. We iterate all blobs, hash them, then iterate all hashes in the request each time. I suggest we just build a hashmap mapping hash to desired position (pseudo-code):
let mut res = vec![None; blob_versioned_hashes.len()];
let versioned_hashes_to_pos = HashMap::from_iter(blob_versioned_hashes.iter().enumerate().map(|(i, h)| (*h, i)));
for blob in blobs_pool {
let hash = blob.hopefully_precomputed_sha2();
let Some(pos) = versioned_hashes_to_pos.get(&hash) else {
continue;
};
res[pos] = build_the_thing(blob);
}Extra points for keeping a mapping of versioned hash to blob as well.
Again, not to handle now, just pointing out the issues in the original code.
3 participants
Motivation
Adds new rpc method almost identical to
engine_getBlobsV2expect the new method allows partial data as opposed to all or nothing.Description
Closes #5634