From 2b5720056cbe3e125dfbb0c8cceb30faa997025c Mon Sep 17 00:00:00 2001 From: Juhyung Park Date: Tue, 10 Sep 2019 14:54:43 +0900 Subject: [PATCH 1/3] Use TendermintState instead of Step in some places Since TendermintState has more information than Step, using TendermintState is better. Also, if TendermintState::Commit has block_hash as a field, Step -> TendermintState conversion will be broken. --- core/src/consensus/tendermint/types.rs | 11 ----- core/src/consensus/tendermint/worker.rs | 60 +++++++++++++------------ 2 files changed, 31 insertions(+), 40 deletions(-) diff --git a/core/src/consensus/tendermint/types.rs b/core/src/consensus/tendermint/types.rs index 70cef83807..c92d656013 100644 --- a/core/src/consensus/tendermint/types.rs +++ b/core/src/consensus/tendermint/types.rs @@ -102,17 +102,6 @@ impl fmt::Debug for TendermintState { } } -impl From for TendermintState { - fn from(s: Step) -> Self { - match s { - Step::Propose => TendermintState::Propose, - Step::Prevote => TendermintState::Prevote, - Step::Precommit => TendermintState::Precommit, - Step::Commit => TendermintState::Commit, - } - } -} - #[derive(Debug, PartialEq, Eq, Clone, Copy, Hash)] pub enum Step { Propose, diff --git a/core/src/consensus/tendermint/worker.rs b/core/src/consensus/tendermint/worker.rs index d0ef5f4ae5..f9d59b6494 100644 --- a/core/src/consensus/tendermint/worker.rs +++ b/core/src/consensus/tendermint/worker.rs @@ -592,8 +592,8 @@ impl Worker { self.votes_received = BitSet::new(); } - fn move_to_step(&mut self, step: Step, is_restoring: bool) { - let prev_step = mem::replace(&mut self.step, step.into()); + fn move_to_step(&mut self, state: TendermintState, is_restoring: bool) { + let prev_step = mem::replace(&mut self.step, state.clone()); if !is_restoring { self.backup(); } @@ -602,18 +602,18 @@ impl Worker { self.timeout_token_nonce += 1; self.extension .send(network::Event::SetTimerStep { - step, + step: state.to_step(), view: self.view, expired_token_nonce, }) .unwrap(); - let vote_step = VoteStep::new(self.height, self.view, step); + let vote_step = VoteStep::new(self.height, self.view, state.to_step()); // If there are not enough pre-votes or pre-commits, // move_to_step can be called with the same step // Also, when moving to the commit step, // keep `votes_received` for gossiping. - if prev_step.to_step() != step && step != Step::Commit { + if prev_step.to_step() != state.to_step() && !state.is_commit() { self.votes_received = BitSet::new(); } @@ -624,13 +624,13 @@ impl Worker { self.last_two_thirds_majority.view(), self.votes_received, ); - match step { + match state.to_step() { Step::Propose => { cinfo!(ENGINE, "move_to_step: Propose."); if let Some(hash) = self.votes.get_block_hashes(&vote_step).first() { if self.client().block(&BlockId::Hash(*hash)).is_some() { self.proposal = Proposal::new_imported(*hash); - self.move_to_step(Step::Prevote, is_restoring); + self.move_to_step(TendermintState::Prevote, is_restoring); } else { cwarn!(ENGINE, "Proposal is received but not imported"); // Proposal is received but is not verified yet. @@ -822,7 +822,7 @@ impl Worker { let next_step = match self.step { TendermintState::Precommit if message.on.block_hash.is_none() && has_enough_aligned_votes => { self.increment_view(1); - Some(Step::Propose) + Some(TendermintState::Propose) } TendermintState::Precommit if has_enough_aligned_votes => { let bh = message.on.block_hash.expect("previous guard ensures is_some; qed"); @@ -832,16 +832,16 @@ impl Worker { // Update the best block hash as the hash of the committed block self.client().update_best_as_committed(bh); - Some(Step::Commit) + Some(TendermintState::Commit) } else { cwarn!(ENGINE, "Cannot find a proposal which committed"); self.increment_view(1); - Some(Step::Propose) + Some(TendermintState::Propose) } } // Avoid counting votes twice. - TendermintState::Prevote if lock_change => Some(Step::Precommit), - TendermintState::Prevote if has_enough_aligned_votes => Some(Step::Precommit), + TendermintState::Prevote if lock_change => Some(TendermintState::Precommit), + TendermintState::Prevote if has_enough_aligned_votes => Some(TendermintState::Precommit), _ => None, }; @@ -856,7 +856,7 @@ impl Worker { { let height = self.height; self.move_to_height(height + 1); - self.move_to_step(Step::Propose, is_restoring); + self.move_to_step(TendermintState::Propose, is_restoring); return } @@ -904,7 +904,7 @@ impl Worker { let current_step = self.step.clone(); match current_step { TendermintState::Propose => { - self.move_to_step(Step::Prevote, false); + self.move_to_step(TendermintState::Prevote, false); } TendermintState::ProposeWaitImported { block, @@ -944,13 +944,13 @@ impl Worker { if proposal_at_view_0 == Some(proposal.hash()) { self.proposal = Proposal::new_imported(proposal.hash()) } - self.move_to_step(Step::Prevote, false); + self.move_to_step(TendermintState::Prevote, false); } } fn submit_proposal_block(&mut self, sealed_block: &SealedBlock) { cinfo!(ENGINE, "Submitting proposal block {}", sealed_block.header().hash()); - self.move_to_step(Step::Prevote, false); + self.move_to_step(TendermintState::Prevote, false); self.broadcast_proposal_block(self.view, encoded::Block::new(sealed_block.rlp_bytes())); } @@ -968,14 +968,16 @@ impl Worker { let client = self.client(); let backup = restore(client.get_kvdb().as_ref()); if let Some(backup) = backup { - let backup_step = if backup.step == Step::Commit { + let backup_step = match backup.step { + Step::Propose => TendermintState::Propose, + Step::Prevote => TendermintState::Prevote, + Step::Precommit => TendermintState::Precommit, // If the backuped step is `Commit`, we should start at `Precommit` to update the // chain's best block safely. - Step::Precommit - } else { - backup.step + Step::Commit => TendermintState::Precommit, }; - self.step = backup_step.into(); + + self.step = backup_step; self.height = backup.height; self.view = backup.view; self.last_confirmed_view = backup.last_confirmed_view; @@ -1188,7 +1190,7 @@ impl Worker { let next_step = match self.step { TendermintState::Propose => { cinfo!(ENGINE, "Propose timeout."); - Step::Prevote + TendermintState::Prevote } TendermintState::ProposeWaitBlockGeneration { .. @@ -1210,20 +1212,20 @@ impl Worker { } TendermintState::Prevote if self.has_enough_any_votes() => { cinfo!(ENGINE, "Prevote timeout."); - Step::Precommit + TendermintState::Precommit } TendermintState::Prevote => { cinfo!(ENGINE, "Prevote timeout without enough votes."); - Step::Prevote + TendermintState::Prevote } TendermintState::Precommit if self.has_enough_any_votes() => { cinfo!(ENGINE, "Precommit timeout."); self.increment_view(1); - Step::Propose + TendermintState::Propose } TendermintState::Precommit => { cinfo!(ENGINE, "Precommit timeout without enough votes."); - Step::Precommit + TendermintState::Precommit } TendermintState::Commit => { cinfo!(ENGINE, "Commit timeout."); @@ -1234,7 +1236,7 @@ impl Worker { } let height = self.height; self.move_to_height(height + 1); - Step::Propose + TendermintState::Propose } TendermintState::CommitTimedout => unreachable!(), }; @@ -1457,14 +1459,14 @@ impl Worker { } } if height_changed { - self.move_to_step(Step::Propose, false); + self.move_to_step(TendermintState::Propose, false); return } } if !enacted.is_empty() && self.can_move_from_commit_to_propose() { let new_height = self.height + 1; self.move_to_height(new_height); - self.move_to_step(Step::Propose, false) + self.move_to_step(TendermintState::Propose, false) } } From 1532d5c1a9d106e5136c0adcbed21aaea69bf195 Mon Sep 17 00:00:00 2001 From: Juhyung Park Date: Tue, 10 Sep 2019 15:49:33 +0900 Subject: [PATCH 2/3] Move "Transition to {state}" log in the move_to_step function --- core/src/consensus/tendermint/worker.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/src/consensus/tendermint/worker.rs b/core/src/consensus/tendermint/worker.rs index f9d59b6494..0252140c90 100644 --- a/core/src/consensus/tendermint/worker.rs +++ b/core/src/consensus/tendermint/worker.rs @@ -593,6 +593,7 @@ impl Worker { } fn move_to_step(&mut self, state: TendermintState, is_restoring: bool) { + ctrace!(ENGINE, "Transition to {:?} triggered.", state); let prev_step = mem::replace(&mut self.step, state.clone()); if !is_restoring { self.backup(); @@ -846,7 +847,6 @@ impl Worker { }; if let Some(step) = next_step { - ctrace!(ENGINE, "Transition to {:?} triggered.", step); self.move_to_step(step, is_restoring); return } From f63dbccd9e4496dc1ddf6e67d724b08637f4453c Mon Sep 17 00:00:00 2001 From: Juhyung Park Date: Wed, 11 Sep 2019 15:51:20 +0900 Subject: [PATCH 3/3] Move to Commit state if enough precommits are collected --- core/src/consensus/tendermint/types.rs | 66 ++++++++++-- core/src/consensus/tendermint/worker.rs | 136 ++++++++++++++++++------ 2 files changed, 161 insertions(+), 41 deletions(-) diff --git a/core/src/consensus/tendermint/types.rs b/core/src/consensus/tendermint/types.rs index c92d656013..91c63f242b 100644 --- a/core/src/consensus/tendermint/types.rs +++ b/core/src/consensus/tendermint/types.rs @@ -23,6 +23,7 @@ use rlp::{Decodable, DecoderError, Encodable, RlpStream, UntrustedRlp}; use super::super::BitSet; use super::message::VoteStep; use crate::block::{IsBlock, SealedBlock}; +use consensus::tendermint::BlockHash; pub type Height = u64; pub type View = u64; @@ -41,8 +42,14 @@ pub enum TendermintState { }, Prevote, Precommit, - Commit, - CommitTimedout, + Commit { + view: View, + block_hash: H256, + }, + CommitTimedout { + view: View, + block_hash: H256, + }, } impl TendermintState { @@ -60,25 +67,60 @@ impl TendermintState { } => Step::Propose, TendermintState::Prevote => Step::Prevote, TendermintState::Precommit => Step::Precommit, - TendermintState::Commit => Step::Commit, - TendermintState::CommitTimedout => Step::Commit, + TendermintState::Commit { + .. + } => Step::Commit, + TendermintState::CommitTimedout { + .. + } => Step::Commit, } } pub fn is_commit(&self) -> bool { match self { - TendermintState::Commit => true, - TendermintState::CommitTimedout => true, + TendermintState::Commit { + .. + } => true, + TendermintState::CommitTimedout { + .. + } => true, _ => false, } } pub fn is_commit_timedout(&self) -> bool { match self { - TendermintState::CommitTimedout => true, + TendermintState::CommitTimedout { + .. + } => true, _ => false, } } + + pub fn committed(&self) -> Option<(View, BlockHash)> { + match self { + TendermintState::Commit { + block_hash, + view, + } => Some((*view, *block_hash)), + TendermintState::CommitTimedout { + block_hash, + view, + } => Some((*view, *block_hash)), + TendermintState::Propose => None, + TendermintState::ProposeWaitBlockGeneration { + .. + } => None, + TendermintState::ProposeWaitImported { + .. + } => None, + TendermintState::ProposeWaitEmptyBlockTimer { + .. + } => None, + TendermintState::Prevote => None, + TendermintState::Precommit => None, + } + } } impl fmt::Debug for TendermintState { @@ -96,8 +138,14 @@ impl fmt::Debug for TendermintState { } => write!(f, "TendermintState::ProposeWaitEmptyBlockTimer({})", block.header().hash()), TendermintState::Prevote => write!(f, "TendermintState::Prevote"), TendermintState::Precommit => write!(f, "TendermintState::Precommit"), - TendermintState::Commit => write!(f, "TendermintState::Commit"), - TendermintState::CommitTimedout => write!(f, "TendermintState::CommitTimedout"), + TendermintState::Commit { + block_hash, + view, + } => write!(f, "TendermintState::Commit({}, {})", block_hash, view), + TendermintState::CommitTimedout { + block_hash, + view, + } => write!(f, "TendermintState::CommitTimedout({}, {})", block_hash, view), } } } diff --git a/core/src/consensus/tendermint/worker.rs b/core/src/consensus/tendermint/worker.rs index 0252140c90..d54d34b67e 100644 --- a/core/src/consensus/tendermint/worker.rs +++ b/core/src/consensus/tendermint/worker.rs @@ -405,6 +405,10 @@ impl Worker { /// Check Tendermint can move from the commit step to the propose step fn can_move_from_commit_to_propose(&self) -> bool { + if !self.step.is_commit() { + return false + } + let vote_step = VoteStep::new(self.height, self.last_confirmed_view, Step::Precommit); if self.step.is_commit_timedout() && self.check_current_block_exists() { cinfo!(ENGINE, "Transition to Propose because best block is changed after commit timeout"); @@ -442,6 +446,20 @@ impl Worker { Some((proposal.signature, proposal.signer_index, bytes)) } + fn is_proposal_received(&self, height: Height, view: View, block_hash: BlockHash) -> bool { + let all_votes = self.votes.get_all_votes_in_round(&VoteStep { + height, + view, + step: Step::Propose, + }); + + if let Some(proposal) = all_votes.first() { + proposal.on.block_hash.expect("Proposal message always include block hash") == block_hash + } else { + false + } + } + pub fn vote_step(&self) -> VoteStep { VoteStep { height: self.height, @@ -592,6 +610,7 @@ impl Worker { self.votes_received = BitSet::new(); } + #[allow(clippy::cognitive_complexity)] fn move_to_step(&mut self, state: TendermintState, is_restoring: bool) { ctrace!(ENGINE, "Transition to {:?} triggered.", state); let prev_step = mem::replace(&mut self.step, state.clone()); @@ -707,6 +726,34 @@ impl Worker { } Step::Commit => { cinfo!(ENGINE, "move_to_step: Commit."); + let (view, block_hash) = state.committed().expect("commit always has committed_view"); + self.save_last_confirmed_view(view); + + let proposal_received = self.is_proposal_received(self.height, view, block_hash); + let proposal_imported = self.client().block(&block_hash.into()).is_some(); + let best_block_header = self.client().best_block_header(); + if best_block_header.number() >= self.height { + cwarn!( + ENGINE, + "best_block_header.number() >= self.height ({} >= {}) in Commit state", + best_block_header.number(), + self.height + ); + return + } + + let should_update_best_block = best_block_header.hash() != block_hash; + + cdebug!( + ENGINE, + "commited, proposal_received: {}, proposal_imported: {}, should_update_best_block: {}", + proposal_received, + proposal_imported, + should_update_best_block + ); + if proposal_imported && should_update_best_block { + self.client().update_best_as_committed(block_hash); + } } } } @@ -818,6 +865,32 @@ impl Worker { self.last_two_thirds_majority = TwoThirdsMajority::from_message(message.on.step.view, message.on.block_hash); } + + if vote_step.step == Step::Precommit + && self.height == vote_step.height + && message.on.block_hash.is_some() + && has_enough_aligned_votes + { + if self.can_move_from_commit_to_propose() { + let height = self.height; + self.move_to_height(height + 1); + self.move_to_step(TendermintState::Propose, is_restoring); + return + } + + let block_hash = message.on.block_hash.expect("Upper if already checked block hash"); + if !self.step.is_commit() { + self.move_to_step( + TendermintState::Commit { + block_hash, + view: vote_step.view, + }, + is_restoring, + ); + return + } + } + // Check if it can affect the step transition. if self.is_step(message) { let next_step = match self.step { @@ -825,21 +898,6 @@ impl Worker { self.increment_view(1); Some(TendermintState::Propose) } - TendermintState::Precommit if has_enough_aligned_votes => { - let bh = message.on.block_hash.expect("previous guard ensures is_some; qed"); - if self.client().block(&BlockId::Hash(bh)).is_some() { - // Commit the block, and update the last confirmed view - self.save_last_confirmed_view(message.on.step.view); - - // Update the best block hash as the hash of the committed block - self.client().update_best_as_committed(bh); - Some(TendermintState::Commit) - } else { - cwarn!(ENGINE, "Cannot find a proposal which committed"); - self.increment_view(1); - Some(TendermintState::Propose) - } - } // Avoid counting votes twice. TendermintState::Prevote if lock_change => Some(TendermintState::Precommit), TendermintState::Prevote if has_enough_aligned_votes => Some(TendermintState::Precommit), @@ -850,14 +908,6 @@ impl Worker { self.move_to_step(step, is_restoring); return } - } else if vote_step.step == Step::Precommit - && self.height == vote_step.height - && self.can_move_from_commit_to_propose() - { - let height = self.height; - self.move_to_height(height + 1); - self.move_to_step(TendermintState::Propose, is_restoring); - return } // self.move_to_step() calls self.broadcast_state() @@ -1227,18 +1277,27 @@ impl Worker { cinfo!(ENGINE, "Precommit timeout without enough votes."); TendermintState::Precommit } - TendermintState::Commit => { + TendermintState::Commit { + block_hash, + view, + } => { cinfo!(ENGINE, "Commit timeout."); - if !self.check_current_block_exists() { + if self.client().block(&block_hash.into()).is_none() { cwarn!(ENGINE, "Best chain is not updated yet, wait until imported"); - self.step = TendermintState::CommitTimedout; + self.step = TendermintState::CommitTimedout { + block_hash, + view, + }; return } + let height = self.height; self.move_to_height(height + 1); TendermintState::Propose } - TendermintState::CommitTimedout => unreachable!(), + TendermintState::CommitTimedout { + .. + } => unreachable!(), }; self.move_to_step(next_step, false); @@ -1439,6 +1498,24 @@ impl Worker { } }; + if self.step.is_commit() && (imported.len() + enacted.len() == 1) { + let (_, committed_block_hash) = self.step.committed().expect("Commit state always has block_hash"); + if imported.first() == Some(&committed_block_hash) { + cdebug!(ENGINE, "Committed block {} is committed_block_hash", committed_block_hash); + self.client().update_best_as_committed(committed_block_hash); + return + } + if enacted.first() == Some(&committed_block_hash) { + cdebug!(ENGINE, "Committed block {} is now the best block", committed_block_hash); + if self.can_move_from_commit_to_propose() { + let new_height = self.height + 1; + self.move_to_height(new_height); + self.move_to_step(TendermintState::Propose, false); + return + } + } + } + if !imported.is_empty() { let mut height_changed = false; for hash in imported { @@ -1463,11 +1540,6 @@ impl Worker { return } } - if !enacted.is_empty() && self.can_move_from_commit_to_propose() { - let new_height = self.height + 1; - self.move_to_height(new_height); - self.move_to_step(TendermintState::Propose, false) - } } fn send_proposal_block(