From ab265c1186b5929d17e34d91eaa05aeb4e16e413 Mon Sep 17 00:00:00 2001 From: Michael Sproul Date: Wed, 24 Jan 2024 11:57:03 +1100 Subject: [PATCH 1/2] Fix off-by-one in backfill sig verification --- .../src/data_availability_checker.rs | 12 +++++++++++ .../beacon_chain/src/historical_blocks.rs | 4 ++-- beacon_node/beacon_chain/tests/store_tests.rs | 20 +++++++++++++++++++ 3 files changed, 34 insertions(+), 2 deletions(-) diff --git a/beacon_node/beacon_chain/src/data_availability_checker.rs b/beacon_node/beacon_chain/src/data_availability_checker.rs index 21cac9a2649..48d505e9e7b 100644 --- a/beacon_node/beacon_chain/src/data_availability_checker.rs +++ b/beacon_node/beacon_chain/src/data_availability_checker.rs @@ -545,6 +545,18 @@ pub struct AvailableBlock { } impl AvailableBlock { + pub fn __new_for_testing( + block_root: Hash256, + block: Arc>, + blobs: Option>, + ) -> Self { + Self { + block_root, + block, + blobs, + } + } + pub fn block(&self) -> &SignedBeaconBlock { &self.block } diff --git a/beacon_node/beacon_chain/src/historical_blocks.rs b/beacon_node/beacon_chain/src/historical_blocks.rs index b5b42fcfcdf..85208c8ad6f 100644 --- a/beacon_node/beacon_chain/src/historical_blocks.rs +++ b/beacon_node/beacon_chain/src/historical_blocks.rs @@ -135,20 +135,20 @@ impl BeaconChain { prev_block_slot = block.slot(); expected_block_root = block.message().parent_root(); + signed_blocks.push(block); // If we've reached genesis, add the genesis block root to the batch for all slots // between 0 and the first block slot, and set the anchor slot to 0 to indicate // completion. if expected_block_root == self.genesis_block_root { let genesis_slot = self.spec.genesis_slot; - for slot in genesis_slot.as_usize()..block.slot().as_usize() { + for slot in genesis_slot.as_usize()..prev_block_slot.as_usize() { chunk_writer.set(slot, self.genesis_block_root, &mut cold_batch)?; } prev_block_slot = genesis_slot; expected_block_root = Hash256::zero(); break; } - signed_blocks.push(block); } chunk_writer.write(&mut cold_batch)?; // these were pushed in reverse order so we reverse again diff --git a/beacon_node/beacon_chain/tests/store_tests.rs b/beacon_node/beacon_chain/tests/store_tests.rs index 9b832bd7646..f2c4b5e92bd 100644 --- a/beacon_node/beacon_chain/tests/store_tests.rs +++ b/beacon_node/beacon_chain/tests/store_tests.rs @@ -3,6 +3,7 @@ use beacon_chain::attestation_verification::Error as AttnError; use beacon_chain::block_verification_types::RpcBlock; use beacon_chain::builder::BeaconChainBuilder; +use beacon_chain::data_availability_checker::AvailableBlock; use beacon_chain::schema_change::migrate_schema; use beacon_chain::test_utils::{ mock_execution_layer_from_parts, test_spec, AttestationStrategy, BeaconChainHarness, @@ -2547,6 +2548,25 @@ async fn weak_subjectivity_sync_test(slots: Vec, checkpoint_slot: Slot) { } } + // Corrupt the signature on the 1st block to ensure that the backfill processor is checking + // signatures correctly. This is a regression test for XXX. + let mut batch_with_invalid_first_block = available_blocks.clone(); + batch_with_invalid_first_block[0] = { + let (block_root, block, blobs) = available_blocks[0].clone().deconstruct(); + let mut corrupt_block = (*block).clone(); + *corrupt_block.signature_mut() = Signature::empty(); + AvailableBlock::__new_for_testing(block_root, Arc::new(corrupt_block), blobs) + }; + + // Importing the invalid batch should error. + assert!(matches!( + beacon_chain + .import_historical_block_batch(batch_with_invalid_first_block) + .unwrap_err(), + BeaconChainError::HistoricalBlockError(HistoricalBlockError::InvalidSignature) + )); + + // Importing the batch with valid signatures should succeed. beacon_chain .import_historical_block_batch(available_blocks.clone()) .unwrap(); From 0aa5ed2f7c4afe321591749fa3a12ba4baef903b Mon Sep 17 00:00:00 2001 From: Michael Sproul Date: Wed, 24 Jan 2024 12:41:01 +1100 Subject: [PATCH 2/2] Add self-referential PR link --- beacon_node/beacon_chain/tests/store_tests.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/beacon_node/beacon_chain/tests/store_tests.rs b/beacon_node/beacon_chain/tests/store_tests.rs index f2c4b5e92bd..ffd1b843a18 100644 --- a/beacon_node/beacon_chain/tests/store_tests.rs +++ b/beacon_node/beacon_chain/tests/store_tests.rs @@ -2549,7 +2549,7 @@ async fn weak_subjectivity_sync_test(slots: Vec, checkpoint_slot: Slot) { } // Corrupt the signature on the 1st block to ensure that the backfill processor is checking - // signatures correctly. This is a regression test for XXX. + // signatures correctly. Regression test for https://github.com/sigp/lighthouse/pull/5120. let mut batch_with_invalid_first_block = available_blocks.clone(); batch_with_invalid_first_block[0] = { let (block_root, block, blobs) = available_blocks[0].clone().deconstruct();