From f64c147c4ac7ff21e7867830552ab1667e8e65a4 Mon Sep 17 00:00:00 2001 From: Bhanu Pulluri <59369753+pullurib@users.noreply.github.com> Date: Mon, 25 Nov 2024 15:55:22 -0500 Subject: [PATCH] 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 --------- Signed-off-by: Bhanu Pulluri Co-authored-by: Bhanu Pulluri Co-authored-by: Sally MacFarlane --- CHANGELOG.md | 1 + .../consensus/qbft/support/TestContext.java | 36 +++++++++++++++---- .../consensus/qbft/test/RoundChangeTest.java | 16 +++++---- .../qbft/statemachine/QbftRound.java | 16 +++++++-- .../qbft/statemachine/QbftRoundFactory.java | 3 +- .../qbft/validation/ProposalValidator.java | 13 +++++-- 6 files changed, 66 insertions(+), 19 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5472920ef5..e02ad1bc02 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/consensus/qbft/src/integration-test/java/org/hyperledger/besu/consensus/qbft/support/TestContext.java b/consensus/qbft/src/integration-test/java/org/hyperledger/besu/consensus/qbft/support/TestContext.java index 10cdfa9150..54dab34bdf 100644 --- a/consensus/qbft/src/integration-test/java/org/hyperledger/besu/consensus/qbft/support/TestContext.java +++ b/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) { diff --git a/consensus/qbft/src/integration-test/java/org/hyperledger/besu/consensus/qbft/test/RoundChangeTest.java b/consensus/qbft/src/integration-test/java/org/hyperledger/besu/consensus/qbft/test/RoundChangeTest.java index be439e4c1e..b1464efd53 100644 --- a/consensus/qbft/src/integration-test/java/org/hyperledger/besu/consensus/qbft/test/RoundChangeTest.java +++ b/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> 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( diff --git a/consensus/qbft/src/main/java/org/hyperledger/besu/consensus/qbft/statemachine/QbftRound.java b/consensus/qbft/src/main/java/org/hyperledger/besu/consensus/qbft/statemachine/QbftRound.java index e0a8cb1c72..64f23cef27 100644 --- a/consensus/qbft/src/main/java/org/hyperledger/besu/consensus/qbft/statemachine/QbftRound.java +++ b/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,8 +211,9 @@ public class QbftRound { proposal.getSignedPayload().getPayload().getProposedBlock(), roundChanges, prepares); - updateStateWithProposedBlock(proposal); - sendPrepare(block); + if (updateStateWithProposedBlock(proposal)) { + sendPrepare(block); + } } /** diff --git a/consensus/qbft/src/main/java/org/hyperledger/besu/consensus/qbft/statemachine/QbftRoundFactory.java b/consensus/qbft/src/main/java/org/hyperledger/besu/consensus/qbft/statemachine/QbftRoundFactory.java index 3025330009..6383945bcd 100644 --- a/consensus/qbft/src/main/java/org/hyperledger/besu/consensus/qbft/statemachine/QbftRoundFactory.java +++ b/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 = diff --git a/consensus/qbft/src/main/java/org/hyperledger/besu/consensus/qbft/validation/ProposalValidator.java b/consensus/qbft/src/main/java/org/hyperledger/besu/consensus/qbft/validation/ProposalValidator.java index 81cbffdff9..bc3f1bb205 100644 --- a/consensus/qbft/src/main/java/org/hyperledger/besu/consensus/qbft/validation/ProposalValidator.java +++ b/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; }