From bcbbde61ea87bb8d476d9ebd57094b1da557cfa2 Mon Sep 17 00:00:00 2001 From: tmohay <37158202+rain-on@users.noreply.github.com> Date: Thu, 17 Jan 2019 15:18:18 +1100 Subject: [PATCH] Prevent the block being mined twice (#576) Testing indicated that if quorum-1 commit messages is received prior to the proposal, the proposed block will be imported twice. Theoretically this is handled safely by the BlockImporter, however is clearly an error in the IbftRound code. This commit updates IbftRound such that the import can only occur once. Signed-off-by: Adrian Sutton --- .../statemachine/IbftBlockHeightManager.java | 2 +- .../ibft/statemachine/IbftRound.java | 30 +++++++++-- .../ibft/statemachine/IbftRoundTest.java | 53 ++++++++++++++++++- 3 files changed, 78 insertions(+), 7 deletions(-) diff --git a/consensus/ibft/src/main/java/tech/pegasys/pantheon/consensus/ibft/statemachine/IbftBlockHeightManager.java b/consensus/ibft/src/main/java/tech/pegasys/pantheon/consensus/ibft/statemachine/IbftBlockHeightManager.java index b98d464a46..733eb2419c 100644 --- a/consensus/ibft/src/main/java/tech/pegasys/pantheon/consensus/ibft/statemachine/IbftBlockHeightManager.java +++ b/consensus/ibft/src/main/java/tech/pegasys/pantheon/consensus/ibft/statemachine/IbftBlockHeightManager.java @@ -236,7 +236,7 @@ public class IbftBlockHeightManager { if (messageAge == FUTURE_ROUND) { startNewRound(payload.getRoundIdentifier().getRoundNumber()); } - currentRound.handleProposalMessage(payload.getProposalPayload()); + currentRound.handleProposalFromNewRound(signedPayload); } } diff --git a/consensus/ibft/src/main/java/tech/pegasys/pantheon/consensus/ibft/statemachine/IbftRound.java b/consensus/ibft/src/main/java/tech/pegasys/pantheon/consensus/ibft/statemachine/IbftRound.java index 96f5d81ca5..b9d7cc4d85 100644 --- a/consensus/ibft/src/main/java/tech/pegasys/pantheon/consensus/ibft/statemachine/IbftRound.java +++ b/consensus/ibft/src/main/java/tech/pegasys/pantheon/consensus/ibft/statemachine/IbftRound.java @@ -23,6 +23,7 @@ import tech.pegasys.pantheon.consensus.ibft.blockcreation.IbftBlockCreator; import tech.pegasys.pantheon.consensus.ibft.network.IbftMessageTransmitter; import tech.pegasys.pantheon.consensus.ibft.payload.CommitPayload; import tech.pegasys.pantheon.consensus.ibft.payload.MessageFactory; +import tech.pegasys.pantheon.consensus.ibft.payload.NewRoundPayload; import tech.pegasys.pantheon.consensus.ibft.payload.PreparePayload; import tech.pegasys.pantheon.consensus.ibft.payload.PreparedCertificate; import tech.pegasys.pantheon.consensus.ibft.payload.ProposalPayload; @@ -136,8 +137,26 @@ public class IbftRound { public void handleProposalMessage(final SignedData msg) { LOG.info("Handling a Proposal message."); + + if (getRoundIdentifier().getRoundNumber() != 0) { + LOG.info("Illegally received a Proposal message when not in Round 0."); + return; + } + actionReceivedProposal(msg); + } + + public void handleProposalFromNewRound(final SignedData msg) { + LOG.info("Handling a New Round Proposal."); + + if (getRoundIdentifier().getRoundNumber() == 0) { + LOG.info("Illegally received a NewRound message when in Round 0."); + return; + } + actionReceivedProposal(msg.getPayload().getProposalPayload()); + } + + private void actionReceivedProposal(final SignedData msg) { final Block block = msg.getPayload().getBlock(); - final boolean wasCommitted = roundState.isCommitted(); if (updateStateWithProposedBlock(msg)) { LOG.info("Sending prepare message."); @@ -147,10 +166,6 @@ public class IbftRound { roundState.getRoundIdentifier(), block.getHash()); peerIsPrepared(localPrepareMessage); } - - if (wasCommitted != roundState.isCommitted()) { - importBlockToChain(); - } } public void handlePrepareMessage(final SignedData msg) { @@ -169,6 +184,7 @@ public class IbftRound { private boolean updateStateWithProposedBlock(final SignedData msg) { final boolean wasPrepared = roundState.isPrepared(); + final boolean wasCommitted = roundState.isCommitted(); final boolean blockAccepted = roundState.setProposedBlock(msg); if (blockAccepted) { // There are times handling a proposed block is enough to enter prepared. @@ -177,6 +193,10 @@ public class IbftRound { final Block block = roundState.getProposedBlock().get(); transmitter.multicastCommit(getRoundIdentifier(), block.getHash(), createCommitSeal(block)); } + if (wasCommitted != roundState.isCommitted()) { + importBlockToChain(); + } + final SignedData localCommitMessage = messageFactory.createSignedCommitPayload( roundState.getRoundIdentifier(), diff --git a/consensus/ibft/src/test/java/tech/pegasys/pantheon/consensus/ibft/statemachine/IbftRoundTest.java b/consensus/ibft/src/test/java/tech/pegasys/pantheon/consensus/ibft/statemachine/IbftRoundTest.java index 1b21e5f4be..5b523674ec 100644 --- a/consensus/ibft/src/test/java/tech/pegasys/pantheon/consensus/ibft/statemachine/IbftRoundTest.java +++ b/consensus/ibft/src/test/java/tech/pegasys/pantheon/consensus/ibft/statemachine/IbftRoundTest.java @@ -67,7 +67,7 @@ import org.mockito.junit.MockitoJUnitRunner; public class IbftRoundTest { private final KeyPair localNodeKeys = KeyPair.generate(); - private final ConsensusRoundIdentifier roundIdentifier = new ConsensusRoundIdentifier(1, 1); + private final ConsensusRoundIdentifier roundIdentifier = new ConsensusRoundIdentifier(1, 0); private final MessageFactory messageFactory = new MessageFactory(localNodeKeys); private final Subscribers subscribers = new Subscribers<>(); private ProtocolContext protocolContext; @@ -375,4 +375,55 @@ public class IbftRoundTest { round.createAndSendProposalMessage(15); verify(minedBlockObserver).blockMined(any()); } + + @Test + public void blockIsOnlyImportedOnceWhenCommitsAreReceivedBeforeProposal() { + final ConsensusRoundIdentifier roundIdentifier = new ConsensusRoundIdentifier(1, 0); + final int QUORUM_SIZE = 2; + final RoundState roundState = new RoundState(roundIdentifier, QUORUM_SIZE, messageValidator); + final IbftRound round = + new IbftRound( + roundState, + blockCreator, + protocolContext, + blockImporter, + subscribers, + localNodeKeys, + messageFactory, + transmitter); + + round.handleCommitMessage( + messageFactory.createSignedCommitPayload( + roundIdentifier, proposedBlock.getHash(), remoteCommitSeal)); + + round.handleProposalMessage( + messageFactory.createSignedProposalPayload(roundIdentifier, proposedBlock)); + + verify(blockImporter, times(1)).importBlock(any(), any(), any()); + } + + @Test + public void blockIsImportedOnlyOnceIfQuorumCommitsAreReceivedPriorToProposal() { + final int QUORUM_SIZE = 1; + final RoundState roundState = new RoundState(roundIdentifier, QUORUM_SIZE, messageValidator); + final IbftRound round = + new IbftRound( + roundState, + blockCreator, + protocolContext, + blockImporter, + subscribers, + localNodeKeys, + messageFactory, + transmitter); + + round.handleCommitMessage( + messageFactory.createSignedCommitPayload( + roundIdentifier, proposedBlock.getHash(), remoteCommitSeal)); + + round.handleProposalMessage( + messageFactory.createSignedProposalPayload(roundIdentifier, proposedBlock)); + + verify(blockImporter, times(1)).importBlock(any(), any(), any()); + } }