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

AuthorityEngine: Minor cleanups. #11408

Merged
ordian merged 1 commit into from
Jan 27, 2020
Merged

AuthorityEngine: Minor cleanups. #11408

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
202 changes: 107 additions & 95 deletions ethcore/engines/authority-round/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -370,7 +370,6 @@ impl EpochManager {
debug!(target: "engine", "Zooming to epoch after block {}", hash);
trace!(target: "engine", "Current validator set: {:?}", self.validators());


// epoch_transition_for can be an expensive call, but in the absence of
// forks it will only need to be called for the block directly after
// epoch transition, in which case it will be O(1) and require a single
Expand All @@ -390,25 +389,27 @@ impl EpochManager {
let (signal_number, set_proof, _) = destructure_proofs(&last_transition.proof)
.expect("proof produced by this engine; therefore it is valid; qed");

trace!(target: "engine", "extracting epoch validator set for epoch ({}, {}) signalled at #{}",
last_transition.block_number, last_transition.block_hash, signal_number);
trace!(
target: "engine",
"extracting epoch validator set for epoch ({}, {}) signalled at #{}",
last_transition.block_number, last_transition.block_hash, signal_number
);

let first = signal_number == 0;
let epoch_set = validators.epoch_set(
let (list, _) = validators.epoch_set(
first,
machine,
signal_number, // use signal number so multi-set first calculation is correct.
set_proof,
)
.ok()
.map(|(list, _)| {
trace!(target: "engine", "Updating finality checker with new validator set extracted from epoch ({}, {}): {:?}",
last_transition.block_number, last_transition.block_hash, &list);
).expect("proof produced by this engine; therefore it is valid; qed");

list.into_inner()
})
.expect("proof produced by this engine; therefore it is valid; qed");
trace!(
target: "engine",
"Updating finality checker with new validator set extracted from epoch ({}, {}): {:?}",
last_transition.block_number, last_transition.block_hash, &list
);

let epoch_set = list.into_inner();
let two_thirds_majority_transition = self.finality_checker.two_thirds_majority_transition();
self.finality_checker = RollingFinality::blank(epoch_set, two_thirds_majority_transition);
}
Expand All @@ -435,10 +436,22 @@ 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.
///
/// An empty step message is created _instead of_ a block if there are no pending transactions.
/// It cannot itself be a parent, and `parent_hash` always points to the most recent block. E.g.:
/// * Validator A creates block `bA`.
/// * Validator B has no pending transactions, so it signs an empty step message `mB`
/// instead whose hash points to block `bA`.
/// * Validator C also has no pending transactions, so it also signs an empty step message `mC`
/// instead whose hash points to block `bA`.
/// * Validator D creates block `bD`. The parent is block `bA`, and the header includes `mB` and `mC`.
#[derive(Clone, Debug, PartialEq, Eq)]
struct EmptyStep {
/// The signature of the other two fields, by the message's author.
signature: H520,
/// This message's step number.
step: u64,
/// The hash of the most recent block.
Copy link
Copy Markdown
Collaborator

@dvdplm dvdplm Jan 27, 2020

Choose a reason for hiding this comment

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

Is this the hash of the parent to the block where the authority emits the EmptyStep message?

Copy link
Copy Markdown
Contributor Author

@afck afck Jan 27, 2020

Choose a reason for hiding this comment

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

If I understand correctly, an empty step message is created instead of a block, if a validator has no pending transactions. It cannot itself be a parent, and this hash always points to the most recent block.
E.g.:

  • Validator A creates block 1.
  • Validator B has no pending transaction, so it signs an empty step message mB instead whose hash points to block 1.
  • Validator C also has no pending transactions, so it also signs an empty step message mC instead whose hash points to block 1.
  • Validator D creates block 2. The parent is block 1, and the header includes mB and mC.

(Would be good if someone could confirm my understanding!)

Copy link
Copy Markdown
Collaborator

@dvdplm dvdplm Jan 27, 2020

Choose a reason for hiding this comment

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

Oh right, makes sense now. Thank you for explaining.

Copy link
Copy Markdown
Collaborator

@dvdplm dvdplm Jan 27, 2020

Choose a reason for hiding this comment

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

Maybe your excellent comment here could go into the module level docs or the docs for EmptyStep for the benefit of the next clueless code reader? :)

Copy link
Copy Markdown
Contributor Author

@afck afck Jan 27, 2020

Choose a reason for hiding this comment

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

Done.

parent_hash: H256,
}

Expand All @@ -447,6 +460,7 @@ impl PartialOrd for EmptyStep {
Some(self.cmp(other))
}
}

impl Ord for EmptyStep {
fn cmp(&self, other: &Self) -> cmp::Ordering {
self.step.cmp(&other.step)
Expand All @@ -463,6 +477,7 @@ impl EmptyStep {
EmptyStep { signature, step, parent_hash }
}

/// Returns `true` if the message has a valid signature by the expected proposer in the message's step.
fn verify(&self, validators: &dyn ValidatorSet) -> Result<bool, Error> {
let message = keccak(empty_step_rlp(self.step, &self.parent_hash));
let correct_proposer = step_proposer(validators, &self.parent_hash, self.step);
Expand Down Expand Up @@ -773,7 +788,7 @@ fn verify_external(header: &Header, validators: &dyn ValidatorSet, empty_steps_t
}

fn combine_proofs(signal_number: BlockNumber, set_proof: &[u8], finality_proof: &[u8]) -> Vec<u8> {
let mut stream = ::rlp::RlpStream::new_list(3);
let mut stream = RlpStream::new_list(3);
stream.append(&signal_number).append(&set_proof).append(&finality_proof);
stream.out()
}
Expand Down Expand Up @@ -830,30 +845,21 @@ impl AuthorityRound {
let initial_step = our_params.start_step.unwrap_or(0);

let mut durations = Vec::new();
let mut prev_step = 0u64;
let mut prev_time = 0u64;
let mut prev_dur = our_params.step_durations[&0];
durations.push(StepDurationInfo {
transition_step: prev_step,
transition_timestamp: prev_time,
step_duration: prev_dur
});
for (time, dur) in our_params.step_durations.iter().skip(1) {
let (step, time) = next_step_time_duration(
StepDurationInfo{
transition_step: prev_step,
transition_timestamp: prev_time,
step_duration: prev_dur,
}, *time)
.ok_or(BlockError::TimestampOverflow)?;
durations.push(StepDurationInfo {
transition_step: step,
transition_timestamp: time,
step_duration: *dur
});
prev_step = step;
prev_time = time;
prev_dur = *dur;
{
let mut dur_info = StepDurationInfo {
transition_step: 0u64,
transition_timestamp: 0u64,
step_duration: our_params.step_durations[&0],
};
durations.push(dur_info);
for (time, dur) in our_params.step_durations.iter().skip(1) {
let (step, time) = next_step_time_duration(dur_info, *time)
.ok_or(BlockError::TimestampOverflow)?;
dur_info.transition_step = step;
dur_info.transition_timestamp = time;
dur_info.step_duration = *dur;
durations.push(dur_info);
Comment thread
dvdplm marked this conversation as resolved.
}
}

let step = Step {
Expand Down Expand Up @@ -907,13 +913,7 @@ impl AuthorityRound {
(CowLike::Borrowed(&*self.validators), header.number())
} else {
let mut epoch_manager = self.epoch_manager.lock();
let client = match self.client.read().as_ref().and_then(|weak| weak.upgrade()) {
Some(client) => client,
None => {
debug!(target: "engine", "Unable to verify sig: missing client ref.");
return Err(EngineError::RequiresClient.into())
}
};
let client = self.upgrade_client_or("Unable to verify sig")?;

if !epoch_manager.zoom_to_after(&*client, &self.machine, &*self.validators, *header.parent_hash()) {
debug!(target: "engine", "Unable to zoom to epoch.");
Expand Down Expand Up @@ -981,14 +981,19 @@ impl AuthorityRound {
}

fn broadcast_message(&self, message: Vec<u8>) {
if let Some(ref weak) = *self.client.read() {
if let Some(c) = weak.upgrade() {
c.broadcast_consensus_message(message);
}
if let Ok(c) = self.upgrade_client_or(None) {
c.broadcast_consensus_message(message);
}
}

fn report_skipped(&self, header: &Header, current_step: u64, parent_step: u64, validators: &dyn ValidatorSet, set_number: u64) {
fn report_skipped(
&self,
header: &Header,
current_step: u64,
parent_step: u64,
validators: &dyn 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;
Expand All @@ -1004,8 +1009,12 @@ impl AuthorityRound {
if skipped_primary != me {
// Stop reporting once validators start repeating.
if !reported.insert(skipped_primary) { break; }
trace!(target: "engine", "Reporting benign misbehaviour (cause: skipped step) at block #{}, epoch set number {}, step proposer={:#x}. Own address: {}",
header.number(), set_number, skipped_primary, me);
trace!(
target: "engine",
"Reporting benign misbehaviour (cause: skipped step) at block #{}, \
epoch set number {}, step proposer={:#x}. Own address: {}",
header.number(), set_number, skipped_primary, me
);
self.validators.report_benign(&skipped_primary, set_number, header.number());
} else {
trace!(target: "engine", "Primary that skipped is self, not self-reporting. Own address: {}", me);
Expand All @@ -1018,12 +1027,9 @@ impl AuthorityRound {
fn build_finality(&self, chain_head: &Header, ancestry: &mut dyn Iterator<Item=Header>) -> Vec<H256> {
if self.immediate_transitions { return Vec::new() }

let client = match self.client.read().as_ref().and_then(|weak| weak.upgrade()) {
Some(client) => client,
None => {
warn!(target: "engine", "Unable to apply ancestry actions: missing client ref.");
return Vec::new();
}
let client = match self.upgrade_client_or("Unable to apply ancestry actions") {
Ok(client) => client,
Err(_) => return Vec::new(),
};

let mut epoch_manager = self.epoch_manager.lock();
Expand Down Expand Up @@ -1079,7 +1085,7 @@ impl AuthorityRound {
}

fn address(&self) -> Option<Address> {
self.signer.read().as_ref().map(|s| s.address() )
self.signer.read().as_ref().map(|s| s.address())
}

/// Make calls to the randomness contract.
Expand All @@ -1095,10 +1101,7 @@ impl AuthorityRound {
None => return Ok(Vec::new()), // We are not a validator, so we shouldn't call the contracts.
};
let our_addr = signer.address();
let client = self.client.read().as_ref().and_then(|weak| weak.upgrade()).ok_or_else(|| {
debug!(target: "engine", "Unable to prepare block: missing client ref.");
EngineError::RequiresClient
})?;
let client = self.upgrade_client_or("Unable to prepare block")?;
let full_client = client.as_full_client()
Comment thread
dvdplm marked this conversation as resolved.
.ok_or_else(|| EngineError::FailedSystemCall("Failed to upgrade to BlockchainClient.".to_string()))?;

Expand All @@ -1116,6 +1119,18 @@ impl AuthorityRound {
let tx_request = TransactionRequest::call(contract_addr, data).gas_price(U256::zero()).nonce(nonce);
Ok(vec![full_client.create_transaction(tx_request)?])
}

/// Returns the reference to the client, if registered.
fn upgrade_client_or<'a, T>(&self, opt_error_msg: T) -> Result<Arc<dyn EngineClient>, EngineError>
where T: Into<Option<&'a str>>,
{
self.client.read().as_ref().and_then(|weak| weak.upgrade()).ok_or_else(|| {
if let Some(error_msg) = opt_error_msg.into() {
debug!(target: "engine", "{}: missing client ref.", error_msg);
}
EngineError::RequiresClient
})
}
}

fn unix_now() -> Duration {
Expand Down Expand Up @@ -1174,10 +1189,8 @@ impl Engine for AuthorityRound {
fn step(&self) {
self.step.inner.increment();
self.step.can_propose.store(true, AtomicOrdering::SeqCst);
if let Some(ref weak) = *self.client.read() {
if let Some(c) = weak.upgrade() {
c.update_sealing(ForceUpdateSealing::No);
}
if let Ok(c) = self.upgrade_client_or(None) {
c.update_sealing(ForceUpdateSealing::No);
}
}

Expand Down Expand Up @@ -1257,12 +1270,9 @@ impl Engine for AuthorityRound {
}
};

let client = match self.client.read().as_ref().and_then(|weak| weak.upgrade()) {
Some(client) => client,
None => {
warn!(target: "engine", "Not preparing block: missing client ref.");
return SealingState::NotReady;
}
let client = match self.upgrade_client_or("Not preparing block") {
Ok(client) => client,
Err(_) => return SealingState::NotReady,
};

let parent = match client.as_full_client() {
Expand Down Expand Up @@ -1296,7 +1306,7 @@ impl Engine for AuthorityRound {
}

fn handle_message(&self, rlp: &[u8]) -> Result<(), EngineError> {
fn fmt_err<T: ::std::fmt::Debug>(x: T) -> EngineError {
fn fmt_err<T: fmt::Debug>(x: T) -> EngineError {
EngineError::MalformedMessage(format!("{:?}", x))
}

Expand Down Expand Up @@ -1625,8 +1635,12 @@ impl Engine for AuthorityRound {
match validate_empty_steps() {
Ok(len) => len,
Err(err) => {
trace!(target: "engine", "Reporting benign misbehaviour (cause: invalid empty steps) at block #{}, epoch set number {}. Own address: {}",
header.number(), set_number, self.address().unwrap_or_default());
trace!(
target: "engine",
"Reporting benign misbehaviour (cause: invalid empty steps) \
at block #{}, epoch set number {}. Own address: {}",
header.number(), set_number, self.address().unwrap_or_default()
);
self.validators.report_benign(header.author(), set_number, header.number());
return Err(err);
},
Expand All @@ -1640,7 +1654,10 @@ impl Engine for AuthorityRound {
if header.number() >= self.validate_score_transition {
let expected_difficulty = calculate_score(parent_step.into(), step.into(), empty_steps_len.into());
if header.difficulty() != &expected_difficulty {
return Err(From::from(BlockError::InvalidDifficulty(Mismatch { expected: expected_difficulty, found: header.difficulty().clone() })));
return Err(From::from(BlockError::InvalidDifficulty(Mismatch {
expected: expected_difficulty,
found: header.difficulty().clone()
})));
}
}

Expand All @@ -1656,7 +1673,10 @@ impl Engine for AuthorityRound {
let res = verify_external(header, &*validators, self.empty_steps_transition);
match res {
Err(Error::Engine(EngineError::NotProposer(_))) => {
trace!(target: "engine", "Reporting benign misbehaviour (cause: block from incorrect proposer) at block #{}, epoch set number {}. Own address: {}",
trace!(
target: "engine",
"Reporting benign misbehaviour (cause: block from incorrect proposer) \
at block #{}, epoch set number {}. Own address: {}",
header.number(), set_number, self.address().unwrap_or_default());
self.validators.report_benign(header.author(), set_number, header.number());
},
Expand Down Expand Up @@ -1692,13 +1712,7 @@ impl Engine for AuthorityRound {
if self.immediate_transitions { return None }

let epoch_transition_hash = {
let client = match self.client.read().as_ref().and_then(|weak| weak.upgrade()) {
Some(client) => client,
None => {
warn!(target: "engine", "Unable to check for epoch end: missing client ref.");
return None;
}
};
let client = self.upgrade_client_or("Unable to check for epoch end").ok()?;

let mut epoch_manager = self.epoch_manager.lock();
if !epoch_manager.zoom_to_after(&*client, &self.machine, &*self.validators, *chain_head.parent_hash()) {
Expand All @@ -1710,7 +1724,7 @@ impl Engine for AuthorityRound {

let mut hash = *chain_head.parent_hash();

let mut ancestry = std::iter::repeat_with(move || {
let mut ancestry = iter::repeat_with(move || {
chain(hash).and_then(|header| {
if header.number() == 0 { return None }
hash = *header.parent_hash();
Expand Down Expand Up @@ -1739,7 +1753,11 @@ impl Engine for AuthorityRound {

// Apply transitions that don't require finality and should be enacted immediately (e.g from chain spec)
if let Some(change) = self.validators.is_epoch_end(first, chain_head) {
info!(target: "engine", "Immediately applying validator set change signalled at block {}", chain_head.number());
info!(
target: "engine",
"Immediately applying validator set change signalled at block {}",
chain_head.number()
);
self.epoch_manager.lock().note_new_epoch();
let change = combine_proofs(chain_head.number(), &change, &[]);
return Some(change)
Expand All @@ -1752,7 +1770,7 @@ impl Engine for AuthorityRound {
// to construct transition proof. author == ec_recover(sig) known
// since the blocks are in the DB.
let mut hash = chain_head.hash();
let mut finality_proof: Vec<_> = std::iter::repeat_with(move || {
let mut finality_proof: Vec<_> = iter::repeat_with(move || {
chain(hash).and_then(|header| {
hash = *header.parent_hash();
if header.number() == 0 { None }
Expand Down Expand Up @@ -1865,13 +1883,7 @@ impl Engine for AuthorityRound {

fn gas_limit_override(&self, header: &Header) -> Option<U256> {
let (_, &address) = self.block_gas_limit_contract_transitions.range(..=header.number()).last()?;
let client = match self.client.read().as_ref().and_then(|weak| weak.upgrade()) {
Some(client) => client,
None => {
error!(target: "engine", "Unable to prepare block: missing client ref.");
return None;
}
};
let client = self.upgrade_client_or("Unable to prepare block").ok()?;
let full_client = match client.as_full_client() {
Some(full_client) => full_client,
None => {
Expand Down Expand Up @@ -2392,7 +2404,7 @@ mod tests {
]);
}

fn assert_insufficient_proof<T: ::std::fmt::Debug>(result: Result<T, Error>, contains: &str) {
fn assert_insufficient_proof<T: std::fmt::Debug>(result: Result<T, Error>, contains: &str) {
match result {
Err(Error::Engine(EngineError::InsufficientProof(ref s))) =>{
assert!(s.contains(contains), "Expected {:?} to contain {:?}", s, contains);
Expand Down