From fbd933fe0793dcb0931d493029f40a22a53f156f Mon Sep 17 00:00:00 2001 From: tmohay <37158202+rain-on@users.noreply.github.com> Date: Thu, 10 Jan 2019 07:26:34 +1100 Subject: [PATCH] NewRoundMessageValidator ignores Round Number when comparing blocks (#523) * NewRoundMessageValidator ignores Round Number when comparing blocks NewRoundMessageValidator previously ensured the block in the newest PreparedCertificate was the same as that in the Proposal by comparing block hashes. However, the hash-function for received IBFT blocks includes round number - thus the blocks will never be deemed 'equal'. As such, the blocks must be compared using a hash function which ignores the round number - i.e. the OnChain function. --- .../validation/NewRoundMessageValidator.java | 39 ++++++----- .../NewRoundMessageValidatorTest.java | 66 +++++++++---------- 2 files changed, 56 insertions(+), 49 deletions(-) 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 cf9f62d062..f0cbc9b9ba 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 @@ -16,6 +16,7 @@ import static tech.pegasys.pantheon.consensus.ibft.IbftHelpers.findLatestPrepare import static tech.pegasys.pantheon.consensus.ibft.IbftHelpers.prepareMessageCountForQuorum; import tech.pegasys.pantheon.consensus.ibft.ConsensusRoundIdentifier; +import tech.pegasys.pantheon.consensus.ibft.IbftBlockHashing; import tech.pegasys.pantheon.consensus.ibft.blockcreation.ProposerSelector; import tech.pegasys.pantheon.consensus.ibft.ibftmessagedata.NewRoundPayload; import tech.pegasys.pantheon.consensus.ibft.ibftmessagedata.PreparedCertificate; @@ -25,6 +26,7 @@ import tech.pegasys.pantheon.consensus.ibft.ibftmessagedata.RoundChangePayload; import tech.pegasys.pantheon.consensus.ibft.ibftmessagedata.SignedData; import tech.pegasys.pantheon.consensus.ibft.validation.RoundChangeMessageValidator.MessageValidatorForHeightFactory; import tech.pegasys.pantheon.ethereum.core.Address; +import tech.pegasys.pantheon.ethereum.core.Hash; import java.util.Collection; import java.util.Optional; @@ -77,15 +79,11 @@ public class NewRoundMessageValidator { return false; } - final SignedData proposalMessage = payload.getProposalPayload(); - - if (!msg.getSender().equals(proposalMessage.getSender())) { - LOG.info("Invalid NewRound message, embedded Proposal message not signed by sender."); - return false; - } - - if (!proposalMessage.getPayload().getRoundIdentifier().equals(rootRoundIdentifier)) { - LOG.info("Invalid NewRound message, embedded Proposal has mismatched round."); + final SignedData proposalPayload = payload.getProposalPayload(); + final MessageValidator proposalValidator = + messageValidatorFactory.createAt(rootRoundIdentifier); + if (!proposalValidator.addSignedProposalPayload(proposalPayload)) { + LOG.info("Invalid NewRound message, embedded proposal failed validation"); return false; } @@ -152,13 +150,22 @@ public class NewRoundMessageValidator { "No round change messages have a preparedCertificate, any valid block may be proposed."); return true; } - if (!latestPreparedCertificate - .get() - .getProposalPayload() - .getPayload() - .getBlock() - .getHash() - .equals(payload.getProposalPayload().getPayload().getBlock().getHash())) { + + // Get the hash of the block in latest prepareCert, not including the Round field. + final Hash roundAgnosticBlockHashPreparedCert = + IbftBlockHashing.calculateHashOfIbftBlockOnChain( + latestPreparedCertificate + .get() + .getProposalPayload() + .getPayload() + .getBlock() + .getHeader()); + + final Hash roundAgnosticBlockHashProposal = + IbftBlockHashing.calculateHashOfIbftBlockOnChain( + payload.getProposalPayload().getPayload().getBlock().getHeader()); + + if (!roundAgnosticBlockHashPreparedCert.equals(roundAgnosticBlockHashProposal)) { LOG.info( "Invalid NewRound message, block in latest RoundChange does not match proposed block."); return false; 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 a3df85efb8..b77d3be9eb 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 @@ -31,7 +31,6 @@ import tech.pegasys.pantheon.consensus.ibft.validation.RoundChangeMessageValidat import tech.pegasys.pantheon.crypto.SECP256K1.KeyPair; import tech.pegasys.pantheon.ethereum.core.Address; import tech.pegasys.pantheon.ethereum.core.Block; -import tech.pegasys.pantheon.ethereum.core.Hash; import tech.pegasys.pantheon.ethereum.core.Util; import java.util.Collections; @@ -49,7 +48,6 @@ public class NewRoundMessageValidatorTest { private final KeyPair otherValidatorKey = KeyPair.generate(); private final MessageFactory proposerMessageFactory = new MessageFactory(proposerKey); private final MessageFactory validatorMessageFactory = new MessageFactory(validatorKey); - private final MessageFactory otherValidatorMessageFactory = new MessageFactory(otherValidatorKey); private final Address proposerAddress = Util.publicKeyToAddress(proposerKey.getPublicKey()); private final List
validators = Lists.newArrayList(); private final long chainHeight = 2; @@ -57,16 +55,14 @@ public class NewRoundMessageValidatorTest { new ConsensusRoundIdentifier(chainHeight, 4); private NewRoundMessageValidator validator; - private final Block proposedBlock = mock(Block.class); private final ProposerSelector proposerSelector = mock(ProposerSelector.class); private final MessageValidatorForHeightFactory validatorFactory = mock(MessageValidatorForHeightFactory.class); private final MessageValidator messageValidator = mock(MessageValidator.class); - private final SignedData validMsg = - createValidNewRoundMessageSignedBy(proposerKey); - private final NewRoundPayload.Builder msgBuilder = - NewRoundPayload.Builder.fromExisting(validMsg.getPayload()); + private Block proposedBlock; + private SignedData validMsg; + private NewRoundPayload.Builder msgBuilder; @Before public void setup() { @@ -74,14 +70,16 @@ public class NewRoundMessageValidatorTest { validators.add(Util.publicKeyToAddress(validatorKey.getPublicKey())); validators.add(Util.publicKeyToAddress(otherValidatorKey.getPublicKey())); + proposedBlock = TestHelpers.createProposalBlock(validators, roundIdentifier.getRoundNumber()); + validMsg = createValidNewRoundMessageSignedBy(proposerKey); + msgBuilder = NewRoundPayload.Builder.fromExisting(validMsg.getPayload()); + when(proposerSelector.selectProposerForRound(any())).thenReturn(proposerAddress); when(validatorFactory.createAt(any())).thenReturn(messageValidator); when(messageValidator.addSignedProposalPayload(any())).thenReturn(true); when(messageValidator.validatePrepareMessage(any())).thenReturn(true); - when(proposedBlock.getHash()).thenReturn(Hash.fromHexStringLenient("1")); - validator = new NewRoundMessageValidator( validators, proposerSelector, validatorFactory, 1, chainHeight); @@ -147,16 +145,6 @@ public class NewRoundMessageValidatorTest { assertThat(validator.validateNewRoundMessage(inValidMsg)).isFalse(); } - @Test - public void newRoundContainingProposalFromDifferentValidatorFails() { - msgBuilder.setProposalPayload( - validatorMessageFactory.createSignedProposalPayload(roundIdentifier, proposedBlock)); - - final SignedData inValidMsg = signPayload(msgBuilder.build(), proposerKey); - - assertThat(validator.validateNewRoundMessage(inValidMsg)).isFalse(); - } - @Test public void newRoundWithEmptyRoundChangeCertificateFails() { msgBuilder.setRoundChangeCertificate(new RoundChangeCertificate(Collections.emptyList())); @@ -166,23 +154,14 @@ public class NewRoundMessageValidatorTest { assertThat(validator.validateNewRoundMessage(inValidMsg)).isFalse(); } - @Test - public void newRoundWithMismatchOfRoundIdentifierToProposalMessageFails() { - final ConsensusRoundIdentifier mismatchedRound = TestHelpers.createFrom(roundIdentifier, 0, -1); - - msgBuilder.setProposalPayload( - proposerMessageFactory.createSignedProposalPayload(mismatchedRound, proposedBlock)); - - final SignedData inValidMsg = signPayload(msgBuilder.build(), proposerKey); - - assertThat(validator.validateNewRoundMessage(inValidMsg)).isFalse(); - } - @Test public void newRoundWithProposalNotMatchingLatestRoundChangeFails() { final ConsensusRoundIdentifier preparedRound = TestHelpers.createFrom(roundIdentifier, 0, -1); - final Block prevProposedBlock = mock(Block.class); - when(prevProposedBlock.getHash()).thenReturn(Hash.fromHexStringLenient("2")); + // The previous block has been constructed with less validators, so is thus not identical + // to the block in the new proposal (so should fail). + final Block prevProposedBlock = + TestHelpers.createProposalBlock(validators.subList(0, 1), roundIdentifier.getRoundNumber()); + final PreparedCertificate misMatchedPreparedCertificate = new PreparedCertificate( proposerMessageFactory.createSignedProposalPayload(preparedRound, prevProposedBlock), @@ -289,4 +268,25 @@ public class NewRoundMessageValidatorTest { assertThat(validator.validateNewRoundMessage(msg)).isTrue(); } + + @Test + public void embeddedProposalFailsValidation() { + when(messageValidator.addSignedProposalPayload(any())).thenReturn(false, true); + + final SignedData proposal = + proposerMessageFactory.createSignedProposalPayload(roundIdentifier, proposedBlock); + + final SignedData msg = + proposerMessageFactory.createSignedNewRoundPayload( + roundIdentifier, + new RoundChangeCertificate( + Lists.newArrayList( + proposerMessageFactory.createSignedRoundChangePayload( + roundIdentifier, Optional.empty()), + validatorMessageFactory.createSignedRoundChangePayload( + roundIdentifier, Optional.empty()))), + proposal); + + assertThat(validator.validateNewRoundMessage(msg)).isFalse(); + } }