From 619ad106cff76fa3287805eecc7674022475952f Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Fri, 14 Aug 2020 06:36:38 +0000 Subject: [PATCH] Restrict fork choice getters to finalized blocks (#1475) ## Issue Addressed - Resolves #1451 ## Proposed Changes - Restricts the `contains_block` and `contains_block` so they only indicate a block is present if it descends from the finalized root. This helps to ensure that fork choice never points to a block that has been pruned from the database. - Resolves #1451 - Before importing a block, double-check that its parent is known and a descendant of the finalized root. - Split a big, monolithic block verification test into smaller tests. ## Additional Notes I suspect there would be a craftier way to do the `is_descendant_of_finalized` check, but we're a bit tight on time now and we can optimize later if it starts showing in benches. ## TODO - [x] Tests --- beacon_node/beacon_chain/src/beacon_chain.rs | 21 +- .../beacon_chain/src/block_verification.rs | 49 ++- .../beacon_chain/tests/block_verification.rs | 295 +++++++++++------- consensus/fork_choice/src/fork_choice.rs | 19 +- .../src/proto_array_fork_choice.rs | 88 ++++++ 5 files changed, 352 insertions(+), 120 deletions(-) diff --git a/beacon_node/beacon_chain/src/beacon_chain.rs b/beacon_node/beacon_chain/src/beacon_chain.rs index 0ab7be80e..6fa06d3a8 100644 --- a/beacon_node/beacon_chain/src/beacon_chain.rs +++ b/beacon_node/beacon_chain/src/beacon_chain.rs @@ -3,8 +3,9 @@ use crate::attestation_verification::{ VerifiedUnaggregatedAttestation, }; use crate::block_verification::{ - check_block_relevancy, get_block_root, signature_verify_chain_segment, BlockError, - FullyVerifiedBlock, GossipVerifiedBlock, IntoFullyVerifiedBlock, + check_block_is_finalized_descendant, check_block_relevancy, get_block_root, + signature_verify_chain_segment, BlockError, FullyVerifiedBlock, GossipVerifiedBlock, + IntoFullyVerifiedBlock, }; use crate::errors::{BeaconChainError as Error, BlockProductionError}; use crate::eth1_chain::{Eth1Chain, Eth1ChainBackend}; @@ -1214,6 +1215,14 @@ impl BeaconChain { // However, we will potentially get a `ParentUnknown` on a later block. The sync // protocol will need to ensure this is handled gracefully. Err(BlockError::WouldRevertFinalizedSlot { .. }) => continue, + // The block has a known parent that does not descend from the finalized block. + // There is no need to process this block or any children. + Err(BlockError::NotFinalizedDescendant { block_parent_root }) => { + return ChainSegmentResult::Failed { + imported_blocks, + error: BlockError::NotFinalizedDescendant { block_parent_root }, + } + } // If there was an error whilst determining if the block was invalid, return that // error. Err(BlockError::BeaconChainError(e)) => { @@ -1415,7 +1424,6 @@ impl BeaconChain { fully_verified_block: FullyVerifiedBlock, ) -> Result> { let signed_block = fully_verified_block.block; - let block = &signed_block.message; let block_root = fully_verified_block.block_root; let state = fully_verified_block.state; let parent_block = fully_verified_block.parent_block; @@ -1427,7 +1435,7 @@ impl BeaconChain { // Iterate through the attestations in the block and register them as an "observed // attestation". This will stop us from propagating them on the gossip network. - for a in &block.body.attestations { + for a in &signed_block.message.body.attestations { match self.observed_attestations.observe_attestation(a, None) { // If the observation was successful or if the slot for the attestation was too // low, continue. @@ -1477,6 +1485,11 @@ impl BeaconChain { let mut fork_choice = self.fork_choice.write(); + // Do not import a block that doesn't descend from the finalized root. + let signed_block = + check_block_is_finalized_descendant::(signed_block, &fork_choice, &self.store)?; + let block = &signed_block.message; + // Register the new block with the fork choice service. { let _fork_choice_block_timer = diff --git a/beacon_node/beacon_chain/src/block_verification.rs b/beacon_node/beacon_chain/src/block_verification.rs index 710e7606c..38acf1b17 100644 --- a/beacon_node/beacon_chain/src/block_verification.rs +++ b/beacon_node/beacon_chain/src/block_verification.rs @@ -48,6 +48,7 @@ use crate::{ }, metrics, BeaconChain, BeaconChainError, BeaconChainTypes, BeaconSnapshot, }; +use fork_choice::{ForkChoice, ForkChoiceStore}; use parking_lot::RwLockReadGuard; use slog::{error, Logger}; use slot_clock::SlotClock; @@ -62,7 +63,7 @@ use std::borrow::Cow; use std::convert::TryFrom; use std::fs; use std::io::Write; -use store::{Error as DBError, HotStateSummary, StoreOp}; +use store::{Error as DBError, HotColdDB, HotStateSummary, StoreOp}; use tree_hash::TreeHash; use types::{ BeaconBlock, BeaconState, BeaconStateError, ChainSpec, CloneConfig, EthSpec, Hash256, @@ -118,6 +119,13 @@ 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 }, /// Block is already known, no need to re-import. /// /// ## Peer scoring @@ -397,6 +405,15 @@ impl GossipVerifiedBlock { }); } + // Do not process a block that doesn't descend from the finalized root. + // + // We check this *before* we load the parent so that we can return a more detailed error. + let block = check_block_is_finalized_descendant::( + block, + &chain.fork_choice.read(), + &chain.store, + )?; + let (mut parent, block) = load_parent(block, chain)?; let block_root = get_block_root(&block); @@ -779,6 +796,36 @@ fn check_block_against_finalized_slot( } } +/// Returns `Ok(block)` if the block descends from the finalized root. +pub fn check_block_is_finalized_descendant>( + block: SignedBeaconBlock, + fork_choice: &ForkChoice, + store: &HotColdDB, +) -> Result, BlockError> { + if fork_choice.is_descendant_of_finalized(block.parent_root()) { + Ok(block) + } else { + // If fork choice does *not* consider the parent to be a descendant of the finalized block, + // then there are two more cases: + // + // 1. We have the parent stored in our database. Because fork-choice has confirmed the + // parent is *not* in our post-finalization DAG, all other blocks must be either + // pre-finalization or conflicting with finalization. + // 2. The parent is unknown to us, we probably want to download it since it might actually + // descend from the finalized root. + if store + .item_exists::>(&block.parent_root()) + .map_err(|e| BlockError::BeaconChainError(e.into()))? + { + Err(BlockError::NotFinalizedDescendant { + block_parent_root: block.parent_root(), + }) + } else { + Err(BlockError::ParentUnknown(Box::new(block))) + } + } +} + /// Performs simple, cheap checks to ensure that the block is relevant to be imported. /// /// `Ok(block_root)` is returned if the block passes these checks and should progress with diff --git a/beacon_node/beacon_chain/tests/block_verification.rs b/beacon_node/beacon_chain/tests/block_verification.rs index 30c7c5603..fbee03268 100644 --- a/beacon_node/beacon_chain/tests/block_verification.rs +++ b/beacon_node/beacon_chain/tests/block_verification.rs @@ -18,8 +18,9 @@ use types::{ type E = MainnetEthSpec; // Should ideally be divisible by 3. -pub const VALIDATOR_COUNT: usize = 24; -pub const CHAIN_SEGMENT_LENGTH: usize = 64 * 5; +const VALIDATOR_COUNT: usize = 24; +const CHAIN_SEGMENT_LENGTH: usize = 64 * 5; +const BLOCK_INDICES: &[usize] = &[0, 1, 32, 64, 68 + 1, 129, CHAIN_SEGMENT_LENGTH - 1]; lazy_static! { /// A cached set of keys. @@ -272,17 +273,73 @@ fn chain_segment_non_linear_slots() { ); } +fn assert_invalid_signature( + harness: &BeaconChainHarness>, + block_index: usize, + snapshots: &[BeaconSnapshot], + item: &str, +) { + let blocks = snapshots + .iter() + .map(|snapshot| snapshot.beacon_block.clone()) + .collect(); + + // Ensure the block will be rejected if imported in a chain segment. + assert!( + matches!( + harness + .chain + .process_chain_segment(blocks) + .into_block_error(), + Err(BlockError::InvalidSignature) + ), + "should not import chain segment with an invalid {} signature", + item + ); + + // Ensure the block will be rejected if imported on its own (without gossip checking). + let ancestor_blocks = CHAIN_SEGMENT + .iter() + .take(block_index) + .map(|snapshot| snapshot.beacon_block.clone()) + .collect(); + // We don't care if this fails, we just call this to ensure that all prior blocks have been + // imported prior to this test. + let _ = harness.chain.process_chain_segment(ancestor_blocks); + assert!( + matches!( + harness + .chain + .process_block(snapshots[block_index].beacon_block.clone()), + Err(BlockError::InvalidSignature) + ), + "should not import individual block with an invalid {} signature", + item + ); + + // NOTE: we choose not to check gossip verification here. It only checks one signature + // (proposal) and that is already tested elsewhere in this file. + // + // It's not trivial to just check gossip verification since it will start refusing + // blocks as soon as it has seen one valid proposal signature for a given (validator, + // slot) tuple. +} + +fn get_invalid_sigs_harness() -> BeaconChainHarness> { + let harness = get_harness(VALIDATOR_COUNT); + harness + .chain + .slot_clock + .set_slot(CHAIN_SEGMENT.last().unwrap().beacon_block.slot().as_u64()); + harness +} #[test] -fn invalid_signatures() { - let mut checked_attestation = false; - - for &block_index in &[0, 1, 32, 64, 68 + 1, 129, CHAIN_SEGMENT.len() - 1] { - let harness = get_harness(VALIDATOR_COUNT); - harness - .chain - .slot_clock - .set_slot(CHAIN_SEGMENT.last().unwrap().beacon_block.slot().as_u64()); - +fn invalid_signature_gossip_block() { + for &block_index in BLOCK_INDICES { + // Ensure the block will be rejected if imported on its own (without gossip checking). + let harness = get_invalid_sigs_harness(); + let mut snapshots = CHAIN_SEGMENT.clone(); + snapshots[block_index].beacon_block.signature = junk_signature(); // Import all the ancestors before the `block_index` block. let ancestor_blocks = CHAIN_SEGMENT .iter() @@ -294,75 +351,6 @@ fn invalid_signatures() { .process_chain_segment(ancestor_blocks) .into_block_error() .expect("should import all blocks prior to the one being tested"); - - // For the given snapshots, test the following: - // - // - The `process_chain_segment` function returns `InvalidSignature`. - // - The `process_block` function returns `InvalidSignature` when importing the - // `SignedBeaconBlock` directly. - // - The `verify_block_for_gossip` function does _not_ return an error. - // - The `process_block` function returns `InvalidSignature` when verifying the - // `GossipVerifiedBlock`. - let assert_invalid_signature = |snapshots: &[BeaconSnapshot], item: &str| { - let blocks = snapshots - .iter() - .map(|snapshot| snapshot.beacon_block.clone()) - .collect(); - - // Ensure the block will be rejected if imported in a chain segment. - assert!( - matches!( - harness - .chain - .process_chain_segment(blocks) - .into_block_error(), - Err(BlockError::InvalidSignature) - ), - "should not import chain segment with an invalid {} signature", - item - ); - - // Ensure the block will be rejected if imported on its own (without gossip checking). - assert!( - matches!( - harness - .chain - .process_block(snapshots[block_index].beacon_block.clone()), - Err(BlockError::InvalidSignature) - ), - "should not import individual block with an invalid {} signature", - item - ); - - // NOTE: we choose not to check gossip verification here. It only checks one signature - // (proposal) and that is already tested elsewhere in this file. - // - // It's not trivial to just check gossip verification since it will start refusing - // blocks as soon as it has seen one valid proposal signature for a given (validator, - // slot) tuple. - }; - - /* - * Block proposal - */ - let mut snapshots = CHAIN_SEGMENT.clone(); - snapshots[block_index].beacon_block.signature = junk_signature(); - let blocks = snapshots - .iter() - .map(|snapshot| snapshot.beacon_block.clone()) - .collect(); - // Ensure the block will be rejected if imported in a chain segment. - assert!( - matches!( - harness - .chain - .process_chain_segment(blocks) - .into_block_error(), - Err(BlockError::InvalidSignature) - ), - "should not import chain segment with an invalid gossip signature", - ); - // Ensure the block will be rejected if imported on its own (without gossip checking). assert!( matches!( harness @@ -372,10 +360,37 @@ fn invalid_signatures() { ), "should not import individual block with an invalid gossip signature", ); + } +} - /* - * Randao reveal - */ +#[test] +fn invalid_signature_block_proposal() { + for &block_index in BLOCK_INDICES { + let harness = get_invalid_sigs_harness(); + let mut snapshots = CHAIN_SEGMENT.clone(); + snapshots[block_index].beacon_block.signature = junk_signature(); + let blocks = snapshots + .iter() + .map(|snapshot| snapshot.beacon_block.clone()) + .collect::>(); + // Ensure the block will be rejected if imported in a chain segment. + assert!( + matches!( + harness + .chain + .process_chain_segment(blocks) + .into_block_error(), + Err(BlockError::InvalidSignature) + ), + "should not import chain segment with an invalid block signature", + ); + } +} + +#[test] +fn invalid_signature_randao_reveal() { + for &block_index in BLOCK_INDICES { + let harness = get_invalid_sigs_harness(); let mut snapshots = CHAIN_SEGMENT.clone(); snapshots[block_index] .beacon_block @@ -384,11 +399,14 @@ fn invalid_signatures() { .randao_reveal = junk_signature(); update_parent_roots(&mut snapshots); update_proposal_signatures(&mut snapshots, &harness); - assert_invalid_signature(&snapshots, "randao"); + assert_invalid_signature(&harness, block_index, &snapshots, "randao"); + } +} - /* - * Proposer slashing - */ +#[test] +fn invalid_signature_proposer_slashing() { + for &block_index in BLOCK_INDICES { + let harness = get_invalid_sigs_harness(); let mut snapshots = CHAIN_SEGMENT.clone(); let proposer_slashing = ProposerSlashing { signed_header_1: SignedBeaconBlockHeader { @@ -409,11 +427,14 @@ fn invalid_signatures() { .expect("should update proposer slashing"); update_parent_roots(&mut snapshots); update_proposal_signatures(&mut snapshots, &harness); - assert_invalid_signature(&snapshots, "proposer slashing"); + assert_invalid_signature(&harness, block_index, &snapshots, "proposer slashing"); + } +} - /* - * Attester slashing - */ +#[test] +fn invalid_signature_attester_slashing() { + for &block_index in BLOCK_INDICES { + let harness = get_invalid_sigs_harness(); let mut snapshots = CHAIN_SEGMENT.clone(); let indexed_attestation = IndexedAttestation { attesting_indices: vec![0].into(), @@ -445,11 +466,16 @@ fn invalid_signatures() { .expect("should update attester slashing"); update_parent_roots(&mut snapshots); update_proposal_signatures(&mut snapshots, &harness); - assert_invalid_signature(&snapshots, "attester slashing"); + assert_invalid_signature(&harness, block_index, &snapshots, "attester slashing"); + } +} - /* - * Attestation - */ +#[test] +fn invalid_signature_attestation() { + let mut checked_attestation = false; + + for &block_index in BLOCK_INDICES { + let harness = get_invalid_sigs_harness(); let mut snapshots = CHAIN_SEGMENT.clone(); if let Some(attestation) = snapshots[block_index] .beacon_block @@ -461,15 +487,22 @@ fn invalid_signatures() { attestation.signature = junk_aggregate_signature(); update_parent_roots(&mut snapshots); update_proposal_signatures(&mut snapshots, &harness); - assert_invalid_signature(&snapshots, "attestation"); + assert_invalid_signature(&harness, block_index, &snapshots, "attestation"); checked_attestation = true; } + } - /* - * Deposit - * - * Note: an invalid deposit signature is permitted! - */ + assert!( + checked_attestation, + "the test should check an attestation signature" + ) +} + +#[test] +fn invalid_signature_deposit() { + for &block_index in BLOCK_INDICES { + // Note: an invalid deposit signature is permitted! + let harness = get_invalid_sigs_harness(); let mut snapshots = CHAIN_SEGMENT.clone(); let deposit = Deposit { proof: vec![Hash256::zero(); DEPOSIT_TREE_DEPTH + 1].into(), @@ -503,10 +536,13 @@ fn invalid_signatures() { ), "should not throw an invalid signature error for a bad deposit signature" ); + } +} - /* - * Voluntary exit - */ +#[test] +fn invalid_signature_exit() { + for &block_index in BLOCK_INDICES { + let harness = get_invalid_sigs_harness(); let mut snapshots = CHAIN_SEGMENT.clone(); let epoch = snapshots[block_index].beacon_state.current_epoch(); snapshots[block_index] @@ -524,13 +560,8 @@ fn invalid_signatures() { .expect("should update deposit"); update_parent_roots(&mut snapshots); update_proposal_signatures(&mut snapshots, &harness); - assert_invalid_signature(&snapshots, "voluntary exit"); + assert_invalid_signature(&harness, block_index, &snapshots, "voluntary exit"); } - - assert!( - checked_attestation, - "the test should check an attestation signature" - ) } fn unwrap_err(result: Result) -> E { @@ -641,6 +672,48 @@ fn block_gossip_verification() { "should not import a block with an invalid proposal signature" ); + /* + * This test ensures that: + * + * Spec v0.12.2 + * + * The block's parent (defined by block.parent_root) passes validation. + */ + + let mut block = CHAIN_SEGMENT[block_index].beacon_block.clone(); + let parent_root = Hash256::from_low_u64_be(42); + block.message.parent_root = parent_root; + assert!( + matches!( + unwrap_err(harness.chain.verify_block_for_gossip(block)), + BlockError::ParentUnknown(block) + if block.parent_root() == parent_root + ), + "should not import a block for an unknown parent" + ); + + /* + * This test ensures that: + * + * Spec v0.12.2 + * + * The current finalized_checkpoint is an ancestor of block -- i.e. get_ancestor(store, + * block.parent_root, compute_start_slot_at_epoch(store.finalized_checkpoint.epoch)) == + * store.finalized_checkpoint.root + */ + + let mut block = CHAIN_SEGMENT[block_index].beacon_block.clone(); + let parent_root = CHAIN_SEGMENT[0].beacon_block_root; + block.message.parent_root = parent_root; + assert!( + matches!( + unwrap_err(harness.chain.verify_block_for_gossip(block)), + BlockError::NotFinalizedDescendant { block_parent_root } + if block_parent_root == parent_root + ), + "should not import a block that conflicts with finality" + ); + /* * This test ensures that: * diff --git a/consensus/fork_choice/src/fork_choice.rs b/consensus/fork_choice/src/fork_choice.rs index 22204c445..87818ac84 100644 --- a/consensus/fork_choice/src/fork_choice.rs +++ b/consensus/fork_choice/src/fork_choice.rs @@ -738,14 +738,25 @@ where Ok(()) } - /// Returns `true` if the block is known. + /// Returns `true` if the block is known **and** a descendant of the finalized root. pub fn contains_block(&self, block_root: &Hash256) -> bool { - self.proto_array.contains_block(block_root) + self.proto_array.contains_block(block_root) && self.is_descendant_of_finalized(*block_root) } - /// Returns a `ProtoBlock` if the block is known. + /// Returns a `ProtoBlock` if the block is known **and** a descendant of the finalized root. pub fn get_block(&self, block_root: &Hash256) -> Option { - self.proto_array.get_block(block_root) + self.proto_array.get_block(block_root).filter(|block| { + // If available, use the parent_root to perform the lookup since it will involve one + // less lookup. This involves making the assumption that the finalized block will + // always have `block.parent_root` of `None`. + self.is_descendant_of_finalized(block.parent_root.unwrap_or(block.root)) + }) + } + + /// Return `true` if `block_root` is equal to the finalized root, or a known descendant of it. + pub fn is_descendant_of_finalized(&self, block_root: Hash256) -> bool { + self.proto_array + .is_descendant(self.fc_store.finalized_checkpoint().root, block_root) } /// Returns the latest message for a given validator, if any. diff --git a/consensus/proto_array/src/proto_array_fork_choice.rs b/consensus/proto_array/src/proto_array_fork_choice.rs index 5a728d9cf..05f6c5ec4 100644 --- a/consensus/proto_array/src/proto_array_fork_choice.rs +++ b/consensus/proto_array/src/proto_array_fork_choice.rs @@ -198,6 +198,27 @@ impl ProtoArrayForkChoice { }) } + /// Returns `true` if the `descendant_root` has an ancestor with `ancestor_root`. Always + /// returns `false` if either input roots are unknown. + /// + /// ## Notes + /// + /// Still returns `true` if `ancestor_root` is known and `ancestor_root == descendant_root`. + pub fn is_descendant(&self, ancestor_root: Hash256, descendant_root: Hash256) -> bool { + self.proto_array + .indices + .get(&ancestor_root) + .and_then(|ancestor_index| self.proto_array.nodes.get(*ancestor_index)) + .and_then(|ancestor| { + self.proto_array + .iter_block_roots(&descendant_root) + .take_while(|(_root, slot)| *slot >= ancestor.slot) + .find(|(_root, slot)| *slot == ancestor.slot) + .map(|(root, _slot)| root == ancestor_root) + }) + .unwrap_or(false) + } + pub fn latest_message(&self, validator_index: usize) -> Option<(Hash256, Epoch)> { if validator_index < self.votes.0.len() { let vote = &self.votes.0[validator_index]; @@ -309,6 +330,73 @@ mod test_compute_deltas { Hash256::from_low_u64_be(i as u64 + 1) } + #[test] + fn finalized_descendant() { + let genesis_slot = Slot::new(0); + let genesis_epoch = Epoch::new(0); + + let state_root = Hash256::from_low_u64_be(0); + let finalized_root = Hash256::from_low_u64_be(1); + let finalized_desc = Hash256::from_low_u64_be(2); + let not_finalized_desc = Hash256::from_low_u64_be(3); + let unknown = Hash256::from_low_u64_be(4); + + let mut fc = ProtoArrayForkChoice::new( + genesis_slot, + state_root, + genesis_epoch, + genesis_epoch, + finalized_root, + ) + .unwrap(); + + // Add block that is a finalized descendant. + fc.proto_array + .on_block(Block { + slot: genesis_slot + 1, + root: finalized_desc, + parent_root: Some(finalized_root), + state_root, + target_root: finalized_root, + justified_epoch: genesis_epoch, + finalized_epoch: genesis_epoch, + }) + .unwrap(); + + // Add block that is *not* a finalized descendant. + fc.proto_array + .on_block(Block { + slot: genesis_slot + 1, + root: not_finalized_desc, + parent_root: None, + state_root, + target_root: finalized_root, + justified_epoch: genesis_epoch, + finalized_epoch: genesis_epoch, + }) + .unwrap(); + + assert!(!fc.is_descendant(unknown, unknown)); + assert!(!fc.is_descendant(unknown, finalized_root)); + assert!(!fc.is_descendant(unknown, finalized_desc)); + assert!(!fc.is_descendant(unknown, not_finalized_desc)); + + assert!(fc.is_descendant(finalized_root, finalized_root)); + assert!(fc.is_descendant(finalized_root, finalized_desc)); + assert!(!fc.is_descendant(finalized_root, not_finalized_desc)); + assert!(!fc.is_descendant(finalized_root, unknown)); + + assert!(!fc.is_descendant(finalized_desc, not_finalized_desc)); + assert!(fc.is_descendant(finalized_desc, finalized_desc)); + assert!(!fc.is_descendant(finalized_desc, finalized_root)); + assert!(!fc.is_descendant(finalized_desc, unknown)); + + assert!(fc.is_descendant(not_finalized_desc, not_finalized_desc)); + assert!(!fc.is_descendant(not_finalized_desc, finalized_desc)); + assert!(!fc.is_descendant(not_finalized_desc, finalized_root)); + assert!(!fc.is_descendant(not_finalized_desc, unknown)); + } + #[test] fn zero_hash() { let validator_count: usize = 16;