#35916 fuzz: improve ipc fuzz coverage
| Reviewability | Reviews | Agreement | Size | ||||
|---|---|---|---|---|---|---|---|
| #35916 fuzz: improve ipc fuzz coverage | Ready | 0 (+1) | Strong | 342 | |||
|
|
|
|
| |||
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
Expands the `ipc` fuzz target to exercise bidirectional IPC callbacks and inject arbitrary Cap'n Proto payloads for `CTransaction` and `UniValue` types. It also adds server-side assertions to ensure deserialized arguments match sent values prior to round-trip return.
Problem: The existing IPC fuzz target only tested round-trip calls using valid data generated by libmultiprocess serialization, leaving reverse callback communication, server-side deserialization discrepancies, and parser handling of raw or malformed payloads unexercised.
Evidence of importance:
- Follows up on merged PR #35118 and implements reviewer suggestions to assert arguments on the server side (PR description citing reviews on #35118).
- Continues implementation of the IPC fuzzing proposal outlined in open issue #23015.
Scope: Touches only fuzz testing code under `src/ipc/test/fuzz/` without changing production IPC or libmultiprocess logic.
Risk of a mistake: low (tests). Modifications are restricted entirely to IPC fuzz test targets and will not impact production node behavior or release binaries.
Changed since the previous assessment: Patch-id updated from cbcf35a3ec5185bc to 3dc8ba294e6b45da, but the overall approach, commit structure, and modified files remain unchanged.
Reviewability
Ready
Nothing known that would invalidate a review now.
Waiting on (from the thread read):
- reviewer: frankomosh to re-review after author added local deserialization checks for transactions and UniValue 2026-10-09
Agreement
Strong
substantive support, no open objection (jeanpablojp)
Thread notes: The author addressed frankomosh's objections regarding error swallowing and mutant killing by pushing local deserialization checks for transaction and UniValue payloads on both the request and response sides.
Objections, suggestions, and questions:
| Reviewer | Type | Harm, request, or question | Status | Must be | Author replied | Quote |
|---|---|---|---|---|---|---|
| jeanpablojp (contributor) | suggestion, maintenance | Add a comment explaining why sendUniValuePayload treats any kj error as fatal | fixed · fix pushed · author agreed and added the clarifying comment | answered | yes | 2026-08-15: 'Took me a bit to see why sendTransactionPayload swallows FAILED while sendUniValuePayload treats any kj error as fatal. That's correct, but you only get it by reading the hooks. A one-line comment wou' Status from: 2026-08-26: enirox001: 'added the suggested comment to clarify why any KJ error is unexpected here. Thanks' |
| Eunovo (member) | suggestion, approach | Use ConsumeTransaction from src/test/fuzz/util.cpp to generate a random transaction instead of a random length byte vector | answered · author explained raw bytes intentionally test client deserialization with malformed responses; frankomosh noted fuzzer quickly learns valid txs | answered | yes | 2026-09-15: 'Use `ConsumeTransaction` in `src/test/fuzz/util.cpp` to create a random tx from a `FuzzedDataProvider` instead of using a random lenght byte vector.' Status from: 2026-09-16: enirox001: 'We intentionally return arbitrary transaction bytes from the raw server so the client deserialization is exercised with ' |
| Eunovo (member) | suggestion, approach | Construct a valid JSON payload instead of raw bytes that mostly generate invalid JSON | answered · author explained invalid JSON intentionally tests client rejection of malformed responses, while valid JSON is covered separately | answered | yes | 2026-09-15: 'Most of the generated data will be invalid JSON; this means the fuzzer will hit the "invalid JSON received over IPC" error most of the time. I think it's much better to construct a valid JSON payload,' Status from: 2026-09-16: enirox001: 'Invalid json is intentional here because it tests client side rejection of malformed ipc responses.' |
| Eunovo (member) | suggestion, approach | Use ConsumeTransaction from src/test/fuzz/util.cpp for raw request payload vectors | answered · author reiterated that bypassing normal serialization intentionally exercises malformed transaction payload handling | answered | yes | 2026-09-15: 'You should use `ConsumeTransaction` from `src/test/fuzz/util.cpp`. Most of the vectors produced will be invalid.' Status from: 2026-09-16: enirox001: 'this intentionally bypasses normal serialization to exercise malformed transaction payload handling.' |
| Eunovo (member) | suggestion, approach | Construct valid JSON payloads for request branches | answered · author referred back to previous response | answered | yes | 2026-09-15: 'Same as in my previous UniValue comment.' Status from: 2026-09-16: enirox001: 'Same reason as https://github.com/bitcoin/bitcoin/pull/35916#discussion_r4028056893' |
| frankomosh (contributor) | objection, correctness | Accepting any server error instead of only bad tx bytes errors could hide real server bugs | fixed · fix pushed · author agreed and added checks on both request and response sides so ipc succeeds when bytes decode locally and fails when they do not | changed | yes | 2026-10-02: 'This might be accepting just about any server error (I believe you intend for it to accept only "bad tx bytes" errors). And could therefore hide a real server bug.' Status from: 2026-10-09: enirox001: 'Added the check on both the request and response sides, so ipc must succeed when the bytes decode locally and reject them when they don’t.' |
| frankomosh (contributor) | objection, correctness | Add a response-side check to verify invalid JSON rejection so the fuzzer catches mutants removing the rejection | fixed · fix pushed · author added local UniValue::read() checks on both request and response sides so valid json succeeds and invalid json produces expected rejection | changed | yes | 2026-10-02: 'One possible option of killing it is the same approach as the tx one above. I tried it, and the response-side check catches the mutant' Status from: 2026-10-09: enirox001: 'Added local `UniValue::read()` checks on both the request and response sides, so valid json succeeds and invalid json produces the expected rejection error.' |
Support:
- jeanpablojp (contributor) (Tested ACK, 2026-08-15) [both]: All green, and it matches the follow-up #35118 asked for.
- frankomosh (contributor) (Concept ACK, 2026-10-02): (no reason given) [not substantive]
Participants:
- jeanpablojp (contributor, support, 2 statements 2026-08-15 to 2026-08-15): Tested ACK; suggested an explanatory comment on UniValue error handling
- Eunovo (member, neutral, 5 statements 2026-09-15 to 2026-09-15): suggested generating structured valid payloads instead of random byte vectors
- frankomosh (contributor, objection, 4 statements 2026-10-02 to 2026-10-02): Concept ACK, but objected that swallowing all server errors could hide bugs, and suggested killing mutants that remove invalid JSON rejection
Dependencies
Enables:
Files
0 lines under test/bench/ci.
- src/ipc/test/fuzz/ipc.cpp +255/-7
- src/ipc/test/fuzz/ipc_fuzz.h +78/-6
- src/ipc/test/fuzz/ipc_fuzz.capnp +9/-0
Card
The short text the ranking pass compares across PRs.
Expands the IPC fuzz target to test bidirectional callback communication, server-side argument integrity, and client/server deserialization of arbitrary raw transaction and UniValue payloads. The change ensures libmultiprocess and its Cap'n Proto hooks handle malformed inputs gracefully without relying solely on valid round-trip serialization. It builds on #35118 and contributes toward multiprocess fuzzing goals in #23015.
Data
thread read through 2026-10-09 (20 statements), head 20ba949c0e
labels: Fuzzing
Categories
IPC / multiprocess
P3 · test coverage
Expands the IPC fuzz target to test bidirectional callbacks, arbitrary Cap'n Proto payloads, and server-side deserialization. This provides useful coverage for process-boundary serialization as outlined in #23015 and requested in #35118, but does not fix an active bug or block core functionality.
Membership: Changes fuzz testing files located entirely within `src/ipc/test/fuzz/` to test IPC interfaces and Cap'n Proto deserialization.
Factors: security/stability 1, bug 0, performance 0, user value 0, leverage 1