From 21bab0899a5fc2201abfbc0be30e17bb81472cf6 Mon Sep 17 00:00:00 2001 From: Michael Sproul Date: Wed, 22 Oct 2025 00:58:12 +1100 Subject: [PATCH] Improve block header signature handling (#8253) Closes: - https://github.com/sigp/lighthouse/issues/7650 Reject blob and data column sidecars from RPC with invalid signatures. Co-Authored-By: Michael Sproul --- beacon_node/beacon_chain/src/beacon_chain.rs | 68 ++++++---- beacon_node/beacon_chain/src/test_utils.rs | 2 +- .../beacon_chain/tests/blob_verification.rs | 120 ++++++++++++++++++ .../beacon_chain/tests/column_verification.rs | 117 +++++++++++++++++ beacon_node/beacon_chain/tests/events.rs | 53 ++++---- beacon_node/beacon_chain/tests/main.rs | 2 + 6 files changed, 307 insertions(+), 55 deletions(-) create mode 100644 beacon_node/beacon_chain/tests/blob_verification.rs create mode 100644 beacon_node/beacon_chain/tests/column_verification.rs diff --git a/beacon_node/beacon_chain/src/beacon_chain.rs b/beacon_node/beacon_chain/src/beacon_chain.rs index e8db154a9b..ab157163f9 100644 --- a/beacon_node/beacon_chain/src/beacon_chain.rs +++ b/beacon_node/beacon_chain/src/beacon_chain.rs @@ -3564,7 +3564,7 @@ impl BeaconChain { .await } - fn check_blobs_for_slashability<'a>( + fn check_blob_header_signature_and_slashability<'a>( self: &Arc, block_root: Hash256, blobs: impl IntoIterator>, @@ -3575,17 +3575,20 @@ impl BeaconChain { .map(|b| b.signed_block_header.clone()) .unique() { - if verify_header_signature::(self, &header).is_ok() { - slashable_cache - .observe_slashable( - header.message.slot, - header.message.proposer_index, - block_root, - ) - .map_err(|e| BlockError::BeaconChainError(Box::new(e.into())))?; - if let Some(slasher) = self.slasher.as_ref() { - slasher.accept_block_header(header); - } + // Return an error if *any* header signature is invalid, we do not want to import this + // list of blobs into the DA checker. However, we will process any valid headers prior + // to the first invalid header in the slashable cache & slasher. + verify_header_signature::(self, &header)?; + + slashable_cache + .observe_slashable( + header.message.slot, + header.message.proposer_index, + block_root, + ) + .map_err(|e| BlockError::BeaconChainError(Box::new(e.into())))?; + if let Some(slasher) = self.slasher.as_ref() { + slasher.accept_block_header(header); } } Ok(()) @@ -3599,7 +3602,10 @@ impl BeaconChain { block_root: Hash256, blobs: FixedBlobSidecarList, ) -> Result { - self.check_blobs_for_slashability(block_root, blobs.iter().flatten().map(Arc::as_ref))?; + self.check_blob_header_signature_and_slashability( + block_root, + blobs.iter().flatten().map(Arc::as_ref), + )?; let availability = self .data_availability_checker .put_rpc_blobs(block_root, blobs)?; @@ -3616,12 +3622,15 @@ impl BeaconChain { ) -> Result { let availability = match engine_get_blobs_output { EngineGetBlobsOutput::Blobs(blobs) => { - self.check_blobs_for_slashability(block_root, blobs.iter().map(|b| b.as_blob()))?; + self.check_blob_header_signature_and_slashability( + block_root, + blobs.iter().map(|b| b.as_blob()), + )?; self.data_availability_checker .put_kzg_verified_blobs(block_root, blobs)? } EngineGetBlobsOutput::CustodyColumns(data_columns) => { - self.check_columns_for_slashability( + self.check_data_column_sidecar_header_signature_and_slashability( block_root, data_columns.iter().map(|c| c.as_data_column()), )?; @@ -3642,7 +3651,7 @@ impl BeaconChain { block_root: Hash256, custody_columns: DataColumnSidecarList, ) -> Result { - self.check_columns_for_slashability( + self.check_data_column_sidecar_header_signature_and_slashability( block_root, custody_columns.iter().map(|c| c.as_ref()), )?; @@ -3659,7 +3668,7 @@ impl BeaconChain { .await } - fn check_columns_for_slashability<'a>( + fn check_data_column_sidecar_header_signature_and_slashability<'a>( self: &Arc, block_root: Hash256, custody_columns: impl IntoIterator>, @@ -3673,17 +3682,20 @@ impl BeaconChain { .map(|c| c.signed_block_header.clone()) .unique() { - if verify_header_signature::(self, &header).is_ok() { - slashable_cache - .observe_slashable( - header.message.slot, - header.message.proposer_index, - block_root, - ) - .map_err(|e| BlockError::BeaconChainError(Box::new(e.into())))?; - if let Some(slasher) = self.slasher.as_ref() { - slasher.accept_block_header(header); - } + // Return an error if *any* header signature is invalid, we do not want to import this + // list of blobs into the DA checker. However, we will process any valid headers prior + // to the first invalid header in the slashable cache & slasher. + verify_header_signature::(self, &header)?; + + slashable_cache + .observe_slashable( + header.message.slot, + header.message.proposer_index, + block_root, + ) + .map_err(|e| BlockError::BeaconChainError(Box::new(e.into())))?; + if let Some(slasher) = self.slasher.as_ref() { + slasher.accept_block_header(header); } } Ok(()) diff --git a/beacon_node/beacon_chain/src/test_utils.rs b/beacon_node/beacon_chain/src/test_utils.rs index 1d57550156..0b125efa32 100644 --- a/beacon_node/beacon_chain/src/test_utils.rs +++ b/beacon_node/beacon_chain/src/test_utils.rs @@ -2437,7 +2437,7 @@ where } /// Builds an `RpcBlock` from a `SignedBeaconBlock` and `BlobsList`. - fn build_rpc_block_from_blobs( + pub fn build_rpc_block_from_blobs( &self, block_root: Hash256, block: Arc>>, diff --git a/beacon_node/beacon_chain/tests/blob_verification.rs b/beacon_node/beacon_chain/tests/blob_verification.rs new file mode 100644 index 0000000000..c42a2828c0 --- /dev/null +++ b/beacon_node/beacon_chain/tests/blob_verification.rs @@ -0,0 +1,120 @@ +#![cfg(not(debug_assertions))] + +use beacon_chain::test_utils::{ + AttestationStrategy, BeaconChainHarness, BlockStrategy, EphemeralHarnessType, test_spec, +}; +use beacon_chain::{ + AvailabilityProcessingStatus, BlockError, ChainConfig, InvalidSignature, NotifyExecutionLayer, + block_verification_types::AsBlock, +}; +use logging::create_test_tracing_subscriber; +use std::sync::{Arc, LazyLock}; +use types::{blob_sidecar::FixedBlobSidecarList, *}; + +type E = MainnetEthSpec; + +// Should ideally be divisible by 3. +const VALIDATOR_COUNT: usize = 24; + +/// A cached set of keys. +static KEYPAIRS: LazyLock> = + LazyLock::new(|| types::test_utils::generate_deterministic_keypairs(VALIDATOR_COUNT)); + +fn get_harness( + validator_count: usize, + spec: Arc, +) -> BeaconChainHarness> { + create_test_tracing_subscriber(); + let harness = BeaconChainHarness::builder(MainnetEthSpec) + .spec(spec) + .chain_config(ChainConfig { + reconstruct_historic_states: true, + ..ChainConfig::default() + }) + .keypairs(KEYPAIRS[0..validator_count].to_vec()) + .fresh_ephemeral_store() + .mock_execution_layer() + .build(); + + harness.advance_slot(); + + harness +} + +// Regression test for https://github.com/sigp/lighthouse/issues/7650 +#[tokio::test] +async fn rpc_blobs_with_invalid_header_signature() { + let spec = Arc::new(test_spec::()); + + // Only run this test if blobs are enabled and columns are disabled. + if spec.deneb_fork_epoch.is_none() || spec.is_fulu_scheduled() { + return; + } + + let harness = get_harness(VALIDATOR_COUNT, spec); + + let num_blocks = E::slots_per_epoch() as usize; + + // Add some chain depth. + harness + .extend_chain( + num_blocks, + BlockStrategy::OnCanonicalHead, + AttestationStrategy::AllValidators, + ) + .await; + + // Produce a block with blobs. + harness.execution_block_generator().set_min_blob_count(1); + let head_state = harness.get_current_state(); + let slot = head_state.slot() + 1; + let ((signed_block, opt_blobs), _) = harness.make_block(head_state, slot).await; + let (kzg_proofs, blobs) = opt_blobs.unwrap(); + assert!(!blobs.is_empty()); + let block_root = signed_block.canonical_root(); + + // Process the block without blobs so that it doesn't become available. + harness.advance_slot(); + let rpc_block = harness + .build_rpc_block_from_blobs(block_root, signed_block.clone(), None) + .unwrap(); + let availability = harness + .chain + .process_block( + block_root, + rpc_block, + NotifyExecutionLayer::Yes, + BlockImportSource::RangeSync, + || Ok(()), + ) + .await + .unwrap(); + assert_eq!( + availability, + AvailabilityProcessingStatus::MissingComponents(slot, block_root) + ); + + // Build blob sidecars with invalid signatures in the block header. + let mut corrupt_block = (*signed_block).clone(); + *corrupt_block.signature_mut() = Signature::infinity().unwrap(); + + let max_len = harness + .chain + .spec + .max_blobs_per_block(slot.epoch(E::slots_per_epoch())) as usize; + let mut blob_sidecars = FixedBlobSidecarList::new(vec![None; max_len]); + for (i, (kzg_proof, blob)) in kzg_proofs.into_iter().zip(blobs).enumerate() { + let blob_sidecar = BlobSidecar::new(i, blob, &corrupt_block, kzg_proof).unwrap(); + blob_sidecars[i] = Some(Arc::new(blob_sidecar)); + } + + let err = harness + .chain + .process_rpc_blobs(slot, block_root, blob_sidecars) + .await + .unwrap_err(); + assert!(matches!( + err, + BlockError::InvalidSignature(InvalidSignature::ProposerSignature) + )); +} diff --git a/beacon_node/beacon_chain/tests/column_verification.rs b/beacon_node/beacon_chain/tests/column_verification.rs new file mode 100644 index 0000000000..5cd3811ea5 --- /dev/null +++ b/beacon_node/beacon_chain/tests/column_verification.rs @@ -0,0 +1,117 @@ +#![cfg(not(debug_assertions))] + +use beacon_chain::test_utils::{ + AttestationStrategy, BeaconChainHarness, BlockStrategy, EphemeralHarnessType, + generate_data_column_sidecars_from_block, test_spec, +}; +use beacon_chain::{ + AvailabilityProcessingStatus, BlockError, ChainConfig, InvalidSignature, NotifyExecutionLayer, + block_verification_types::AsBlock, +}; +use logging::create_test_tracing_subscriber; +use std::sync::{Arc, LazyLock}; +use types::*; + +type E = MainnetEthSpec; + +// Should ideally be divisible by 3. +const VALIDATOR_COUNT: usize = 24; + +/// A cached set of keys. +static KEYPAIRS: LazyLock> = + LazyLock::new(|| types::test_utils::generate_deterministic_keypairs(VALIDATOR_COUNT)); + +fn get_harness( + validator_count: usize, + spec: Arc, + supernode: bool, +) -> BeaconChainHarness> { + create_test_tracing_subscriber(); + let harness = BeaconChainHarness::builder(MainnetEthSpec) + .spec(spec) + .chain_config(ChainConfig { + reconstruct_historic_states: true, + ..ChainConfig::default() + }) + .keypairs(KEYPAIRS[0..validator_count].to_vec()) + .import_all_data_columns(supernode) + .fresh_ephemeral_store() + .mock_execution_layer() + .build(); + + harness.advance_slot(); + + harness +} + +// Regression test for https://github.com/sigp/lighthouse/issues/7650 +#[tokio::test] +async fn rpc_columns_with_invalid_header_signature() { + let spec = Arc::new(test_spec::()); + + // Only run this test if columns are enabled. + if !spec.is_fulu_scheduled() { + return; + } + + let supernode = true; + let harness = get_harness(VALIDATOR_COUNT, spec, supernode); + + let num_blocks = E::slots_per_epoch() as usize; + + // Add some chain depth. + harness + .extend_chain( + num_blocks, + BlockStrategy::OnCanonicalHead, + AttestationStrategy::AllValidators, + ) + .await; + + // Produce a block with blobs. + harness.execution_block_generator().set_min_blob_count(1); + let head_state = harness.get_current_state(); + let slot = head_state.slot() + 1; + let ((signed_block, opt_blobs), _) = harness.make_block(head_state, slot).await; + let (_, blobs) = opt_blobs.unwrap(); + assert!(!blobs.is_empty()); + let block_root = signed_block.canonical_root(); + + // Process the block without blobs so that it doesn't become available. + harness.advance_slot(); + let rpc_block = harness + .build_rpc_block_from_blobs(block_root, signed_block.clone(), None) + .unwrap(); + let availability = harness + .chain + .process_block( + block_root, + rpc_block, + NotifyExecutionLayer::Yes, + BlockImportSource::RangeSync, + || Ok(()), + ) + .await + .unwrap(); + assert_eq!( + availability, + AvailabilityProcessingStatus::MissingComponents(slot, block_root) + ); + + // Build blob sidecars with invalid signatures in the block header. + let mut corrupt_block = (*signed_block).clone(); + *corrupt_block.signature_mut() = Signature::infinity().unwrap(); + + let data_column_sidecars = + generate_data_column_sidecars_from_block(&corrupt_block, &harness.chain.spec); + + let err = harness + .chain + .process_rpc_custody_columns(data_column_sidecars) + .await + .unwrap_err(); + assert!(matches!( + err, + BlockError::InvalidSignature(InvalidSignature::ProposerSignature) + )); +} diff --git a/beacon_node/beacon_chain/tests/events.rs b/beacon_node/beacon_chain/tests/events.rs index 0fc097ae8f..466058eea3 100644 --- a/beacon_node/beacon_chain/tests/events.rs +++ b/beacon_node/beacon_chain/tests/events.rs @@ -1,15 +1,13 @@ use beacon_chain::blob_verification::GossipVerifiedBlob; use beacon_chain::data_column_verification::GossipVerifiedDataColumn; -use beacon_chain::test_utils::{BeaconChainHarness, TEST_DATA_COLUMN_SIDECARS_SSZ}; +use beacon_chain::test_utils::{BeaconChainHarness, generate_data_column_sidecars_from_block}; use eth2::types::{EventKind, SseBlobSidecar, SseDataColumnSidecar}; use rand::SeedableRng; use rand::rngs::StdRng; use std::sync::Arc; use types::blob_sidecar::FixedBlobSidecarList; use types::test_utils::TestRandom; -use types::{ - BlobSidecar, DataColumnSidecar, EthSpec, ForkName, MinimalEthSpec, RuntimeVariableList, Slot, -}; +use types::{BlobSidecar, DataColumnSidecar, EthSpec, ForkName, MinimalEthSpec, Slot}; type E = MinimalEthSpec; @@ -108,19 +106,18 @@ async fn blob_sidecar_event_on_process_rpc_blobs() { let mut blob_event_receiver = event_handler.subscribe_blob_sidecar(); // build and process multiple rpc blobs - let kzg = harness.chain.kzg.as_ref(); - let mut rng = StdRng::seed_from_u64(0xDEADBEEF0BAD5EEDu64); + harness.execution_block_generator().set_min_blob_count(2); - let mut blob_1 = BlobSidecar::random_valid(&mut rng, kzg).unwrap(); - let mut blob_2 = BlobSidecar { - index: 1, - ..BlobSidecar::random_valid(&mut rng, kzg).unwrap() - }; - let parent_root = harness.chain.head().head_block_root(); - blob_1.signed_block_header.message.parent_root = parent_root; - blob_2.signed_block_header.message.parent_root = parent_root; - let blob_1 = Arc::new(blob_1); - let blob_2 = Arc::new(blob_2); + let head_state = harness.get_current_state(); + let slot = head_state.slot() + 1; + let ((signed_block, opt_blobs), _) = harness.make_block(head_state, slot).await; + let (kzg_proofs, blobs) = opt_blobs.unwrap(); + assert!(blobs.len() > 2); + + let blob_1 = + Arc::new(BlobSidecar::new(0, blobs[0].clone(), &signed_block, kzg_proofs[0]).unwrap()); + let blob_2 = + Arc::new(BlobSidecar::new(1, blobs[1].clone(), &signed_block, kzg_proofs[1]).unwrap()); let blobs = FixedBlobSidecarList::new(vec![Some(blob_1.clone()), Some(blob_2.clone())]); let expected_sse_blobs = vec![ @@ -130,7 +127,7 @@ async fn blob_sidecar_event_on_process_rpc_blobs() { let _ = harness .chain - .process_rpc_blobs(blob_1.slot(), blob_1.block_root(), blobs) + .process_rpc_blobs(slot, blob_1.block_root(), blobs) .await .unwrap(); @@ -159,20 +156,24 @@ async fn data_column_sidecar_event_on_process_rpc_columns() { let event_handler = harness.chain.event_handler.as_ref().unwrap(); let mut data_column_event_receiver = event_handler.subscribe_data_column_sidecar(); + // build a valid block + harness.execution_block_generator().set_min_blob_count(1); + + let head_state = harness.get_current_state(); + let slot = head_state.slot() + 1; + let ((signed_block, opt_blobs), _) = harness.make_block(head_state, slot).await; + let (_, blobs) = opt_blobs.unwrap(); + assert!(!blobs.is_empty()); + // load the precomputed column sidecar to avoid computing them for every block in the tests. - let mut sidecar = RuntimeVariableList::>::from_ssz_bytes( - TEST_DATA_COLUMN_SIDECARS_SSZ, - E::number_of_columns(), - ) - .unwrap()[0] - .clone(); - let parent_root = harness.chain.head().head_block_root(); - sidecar.signed_block_header.message.parent_root = parent_root; + let data_column_sidecars = + generate_data_column_sidecars_from_block(&signed_block, &harness.chain.spec); + let sidecar = data_column_sidecars[0].clone(); let expected_sse_data_column = SseDataColumnSidecar::from_data_column_sidecar(&sidecar); let _ = harness .chain - .process_rpc_custody_columns(vec![Arc::new(sidecar)]) + .process_rpc_custody_columns(vec![sidecar]) .await .unwrap(); diff --git a/beacon_node/beacon_chain/tests/main.rs b/beacon_node/beacon_chain/tests/main.rs index f0978c5f05..aec4416419 100644 --- a/beacon_node/beacon_chain/tests/main.rs +++ b/beacon_node/beacon_chain/tests/main.rs @@ -1,8 +1,10 @@ mod attestation_production; mod attestation_verification; mod bellatrix; +mod blob_verification; mod block_verification; mod capella; +mod column_verification; mod events; mod op_verification; mod payload_invalidation;