From 8ca76cf68aef134f60cb9307713bd95e63125d86 Mon Sep 17 00:00:00 2001 From: dapplion <35266934+dapplion@users.noreply.github.com> Date: Sun, 20 Oct 2024 19:44:13 +0300 Subject: [PATCH] Group invalid block error variants --- beacon_node/beacon_chain/src/beacon_chain.rs | 2 +- .../beacon_chain/src/block_verification.rs | 248 +++++++++++------- .../beacon_chain/src/execution_payload.rs | 12 +- beacon_node/beacon_chain/src/lib.rs | 2 +- beacon_node/beacon_chain/tests/rewards.rs | 4 +- .../gossip_methods.rs | 10 +- .../network_beacon_processor/sync_methods.rs | 7 +- .../network/src/sync/block_lookups/tests.rs | 2 +- 8 files changed, 164 insertions(+), 123 deletions(-) diff --git a/beacon_node/beacon_chain/src/beacon_chain.rs b/beacon_node/beacon_chain/src/beacon_chain.rs index f8dfbc55155..43f39d56e18 100644 --- a/beacon_node/beacon_chain/src/beacon_chain.rs +++ b/beacon_node/beacon_chain/src/beacon_chain.rs @@ -2704,7 +2704,7 @@ impl BeaconChain { if let Err(e) = block.as_block().fork_name(&self.spec) { return Err(ChainSegmentResult::Failed { imported_blocks, - error: BlockError::InconsistentFork(e), + error: e.into(), }); } diff --git a/beacon_node/beacon_chain/src/block_verification.rs b/beacon_node/beacon_chain/src/block_verification.rs index a8233f170f6..1281291bcca 100644 --- a/beacon_node/beacon_chain/src/block_verification.rs +++ b/beacon_node/beacon_chain/src/block_verification.rs @@ -155,14 +155,13 @@ pub enum BlockError { present_slot: Slot, block_slot: Slot, }, - /// The block state_root does not match the generated state. + /// The block conflicts with finalization, no need to propagate. /// /// ## Peer scoring /// - /// The peer has incompatible state transition logic and is faulty. - StateRootMismatch { block: Hash256, local: Hash256 }, - /// The block was a genesis block, these blocks cannot be re-imported. - GenesisBlock, + /// It's unclear if this block is valid, but it conflicts with finality and shouldn't be + /// imported. + NotFinalizedDescendant { block_parent_root: Hash256 }, /// The slot is finalized, no need to import. /// /// ## Peer scoring @@ -173,13 +172,8 @@ pub enum BlockError { block_slot: Slot, finalized_slot: Slot, }, - /// The block conflicts with finalization, no need to propagate. - /// - /// ## Peer scoring - /// - /// It's unclear if this block is valid, but it conflicts with finality and shouldn't be - /// imported. - NotFinalizedDescendant { block_parent_root: Hash256 }, + /// The block was a genesis block, these blocks cannot be re-imported. + GenesisBlock, /// Block is already known and valid, no need to re-import. /// /// ## Peer scoring @@ -192,45 +186,19 @@ pub enum BlockError { /// /// The block could be valid, or invalid. We don't know. DuplicateImportStatusUnknown(Hash256), - /// The block slot exceeds the MAXIMUM_BLOCK_SLOT_NUMBER. - /// - /// ## Peer scoring - /// - /// We set a very, very high maximum slot number and this block exceeds it. There's no good - /// reason to be sending these blocks, they're from future slots. - /// - /// The block is invalid and the peer is faulty. - BlockSlotLimitReached, - /// The `BeaconBlock` has a `proposer_index` that does not match the index we computed locally. + /// BeaconBlock data structure is spec invalid, excludes proposer signature validity as it's + /// outside the BeaconBlock container. /// /// ## Peer scoring /// /// The block is invalid and the peer is faulty. - IncorrectBlockProposer { block: u64, local_shuffling: u64 }, + InvalidBlock(InvalidBlockError), /// 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). - /// - /// ## Peer scoring - /// - /// The block is invalid and the peer is faulty. - InvalidSignature, - /// The provided block is not from a later slot than its parent. - /// - /// ## Peer scoring - /// - /// The block is invalid and the peer is faulty. - BlockIsNotLaterThanParent { block_slot: Slot, parent_slot: Slot }, /// At least one block in the chain segment did not have it's parent root set to the root of /// the prior block. /// @@ -245,12 +213,6 @@ pub enum BlockError { /// /// The chain of blocks is invalid and the peer is faulty. NonLinearSlots, - /// The block failed the specification's `per_block_processing` function, it is invalid. - /// - /// ## Peer scoring - /// - /// The block is invalid and the peer is faulty. - PerBlockProcessingError(BlockProcessingError), /// There was an error whilst processing the block. It is not necessarily invalid. /// /// ## Peer scoring @@ -265,28 +227,12 @@ pub enum BlockError { /// /// The block is invalid and the peer is faulty. WeakSubjectivityConflict, - /// The block has the wrong structure for the fork at `block.slot`. - /// - /// ## Peer scoring - /// - /// The block is invalid and the peer is faulty. - InconsistentFork(InconsistentFork), /// There was an error while validating the ExecutionPayload /// /// ## Peer scoring /// /// See `ExecutionPayloadError` for scoring information ExecutionPayloadError(ExecutionPayloadError), - /// The block references an parent block which has an execution payload which was found to be - /// invalid. - /// - /// ## Peer scoring - /// - /// The peer sent us an invalid block, we must penalise harshly. - /// If it's actually our fault (e.g. our execution node database is corrupt) we have bigger - /// problems to worry about than losing peers, and we're doing the network a favour by - /// disconnecting. - ParentExecutionPayloadInvalid { parent_root: Hash256 }, /// The block is a slashable equivocation from the proposer. /// /// ## Peer scoring @@ -328,6 +274,72 @@ pub enum BlockError { InternalError(String), } +#[derive(Debug)] +pub enum InvalidBlockError { + /// The block state_root does not match the generated state. + /// + /// ## Peer scoring + /// + /// The peer has incompatible state transition logic and is faulty. + StateRootMismatch { block: Hash256, local: Hash256 }, + /// The block slot exceeds the MAXIMUM_BLOCK_SLOT_NUMBER. + /// + /// ## Peer scoring + /// + /// We set a very, very high maximum slot number and this block exceeds it. There's no good + /// reason to be sending these blocks, they're from future slots. + /// + /// The block is invalid and the peer is faulty. + BlockSlotLimitReached, + /// The `BeaconBlock` has a `proposer_index` that does not match the index we computed locally. + /// + /// ## Peer scoring + /// + /// The block is invalid and the peer is faulty. + IncorrectBlockProposer { block: u64, local_shuffling: u64 }, + /// 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 body is invalid (exactly which is unknown, but excludes the + /// proposer signature). + /// + /// ## Peer scoring + /// + /// The block is invalid and the peer is faulty. + InvalidBlockBodySignatures, + /// The provided block is not from a later slot than its parent. + /// + /// ## Peer scoring + /// + /// The block is invalid and the peer is faulty. + BlockIsNotLaterThanParent { block_slot: Slot, parent_slot: Slot }, + /// The block failed the specification's `per_block_processing` function, it is invalid. + /// + /// ## Peer scoring + /// + /// The block is invalid and the peer is faulty. + PerBlockProcessingError(BlockProcessingError), + /// The block has the wrong structure for the fork at `block.slot`. + /// + /// ## Peer scoring + /// + /// The block is invalid and the peer is faulty. + InconsistentFork(InconsistentFork), + /// The block references an parent block which has an execution payload which was found to be + /// invalid. + /// + /// ## Peer scoring + /// + /// The peer sent us an invalid block, we must penalise harshly. + /// If it's actually our fault (e.g. our execution node database is corrupt) we have bigger + /// problems to worry about than losing peers, and we're doing the network a favour by + /// disconnecting. + ParentExecutionPayloadInvalid { parent_root: Hash256 }, +} + impl From for BlockError { fn from(e: AvailabilityCheckError) -> Self { Self::AvailabilityCheck(e) @@ -444,7 +456,7 @@ impl From for BlockError { impl From for BlockError { fn from(e: InconsistentFork) -> Self { - BlockError::InconsistentFork(e) + BlockError::InvalidBlock(InvalidBlockError::InconsistentFork(e)) } } @@ -462,10 +474,10 @@ impl From for BlockError { BlockSignatureVerifierError::IncorrectBlockProposer { block, local_shuffling, - } => BlockError::IncorrectBlockProposer { + } => BlockError::InvalidBlock(InvalidBlockError::IncorrectBlockProposer { block, local_shuffling, - }, + }), e => BlockError::BeaconChainError(BeaconChainError::BlockSignatureVerifierError(e)), } } @@ -651,7 +663,8 @@ pub fn signature_verify_chain_segment( } if signature_verifier.verify().is_err() { - return Err(BlockError::InvalidSignature); + todo!("should have separate error for chain-segment") + // return Err(BlockError::InvalidSignature); } drop(pubkey_cache); @@ -820,9 +833,7 @@ impl GossipVerifiedBlock { chain: &BeaconChain, ) -> Result { // Ensure the block is the correct structure for the fork at `block.slot()`. - block - .fork_name(&chain.spec) - .map_err(BlockError::InconsistentFork)?; + block.fork_name(&chain.spec)?; // Do not gossip or process blocks from future slots. let present_slot_with_tolerance = chain @@ -887,10 +898,12 @@ impl GossipVerifiedBlock { // // https://github.com/ethereum/eth2.0-specs/pull/2196 if parent_block.slot >= block.slot() { - return Err(BlockError::BlockIsNotLaterThanParent { - block_slot: block.slot(), - parent_slot: parent_block.slot, - }); + return Err(BlockError::InvalidBlock( + InvalidBlockError::BlockIsNotLaterThanParent { + block_slot: block.slot(), + parent_slot: parent_block.slot, + }, + )); } let proposer_shuffling_decision_block = @@ -954,7 +967,11 @@ impl GossipVerifiedBlock { let pubkey_cache = get_validator_pubkey_cache(chain)?; let pubkey = pubkey_cache .get(block.message().proposer_index() as usize) - .ok_or_else(|| BlockError::UnknownValidator(block.message().proposer_index()))?; + .ok_or_else(|| { + BlockError::InvalidBlock(InvalidBlockError::UnknownValidator( + block.message().proposer_index(), + )) + })?; block.verify_signature( Some(block_root), pubkey, @@ -994,10 +1011,12 @@ impl GossipVerifiedBlock { }; if block.message().proposer_index() != expected_proposer as u64 { - return Err(BlockError::IncorrectBlockProposer { - block: block.message().proposer_index(), - local_shuffling: expected_proposer as u64, - }); + return Err(BlockError::InvalidBlock( + InvalidBlockError::IncorrectBlockProposer { + block: block.message().proposer_index(), + local_shuffling: expected_proposer as u64, + }, + )); } // Validate the block's execution_payload (if any). @@ -1068,10 +1087,7 @@ impl SignatureVerifiedBlock { chain: &BeaconChain, ) -> Result { // Ensure the block is the correct structure for the fork at `block.slot()`. - block - .as_block() - .fork_name(&chain.spec) - .map_err(BlockError::InconsistentFork)?; + block.as_block().fork_name(&chain.spec)?; // Check the anchor slot before loading the parent, to avoid spurious lookups. check_block_against_anchor_slot(block.message(), chain)?; @@ -1102,7 +1118,17 @@ impl SignatureVerifiedBlock { parent: Some(parent), }) } else { - Err(BlockError::InvalidSignature) + // Re-verify the proposer signature in isolation to attribute fault + let mut signature_verifier = get_signature_verifier(&state, &pubkey_cache, &chain.spec); + signature_verifier.include_block_proposal(block.as_block(), Some(block_root), None)?; + if signature_verifier.verify().is_ok() { + // Proposer signature is valid, the invalid signature must be in the body + Err(BlockError::InvalidBlock( + InvalidBlockError::InvalidBlockBodySignatures, + )) + } else { + Err(BlockError::ProposalSignatureInvalid) + } } } @@ -1157,7 +1183,9 @@ impl SignatureVerifiedBlock { consensus_context, }) } else { - Err(BlockError::InvalidSignature) + Err(BlockError::InvalidBlock( + InvalidBlockError::InvalidBlockBodySignatures, + )) } } @@ -1320,9 +1348,11 @@ impl ExecutionPendingBlock { // Reject any block where the parent has an invalid payload. It's impossible for a valid // block to descend from an invalid parent. if parent.execution_status.is_invalid() { - return Err(BlockError::ParentExecutionPayloadInvalid { - parent_root: block.parent_root(), - }); + return Err(BlockError::InvalidBlock( + InvalidBlockError::ParentExecutionPayloadInvalid { + parent_root: block.parent_root(), + }, + )); } } else { // Reject any block if its parent is not known to fork choice. @@ -1450,10 +1480,12 @@ impl ExecutionPendingBlock { // The block must have a higher slot than its parent. if block.slot() <= parent.beacon_block.slot() { - return Err(BlockError::BlockIsNotLaterThanParent { - block_slot: block.slot(), - parent_slot: parent.beacon_block.slot(), - }); + return Err(BlockError::InvalidBlock( + InvalidBlockError::BlockIsNotLaterThanParent { + block_slot: block.slot(), + parent_slot: parent.beacon_block.slot(), + }, + )); } // Perform a sanity check on the pre-state. @@ -1628,7 +1660,11 @@ impl ExecutionPendingBlock { // Capture `BeaconStateError` so that we can easily distinguish between a block // that's invalid and one that caused an internal error. BlockProcessingError::BeaconStateError(e) => return Err(e.into()), - other => return Err(BlockError::PerBlockProcessingError(other)), + other => { + return Err(BlockError::InvalidBlock( + InvalidBlockError::PerBlockProcessingError(other), + )) + } } }; @@ -1655,10 +1691,12 @@ impl ExecutionPendingBlock { */ if block.state_root() != state_root { - return Err(BlockError::StateRootMismatch { - block: block.state_root(), - local: state_root, - }); + return Err(BlockError::InvalidBlock( + InvalidBlockError::StateRootMismatch { + block: block.state_root(), + local: state_root, + }, + )); } /* @@ -1679,7 +1717,11 @@ impl ExecutionPendingBlock { for (i, attestation) in block.message().body().attestations().enumerate() { let indexed_attestation = consensus_context .get_indexed_attestation(&state, attestation) - .map_err(|e| BlockError::PerBlockProcessingError(e.into_with_index(i)))?; + .map_err(|e| { + BlockError::InvalidBlock(InvalidBlockError::PerBlockProcessingError( + e.into_with_index(i), + )) + })?; match fork_choice.on_attestation( current_slot, @@ -1824,7 +1866,9 @@ pub fn check_block_relevancy( // This is an artificial (non-spec) restriction that provides some protection from overflow // abuses. if block.slot() >= MAXIMUM_BLOCK_SLOT_NUMBER { - return Err(BlockError::BlockSlotLimitReached); + return Err(BlockError::InvalidBlock( + InvalidBlockError::BlockSlotLimitReached, + )); } // Do not process a block from a finalized slot. @@ -2009,14 +2053,14 @@ pub trait BlockBlobError: From + From + Debu impl BlockBlobError for BlockError { fn not_later_than_parent_error(block_slot: Slot, parent_slot: Slot) -> Self { - BlockError::BlockIsNotLaterThanParent { + BlockError::InvalidBlock(InvalidBlockError::BlockIsNotLaterThanParent { block_slot, parent_slot, - } + }) } fn unknown_validator_error(validator_index: u64) -> Self { - BlockError::UnknownValidator(validator_index) + BlockError::InvalidBlock(InvalidBlockError::UnknownValidator(validator_index)) } fn proposer_signature_invalid() -> Self { diff --git a/beacon_node/beacon_chain/src/execution_payload.rs b/beacon_node/beacon_chain/src/execution_payload.rs index b9b98bfbc00..6c43843a0fd 100644 --- a/beacon_node/beacon_chain/src/execution_payload.rs +++ b/beacon_node/beacon_chain/src/execution_payload.rs @@ -10,7 +10,7 @@ use crate::otb_verification_service::OptimisticTransitionBlock; use crate::{ BeaconChain, BeaconChainError, BeaconChainTypes, BlockError, BlockProductionError, - ExecutionPayloadError, + ExecutionPayloadError, InvalidBlockError, }; use execution_layer::{ BlockProposalContents, BlockProposalContentsType, BuilderParams, NewPayloadRequest, @@ -77,7 +77,7 @@ impl PayloadNotifier { block_message.body(), &chain.spec, ) - .map_err(BlockError::PerBlockProcessingError)?; + .map_err(|e| BlockError::InvalidBlock(InvalidBlockError::PerBlockProcessingError(e)))?; match notify_execution_layer { NotifyExecutionLayer::No if chain.config.optimistic_finalized_sync => { @@ -348,9 +348,11 @@ pub fn validate_execution_payload_for_gossip( // If the parent has an invalid payload then it's impossible to build a valid block upon // it. Reject the block. ExecutionStatus::Invalid(_) => { - return Err(BlockError::ParentExecutionPayloadInvalid { - parent_root: parent_block.root, - }) + return Err(BlockError::InvalidBlock( + InvalidBlockError::ParentExecutionPayloadInvalid { + parent_root: parent_block.root, + }, + )) } }; diff --git a/beacon_node/beacon_chain/src/lib.rs b/beacon_node/beacon_chain/src/lib.rs index b89c00e0af4..c61822f8daf 100644 --- a/beacon_node/beacon_chain/src/lib.rs +++ b/beacon_node/beacon_chain/src/lib.rs @@ -77,7 +77,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, + InvalidBlockError, PayloadVerificationOutcome, PayloadVerificationStatus, }; pub use block_verification_types::AvailabilityPendingExecutedBlock; pub use block_verification_types::ExecutedBlock; diff --git a/beacon_node/beacon_chain/tests/rewards.rs b/beacon_node/beacon_chain/tests/rewards.rs index be7045c54a9..5636fb2c4b8 100644 --- a/beacon_node/beacon_chain/tests/rewards.rs +++ b/beacon_node/beacon_chain/tests/rewards.rs @@ -7,7 +7,7 @@ use beacon_chain::test_utils::{ use beacon_chain::{ test_utils::{AttestationStrategy, BlockStrategy, RelativeSyncCommittee}, types::{Epoch, EthSpec, Keypair, MinimalEthSpec}, - BlockError, ChainConfig, StateSkipConfig, WhenSlotSkipped, + BlockError, ChainConfig, InvalidBlockError, StateSkipConfig, WhenSlotSkipped, }; use eth2::lighthouse::attestation_rewards::TotalAttestationRewards; use eth2::lighthouse::StandardAttestationRewards; @@ -278,7 +278,7 @@ async fn test_rewards_base_multi_inclusion() { // funky hack: on first try, the state root will mismatch due to our modification // thankfully, the correct state root is reported back, so we just take that one :^) // there probably is a better way... - let Err(BlockError::StateRootMismatch { local, .. }) = harness + let Err(BlockError::InvalidBlock(InvalidBlockError::StateRootMismatch { local, .. })) = harness .process_block(slot, block.0.canonical_root(), block.clone()) .await else { 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 4d875cb4a14..67a13abeaf1 100644 --- a/beacon_node/network/src/network_beacon_processor/gossip_methods.rs +++ b/beacon_node/network/src/network_beacon_processor/gossip_methods.rs @@ -1273,20 +1273,12 @@ impl NetworkBeaconProcessor { self.propagate_validation_result(message_id, peer_id, MessageAcceptance::Ignore); return None; } - Err(e @ BlockError::StateRootMismatch { .. }) - | Err(e @ BlockError::IncorrectBlockProposer { .. }) - | Err(e @ BlockError::BlockSlotLimitReached) + Err(e @ BlockError::InvalidBlock { .. }) | 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::WeakSubjectivityConflict) - | Err(e @ BlockError::InconsistentFork(_)) | Err(e @ BlockError::ExecutionPayloadError(_)) - | Err(e @ BlockError::ParentExecutionPayloadInvalid { .. }) | Err(e @ BlockError::GenesisBlock) => { warn!(self.log, "Could not verify block for gossip. Rejecting the block"; "error" => %e); diff --git a/beacon_node/network/src/network_beacon_processor/sync_methods.rs b/beacon_node/network/src/network_beacon_processor/sync_methods.rs index 82d06c20f8e..6d9033b2ee7 100644 --- a/beacon_node/network/src/network_beacon_processor/sync_methods.rs +++ b/beacon_node/network/src/network_beacon_processor/sync_methods.rs @@ -11,7 +11,8 @@ use beacon_chain::data_availability_checker::MaybeAvailableBlock; use beacon_chain::data_column_verification::verify_kzg_for_data_column_list; use beacon_chain::{ validator_monitor::get_slot_delay_ms, AvailabilityProcessingStatus, BeaconChainError, - BeaconChainTypes, BlockError, ChainSegmentResult, HistoricalBlockError, NotifyExecutionLayer, + BeaconChainTypes, BlockError, ChainSegmentResult, HistoricalBlockError, InvalidBlockError, + NotifyExecutionLayer, }; use beacon_processor::{ work_reprocessing_queue::{QueuedRpcBlock, ReprocessQueueMessage}, @@ -805,7 +806,9 @@ impl NetworkBeaconProcessor { }) } } - ref err @ BlockError::ParentExecutionPayloadInvalid { ref parent_root } => { + ref err @ BlockError::InvalidBlock( + InvalidBlockError::ParentExecutionPayloadInvalid { ref parent_root }, + ) => { warn!( self.log, "Failed to sync chain built on invalid parent"; diff --git a/beacon_node/network/src/sync/block_lookups/tests.rs b/beacon_node/network/src/sync/block_lookups/tests.rs index 7192faa12dc..909a87b8d69 100644 --- a/beacon_node/network/src/sync/block_lookups/tests.rs +++ b/beacon_node/network/src/sync/block_lookups/tests.rs @@ -1708,7 +1708,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::ProposalSignatureInvalid.into()); rig.parent_lookup_block_response(id, peer_id, None); rig.expect_penalty(peer_id, "lookup_block_processing_failure"); }