QBFT: Fix validation of proposal based on older round's prepared block (#7875)

* QBFT: Fix validation of proposal based on older round's prepared block

* Add round numbers to new blocks and prepared based proposals

* QBFT fix around proposal validation by proposer and testcase updates

- Check the result of proposer's self proposal validation before sending prepare message
- Update roundchange testcases to include round numbers in blocks

* Update changelog with qbft fix

Signed-off-by: Bhanu Pulluri <bhanu.pulluri@kaleido.io>

---------

Signed-off-by: Bhanu Pulluri <bhanu.pulluri@kaleido.io>
Co-authored-by: Bhanu Pulluri <bhanu.pulluri@kaleido.io>
Co-authored-by: Sally MacFarlane <macfarla.github@gmail.com>

CHANGELOG.md

@@ -30,6 +30,7 @@
 ### Bug fixes
 - Fix registering new metric categories from plugins [#7825](https://github.com/hyperledger/besu/pull/7825)
 - Fix CVE-2024-47535 [7878](https://github.com/hyperledger/besu/pull/7878)
+- Fix QBFT prepared block based proposal validation [#7875](https://github.com/hyperledger/besu/pull/7875)
 
 ## 24.10.0
 

consensus/qbft/src/integration-test/java/org/hyperledger/besu/consensus/qbft/support/TestContext.java

@@ -97,13 +97,37 @@ public class TestContext {
   }
 
   public Block createBlockForProposalFromChainHead(final long timestamp) {
-    return createBlockForProposalFromChainHead(timestamp, finalState.getLocalAddress());
+    return createBlockForProposalFromChainHead(timestamp, finalState.getLocalAddress(), 0);
+  }
+
+  public Block createBlockForProposalFromChainHead(final long timestamp, final int roundNumber) {
+    return createBlockForProposalFromChainHead(
+        timestamp, finalState.getLocalAddress(), roundNumber);
+  }
+
+  public Block createBlockForProposalFromChainHead(final long timestamp, final Address proposer) {
+    // this implies that EVERY block will have this node as the proposer :/
+    return createBlockForProposal(blockchain.getChainHeadHeader(), timestamp, proposer, 0);
+  }
+
+  public Block createBlockForProposalFromChainHead(
+      final long timestamp, final Address proposer, final int roundNumber) {
+    // this implies that EVERY block will have this node as the proposer :/
+    return createBlockForProposal(
+        blockchain.getChainHeadHeader(), timestamp, proposer, roundNumber);
   }
 
   public Block createBlockForProposal(
-      final BlockHeader parent, final long timestamp, final Address proposer) {
+      final BlockHeader parent,
+      final long timestamp,
+      final Address proposer,
+      final int roundNumber) {
     final Block block =
-        finalState.getBlockCreatorFactory().create(0).createBlock(timestamp, parent).getBlock();
+        finalState
+            .getBlockCreatorFactory()
+            .create(roundNumber)
+            .createBlock(timestamp, parent)
+            .getBlock();
 
     final BlockHeaderBuilder headerBuilder = BlockHeaderBuilder.fromHeader(block.getHeader());
     headerBuilder
@@ -114,9 +138,9 @@ public class TestContext {
     return new Block(newHeader, block.getBody());
   }
 
-  public Block createBlockForProposalFromChainHead(final long timestamp, final Address proposer) {
-    // this implies that EVERY block will have this node as the proposer :/
-    return createBlockForProposal(blockchain.getChainHeadHeader(), timestamp, proposer);
+  public Block createBlockForProposal(
+      final BlockHeader parent, final long timestamp, final Address proposer) {
+    return createBlockForProposal(parent, timestamp, proposer, 0);
   }
 
   public RoundSpecificPeers roundSpecificPeers(final ConsensusRoundIdentifier roundId) {

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

@@ -144,7 +144,8 @@ public class RoundChangeTest {
   public void whenSufficientRoundChangeMessagesAreReceivedForNewRoundLocalNodeCreatesProposalMsg() {
     // Note: Round-4 is the next round for which the local node is Proposer
     final ConsensusRoundIdentifier targetRound = new ConsensusRoundIdentifier(1, 4);
-    final Block locallyProposedBlock = context.createBlockForProposalFromChainHead(blockTimeStamp);
+    final Block locallyProposedBlock =
+        context.createBlockForProposalFromChainHead(blockTimeStamp, 4);
 
     final RoundChange rc1 = peers.getNonProposing(0).injectRoundChange(targetRound, empty());
     final RoundChange rc2 = peers.getNonProposing(1).injectRoundChange(targetRound, empty());
@@ -177,14 +178,14 @@ public class RoundChangeTest {
             context,
             new ConsensusRoundIdentifier(1, 1),
             context.createBlockForProposalFromChainHead(
-                ARBITRARY_BLOCKTIME / 2, peers.getProposer().getNodeAddress()));
+                ARBITRARY_BLOCKTIME / 2, peers.getProposer().getNodeAddress(), 1));
 
     final PreparedCertificate bestPrepCert =
         createValidPreparedCertificate(
             context,
             new ConsensusRoundIdentifier(1, 2),
             context.createBlockForProposalFromChainHead(
-                ARBITRARY_BLOCKTIME, peers.getProposer().getNodeAddress()));
+                ARBITRARY_BLOCKTIME, peers.getProposer().getNodeAddress(), 2));
 
     final ConsensusRoundIdentifier targetRound = new ConsensusRoundIdentifier(1, 4);
 
@@ -206,7 +207,7 @@ public class RoundChangeTest {
     // round number.
     final Block expectedBlockToPropose =
         context.createBlockForProposalFromChainHead(
-            ARBITRARY_BLOCKTIME, peers.getProposer().getNodeAddress());
+            ARBITRARY_BLOCKTIME, peers.getProposer().getNodeAddress(), 4);
 
     final Proposal expectedProposal =
         localNodeMessageFactory.createProposal(
@@ -234,7 +235,8 @@ public class RoundChangeTest {
     final ConsensusRoundIdentifier priorRound = new ConsensusRoundIdentifier(1, 4);
     peers.roundChange(priorRound);
 
-    final Block locallyProposedBlock = context.createBlockForProposalFromChainHead(blockTimeStamp);
+    final Block locallyProposedBlock =
+        context.createBlockForProposalFromChainHead(blockTimeStamp, 9);
 
     final Proposal expectedProposal =
         localNodeMessageFactory.createProposal(
@@ -271,7 +273,7 @@ public class RoundChangeTest {
             context,
             new ConsensusRoundIdentifier(1, 2),
             context.createBlockForProposalFromChainHead(
-                ARBITRARY_BLOCKTIME, peers.getProposer().getNodeAddress()));
+                ARBITRARY_BLOCKTIME, peers.getProposer().getNodeAddress(), 2));
 
     final List<SignedData<RoundChangePayload>> roundChangeMessages = Lists.newArrayList();
     // Create a roundChange containing a PreparedCertificate
@@ -288,7 +290,7 @@ public class RoundChangeTest {
 
     final Block expectedBlockToPropose =
         context.createBlockForProposalFromChainHead(
-            ARBITRARY_BLOCKTIME, peers.getProposer().getNodeAddress());
+            ARBITRARY_BLOCKTIME, peers.getProposer().getNodeAddress(), 4);
 
     final Proposal expectedProposal =
         localNodeMessageFactory.createProposal(

consensus/qbft/src/main/java/org/hyperledger/besu/consensus/qbft/statemachine/QbftRound.java

@@ -160,9 +160,18 @@ public class QbftRound {
     } else {
       LOG.debug(
           "Sending proposal from PreparedCertificate. round={}", roundState.getRoundIdentifier());
-      blockToPublish = bestPreparedCertificate.get().getBlock();
+      Block preparedBlock = bestPreparedCertificate.get().getBlock();
+      final BftBlockInterface bftBlockInterface =
+          protocolContext.getConsensusContext(BftContext.class).getBlockInterface();
+      blockToPublish =
+          bftBlockInterface.replaceRoundInBlock(
+              preparedBlock,
+              roundState.getRoundIdentifier().getRoundNumber(),
+              BftBlockHeaderFunctions.forCommittedSeal(bftExtraDataCodec));
     }
 
+    LOG.debug(" proposal - new/prepared block hash : {}", blockToPublish.getHash());
+
     updateStateWithProposalAndTransmit(
         blockToPublish,
         roundChangeArtifacts.getRoundChanges(),
@@ -202,9 +211,10 @@ public class QbftRound {
         proposal.getSignedPayload().getPayload().getProposedBlock(),
         roundChanges,
         prepares);
-    updateStateWithProposedBlock(proposal);
+    if (updateStateWithProposedBlock(proposal)) {
       sendPrepare(block);
     }
+  }
 
   /**
    * Handle proposal message.

consensus/qbft/src/main/java/org/hyperledger/besu/consensus/qbft/statemachine/QbftRoundFactory.java

@@ -99,7 +99,8 @@ public class QbftRoundFactory {
    */
   public QbftRound createNewRoundWithState(
       final BlockHeader parentHeader, final RoundState roundState) {
-    final BlockCreator blockCreator = blockCreatorFactory.create(0);
+    final BlockCreator blockCreator =
+        blockCreatorFactory.create(roundState.getRoundIdentifier().getRoundNumber());
 
     // TODO(tmm): Why is this created everytime?!
     final QbftMessageTransmitter messageTransmitter =

consensus/qbft/src/main/java/org/hyperledger/besu/consensus/qbft/validation/ProposalValidator.java

@@ -137,6 +137,13 @@ public class ProposalValidator {
         final PreparedRoundMetadata metadata =
             roundChangeWithLatestPreparedRound.get().getPayload().getPreparedRoundMetadata().get();
 
+        LOG.debug(
+            "Prepared Metadata blockhash : {}, proposal blockhash: {}, prepared round in message: {}, proposal round in message: {}",
+            metadata.getPreparedBlockHash(),
+            proposal.getBlock().getHash(),
+            metadata.getPreparedRound(),
+            proposal.getRoundIdentifier().getRoundNumber());
+
         // The Hash in the roundchange/proposals is NOT the same as the value in the
         // prepares/roundchanges
         // as said payloads reference the block with an OLD round number in it - therefore, need
@@ -155,8 +162,10 @@ public class ProposalValidator {
 
         if (!metadata.getPreparedBlockHash().equals(expectedPriorBlockHash)) {
           LOG.info(
-              "{}: Latest Prepared Metadata blockhash does not align with proposed block",
-              ERROR_PREFIX);
+              "{}: Latest Prepared Metadata blockhash does not align with proposed block. Expected: {}, Actual: {}",
+              ERROR_PREFIX,
+              expectedPriorBlockHash,
+              metadata.getPreparedBlockHash());
           return false;
         }