From 2fc38e737384c0c03796e50fefdf9186cdf5e6c6 Mon Sep 17 00:00:00 2001 From: tmohay <37158202+rain-on@users.noreply.github.com> Date: Wed, 19 Dec 2018 19:28:39 +1100 Subject: [PATCH] Update RoundChangeManager correctly create its message validator (#450) The RoundChangeManager was incorrectly creating its internal RoundChangeMessageValidator with "quorum" number of Prepare messages required - however, given the ProposalMessage in the Prepared- Certificate represents a valid node. --- .../ibft/statemachine/RoundChangeManager.java | 2 +- .../ibft/validation/MessageValidator.java | 2 +- .../pantheon/consensus/ibft/TestHelpers.java | 14 ++--- .../ibftmessagedata/NewRoundPayloadTest.java | 9 ++- .../PreparedCertificateTest.java | 6 +- .../ibftmessagedata/ProposalPayloadTest.java | 6 +- .../RoundChangeCertificateTest.java | 5 +- .../RoundChangePayloadTest.java | 6 +- .../statemachine/RoundChangeManagerTest.java | 60 ++++++++++++++++++- 9 files changed, 93 insertions(+), 17 deletions(-) diff --git a/consensus/ibft/src/main/java/tech/pegasys/pantheon/consensus/ibft/statemachine/RoundChangeManager.java b/consensus/ibft/src/main/java/tech/pegasys/pantheon/consensus/ibft/statemachine/RoundChangeManager.java index 4d0adc2b4c..a439674217 100644 --- a/consensus/ibft/src/main/java/tech/pegasys/pantheon/consensus/ibft/statemachine/RoundChangeManager.java +++ b/consensus/ibft/src/main/java/tech/pegasys/pantheon/consensus/ibft/statemachine/RoundChangeManager.java @@ -89,7 +89,7 @@ public class RoundChangeManager { this.quorumSize = IbftHelpers.calculateRequiredValidatorQuorum(validators.size()); this.roundChangeMessageValidator = new RoundChangeMessageValidator( - messageValidityFactory, validators, quorumSize, sequenceNumber); + messageValidityFactory, validators, quorumSize - 1, sequenceNumber); } /** diff --git a/consensus/ibft/src/main/java/tech/pegasys/pantheon/consensus/ibft/validation/MessageValidator.java b/consensus/ibft/src/main/java/tech/pegasys/pantheon/consensus/ibft/validation/MessageValidator.java index 061f766383..908d632a79 100644 --- a/consensus/ibft/src/main/java/tech/pegasys/pantheon/consensus/ibft/validation/MessageValidator.java +++ b/consensus/ibft/src/main/java/tech/pegasys/pantheon/consensus/ibft/validation/MessageValidator.java @@ -99,7 +99,7 @@ public class MessageValidator { final Block proposedBlock = msg.getPayload().getBlock(); if (!headerValidator.validateHeader( proposedBlock.getHeader(), parentHeader, protocolContext, FULL)) { - LOG.info("Invalid Prepare message, block did not pass header validation."); + LOG.info("Invalid Proposal message, block did not pass header validation."); return false; } diff --git a/consensus/ibft/src/test/java/tech/pegasys/pantheon/consensus/ibft/TestHelpers.java b/consensus/ibft/src/test/java/tech/pegasys/pantheon/consensus/ibft/TestHelpers.java index f868903165..fb208e7357 100644 --- a/consensus/ibft/src/test/java/tech/pegasys/pantheon/consensus/ibft/TestHelpers.java +++ b/consensus/ibft/src/test/java/tech/pegasys/pantheon/consensus/ibft/TestHelpers.java @@ -12,18 +12,16 @@ */ package tech.pegasys.pantheon.consensus.ibft; -import static java.util.Collections.singletonList; - import tech.pegasys.pantheon.ethereum.core.Address; import tech.pegasys.pantheon.ethereum.core.Block; import tech.pegasys.pantheon.ethereum.core.BlockDataGenerator; import tech.pegasys.pantheon.ethereum.core.BlockDataGenerator.BlockOptions; import tech.pegasys.pantheon.util.bytes.BytesValue; +import java.util.Collections; +import java.util.List; import java.util.Optional; -import com.google.common.collect.Lists; - public class TestHelpers { public static ConsensusRoundIdentifier createFrom( @@ -32,14 +30,14 @@ public class TestHelpers { initial.getSequenceNumber() + offSetSequence, initial.getRoundNumber() + offSetRound); } - public static Block createProposalBlock() { + public static Block createProposalBlock(final List
validators, final int round) { final BytesValue extraData = new IbftExtraData( BytesValue.wrap(new byte[32]), - Lists.newArrayList(), + Collections.emptyList(), Optional.empty(), - 0, - singletonList(Address.fromHexString(String.format("%020d", 1)))) + round, + validators) .encode(); final BlockOptions blockOptions = BlockOptions.create() diff --git a/consensus/ibft/src/test/java/tech/pegasys/pantheon/consensus/ibft/ibftmessagedata/NewRoundPayloadTest.java b/consensus/ibft/src/test/java/tech/pegasys/pantheon/consensus/ibft/ibftmessagedata/NewRoundPayloadTest.java index 3da7d9f7fb..84ae816ec7 100644 --- a/consensus/ibft/src/test/java/tech/pegasys/pantheon/consensus/ibft/ibftmessagedata/NewRoundPayloadTest.java +++ b/consensus/ibft/src/test/java/tech/pegasys/pantheon/consensus/ibft/ibftmessagedata/NewRoundPayloadTest.java @@ -12,12 +12,14 @@ */ package tech.pegasys.pantheon.consensus.ibft.ibftmessagedata; +import static java.util.Collections.singletonList; import static org.assertj.core.api.Assertions.assertThat; import tech.pegasys.pantheon.consensus.ibft.ConsensusRoundIdentifier; import tech.pegasys.pantheon.consensus.ibft.TestHelpers; import tech.pegasys.pantheon.consensus.ibft.ibftmessage.IbftV2; import tech.pegasys.pantheon.crypto.SECP256K1.Signature; +import tech.pegasys.pantheon.ethereum.core.AddressHelpers; import tech.pegasys.pantheon.ethereum.core.Block; import tech.pegasys.pantheon.ethereum.core.Hash; import tech.pegasys.pantheon.ethereum.rlp.BytesValueRLPOutput; @@ -32,12 +34,14 @@ import org.assertj.core.util.Lists; import org.junit.Test; public class NewRoundPayloadTest { + private static final ConsensusRoundIdentifier ROUND_IDENTIFIER = new ConsensusRoundIdentifier(0x1234567890ABCDEFL, 0xFEDCBA98); @Test public void roundTripRlpWithNoRoundChangePayloads() { - final Block block = TestHelpers.createProposalBlock(); + final Block block = + TestHelpers.createProposalBlock(singletonList(AddressHelpers.ofValue(1)), 0); final ProposalPayload proposalPayload = new ProposalPayload(ROUND_IDENTIFIER, block); final Signature signature = Signature.create(BigInteger.ONE, BigInteger.TEN, (byte) 0); final SignedData proposalPayloadSignedData = @@ -60,7 +64,8 @@ public class NewRoundPayloadTest { @Test public void roundTripRlpWithRoundChangePayloads() { - final Block block = TestHelpers.createProposalBlock(); + final Block block = + TestHelpers.createProposalBlock(singletonList(AddressHelpers.ofValue(1)), 0); final ProposalPayload proposalPayload = new ProposalPayload(ROUND_IDENTIFIER, block); final Signature signature = Signature.create(BigInteger.ONE, BigInteger.TEN, (byte) 0); final SignedData signedProposal = SignedData.from(proposalPayload, signature); diff --git a/consensus/ibft/src/test/java/tech/pegasys/pantheon/consensus/ibft/ibftmessagedata/PreparedCertificateTest.java b/consensus/ibft/src/test/java/tech/pegasys/pantheon/consensus/ibft/ibftmessagedata/PreparedCertificateTest.java index 73010248b2..9a56adab83 100644 --- a/consensus/ibft/src/test/java/tech/pegasys/pantheon/consensus/ibft/ibftmessagedata/PreparedCertificateTest.java +++ b/consensus/ibft/src/test/java/tech/pegasys/pantheon/consensus/ibft/ibftmessagedata/PreparedCertificateTest.java @@ -12,11 +12,13 @@ */ package tech.pegasys.pantheon.consensus.ibft.ibftmessagedata; +import static java.util.Collections.singletonList; import static org.assertj.core.api.Assertions.assertThat; import tech.pegasys.pantheon.consensus.ibft.ConsensusRoundIdentifier; import tech.pegasys.pantheon.consensus.ibft.TestHelpers; import tech.pegasys.pantheon.crypto.SECP256K1.Signature; +import tech.pegasys.pantheon.ethereum.core.AddressHelpers; import tech.pegasys.pantheon.ethereum.core.Block; import tech.pegasys.pantheon.ethereum.core.Hash; import tech.pegasys.pantheon.ethereum.rlp.BytesValueRLPOutput; @@ -31,6 +33,7 @@ import org.assertj.core.util.Lists; import org.junit.Test; public class PreparedCertificateTest { + private static final ConsensusRoundIdentifier ROUND_IDENTIFIER = new ConsensusRoundIdentifier(0x1234567890ABCDEFL, 0xFEDCBA98); @@ -74,7 +77,8 @@ public class PreparedCertificateTest { } private SignedData signedProposal() { - final Block block = TestHelpers.createProposalBlock(); + final Block block = + TestHelpers.createProposalBlock(singletonList(AddressHelpers.ofValue(1)), 0); final ProposalPayload proposalPayload = new ProposalPayload(ROUND_IDENTIFIER, block); final Signature signature = Signature.create(BigInteger.ONE, BigInteger.TEN, (byte) 0); return SignedData.from(proposalPayload, signature); diff --git a/consensus/ibft/src/test/java/tech/pegasys/pantheon/consensus/ibft/ibftmessagedata/ProposalPayloadTest.java b/consensus/ibft/src/test/java/tech/pegasys/pantheon/consensus/ibft/ibftmessagedata/ProposalPayloadTest.java index d30e25d016..222fb31647 100644 --- a/consensus/ibft/src/test/java/tech/pegasys/pantheon/consensus/ibft/ibftmessagedata/ProposalPayloadTest.java +++ b/consensus/ibft/src/test/java/tech/pegasys/pantheon/consensus/ibft/ibftmessagedata/ProposalPayloadTest.java @@ -12,11 +12,13 @@ */ package tech.pegasys.pantheon.consensus.ibft.ibftmessagedata; +import static java.util.Collections.singletonList; import static org.assertj.core.api.Assertions.assertThat; import tech.pegasys.pantheon.consensus.ibft.ConsensusRoundIdentifier; import tech.pegasys.pantheon.consensus.ibft.TestHelpers; import tech.pegasys.pantheon.consensus.ibft.ibftmessage.IbftV2; +import tech.pegasys.pantheon.ethereum.core.AddressHelpers; import tech.pegasys.pantheon.ethereum.core.Block; import tech.pegasys.pantheon.ethereum.rlp.BytesValueRLPOutput; import tech.pegasys.pantheon.ethereum.rlp.RLP; @@ -25,12 +27,14 @@ import tech.pegasys.pantheon.ethereum.rlp.RLPInput; import org.junit.Test; public class ProposalPayloadTest { + private static final ConsensusRoundIdentifier ROUND_IDENTIFIER = new ConsensusRoundIdentifier(0x1234567890ABCDEFL, 0xFEDCBA98); @Test public void roundTripRlp() { - final Block block = TestHelpers.createProposalBlock(); + final Block block = + TestHelpers.createProposalBlock(singletonList(AddressHelpers.ofValue(1)), 0); final ProposalPayload expectedProposalPayload = new ProposalPayload(ROUND_IDENTIFIER, block); final BytesValueRLPOutput rlpOut = new BytesValueRLPOutput(); expectedProposalPayload.writeTo(rlpOut); diff --git a/consensus/ibft/src/test/java/tech/pegasys/pantheon/consensus/ibft/ibftmessagedata/RoundChangeCertificateTest.java b/consensus/ibft/src/test/java/tech/pegasys/pantheon/consensus/ibft/ibftmessagedata/RoundChangeCertificateTest.java index 6c4580b297..b10e6d929e 100644 --- a/consensus/ibft/src/test/java/tech/pegasys/pantheon/consensus/ibft/ibftmessagedata/RoundChangeCertificateTest.java +++ b/consensus/ibft/src/test/java/tech/pegasys/pantheon/consensus/ibft/ibftmessagedata/RoundChangeCertificateTest.java @@ -12,12 +12,14 @@ */ package tech.pegasys.pantheon.consensus.ibft.ibftmessagedata; +import static java.util.Collections.singletonList; import static org.assertj.core.api.Assertions.assertThat; import tech.pegasys.pantheon.consensus.ibft.ConsensusRoundIdentifier; import tech.pegasys.pantheon.consensus.ibft.TestHelpers; import tech.pegasys.pantheon.consensus.ibft.ibftmessage.IbftV2; import tech.pegasys.pantheon.crypto.SECP256K1.Signature; +import tech.pegasys.pantheon.ethereum.core.AddressHelpers; import tech.pegasys.pantheon.ethereum.core.Block; import tech.pegasys.pantheon.ethereum.core.Hash; import tech.pegasys.pantheon.ethereum.rlp.BytesValueRLPOutput; @@ -51,7 +53,8 @@ public class RoundChangeCertificateTest { @Test public void rlpRoundTripWithPreparedCertificate() { - final Block block = TestHelpers.createProposalBlock(); + final Block block = + TestHelpers.createProposalBlock(singletonList(AddressHelpers.ofValue(1)), 0); final ProposalPayload proposalPayload = new ProposalPayload(ROUND_IDENTIFIER, block); final Signature signature = Signature.create(BigInteger.ONE, BigInteger.TEN, (byte) 0); SignedData signedProposal = SignedData.from(proposalPayload, signature); diff --git a/consensus/ibft/src/test/java/tech/pegasys/pantheon/consensus/ibft/ibftmessagedata/RoundChangePayloadTest.java b/consensus/ibft/src/test/java/tech/pegasys/pantheon/consensus/ibft/ibftmessagedata/RoundChangePayloadTest.java index 334b22da59..57447ca1f6 100644 --- a/consensus/ibft/src/test/java/tech/pegasys/pantheon/consensus/ibft/ibftmessagedata/RoundChangePayloadTest.java +++ b/consensus/ibft/src/test/java/tech/pegasys/pantheon/consensus/ibft/ibftmessagedata/RoundChangePayloadTest.java @@ -12,6 +12,7 @@ */ package tech.pegasys.pantheon.consensus.ibft.ibftmessagedata; +import static java.util.Collections.singletonList; import static java.util.Optional.empty; import static org.assertj.core.api.Assertions.assertThat; @@ -19,6 +20,7 @@ import tech.pegasys.pantheon.consensus.ibft.ConsensusRoundIdentifier; import tech.pegasys.pantheon.consensus.ibft.TestHelpers; import tech.pegasys.pantheon.consensus.ibft.ibftmessage.IbftV2; import tech.pegasys.pantheon.crypto.SECP256K1.Signature; +import tech.pegasys.pantheon.ethereum.core.AddressHelpers; import tech.pegasys.pantheon.ethereum.core.Block; import tech.pegasys.pantheon.ethereum.core.Hash; import tech.pegasys.pantheon.ethereum.rlp.BytesValueRLPOutput; @@ -33,6 +35,7 @@ import org.assertj.core.util.Lists; import org.junit.Test; public class RoundChangePayloadTest { + private static final ConsensusRoundIdentifier ROUND_IDENTIFIER = new ConsensusRoundIdentifier(0x1234567890ABCDEFL, 0xFEDCBA98); private static final Signature SIGNATURE = @@ -95,7 +98,8 @@ public class RoundChangePayloadTest { } private SignedData signedProposal() { - final Block block = TestHelpers.createProposalBlock(); + final Block block = + TestHelpers.createProposalBlock(singletonList(AddressHelpers.ofValue(1)), 0); final ProposalPayload proposalPayload = new ProposalPayload(ROUND_IDENTIFIER, block); final Signature signature = Signature.create(BigInteger.ONE, BigInteger.TEN, (byte) 0); return SignedData.from(proposalPayload, signature); diff --git a/consensus/ibft/src/test/java/tech/pegasys/pantheon/consensus/ibft/statemachine/RoundChangeManagerTest.java b/consensus/ibft/src/test/java/tech/pegasys/pantheon/consensus/ibft/statemachine/RoundChangeManagerTest.java index c80f3d04ce..8633004524 100644 --- a/consensus/ibft/src/test/java/tech/pegasys/pantheon/consensus/ibft/statemachine/RoundChangeManagerTest.java +++ b/consensus/ibft/src/test/java/tech/pegasys/pantheon/consensus/ibft/statemachine/RoundChangeManagerTest.java @@ -13,11 +13,17 @@ package tech.pegasys.pantheon.consensus.ibft.statemachine; import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.ArgumentMatchers.any; import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; import tech.pegasys.pantheon.consensus.ibft.ConsensusRoundIdentifier; import tech.pegasys.pantheon.consensus.ibft.IbftContext; +import tech.pegasys.pantheon.consensus.ibft.TestHelpers; import tech.pegasys.pantheon.consensus.ibft.ibftmessagedata.MessageFactory; +import tech.pegasys.pantheon.consensus.ibft.ibftmessagedata.PreparePayload; +import tech.pegasys.pantheon.consensus.ibft.ibftmessagedata.PreparedCertificate; +import tech.pegasys.pantheon.consensus.ibft.ibftmessagedata.ProposalPayload; import tech.pegasys.pantheon.consensus.ibft.ibftmessagedata.RoundChangePayload; import tech.pegasys.pantheon.consensus.ibft.ibftmessagedata.SignedData; import tech.pegasys.pantheon.consensus.ibft.validation.MessageValidator; @@ -25,15 +31,19 @@ import tech.pegasys.pantheon.crypto.SECP256K1.KeyPair; import tech.pegasys.pantheon.ethereum.ProtocolContext; import tech.pegasys.pantheon.ethereum.chain.MutableBlockchain; import tech.pegasys.pantheon.ethereum.core.Address; +import tech.pegasys.pantheon.ethereum.core.Block; import tech.pegasys.pantheon.ethereum.core.BlockHeader; import tech.pegasys.pantheon.ethereum.core.Util; import tech.pegasys.pantheon.ethereum.db.WorldStateArchive; import tech.pegasys.pantheon.ethereum.mainnet.BlockHeaderValidator; +import java.util.Collections; import java.util.List; import java.util.Map; import java.util.Optional; +import java.util.stream.Collectors; +import com.google.common.base.Preconditions; import com.google.common.collect.Lists; import com.google.common.collect.Maps; import org.junit.Before; @@ -51,10 +61,10 @@ public class RoundChangeManagerTest { private final ConsensusRoundIdentifier ri1 = new ConsensusRoundIdentifier(2, 1); private final ConsensusRoundIdentifier ri2 = new ConsensusRoundIdentifier(2, 2); private final ConsensusRoundIdentifier ri3 = new ConsensusRoundIdentifier(2, 3); + private final List
validators = Lists.newArrayList(); @Before public void setup() { - List
validators = Lists.newArrayList(); validators.add(Util.publicKeyToAddress(proposerKey.getPublicKey())); validators.add(Util.publicKeyToAddress(validator1Key.getPublicKey())); @@ -67,6 +77,7 @@ public class RoundChangeManagerTest { @SuppressWarnings("unchecked") BlockHeaderValidator headerValidator = (BlockHeaderValidator) mock(BlockHeaderValidator.class); + when(headerValidator.validateHeader(any(), any(), any(), any())).thenReturn(true); BlockHeader parentHeader = mock(BlockHeader.class); Map messageValidators = Maps.newHashMap(); @@ -110,6 +121,35 @@ public class RoundChangeManagerTest { return messageFactory.createSignedRoundChangePayload(round, Optional.empty()); } + private SignedData makeRoundChangeMessageWithPreparedCert( + final KeyPair key, + final ConsensusRoundIdentifier round, + final List prepareProviders) { + Preconditions.checkArgument(!prepareProviders.contains(key)); + + final MessageFactory messageFactory = new MessageFactory(key); + + final ConsensusRoundIdentifier proposalRound = TestHelpers.createFrom(round, 0, -1); + final Block block = TestHelpers.createProposalBlock(validators, proposalRound.getRoundNumber()); + // Proposal must come from an earlier round. + final SignedData proposal = + messageFactory.createSignedProposalPayload(proposalRound, block); + + final List> preparePayloads = + prepareProviders + .stream() + .map( + k -> { + final MessageFactory prepareFactory = new MessageFactory(k); + return prepareFactory.createSignedPreparePayload(proposalRound, block.getHash()); + }) + .collect(Collectors.toList()); + + final PreparedCertificate cert = new PreparedCertificate(proposal, preparePayloads); + + return messageFactory.createSignedRoundChangePayload(round, Optional.of(cert)); + } + @Test public void rejectsInvalidRoundChangeMessage() { SignedData roundChangeData = makeRoundChangeMessage(nonValidatorKey, ri1); @@ -193,4 +233,22 @@ public class RoundChangeManagerTest { .isEqualTo(Optional.empty()); assertThat(manager.roundChangeCache.get(ri2).receivedMessages.size()).isEqualTo(2); } + + @Test + public void roundChangeMessagesWithPreparedCertificateMustHaveSufficientPrepareMessages() { + // Specifically, prepareMessage count is ONE LESS than the calculated quorum size (as the + // proposal acts as the extra msg). + // There are 3 validators, therefore, should only need 2 prepare message to be acceptable. + + // These tests are run at ri2, such that validators can be found for past round at ri1. + SignedData roundChangeData = + makeRoundChangeMessageWithPreparedCert(proposerKey, ri2, Collections.emptyList()); + assertThat(manager.appendRoundChangeMessage(roundChangeData)).isEmpty(); + assertThat(manager.roundChangeCache.get(ri2)).isNull(); + + roundChangeData = + makeRoundChangeMessageWithPreparedCert(proposerKey, ri2, Lists.newArrayList(validator1Key)); + assertThat(manager.appendRoundChangeMessage(roundChangeData)).isEmpty(); + assertThat(manager.roundChangeCache.get(ri2).receivedMessages.size()).isEqualTo(1); + } }