Implement /eth/v1/beacon/blobs endpoint
#8103
Conversation
chong-he
added
ready-for-review
jimmygchen
added
waiting-on-author
|
btw very thorough with the spec compliance, i think the impl great in terms of correctness, and thanks for the test outputs. Nice work 👍 |
jimmygchen
added
v8.0.0
|
Bumping this to v8 mainnet release |
chong-he
added
ready-for-review
Thanks so much for the thorough review. This PR is ready for another round of review. Thank you both @michaelsproul and @jimmygchen |
| .map_err(|e| { | ||
| format!("Failed to recover cells and compute KZG proofs: {e:?}") | ||
| })?; | ||
| cells.into_iter().collect() |
There was a problem hiding this comment.
is it possible to avoid the collect into Vec here? it doesn't seem necessary and we're just converting the vec back into into_inter() below.
| debug!("Downloading finalized blobs"); | ||
| if let Some(response) = remote | ||
| .get_blobs::<E>(BlockId::Root(block_root), None, &spec) | ||
| .get_blob_sidecars::<E>(BlockId::Root(block_root), None, &spec) |
There was a problem hiding this comment.
This endpoint is being deprecated and will likely be removed soon - we can probably address this in a separate PR
There was a problem hiding this comment.
We're handling this gracefully at the moment so it's not a problem, i think we might eventually just remove this call and download columns from peers.
| }) | ||
| .map(|index| index as u64) | ||
| .collect() | ||
| } else { |
There was a problem hiding this comment.
I think we don't need this else statement here, because the get_blobs_from_data_columns and get_blobs functions accept Option and already handles this.
So this could just be simplified to:
let blob_indices_opt = query.versioned_hashes.map(|versioned_hashes| {
versioned_hashes
.iter()
.flat_map(|versioned_hash| {
blob_kzg_commitments.iter().position(move |commitment| {
let computed_hash = kzg_commitment_to_versioned_hash(commitment);
computed_hash == *versioned_hash
})
})
.map(|index| index as u64)
.collect::<Vec<_>>()
});
| let blobs = blob_sidecar_list | ||
| .into_iter() | ||
| .map(|sidecar| BlobWrapper::<T::EthSpec> { | ||
| blob: sidecar.blob.clone(), |
There was a problem hiding this comment.
Note that this is an expensive clone as blob isn't Arc'd.
However I don't see easy way around this unless we change the functions above to return Blobs instead.
There was a problem hiding this comment.
Just a note and no change required here
| let list: Vec<_> = vec | ||
| .into_iter() | ||
| .filter(|blob_sidecar| vec.contains(&blob_sidecar.index)) | ||
| .flat_map(|index| blob_sidecar_list.get(index as usize).cloned()) |
Issue Addressed
Proposed change
Rename current
blob_sidecarsendpoint fromget_blobstoget_blob_sidecarsto "give way" to the new get blobs endpoint. I thought this is the better way to have this new get_blobs endpoint name it this way in the code, but I am happy to change if that's not suitable/confusingAdditional Info
Testing results:
Query with:
curl -X 'GET' 'http://localhost:5052/eth/v1/beacon/blobs/12647385' | jqResponse:
Query with
versioned_hashes:curl -X 'GET' \ 'http://localhost:5052/eth/v1/beacon/blobs/12647385?versioned_hashes=0x01171d78b2bd7cf3c3e458f7337f37272bfe6972cdd07b6e12f399a94ff6627c&versioned_hashes=0x011384275182442b69831cc581de3b80fe60b13a8efd870f8b680422087b960e' \ -H 'accept: application/json' | jqResponse: