Skip to content

Commit 3872887

Browse files
mergify[bot]mergify[bot]
authored
Merge of #6519 
2 parents 33b8555 + be8b3e1 commit 3872887

File tree

5 files changed

+84
-44
lines changed

5 files changed

+84
-44
lines changed

‎beacon_node/beacon_chain/src/block_verification.rs

Lines changed: 44 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -208,24 +208,18 @@ pub enum BlockError {
208208
///
209209
/// The block is invalid and the peer is faulty.
210210
IncorrectBlockProposer { block: u64, local_shuffling: u64 },
211-
/// The proposal signature in invalid.
212-
///
213-
/// ## Peer scoring
214-
///
215-
/// The block is invalid and the peer is faulty.
216-
ProposalSignatureInvalid,
217211
/// The `block.proposal_index` is not known.
218212
///
219213
/// ## Peer scoring
220214
///
221215
/// The block is invalid and the peer is faulty.
222216
UnknownValidator(u64),
223-
/// A signature in the block is invalid (exactly which is unknown).
217+
/// A signature in the block is invalid
224218
///
225219
/// ## Peer scoring
226220
///
227221
/// The block is invalid and the peer is faulty.
228-
InvalidSignature,
222+
InvalidSignature(InvalidSignature),
229223
/// The provided block is not from a later slot than its parent.
230224
///
231225
/// ## Peer scoring
@@ -329,6 +323,17 @@ pub enum BlockError {
329323
InternalError(String),
330324
}
331325

326+
/// Which specific signature(s) are invalid in a SignedBeaconBlock
327+
#[derive(Debug)]
328+
pub enum InvalidSignature {
329+
// The outer signature in a SignedBeaconBlock
330+
ProposerSignature,
331+
// One or more signatures in BeaconBlockBody
332+
BlockBodySignatures,
333+
// One or more signatures in SignedBeaconBlock
334+
Unknown,
335+
}
336+
332337
impl From<AvailabilityCheckError> for BlockError {
333338
fn from(e: AvailabilityCheckError) -> Self {
334339
Self::AvailabilityCheck(e)
@@ -523,7 +528,9 @@ pub enum BlockSlashInfo<TErr> {
523528
impl BlockSlashInfo<BlockError> {
524529
pub fn from_early_error_block(header: SignedBeaconBlockHeader, e: BlockError) -> Self {
525530
match e {
526-
BlockError::ProposalSignatureInvalid => BlockSlashInfo::SignatureInvalid(e),
531+
BlockError::InvalidSignature(InvalidSignature::ProposerSignature) => {
532+
BlockSlashInfo::SignatureInvalid(e)
533+
}
527534
// `InvalidSignature` could indicate any signature in the block, so we want
528535
// to recheck the proposer signature alone.
529536
_ => BlockSlashInfo::SignatureNotChecked(header, e),
@@ -652,7 +659,7 @@ pub fn signature_verify_chain_segment<T: BeaconChainTypes>(
652659
}
653660

654661
if signature_verifier.verify().is_err() {
655-
return Err(BlockError::InvalidSignature);
662+
return Err(BlockError::InvalidSignature(InvalidSignature::Unknown));
656663
}
657664

658665
drop(pubkey_cache);
@@ -964,7 +971,9 @@ impl<T: BeaconChainTypes> GossipVerifiedBlock<T> {
964971
};
965972

966973
if !signature_is_valid {
967-
return Err(BlockError::ProposalSignatureInvalid);
974+
return Err(BlockError::InvalidSignature(
975+
InvalidSignature::ProposerSignature,
976+
));
968977
}
969978

970979
chain
@@ -1098,7 +1107,26 @@ impl<T: BeaconChainTypes> SignatureVerifiedBlock<T> {
10981107
parent: Some(parent),
10991108
})
11001109
} else {
1101-
Err(BlockError::InvalidSignature)
1110+
// Re-verify the proposer signature in isolation to attribute fault
1111+
let pubkey = pubkey_cache
1112+
.get(block.message().proposer_index() as usize)
1113+
.ok_or_else(|| BlockError::UnknownValidator(block.message().proposer_index()))?;
1114+
if block.as_block().verify_signature(
1115+
Some(block_root),
1116+
pubkey,
1117+
&state.fork(),
1118+
chain.genesis_validators_root,
1119+
&chain.spec,
1120+
) {
1121+
// Proposer signature is valid, the invalid signature must be in the body
1122+
Err(BlockError::InvalidSignature(
1123+
InvalidSignature::BlockBodySignatures,
1124+
))
1125+
} else {
1126+
Err(BlockError::InvalidSignature(
1127+
InvalidSignature::ProposerSignature,
1128+
))
1129+
}
11021130
}
11031131
}
11041132

@@ -1153,7 +1181,9 @@ impl<T: BeaconChainTypes> SignatureVerifiedBlock<T> {
11531181
consensus_context,
11541182
})
11551183
} else {
1156-
Err(BlockError::InvalidSignature)
1184+
Err(BlockError::InvalidSignature(
1185+
InvalidSignature::BlockBodySignatures,
1186+
))
11571187
}
11581188
}
11591189

@@ -1981,7 +2011,7 @@ impl BlockBlobError for BlockError {
19812011
}
19822012

19832013
fn proposer_signature_invalid() -> Self {
1984-
BlockError::ProposalSignatureInvalid
2014+
BlockError::InvalidSignature(InvalidSignature::ProposerSignature)
19852015
}
19862016
}
19872017

‎beacon_node/beacon_chain/src/lib.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -78,7 +78,7 @@ pub use beacon_fork_choice_store::{BeaconForkChoiceStore, Error as ForkChoiceSto
7878
pub use block_verification::{
7979
build_blob_data_column_sidecars, get_block_root, BlockError, ExecutionPayloadError,
8080
ExecutionPendingBlock, GossipVerifiedBlock, IntoExecutionPendingBlock, IntoGossipVerifiedBlock,
81-
PayloadVerificationOutcome, PayloadVerificationStatus,
81+
InvalidSignature, PayloadVerificationOutcome, PayloadVerificationStatus,
8282
};
8383
pub use block_verification_types::AvailabilityPendingExecutedBlock;
8484
pub use block_verification_types::ExecutedBlock;

‎beacon_node/beacon_chain/tests/block_verification.rs

Lines changed: 35 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@ use beacon_chain::{
99
};
1010
use beacon_chain::{
1111
BeaconSnapshot, BlockError, ChainConfig, ChainSegmentResult, IntoExecutionPendingBlock,
12-
NotifyExecutionLayer,
12+
InvalidSignature, NotifyExecutionLayer,
1313
};
1414
use logging::test_logger;
1515
use slasher::{Config as SlasherConfig, Slasher};
@@ -438,7 +438,7 @@ async fn assert_invalid_signature(
438438
.process_chain_segment(blocks, NotifyExecutionLayer::Yes)
439439
.await
440440
.into_block_error(),
441-
Err(BlockError::InvalidSignature)
441+
Err(BlockError::InvalidSignature(InvalidSignature::Unknown))
442442
),
443443
"should not import chain segment with an invalid {} signature",
444444
item
@@ -480,7 +480,12 @@ async fn assert_invalid_signature(
480480
)
481481
.await;
482482
assert!(
483-
matches!(process_res, Err(BlockError::InvalidSignature)),
483+
matches!(
484+
process_res,
485+
Err(BlockError::InvalidSignature(
486+
InvalidSignature::BlockBodySignatures
487+
))
488+
),
484489
"should not import individual block with an invalid {} signature, got: {:?}",
485490
item,
486491
process_res
@@ -536,21 +541,25 @@ async fn invalid_signature_gossip_block() {
536541
.into_block_error()
537542
.expect("should import all blocks prior to the one being tested");
538543
let signed_block = SignedBeaconBlock::from_block(block, junk_signature());
544+
let process_res = harness
545+
.chain
546+
.process_block(
547+
signed_block.canonical_root(),
548+
Arc::new(signed_block),
549+
NotifyExecutionLayer::Yes,
550+
BlockImportSource::Lookup,
551+
|| Ok(()),
552+
)
553+
.await;
539554
assert!(
540555
matches!(
541-
harness
542-
.chain
543-
.process_block(
544-
signed_block.canonical_root(),
545-
Arc::new(signed_block),
546-
NotifyExecutionLayer::Yes,
547-
BlockImportSource::Lookup,
548-
|| Ok(()),
549-
)
550-
.await,
551-
Err(BlockError::InvalidSignature)
556+
process_res,
557+
Err(BlockError::InvalidSignature(
558+
InvalidSignature::ProposerSignature
559+
))
552560
),
553-
"should not import individual block with an invalid gossip signature",
561+
"should not import individual block with an invalid gossip signature, got: {:?}",
562+
process_res
554563
);
555564
}
556565
}
@@ -578,16 +587,18 @@ async fn invalid_signature_block_proposal() {
578587
})
579588
.collect::<Vec<_>>();
580589
// Ensure the block will be rejected if imported in a chain segment.
590+
let process_res = harness
591+
.chain
592+
.process_chain_segment(blocks, NotifyExecutionLayer::Yes)
593+
.await
594+
.into_block_error();
581595
assert!(
582596
matches!(
583-
harness
584-
.chain
585-
.process_chain_segment(blocks, NotifyExecutionLayer::Yes)
586-
.await
587-
.into_block_error(),
588-
Err(BlockError::InvalidSignature)
597+
process_res,
598+
Err(BlockError::InvalidSignature(InvalidSignature::Unknown))
589599
),
590-
"should not import chain segment with an invalid block signature",
600+
"should not import chain segment with an invalid block signature, got: {:?}",
601+
process_res
591602
);
592603
}
593604
}
@@ -890,7 +901,7 @@ async fn invalid_signature_deposit() {
890901
.process_chain_segment(blocks, NotifyExecutionLayer::Yes)
891902
.await
892903
.into_block_error(),
893-
Err(BlockError::InvalidSignature)
904+
Err(BlockError::InvalidSignature(InvalidSignature::Unknown))
894905
),
895906
"should not throw an invalid signature error for a bad deposit signature"
896907
);
@@ -1086,7 +1097,7 @@ async fn block_gossip_verification() {
10861097
)))
10871098
.await
10881099
),
1089-
BlockError::ProposalSignatureInvalid
1100+
BlockError::InvalidSignature(InvalidSignature::ProposerSignature)
10901101
),
10911102
"should not import a block with an invalid proposal signature"
10921103
);

‎beacon_node/network/src/network_beacon_processor/gossip_methods.rs

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1290,13 +1290,12 @@ impl<T: BeaconChainTypes> NetworkBeaconProcessor<T> {
12901290
Err(e @ BlockError::StateRootMismatch { .. })
12911291
| Err(e @ BlockError::IncorrectBlockProposer { .. })
12921292
| Err(e @ BlockError::BlockSlotLimitReached)
1293-
| Err(e @ BlockError::ProposalSignatureInvalid)
12941293
| Err(e @ BlockError::NonLinearSlots)
12951294
| Err(e @ BlockError::UnknownValidator(_))
12961295
| Err(e @ BlockError::PerBlockProcessingError(_))
12971296
| Err(e @ BlockError::NonLinearParentRoots)
12981297
| Err(e @ BlockError::BlockIsNotLaterThanParent { .. })
1299-
| Err(e @ BlockError::InvalidSignature)
1298+
| Err(e @ BlockError::InvalidSignature(_))
13001299
| Err(e @ BlockError::WeakSubjectivityConflict)
13011300
| Err(e @ BlockError::InconsistentFork(_))
13021301
| Err(e @ BlockError::ExecutionPayloadError(_))

‎beacon_node/network/src/sync/tests/lookups.rs

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1677,7 +1677,7 @@ fn test_parent_lookup_too_many_processing_attempts_must_blacklist() {
16771677
rig.assert_not_failed_chain(block_root);
16781678
// send the right parent but fail processing
16791679
rig.parent_lookup_block_response(id, peer_id, Some(parent.clone().into()));
1680-
rig.parent_block_processed(block_root, BlockError::InvalidSignature.into());
1680+
rig.parent_block_processed(block_root, BlockError::BlockSlotLimitReached.into());
16811681
rig.parent_lookup_block_response(id, peer_id, None);
16821682
rig.expect_penalty(peer_id, "lookup_block_processing_failure");
16831683
}
@@ -2575,7 +2575,7 @@ mod deneb_only {
25752575
fn invalid_parent_processed(mut self) -> Self {
25762576
self.rig.parent_block_processed(
25772577
self.block_root,
2578-
BlockProcessingResult::Err(BlockError::ProposalSignatureInvalid),
2578+
BlockProcessingResult::Err(BlockError::BlockSlotLimitReached),
25792579
);
25802580
assert_eq!(self.rig.active_parent_lookups_count(), 1);
25812581
self
@@ -2584,7 +2584,7 @@ mod deneb_only {
25842584
fn invalid_block_processed(mut self) -> Self {
25852585
self.rig.single_block_component_processed(
25862586
self.block_req_id.expect("block request id").lookup_id,
2587-
BlockProcessingResult::Err(BlockError::ProposalSignatureInvalid),
2587+
BlockProcessingResult::Err(BlockError::BlockSlotLimitReached),
25882588
);
25892589
self.rig.assert_single_lookups_count(1);
25902590
self

0 commit comments

Comments
 (0)