Bft messages have duplicate prepares in roundChange message

CHANGELOG.md

@@ -1,5 +1,10 @@
 # Changelog
 
+## 21.7.0-RC2
+
+### Bug Fixes
+- Ibft2 could create invalid RoundChange messages in some circumstances containing duplicate prepares [\#2449](https://github.com/hyperledger/besu/pull/2449)
+
 ## 21.7.0-RC1
 
 ### Additions and Improvements

consensus/common/src/main/java/org/hyperledger/besu/consensus/common/bft/messagewrappers/BftMessage.java

@@ -25,6 +25,7 @@
 import org.hyperledger.besu.ethereum.rlp.BytesValueRLPOutput;
 import org.hyperledger.besu.ethereum.rlp.RLPInput;
 
+import java.util.Objects;
 import java.util.StringJoiner;
 import java.util.function.Function;
 
@@ -66,13 +67,6 @@ protected P getPayload() {
     return payload.getPayload();
   }
 
-  @Override
-  public String toString() {
-    return new StringJoiner(", ", BftMessage.class.getSimpleName() + "[", "]")
-        .add("payload=" + payload)
-        .toString();
-  }
-
   protected static <T extends Payload> SignedData<T> readPayload(
       final RLPInput rlpInput, final Function<RLPInput, T> decoder) {
     rlpInput.enterList();
@@ -83,4 +77,28 @@ protected static <T extends Payload> SignedData<T> readPayload(
 
     return SignedData.create(unsignedMessageData, signature);
   }
+
+  @Override
+  public String toString() {
+    return new StringJoiner(", ", BftMessage.class.getSimpleName() + "[", "]")
+        .add("payload=" + payload)
+        .toString();
+  }
+
+  @Override
+  public boolean equals(final Object o) {
+    if (this == o) {
+      return true;
+    }
+    if (o == null || getClass() != o.getClass()) {
+      return false;
+    }
+    final BftMessage<?> that = (BftMessage<?>) o;
+    return Objects.equals(payload, that.payload);
+  }
+
+  @Override
+  public int hashCode() {
+    return Objects.hash(payload);
+  }
 }

consensus/ibft/src/integration-test/java/org/hyperledger/besu/consensus/ibft/tests/round/IbftRoundIntegrationTest.java

@@ -65,6 +65,7 @@
 public class IbftRoundIntegrationTest {
 
   private final MessageFactory peerMessageFactory = new MessageFactory(NodeKeyUtils.generate());
+  private final MessageFactory peerMessageFactory2 = new MessageFactory(NodeKeyUtils.generate());
   private final ConsensusRoundIdentifier roundIdentifier = new ConsensusRoundIdentifier(1, 0);
   private final Subscribers<MinedBlockObserver> subscribers = Subscribers.create();
   private ProtocolContext protocolContext;
@@ -181,7 +182,7 @@ public void failuresToSignStillAllowBlockToBeImported() {
     verifyNoInteractions(multicaster);
 
     round.handleCommitMessage(
-        peerMessageFactory.createCommit(roundIdentifier, Hash.EMPTY, remoteCommitSeal));
+        peerMessageFactory2.createCommit(roundIdentifier, Hash.EMPTY, remoteCommitSeal));
     assertThat(roundState.isCommitted()).isTrue();
     verifyNoInteractions(multicaster);
 

consensus/ibft/src/test/java/org/hyperledger/besu/consensus/ibft/statemachine/RoundStateTest.java

@@ -21,10 +21,13 @@
 import static org.mockito.Mockito.when;
 
 import org.hyperledger.besu.consensus.common.bft.ConsensusRoundIdentifier;
+import org.hyperledger.besu.consensus.common.bft.messagewrappers.BftMessage;
+import org.hyperledger.besu.consensus.common.bft.payload.SignedData;
 import org.hyperledger.besu.consensus.ibft.messagewrappers.Commit;
 import org.hyperledger.besu.consensus.ibft.messagewrappers.Prepare;
 import org.hyperledger.besu.consensus.ibft.messagewrappers.Proposal;
 import org.hyperledger.besu.consensus.ibft.payload.MessageFactory;
+import org.hyperledger.besu.consensus.ibft.payload.PreparePayload;
 import org.hyperledger.besu.consensus.ibft.validation.MessageValidator;
 import org.hyperledger.besu.crypto.NodeKey;
 import org.hyperledger.besu.crypto.NodeKeyUtils;
@@ -38,6 +41,7 @@
 import java.math.BigInteger;
 import java.util.List;
 import java.util.Optional;
+import java.util.stream.Collectors;
 
 import com.google.common.collect.Lists;
 import org.junit.Before;
@@ -265,4 +269,44 @@ public void commitSealsAreExtractedFromReceivedMessages() {
     assertThat(roundState.getCommitSeals())
         .containsOnly(firstCommit.getCommitSeal(), secondCommit.getCommitSeal());
   }
+
+  @Test
+  public void duplicatePreparesAreNotIncludedInRoundChangeMessage() {
+    final RoundState roundState = new RoundState(roundIdentifier, 2, messageValidator);
+
+    final Prepare firstPrepare =
+        validatorMessageFactories.get(1).createPrepare(roundIdentifier, block.getHash());
+
+    final Prepare secondPrepare =
+        validatorMessageFactories.get(2).createPrepare(roundIdentifier, block.getHash());
+
+    final Prepare duplicatePrepare =
+        validatorMessageFactories.get(2).createPrepare(roundIdentifier, block.getHash());
+
+    final Proposal proposal =
+        validatorMessageFactories.get(0).createProposal(roundIdentifier, block, Optional.empty());
+
+    when(messageValidator.validateProposal(any())).thenReturn(true);
+    when(messageValidator.validatePrepare(any())).thenReturn(true);
+
+    roundState.setProposedBlock(proposal);
+    roundState.addPrepareMessage(firstPrepare);
+    roundState.addPrepareMessage(secondPrepare);
+    roundState.addPrepareMessage(duplicatePrepare);
+
+    assertThat(roundState.isPrepared()).isTrue();
+    assertThat(roundState.isCommitted()).isFalse();
+
+    final Optional<PreparedRoundArtifacts> preparedRoundArtifacts =
+        roundState.constructPreparedRoundArtifacts();
+    assertThat(preparedRoundArtifacts).isNotEmpty();
+    assertThat(preparedRoundArtifacts.get().getBlock()).isEqualTo(block);
+
+    final List<SignedData<PreparePayload>> expectedPrepares =
+        List.of(firstPrepare, secondPrepare).stream()
+            .map(BftMessage::getSignedPayload)
+            .collect(Collectors.toList());
+    assertThat(preparedRoundArtifacts.get().getPreparedCertificate().getPreparePayloads())
+        .isEqualTo(expectedPrepares);
+  }
 }

consensus/qbft/src/integration-test/java/org/hyperledger/besu/consensus/qbft/test/round/QbftRoundIntegrationTest.java

@@ -65,6 +65,7 @@
 public class QbftRoundIntegrationTest {
 
   private final MessageFactory peerMessageFactory = new MessageFactory(NodeKeyUtils.generate());
+  private final MessageFactory peerMessageFactory2 = new MessageFactory(NodeKeyUtils.generate());
   private final ConsensusRoundIdentifier roundIdentifier = new ConsensusRoundIdentifier(1, 0);
   private final Subscribers<MinedBlockObserver> subscribers = Subscribers.create();
   private final BftExtraDataCodec bftExtraDataCodec = new QbftExtraDataCodec();
@@ -184,7 +185,7 @@ public void failuresToSignStillAllowBlockToBeImported() {
     verifyNoInteractions(multicaster);
 
     round.handleCommitMessage(
-        peerMessageFactory.createCommit(roundIdentifier, Hash.EMPTY, remoteCommitSeal));
+        peerMessageFactory2.createCommit(roundIdentifier, Hash.EMPTY, remoteCommitSeal));
     assertThat(roundState.isCommitted()).isTrue();
     verifyNoInteractions(multicaster);
 

consensus/qbft/src/test/java/org/hyperledger/besu/consensus/qbft/statemachine/QbftRoundTest.java

@@ -77,8 +77,10 @@
 public class QbftRoundTest {
 
   private final NodeKey nodeKey = NodeKeyUtils.generate();
+  private final NodeKey nodeKey2 = NodeKeyUtils.generate();
   private final ConsensusRoundIdentifier roundIdentifier = new ConsensusRoundIdentifier(1, 0);
   private final MessageFactory messageFactory = new MessageFactory(nodeKey);
+  private final MessageFactory messageFactory2 = new MessageFactory(nodeKey2);
   private final Subscribers<MinedBlockObserver> subscribers = Subscribers.create();
   private final BftExtraDataCodec bftExtraDataCodec = new QbftExtraDataCodec();
   private ProtocolContext protocolContext;
@@ -243,7 +245,7 @@ public void localNodeProposesToNetworkOfTwoValidatorsImportsOnReceptionOfCommitF
     verify(blockImporter, never()).importBlock(any(), any(), any());
 
     round.handlePrepareMessage(
-        messageFactory.createPrepare(roundIdentifier, proposedBlock.getHash()));
+        messageFactory2.createPrepare(roundIdentifier, proposedBlock.getHash()));
 
     verify(transmitter, times(1))
         .multicastCommit(roundIdentifier, proposedBlock.getHash(), localCommitSeal);
@@ -321,7 +323,7 @@ public void aProposalMessageWithTheSameBlockIsSentUponReceptionOfARoundChangeWit
     // Inject a single Prepare message, and confirm the roundState has gone to Prepared (which
     // indicates the block has entered the roundState (note: all msgs are deemed valid due to mocks)
     round.handlePrepareMessage(
-        messageFactory.createPrepare(roundIdentifier, proposedBlock.getHash()));
+        messageFactory2.createPrepare(roundIdentifier, proposedBlock.getHash()));
     assertThat(roundState.isPrepared()).isTrue();
   }
 
@@ -360,7 +362,7 @@ public void creatingNewBlockFromEmptyPreparedCertificateUpdatesInternalState() {
     // Inject a single Prepare message, and confirm the roundState has gone to Prepared (which
     // indicates the block has entered the roundState (note: all msgs are deemed valid due to mocks)
     round.handlePrepareMessage(
-        messageFactory.createPrepare(roundIdentifier, proposedBlock.getHash()));
+        messageFactory2.createPrepare(roundIdentifier, proposedBlock.getHash()));
     assertThat(roundState.isPrepared()).isTrue();
   }
 

consensus/qbft/src/test/java/org/hyperledger/besu/consensus/qbft/statemachine/RoundStateTest.java

@@ -21,10 +21,13 @@
 import static org.mockito.Mockito.when;
 
 import org.hyperledger.besu.consensus.common.bft.ConsensusRoundIdentifier;
+import org.hyperledger.besu.consensus.common.bft.messagewrappers.BftMessage;
+import org.hyperledger.besu.consensus.common.bft.payload.SignedData;
 import org.hyperledger.besu.consensus.qbft.messagewrappers.Commit;
 import org.hyperledger.besu.consensus.qbft.messagewrappers.Prepare;
 import org.hyperledger.besu.consensus.qbft.messagewrappers.Proposal;
 import org.hyperledger.besu.consensus.qbft.payload.MessageFactory;
+import org.hyperledger.besu.consensus.qbft.payload.PreparePayload;
 import org.hyperledger.besu.consensus.qbft.validation.MessageValidator;
 import org.hyperledger.besu.crypto.NodeKey;
 import org.hyperledger.besu.crypto.NodeKeyUtils;
@@ -38,6 +41,8 @@
 import java.math.BigInteger;
 import java.util.Collections;
 import java.util.List;
+import java.util.Optional;
+import java.util.stream.Collectors;
 
 import com.google.common.base.Supplier;
 import com.google.common.base.Suppliers;
@@ -148,7 +153,7 @@ public void prepareMessagesCanBeReceivedPriorToProposal() {
         validatorMessageFactories.get(2).createPrepare(roundIdentifier, block.getHash());
 
     final Prepare thirdPrepare =
-        validatorMessageFactories.get(2).createPrepare(roundIdentifier, block.getHash());
+        validatorMessageFactories.get(0).createPrepare(roundIdentifier, block.getHash());
 
     roundState.addPrepareMessage(firstPrepare);
     assertThat(roundState.isPrepared()).isFalse();
@@ -209,7 +214,7 @@ public void invalidPriorPrepareMessagesAreDiscardedUponSubsequentProposal() {
   }
 
   @Test
-  public void prepareMessageIsValidatedAgainstExitingProposal() {
+  public void prepareMessageIsValidatedAgainstExistingProposal() {
     final RoundState roundState = new RoundState(roundIdentifier, 2, messageValidator);
 
     final Prepare firstPrepare =
@@ -219,7 +224,7 @@ public void prepareMessageIsValidatedAgainstExitingProposal() {
         validatorMessageFactories.get(2).createPrepare(roundIdentifier, block.getHash());
 
     final Prepare thirdPrepare =
-        validatorMessageFactories.get(2).createPrepare(roundIdentifier, block.getHash());
+        validatorMessageFactories.get(0).createPrepare(roundIdentifier, block.getHash());
 
     final Proposal proposal =
         validatorMessageFactories
@@ -287,4 +292,47 @@ public void commitSealsAreExtractedFromReceivedMessages() {
     assertThat(roundState.getCommitSeals())
         .containsOnly(firstCommit.getCommitSeal(), secondCommit.getCommitSeal());
   }
+
+  @Test
+  public void duplicatePreparesAreNotIncludedInRoundChangeMessage() {
+    final RoundState roundState = new RoundState(roundIdentifier, 2, messageValidator);
+
+    final Prepare firstPrepare =
+        validatorMessageFactories.get(1).createPrepare(roundIdentifier, block.getHash());
+
+    final Prepare secondPrepare =
+        validatorMessageFactories.get(2).createPrepare(roundIdentifier, block.getHash());
+
+    final Prepare duplicatePrepare =
+        validatorMessageFactories.get(2).createPrepare(roundIdentifier, block.getHash());
+
+    final Proposal proposal =
+        validatorMessageFactories
+            .get(0)
+            .createProposal(
+                roundIdentifier, block, Collections.emptyList(), Collections.emptyList());
+
+    when(messageValidator.validateProposal(any())).thenReturn(true);
+    when(messageValidator.validatePrepare(any())).thenReturn(true);
+
+    roundState.setProposedBlock(proposal);
+    roundState.addPrepareMessage(firstPrepare);
+    roundState.addPrepareMessage(secondPrepare);
+    roundState.addPrepareMessage(duplicatePrepare);
+
+    assertThat(roundState.isPrepared()).isTrue();
+    assertThat(roundState.isCommitted()).isFalse();
+
+    final Optional<PreparedCertificate> preparedCertificate =
+        roundState.constructPreparedCertificate();
+    assertThat(preparedCertificate).isNotEmpty();
+    assertThat(preparedCertificate.get().getRound()).isEqualTo(roundIdentifier.getRoundNumber());
+    assertThat(preparedCertificate.get().getBlock()).isEqualTo(block);
+
+    final List<SignedData<PreparePayload>> expectedPrepares =
+        List.of(firstPrepare, secondPrepare).stream()
+            .map(BftMessage::getSignedPayload)
+            .collect(Collectors.toList());
+    assertThat(preparedCertificate.get().getPrepares()).isEqualTo(expectedPrepares);
+  }
 }