From 35a2b87174f031ea0e34b2074ebf6d14340b8f65 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomasz=20Drwi=C4=99ga?= Date: Wed, 21 Nov 2018 21:30:03 +0000 Subject: [PATCH] Fix empty steps (#9939) * Don't send empty step twice or empty step then block. * Perform basic validation of locally sealed blocks. * Don't include empty step twice. --- ethcore/src/client/client.rs | 22 ++- ethcore/src/engines/authority_round/mod.rs | 190 +++++++++++++++------ 2 files changed, 154 insertions(+), 58 deletions(-) diff --git a/ethcore/src/client/client.rs b/ethcore/src/client/client.rs index 8412881a2..d5ae9d80f 100644 --- a/ethcore/src/client/client.rs +++ b/ethcore/src/client/client.rs @@ -2287,26 +2287,34 @@ impl ScheduleInfo for Client { impl ImportSealedBlock for Client { fn import_sealed_block(&self, block: SealedBlock) -> EthcoreResult { - let h = block.header().hash(); let start = Instant::now(); + let header = block.header().clone(); let route = { + // Do a super duper basic verification to detect potential bugs + if let Err(e) = self.engine.verify_block_basic(&header) { + self.importer.bad_blocks.report( + block.rlp_bytes(), + format!("Detected an issue with locally sealed block: {}", e), + ); + return Err(e.into()); + } + // scope for self.import_lock let _import_lock = self.importer.import_lock.lock(); trace_time!("import_sealed_block"); - let number = block.header().number(); let block_data = block.rlp_bytes(); - let header = block.header().clone(); let route = self.importer.commit_block(block, &header, encoded::Block::new(block_data), self); - trace!(target: "client", "Imported sealed block #{} ({})", number, h); + trace!(target: "client", "Imported sealed block #{} ({})", header.number(), header.hash()); self.state_db.write().sync_cache(&route.enacted, &route.retracted, false); route }; + let h = header.hash(); let route = ChainRoute::from([route].as_ref()); self.importer.miner.chain_new_blocks( self, - &[h.clone()], + &[h], &[], route.enacted(), route.retracted(), @@ -2314,10 +2322,10 @@ impl ImportSealedBlock for Client { ); self.notify(|notify| { notify.new_blocks( - vec![h.clone()], + vec![h], vec![], route.clone(), - vec![h.clone()], + vec![h], vec![], start.elapsed(), ); diff --git a/ethcore/src/engines/authority_round/mod.rs b/ethcore/src/engines/authority_round/mod.rs index 1a54ccec1..bf404b476 100644 --- a/ethcore/src/engines/authority_round/mod.rs +++ b/ethcore/src/engines/authority_round/mod.rs @@ -16,8 +16,8 @@ //! A blockchain engine that supports a non-instant BFT proof-of-authority. -use std::collections::{BTreeMap, HashSet}; -use std::fmt; +use std::collections::{BTreeMap, BTreeSet, HashSet}; +use std::{cmp, fmt}; use std::iter::FromIterator; use std::ops::Deref; use std::sync::atomic::{AtomicUsize, AtomicBool, Ordering as AtomicOrdering}; @@ -123,10 +123,10 @@ struct Step { } impl Step { - fn load(&self) -> usize { self.inner.load(AtomicOrdering::SeqCst) } + fn load(&self) -> u64 { self.inner.load(AtomicOrdering::SeqCst) as u64 } fn duration_remaining(&self) -> Duration { let now = unix_now(); - let expected_seconds = (self.load() as u64) + let expected_seconds = self.load() .checked_add(1) .and_then(|ctr| ctr.checked_mul(self.duration as u64)) .map(Duration::from_secs); @@ -162,8 +162,8 @@ impl Step { } } - fn check_future(&self, given: usize) -> Result<(), Option>> { - const REJECTED_STEP_DRIFT: usize = 4; + fn check_future(&self, given: u64) -> Result<(), Option>> { + const REJECTED_STEP_DRIFT: u64 = 4; // Verify if the step is correct. if given <= self.load() { @@ -182,8 +182,8 @@ impl Step { let d = self.duration as u64; Err(Some(OutOfBounds { min: None, - max: Some(d * current as u64), - found: d * given as u64, + max: Some(d * current), + found: d * given, })) } else { Ok(()) @@ -192,8 +192,8 @@ impl Step { } // Chain scoring: total weight is sqrt(U256::max_value())*height - step -fn calculate_score(parent_step: U256, current_step: U256, current_empty_steps: U256) -> U256 { - U256::from(U128::max_value()) + parent_step - current_step + current_empty_steps +fn calculate_score(parent_step: u64, current_step: u64, current_empty_steps: usize) -> U256 { + U256::from(U128::max_value()) + U256::from(parent_step) - U256::from(current_step) + U256::from(current_empty_steps) } struct EpochManager { @@ -284,13 +284,26 @@ impl EpochManager { /// A message broadcast by authorities when it's their turn to seal a block but there are no /// transactions. Other authorities accumulate these messages and later include them in the seal as /// proof. -#[derive(Clone, Debug)] +#[derive(Clone, Debug, PartialEq, Eq)] struct EmptyStep { signature: H520, - step: usize, + step: u64, parent_hash: H256, } +impl PartialOrd for EmptyStep { + fn partial_cmp(&self, other: &Self) -> Option { + Some(self.cmp(other)) + } +} +impl Ord for EmptyStep { + fn cmp(&self, other: &Self) -> cmp::Ordering { + self.step.cmp(&other.step) + .then_with(|| self.parent_hash.cmp(&other.parent_hash)) + .then_with(|| self.signature.cmp(&other.signature)) + } +} + impl EmptyStep { fn from_sealed(sealed_empty_step: SealedEmptyStep, parent_hash: &H256) -> EmptyStep { let signature = sealed_empty_step.signature; @@ -353,7 +366,7 @@ pub fn empty_step_full_rlp(signature: &H520, empty_step_rlp: &[u8]) -> Vec { s.out() } -pub fn empty_step_rlp(step: usize, parent_hash: &H256) -> Vec { +pub fn empty_step_rlp(step: u64, parent_hash: &H256) -> Vec { let mut s = RlpStream::new_list(2); s.append(&step).append(parent_hash); s.out() @@ -365,7 +378,7 @@ pub fn empty_step_rlp(step: usize, parent_hash: &H256) -> Vec { /// empty message is included. struct SealedEmptyStep { signature: H520, - step: usize, + step: u64, } impl Encodable for SealedEmptyStep { @@ -399,7 +412,7 @@ pub struct AuthorityRound { validators: Box, validate_score_transition: u64, validate_step_transition: u64, - empty_steps: Mutex>, + empty_steps: Mutex>, epoch_manager: Mutex, immediate_transitions: bool, block_reward: U256, @@ -494,7 +507,7 @@ fn header_expected_seal_fields(header: &Header, empty_steps_transition: u64) -> } } -fn header_step(header: &Header, empty_steps_transition: u64) -> Result { +fn header_step(header: &Header, empty_steps_transition: u64) -> Result { let expected_seal_fields = header_expected_seal_fields(header, empty_steps_transition); Rlp::new(&header.seal().get(0).expect( &format!("was either checked with verify_block_basic or is genesis; has {} fields; qed (Make sure the spec file has a correct genesis seal)", expected_seal_fields))).as_val() @@ -533,17 +546,17 @@ fn header_empty_steps_signers(header: &Header, empty_steps_transition: u64) -> R } } -fn step_proposer(validators: &ValidatorSet, bh: &H256, step: usize) -> Address { - let proposer = validators.get(bh, step); +fn step_proposer(validators: &ValidatorSet, bh: &H256, step: u64) -> Address { + let proposer = validators.get(bh, step as usize); trace!(target: "engine", "Fetched proposer for step {}: {}", step, proposer); proposer } -fn is_step_proposer(validators: &ValidatorSet, bh: &H256, step: usize, address: &Address) -> bool { +fn is_step_proposer(validators: &ValidatorSet, bh: &H256, step: u64, address: &Address) -> bool { step_proposer(validators, bh, step) == *address } -fn verify_timestamp(step: &Step, header_step: usize) -> Result<(), BlockError> { +fn verify_timestamp(step: &Step, header_step: u64) -> Result<(), BlockError> { match step.check_future(header_step) { Err(None) => { trace!(target: "engine", "verify_timestamp: block from the future"); @@ -564,7 +577,7 @@ fn verify_external(header: &Header, validators: &ValidatorSet, empty_steps_trans let header_step = header_step(header, empty_steps_transition)?; let proposer_signature = header_signature(header, empty_steps_transition)?; - let correct_proposer = validators.get(header.parent_hash(), header_step); + let correct_proposer = validators.get(header.parent_hash(), header_step as usize); let is_invalid_proposer = *header.author() != correct_proposer || { let empty_steps_rlp = if header.number() >= empty_steps_transition { Some(header_empty_steps_raw(header)) @@ -634,13 +647,13 @@ impl AuthorityRound { panic!("authority_round: step duration can't be zero") } let should_timeout = our_params.start_step.is_none(); - let initial_step = our_params.start_step.unwrap_or_else(|| (unix_now().as_secs() / (our_params.step_duration as u64))) as usize; + let initial_step = our_params.start_step.unwrap_or_else(|| (unix_now().as_secs() / (our_params.step_duration as u64))); let engine = Arc::new( AuthorityRound { transition_service: IoService::<()>::start()?, step: Arc::new(PermissionedStep { inner: Step { - inner: AtomicUsize::new(initial_step), + inner: AtomicUsize::new(initial_step as usize), calibrate: our_params.start_step.is_none(), duration: our_params.step_duration, }, @@ -651,7 +664,7 @@ impl AuthorityRound { validators: our_params.validators, validate_score_transition: our_params.validate_score_transition, validate_step_transition: our_params.validate_step_transition, - empty_steps: Mutex::new(Vec::new()), + empty_steps: Default::default(), epoch_manager: Mutex::new(EpochManager::blank()), immediate_transitions: our_params.immediate_transitions, block_reward: our_params.block_reward, @@ -699,22 +712,41 @@ impl AuthorityRound { }) } - fn empty_steps(&self, from_step: U256, to_step: U256, parent_hash: H256) -> Vec { - self.empty_steps.lock().iter().filter(|e| { - U256::from(e.step) > from_step && - U256::from(e.step) < to_step && - e.parent_hash == parent_hash - }).cloned().collect() + fn empty_steps(&self, from_step: u64, to_step: u64, parent_hash: H256) -> Vec { + let from = EmptyStep { + step: from_step + 1, + parent_hash, + signature: Default::default(), + }; + let to = EmptyStep { + step: to_step, + parent_hash: Default::default(), + signature: Default::default(), + }; + + if from >= to { + return vec![]; + } + + self.empty_steps.lock() + .range(from..to) + .filter(|e| e.parent_hash == parent_hash) + .cloned() + .collect() } - fn clear_empty_steps(&self, step: U256) { + fn clear_empty_steps(&self, step: u64) { // clear old `empty_steps` messages - self.empty_steps.lock().retain(|e| U256::from(e.step) > step); + let mut empty_steps = self.empty_steps.lock(); + *empty_steps = empty_steps.split_off(&EmptyStep { + step: step + 1, + parent_hash: Default::default(), + signature: Default::default(), + }); } fn handle_empty_step_message(&self, empty_step: EmptyStep) { - let mut empty_steps = self.empty_steps.lock(); - empty_steps.push(empty_step); + self.empty_steps.lock().insert(empty_step); } fn generate_empty_step(&self, parent_hash: &H256) { @@ -744,7 +776,7 @@ impl AuthorityRound { } } - fn report_skipped(&self, header: &Header, current_step: usize, parent_step: usize, validators: &ValidatorSet, set_number: u64) { + fn report_skipped(&self, header: &Header, current_step: u64, parent_step: u64, validators: &ValidatorSet, set_number: u64) { // we're building on top of the genesis block so don't report any skipped steps if header.number() == 1 { return; @@ -937,12 +969,12 @@ impl Engine for AuthorityRound { let current_step = self.step.inner.load(); let current_empty_steps_len = if header.number() >= self.empty_steps_transition { - self.empty_steps(parent_step.into(), current_step.into(), parent.hash()).len() + self.empty_steps(parent_step, current_step, parent.hash()).len() } else { 0 }; - let score = calculate_score(parent_step.into(), current_step.into(), current_empty_steps_len.into()); + let score = calculate_score(parent_step, current_step, current_empty_steps_len); header.set_difficulty(score); } @@ -986,8 +1018,8 @@ impl Engine for AuthorityRound { } let header = block.header(); - let parent_step: U256 = header_step(parent, self.empty_steps_transition) - .expect("Header has been verified; qed").into(); + let parent_step = header_step(parent, self.empty_steps_transition) + .expect("Header has been verified; qed"); let step = self.step.inner.load(); @@ -1022,7 +1054,7 @@ impl Engine for AuthorityRound { if is_step_proposer(&*validators, header.parent_hash(), step, header.author()) { // this is guarded against by `can_propose` unless the block was signed // on the same step (implies same key) and on a different node. - if parent_step == step.into() { + if parent_step == step { warn!("Attempted to seal block on the same step as parent. Is this authority sealing with more than one node?"); return Seal::None; } @@ -1034,7 +1066,10 @@ impl Engine for AuthorityRound { block.transactions().is_empty() && empty_steps.len() < self.maximum_empty_steps { - self.generate_empty_step(header.parent_hash()); + if self.step.can_propose.compare_and_swap(true, false, AtomicOrdering::SeqCst) { + self.generate_empty_step(header.parent_hash()); + } + return Seal::None; } @@ -1058,7 +1093,7 @@ impl Engine for AuthorityRound { // report any skipped primaries between the parent block and // the block we're sealing, unless we have empty steps enabled if header.number() < self.empty_steps_transition { - self.report_skipped(header, step, u64::from(parent_step) as usize, &*validators, set_number); + self.report_skipped(header, step, parent_step, &*validators, set_number); } let mut fields = vec![ @@ -1593,12 +1628,12 @@ mod tests { // Two validators. // Spec starts with step 2. - header.set_difficulty(calculate_score(U256::from(0), U256::from(2), U256::zero())); + header.set_difficulty(calculate_score(0, 2, 0)); let signature = tap.sign(addr, Some("0".into()), header.bare_hash()).unwrap(); header.set_seal(vec![encode(&2usize), encode(&(&*signature as &[u8]))]); assert!(engine.verify_block_family(&header, &parent_header).is_ok()); assert!(engine.verify_block_external(&header).is_err()); - header.set_difficulty(calculate_score(U256::from(0), U256::from(1), U256::zero())); + header.set_difficulty(calculate_score(0, 1, 0)); let signature = tap.sign(addr, Some("0".into()), header.bare_hash()).unwrap(); header.set_seal(vec![encode(&1usize), encode(&(&*signature as &[u8]))]); assert!(engine.verify_block_family(&header, &parent_header).is_ok()); @@ -1622,7 +1657,7 @@ mod tests { // Two validators. // Spec starts with step 2. - header.set_difficulty(calculate_score(U256::from(0), U256::from(1), U256::zero())); + header.set_difficulty(calculate_score(0, 1, 0)); let signature = tap.sign(addr, Some("0".into()), header.bare_hash()).unwrap(); header.set_seal(vec![encode(&1usize), encode(&(&*signature as &[u8]))]); assert!(engine.verify_block_family(&header, &parent_header).is_ok()); @@ -1650,10 +1685,10 @@ mod tests { // Two validators. // Spec starts with step 2. header.set_seal(vec![encode(&5usize), encode(&(&*signature as &[u8]))]); - header.set_difficulty(calculate_score(U256::from(4), U256::from(5), U256::zero())); + header.set_difficulty(calculate_score(4, 5, 0)); assert!(engine.verify_block_family(&header, &parent_header).is_ok()); header.set_seal(vec![encode(&3usize), encode(&(&*signature as &[u8]))]); - header.set_difficulty(calculate_score(U256::from(4), U256::from(3), U256::zero())); + header.set_difficulty(calculate_score(4, 3, 0)); assert!(engine.verify_block_family(&header, &parent_header).is_err()); } @@ -1687,7 +1722,7 @@ mod tests { parent_header.set_seal(vec![encode(&1usize)]); parent_header.set_gas_limit("222222".parse::().unwrap()); let mut header: Header = Header::default(); - header.set_difficulty(calculate_score(U256::from(1), U256::from(3), U256::zero())); + header.set_difficulty(calculate_score(1, 3, 0)); header.set_gas_limit("222222".parse::().unwrap()); header.set_seal(vec![encode(&3usize)]); @@ -1801,14 +1836,14 @@ mod tests { (spec, tap, accounts) } - fn empty_step(engine: &EthEngine, step: usize, parent_hash: &H256) -> EmptyStep { + fn empty_step(engine: &EthEngine, step: u64, parent_hash: &H256) -> EmptyStep { let empty_step_rlp = super::empty_step_rlp(step, parent_hash); let signature = engine.sign(keccak(&empty_step_rlp)).unwrap().into(); let parent_hash = parent_hash.clone(); EmptyStep { step, signature, parent_hash } } - fn sealed_empty_step(engine: &EthEngine, step: usize, parent_hash: &H256) -> SealedEmptyStep { + fn sealed_empty_step(engine: &EthEngine, step: u64, parent_hash: &H256) -> SealedEmptyStep { let empty_step_rlp = super::empty_step_rlp(step, parent_hash); let signature = engine.sign(keccak(&empty_step_rlp)).unwrap().into(); SealedEmptyStep { signature, step } @@ -1844,6 +1879,11 @@ mod tests { // we've received the message assert!(notify.messages.read().contains(&empty_step_rlp)); + let len = notify.messages.read().len(); + + // make sure that we don't generate empty step for the second time + assert_eq!(engine.generate_seal(b1.block(), &genesis_header), Seal::None); + assert_eq!(len, notify.messages.read().len()); } #[test] @@ -2058,7 +2098,7 @@ mod tests { let empty_step3 = sealed_empty_step(engine, 3, &parent_header.hash()); let empty_steps = vec![empty_step2, empty_step3]; - header.set_difficulty(calculate_score(U256::from(0), U256::from(4), U256::from(2))); + header.set_difficulty(calculate_score(0, 4, 2)); let signature = tap.sign(addr1, Some("1".into()), header.bare_hash()).unwrap(); header.set_seal(vec![ encode(&4usize), @@ -2173,4 +2213,52 @@ mod tests { BTreeMap::default(), ); } + + #[test] + fn test_empty_steps() { + let last_benign = Arc::new(AtomicUsize::new(0)); + let params = AuthorityRoundParams { + step_duration: 4, + start_step: Some(1), + validators: Box::new(TestSet::new(Default::default(), last_benign.clone())), + validate_score_transition: 0, + validate_step_transition: 0, + immediate_transitions: true, + maximum_uncle_count_transition: 0, + maximum_uncle_count: 0, + empty_steps_transition: 0, + maximum_empty_steps: 10, + block_reward: Default::default(), + block_reward_contract_transition: 0, + block_reward_contract: Default::default(), + }; + + let mut c_params = ::spec::CommonParams::default(); + c_params.gas_limit_bound_divisor = 5.into(); + let machine = ::machine::EthereumMachine::regular(c_params, Default::default()); + let engine = AuthorityRound::new(params, machine).unwrap(); + + + let parent_hash: H256 = 1.into(); + let signature = H520::default(); + let step = |step: u64| EmptyStep { + step, + parent_hash, + signature, + }; + + engine.handle_empty_step_message(step(1)); + engine.handle_empty_step_message(step(3)); + engine.handle_empty_step_message(step(2)); + engine.handle_empty_step_message(step(1)); + + assert_eq!(engine.empty_steps(0, 4, parent_hash), vec![step(1), step(2), step(3)]); + assert_eq!(engine.empty_steps(2, 3, parent_hash), vec![]); + assert_eq!(engine.empty_steps(2, 4, parent_hash), vec![step(3)]); + + engine.clear_empty_steps(2); + + assert_eq!(engine.empty_steps(0, 3, parent_hash), vec![]); + assert_eq!(engine.empty_steps(0, 4, parent_hash), vec![step(3)]); + } }