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(); + } }