[Epoch Sync] EpochSyncProof validation #12020

staffik · 2024-08-29T16:52:02Z

Issue: #11932

Implements EpochSyncProof validation, according to:

Summary:

Move block merkle proof retrieval logic from Chain to a separate trait (MerkleProofAccess). Make Store implements this trait so that we can get_block_proof while serving EpochSyncRequest from a separate thread and having no access to Chain.
Refactor verify_approval and compute_bp_hash so that we can use it for proof validation.
Derive first_block_info_in_epoch from block header.
Implement the validation logic.
Add 2 more fields to the proof
- EpochSyncProofPastEpochData::protocol_version
- merkle_proof_for_first_block generated at height of final block of current epoch.
- Apparently, we need to add even more to enable generating merkle_proof_for_first_block at the bootstrapped node. That will be done as a follow-up issue: [Epoch Sync] Generate merkle proof for the first block of current epoch #12255.
Create a bunch of follow-up issues (added to the tracking issue):

staffik · 2024-09-23T13:38:54Z

chain/client/src/sync/epoch.rs

        )?;

        store_update.set_ser(
            DBCol::BlockOrdinal,
-            &borsh::to_vec(&proof.current_epoch.merkle_proof_for_first_block.size()).unwrap(),
+            &index_to_bytes(last_header.block_ordinal()),


Concern: BlockHeader::block_ordinal() is supported since BlockHeaderV3.

I don't think that's an issue. EpochSync to a block header version below V3 is unsupportable due to also not having epoch_data_hash.

But it will be worth making a test to check that trying to sync to a version below V3 will fail. I'll file an issue.

staffik · 2024-09-23T13:46:38Z

chain/client/src/sync/epoch.rs

+        let last_epoch_data = &proof.past_epochs[proof.past_epochs.len() - 2];
+        let last_epoch_last_final_block_height = last_epoch_data.last_final_block_header.height();
+        let first_block_info_in_epoch =
+            BlockInfo::from_header(&last_header, last_epoch_last_final_block_height);


Concern: there is also from_header_and_endorsements() method, but from_header would copy endorsements from the header. cc @tayfunelmas

staffik · 2024-09-23T15:51:08Z

Looking into failing integration tests.

robin-near · 2024-09-23T17:14:55Z

chain/chain/src/store/merkle_proof.rs

+    }
+}
+
+impl MerkleProofAccess for Store {


Very nice design to make it work for both ChainStore and Store!

chain/chain/src/store/merkle_proof.rs

robin-near · 2024-09-23T18:23:29Z

chain/client/src/sync/epoch.rs

        )?;

        store_update.set_ser(
            DBCol::BlockOrdinal,
-            &borsh::to_vec(&proof.current_epoch.merkle_proof_for_first_block.size()).unwrap(),
+            &index_to_bytes(last_header.block_ordinal()),


I don't think that's an issue. EpochSync to a block header version below V3 is unsupportable due to also not having epoch_data_hash.

robin-near · 2024-09-23T18:23:52Z

chain/client/src/sync/epoch.rs

        )?;

        store_update.set_ser(
            DBCol::BlockOrdinal,
-            &borsh::to_vec(&proof.current_epoch.merkle_proof_for_first_block.size()).unwrap(),
+            &index_to_bytes(last_header.block_ordinal()),


But it will be worth making a test to check that trying to sync to a version below V3 will fail. I'll file an issue.

chain/client/src/sync/epoch.rs

robin-near · 2024-09-23T18:34:10Z

chain/client/src/sync/epoch.rs

+        epoch_manager: &dyn EpochManagerAdapter,
+    ) -> Result<(), Error> {
+        // Verify epoch.block_producers
+        let bp_hash = Chain::compute_bp_hash_from_validator_stakes(


Nice refactoring for this function. I was worried earlier about the protocol version check in here, but this is good.

robin-near · 2024-09-23T18:36:19Z

chain/client/src/sync/epoch.rs

@@ -514,6 +508,148 @@ impl EpochSync {

        Ok(())
    }
+
+    // Verify EpochSyncProof
+    fn verify_proof(


Could we add some tests (can add some to the testloop test) that messing with the proof will result in a failing verification? We could, e.g. check that tweaking every byte of a valid serialized proof will cause a failure one way or another. It doesn't cover all possible attacks but at least makes sure that we aren't forgetting any fields.

Created follow-up issue for tests: #12260.
Might need fixing other issues first.

robin-near · 2024-09-23T18:36:33Z

Great implementation!

staffik · 2024-09-25T13:08:11Z

Addressed PR comments here: 76bdb69
Needed to add two more fields to the proof.
Also, there are 2 things marked as todo and this PR does not includes tests.

fix

codecov · 2024-10-18T08:35:00Z

Codecov Report

Attention: Patch coverage is 79.58115% with 78 lines in your changes missing coverage. Please review.

Project coverage is 71.62%. Comparing base (ba6c707) to head (2a35498).
Report is 2 commits behind head on master.

Files with missing lines	Patch %	Lines
chain/chain/src/store/merkle_proof.rs	75.46%	25 Missing and 15 partials ⚠️
chain/client/src/sync/epoch.rs	83.92%	17 Missing and 10 partials ⚠️
chain/chain/src/test_utils/kv_runtime.rs	0.00%	10 Missing ⚠️
chain/chain-primitives/src/error.rs	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #12020      +/-   ##
==========================================
- Coverage   71.62%   71.62%   -0.01%     
==========================================
  Files         837      838       +1     
  Lines      167105   167319     +214     
  Branches   167105   167319     +214     
==========================================
+ Hits       119696   119842     +146     
- Misses      42180    42242      +62     
- Partials     5229     5235       +6

Flag	Coverage Δ
backward-compatibility	`0.16% <0.00%> (-0.01%)`	⬇️
db-migration	`0.16% <0.00%> (-0.01%)`	⬇️
genesis-check	`1.25% <0.00%> (-0.01%)`	⬇️
integration-tests	`38.92% <79.58%> (+0.04%)`	⬆️
linux	`71.23% <79.58%> (-0.02%)`	⬇️
linux-nightly	`71.21% <79.58%> (+<0.01%)`	⬆️
macos	`54.24% <8.90%> (-0.07%)`	⬇️
pytests	`1.56% <0.00%> (-0.01%)`	⬇️
sanity-checks	`1.37% <0.00%> (-0.01%)`	⬇️
unittests	`65.35% <8.90%> (-0.09%)`	⬇️
upgradability	`0.21% <0.00%> (-0.01%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

staffik force-pushed the epoch-sync-v4-validation branch from 5113cf7 to f80fc66 Compare September 5, 2024 13:30

staffik force-pushed the epoch-sync-v4-validation branch from f80fc66 to 00c1851 Compare September 23, 2024 12:35

staffik marked this pull request as ready for review September 23, 2024 12:43

staffik requested a review from a team as a code owner September 23, 2024 12:43

staffik requested review from Longarithm and robin-near September 23, 2024 12:43

staffik commented Sep 23, 2024

View reviewed changes

robin-near reviewed Sep 23, 2024

View reviewed changes

robin-near approved these changes Oct 15, 2024

View reviewed changes

staffik added 7 commits October 18, 2024 10:05

Verify past epochs

585433f

Verify last epoch

6f73e09

Refactor: MerkleProofAccess

1da8616

Verify first block in current epoch

6e14b71

fix

1a660fb

fix

aa19137

fix

Leave merkle_proof_for_first_block ungenerated and unverified for now

9e2f79d

staffik force-pushed the epoch-sync-v4-validation branch from 76bdb69 to 9e2f79d Compare October 18, 2024 08:17

staffik mentioned this pull request Oct 18, 2024

[Epoch Sync] Generate merkle proof for the first block of current epoch #12255

Closed

This was referenced Oct 18, 2024

[Epoch Sync] Merkle tree size for a block mismatches with the block ordinal? #12256

Closed

[Epoch Sync] Missing epoch_sync_data_hash in final_block_header_in_next_epoch #12258

Closed

[Epoch Sync] Verify 2 last block headers in prev epoch #12259

Closed

Update TODO comments

2a35498

staffik mentioned this pull request Oct 18, 2024

[Epoch Sync] Add tests for epoch sync proof verification #12260

Open

staffik added this pull request to the merge queue Oct 18, 2024

Merged via the queue into master with commit 2c40e53 Oct 18, 2024
28 of 30 checks passed

staffik deleted the epoch-sync-v4-validation branch October 18, 2024 10:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Epoch Sync] EpochSyncProof validation #12020

[Epoch Sync] EpochSyncProof validation #12020

staffik commented Aug 29, 2024 •

edited

Loading

staffik Sep 23, 2024

robin-near Sep 23, 2024

robin-near Sep 23, 2024

staffik Sep 23, 2024

staffik commented Sep 23, 2024

robin-near Sep 23, 2024

robin-near Sep 23, 2024

robin-near Sep 23, 2024

robin-near Sep 23, 2024

robin-near Sep 23, 2024

staffik Oct 18, 2024

robin-near commented Sep 23, 2024

staffik commented Sep 25, 2024

codecov bot commented Oct 18, 2024 •

edited

Loading

[Epoch Sync] EpochSyncProof validation #12020

[Epoch Sync] EpochSyncProof validation #12020

Conversation

staffik commented Aug 29, 2024 • 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

staffik commented Sep 23, 2024

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

robin-near commented Sep 23, 2024

staffik commented Sep 25, 2024

codecov bot commented Oct 18, 2024 • edited Loading

Codecov Report

staffik commented Aug 29, 2024 •

edited

Loading

codecov bot commented Oct 18, 2024 •

edited

Loading