From ed00206d41a28ad32d093679d05a5577579f868d Mon Sep 17 00:00:00 2001 From: Juhyung Park Date: Mon, 7 Oct 2019 15:47:30 +0900 Subject: [PATCH 1/2] Recover TendermintState when empty proposal timer is fired in a wrong time The Tendermint module was changing its step to 'Propose' when the "engine timeout empty proposal" timer is fired. If the timer is called in the wrong time the step should not be changed. This commit makes the step change when it is needed. --- core/src/consensus/tendermint/types.rs | 9 +++++++ core/src/consensus/tendermint/worker.rs | 36 +++++++++++++------------ 2 files changed, 28 insertions(+), 17 deletions(-) diff --git a/core/src/consensus/tendermint/types.rs b/core/src/consensus/tendermint/types.rs index bdacb40906..7cf582ebfe 100644 --- a/core/src/consensus/tendermint/types.rs +++ b/core/src/consensus/tendermint/types.rs @@ -76,6 +76,15 @@ impl TendermintState { } } + pub fn is_propose_wait_empty_block_timer(&self) -> bool { + match self { + TendermintState::ProposeWaitEmptyBlockTimer { + .. + } => true, + _ => false, + } + } + pub fn is_commit(&self) -> bool { match self { TendermintState::Commit { diff --git a/core/src/consensus/tendermint/worker.rs b/core/src/consensus/tendermint/worker.rs index 86f6344af2..5e584e15f9 100644 --- a/core/src/consensus/tendermint/worker.rs +++ b/core/src/consensus/tendermint/worker.rs @@ -1208,25 +1208,27 @@ impl Worker { fn on_timeout(&mut self, token: usize) { // Timeout from empty block generation if token == ENGINE_TIMEOUT_EMPTY_PROPOSAL { - let prev_step = mem::replace(&mut self.step, TendermintState::Propose); - match prev_step { - TendermintState::ProposeWaitEmptyBlockTimer { - block, - } => { - if self.height == block.header().number() { - cdebug!( - ENGINE, - "Empty proposal timer is finished, go to the prevote step and broadcast the block" - ); - self.submit_proposal_block(block.as_ref()); - } else { - cwarn!(ENGINE, "Empty proposal timer was for previous height."); - } - } - _ => { - cwarn!(ENGINE, "Empty proposal timer was not cleared."); + let block = if self.step.is_propose_wait_empty_block_timer() { + let previous = mem::replace(&mut self.step, TendermintState::Propose); + match previous { + TendermintState::ProposeWaitEmptyBlockTimer { + block, + } => block, + _ => unreachable!(), } + } else { + cwarn!(ENGINE, "Empty proposal timer was not cleared."); + return + }; + + if self.height == block.header().number() { + cdebug!(ENGINE, "Empty proposal timer is finished, go to the prevote step and broadcast the block"); + self.submit_proposal_block(block.as_ref()); + } else { + self.move_to_step(TendermintState::Prevote, false); + cwarn!(ENGINE, "Empty proposal timer was for previous height."); } + return } From 3af32fa51511d7dd65d33cf93b2f68f871b072f2 Mon Sep 17 00:00:00 2001 From: Juhyung Park Date: Mon, 7 Oct 2019 16:46:08 +0900 Subject: [PATCH 2/2] Inline submit_proposal_block function in Tendermint It is hard to know whether submit_proposal_block changes step or not. Since submit_proposal_block has only two lines of code, it doesn't remove code duplication. To clarify step transition, inline the submit_proposal_block function. --- core/src/consensus/tendermint/worker.rs | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/core/src/consensus/tendermint/worker.rs b/core/src/consensus/tendermint/worker.rs index 5e584e15f9..968c744c31 100644 --- a/core/src/consensus/tendermint/worker.rs +++ b/core/src/consensus/tendermint/worker.rs @@ -949,7 +949,9 @@ impl Worker { block, } => { if !block.transactions().is_empty() { - self.submit_proposal_block(&block); + cinfo!(ENGINE, "Submitting proposal block {}", block.header().hash()); + self.move_to_step(TendermintState::Prevote, false); + self.broadcast_proposal_block(self.view, encoded::Block::new(block.rlp_bytes())); } else { ctrace!(ENGINE, "Empty proposal is generated, set timer"); self.step = TendermintState::ProposeWaitEmptyBlockTimer { @@ -986,12 +988,6 @@ impl Worker { } } - fn submit_proposal_block(&mut self, sealed_block: &SealedBlock) { - cinfo!(ENGINE, "Submitting proposal block {}", sealed_block.header().hash()); - self.move_to_step(TendermintState::Prevote, false); - self.broadcast_proposal_block(self.view, encoded::Block::new(sealed_block.rlp_bytes())); - } - fn backup(&self) { backup(self.client().get_kvdb().as_ref(), BackupView { height: &self.height, @@ -1221,11 +1217,14 @@ impl Worker { return }; + // When self.height != block.header().number() && "propose timeout" is already called, + // the state is stuck and can't move to Prevote. We should change the step to Prevote. + self.move_to_step(TendermintState::Prevote, false); if self.height == block.header().number() { cdebug!(ENGINE, "Empty proposal timer is finished, go to the prevote step and broadcast the block"); - self.submit_proposal_block(block.as_ref()); + cinfo!(ENGINE, "Submitting proposal block {}", block.header().hash()); + self.broadcast_proposal_block(self.view, encoded::Block::new(block.rlp_bytes())); } else { - self.move_to_step(TendermintState::Prevote, false); cwarn!(ENGINE, "Empty proposal timer was for previous height."); }