From b284f81a7d6ee1a3c53fe6b4f82cb1ac3d4b9c46 Mon Sep 17 00:00:00 2001 From: Michael Sproul Date: Wed, 14 Sep 2022 17:28:49 +1000 Subject: [PATCH] Tweak signature verifier handling of proposer --- .../beacon_chain/src/block_verification.rs | 7 +++--- beacon_node/store/src/hot_cold_store.rs | 4 +-- beacon_node/store/src/hot_state_iter.rs | 2 +- .../src/per_block_processing.rs | 2 ++ .../block_signature_verifier.rs | 8 ++++-- .../per_block_processing/signature_sets.rs | 25 +++++++++++++------ .../altair/participation_cache.rs | 1 + .../types/src/beacon_state/committee_cache.rs | 1 + lcli/src/skip_slots.rs | 4 +-- lcli/src/transition_blocks.rs | 11 ++++---- 10 files changed, 42 insertions(+), 23 deletions(-) diff --git a/beacon_node/beacon_chain/src/block_verification.rs b/beacon_node/beacon_chain/src/block_verification.rs index f4eef11441..5237e598d9 100644 --- a/beacon_node/beacon_chain/src/block_verification.rs +++ b/beacon_node/beacon_chain/src/block_verification.rs @@ -548,7 +548,7 @@ pub fn signature_verify_chain_segment( let mut signature_verifier = get_signature_verifier(&state, &pubkey_cache, &chain.spec); for (block_root, block) in &chain_segment { - signature_verifier.include_all_signatures(block, Some(*block_root))?; + signature_verifier.include_all_signatures(block, Some(*block_root), true)?; } if signature_verifier.verify().is_err() { @@ -938,12 +938,13 @@ impl SignatureVerifiedBlock { let mut signature_verifier = get_signature_verifier(&state, &pubkey_cache, &chain.spec); - signature_verifier.include_all_signatures(&block, Some(block_root))?; + signature_verifier.include_all_signatures(&block, Some(block_root), true)?; if signature_verifier.verify().is_ok() { Ok(Self { consensus_context: ConsensusContext::new(block.slot()) - .set_current_block_root(block_root), + .set_current_block_root(block_root) + .set_proposer_index(block.message().proposer_index()), block, block_root, parent: Some(parent), diff --git a/beacon_node/store/src/hot_cold_store.rs b/beacon_node/store/src/hot_cold_store.rs index 4f56956b8b..59452653a0 100644 --- a/beacon_node/store/src/hot_cold_store.rs +++ b/beacon_node/store/src/hot_cold_store.rs @@ -892,11 +892,11 @@ impl, Cold: ItemStore> HotColdDB // Backtrack until we reach a state that is in the cache, or in the worst case // the finalized state (this should only be reachable on first start-up). - let mut state_root_iter = HotStateRootIter::new(self, slot, *state_root); + let state_root_iter = HotStateRootIter::new(self, slot, *state_root); let mut state_roots = Vec::with_capacity(32); let mut state = None; - while let Some(res) = state_root_iter.next() { + for res in state_root_iter { let (prior_state_root, prior_slot) = res?; state_roots.push(Ok((prior_state_root, prior_slot))); diff --git a/beacon_node/store/src/hot_state_iter.rs b/beacon_node/store/src/hot_state_iter.rs index 51dc917a5c..7d31b3f1a5 100644 --- a/beacon_node/store/src/hot_state_iter.rs +++ b/beacon_node/store/src/hot_state_iter.rs @@ -28,7 +28,7 @@ impl<'a, E: EthSpec, Hot: ItemStore, Cold: ItemStore> HotStateRootIter<'a, let summary = self .store .load_hot_state_summary(&self.next_state_root)? - .ok_or_else(|| HotColdDBError::MissingHotStateSummary(self.next_state_root))?; + .ok_or(HotColdDBError::MissingHotStateSummary(self.next_state_root))?; let slot = self.next_slot; let state_root = self.next_state_root; diff --git a/consensus/state_processing/src/per_block_processing.rs b/consensus/state_processing/src/per_block_processing.rs index 397b2ad671..eca27c82e0 100644 --- a/consensus/state_processing/src/per_block_processing.rs +++ b/consensus/state_processing/src/per_block_processing.rs @@ -119,6 +119,7 @@ pub fn per_block_processing>( |pk_bytes| pk_bytes.decompress().ok().map(Cow::Owned), signed_block, block_root, + false, spec ) .is_ok(), @@ -249,6 +250,7 @@ pub fn verify_block_signature>( |i| get_pubkey_from_state(state, i), block, block_root, + false, spec )? .verify(), diff --git a/consensus/state_processing/src/per_block_processing/block_signature_verifier.rs b/consensus/state_processing/src/per_block_processing/block_signature_verifier.rs index 78205ca92c..8ec16d9e16 100644 --- a/consensus/state_processing/src/per_block_processing/block_signature_verifier.rs +++ b/consensus/state_processing/src/per_block_processing/block_signature_verifier.rs @@ -123,10 +123,11 @@ where decompressor: D, block: &'a SignedBeaconBlock, block_root: Option, + check_proposer_index: bool, spec: &'a ChainSpec, ) -> Result<()> { let mut verifier = Self::new(state, get_pubkey, decompressor, spec); - verifier.include_all_signatures(block, block_root)?; + verifier.include_all_signatures(block, block_root, check_proposer_index)?; verifier.verify() } @@ -135,8 +136,9 @@ where &mut self, block: &'a SignedBeaconBlock, block_root: Option, + check_proposer_index: bool, ) -> Result<()> { - self.include_block_proposal(block, block_root)?; + self.include_block_proposal(block, block_root, check_proposer_index)?; self.include_all_signatures_except_proposal(block)?; Ok(()) @@ -164,12 +166,14 @@ where &mut self, block: &'a SignedBeaconBlock, block_root: Option, + check_proposer_index: bool, ) -> Result<()> { let set = block_proposal_signature_set( self.state, self.get_pubkey.clone(), block, block_root, + check_proposer_index, self.spec, )?; self.sets.push(set); diff --git a/consensus/state_processing/src/per_block_processing/signature_sets.rs b/consensus/state_processing/src/per_block_processing/signature_sets.rs index 73cef3a246..4fe045b503 100644 --- a/consensus/state_processing/src/per_block_processing/signature_sets.rs +++ b/consensus/state_processing/src/per_block_processing/signature_sets.rs @@ -76,6 +76,7 @@ pub fn block_proposal_signature_set<'a, T, F, Payload: ExecPayload>( get_pubkey: F, signed_block: &'a SignedBeaconBlock, block_root: Option, + check_proposer_index: bool, spec: &'a ChainSpec, ) -> Result> where @@ -83,14 +84,20 @@ where F: Fn(usize) -> Option>, { let block = signed_block.message(); - let proposer_index = state.get_beacon_proposer_index(block.slot(), spec)? as u64; - if proposer_index != block.proposer_index() { - return Err(Error::IncorrectBlockProposer { - block: block.proposer_index(), - local_shuffling: proposer_index, - }); - } + let proposer_index = if check_proposer_index { + let proposer_index = state.get_beacon_proposer_index(block.slot(), spec)? as u64; + + if proposer_index != block.proposer_index() { + return Err(Error::IncorrectBlockProposer { + block: block.proposer_index(), + local_shuffling: proposer_index, + }); + } + proposer_index + } else { + block.proposer_index() + }; block_proposal_signature_set_from_parts( signed_block, @@ -162,7 +169,9 @@ where T: EthSpec, F: Fn(usize) -> Option>, { - let proposer_index = state.get_beacon_proposer_index(block.slot(), spec)?; + // FIXME(sproul): ensure this is checked elsewhere + let proposer_index = block.proposer_index() as usize; + // let proposer_index = state.get_beacon_proposer_index(block.slot(), spec)?; let domain = spec.get_domain( block.slot().epoch(T::slots_per_epoch()), diff --git a/consensus/state_processing/src/per_epoch_processing/altair/participation_cache.rs b/consensus/state_processing/src/per_epoch_processing/altair/participation_cache.rs index 8eda0ecc04..52d372bb56 100644 --- a/consensus/state_processing/src/per_epoch_processing/altair/participation_cache.rs +++ b/consensus/state_processing/src/per_epoch_processing/altair/participation_cache.rs @@ -330,6 +330,7 @@ impl ParticipationCache { } } + #[allow(clippy::indexing_slicing)] if is_eligible || is_active_current_epoch { let effective_balance = val.effective_balance; let base_reward = diff --git a/consensus/types/src/beacon_state/committee_cache.rs b/consensus/types/src/beacon_state/committee_cache.rs index 125ac289a7..81e0d0a6f1 100644 --- a/consensus/types/src/beacon_state/committee_cache.rs +++ b/consensus/types/src/beacon_state/committee_cache.rs @@ -42,6 +42,7 @@ pub struct CommitteeCache { /// common entries, and that new entries at the end are all `None`. /// /// In practice this is only used in tests. +#[allow(clippy::indexing_slicing)] fn compare_shuffling_positions(xs: &Vec, ys: &Vec) -> bool { use std::cmp::Ordering; diff --git a/lcli/src/skip_slots.rs b/lcli/src/skip_slots.rs index 28310f7683..125b5c6131 100644 --- a/lcli/src/skip_slots.rs +++ b/lcli/src/skip_slots.rs @@ -55,7 +55,7 @@ use std::fs::File; use std::io::prelude::*; use std::path::PathBuf; use std::time::{Duration, Instant}; -use types::{BeaconState, CloneConfig, EthSpec, Hash256}; +use types::{BeaconState, EthSpec, Hash256}; const HTTP_TIMEOUT: Duration = Duration::from_secs(10); @@ -121,7 +121,7 @@ pub fn run(mut env: Environment, matches: &ArgMatches) -> Result< }; for i in 0..runs { - let mut state = state.clone_with(CloneConfig::committee_caches_only()); + let mut state = state.clone(); let start = Instant::now(); diff --git a/lcli/src/transition_blocks.rs b/lcli/src/transition_blocks.rs index 6159723c98..f0db165f9a 100644 --- a/lcli/src/transition_blocks.rs +++ b/lcli/src/transition_blocks.rs @@ -80,7 +80,7 @@ use std::path::{Path, PathBuf}; use std::sync::Arc; use std::time::{Duration, Instant}; use store::HotColdDB; -use types::{BeaconState, ChainSpec, CloneConfig, EthSpec, Hash256, SignedBeaconBlock}; +use types::{BeaconState, ChainSpec, EthSpec, Hash256, SignedBeaconBlock}; const HTTP_TIMEOUT: Duration = Duration::from_secs(10); @@ -224,7 +224,7 @@ pub fn run(mut env: Environment, matches: &ArgMatches) -> Result< let mut output_post_state = None; for i in 0..runs { - let pre_state = pre_state.clone_with(CloneConfig::all()); + let pre_state = pre_state.clone(); let block = block.clone(); let start = Instant::now(); @@ -286,7 +286,6 @@ pub fn run(mut env: Environment, matches: &ArgMatches) -> Result< } drop(pre_state); - drop(post_state); Ok(()) } @@ -322,7 +321,6 @@ fn do_transition( } state_root_opt = Some(state_root); } - println!("Slot processing: {}ms", t.elapsed().as_millis()); let state_root = state_root_opt.ok_or("Failed to compute state root, internal error")?; @@ -361,6 +359,7 @@ fn do_transition( decompressor, &block, Some(block_root), + false, spec, ) .map_err(|e| format!("Invalid block signature: {:?}", e))?; @@ -368,10 +367,12 @@ fn do_transition( } let t = Instant::now(); + let mut ctxt = ConsensusContext::new(pre_state.slot()) + .set_current_block_root(block_root) + .set_proposer_index(block.message().proposer_index()); per_block_processing( &mut pre_state, &block, - None, BlockSignatureStrategy::NoVerification, VerifyBlockRoot::True, &mut ctxt,