lighthouse icon indicating copy to clipboard operation
lighthouse copied to clipboard

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

Open SunnysidedJ opened this issue 8 months ago • 6 comments

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 avatar Apr 23 '25 19:04 SunnysidedJ

Currently under debugging its test case. The test is: https://github.com/sigp/lighthouse/blob/c13e069c9c633535336bca6d6f5f7b30720df629/beacon_node/beacon_chain/tests/store_tests.rs#L2267

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 avatar Apr 23 '25 19:04 SunnysidedJ

Status: Ready for review. Tests run:

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

SunnysidedJ avatar Apr 30 '25 13:04 SunnysidedJ

Hey @dapplion, I much liked your implementation from https://github.com/sigp/lighthouse/pull/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.

  1. I made minimal changes to sync_methods.rs as there are many more changes related to your PR (peer scoring itself).
  2. 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.
  3. 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.

SunnysidedJ avatar Apr 30 '25 13:04 SunnysidedJ

Restructured backfill importing such that

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

SunnysidedJ avatar May 05 '25 15:05 SunnysidedJ

@dapplion could you review the PR?

SunnysidedJ avatar May 16 '25 05:05 SunnysidedJ

We are focusing on merging https://github.com/sigp/lighthouse/pull/7352 first, will port the remaining diff from here later

dapplion avatar May 16 '25 20:05 dapplion

This is quite outdated and a lot of the code touched in this PR has changed. Closing this and we'll address this separately. Thanks for this @SunnysidedJ

jimmygchen avatar Sep 10 '25 12:09 jimmygchen