#36642 rpc: reset id and version per batch element
| Reviewability | Reviews | Agreement | Size | ||||
|---|---|---|---|---|---|---|---|
| #36642 rpc: reset id and version per batch element | Ready | 0 (+1) | Strong | 5 + 14 tests | |||
|
|
|
|
| |||
Reviewability: ReadyStalePaused Agreement: StrongPositivePositive w/ caveatsNeutralMildDisputedBlockedCrickets Reviews: current code-review ACKs, then (+stale ACKs) and -NACKs; greener = more ACKs. Size: lines added or modified outside tests, then in tests; greener = smaller. | |||||||
Goal
Reset the request ID and JSON-RPC version fields at the start of JSONRPCRequest::parse. This ensures malformed non-object elements in a batch request do not inherit the ID and version of the preceding element when generating error replies.
Problem: When processing a JSON-RPC batch, the server reuses a single JSONRPCRequest object across all items. If an element is not a JSON object, parse() throws an error before resetting the ID and version, causing the error response to reuse the previous element's ID or omit the error entirely if the previous element was a JSON-RPC 2.0 notification.
Evidence of importance:
- The author demonstrates that a batch like [{"jsonrpc":"2.0","method":"getblockcount","id":"X"}, 2] generates two reply objects with id 'X' (one result and one error), and provides a new test case in test/functional/interface_rpc.py that fails on master and passes with this change (PR description)
Risk of a mistake: medium (rpc_api, tests). A mistake in RPC request state handling could misformat or drop batch responses, confusing RPC clients.
Changed since the previous assessment: None; the previous description was created without the diff, and the diff matches the previous description.
Reviewability
Ready
Nothing known that would invalidate a review now.
Waiting on (from the thread read):
- reviewer: re-review of updated commit 2ab7c47f60 from AndreaDiazCorreia and pinheadmz 2026-11-01
Agreement
Strong
substantive support, no open objection (pinheadmz)
Thread notes: fametrano addressed all three suggestions by removing the redundant version reset and simplifying functional test assertions in push 2ab7c47f60.
Objections, suggestions, and questions:
| Reviewer | Type | Harm, request, or question | Status | Must be | Author replied | Quote |
|---|---|---|---|---|---|---|
| AndreaDiazCorreia (none) | suggestion, maintenance | remove the redundant m_json_version reset before the jsonrpc check | fixed · fix pushed · removed redundant reset in push 2ab7c47f60 | answered | yes | 2026-10-26: 'the existing m_json_version reset before the jsonrpc check is now redundant and could be removed' Status from: 2026-11-01: fametrano: 'I removed the second `m_json_version` reset' |
| pinheadmz (member) | suggestion, maintenance | remove the copy of the reset statement on line 221 | fixed · fix pushed · removed duplicate reset in push 2ab7c47f60 | answered | yes | 2026-10-26: 'remove the copy of this statement on line 221?' Status from: 2026-11-01: fametrano: 'I removed the second `m_json_version` reset' |
| pinheadmz (member) | suggestion, approach | simplify functional test assertions by verifying response correctness and distinct IDs | fixed · fix pushed · simplified test assertions in push 2ab7c47f60 | answered | yes | 2026-10-26: 'what if you just assert the response to the valid request is correct and the second response exists, and doesn't have the same id?' Status from: 2026-11-01: fametrano: 'simplified the test's asserts' |
Support:
- AndreaDiazCorreia (none) (ACK, 2026-10-26) [both]: Reviewed and tested, looks good to me. [not substantive]
- pinheadmz (member) (Concept ACK, 2026-10-26) [read]: This is a reasonable follow up to #27101 and fixes something we missed from the jsonrpc 2.0 spec that could confuse consumers of the interface expecting unique IDs in batch responses.
Participants:
- AndreaDiazCorreia (none, support, 1 statements 2026-10-26 to 2026-10-26): ACKed after testing, suggested removing redundant m_json_version reset
- pinheadmz (member, support, 3 statements 2026-10-26 to 2026-10-26): Concept ACKed, suggested removing duplicate reset and simplifying test assertions
Files
14 lines under test/bench/ci.
- test/functional/interface_rpc.py +14/-0
- src/rpc/request.cpp +5/-0
Card
The short text the ranking pass compares across PRs.
Reset request ID and JSON-RPC version fields at the beginning of request parsing rather than after validating object types. In a JSON-RPC batch, a malformed non-object element throws before resetting those fields on the reused request structure, causing the error reply to reuse the previous element's ID or suppress the error if the previous element was a notification. The author provides a new test in interface_rpc.py demonstrating the bug and verifying the fix.
Data
thread read through 2026-11-01 (5 statements), head 2ab7c47f60
labels: RPC/REST/ZMQ
Categories
RPC / REST / ZMQ
P3 · bug fix
Fixes an RPC batch error-handling bug where malformed elements can reuse the previous request's ID or omit errors entirely. In src/rpc/request.cpp, parse() reuses state across batch items, causing malformed non-object items to inherit the preceding item's ID or notification status (PR description, pinheadmz review). While fixing spec compliance for returned batch responses, this only affects malformed batch payloads and is reasonably deferrable.
Membership: Matches the RPC/REST/ZMQ area label and updates JSON-RPC request parsing in src/rpc/request.cpp.
Factors: security/stability 0, bug 1, performance 0, user value 1, leverage 0