diff --git a/beacon_node/beacon_chain/src/block_verification.rs b/beacon_node/beacon_chain/src/block_verification.rs index 315105ac2b..1265276376 100644 --- a/beacon_node/beacon_chain/src/block_verification.rs +++ b/beacon_node/beacon_chain/src/block_verification.rs @@ -208,24 +208,18 @@ pub enum BlockError { /// /// The block is invalid and the peer is faulty. IncorrectBlockProposer { block: u64, local_shuffling: u64 }, - /// The proposal signature in invalid. - /// - /// ## Peer scoring - /// - /// The block is invalid and the peer is faulty. - ProposalSignatureInvalid, /// The `block.proposal_index` is not known. /// /// ## Peer scoring /// /// The block is invalid and the peer is faulty. UnknownValidator(u64), - /// A signature in the block is invalid (exactly which is unknown). + /// A signature in the block is invalid /// /// ## Peer scoring /// /// The block is invalid and the peer is faulty. - InvalidSignature, + InvalidSignature(InvalidSignature), /// The provided block is not from a later slot than its parent. /// /// ## Peer scoring @@ -329,6 +323,17 @@ pub enum BlockError { InternalError(String), } +/// Which specific signature(s) are invalid in a SignedBeaconBlock +#[derive(Debug)] +pub enum InvalidSignature { + // The outer signature in a SignedBeaconBlock + ProposerSignature, + // One or more signatures in BeaconBlockBody + BlockBodySignatures, + // One or more signatures in SignedBeaconBlock + Unknown, +} + impl From for BlockError { fn from(e: AvailabilityCheckError) -> Self { Self::AvailabilityCheck(e) @@ -523,7 +528,9 @@ pub enum BlockSlashInfo { impl BlockSlashInfo { pub fn from_early_error_block(header: SignedBeaconBlockHeader, e: BlockError) -> Self { match e { - BlockError::ProposalSignatureInvalid => BlockSlashInfo::SignatureInvalid(e), + BlockError::InvalidSignature(InvalidSignature::ProposerSignature) => { + BlockSlashInfo::SignatureInvalid(e) + } // `InvalidSignature` could indicate any signature in the block, so we want // to recheck the proposer signature alone. _ => BlockSlashInfo::SignatureNotChecked(header, e), @@ -652,7 +659,7 @@ pub fn signature_verify_chain_segment( } if signature_verifier.verify().is_err() { - return Err(BlockError::InvalidSignature); + return Err(BlockError::InvalidSignature(InvalidSignature::Unknown)); } drop(pubkey_cache); @@ -964,7 +971,9 @@ impl GossipVerifiedBlock { }; if !signature_is_valid { - return Err(BlockError::ProposalSignatureInvalid); + return Err(BlockError::InvalidSignature( + InvalidSignature::ProposerSignature, + )); } chain @@ -1098,7 +1107,26 @@ impl SignatureVerifiedBlock { parent: Some(parent), }) } else { - Err(BlockError::InvalidSignature) + // Re-verify the proposer signature in isolation to attribute fault + let pubkey = pubkey_cache + .get(block.message().proposer_index() as usize) + .ok_or_else(|| BlockError::UnknownValidator(block.message().proposer_index()))?; + if block.as_block().verify_signature( + Some(block_root), + pubkey, + &state.fork(), + chain.genesis_validators_root, + &chain.spec, + ) { + // Proposer signature is valid, the invalid signature must be in the body + Err(BlockError::InvalidSignature( + InvalidSignature::BlockBodySignatures, + )) + } else { + Err(BlockError::InvalidSignature( + InvalidSignature::ProposerSignature, + )) + } } } @@ -1153,7 +1181,9 @@ impl SignatureVerifiedBlock { consensus_context, }) } else { - Err(BlockError::InvalidSignature) + Err(BlockError::InvalidSignature( + InvalidSignature::BlockBodySignatures, + )) } } @@ -1981,7 +2011,7 @@ impl BlockBlobError for BlockError { } fn proposer_signature_invalid() -> Self { - BlockError::ProposalSignatureInvalid + BlockError::InvalidSignature(InvalidSignature::ProposerSignature) } } diff --git a/beacon_node/beacon_chain/src/lib.rs b/beacon_node/beacon_chain/src/lib.rs index 4783945eb1..456b3c0dd8 100644 --- a/beacon_node/beacon_chain/src/lib.rs +++ b/beacon_node/beacon_chain/src/lib.rs @@ -78,7 +78,7 @@ pub use beacon_fork_choice_store::{BeaconForkChoiceStore, Error as ForkChoiceSto pub use block_verification::{ build_blob_data_column_sidecars, get_block_root, BlockError, ExecutionPayloadError, ExecutionPendingBlock, GossipVerifiedBlock, IntoExecutionPendingBlock, IntoGossipVerifiedBlock, - PayloadVerificationOutcome, PayloadVerificationStatus, + InvalidSignature, PayloadVerificationOutcome, PayloadVerificationStatus, }; pub use block_verification_types::AvailabilityPendingExecutedBlock; pub use block_verification_types::ExecutedBlock; diff --git a/beacon_node/beacon_chain/tests/block_verification.rs b/beacon_node/beacon_chain/tests/block_verification.rs index 1a651332ad..46f5befbba 100644 --- a/beacon_node/beacon_chain/tests/block_verification.rs +++ b/beacon_node/beacon_chain/tests/block_verification.rs @@ -9,7 +9,7 @@ use beacon_chain::{ }; use beacon_chain::{ BeaconSnapshot, BlockError, ChainConfig, ChainSegmentResult, IntoExecutionPendingBlock, - NotifyExecutionLayer, + InvalidSignature, NotifyExecutionLayer, }; use logging::test_logger; use slasher::{Config as SlasherConfig, Slasher}; @@ -438,7 +438,7 @@ async fn assert_invalid_signature( .process_chain_segment(blocks, NotifyExecutionLayer::Yes) .await .into_block_error(), - Err(BlockError::InvalidSignature) + Err(BlockError::InvalidSignature(InvalidSignature::Unknown)) ), "should not import chain segment with an invalid {} signature", item @@ -480,7 +480,12 @@ async fn assert_invalid_signature( ) .await; assert!( - matches!(process_res, Err(BlockError::InvalidSignature)), + matches!( + process_res, + Err(BlockError::InvalidSignature( + InvalidSignature::BlockBodySignatures + )) + ), "should not import individual block with an invalid {} signature, got: {:?}", item, process_res @@ -536,21 +541,25 @@ async fn invalid_signature_gossip_block() { .into_block_error() .expect("should import all blocks prior to the one being tested"); let signed_block = SignedBeaconBlock::from_block(block, junk_signature()); + let process_res = harness + .chain + .process_block( + signed_block.canonical_root(), + Arc::new(signed_block), + NotifyExecutionLayer::Yes, + BlockImportSource::Lookup, + || Ok(()), + ) + .await; assert!( matches!( - harness - .chain - .process_block( - signed_block.canonical_root(), - Arc::new(signed_block), - NotifyExecutionLayer::Yes, - BlockImportSource::Lookup, - || Ok(()), - ) - .await, - Err(BlockError::InvalidSignature) + process_res, + Err(BlockError::InvalidSignature( + InvalidSignature::ProposerSignature + )) ), - "should not import individual block with an invalid gossip signature", + "should not import individual block with an invalid gossip signature, got: {:?}", + process_res ); } } @@ -578,16 +587,18 @@ async fn invalid_signature_block_proposal() { }) .collect::>(); // Ensure the block will be rejected if imported in a chain segment. + let process_res = harness + .chain + .process_chain_segment(blocks, NotifyExecutionLayer::Yes) + .await + .into_block_error(); assert!( matches!( - harness - .chain - .process_chain_segment(blocks, NotifyExecutionLayer::Yes) - .await - .into_block_error(), - Err(BlockError::InvalidSignature) + process_res, + Err(BlockError::InvalidSignature(InvalidSignature::Unknown)) ), - "should not import chain segment with an invalid block signature", + "should not import chain segment with an invalid block signature, got: {:?}", + process_res ); } } @@ -890,7 +901,7 @@ async fn invalid_signature_deposit() { .process_chain_segment(blocks, NotifyExecutionLayer::Yes) .await .into_block_error(), - Err(BlockError::InvalidSignature) + Err(BlockError::InvalidSignature(InvalidSignature::Unknown)) ), "should not throw an invalid signature error for a bad deposit signature" ); @@ -1086,7 +1097,7 @@ async fn block_gossip_verification() { ))) .await ), - BlockError::ProposalSignatureInvalid + BlockError::InvalidSignature(InvalidSignature::ProposerSignature) ), "should not import a block with an invalid proposal signature" ); diff --git a/beacon_node/network/src/network_beacon_processor/gossip_methods.rs b/beacon_node/network/src/network_beacon_processor/gossip_methods.rs index 6b5753e96a..dc8d32800e 100644 --- a/beacon_node/network/src/network_beacon_processor/gossip_methods.rs +++ b/beacon_node/network/src/network_beacon_processor/gossip_methods.rs @@ -1290,13 +1290,12 @@ impl NetworkBeaconProcessor { Err(e @ BlockError::StateRootMismatch { .. }) | Err(e @ BlockError::IncorrectBlockProposer { .. }) | Err(e @ BlockError::BlockSlotLimitReached) - | Err(e @ BlockError::ProposalSignatureInvalid) | Err(e @ BlockError::NonLinearSlots) | Err(e @ BlockError::UnknownValidator(_)) | Err(e @ BlockError::PerBlockProcessingError(_)) | Err(e @ BlockError::NonLinearParentRoots) | Err(e @ BlockError::BlockIsNotLaterThanParent { .. }) - | Err(e @ BlockError::InvalidSignature) + | Err(e @ BlockError::InvalidSignature(_)) | Err(e @ BlockError::WeakSubjectivityConflict) | Err(e @ BlockError::InconsistentFork(_)) | Err(e @ BlockError::ExecutionPayloadError(_)) diff --git a/beacon_node/network/src/sync/tests/lookups.rs b/beacon_node/network/src/sync/tests/lookups.rs index f623aa2c12..f772010500 100644 --- a/beacon_node/network/src/sync/tests/lookups.rs +++ b/beacon_node/network/src/sync/tests/lookups.rs @@ -1677,7 +1677,7 @@ fn test_parent_lookup_too_many_processing_attempts_must_blacklist() { rig.assert_not_failed_chain(block_root); // send the right parent but fail processing rig.parent_lookup_block_response(id, peer_id, Some(parent.clone().into())); - rig.parent_block_processed(block_root, BlockError::InvalidSignature.into()); + rig.parent_block_processed(block_root, BlockError::BlockSlotLimitReached.into()); rig.parent_lookup_block_response(id, peer_id, None); rig.expect_penalty(peer_id, "lookup_block_processing_failure"); } @@ -2575,7 +2575,7 @@ mod deneb_only { fn invalid_parent_processed(mut self) -> Self { self.rig.parent_block_processed( self.block_root, - BlockProcessingResult::Err(BlockError::ProposalSignatureInvalid), + BlockProcessingResult::Err(BlockError::BlockSlotLimitReached), ); assert_eq!(self.rig.active_parent_lookups_count(), 1); self @@ -2584,7 +2584,7 @@ mod deneb_only { fn invalid_block_processed(mut self) -> Self { self.rig.single_block_component_processed( self.block_req_id.expect("block request id").lookup_id, - BlockProcessingResult::Err(BlockError::ProposalSignatureInvalid), + BlockProcessingResult::Err(BlockError::BlockSlotLimitReached), ); self.rig.assert_single_lookups_count(1); self