Simplify into_responses_with_blobs logic #6927

dapplion · 2025-02-06T13:48:36Z

Issue Addressed

Pull simplification from Match block and blobs after validating execution #6469
Part of Improve range sync with PeerDAS #6258

Proposed Changes

The job of into_responses_with_blobs is to match block and blobs with the same block root. We can just use a HashMap and remove 41 lines of code.

realbigsean · 2025-02-06T18:14:43Z

looks like there's a failing test here

pawanjay176

Looks good.
The one issue I see with this is we aren't enforcing the order of the blobs based on the index of the sidecars.
We will throw the error as KzgCommitmentMismatch when trying to build the rpc block. Not sure if this would be an issue.

Edit: Actually the spec requires the sender to send it in the right order, so we were being quite permissive before

The following blob sidecars, where they exist, MUST be sent in consecutive (slot, index) order

Edit 2: I think it is fine

pawanjay176

LGTM

dapplion · 2025-02-07T21:40:10Z

Actually the spec requires the sender to send it in the right order, so we were being quite permissive before

We can enforce that if we want, but why would we do so if we don't need to? If we had some optimization we can expect that behaviour but I don't see any damage to us from peers sending blobs or blocks out of order

jimmygchen · 2025-02-14T07:09:29Z

beacon_node/network/src/sync/block_sidecar_coupling.rs

-        Ok(responses)
+        // Now collect all blobs that match to the block by block root. BlobsByRange request checks
+        // the inclusion proof so we know that the commitment is the expected.
+        // RpcBlock::new ensures that the count of blobs is consistent with the block


We don't actually perform the check if the blob list is empty or None.
Seems like it's worth checking in both cases?

Fixed with 16c95f0

jimmygchen · 2025-02-14T07:13:07Z

beacon_node/network/src/sync/block_sidecar_coupling.rs

+            .map(|block| {
+                let block_root = get_block_root(&block);
+                let max_blobs_per_block = spec.max_blobs_per_block(block.epoch()) as usize;
+                let blobs = blobs_by_block.remove(&block_root).unwrap_or_default();


The existing code ensures the blobs are in order, and i think our code does assume they are - I think we should make sure they're in order here?

Oh sorry just read Pawan's comment above.

but I don't see any damage to us from peers sending blobs or blocks out of order

Can you confirm this? IIRC we have some assumptions on the blob order in the code somewhere.

There's one here but it's not affected by this change, although i feel like we should remove the assumption here

lighthouse/beacon_node/beacon_chain/src/kzg_utils.rs

Lines 166 to 176 in 5f053b0

// NOTE: assumes blob sidecars are ordered by index

let blob_cells_and_proofs_vec = blobs

.into_par_iter()

.map(|blob| {

let blob = blob

.as_ref()

.try_into()

.expect("blob should have a guaranteed size due to FixedVector");

kzg.compute_cells_and_proofs(blob)

})

.collect::<Result<Vec<_>, KzgError>>()?;

Also the comment here mentions the assumption too:

lighthouse/beacon_node/beacon_chain/src/block_verification_types.rs

Lines 89 to 91 in 2792705

/// This variant is used with parent lookups and by-range responses. It should have all blobs

/// ordered, all block roots matching, and the correct number of blobs for this block.

BlockAndBlobs(Arc<SignedBeaconBlock<E>>, BlobSidecarList<E>),

b983fa9 enforces ordered responses on all by_range requests

dapplion · 2025-02-17T23:49:21Z

beacon_node/beacon_chain/src/block_verification_types.rs

-                }
-            }
+        if block.num_expected_blobs() != blobs.as_ref().map(|blobs| blobs.len()).unwrap_or(0) {
+            return Err(AvailabilityCheckError::MissingBlobs);


Now we always check that the blob count is consistent with the block regardless of empty list or None.

dapplion · 2025-02-17T23:49:26Z

beacon_node/beacon_chain/src/block_verification_types.rs

-            for (blob, &block_commitment) in blobs.iter().zip(block_commitments.iter()) {
-                let blob_commitment = blob.kzg_commitment;
-                if blob_commitment != block_commitment {
-                    return Err(AvailabilityCheckError::KzgCommitmentMismatch {


We don't need to check the commitment equivalence anymore because:

We check the inclusion proof of each blob

We match blobs and blocks by block root

Therefore it's impossible for the commitment to not be equivalent

dapplion · 2025-02-17T23:49:54Z

beacon_node/network/src/sync/block_sidecar_coupling.rs

-        Ok(responses)
+        // Now collect all blobs that match to the block by block root. BlobsByRange request checks
+        // the inclusion proof so we know that the commitment is the expected.
+        // RpcBlock::new ensures that the count of blobs is consistent with the block


Fixed with 16c95f0

dapplion · 2025-02-17T23:51:47Z

beacon_node/network/src/sync/network_context/requests/blobs_by_range.rs

+                        return Err(LookupVerifyError::DuplicatedData(blob.slot(), blob.index));
+                    } else if blob.index != prev.index + 1 {
+                        return Err(LookupVerifyError::NotSorted("non-sequential indices"));


We could group these two conditions into the "non-sequential" one. But I feel it's good to know if a sender is faulty due to duplicate data or non-sequential

dapplion requested a review from jxs as a code owner February 6, 2025 13:48

dapplion requested review from pawanjay176 and removed request for jxs February 6, 2025 13:48

Simplify into_responses_with_blobs logic

dd29c33

dapplion force-pushed the into_responses_with_blobs branch from e7d0352 to dd29c33 Compare February 6, 2025 13:58

realbigsean self-requested a review February 6, 2025 18:14

pawanjay176 reviewed Feb 6, 2025

View reviewed changes

pawanjay176 approved these changes Feb 6, 2025

View reviewed changes

jimmygchen requested changes Feb 14, 2025

View reviewed changes

jimmygchen added the waiting-on-author The reviewer has suggested changes and awaits thier implementation. label Feb 14, 2025

dapplion added 3 commits February 17, 2025 20:00

Sort blobs ascending

62d67ec

Enforce strict ordering on by_range responses

b983fa9

Always check that count of blobs is consistent with the block

16c95f0

dapplion commented Feb 17, 2025

View reviewed changes

dapplion added ready-for-review The code is ready for review and removed waiting-on-author The reviewer has suggested changes and awaits thier implementation. labels Feb 17, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simplify into_responses_with_blobs logic #6927

Simplify into_responses_with_blobs logic #6927

dapplion commented Feb 6, 2025

realbigsean commented Feb 6, 2025

pawanjay176 left a comment •

edited

Loading

pawanjay176 left a comment

dapplion commented Feb 7, 2025

jimmygchen Feb 14, 2025

dapplion Feb 17, 2025

jimmygchen Feb 14, 2025

jimmygchen Feb 14, 2025 •

edited

Loading

jimmygchen Feb 14, 2025

dapplion Feb 17, 2025

dapplion Feb 17, 2025

dapplion Feb 17, 2025

dapplion Feb 17, 2025

dapplion Feb 17, 2025

	// NOTE: assumes blob sidecars are ordered by index
	let blob_cells_and_proofs_vec = blobs
	.into_par_iter()
	.map(\|blob\| {
	let blob = blob
	.as_ref()
	.try_into()
	.expect("blob should have a guaranteed size due to FixedVector");
	kzg.compute_cells_and_proofs(blob)
	})
	.collect::<Result<Vec<_>, KzgError>>()?;

	/// This variant is used with parent lookups and by-range responses. It should have all blobs
	/// ordered, all block roots matching, and the correct number of blobs for this block.
	BlockAndBlobs(Arc<SignedBeaconBlock<E>>, BlobSidecarList<E>),

Simplify into_responses_with_blobs logic #6927

Are you sure you want to change the base?

Simplify into_responses_with_blobs logic #6927

Conversation

dapplion commented Feb 6, 2025

Issue Addressed

Proposed Changes

realbigsean commented Feb 6, 2025

pawanjay176 left a comment • edited Loading

Choose a reason for hiding this comment

pawanjay176 left a comment

Choose a reason for hiding this comment

dapplion commented Feb 7, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jimmygchen Feb 14, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pawanjay176 left a comment •

edited

Loading

jimmygchen Feb 14, 2025 •

edited

Loading