From d6458c0e3f0e524b2115428586343900f0eaf708 Mon Sep 17 00:00:00 2001 From: tmohay Date: Wed, 27 Feb 2019 10:30:41 +1100 Subject: [PATCH 1/3] RoundChangeCertificateValidator requires unique authors --- .../RoundChangeCertificateValidator.java | 16 +++++++++ .../RoundChangeCertificateValidatorTest.java | 34 ++++++++++++++----- 2 files changed, 42 insertions(+), 8 deletions(-) diff --git a/consensus/ibft/src/main/java/tech/pegasys/pantheon/consensus/ibft/validation/RoundChangeCertificateValidator.java b/consensus/ibft/src/main/java/tech/pegasys/pantheon/consensus/ibft/validation/RoundChangeCertificateValidator.java index 5e5833bccd..0227c5632c 100644 --- a/consensus/ibft/src/main/java/tech/pegasys/pantheon/consensus/ibft/validation/RoundChangeCertificateValidator.java +++ b/consensus/ibft/src/main/java/tech/pegasys/pantheon/consensus/ibft/validation/RoundChangeCertificateValidator.java @@ -58,6 +58,10 @@ public boolean validateRoundChangeMessagesAndEnsureTargetRoundMatchesRoot( final Collection> roundChangeMsgs = roundChangeCert.getRoundChangePayloads(); + if (!validateDistinctAuthors(roundChangeMsgs)) { + return false; + } + if (roundChangeMsgs.size() < quorum) { LOG.info("Invalid RoundChangeCertificate, insufficient RoundChange messages."); return false; @@ -84,6 +88,18 @@ public boolean validateRoundChangeMessagesAndEnsureTargetRoundMatchesRoot( return true; } + private boolean validateDistinctAuthors( + final Collection> roundChangeMsgs) { + final long distinctAuthorCount = + roundChangeMsgs.stream().map(SignedData::getAuthor).distinct().count(); + + if (distinctAuthorCount != roundChangeMsgs.size()) { + LOG.info("Invalid RoundChangeCertificate, multiple RoundChanges from the same author."); + return false; + } + return true; + } + public boolean validateProposalMessageMatchesLatestPrepareCertificate( final RoundChangeCertificate roundChangeCert, final Block proposedBlock) { diff --git a/consensus/ibft/src/test/java/tech/pegasys/pantheon/consensus/ibft/validation/RoundChangeCertificateValidatorTest.java b/consensus/ibft/src/test/java/tech/pegasys/pantheon/consensus/ibft/validation/RoundChangeCertificateValidatorTest.java index 910f91c508..770e83aec2 100644 --- a/consensus/ibft/src/test/java/tech/pegasys/pantheon/consensus/ibft/validation/RoundChangeCertificateValidatorTest.java +++ b/consensus/ibft/src/test/java/tech/pegasys/pantheon/consensus/ibft/validation/RoundChangeCertificateValidatorTest.java @@ -13,6 +13,7 @@ package tech.pegasys.pantheon.consensus.ibft.validation; import static java.util.Collections.singletonList; +import static java.util.Optional.empty; import static org.assertj.core.api.Assertions.assertThat; import static org.mockito.ArgumentMatchers.any; import static org.mockito.Mockito.mock; @@ -84,9 +85,9 @@ public void roundChangeMessagesDoNotAllTargetRoundFails() { final RoundChangeCertificate.Builder roundChangeBuilder = new RoundChangeCertificate.Builder(); roundChangeBuilder.appendRoundChangeMessage( - proposerMessageFactory.createRoundChange(roundIdentifier, Optional.empty())); + proposerMessageFactory.createRoundChange(roundIdentifier, empty())); roundChangeBuilder.appendRoundChangeMessage( - proposerMessageFactory.createRoundChange(prevRound, Optional.empty())); + proposerMessageFactory.createRoundChange(prevRound, empty())); assertThat( validator.validateRoundChangeMessagesAndEnsureTargetRoundMatchesRoot( @@ -104,8 +105,7 @@ public void invalidPrepareMessageInOnePrepareCertificateFails() { roundIdentifier, Optional.of( new PreparedRoundArtifacts( - proposerMessageFactory.createProposal( - prevRound, proposedBlock, Optional.empty()), + proposerMessageFactory.createProposal(prevRound, proposedBlock, empty()), Lists.newArrayList( validatorMessageFactory.createPrepare( prevRound, proposedBlock.getHash())))))); @@ -130,8 +130,7 @@ public void detectsTheSuppliedBlockIsNotInLatestPrepareCertificate() { final PreparedRoundArtifacts mismatchedRoundArtefacts = new PreparedRoundArtifacts( - proposerMessageFactory.createProposal( - preparedRound, prevProposedBlock, Optional.empty()), + proposerMessageFactory.createProposal(preparedRound, prevProposedBlock, empty()), singletonList( validatorMessageFactory.createPrepare(preparedRound, prevProposedBlock.getHash()))); @@ -154,7 +153,7 @@ public void correctlyMatchesBlockAgainstLatestInRoundChangeCertificate() { TestHelpers.createFrom(roundIdentifier, 0, -1); final Block latterBlock = TestHelpers.createProposalBlock(validators, latterPrepareRound); final Proposal latterProposal = - proposerMessageFactory.createProposal(latterPrepareRound, latterBlock, Optional.empty()); + proposerMessageFactory.createProposal(latterPrepareRound, latterBlock, empty()); final Optional latterTerminatedRoundArtefacts = Optional.of( new PreparedRoundArtifacts( @@ -171,7 +170,7 @@ public void correctlyMatchesBlockAgainstLatestInRoundChangeCertificate() { final Block earlierBlock = TestHelpers.createProposalBlock(validators.subList(0, 1), earlierPreparedRound); final Proposal earlierProposal = - proposerMessageFactory.createProposal(earlierPreparedRound, earlierBlock, Optional.empty()); + proposerMessageFactory.createProposal(earlierPreparedRound, earlierBlock, empty()); final Optional earlierTerminatedRoundArtefacts = Optional.of( new PreparedRoundArtifacts( @@ -200,4 +199,23 @@ public void correctlyMatchesBlockAgainstLatestInRoundChangeCertificate() { roundChangeCert, latterBlock)) .isTrue(); } + + @Test + public void roundChangeCertificateWithTwoRoundChangesFromTheSameAuthorFailsValidation() { + + final RoundChangeCertificate roundChangeCert = + new RoundChangeCertificate( + org.assertj.core.util.Lists.newArrayList( + proposerMessageFactory + .createRoundChange(roundIdentifier, empty()) + .getSignedPayload(), + proposerMessageFactory + .createRoundChange(roundIdentifier, empty()) + .getSignedPayload())); + + assertThat( + validator.validateRoundChangeMessagesAndEnsureTargetRoundMatchesRoot( + roundIdentifier, roundChangeCert)) + .isFalse(); + } } From 85c1bef15232e8151e2a65a7143836517634d14c Mon Sep 17 00:00:00 2001 From: tmohay Date: Wed, 27 Feb 2019 12:58:53 +1100 Subject: [PATCH 2/3] post review --- .../ibft/validation/RoundChangeCertificateValidator.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/consensus/ibft/src/main/java/tech/pegasys/pantheon/consensus/ibft/validation/RoundChangeCertificateValidator.java b/consensus/ibft/src/main/java/tech/pegasys/pantheon/consensus/ibft/validation/RoundChangeCertificateValidator.java index 0227c5632c..256758942d 100644 --- a/consensus/ibft/src/main/java/tech/pegasys/pantheon/consensus/ibft/validation/RoundChangeCertificateValidator.java +++ b/consensus/ibft/src/main/java/tech/pegasys/pantheon/consensus/ibft/validation/RoundChangeCertificateValidator.java @@ -58,7 +58,7 @@ public boolean validateRoundChangeMessagesAndEnsureTargetRoundMatchesRoot( final Collection> roundChangeMsgs = roundChangeCert.getRoundChangePayloads(); - if (!validateDistinctAuthors(roundChangeMsgs)) { + if (hasDuplicateAuthors(roundChangeMsgs)) { return false; } @@ -88,7 +88,7 @@ public boolean validateRoundChangeMessagesAndEnsureTargetRoundMatchesRoot( return true; } - private boolean validateDistinctAuthors( + private boolean hasDuplicateAuthors( final Collection> roundChangeMsgs) { final long distinctAuthorCount = roundChangeMsgs.stream().map(SignedData::getAuthor).distinct().count(); From 05b3e1f85e4bfe648d4e01b662599c8761d6cd92 Mon Sep 17 00:00:00 2001 From: tmohay Date: Wed, 27 Feb 2019 14:16:08 +1100 Subject: [PATCH 3/3] fix backward logic --- .../ibft/validation/RoundChangeCertificateValidator.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/consensus/ibft/src/main/java/tech/pegasys/pantheon/consensus/ibft/validation/RoundChangeCertificateValidator.java b/consensus/ibft/src/main/java/tech/pegasys/pantheon/consensus/ibft/validation/RoundChangeCertificateValidator.java index 256758942d..c7ca51b09d 100644 --- a/consensus/ibft/src/main/java/tech/pegasys/pantheon/consensus/ibft/validation/RoundChangeCertificateValidator.java +++ b/consensus/ibft/src/main/java/tech/pegasys/pantheon/consensus/ibft/validation/RoundChangeCertificateValidator.java @@ -95,9 +95,9 @@ private boolean hasDuplicateAuthors( if (distinctAuthorCount != roundChangeMsgs.size()) { LOG.info("Invalid RoundChangeCertificate, multiple RoundChanges from the same author."); - return false; + return true; } - return true; + return false; } public boolean validateProposalMessageMatchesLatestPrepareCertificate(