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

ryanofsky commented at 5:39 AM on October 14, 2026: contributor

Problem: A program using the Chain IPC interface from #29409 can crash bitcoin-node with a single bad call: asking for a block hash above the tip, leaving the transaction argument of broadcastTransaction or checkChainLimits unset, or returning an error from a notification handler. Separately, findAncestorByHash reports success when the descendant block is unknown (#36472). The Chain methods were written for in-process C++ callers like the wallet, which follow documented preconditions, so they assert or dereference null instead of returning errors.

Solution: Make these Chain methods in src/node/interfaces.cpp return errors for invalid arguments, catch and log IPC errors from notification handlers, and fix the findAncestorByHash check. Three small commits, each with unit tests in src/test/interfaces_tests.cpp. Valid calls behave the same as before. Details are in the commit messages.

This PR does not depend on #29409, but the fixes matter if it is merged and interfaces::Chain is exposed over IPC. The problems were reported by zaidmstrr, willcl-ark and xyzconstant while reviewing #29409, and by enirox001 in #36472. The findAncestorByHash fix is the same one previously proposed by Romansage6 and Yudis-bit in #36473, #36484 and #36490.

interfaces: Fix findAncestorByHash returning true for unknown block

Chain::findAncestorByHash returned true when block_hash was unknown and
ancestor_hash was known, because the check clearing the ancestor pointer
only ran when both hashes were found. Clear the ancestor whenever the
block is not found.

Fixes #36472, reported by enirox001 while testing the IPC Chain interface
from #29409. The same fix was previously proposed in #36473, #36484 and
#36490 by Romansage6 and Yudis-bit.

This change was written with Claude Opus 5.5 (1M context).

03e46542e6

interfaces: Return errors instead of crashing on invalid Chain arguments

Chain methods were written for in-process C++ callers, which are expected
to follow documented preconditions. Over IPC, non-C++ clients can pass any
value, including null pointers for omitted Cap'n Proto fields, and an
invalid argument should not be able to crash the node.

- getBlockHash throws std::out_of_range instead of asserting when the
  height is not in the active chain. libmultiprocess sends the exception
  to the IPC client as an error.
- broadcastTransaction returns an error for a null transaction or an out
  of range broadcast method, instead of dereferencing null or silently
  doing nothing.
- checkChainLimits returns an error for a null transaction instead of
  dereferencing it.

These were reported by zaidmstrr and willcl-ark while reviewing #29409.

This change was written with Claude Opus 5.5 (1M context).

42fcc504e2

interfaces: Catch IPC exceptions thrown by Chain notification handlers

Chain::Notifications methods are called from the validation interface
scheduler thread, where an uncaught exception terminates the node.
A handler implemented by an IPC client throws ipc::Exception on the node
side if the client returns an error or disconnects. Catch and log these
exceptions so a misbehaving client cannot crash the node. Exceptions from
in-process handlers are still not caught, since they indicate bugs.

Reported by xyzconstant while reviewing #29409.

This change was written with Claude Opus 5.5 (1M context).

b67d871f95

DrahtBot added the label IPC on Oct 14, 2026

DrahtBot commented at 5:39 AM on October 14, 2026: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

External sites

Code coverage & benchmarks: https://corecheck.dev/bitcoin/bitcoin/pulls/36520
Ralph review <img src=https://review.fish.foo/assets/bitcoin/bitcoin/36520.png alt="Ralph review status"> https://git.fish.foo/bitcoin/bitcoin/pulls/36520#author-ralph

Reviews

See the guideline and AI policy for information on the review process.

Type	Reviewers
ACK	xyzconstant
Concept ACK	enirox001

If your review is incorrectly listed, please copy-paste <code></code> into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#36598 <img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/36598.svg"> ([Demo] validation: remove AssumeUTXO by l0rinc)
#36597 <img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/36597.svg"> (wallet: return broadcast errors from CommitTransaction by pseudoramdom)
#36074 <img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/36074.svg"> (scripted-diff: [test] Add util/check.h includes for assertions by maflcko)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

LLM Linter (✨ experimental)

Possible places where comparison-specific test macros should replace generic comparisons:

src/test/interfaces_tests.cpp: BOOST_CHECK_THROW(chain->getBlockHash(tip->nHeight + 1), std::out_of_range) → Prefer BOOST_CHECK_EXCEPTION with a predicate checking the exact “not in the active chain” message.
src/test/interfaces_tests.cpp: BOOST_CHECK_THROW(chain->getBlockHash(-1), std::out_of_range) → Prefer BOOST_CHECK_EXCEPTION with a predicate checking the exact “Block height -1 is not in the active chain” message.

2026-10-14 05:39:57

enirox001 commented at 10:43 AM on October 14, 2026: contributor

Concept ACK

I like this approach, which includes the specific fixes while also making the chain interface more defensive.

These methods were originally being called in process where the callers are expected to follow the preconditions, but once they are exposed over IPC, it makes sense to be more defensive around invalid inputs. Ideally an unexpected/bad IPC call should result in an error rather than crashing the nod

cyb3ralbert commented at 9:45 AM on October 22, 2026: contributor

Could hasBlocks() be covered here, or should it be a follow-up? It has the same kind of crash as the ones fixed here. With a negative max_height, block->GetAncestor(*max_height) returns null (chain.cpp:84), and the loop then dereferences it (node/interfaces.cpp:691-692 @ b67d871f95, reading the code). Over IPC, hasBlocks(tip, 0, -1) kills the node (exit -11) with and without this PR; an in-process scratch unit test hits a memory access violation.

Is out_of_range intended over a runtime_error-derived type? getBlockHash now throws std::out_of_range (a std::logic_error), while wallet/load.cpp:161 and wallet/wallet.cpp:313 catch only std::runtime_error. Reading the code, a throw during loadwallet would skip the FAILED_LOAD handling and reach the generic std::exception catch in rpc/server.cpp:891, and one during startup wallet loading would skip initError. I did not exercise this path.

Tested on Linux aarch64 (Debian 13), GCC 14.2.0, with b67d871f95fa1b9c47fc2f8d08d80e27f0a81e16 merged with #29409 (0928430e16, merges cleanly). The PR alone also merges cleanly onto master de96f3ae6d.

interfaces_tests: 8/8 pass. Full unit suite: 879 cases pass. Functional (tool_bitcoin, feature_init, wallet_*, mempool_*, interface_ipc*): 95 passed, 4 skipped (previous releases not available).
Reverting the src changes of one commit at a time, while keeping the PR's tests: 03e46542e6 makes findAncestorByHash fail at interfaces_tests.cpp:108; 42fcc504e2 makes invalidArguments abort in getBlockHash; b67d871f95 makes throwingNotifications terminate on the uncaught ipc::Exception. The other two tests pass in each case.
Over IPC with a Python capnp client, comparing the same tree with the PR's src changes reverted against the merged tree:
- getBlockHash(tip+1) and getBlockHash(-1): node exits with -6 (assertion), vs error Block height N is not in the active chain with the node alive.
- broadcastTransaction and checkChainLimits with tx unset: node exits with -11, vs Transaction is null.
- broadcastTransaction with method 255: (true, ""), vs Invalid broadcast method 255.
- findAncestorByHash(unknown block, known ancestor): true, vs false.
- A handler that raises in its first blockConnected: node exits with -6, vs node alive, one Chain notification blockConnected failed warning, and the handler still receives blockConnected and updatedBlockTip 3/3 for three mined blocks.
- Client process killed while a blockConnected call is in flight: terminate on ipc::Exception: IPC client method call interrupted by disconnect. (exit -6), vs node alive with Chain notification blockConnected failed: IPC client method call interrupted by disconnect. and the next block mined normally. Client killed while its handler is registered but no call is in flight: node alive in both.
Coverage nit: the PR's tests make only blockConnected and updatedBlockTip throw. A scratch case throwing ipc::Exception from all six Notifications methods passes on the head. Removing the Notify wrapper from transactionAddedToMempool, transactionRemovedFromMempool, blockDisconnected or chainStateFlushed one at a time leaves the PR's three tests passing, while the scratch case fails for each.

Not tested: a reorg between getHeight() and getBlockHash() at the wallet call sites.

in src/node/interfaces.cpp:521 in b67d871f95

 522 | +    void Notify(const char* name, Fn&& fn)
 523 | +    {
 524 | +        try {
 525 | +            fn();
 526 | +        } catch (const ipc::Exception& e) {
 527 | +            LogWarning("Chain notification %s failed: %s", name, e.what());

xyzconstant commented at 10:06 PM on October 22, 2026:

nit: This msg could be more explicit with something like "Failed to deliver %s notification to IPC subscriber: %s"

in src/node/interfaces.cpp:514 in b67d871f95

 515 | +    //! notification handler in an IPC client throws ipc::Exception if it
 516 | +    //! returns an error or disconnects, and should not be able to crash the
 517 | +    //! node, which happens if an exception escapes the validation interface
 518 | +    //! scheduler thread. Other exceptions are not caught: an in-process handler
 519 | +    //! like the wallet throwing indicates a bug, and continuing could leave it
 520 | +    //! with missing or inconsistent state.

xyzconstant commented at 10:19 PM on October 22, 2026:

Style nit, feel free to ignore:

I would like to see something shorter and more concise, like:

//! Call a chain notification method on the subscriber.
//!
//! For IPC subscribers: If the call fails because the subscriber returned an
//! error or disconnected, just log a warning. In-process subscribers are not
//! expected to fail.

xyzconstant commented at 11:09 PM on October 22, 2026: contributor

LGTM ACK b67d871f95. Left some nits on the NotificationsProxy changes.

This PR fixes a set of issues reported by IPC consumers testing the Chain interface exposed in #29409.

For anyone wondering, here is a short summary of the flawed Chain interface methods and their fixes:

findAncestorByHash - Returned an incorrect value (true) if an unknown block was passed, fixed by explicitly checking if the value returned by BlockManager::LookupBlockIndex on the block_hash arg was null.
getBlockHash - The assertion failed if the provided height was out of the active chain bounds, crashing the process. Fixed by throwing std::out_of_range instead.
broadcastTransaction, checkChainLimits - Passing a null tx crashes the process since it is internally dereferenced. Fixed by adding an early check on tx ensuring it's not null.
broadcastTransaction - The broadcast_method can be anything other than the values in TxBroadcast when received over IPC. Fixed by adding a helper that checks this arg is valid rather than relying on the type system.

In addition to these Chain interface methods, the process can also perform IPC calls back to subscribers provided by clients. Such IPC calls may fail, and before this PR, we didn't handle these failures because in-process call failures are bugs; now, this PR catches only IPC exceptions (ipc::Exception) and logs a warning.

Checked whether these issues exist across other Chain methods not modified by this PR, but only hasBlocks() (as @cyb3ralbert reported in #36520 (comment)) fails if a negative max_height is passed. Here's a diff to test it:

diff --git a/src/test/interfaces_tests.cpp b/src/test/interfaces_tests.cpp
index 031415bff1..98eb28a8e3 100644
--- a/src/test/interfaces_tests.cpp
+++ b/src/test/interfaces_tests.cpp
@@ -187,6 +187,8 @@ BOOST_FIXTURE_TEST_CASE(invalidArguments, TestChain100Setup)
     BOOST_CHECK_THROW(chain->getBlockHash(tip->nHeight + 1), std::out_of_range);
     BOOST_CHECK_THROW(chain->getBlockHash(-1), std::out_of_range);
 
+    BOOST_CHECK(!chain->hasBlocks(tip->GetBlockHash(), /*min_height=*/0, /*max_height=*/-1));
+
     BOOST_CHECK(!chain->checkChainLimits(nullptr));
 
     std::string err;

I think this issue should also be covered in this PR. Nonetheless, the current fixes here are clean and can be merged as is.

DrahtBot requested review from enirox001 on Oct 22, 2026