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

Tendermint fixes #5415

Merged
merged 7 commits into from
Apr 10, 2017
Merged

Tendermint fixes #5415

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
20bcd58
more resilience
Mar 14, 2017
bacfefd
refactor commit
Apr 2, 2017
a0d1023
Merge branch 'master' into tender-lock
Apr 2, 2017
eea0749
fix proposal broadcast
Apr 6, 2017
f5ac81d
list encoding
Apr 6, 2017
8015b54
address grumbles
Apr 10, 2017
0f2b17c
to into
Apr 10, 2017
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
40 changes: 10 additions & 30 deletions ethcore/src/engines/tendermint/message.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,11 @@ pub fn consensus_view(header: &Header) -> Result<View, ::rlp::DecoderError> {
UntrustedRlp::new(view_rlp.as_slice()).as_val()
}

/// Proposal signature.
pub fn proposal_signature(header: &Header) -> Result<H520, ::rlp::DecoderError> {
UntrustedRlp::new(header.seal().get(1).expect("seal passed basic verification; seal has 3 fields; qed").as_slice()).as_val()
}

impl Message for ConsensusMessage {
type Round = VoteStep;

Expand All @@ -84,34 +89,18 @@ impl ConsensusMessage {

pub fn new_proposal(header: &Header) -> Result<Self, ::rlp::DecoderError> {
Ok(ConsensusMessage {
signature: proposal_signature(header)?,
vote_step: VoteStep::new(header.number() as Height, consensus_view(header)?, Step::Propose),
signature: UntrustedRlp::new(header.seal().get(1).expect("seal passed basic verification; seal has 3 fields; qed").as_slice()).as_val()?,
block_hash: Some(header.bare_hash()),
})
}

pub fn new_commit(proposal: &ConsensusMessage, signature: H520) -> Self {
let mut vote_step = proposal.vote_step.clone();
vote_step.step = Step::Precommit;
ConsensusMessage {
vote_step: vote_step,
block_hash: proposal.block_hash,
signature: signature,
}
}

pub fn verify(&self) -> Result<Address, Error> {
let full_rlp = ::rlp::encode(self);
let block_info = Rlp::new(&full_rlp).at(1);
let public_key = recover(&self.signature.into(), &block_info.as_raw().sha3())?;
Ok(public_to_address(&public_key))
}

pub fn precommit_hash(&self) -> H256 {
let mut vote_step = self.vote_step.clone();
vote_step.step = Step::Precommit;
message_info_rlp(&vote_step, self.block_hash).sha3()
}
}

impl Default for VoteStep {
Expand Down Expand Up @@ -203,6 +192,10 @@ pub fn message_full_rlp(signature: &H520, vote_info: &Bytes) -> Bytes {
s.out()
}

pub fn message_hash(vote_step: VoteStep, block_hash: H256) -> H256 {
message_info_rlp(&vote_step, Some(block_hash)).sha3()
}

#[cfg(test)]
mod tests {
use util::*;
Expand Down Expand Up @@ -294,19 +287,6 @@ mod tests {
);
}

#[test]
fn message_info_from_header() {
let header = Header::default();
let pro = ConsensusMessage {
signature: Default::default(),
vote_step: VoteStep::new(0, 0, Step::Propose),
block_hash: Some(header.bare_hash())
};
let pre = message_info_rlp(&VoteStep::new(0, 0, Step::Precommit), Some(header.bare_hash()));

assert_eq!(pro.precommit_hash(), pre.sha3());
}

#[test]
fn step_ordering() {
assert!(VoteStep::new(10, 123, Step::Precommit) < VoteStep::new(11, 123, Step::Precommit));
Expand Down
164 changes: 86 additions & 78 deletions ethcore/src/engines/tendermint/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,8 @@ pub struct Tendermint {
proposal: RwLock<Option<H256>>,
/// Hash of the proposal parent block.
proposal_parent: RwLock<H256>,
/// Last block proposed by this validator.
last_proposed: RwLock<H256>,
/// Set used to determine the current validators.
validators: Box<ValidatorSet>,
}
Expand All @@ -122,6 +124,7 @@ impl Tendermint {
last_lock: AtomicUsize::new(0),
proposal: RwLock::new(None),
proposal_parent: Default::default(),
last_proposed: Default::default(),
validators: new_validator_set(our_params.validators),
});
let handler = TransitionHandler::new(Arc::downgrade(&engine) as Weak<Engine>, Box::new(our_params.timeouts));
Expand Down Expand Up @@ -196,6 +199,7 @@ impl Tendermint {
self.height.store(new_height, AtomicOrdering::SeqCst);
self.view.store(0, AtomicOrdering::SeqCst);
*self.lock_change.write() = None;
*self.proposal.write() = None;
}

/// Use via step_service to transition steps.
Expand All @@ -206,7 +210,6 @@ impl Tendermint {
*self.step.write() = step;
match step {
Step::Propose => {
*self.proposal.write() = None;
self.update_sealing()
},
Step::Prevote => {
Expand All @@ -230,28 +233,6 @@ impl Tendermint {
},
Step::Commit => {
trace!(target: "engine", "to_step: Commit.");
// Commit the block using a complete signature set.
let view = self.view.load(AtomicOrdering::SeqCst);
let height = self.height.load(AtomicOrdering::SeqCst);
if let Some(block_hash) = *self.proposal.read() {
// Generate seal and remove old votes.
if self.is_signer_proposer(&*self.proposal_parent.read()) {
let proposal_step = VoteStep::new(height, view, Step::Propose);
let precommit_step = VoteStep::new(proposal_step.height, proposal_step.view, Step::Precommit);
if let Some(seal) = self.votes.seal_signatures(proposal_step, precommit_step, &block_hash) {
trace!(target: "engine", "Collected seal: {:?}", seal);
let seal = vec![
::rlp::encode(&view).to_vec(),
::rlp::encode(&seal.proposal).to_vec(),
::rlp::encode_list(&seal.votes).to_vec()
];
self.submit_seal(block_hash, seal);
self.to_next_height(height);
} else {
warn!(target: "engine", "Not enough votes found!");
}
}
}
},
}
}
Expand All @@ -260,8 +241,17 @@ impl Tendermint {
self.validators.contains(&*self.proposal_parent.read(), address)
}

fn is_above_threshold(&self, n: usize) -> bool {
n > self.validators.count(&*self.proposal_parent.read()) * 2/3
fn is_above_threshold(&self, n: usize) -> Result<(), EngineError> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Functions with is_ prefix usually return bool. Maybe check_ or validate_if would be a better name?

let threshold = self.validators.count(&*self.proposal_parent.read()) * 2/3;
Copy link
Collaborator

Choose a reason for hiding this comment

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

That won't really work for a single validator, right? If you ever reach to such state you will never be able to mint any new blocks.

Copy link
Author

Choose a reason for hiding this comment

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

Integer arithmetic, threshold will be 0, so 1 is enough.

if n > threshold {
Ok(())
} else {
Err(EngineError::BadSealFieldSize(OutOfBounds {
min: Some(threshold),
max: None,
found: n
}))
}
}

/// Find the designated for the given view.
Expand Down Expand Up @@ -308,21 +298,21 @@ impl Tendermint {

fn has_enough_any_votes(&self) -> bool {
let step_votes = self.votes.count_round_votes(&VoteStep::new(self.height.load(AtomicOrdering::SeqCst), self.view.load(AtomicOrdering::SeqCst), *self.step.read()));
self.is_above_threshold(step_votes)
self.is_above_threshold(step_votes).is_ok()
}

fn has_enough_future_step_votes(&self, vote_step: &VoteStep) -> bool {
if vote_step.view > self.view.load(AtomicOrdering::SeqCst) {
let step_votes = self.votes.count_round_votes(vote_step);
self.is_above_threshold(step_votes)
self.is_above_threshold(step_votes).is_ok()
} else {
false
}
}

fn has_enough_aligned_votes(&self, message: &ConsensusMessage) -> bool {
let aligned_count = self.votes.count_aligned_votes(&message);
self.is_above_threshold(aligned_count)
self.is_above_threshold(aligned_count).is_ok()
}

fn handle_valid_message(&self, message: &ConsensusMessage) {
Expand All @@ -337,18 +327,32 @@ impl Tendermint {
&& self.has_enough_aligned_votes(message);
if lock_change {
trace!(target: "engine", "handle_valid_message: Lock change.");
*self.lock_change.write() = Some(message.clone());
*self.lock_change.write() = Some(message.clone());
}
// Check if it can affect the step transition.
if self.is_height(message) {
let next_step = match *self.step.read() {
Step::Precommit if message.block_hash.is_none() && self.has_enough_aligned_votes(message) => {
self.increment_view(1);
Some(Step::Propose)
},
Step::Precommit if self.has_enough_aligned_votes(message) => {
if message.block_hash.is_none() {
self.increment_view(1);
Some(Step::Propose)
} else {
Some(Step::Commit)
let bh = message.block_hash.expect("previous guard ensures is_some; qed");
if *self.last_proposed.read() == bh {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are self.step and self.last_proposed always locked in the same order?

Copy link
Author

Choose a reason for hiding this comment

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

They are never write in the same context.

// Commit the block using a complete signature set.
// Generate seal and remove old votes.
let precommits = self.votes.round_signatures(vote_step, &bh);
trace!(target: "engine", "Collected seal: {:?}", precommits);
let seal = vec![
::rlp::encode(&vote_step.view).to_vec(),
::rlp::NULL_RLP.to_vec(),
::rlp::encode_list(&precommits).to_vec()
];
self.submit_seal(bh, seal);
self.votes.throw_out_old(&vote_step);
}
self.to_next_height(self.height.load(AtomicOrdering::SeqCst));
Some(Step::Commit)
},
Step::Precommit if self.has_enough_future_step_votes(&vote_step) => {
self.increment_view(vote_step.view - self.view.load(AtomicOrdering::SeqCst));
Expand Down Expand Up @@ -442,6 +446,8 @@ impl Engine for Tendermint {
// Insert Propose vote.
debug!(target: "engine", "Submitting proposal {} at height {} view {}.", header.bare_hash(), height, view);
self.votes.vote(ConsensusMessage::new(signature, height, view, Step::Propose, bh), author);
// Remember the owned block.
*self.last_proposed.write() = header.bare_hash();
// Remember proposal for later seal submission.
*self.proposal.write() = bh;
*self.proposal_parent.write() = header.parent_hash().clone();
Expand Down Expand Up @@ -491,14 +497,15 @@ impl Engine for Tendermint {
fn verify_block_basic(&self, header: &Header, _block: Option<&[u8]>) -> Result<(), Error> {
let seal_length = header.seal().len();
if seal_length == self.seal_fields() {
let proposal_len = header.seal()[1].len();
let signatures_len = header.seal()[2].len();
if signatures_len >= 1 {
if (proposal_len == 1) ^ (signatures_len == 1) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't really get that change here.
So the block is valid either if:

  1. there is one proposal and the number of signatures is not one (so 0 or 2 sigs is ok)
  2. or there is one signature and the number of proposals is not one (so 0 or 2 proposals is ok),

Why is that correct? (Shouldn't it be either (a proposal and 0 sigs) or (a signature and 0 proposals)?)

Copy link
Author

Choose a reason for hiding this comment

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

This is sort of wonky, since its just the length of the seal field (1 means no signatures). Will change it to comparing actual RLPs.

Copy link
Collaborator

Choose a reason for hiding this comment

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

!= instead of ^ reads better for bools

Ok(())
} else {
Err(From::from(EngineError::BadSealFieldSize(OutOfBounds {
min: Some(1),
max: None,
found: signatures_len
max: Some(1),
found: proposal_len
})))
}
} else {
Expand All @@ -515,50 +522,45 @@ impl Engine for Tendermint {

/// Verify validators and gas limit.
fn verify_block_family(&self, header: &Header, parent: &Header, _block: Option<&[u8]>) -> Result<(), Error> {
let proposal = ConsensusMessage::new_proposal(header)?;
let proposer = proposal.verify()?;
if !self.is_authority(&proposer) {
Err(EngineError::NotAuthorized(proposer))?
if header.number() == 0 {
Err(BlockError::RidiculousNumber(OutOfBounds { min: Some(1), max: None, found: header.number() }))?;
}

let precommit_hash = proposal.precommit_hash();
let ref signatures_field = header.seal()[2];
let mut signature_count = 0;
let mut origins = HashSet::new();
for rlp in UntrustedRlp::new(signatures_field).iter() {
let precommit: ConsensusMessage = ConsensusMessage::new_commit(&proposal, rlp.as_val()?);
let address = match self.votes.get(&precommit) {
Some(a) => a,
None => public_to_address(&recover(&precommit.signature.into(), &precommit_hash)?),
};
if !self.validators.contains(header.parent_hash(), &address) {
Err(EngineError::NotAuthorized(address.to_owned()))?
if let Ok(proposal) = ConsensusMessage::new_proposal(header) {
let proposer = proposal.verify()?;
if !self.is_authority(&proposer) {
Err(EngineError::NotAuthorized(proposer))?
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think return Err(EngineError::NotAuthorized(proposer)) would be way clearer, but I saw that you're using this pattern in many places already.

Copy link
Author

Choose a reason for hiding this comment

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

You have to do into as well, but can change.

}
self.is_view_proposer(header.parent_hash(), proposal.vote_step.height, proposal.vote_step.view, &proposer)?;
} else {
let vote_step = VoteStep::new(header.number() as usize, consensus_view(header)?, Step::Precommit);
let precommit_hash = message_hash(vote_step.clone(), header.bare_hash());
let ref signatures_field = header.seal()[2];
Copy link
Collaborator

Choose a reason for hiding this comment

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

let signatures_field = &header.seal()[2]; would be enough. Do we ever prove that seal has at least 2 elements? explicit expect would have been better.

Copy link
Author

Choose a reason for hiding this comment

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

In verify_block_basic, sure can do.

let mut signature_count = 0;
let mut origins = HashSet::new();
for rlp in UntrustedRlp::new(signatures_field).iter() {
let precommit = ConsensusMessage {
signature: rlp.as_val()?,
block_hash: Some(header.bare_hash()),
vote_step: vote_step.clone(),
};
let address = match self.votes.get(&precommit) {
Some(a) => a,
None => public_to_address(&recover(&precommit.signature.into(), &precommit_hash)?),
};
if !self.validators.contains(header.parent_hash(), &address) {
Err(EngineError::NotAuthorized(address.to_owned()))?
}

if origins.insert(address) {
signature_count += 1;
} else {
warn!(target: "engine", "verify_block_unordered: Duplicate signature from {} on the seal.", address);
Err(BlockError::InvalidSeal)?;
}
}

// Check if its a proposal if there is not enough precommits.
if !self.is_above_threshold(signature_count) {
let signatures_len = signatures_field.len();
// Proposal has to have an empty signature list.
if signatures_len != 1 {
Err(EngineError::BadSealFieldSize(OutOfBounds {
min: Some(1),
max: Some(1),
found: signatures_len
}))?;
if origins.insert(address) {
signature_count += 1;
} else {
warn!(target: "engine", "verify_block_unordered: Duplicate signature from {} on the seal.", address);
Err(BlockError::InvalidSeal)?;
}
}
self.is_view_proposer(header.parent_hash(), proposal.vote_step.height, proposal.vote_step.view, &proposer)?;
}

if header.number() == 0 {
Err(BlockError::RidiculousNumber(OutOfBounds { min: Some(1), max: None, found: header.number() }))?;
self.is_above_threshold(signature_count)?
Copy link
Collaborator

Choose a reason for hiding this comment

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

origins.len() would work here, signature_count is redundant.

}

let gas_limit_divisor = self.gas_limit_bound_divisor;
Expand Down Expand Up @@ -590,13 +592,14 @@ impl Engine for Tendermint {
fn is_proposal(&self, header: &Header) -> bool {
let signatures_len = header.seal()[2].len();
// Signatures have to be an empty list rlp.
let proposal = ConsensusMessage::new_proposal(header).expect("block went through full verification; this Engine verifies new_proposal creation; qed");
if signatures_len != 1 {
// New Commit received, skip to next height.
trace!(target: "engine", "Received a commit: {:?}.", proposal.vote_step);
self.to_next_height(proposal.vote_step.height);
trace!(target: "engine", "Received a commit: {:?}.", header.number());
self.to_next_height(header.number() as usize);
self.to_step(Step::Commit);
return false;
}
let proposal = ConsensusMessage::new_proposal(header).expect("block went through full verification; this Engine verifies new_proposal creation; qed");
let proposer = proposal.verify().expect("block went through full verification; this Engine tries verify; qed");
debug!(target: "engine", "Received a new proposal {:?} from {}.", proposal.vote_step, proposer);
if self.is_view(&proposal) {
Expand Down Expand Up @@ -647,6 +650,10 @@ impl Engine for Tendermint {
}

fn register_client(&self, client: Weak<Client>) {
use client::BlockChainClient;
if let Some(c) = client.upgrade() {
self.height.store(c.chain_info().best_block_number as usize + 1, AtomicOrdering::SeqCst);
}
*self.client.write() = Some(client.clone());
self.validators.register_contract(client);
}
Expand Down Expand Up @@ -825,6 +832,7 @@ mod tests {
let vote_info = message_info_rlp(&VoteStep::new(2, 0, Step::Precommit), Some(header.bare_hash()));
let signature1 = tap.sign(proposer, None, vote_info.sha3()).unwrap();

seal[1] = ::rlp::NULL_RLP.to_vec();
seal[2] = ::rlp::encode_list(&vec![H520::from(signature1.clone())]).to_vec();
header.set_seal(seal.clone());

Expand Down
Loading