Skip to content
This repository has been archived by the owner on Jun 11, 2024. It is now read-only.

Implement BFT fork choice rules #3725

Merged

Conversation

2snEM6
Copy link
Contributor

@2snEM6 2snEM6 commented May 24, 2019

What was the problem?

New fork choice rules needed to be implemented to comply with the new BFT consensus protocol

How did I fix it?

Implementing Fork Choice Rules according to LIP-0014

How to test it?

Run unit tests for blocks/process.js

Review checklist

@2snEM6 2snEM6 self-assigned this May 24, 2019
@2snEM6 2snEM6 changed the title 3559 implement bft fork choice rules Implement BFT fork choice rules May 24, 2019
@SargeKhan SargeKhan changed the base branch from feature/introduce_bft_consensus to 3556-height-previous-prevoted May 24, 2019 14:43
framework/src/modules/chain/submodules/blocks/process.js Outdated Show resolved Hide resolved
framework/src/modules/chain/submodules/blocks/process.js Outdated Show resolved Hide resolved
framework/src/modules/chain/submodules/blocks/process.js Outdated Show resolved Hide resolved
framework/src/modules/chain/submodules/blocks/process.js Outdated Show resolved Hide resolved
framework/src/modules/chain/submodules/blocks/process.js Outdated Show resolved Hide resolved
framework/src/modules/chain/submodules/blocks/process.js Outdated Show resolved Hide resolved
framework/src/modules/chain/submodules/blocks/process.js Outdated Show resolved Hide resolved
framework/src/modules/chain/submodules/blocks/process.js Outdated Show resolved Hide resolved
framework/src/modules/chain/submodules/blocks/process.js Outdated Show resolved Hide resolved
framework/src/modules/chain/submodules/blocks/process.js Outdated Show resolved Hide resolved
Copy link
Contributor

@nazarhussain nazarhussain left a comment

Choose a reason for hiding this comment

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

@limiaspasdaniel A suggestion to make this logic look more readable.

// Case 2
const isValidBlock = (last, current) =>
	last.height + 1 === current.height && last.id === current.previousBlock;

// Case 1
const isIdenticalBlock = (last, current) => last.id === current.id;

const isDuplicateBlock = (last, current) =>
	last.height === current.height &&
	last.heightPrevoted === current.heightPrevoted &&
	last.previousBlock === current.previousBlock;

// Case 3
const isDoubleForging = (last, current) =>
	isDuplicateBlock(last, current) &&
	last.generatorPublicKey === current.generatorPublicKey;

// Case 4
const isTieBlocks = (last, current) =>
	isDuplicateBlock(last, current) &&
	slots.getSlotNumber(last.timestamp) <
		slots.getSlotNumber(current.timestamp) &&
	!this._receivedInSlot(last, modules.blocks.lastReceipt.get()) &&
	this._receivedInSlot(current, current.timestamp);

// Case 5
const isDifferentChain = (last, current) =>
	last.heightPrevoted < current.heightPrevoted ||
	(last.height < current.height &&
		last.heightPrevoted === current.heightPrevoted);

if(isIdenticalBlock(last, current)){
	return this._handleSameBlockReceived(current);
} else if(isValidBlock(last, current)) {
	return this._handleGoodBlock(current);
} else if(isDoubleForging(last, current)) {
	return this._handleDoubleForging(current);
} else if(isTieBlocks(last, current)) {
	return this._handleDoubleForgingTieBreak(current);
}else if(isDifferentChain(last, current)) {
	return this._handleMovingToDifferentChain(current);
} else {
	return this._handleDiscardedBlock(current);
}

@2snEM6
Copy link
Contributor Author

2snEM6 commented May 28, 2019

@nazarhussain, regarding your suggestion to make the logic more readable:

I like the format and it definitely reads better and clearer, however, I wouldn't nest functions inside functions, this complicates testing. What I can do instead is move those functions as private functions with the following format this._functionName as we are not using _private anymore. This way we can still apply your

@2snEM6 2snEM6 force-pushed the 3559-Implement_BFT_fork_choice_rules branch from c730343 to 5a39ee9 Compare June 3, 2019 16:01
@2snEM6 2snEM6 force-pushed the 3559-Implement_BFT_fork_choice_rules branch from fc9ea4c to 9925aa5 Compare June 4, 2019 12:14
@2snEM6 2snEM6 force-pushed the 3559-Implement_BFT_fork_choice_rules branch from d01290c to 5659c57 Compare June 5, 2019 10:34
@2snEM6 2snEM6 closed this Jun 5, 2019
@2snEM6 2snEM6 force-pushed the 3559-Implement_BFT_fork_choice_rules branch from 5659c57 to 99d9da9 Compare June 5, 2019 12:19
@2snEM6 2snEM6 reopened this Jun 5, 2019
@2snEM6 2snEM6 force-pushed the 3559-Implement_BFT_fork_choice_rules branch from 6ecb34d to 9286cda Compare June 5, 2019 15:10
Copy link
Collaborator

@shuse2 shuse2 left a comment

Choose a reason for hiding this comment

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

Also. logs take first argument as metadata object and second parameter is the string

framework/src/modules/chain/blocks/block_slots.js Outdated Show resolved Hide resolved
framework/src/modules/chain/blocks/blocks.js Outdated Show resolved Hide resolved
framework/src/modules/chain/blocks/blocks.js Outdated Show resolved Hide resolved
framework/src/modules/chain/blocks/blocks.js Outdated Show resolved Hide resolved
framework/src/modules/chain/blocks/blocks.js Outdated Show resolved Hide resolved
framework/src/modules/chain/blocks/blocks.js Show resolved Hide resolved
framework/src/modules/chain/blocks/blocks.js Outdated Show resolved Hide resolved
@2snEM6 2snEM6 changed the base branch from 3556-height-previous-prevoted to feature/introduce_bft_consensus June 7, 2019 14:10
Copy link
Contributor

@michielmulders michielmulders left a comment

Choose a reason for hiding this comment

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

Some small feedback. Looks good overall.

framework/src/modules/chain/blocks/blocks.js Outdated Show resolved Hide resolved
framework/src/modules/chain/blocks/fork_choice_rule.js Outdated Show resolved Hide resolved
Copy link
Collaborator

@shuse2 shuse2 left a comment

Choose a reason for hiding this comment

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

Overall LGTM

framework/src/modules/chain/blocks/blocks.js Outdated Show resolved Hide resolved
framework/src/modules/chain/blocks/blocks.js Outdated Show resolved Hide resolved
framework/src/modules/chain/blocks/blocks.js Outdated Show resolved Hide resolved
@2snEM6 2snEM6 force-pushed the 3559-Implement_BFT_fork_choice_rules branch from 6131a57 to eda13f4 Compare June 12, 2019 16:00
@2snEM6 2snEM6 requested review from shuse2 and michielmulders and removed request for SargeKhan June 13, 2019 12:38
@michielmulders michielmulders self-requested a review June 18, 2019 08:57
Copy link
Contributor

@michielmulders michielmulders left a comment

Choose a reason for hiding this comment

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

Thanks for making the changes!

@SargeKhan SargeKhan merged commit 821ff3d into feature/introduce_bft_consensus Jun 18, 2019
@shuse2 shuse2 deleted the 3559-Implement_BFT_fork_choice_rules branch June 19, 2019 08:58
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants