Merge branch 'unstable' into tracing-integration

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);
@@ -963,7 +970,9 @@ impl<T: BeaconChainTypes> GossipVerifiedBlock<T> {
         };
 
         if !signature_is_valid {
-            return Err(BlockError::ProposalSignatureInvalid);
+            return Err(BlockError::InvalidSignature(
+                InvalidSignature::ProposerSignature,
+            ));
         }
 
         chain
@@ -1097,7 +1106,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,
+                ))
+            }
         }
     }
 
@@ -1152,7 +1180,9 @@ impl<T: BeaconChainTypes> SignatureVerifiedBlock<T> {
                 consensus_context,
             })
         } else {
-            Err(BlockError::InvalidSignature)
+            Err(BlockError::InvalidSignature(
+                InvalidSignature::BlockBodySignatures,
+            ))
         }
     }
 
@@ -1968,7 +1998,7 @@ impl BlockBlobError for BlockError {
     }
 
     fn proposer_signature_invalid() -> Self {
-        BlockError::ProposalSignatureInvalid
+        BlockError::InvalidSignature(InvalidSignature::ProposerSignature)
     }
 }
 

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;

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::create_test_tracing_subscriber;
 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"
     );