Match block and blobs after validating execution #6469

dapplion · 2024-10-06T14:02:04Z

Issue Addressed

After PeerDAS we request block and data to different peers. Consider the following scenario when doing a by range sync:

Block at slot N contains no data
Peer serving the block at slot N forges the block adding KZG commitments
The peers serving data columns do not return any data columns for slot N

Current unstable will attempt to match block and columns and consider the entire RpcBlock invalid. A similar scenario can occur where the block peer is honest and the column peer is dishonest.

We need to make the code more aware of who is at fault, and specifically do not attempt to match columns before validating the block.

Part of

Improve range sync with PeerDAS #6258

Note: this PR touches code on the mainnet path, not only PeerDAS

Proposed Changes

Relax RpcBlock construction with new_unchecked and perform matching in verify_kzg_for_rpc_block after the block is execution validated.

This PR does not include a fix for peer scoring. This will be tackled latter as part of #6258

beacon_node/network/src/sync/block_sidecar_coupling.rs

pawanjay176 · 2024-10-08T23:19:55Z

beacon_node/network/src/sync/block_sidecar_coupling.rs

@@ -85,60 +86,41 @@ impl<E: EthSpec> RangeBlockComponentsRequest<E> {
    }

    pub fn into_responses(self, spec: &ChainSpec) -> Result<Vec<RpcBlock<E>>, String> {
-        if let Some(expects_custody_columns) = self.expects_custody_columns.clone() {
-            self.into_responses_with_custody_columns(expects_custody_columns, spec)
+        if let Some(expected_column_indices) = self.expects_custody_columns.as_ref() {


This field should probably be renamed to expected_column_indices to better reflect its type. Doesn't need to be part of this PR though

beacon_node/network/src/sync/block_sidecar_coupling.rs

pawanjay176 · 2024-10-08T23:51:13Z

beacon_node/network/src/sync/block_sidecar_coupling.rs

-                ));
-            }
+                // Safe to convert to `CustodyDataColumn`: we have asserted that the index of
+                // this column is in the set of `expects_custody_columns` and with the expected


Where have we done this assertion?

In sync we only request custody columns

pawanjay176 · 2024-10-09T01:11:39Z

beacon_node/network/src/sync/block_sidecar_coupling.rs

-            }
-            let blobs = VariableList::from(blobs_buffer.into_iter().flatten().collect::<Vec<_>>());
-            responses.push(RpcBlock::new(None, block, Some(blobs)).map_err(|e| format!("{e:?}"))?)
+        for blob in blobs {


Neat simplificaiton in the logic 👍

beacon_node/network/src/sync/block_sidecar_coupling.rs

pawanjay176 · 2024-10-09T01:20:24Z

beacon_node/beacon_chain/src/data_availability_checker.rs

@@ -311,8 +311,13 @@ impl<T: BeaconChainTypes> DataAvailabilityChecker<T> {
        let (block_root, block, blobs, data_columns) = block.deconstruct();
        if self.blobs_required_for_block(&block) {
            return if let Some(blob_list) = blobs.as_ref() {
+                if block.num_expected_blobs() != blob_list.len() {


Should we also check if the block and blob commitments match here?
Same check in RpcBlock::new

We could also do this in RpcBlock::deconstruct

if we check the inclusion proof, we check that the commitments match the block. I've added a check in the da_checker. It's unnecessary in most paths but would be best to skip it only as an optimization by adding a property to RpcBlock::valid_inclusion_proof, and only if true, skip the check

pawanjay176 · 2024-10-09T03:41:45Z

beacon_node/network/src/sync/block_sidecar_coupling.rs

+        for block in blocks {
+            let block_root = get_block_root(&block);
+            let blobs = blobs_by_block.remove(&block_root).map(VariableList::from);
+            rpc_blocks.push(RpcBlock::new_unchecked(block_root, block, blobs, None));


I think using the unchecked variant here might result in an inconsistent state.

Consider the example you used, the peer/peers have sent us a valid block for a block_root x which contains 1 kzg_commitment. The peer sends us 2 blobs for this block root which do not match with the kzg_commitment in the block. The malicious blobs pass the kzg verification in isolation, they just doesn't match with the block commitment.

Now the unchecked RpcBlock is sent for processing. The block goes through the validation pipeline
I couldn't find anywhere we check that the block and blob commitments match up. Correct me if i'm wrong here though.

We should potentially do this check when we doRpcBlock::deconstruct

I couldn't find anywhere we check that the block and blob commitments match up

In lookup sync when doing a blobs request we check the inclusion proof, here

lighthouse/beacon_node/network/src/sync/network_context/requests/blobs_by_root.rs

Line 53 in 40d3423

if !blob.verify_blob_sidecar_inclusion_proof() {

However we don't for range sync. I've added the inclusion proof check in the da_checker. We can remove it after #6497

pawanjay176 · 2024-10-09T03:44:15Z

beacon_node/beacon_chain/src/block_verification_types.rs

@@ -193,34 +170,60 @@ impl<E: EthSpec> RpcBlock<E> {
        Self::new(Some(block_root), block, blobs)
    }

+    /// Create an RpcBlock from an untrusted block, without checking that the blobs or columns are


I think the docs for RpcBlock and RpcBlockInner need to be updated to reflect the fact that they could be constructed without checking for consistency

dapplion added the ready-for-review The code is ready for review label Oct 6, 2024

dapplion requested review from realbigsean and pawanjay176 October 6, 2024 14:02

pawanjay176 added under-review A reviewer has only partially completed a review. and removed ready-for-review The code is ready for review labels Oct 8, 2024

pawanjay176 requested changes Oct 9, 2024

View reviewed changes

pawanjay176 added waiting-on-author The reviewer has suggested changes and awaits thier implementation. and removed under-review A reviewer has only partially completed a review. labels Oct 10, 2024

dapplion added 2 commits October 24, 2024 14:05

Match block and blobs after validating execution

a0bae5a

Check matching commitments

328f4d4

dapplion force-pushed the coupling-by-range branch from 130c04c to 328f4d4 Compare October 24, 2024 12:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Match block and blobs after validating execution #6469

Match block and blobs after validating execution #6469

dapplion commented Oct 6, 2024 •

edited

Loading

pawanjay176 Oct 8, 2024

pawanjay176 Oct 8, 2024

dapplion Oct 24, 2024

pawanjay176 Oct 9, 2024

pawanjay176 Oct 9, 2024

pawanjay176 Oct 9, 2024

dapplion Oct 24, 2024 •

edited

Loading

pawanjay176 Oct 9, 2024

dapplion Oct 24, 2024 •

edited

Loading

pawanjay176 Oct 9, 2024

Match block and blobs after validating execution #6469

Are you sure you want to change the base?

Match block and blobs after validating execution #6469

Conversation

dapplion commented Oct 6, 2024 • edited Loading

Issue Addressed

Proposed Changes

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

dapplion Oct 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dapplion Oct 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dapplion commented Oct 6, 2024 •

edited

Loading

dapplion Oct 24, 2024 •

edited

Loading

dapplion Oct 24, 2024 •

edited

Loading