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

NewRoundMessageValidator creates RoundChangeValidator with correct value #518

Merged
merged 4 commits into from
Jan 7, 2019
Merged

NewRoundMessageValidator creates RoundChangeValidator with correct value #518

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d3c56f4
Ensure RoundChangeValidators is created with correct value
Jan 7, 2019
ccf73b2
wip
Jan 7, 2019
04b6756
post review
Jan 7, 2019
f02e10b
Merge remote-tracking branch 'upstream/master' into prepare_quorum
Jan 7, 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
2 changes: 1 addition & 1 deletion ...rc/main/java/tech/pegasys/pantheon/consensus/ibft/validation/MessageValidatorFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ public RoundChangeMessageValidator createRoundChangeMessageValidator(
return new RoundChangeMessageValidator(
roundIdentifier -> createMessageValidator(roundIdentifier, parentHeader),
validators,
IbftHelpers.calculateRequiredValidatorQuorum(validators.size()),
IbftHelpers.calculateRequiredValidatorQuorum(validators.size()) - 1,
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do you want one less than the required quorum of validators? [magic number]

Copy link
Contributor

Choose a reason for hiding this comment

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

like above, comment would hep explain why this is being done

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This calc has been broken out to a helper function which is now used in many places (which is good).
Throughout IBFT, the number of prepare messages is 1 less than the actual 2/3 quorum as the Proposal is deemed to act as a prepare message (thus, there is also a rule that none of the prepare messages may come from the Proposer).

parentHeader.getNumber() + 1);
}

Expand Down
29 changes: 21 additions & 8 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 @@ -15,6 +15,7 @@
import static tech.pegasys.pantheon.consensus.ibft.IbftHelpers.findLatestPreparedCertificate;

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 @@ -24,6 +25,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 @@ -121,9 +123,11 @@ private boolean validateRoundChangeMessagesAndEnsureTargetRoundMatchesRoot(

for (final SignedData<RoundChangePayload> roundChangeMsg :
roundChangeCert.getRoundChangePayloads()) {
// RoundChangeValidator requires the "requisite Prepare Msg Count" (which is 1 less than
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: this comment could be incorporated into a method or constant, negating the need for its existence.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Has been moved out to IbftHelpers.prepareMessageCountFromQuorum()

// quorum)
final RoundChangeMessageValidator roundChangeValidator =
new RoundChangeMessageValidator(
messageValidatorFactory, validators, quorumSize, chainHeight);
messageValidatorFactory, validators, quorumSize - 1, chainHeight);

if (!roundChangeValidator.validateMessage(roundChangeMsg)) {
LOG.info("Invalid NewRound message, embedded RoundChange message failed validation.");
Expand All @@ -148,13 +152,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())) {

// The block in the latest
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
36 changes: 16 additions & 20 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 @@ -57,31 +56,31 @@ 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<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 @@ -128,7 +127,7 @@ public void newRoundFromNonProposerFails() {
}

@Test
public void newRoundTargettingRoundZeroFails() {
public void newRoundTargetingRoundZeroFails() {
msgBuilder.setRoundChangeIdentifier(
new ConsensusRoundIdentifier(roundIdentifier.getSequenceNumber(), 0));

Expand Down Expand Up @@ -181,8 +180,11 @@ public void newRoundWithMismatchOfRoundIdentifierToProposalMessageFails() {
@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 All @@ -205,7 +207,7 @@ public void newRoundWithProposalNotMatchingLatestRoundChangeFails() {
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(
Expand All @@ -222,7 +224,7 @@ public void roundChangeMessagesDoNotAllTargetRoundOfNewRoundMsgFails() {
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,
Expand Down Expand Up @@ -260,8 +262,6 @@ public void lastestPreparedCertificateMatchesNewRoundProposalIsSuccessful() {
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
Expand All @@ -277,8 +277,6 @@ public void lastestPreparedCertificateMatchesNewRoundProposalIsSuccessful() {
earlierProposal,
Lists.newArrayList(
validatorMessageFactory.createSignedPreparePayload(
earlierPreparedRound, proposedBlock.getHash()),
otherValidatorMessageFactory.createSignedPreparePayload(
earlierPreparedRound, proposedBlock.getHash()))));

final SignedData<NewRoundPayload> msg =
Expand All @@ -287,9 +285,7 @@ public void lastestPreparedCertificateMatchesNewRoundProposalIsSuccessful() {
new RoundChangeCertificate(
Lists.newArrayList(
proposerMessageFactory.createSignedRoundChangePayload(
roundIdentifier, earlierPreparedCert),
proposerMessageFactory.createSignedRoundChangePayload(
roundIdentifier, preparedCert))),
roundIdentifier, earlierPreparedCert))),
proposal);
proposerMessageFactory.createSignedProposalPayload(roundIdentifier, proposedBlock);

Expand Down