Skip to content

Fix the condition to check whether the new block could change the best block #1811

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Oct 11, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
6 changes: 4 additions & 2 deletions core/src/blockchain/blockchain.rs
Original file line number Diff line number Diff line change
Expand Up @@ -184,17 +184,19 @@ 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,
"Block #{}({}) has higher total score, changing the best proposal/canonical chain.",
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");

Expand Down
5 changes: 3 additions & 2 deletions core/src/blockchain/headerchain.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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!(
Expand All @@ -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");

Expand Down
10 changes: 9 additions & 1 deletion core/src/consensus/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -266,7 +266,15 @@ pub trait ConsensusEngine: Sync + Send {
header.hash()
}

fn can_change_canon_chain(&self, _header: &HeaderView) -> bool {
/// 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 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
}

Expand Down
17 changes: 8 additions & 9 deletions core/src/consensus/tendermint/engine.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<dyn ActionHandler>] {
Expand Down
13 changes: 0 additions & 13 deletions core/src/consensus/tendermint/worker.rs
Original file line number Diff line number Diff line change
Expand Up @@ -133,9 +133,6 @@ pub enum Event {
ap: Arc<AccountProvider>,
address: Address,
},
AllowedHeight {
result: crossbeam::Sender<Height>,
},
Restore(crossbeam::Sender<()>),
ProposalBlock {
signature: SchnorrSignature,
Expand Down Expand Up @@ -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
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was the cause of a bug.
If a proposal is not imported in Commit state yet, the code should return inner.height instead of inner.height + 1.

} else {
inner.height
};
result.send(allowed_height).unwrap();
}
Ok(Event::Restore(result)) => {
inner.restore();
result.send(()).unwrap();
Expand Down