Move to Commit state if enough precommits are collected

Author: majecty

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),
         }
     }
 }

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,28 +865,39 @@ 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 {
                 TendermintState::Precommit if message.on.block_hash.is_none() && has_enough_aligned_votes => {
                     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(