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

NewRoundMessageValidator ignores Round Number when comparing blocks #523

Merged
merged 7 commits into from
Jan 9, 2019
Merged

NewRoundMessageValidator ignores Round Number when comparing blocks #523

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
2b75a65
NewRoundMessageValidator ignores Round Number when comparing blocks
Jan 7, 2019
aef9edf
Updated to validate Proposal in NewRoundMessage
Jan 8, 2019
7d153b1
Added test for failure mode
Jan 8, 2019
f9aa0e4
fix spotless
Jan 8, 2019
acdf522
remove superfluous tests
Jan 8, 2019
942f036
Merge branch 'master' into new_round_hash
rain-on Jan 9, 2019
2fea9b7
Merge branch 'master' into new_round_hash
rain-on Jan 9, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 23 additions & 16 deletions ...c/main/java/tech/pegasys/pantheon/consensus/ibft/validation/NewRoundMessageValidator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
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;
Expand All @@ -25,6 +26,7 @@
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;
Expand Down Expand Up @@ -77,15 +79,11 @@ public boolean validateNewRoundMessage(final SignedData<NewRoundPayload> msg) {
return false;
}

final SignedData<ProposalPayload> 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> proposalPayload = payload.getProposalPayload();
final MessageValidator proposalValidator =
messageValidatorFactory.createAt(rootRoundIdentifier);
if (!proposalValidator.addSignedProposalPayload(proposalPayload)) {
LOG.info("Invalid NewRound message, embedded proposal failed validation");
return false;
}

Expand Down Expand Up @@ -152,13 +150,22 @@ private boolean validateProposalMessageMatchesLatestPrepareCertificate(
"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;
Expand Down
66 changes: 33 additions & 33 deletions ...st/java/tech/pegasys/pantheon/consensus/ibft/validation/NewRoundMessageValidatorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@
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;
Expand All @@ -49,39 +48,38 @@ 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<Address> validators = Lists.newArrayList();
private final long chainHeight = 2;
private final ConsensusRoundIdentifier roundIdentifier =
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<NewRoundPayload> validMsg =
createValidNewRoundMessageSignedBy(proposerKey);
private final NewRoundPayload.Builder msgBuilder =
NewRoundPayload.Builder.fromExisting(validMsg.getPayload());
private Block proposedBlock;
private SignedData<NewRoundPayload> validMsg;
private NewRoundPayload.Builder msgBuilder;

@Before
public void setup() {
validators.add(Util.publicKeyToAddress(proposerKey.getPublicKey()));
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);
Expand Down Expand Up @@ -147,16 +145,6 @@ public void newRoundTargetingDifferentSequenceNumberFails() {
assertThat(validator.validateNewRoundMessage(inValidMsg)).isFalse();
}

@Test
jframe marked this conversation as resolved.
Show resolved Hide resolved
public void newRoundContainingProposalFromDifferentValidatorFails() {
msgBuilder.setProposalPayload(
validatorMessageFactory.createSignedProposalPayload(roundIdentifier, proposedBlock));

final SignedData<NewRoundPayload> inValidMsg = signPayload(msgBuilder.build(), proposerKey);

assertThat(validator.validateNewRoundMessage(inValidMsg)).isFalse();
}

@Test
public void newRoundWithEmptyRoundChangeCertificateFails() {
msgBuilder.setRoundChangeCertificate(new RoundChangeCertificate(Collections.emptyList()));
Expand All @@ -166,23 +154,14 @@ public void newRoundWithEmptyRoundChangeCertificateFails() {
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<NewRoundPayload> 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),
Expand Down Expand Up @@ -289,4 +268,25 @@ public void lastestPreparedCertificateMatchesNewRoundProposalIsSuccessful() {

assertThat(validator.validateNewRoundMessage(msg)).isTrue();
Copy link
Contributor

Choose a reason for hiding this comment

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

Based on the name for the this test and the added comments referring to failing validation, should this assertion be a isFalse() rather than isTrue()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It looks like this test is proving the successful case when we have multiple PrepareCertificates, so ATM, I think its correct.

Copy link
Contributor

Choose a reason for hiding this comment

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

My bad, in GitHub interface I'd assumed choosing to expand the collapsed section fully expanded ...turns out multiple clicks were needed, the isTrue() is in the succeed test, not the fail test.

}

@Test
public void embeddedProposalFailsValidation() {
when(messageValidator.addSignedProposalPayload(any())).thenReturn(false, true);

final SignedData<ProposalPayload> proposal =
proposerMessageFactory.createSignedProposalPayload(roundIdentifier, proposedBlock);

final SignedData<NewRoundPayload> msg =
proposerMessageFactory.createSignedNewRoundPayload(
roundIdentifier,
new RoundChangeCertificate(
Lists.newArrayList(
proposerMessageFactory.createSignedRoundChangePayload(
roundIdentifier, Optional.empty()),
validatorMessageFactory.createSignedRoundChangePayload(
roundIdentifier, Optional.empty()))),
proposal);

assertThat(validator.validateNewRoundMessage(msg)).isFalse();
}
}