From 9d3955b80d650d088d56e03c8d9c0d8dd13a4d85 Mon Sep 17 00:00:00 2001 From: Juhyung Park Date: Thu, 10 Oct 2019 16:01:20 +0900 Subject: [PATCH 1/2] Add comment of can_change_canon_chain function --- core/src/consensus/mod.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/core/src/consensus/mod.rs b/core/src/consensus/mod.rs index 7be9fdce92..28b2414cac 100644 --- a/core/src/consensus/mod.rs +++ b/core/src/consensus/mod.rs @@ -266,6 +266,9 @@ pub trait ConsensusEngine: Sync + Send { header.hash() } + /// In PoW consensus, the higher scored block became the best block. + /// In Tendermint consensus, the highest scored block may not be the best block. + /// Only the child of the current best block could be the next best block in Tendermint consensus. fn can_change_canon_chain(&self, _header: &HeaderView) -> bool { true } From e7d86fbefd0b8a5d51be470ff7e49e8fd806e74f Mon Sep 17 00:00:00 2001 From: Juhyung Park Date: Thu, 10 Oct 2019 18:14:16 +0900 Subject: [PATCH 2/2] Fix the condition to check whether the new block could change the best block When a new block is imported in Tendermint consensus, CodeChain checks the conditions below to judge whether it is safe to change the best block: First, whether the score is higher than before. Second, whether the height of a new block is higher than the finalized block's height. This commit fixes the code that is calculating the finalized block's height erroneously. --- core/src/blockchain/blockchain.rs | 6 ++++-- core/src/blockchain/headerchain.rs | 5 +++-- core/src/consensus/mod.rs | 11 ++++++++--- core/src/consensus/tendermint/engine.rs | 17 ++++++++--------- core/src/consensus/tendermint/worker.rs | 13 ------------- 5 files changed, 23 insertions(+), 29 deletions(-) diff --git a/core/src/blockchain/blockchain.rs b/core/src/blockchain/blockchain.rs index 3b55954fb3..78a59c18f3 100644 --- a/core/src/blockchain/blockchain.rs +++ b/core/src/blockchain/blockchain.rs @@ -184,9 +184,11 @@ impl BlockChain { let new_header = new_block.header_view(); let parent_hash_of_new_block = new_header.parent_hash(); let parent_details_of_new_block = self.block_details(&parent_hash_of_new_block).expect("Invalid parent hash"); + let prev_best_proposal_hash = self.best_proposal_block_hash(); + let prev_best_hash = self.best_block_hash(); if parent_details_of_new_block.total_score + new_header.score() > self.best_proposal_block_detail().total_score - && engine.can_change_canon_chain(&new_header) + && engine.can_change_canon_chain(&new_header, prev_best_hash, prev_best_proposal_hash) { cinfo!( BLOCKCHAIN, @@ -194,7 +196,7 @@ impl BlockChain { new_header.number(), new_header.hash() ); - let prev_best_hash = self.best_block_hash(); + let route = tree_route(self, prev_best_hash, parent_hash_of_new_block) .expect("blocks being imported always within recent history; qed"); diff --git a/core/src/blockchain/headerchain.rs b/core/src/blockchain/headerchain.rs index 490510f556..ab1d592775 100644 --- a/core/src/blockchain/headerchain.rs +++ b/core/src/blockchain/headerchain.rs @@ -242,9 +242,11 @@ impl HeaderChain { fn best_header_changed(&self, new_header: &HeaderView, engine: &dyn CodeChainEngine) -> BestHeaderChanged { let parent_hash_of_new_header = new_header.parent_hash(); let parent_details_of_new_header = self.block_details(&parent_hash_of_new_header).expect("Invalid parent hash"); + let prev_best_proposal_hash = self.best_proposal_header_hash(); + let prev_best_hash = self.best_header_hash(); let is_new_best = parent_details_of_new_header.total_score + new_header.score() > self.best_proposal_header_detail().total_score - && engine.can_change_canon_chain(&new_header); + && engine.can_change_canon_chain(&new_header, prev_best_hash, prev_best_proposal_hash); if is_new_best { ctrace!( @@ -256,7 +258,6 @@ impl HeaderChain { // on new best block we need to make sure that all ancestors // are moved to "canon chain" // find the route between old best block and the new one - let prev_best_hash = self.best_header_hash(); let route = tree_route(self, prev_best_hash, parent_hash_of_new_header) .expect("blocks being imported always within recent history; qed"); diff --git a/core/src/consensus/mod.rs b/core/src/consensus/mod.rs index 28b2414cac..2a682d93f4 100644 --- a/core/src/consensus/mod.rs +++ b/core/src/consensus/mod.rs @@ -266,10 +266,15 @@ pub trait ConsensusEngine: Sync + Send { header.hash() } - /// In PoW consensus, the higher scored block became the best block. + /// In PoW consensus, the higher scored block becomes the best block. /// In Tendermint consensus, the highest scored block may not be the best block. - /// Only the child of the current best block could be the next best block in Tendermint consensus. - fn can_change_canon_chain(&self, _header: &HeaderView) -> bool { + /// Only the descendant of the current best block could be the next best block in Tendermint consensus. + fn can_change_canon_chain( + &self, + _new_header: &HeaderView, + _previous_best_hash: H256, + _previous_best_proposal_hash: H256, + ) -> bool { true } diff --git a/core/src/consensus/tendermint/engine.rs b/core/src/consensus/tendermint/engine.rs index 1805caef4b..2617ef403e 100644 --- a/core/src/consensus/tendermint/engine.rs +++ b/core/src/consensus/tendermint/engine.rs @@ -308,15 +308,14 @@ impl ConsensusEngine for Tendermint { header.parent_hash() } - fn can_change_canon_chain(&self, header: &HeaderView) -> bool { - let (result, receiver) = crossbeam::bounded(1); - self.inner - .send(worker::Event::AllowedHeight { - result, - }) - .unwrap(); - let allowed_height = receiver.recv().unwrap(); - header.number() >= allowed_height + + fn can_change_canon_chain( + &self, + new_header: &HeaderView, + prev_best_hash: H256, + prev_best_proposal_hash: H256, + ) -> bool { + new_header.parent_hash() == prev_best_hash || new_header.parent_hash() == prev_best_proposal_hash } fn action_handlers(&self) -> &[Arc] { diff --git a/core/src/consensus/tendermint/worker.rs b/core/src/consensus/tendermint/worker.rs index 04df0942bd..95f9b3d3a3 100644 --- a/core/src/consensus/tendermint/worker.rs +++ b/core/src/consensus/tendermint/worker.rs @@ -133,9 +133,6 @@ pub enum Event { ap: Arc, address: Address, }, - AllowedHeight { - result: crossbeam::Sender, - }, Restore(crossbeam::Sender<()>), ProposalBlock { signature: SchnorrSignature, @@ -307,16 +304,6 @@ impl Worker { }) => { inner.set_signer(ap, address); } - Ok(Event::AllowedHeight { - result, - }) => { - let allowed_height = if inner.step.is_commit() { - inner.height + 1 - } else { - inner.height - }; - result.send(allowed_height).unwrap(); - } Ok(Event::Restore(result)) => { inner.restore(); result.send(()).unwrap();