From 35f2dfb9b1fccd23ad80289bc03e7065a60fd104 Mon Sep 17 00:00:00 2001 From: tmohay <37158202+rain-on@users.noreply.github.com> Date: Mon, 7 Jan 2019 17:01:04 +1100 Subject: [PATCH] NewRoundMessageValidator creates RoundChangeValidator with correct value (#518) --- .../pantheon/consensus/ibft/IbftHelpers.java | 4 ++++ .../ibft/statemachine/RoundChangeManager.java | 7 ++++++- .../consensus/ibft/statemachine/RoundState.java | 6 +++++- .../ibft/validation/MessageValidatorFactory.java | 5 ++++- .../ibft/validation/NewRoundMessageValidator.java | 6 +++++- .../validation/NewRoundMessageValidatorTest.java | 14 ++++---------- 6 files changed, 28 insertions(+), 14 deletions(-) diff --git a/consensus/ibft/src/main/java/tech/pegasys/pantheon/consensus/ibft/IbftHelpers.java b/consensus/ibft/src/main/java/tech/pegasys/pantheon/consensus/ibft/IbftHelpers.java index 24b0e1834c..948bb12bed 100644 --- a/consensus/ibft/src/main/java/tech/pegasys/pantheon/consensus/ibft/IbftHelpers.java +++ b/consensus/ibft/src/main/java/tech/pegasys/pantheon/consensus/ibft/IbftHelpers.java @@ -34,6 +34,10 @@ public class IbftHelpers { return Util.fastDivCeiling(2 * validatorCount, 3); } + public static long prepareMessageCountForQuorum(final long quorum) { + return quorum - 1; + } + public static Block createSealedBlock( final Block block, final Collection commitSeals) { final BlockHeader initialHeader = block.getHeader(); diff --git a/consensus/ibft/src/main/java/tech/pegasys/pantheon/consensus/ibft/statemachine/RoundChangeManager.java b/consensus/ibft/src/main/java/tech/pegasys/pantheon/consensus/ibft/statemachine/RoundChangeManager.java index a439674217..49e7618105 100644 --- a/consensus/ibft/src/main/java/tech/pegasys/pantheon/consensus/ibft/statemachine/RoundChangeManager.java +++ b/consensus/ibft/src/main/java/tech/pegasys/pantheon/consensus/ibft/statemachine/RoundChangeManager.java @@ -12,6 +12,8 @@ */ package tech.pegasys.pantheon.consensus.ibft.statemachine; +import static tech.pegasys.pantheon.consensus.ibft.IbftHelpers.prepareMessageCountForQuorum; + import tech.pegasys.pantheon.consensus.ibft.ConsensusRoundIdentifier; import tech.pegasys.pantheon.consensus.ibft.IbftHelpers; import tech.pegasys.pantheon.consensus.ibft.ibftmessagedata.RoundChangeCertificate; @@ -89,7 +91,10 @@ public class RoundChangeManager { this.quorumSize = IbftHelpers.calculateRequiredValidatorQuorum(validators.size()); this.roundChangeMessageValidator = new RoundChangeMessageValidator( - messageValidityFactory, validators, quorumSize - 1, sequenceNumber); + messageValidityFactory, + validators, + prepareMessageCountForQuorum(quorumSize), + sequenceNumber); } /** diff --git a/consensus/ibft/src/main/java/tech/pegasys/pantheon/consensus/ibft/statemachine/RoundState.java b/consensus/ibft/src/main/java/tech/pegasys/pantheon/consensus/ibft/statemachine/RoundState.java index 0cacc207e1..7309095ef6 100644 --- a/consensus/ibft/src/main/java/tech/pegasys/pantheon/consensus/ibft/statemachine/RoundState.java +++ b/consensus/ibft/src/main/java/tech/pegasys/pantheon/consensus/ibft/statemachine/RoundState.java @@ -12,6 +12,8 @@ */ package tech.pegasys.pantheon.consensus.ibft.statemachine; +import static tech.pegasys.pantheon.consensus.ibft.IbftHelpers.prepareMessageCountForQuorum; + import tech.pegasys.pantheon.consensus.ibft.ConsensusRoundIdentifier; import tech.pegasys.pantheon.consensus.ibft.ibftmessagedata.CommitPayload; import tech.pegasys.pantheon.consensus.ibft.ibftmessagedata.PreparePayload; @@ -92,7 +94,9 @@ public class RoundState { private void updateState() { // NOTE: The quorumSize for Prepare messages is 1 less than the quorum size as the proposer // does not supply a prepare message - prepared = (preparePayloads.size() >= (quorumSize - 1)) && proposalMessage.isPresent(); + prepared = + (preparePayloads.size() >= prepareMessageCountForQuorum(quorumSize)) + && proposalMessage.isPresent(); committed = (commitPayloads.size() >= quorumSize) && proposalMessage.isPresent(); } diff --git a/consensus/ibft/src/main/java/tech/pegasys/pantheon/consensus/ibft/validation/MessageValidatorFactory.java b/consensus/ibft/src/main/java/tech/pegasys/pantheon/consensus/ibft/validation/MessageValidatorFactory.java index bf30c22ab9..712e82def8 100644 --- a/consensus/ibft/src/main/java/tech/pegasys/pantheon/consensus/ibft/validation/MessageValidatorFactory.java +++ b/consensus/ibft/src/main/java/tech/pegasys/pantheon/consensus/ibft/validation/MessageValidatorFactory.java @@ -12,6 +12,8 @@ */ package tech.pegasys.pantheon.consensus.ibft.validation; +import static tech.pegasys.pantheon.consensus.ibft.IbftHelpers.prepareMessageCountForQuorum; + import tech.pegasys.pantheon.consensus.ibft.ConsensusRoundIdentifier; import tech.pegasys.pantheon.consensus.ibft.IbftContext; import tech.pegasys.pantheon.consensus.ibft.IbftHelpers; @@ -56,7 +58,8 @@ public class MessageValidatorFactory { return new RoundChangeMessageValidator( roundIdentifier -> createMessageValidator(roundIdentifier, parentHeader), validators, - IbftHelpers.calculateRequiredValidatorQuorum(validators.size()), + prepareMessageCountForQuorum( + IbftHelpers.calculateRequiredValidatorQuorum(validators.size())), parentHeader.getNumber() + 1); } diff --git a/consensus/ibft/src/main/java/tech/pegasys/pantheon/consensus/ibft/validation/NewRoundMessageValidator.java b/consensus/ibft/src/main/java/tech/pegasys/pantheon/consensus/ibft/validation/NewRoundMessageValidator.java index 5135bcc028..cf9f62d062 100644 --- a/consensus/ibft/src/main/java/tech/pegasys/pantheon/consensus/ibft/validation/NewRoundMessageValidator.java +++ b/consensus/ibft/src/main/java/tech/pegasys/pantheon/consensus/ibft/validation/NewRoundMessageValidator.java @@ -13,6 +13,7 @@ package tech.pegasys.pantheon.consensus.ibft.validation; import static tech.pegasys.pantheon.consensus.ibft.IbftHelpers.findLatestPreparedCertificate; +import static tech.pegasys.pantheon.consensus.ibft.IbftHelpers.prepareMessageCountForQuorum; import tech.pegasys.pantheon.consensus.ibft.ConsensusRoundIdentifier; import tech.pegasys.pantheon.consensus.ibft.blockcreation.ProposerSelector; @@ -123,7 +124,10 @@ public class NewRoundMessageValidator { roundChangeCert.getRoundChangePayloads()) { final RoundChangeMessageValidator roundChangeValidator = new RoundChangeMessageValidator( - messageValidatorFactory, validators, quorumSize, chainHeight); + messageValidatorFactory, + validators, + prepareMessageCountForQuorum(quorumSize), + chainHeight); if (!roundChangeValidator.validateMessage(roundChangeMsg)) { LOG.info("Invalid NewRound message, embedded RoundChange message failed validation."); diff --git a/consensus/ibft/src/test/java/tech/pegasys/pantheon/consensus/ibft/validation/NewRoundMessageValidatorTest.java b/consensus/ibft/src/test/java/tech/pegasys/pantheon/consensus/ibft/validation/NewRoundMessageValidatorTest.java index 99b3efaf18..a3df85efb8 100644 --- a/consensus/ibft/src/test/java/tech/pegasys/pantheon/consensus/ibft/validation/NewRoundMessageValidatorTest.java +++ b/consensus/ibft/src/test/java/tech/pegasys/pantheon/consensus/ibft/validation/NewRoundMessageValidatorTest.java @@ -128,7 +128,7 @@ public class NewRoundMessageValidatorTest { } @Test - public void newRoundTargettingRoundZeroFails() { + public void newRoundTargetingRoundZeroFails() { msgBuilder.setRoundChangeIdentifier( new ConsensusRoundIdentifier(roundIdentifier.getSequenceNumber(), 0)); @@ -205,7 +205,7 @@ public class NewRoundMessageValidatorTest { public void roundChangeMessagesDoNotAllTargetRoundOfNewRoundMsgFails() { final ConsensusRoundIdentifier prevRound = TestHelpers.createFrom(roundIdentifier, 0, -1); - RoundChangeCertificate.Builder roundChangeBuilder = new RoundChangeCertificate.Builder(); + final RoundChangeCertificate.Builder roundChangeBuilder = new RoundChangeCertificate.Builder(); roundChangeBuilder.appendRoundChangeMessage( proposerMessageFactory.createSignedRoundChangePayload(roundIdentifier, Optional.empty())); roundChangeBuilder.appendRoundChangeMessage( @@ -222,7 +222,7 @@ public class NewRoundMessageValidatorTest { public void invalidEmbeddedRoundChangeMessageFails() { final ConsensusRoundIdentifier prevRound = TestHelpers.createFrom(roundIdentifier, 0, -1); - RoundChangeCertificate.Builder roundChangeBuilder = new RoundChangeCertificate.Builder(); + final RoundChangeCertificate.Builder roundChangeBuilder = new RoundChangeCertificate.Builder(); roundChangeBuilder.appendRoundChangeMessage( proposerMessageFactory.createSignedRoundChangePayload( roundIdentifier, @@ -260,8 +260,6 @@ public class NewRoundMessageValidatorTest { differentProposal, Lists.newArrayList( validatorMessageFactory.createSignedPreparePayload( - roundIdentifier, proposedBlock.getHash()), - otherValidatorMessageFactory.createSignedPreparePayload( roundIdentifier, proposedBlock.getHash())))); // An earlier PrepareCert is added to ensure the path to find the latest PrepareCert @@ -277,8 +275,6 @@ public class NewRoundMessageValidatorTest { earlierProposal, Lists.newArrayList( validatorMessageFactory.createSignedPreparePayload( - earlierPreparedRound, proposedBlock.getHash()), - otherValidatorMessageFactory.createSignedPreparePayload( earlierPreparedRound, proposedBlock.getHash())))); final SignedData msg = @@ -287,9 +283,7 @@ public class NewRoundMessageValidatorTest { new RoundChangeCertificate( Lists.newArrayList( proposerMessageFactory.createSignedRoundChangePayload( - roundIdentifier, earlierPreparedCert), - proposerMessageFactory.createSignedRoundChangePayload( - roundIdentifier, preparedCert))), + roundIdentifier, earlierPreparedCert))), proposal); proposerMessageFactory.createSignedProposalPayload(roundIdentifier, proposedBlock);