Skip to content
This repository has been archived by the owner on Sep 26, 2019. It is now read-only.

IBFT to use VoteTallyCache #907

Merged
merged 8 commits into from
Feb 20, 2019
Merged

IBFT to use VoteTallyCache #907

merged 8 commits into from
Feb 20, 2019

Conversation

rain-on
Copy link
Contributor

@rain-on rain-on commented Feb 19, 2019

It was identified during a demonstration that Pantheon, when running
in IBFT would show a "Bad Block Import" when a validator was added
removed from the validator pool.

It was determined this was due to IBFT maintaining a single, 'global'
copy of the curent list of validators, which was updated when a block
was imported - thus when a block which had been imported via IBFT
was then received via Eth block propogation, the validator list would
not align with the global list (as it had been updated in the IBFT
import).

The solution has been to utilise the VoteTallyCache as used in the
Clique implementation.

tmohay added 2 commits February 19, 2019 16:07
It was identified during a demonstration that Pantheon, when running
in IBFT would show a "Bad Block Import" when a validator was added or
removed from the validator pool.

It was determined this was due to IBFT maintaining a single, 'global'
copy of the curent list of validators, which was updated when a block
was imported - thus when a block which had been imported vi IBFT
was then received via Eth block propogation, the validator list would
not align with the global list (as it had been updated in the IBFT
import).

The solution has been to utilise the VoteTallyCache as used in the
Clique implementation.
@rain-on rain-on requested review from jframe and CjHare February 19, 2019 06:50
@rain-on rain-on changed the title Ibft vtc IBFT to use VoteTallyCache Feb 19, 2019
Copy link
Contributor

@CjHare CjHare left a comment

Choose a reason for hiding this comment

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

Nowhere near as daunting as I expected for a 53 file change set!

@@ -81,7 +81,7 @@
if (header.getNumber() == BlockHeader.GENESIS_BLOCK_NUMBER) {
continue;
}
if (header.getNumber() % 100 == 0) {
if (header.getNumber() % 1 == 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: why not just get rid of the if statement to log every block import

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 not meant to change, was used for debug, reverted.

@@ -162,7 +162,7 @@ private void extractSignatures(final Block block) {
final BlockHeaderValidator<C> blockHeaderValidator = protocolSpec.getBlockHeaderValidator();
final boolean validHeader =
blockHeaderValidator.validateHeader(
header, previousHeader, context, HeaderValidationMode.FULL);
header, previousHeader, context, HeaderValidationMode.DETACHED_ONLY);
Copy link
Contributor

Choose a reason for hiding this comment

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

why did the validation mode need to be changed?

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 a bug in Pantheon which was hidden up until this change - basically they do async block validation (thus block 2 may be validated before block 1) - this is illegal in IBFT, as we must validate a block fully with respect to its parent.

Copy link
Contributor

Choose a reason for hiding this comment

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

Checked and this was changed to fix a bug "the validation rule was a hidden bug in Pantheon - the BlockImporter validates blocks ASYNC - meaning, out of order 😞 Which IBFT can no longer do!
So when doing ASYNC, you MUST do only detached rules as the initial validation, then do the rest at the end"

@rain-on rain-on merged commit f0f47f9 into PegaSysEng:master Feb 20, 2019
@rain-on rain-on deleted the ibft_vtc branch February 20, 2019 04:09
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.

5 participants