Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

Fix aura difficulty race #7198

Merged
merged 5 commits into from
Dec 7, 2017
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

56 changes: 50 additions & 6 deletions ethcore/src/engines/authority_round/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -450,20 +450,29 @@ impl Engine<EthereumMachine> for AuthorityRound {
}

fn seals_internally(&self) -> Option<bool> {
// TODO: accept a `&Call` here so we can query the validator set.
Some(self.signer.read().is_some())
}

/// Attempt to seal the block internally.
///
/// This operation is synchronous and may (quite reasonably) not be available, in which case
/// `Seal::None` will be returned.
fn generate_seal(&self, block: &ExecutedBlock) -> Seal {
fn generate_seal(&self, block: &ExecutedBlock, parent: &Header) -> Seal {
// first check to avoid generating signature most of the time
// (but there's still a race to the `compare_and_swap`)
if !self.can_propose.load(AtomicOrdering::SeqCst) { return Seal::None; }

let header = block.header();
let parent_step: U256 = header_step(parent)
.expect("Header has been verified; qed").into();

let step = self.step.load();
let expected_diff = U256::from(U128::max_value()) + parent_step - step.into();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like the difficulty calcution is already copy pasted in couple of places, maybe we should extract it into a helper function?


if header.difficulty() != &expected_diff {
return Seal::None;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make sense to seal and release a block with a previous step instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This case can also only happen when update_sealing is called upon import of new best block from the prior step, but update_sealing also gets called when the step changes. So triggering sealing again for the correct step is unnecessary.

}

// fetch correct validator set for current epoch, taking into account
// finality of previous transitions.
Expand Down Expand Up @@ -505,6 +514,7 @@ impl Engine<EthereumMachine> for AuthorityRound {
trace!(target: "engine", "generate_seal: {} not a proposer for step {}.",
header.author(), step);
}

Seal::None
}

Expand Down Expand Up @@ -546,7 +556,7 @@ impl Engine<EthereumMachine> for AuthorityRound {
}

/// Check the number of seal fields.
fn verify_block_basic(&self, header: &Header,) -> Result<(), Error> {
fn verify_block_basic(&self, header: &Header) -> Result<(), Error> {
if header.number() >= self.validate_score_transition && *header.difficulty() >= U256::from(U128::max_value()) {
Err(From::from(BlockError::DifficultyOutOfBounds(
OutOfBounds { min: None, max: Some(U256::from(U128::max_value())), found: *header.difficulty() }
Expand Down Expand Up @@ -857,17 +867,51 @@ mod tests {
let b2 = b2.close_and_lock();

engine.set_signer(tap.clone(), addr1, "1".into());
if let Seal::Regular(seal) = engine.generate_seal(b1.block()) {
if let Seal::Regular(seal) = engine.generate_seal(b1.block(), &genesis_header) {
assert!(b1.clone().try_seal(engine, seal).is_ok());
// Second proposal is forbidden.
assert!(engine.generate_seal(b1.block()) == Seal::None);
assert!(engine.generate_seal(b1.block(), &genesis_header) == Seal::None);
}

engine.set_signer(tap, addr2, "2".into());
if let Seal::Regular(seal) = engine.generate_seal(b2.block()) {
if let Seal::Regular(seal) = engine.generate_seal(b2.block(), &genesis_header) {
assert!(b2.clone().try_seal(engine, seal).is_ok());
// Second proposal is forbidden.
assert!(engine.generate_seal(b2.block()) == Seal::None);
assert!(engine.generate_seal(b2.block(), &genesis_header) == Seal::None);
}
}

#[test]
fn checks_difficulty_in_generate_seal() {
let tap = Arc::new(AccountProvider::transient_provider());
let addr1 = tap.insert_account(keccak("1").into(), "1").unwrap();
let addr2 = tap.insert_account(keccak("0").into(), "0").unwrap();

let spec = Spec::new_test_round();
let engine = &*spec.engine;

let genesis_header = spec.genesis_header();
let db1 = spec.ensure_db_good(get_temp_state_db(), &Default::default()).unwrap();
let db2 = spec.ensure_db_good(get_temp_state_db(), &Default::default()).unwrap();
let last_hashes = Arc::new(vec![genesis_header.hash()]);

let b1 = OpenBlock::new(engine, Default::default(), false, db1, &genesis_header, last_hashes.clone(), addr1, (3141562.into(), 31415620.into()), vec![], false).unwrap();
let b1 = b1.close_and_lock();
let b2 = OpenBlock::new(engine, Default::default(), false, db2, &genesis_header, last_hashes, addr2, (3141562.into(), 31415620.into()), vec![], false).unwrap();
let b2 = b2.close_and_lock();

engine.set_signer(tap.clone(), addr1, "1".into());
match engine.generate_seal(b1.block(), &genesis_header) {
Seal::None | Seal::Proposal(_) => panic!("wrong seal"),
Seal::Regular(_) => {
engine.step();

engine.set_signer(tap.clone(), addr2, "0".into());
match engine.generate_seal(b2.block(), &genesis_header) {
Seal::Regular(_) | Seal::Proposal(_) => panic!("sealed despite wrong difficulty"),
Seal::None => {}
}
}
}
}

Expand Down
4 changes: 2 additions & 2 deletions ethcore/src/engines/basic_authority.rs
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,7 @@ impl Engine<EthereumMachine> for BasicAuthority {
}

/// Attempt to seal the block internally.
fn generate_seal(&self, block: &ExecutedBlock) -> Seal {
fn generate_seal(&self, block: &ExecutedBlock, _parent: &Header) -> Seal {
let header = block.header();
let author = header.author();
if self.validators.contains(header.parent_hash(), author) {
Expand Down Expand Up @@ -251,7 +251,7 @@ mod tests {
let last_hashes = Arc::new(vec![genesis_header.hash()]);
let b = OpenBlock::new(engine, Default::default(), false, db, &genesis_header, last_hashes, addr, (3141562.into(), 31415620.into()), vec![], false).unwrap();
let b = b.close_and_lock();
if let Seal::Regular(seal) = engine.generate_seal(b.block()) {
if let Seal::Regular(seal) = engine.generate_seal(b.block(), &genesis_header) {
assert!(b.try_seal(engine, seal).is_ok());
}
}
Expand Down
4 changes: 2 additions & 2 deletions ethcore/src/engines/instant_seal.rs
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ impl<M: Machine> Engine<M> for InstantSeal<M>

fn seals_internally(&self) -> Option<bool> { Some(true) }

fn generate_seal(&self, block: &M::LiveBlock) -> Seal {
fn generate_seal(&self, block: &M::LiveBlock, _parent: &M::Header) -> Seal {
if block.transactions().is_empty() { Seal::None } else { Seal::Regular(Vec::new()) }
}

Expand Down Expand Up @@ -72,7 +72,7 @@ mod tests {
let last_hashes = Arc::new(vec![genesis_header.hash()]);
let b = OpenBlock::new(engine, Default::default(), false, db, &genesis_header, last_hashes, Address::default(), (3141562.into(), 31415620.into()), vec![], false).unwrap();
let b = b.close_and_lock();
if let Seal::Regular(seal) = engine.generate_seal(b.block()) {
if let Seal::Regular(seal) = engine.generate_seal(b.block(), &genesis_header) {
assert!(b.try_seal(engine, seal).is_ok());
}
}
Expand Down
2 changes: 1 addition & 1 deletion ethcore/src/engines/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -225,7 +225,7 @@ pub trait Engine<M: Machine>: Sync + Send {
///
/// It is fine to require access to state or a full client for this function, since
/// light clients do not generate seals.
fn generate_seal(&self, _block: &M::LiveBlock) -> Seal { Seal::None }
fn generate_seal(&self, _block: &M::LiveBlock, _parent: &M::Header) -> Seal { Seal::None }

/// Verify a locally-generated seal of a header.
///
Expand Down
4 changes: 2 additions & 2 deletions ethcore/src/engines/tendermint/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -483,7 +483,7 @@ impl Engine<EthereumMachine> for Tendermint {
///
/// This operation is synchronous and may (quite reasonably) not be available, in which case
/// `Seal::None` will be returned.
fn generate_seal(&self, block: &ExecutedBlock) -> Seal {
fn generate_seal(&self, block: &ExecutedBlock, _parent: &Header) -> Seal {
let header = block.header();
let author = header.author();
// Only proposer can generate seal if None was generated.
Expand Down Expand Up @@ -805,7 +805,7 @@ mod tests {
let last_hashes = Arc::new(vec![genesis_header.hash()]);
let b = OpenBlock::new(spec.engine.as_ref(), Default::default(), false, db.boxed_clone(), &genesis_header, last_hashes, proposer, (3141562.into(), 31415620.into()), vec![], false).unwrap();
let b = b.close();
if let Seal::Proposal(seal) = spec.engine.generate_seal(b.block()) {
if let Seal::Proposal(seal) = spec.engine.generate_seal(b.block(), &genesis_header) {
(b, seal)
} else {
panic!()
Expand Down
8 changes: 7 additions & 1 deletion ethcore/src/miner/miner.rs
Original file line number Diff line number Diff line change
Expand Up @@ -534,7 +534,13 @@ impl Miner {
fn seal_and_import_block_internally(&self, chain: &MiningBlockChainClient, block: ClosedBlock) -> bool {
if !block.transactions().is_empty() || self.forced_sealing() || Instant::now() > *self.next_mandatory_reseal.read() {
trace!(target: "miner", "seal_block_internally: attempting internal seal.");
match self.engine.generate_seal(block.block()) {

let parent_header = match chain.block_header(BlockId::Hash(*block.header().parent_hash())) {
Some(hdr) => hdr.decode(),
None => return false,
};

match self.engine.generate_seal(block.block(), &parent_header) {
// Save proposal for later seal submission and broadcast it.
Seal::Proposal(seal) => {
trace!(target: "miner", "Received a Proposal seal.");
Expand Down