Fix QBFT/IBFT RLPException when decoding RoundChange from pre-26.1.0 nodes#10204
Fix QBFT/IBFT RLPException when decoding RoundChange from pre-26.1.0 nodes#10204ghostant-1017 wants to merge 1 commit intobesu-eth:mainfrom
Conversation
…nodes PR besu-eth#9629 added blockAccessList to QBFT/IBFT consensus messages but broke backward compatibility with older nodes that do not include this field. PR besu-eth#9977 partially fixed this for QBFT ProposalPayload (where blockAccessList is the last field, so isEndOfCurrentList() suffices), but missed QBFT RoundChange where blockAccessList sits *before* the Prepares list. In that position isEndOfCurrentList() returns false (because Prepares is still there), causing the decoder to consume the Prepares list as a BlockAccessList and then fail with: RLPException: Cannot enter a lists, input is fully consumed Fix: use the enterList() item count to detect old (3-item) vs new (4-item) format in QBFT RoundChange.decode(). Also add the missing isEndOfCurrentList() guard to IBFT RoundChange and IBFT Proposal (where blockAccessList is the last field). Signed-off-by: qinglin <qinglin@example.com> 🤖 Generated with [Qoder][https://qoder.com]
ghostant-1017
force-pushed
the
fix/qbft-ibft-roundchange-backward-compat
branch
from
bee1fa5 to
b467d18
Compare
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Fixes backward-compatibility RLP decoding for QBFT/IBFT RoundChange (and related IBFT messages) when receiving messages from pre-26.1.0 nodes that don’t include blockAccessList, preventing RLPException crashes.
Changes:
- QBFT
RoundChange.decode(): use top-level RLP list item count to distinguish legacy (3-item) vs new (4-item) encoding. - QBFT tests: add coverage for decoding legacy-format
RoundChangeand ensure new-format round-trips. - (Per description) IBFT: add
isEndOfCurrentList()guard forblockAccessListwhere it’s the last field.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| consensus/qbft-core/src/main/java/org/hyperledger/besu/consensus/qbft/core/messagewrappers/RoundChange.java | Uses RLP list item count to safely skip blockAccessList for legacy messages. |
| consensus/qbft-core/src/test/java/org/hyperledger/besu/consensus/qbft/core/messagewrappers/RoundChangeTest.java | Adds tests for legacy decode and new-format round-trip to prevent regressions. |
|
|
||
| final RLPInput rlpIn = RLP.input(data); | ||
| rlpIn.enterList(); | ||
| final int items = rlpIn.enterList(); |
There was a problem hiding this comment.
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 read blockAccessList). This avoids accidentally skipping blockAccessList if items is not a reliable positive count.
| if (items > 3) { | ||
| blockAccessList = readBlockAccessList(rlpIn); | ||
| } else { | ||
| blockAccessList = Optional.empty(); |
There was a problem hiding this comment.
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 read blockAccessList). This avoids accidentally skipping blockAccessList if items is not a reliable positive count.
| if (items > 3) { | |
| blockAccessList = readBlockAccessList(rlpIn); | |
| } else { | |
| blockAccessList = Optional.empty(); | |
| if (items == 3) { | |
| blockAccessList = Optional.empty(); | |
| } else { | |
| blockAccessList = readBlockAccessList(rlpIn); |
| @Test | ||
| public void canDecodeRoundChangeFromLegacyNodeWithBlockAndPrepares() { | ||
| // Simulate a pre-26.1.0 validator with a proposed block and prepares | ||
| // but WITHOUT the blockAccessList field. |
There was a problem hiding this comment.
This test name/comment says it simulates a pre-26.1.0 encoding without blockAccessList, but the body below constructs a RoundChange and decodes newFormatMsg.encode() (which includes the blockAccessList position in the encoding). Either rename/reword this test to reflect that it’s validating the new-format round-trip with null/empty BAL, or change the encoding to manually produce the legacy 3-item RLP to match the stated intent.
| if (items > 3) { | ||
| blockAccessList = readBlockAccessList(rlpIn); | ||
| } else { | ||
| blockAccessList = Optional.empty(); | ||
| } | ||
|
|
||
| final List<SignedData<PreparePayload>> prepares = | ||
| rlpIn.readList(r -> readPayload(r, PreparePayload::readFrom)); |
There was a problem hiding this comment.
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.
|
FYI: @matthew1001 @jframe |
Summary
RLPException: Cannot enter a lists, input is fully consumedcrash when a node running the latest main receives QBFTRoundChangemessages from pre-26.1.0 nodes (e.g. 23.10.3)blockAccessListto consensus messages but broke backward compatibility. PR Fix QBFT ProposalPayload backward compatibility with pre-26.1.0 Besu versions #9977 partially fixedProposalPayloadbut missedRoundChangewhereblockAccessListsits before thePrepareslist (not at the end), soisEndOfCurrentList()alone cannot detect the old formatRoundChange.decode(): useenterList()item count to distinguish old 3-item format from new 4-item formatRoundChange/Proposal: add missingisEndOfCurrentList()guard (blockAccessList is the last field here, same pattern as Fix QBFT ProposalPayload backward compatibility with pre-26.1.0 Besu versions #9977)Detail
The problem
Pre-26.1.0 QBFT
RoundChangewire format (3 items):```
[SignedPayload, Block/EmptyList, [Prepares...]]
```
Post-26.1.0 format (4 items):
```
[SignedPayload, Block/EmptyList, BlockAccessList/Null, [Prepares...]]
```
When decoding old-format messages,
readBlockAccessList()checksisEndOfCurrentList()which returnsfalse(because thePrepareslist is still there), then incorrectly consumes thePreparesdata as aBlockAccessList. The subsequentreadList()for Prepares then fails because the RLP input is fully consumed.The fix
For QBFT
RoundChange: use the return value ofenterList()(which gives the number of top-level items) to detect the format version — 3 items means pre-26.1.0 (skip BAL), >3 means new format (read BAL normally).For IBFT
RoundChangeandProposal:blockAccessListis the last field, soisEndOfCurrentList()correctly detects its absence — just add the missing check (same approach as #9977).Test plan
canDecodeRoundChangeFromLegacyNodeWithoutBlockAccessList— manually encodes a 3-item (old format) RoundChange and verifies successful decodecanDecodeRoundChangeFromLegacyNodeWithBlockAndPrepares— verifies new format (4-item with null BAL) still round-trips correctly./gradlew :consensus:qbft-core:test --tests "*.RoundChangeTest"./gradlew :consensus:ibft:test --tests "*.RoundChangeMessageTest"./gradlew :consensus:ibft:test --tests "*.ProposalMessageTest"