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.
This commit is contained in:
Tomasz Drwięga 2018-11-21 21:30:03 +00:00 committed by Andrew Jones
parent 664bb2becd
commit 35a2b87174
2 changed files with 154 additions and 58 deletions

View File

@ -2287,26 +2287,34 @@ impl ScheduleInfo for Client {
impl ImportSealedBlock for Client {
fn import_sealed_block(&self, block: SealedBlock) -> EthcoreResult<H256> {
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(),
);

View File

@ -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<OutOfBounds<u64>>> {
const REJECTED_STEP_DRIFT: usize = 4;
fn check_future(&self, given: u64) -> Result<(), Option<OutOfBounds<u64>>> {
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<cmp::Ordering> {
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<u8> {
s.out()
}
pub fn empty_step_rlp(step: usize, parent_hash: &H256) -> Vec<u8> {
pub fn empty_step_rlp(step: u64, parent_hash: &H256) -> Vec<u8> {
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<u8> {
/// 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<ValidatorSet>,
validate_score_transition: u64,
validate_step_transition: u64,
empty_steps: Mutex<Vec<EmptyStep>>,
empty_steps: Mutex<BTreeSet<EmptyStep>>,
epoch_manager: Mutex<EpochManager>,
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<usize, ::rlp::DecoderError> {
fn header_step(header: &Header, empty_steps_transition: u64) -> Result<u64, ::rlp::DecoderError> {
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<EmptyStep> {
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<EmptyStep> {
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<EthereumMachine> 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<EthereumMachine> 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<EthereumMachine> 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<EthereumMachine> 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<EthereumMachine> 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::<U256>().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::<U256>().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)]);
}
}