#36520 interfaces: Avoid node crashes from invalid Chain IPC calls

ReviewabilityReviewsAgreementSize
#36520 ryanofsky interfaces: Avoid node crashes from invalid Chain IPC callsReady2Strong53 + 67 tests
  • Prevent node crashes and scheduler termination when IPC clients pass invalid inputs or fail notifications
  • Fix findAncestorByHash returning success when the descendant block is missing
  • Ready to review
  • Substantive support with no open objections
  • Verified by functional testing that reported bugs are resolved (enirox001)
  • Support because it cleanly fixes issues found while testing IPC interfaces (xyzconstant)
  • Suggestions open on covering another crash case and exception type choice (cyb3ralbert, xyzconstant)
  • 53 lines added or modified outside tests
  • 67 lines added or modified in tests
  • 9 lines removed in total
  • 3 files, 3 commits
IPC / multiprocess#1 of 18
P1 · bug fix
  • P1 because it prevents node crashes and scheduler thread exits caused by untrusted or failing IPC callers
  • Resilience against invalid arguments and client disconnects is essential across the process boundary
  • Fixes issues encountered while developing multiprocess support
  • Ranking pass: Base of the Chain-over-IPC chain: standalone crash hardening that #29409 and #10102 build on, so it is worth a reviewer first.
Ranking notes
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

Hardens node implementations of interfaces::Chain against crashes caused by invalid arguments or IPC notification failures. Replaces asserts and null dereferences in getBlockHash, broadcastTransaction, and checkChainLimits with bounds checks and error returns. Wraps validation notification forwarding in NotificationsProxy to catch ipc::Exception and avoid crashing the validation scheduler thread, and fixes findAncestorByHash returning true when the descendant block hash is unknown.

Problem: interfaces::Chain methods were designed for well-behaved in-process callers like the internal wallet and assert or dereference null on invalid inputs. When exposed across IPC to out-of-process clients, omitted parameters or invalid inputs crash bitcoin-node, and errors returned by client notification handlers terminate the validation interface scheduler thread. Additionally, findAncestorByHash falsely reports success when the descendant block is missing.

Evidence of importance:

  • Fixes #36472 ('interfaces: findAncestorByHash returns true for an unknown descendant')
  • Author notes: 'The problems were reported by zaidmstrr, willcl-ark and xyzconstant while reviewing #29409, and by enirox001 in #36472.'
  • Commit message notes that notifications run on the validation interface scheduler thread, where uncaught exceptions from disconnected or failing IPC clients terminate the node process.

Risk of a mistake: low (validation, other). Mistakes would at most cause valid interface calls to return unexpected errors or fail to notify an attached IPC client.

Reviewability

Ready

Nothing known that would invalidate a review now.

Waiting on (from the thread read):

  • author: author reply to test cleanup suggestion and questions regarding scope and exception types 2026-10-30

Agreement

Strong

substantive support, no open objection (enirox001, xyzconstant, enirox001)

Thread notes: Added enirox001's Tested ACK and inline suggestion regarding redundant queue synchronization in unit tests. Their comment about shared input validation across the interface was noted as a followup question and not recorded as a claim holding this PR.

Objections, suggestions, and questions:

ReviewerTypeHarm, request, or questionStatusMust beAuthor repliedQuote
cyb3ralbert (contributor)suggestion, scopecover hasBlocks() crash on negative max_height here or in a follow-upopenansweredno2026-10-22: 'Could hasBlocks() be covered here, or should it be a follow-up?'
cyb3ralbert (contributor)question, interfacewhether throwing std::out_of_range in getBlockHash was intended over a std::runtime_error-derived type given wallet catch blocksopenansweredno2026-10-22: 'Is out_of_range intended over a runtime_error-derived type?'
cyb3ralbert (contributor)suggestion, safetyexpand unit tests to cover all six Notifications methods throwing ipc::Exceptionopenansweredno2026-10-22: 'Coverage nit: the PR's tests make only blockConnected and updatedBlockTip throw.'
xyzconstant (contributor)suggestion, scopecover the hasBlocks() negative max_height crash in this PRopenansweredno2026-10-22: 'I think this issue should also be covered in this PR. Nonetheless, the current fixes here are clean and can be merged as is.'
enirox001 (contributor)suggestion, maintenanceremove redundant queue synchronization in throwingNotifications since CreateAndProcessBlock already synchronizes notifications by defaultopenansweredno2026-10-30: 'We can remove the redundant queue synchronization in throwingNotifications as `CreateAndProcessBlock` already synchronizes notifications by default'

Support:

  • enirox001 (contributor) (Concept ACK, 2026-10-14) [read]: makes the chain interface more defensive against invalid inputs when exposed over IPC instead of crashing the node
  • xyzconstant (contributor) (ACK, 2026-10-22) [read]: fixes issues reported by IPC consumers testing the Chain interface; current fixes are clean and can be merged as is
  • enirox001 (contributor) (Tested ACK, 2026-10-30) [both]: fixes resolve problems reported in #36472 and during review of #29409; confirmed resolved in a functional test

Participants:

  • enirox001 (contributor, support, 3 statements 2026-10-14 to 2026-10-30): supports defensively handling IPC calls; tested fix for #36472 and suggested removing redundant test sync
  • cyb3ralbert (contributor, question, 1 statements 2026-10-22 to 2026-10-22): tested extensively; asked whether hasBlocks() should be covered and questioned throwing std::out_of_range
  • xyzconstant (contributor, support, 3 statements 2026-10-22 to 2026-10-22): ACK b67d871f95; suggested also covering hasBlocks() negative max_height crash

Dependencies

Enables:

Files

67 lines under test/bench/ci.

  • src/test/interfaces_tests.cpp +67/-0
  • src/node/interfaces.cpp +51/-8
  • src/interfaces/chain.h +2/-1

Card

The short text the ranking pass compares across PRs.

This PR hardens Bitcoin Core's Chain interface implementations against crashes from invalid arguments and IPC client errors. Currently, Chain methods assume well-behaved in-process callers, asserting or dereferencing null pointers on invalid inputs and permitting client exceptions on the validation scheduler thread to terminate the node. It also fixes a logic bug where findAncestorByHash reports success for unknown descendant blocks (issue 36472). While standalone, these changes primarily safeguard the node before exposing the Chain interface over multiprocess IPC in PR 29409.

Data

dossier JSON · extract JSON · model ledger, generated 2026-10-31T01:17, confidence high, input hash 0ced3e16e3d77384
thread read through 2026-10-30 (7 statements), head b67d871f95
labels: IPC

Categories

IPC / multiprocess

P1 · bug fix (ranking pass: Base of the Chain-over-IPC chain: standalone crash hardening that #29409 and #10102 build on, so it is worth a reviewer first.)

Prevents bitcoin-node crashes and scheduler thread termination caused by invalid IPC arguments or failed client notification handlers. In the IPC category, crash and disconnect handling across the process boundary is a core reliability requirement.

Membership: Carries the IPC label and modifies src/node/interfaces.cpp and src/interfaces/chain.h to harden interfaces::Chain against invalid cross-process calls and IPC notification exceptions.

Factors: security/stability 2, bug 2, performance 0, user value 0, leverage 1