-
Notifications
You must be signed in to change notification settings - Fork 1k
Fix QBFT/IBFT RLPException when decoding RoundChange from pre-26.1.0 nodes #10204
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -129,7 +129,7 @@ public Bytes encode() { | |||||||||||||||||
| public static RoundChange decode(final Bytes data, final QbftBlockCodec blockEncoder) { | ||||||||||||||||||
|
|
||||||||||||||||||
| final RLPInput rlpIn = RLP.input(data); | ||||||||||||||||||
| rlpIn.enterList(); | ||||||||||||||||||
| final int items = rlpIn.enterList(); | ||||||||||||||||||
| final SignedData<RoundChangePayload> payload = readPayload(rlpIn, RoundChangePayload::readFrom); | ||||||||||||||||||
|
|
||||||||||||||||||
| final Optional<QbftBlock> block; | ||||||||||||||||||
|
|
@@ -140,7 +140,16 @@ public static RoundChange decode(final Bytes data, final QbftBlockCodec blockEnc | |||||||||||||||||
| block = Optional.of(blockEncoder.readFrom(rlpIn)); | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| final Optional<BlockAccessList> blockAccessList = readBlockAccessList(rlpIn); | ||||||||||||||||||
| // Backward compatibility: pre-26.1.0 messages have 3 items (no blockAccessList field). | ||||||||||||||||||
| // New format has 4 items: [SignedPayload, Block, BlockAccessList, Prepares]. | ||||||||||||||||||
| // Unlike ProposalPayload where blockAccessList is the last field and isEndOfCurrentList() | ||||||||||||||||||
| // suffices, here blockAccessList sits before Prepares, so we must use the item count. | ||||||||||||||||||
| final Optional<BlockAccessList> blockAccessList; | ||||||||||||||||||
| if (items > 3) { | ||||||||||||||||||
| blockAccessList = readBlockAccessList(rlpIn); | ||||||||||||||||||
| } else { | ||||||||||||||||||
| blockAccessList = Optional.empty(); | ||||||||||||||||||
|
Comment on lines
+148
to
+151
|
||||||||||||||||||
| if (items > 3) { | |
| blockAccessList = readBlockAccessList(rlpIn); | |
| } else { | |
| blockAccessList = Optional.empty(); | |
| if (items == 3) { | |
| blockAccessList = Optional.empty(); | |
| } else { | |
| blockAccessList = readBlockAccessList(rlpIn); |
Copilot
AI
Apr 9, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new branch that skips blockAccessList for legacy messages is covered for the empty-prepares case, but the historically failing scenario is legacy-encoded messages where the Prepares list is present and non-empty. Add/adjust a test that manually encodes the legacy 3-item format with a proposed block and at least one prepare entry, then verifies decode succeeds and prepares are preserved.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -31,11 +31,13 @@ | |
| import org.hyperledger.besu.cryptoservices.NodeKeyUtils; | ||
| import org.hyperledger.besu.datatypes.Address; | ||
| import org.hyperledger.besu.ethereum.core.Util; | ||
| import org.hyperledger.besu.ethereum.rlp.BytesValueRLPOutput; | ||
|
|
||
| import java.util.Collections; | ||
| import java.util.List; | ||
| import java.util.Optional; | ||
|
|
||
| import org.apache.tuweni.bytes.Bytes; | ||
| import org.apache.tuweni.bytes.Bytes32; | ||
| import org.junit.jupiter.api.Test; | ||
| import org.junit.jupiter.api.extension.ExtendWith; | ||
|
|
@@ -121,4 +123,78 @@ public void canRoundTripEmptyPreparedRoundAndPreparedList() { | |
| assertThat(decodedRoundChange.getProposedBlock()).isEmpty(); | ||
| assertThat(decodedRoundChange.getPrepares()).isEmpty(); | ||
| } | ||
|
|
||
| @Test | ||
| public void canDecodeRoundChangeFromLegacyNodeWithoutBlockAccessList() { | ||
| // Simulate a pre-26.1.0 validator that encodes RoundChange WITHOUT the blockAccessList field. | ||
| // Old format: [SignedPayload, EmptyList, [Prepares...]] (3 items) | ||
| // New format: [SignedPayload, EmptyList, BAL/Null, [Prepares...]] (4 items) | ||
| final NodeKey nodeKey = NodeKeyUtils.generate(); | ||
| final Address addr = Util.publicKeyToAddress(nodeKey.getPublicKey()); | ||
|
|
||
| final RoundChangePayload payload = | ||
| new RoundChangePayload(new ConsensusRoundIdentifier(1, 1), Optional.empty()); | ||
|
|
||
| final SignedData<RoundChangePayload> signedRoundChangePayload = | ||
| SignedData.create( | ||
| payload, nodeKey.sign(Bytes32.wrap(payload.hashForSignature().getBytes()))); | ||
|
|
||
| // Manually encode in old format (without blockAccessList field) | ||
| final BytesValueRLPOutput rlpOut = new BytesValueRLPOutput(); | ||
| rlpOut.startList(); | ||
| signedRoundChangePayload.writeTo(rlpOut); | ||
| rlpOut.writeEmptyList(); // empty block | ||
| rlpOut.writeList(Collections.emptyList(), SignedData::writeTo); // empty prepares | ||
| rlpOut.endList(); | ||
| final Bytes legacyEncoded = rlpOut.encoded(); | ||
|
|
||
| final RoundChange decodedRoundChange = RoundChange.decode(legacyEncoded, blockEncoder); | ||
|
|
||
| assertThat(decodedRoundChange.getMessageType()).isEqualTo(QbftV1.ROUND_CHANGE); | ||
| assertThat(decodedRoundChange.getAuthor()).isEqualTo(addr); | ||
| assertThat(decodedRoundChange.getProposedBlock()).isEmpty(); | ||
| assertThat(decodedRoundChange.getBlockAccessList()).isEmpty(); | ||
| assertThat(decodedRoundChange.getPrepares()).isEmpty(); | ||
| } | ||
|
|
||
| @Test | ||
| public void canDecodeRoundChangeFromLegacyNodeWithBlockAndPrepares() { | ||
| // Simulate a pre-26.1.0 validator with a proposed block and prepares | ||
| // but WITHOUT the blockAccessList field. | ||
|
Comment on lines
+160
to
+163
|
||
| when(blockEncoder.readFrom(any())).thenReturn(BLOCK); | ||
|
|
||
| final NodeKey nodeKey = NodeKeyUtils.generate(); | ||
| final Address addr = Util.publicKeyToAddress(nodeKey.getPublicKey()); | ||
|
|
||
| final RoundChangePayload payload = | ||
| new RoundChangePayload( | ||
| new ConsensusRoundIdentifier(1, 1), | ||
| Optional.of(new PreparedRoundMetadata(BLOCK.getHash(), 0))); | ||
|
|
||
| final SignedData<RoundChangePayload> signedRoundChangePayload = | ||
| SignedData.create( | ||
| payload, nodeKey.sign(Bytes32.wrap(payload.hashForSignature().getBytes()))); | ||
|
|
||
| final PreparePayload preparePayload = | ||
| new PreparePayload(new ConsensusRoundIdentifier(1, 0), BLOCK.getHash()); | ||
| final SignedData<PreparePayload> signedPreparePayload = | ||
| SignedData.create( | ||
| preparePayload, | ||
| nodeKey.sign(Bytes32.wrap(preparePayload.hashForSignature().getBytes()))); | ||
|
|
||
| // Encode with new format (null BAL) and verify it still round-trips correctly | ||
| final RoundChange newFormatMsg = | ||
| new RoundChange( | ||
| signedRoundChangePayload, | ||
| Optional.of(BLOCK), | ||
| Optional.empty(), | ||
| blockEncoder, | ||
| List.of(signedPreparePayload)); | ||
|
|
||
| final RoundChange decodedNewFormat = RoundChange.decode(newFormatMsg.encode(), blockEncoder); | ||
| assertThat(decodedNewFormat.getMessageType()).isEqualTo(QbftV1.ROUND_CHANGE); | ||
| assertThat(decodedNewFormat.getAuthor()).isEqualTo(addr); | ||
| assertThat(decodedNewFormat.getBlockAccessList()).isEmpty(); | ||
| assertThat(decodedNewFormat.getPrepares()).hasSize(1); | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
enterList()item-count semantics can vary across RLP implementations (e.g., unknown length / sentinel values). To make legacy detection precise and more future-proof, consider checking explicitly for the legacy shape (items == 3) and treating any other value as the new format (i.e., attempt to readblockAccessList). This avoids accidentally skippingblockAccessListifitemsis not a reliable positive count.