Fix for #7305: Verify blobs and data columns during backfill #7353

SunnysidedJ · 2025-04-23T19:17:01Z

Issue Addressed

#7305: Verify blobs and data columns during backfill

Proposed Changes

Compare signatures (equality check) for blob and data column sidecars
Then, validate KZG commitments as described in the issue.

Additional Info

This will be further compared and leveraged with #7352 as some parts of this issue is also covered as part of the issue it is fixing. So, WIP and will let you know once it is ready under the comments.

SunnysidedJ · 2025-04-23T19:19:57Z

Currently under debugging its test case. The test is:

lighthouse/beacon_node/beacon_chain/tests/store_tests.rs

Line 2267 in c13e069

async fn weak_subjectivity_sync_skips_at_genesis() {

However, the test does not run properly when Deneb or Fulu is enabled, e.g. FORK_NAME=deneb cargo test weak_subjectivity_sync --features "fork_from_env".

SunnysidedJ · 2025-04-30T13:00:40Z

beacon_node/beacon_chain/src/data_availability_checker.rs

@@ -783,23 +783,6 @@ impl<E: EthSpec> AvailableBlock<E> {
        } = self;
        (block_root, block, blob_data)
    }
-
-    /// Only used for testing


Removed as it is no longer called in testing (moved to RpcBlock)

SunnysidedJ · 2025-04-30T13:01:58Z

beacon_node/beacon_chain/src/historical_blocks.rs

+        // Verify that blobs or data columns signatures match
+        let sig_timer = metrics::start_timer(&metrics::BACKFILL_SIGNATURE_TOTAL_TIMES);
+        let setup_timer = metrics::start_timer(&metrics::BACKFILL_SIGNATURE_SETUP_TIMES);
+        // TODO: this logic is redundant with one from range sync. Do we have a good place to make


Maybe move it inside of verify_kzg_for_rpc_blocks?

Sorry, forgot to integrate this. Let me do it tomorrow!

SunnysidedJ · 2025-04-30T13:02:45Z

beacon_node/beacon_chain/src/historical_blocks.rs

 impl<T: BeaconChainTypes> BeaconChain<T> {
+    fn verify_blobs_and_data_columns(


pulled out verification parts as import_historical_block_batch() has become too long

SunnysidedJ · 2025-04-30T13:18:36Z

Status: Ready for review.
Tests run:

pre-Deneb: as it is in store_tests.rs
pre-Fulu, post-Deneb: + incorrect signature in blob header + incorrect KZG commitment in blob
post-Fulu: + incorrect signature in data column header + incorrect KZG commitment in data column

SunnysidedJ · 2025-04-30T13:18:45Z

Hey @dapplion, I much liked your implementation from #7352, where you've used existing functions I wasn't aware of and also made helper functions that share the same logics between range sync.

I tried to minimize the conflicts I hope this is okay with you. Please let me know for any comments!

There're a few parts that I changed from yours.

I made minimal changes to sync_methods.rs as there are many more changes related to your PR (peer scoring itself).
Didn't take assert_correct_historical_block_chain() because if import_historical_block_batch() bails during block signature verification, the oldest block parent is updated regardless. I think this shouldn't happen (I think you also mentioned about DB stuff in your comment?). However, I left as it is to preserve working code.
Properly added timer metrics

I haven't made any changes to the range sync verification as it wasn't scope of the issue (I would've done it if I was aware of) and thought it would create more conflicts between your PR.

beacon_node/beacon_chain/src/block_verification_types.rs

beacon_node/beacon_chain/src/data_availability_checker.rs

beacon_node/beacon_chain/src/historical_blocks.rs

dapplion · 2025-04-30T16:20:48Z

beacon_node/beacon_chain/src/historical_blocks.rs

+        // Verify that blobs or data columns signatures match
+        let sig_timer = metrics::start_timer(&metrics::BACKFILL_SIGNATURE_TOTAL_TIMES);
+        let setup_timer = metrics::start_timer(&metrics::BACKFILL_SIGNATURE_SETUP_TIMES);
+        // TODO: this logic is redundant with one from range sync. Do we have a good place to make


Maybe move it inside of verify_kzg_for_rpc_blocks?

beacon_node/beacon_chain/src/historical_blocks.rs

dapplion · 2025-04-30T16:23:06Z

beacon_node/beacon_chain/src/historical_blocks.rs

-        drop(verify_timer);
-        drop(sig_timer);
+        // Verify signatures in a batch.
+        self.verify_available_block_signatures(&signed_blocks)?;


Order of ops should be:

verify block signatures

match block and sidecar headers

verify kzg proofs

To do proper scoring later

dapplion · 2025-04-30T16:24:55Z

beacon_node/network/src/network_beacon_processor/sync_methods.rs

+                    | HistoricalBlockError::InvalidDataColumnsSignature(_)
+                    | HistoricalBlockError::Unexpected(_)
+                    | HistoricalBlockError::AvailabilityCheckError(_) => {
+                        warn!(


Only internal errors should be a warn, peers can send invalid objects and users can't do anything about it.

dapplion · 2025-04-30T16:25:49Z

beacon_node/beacon_chain/tests/store_tests.rs

@@ -2279,6 +2279,9 @@ async fn weak_subjectivity_sync_test(slots: Vec<Slot>, checkpoint_slot: Slot) {
    let temp1 = tempdir().unwrap();
    let full_store = get_store(&temp1);

+    let is_deneb = full_store.get_chain_spec().deneb_fork_epoch.is_some();
+    let is_fulu = full_store.get_chain_spec().is_peer_das_scheduled();


ForkName::fulu_enabled()

dapplion · 2025-04-30T16:26:15Z

beacon_node/beacon_chain/tests/store_tests.rs

@@ -2279,6 +2279,9 @@ async fn weak_subjectivity_sync_test(slots: Vec<Slot>, checkpoint_slot: Slot) {
    let temp1 = tempdir().unwrap();
    let full_store = get_store(&temp1);

+    let is_deneb = full_store.get_chain_spec().deneb_fork_epoch.is_some();


Use ForkName::deneb_enabled()

The way I think I can use is something like fork_name_at_slot::(Slot::new(0)). I can see that it is safer to use ForkName than ChainSpec (that it checks previous forks are enabled), but I'm not sure it is cleaner here, and all others are using this format.

dapplion · 2025-04-30T16:27:10Z

beacon_node/beacon_chain/tests/store_tests.rs

+                    false
+                }
+            })
+            .unwrap();


Why do you need to find this position? Just setup the rpc_blocks such that the first block always has data

Then, we might get to test non-first index as well!

beacon_node/beacon_chain/tests/store_tests.rs

SunnysidedJ · 2025-05-05T15:37:22Z

Restructured backfill importing such that

it now clearly separates block validation and storing.
it follows PeerDAS scoring order

SunnysidedJ · 2025-05-16T05:59:26Z

@dapplion could you review the PR?

dapplion · 2025-05-16T20:14:45Z

We are focusing on merging #7352 first, will port the remaining diff from here later

work so far

3e5604d

SunnysidedJ mentioned this pull request Apr 23, 2025

Verify blobs and data columns during backfill #7305

Open

3 tasks

SunnysidedJ added 2 commits April 28, 2025 18:10

test done for deneb blobs. fulu cols in progress

d15aa3f

taking changes from dapplion and refactor and clean-ups

a58760a

jimmygchen added das Data Availability Sampling work-in-progress PR is a work-in-progress labels Apr 30, 2025

SunnysidedJ commented Apr 30, 2025

View reviewed changes

SunnysidedJ marked this pull request as ready for review April 30, 2025 13:19

SunnysidedJ requested a review from jxs as a code owner April 30, 2025 13:19

dapplion reviewed Apr 30, 2025

View reviewed changes

beacon_node/beacon_chain/src/block_verification_types.rs Outdated Show resolved Hide resolved

dapplion reviewed Apr 30, 2025

View reviewed changes

restructured importing for logical code separation

6a9f381

signature header call to a single place

b85dbdb

SunnysidedJ requested a review from dapplion May 8, 2025 11:25

dapplion mentioned this pull request May 19, 2025

Fix PeerDAS sync scoring #7352

Merged

2 tasks

		impl<T: BeaconChainTypes> BeaconChain<T> {
		fn verify_blobs_and_data_columns(

Fix for #7305: Verify blobs and data columns during backfill #7353

Are you sure you want to change the base?

Fix for #7305: Verify blobs and data columns during backfill #7353

Uh oh!

Conversation

SunnysidedJ commented Apr 23, 2025

Issue Addressed

Proposed Changes

Additional Info

Uh oh!

SunnysidedJ commented Apr 23, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SunnysidedJ commented Apr 30, 2025

Uh oh!

SunnysidedJ commented Apr 30, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dapplion Apr 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

SunnysidedJ commented May 5, 2025

Uh oh!

SunnysidedJ commented May 16, 2025

Uh oh!

dapplion commented May 16, 2025

Uh oh!

Uh oh!

dapplion Apr 30, 2025 •

edited

Loading