Detect invalid proposer signature on RPC block processing (#6519)

dapplion · web-flow · commit c6ebaba89270 · 2025-01-28T19:01:26.000Z
Complements - #6321 by detecting if the proposer signature is valid or not during RPC block processing. In lookup sync, if the invalid signature signature is the proposer signature, it's not deterministic on the block root. So we should only penalize the sending peer and retry. Otherwise, if it's on the body we should drop the lookup and penalize all peers that claim to have imported the block
diff --git 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<AvailabilityCheckError> for BlockError {
     fn from(e: AvailabilityCheckError) -> Self {
         Self::AvailabilityCheck(e)
@@ -523,7 +528,9 @@ pub enum BlockSlashInfo<TErr> {
 impl BlockSlashInfo<BlockError> {
     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<T: BeaconChainTypes>(
     }
 
     if signature_verifier.verify().is_err() {
-        return Err(BlockError::InvalidSignature);
+        return Err(BlockError::InvalidSignature(InvalidSignature::Unknown));
     }
 
     drop(pubkey_cache);
@@ -964,7 +971,9 @@ impl<T: BeaconChainTypes> GossipVerifiedBlock<T> {
         };
 
         if !signature_is_valid {
-            return Err(BlockError::ProposalSignatureInvalid);
+            return Err(BlockError::InvalidSignature(
+                InvalidSignature::ProposerSignature,
+            ));
         }
 
         chain
@@ -1098,7 +1107,26 @@ impl<T: BeaconChainTypes> SignatureVerifiedBlock<T> {
                 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<T: BeaconChainTypes> SignatureVerifiedBlock<T> {
                 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
@@ -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
@@ -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::<Vec<_>>();
         // 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
@@ -1290,13 +1290,12 @@ impl<T: BeaconChainTypes> NetworkBeaconProcessor<T> {
             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
@@ -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
