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, passing a negative height to hasBlocks, 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, by enirox001 in #36472, and by cyb3ralbert and xyzconstant here (hasBlocks). The findAncestorByHash fix is the same one previously proposed by Romansage6 and Yudis-bit in #36473, #36484 and #36490.

Fixes #36472.

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
Stale ACK	xyzconstant, enirox001

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

Conflicts

No conflicts as of last run.

LLM Linter (✨ experimental)

Possible places where named args for integral literals may be used (e.g. func(x, /*named_arg=*/0) in C++, and func(x, named_arg=0) in Python):

src/test/interfaces_tests.cpp: chain->hasBlocks(tip->GetBlockHash(), 0, -1)

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::runtime_error) → Prefer BOOST_CHECK_EXCEPTION with a predicate checking the expected error message.
src/test/interfaces_tests.cpp: BOOST_CHECK_THROW(chain->getBlockHash(-1), std::runtime_error) → Prefer BOOST_CHECK_EXCEPTION with a predicate checking the expected error message.

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"

ryanofsky commented at 5:48 PM on November 5, 2026:

re: #36520 (review)

Thanks, agree it should say an IPC client was involved. Changed to "IPC client failed to handle %s notification: %s" in the latest push, since the exception also covers a client that received the call and returned an error.

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.

ryanofsky commented at 5:48 PM on November 5, 2026:

re: #36520 (review)

Thanks, took this in the latest push: the doc comment is now close to your version, and the sentence about why in-process exceptions are not caught moved next to the catch.

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

in src/test/interfaces_tests.cpp:226 in b67d871f95

 221 | +    };
 222 | +    auto notifications{std::make_shared<ThrowingNotifications>()};
 223 | +    auto handler{m_node.chain->handleNotifications(notifications)};
 224 | +    for (int i = 0; i < 2; ++i) {
 225 | +        CreateAndProcessBlock({}, CScript() << OP_TRUE);
 226 | +        m_node.validation_signals->SyncWithValidationInterfaceQueue();

enirox001 commented at 1:25 PM on October 30, 2026:

In commit : interfaces: Catch IPC exceptions thrown by Chain notification handlers

We can remove the redundant queue synchronization in throwingNotifications as CreateAndProcessBlock already synchronizes notifications by default

ryanofsky commented at 5:48 PM on November 5, 2026:

re: #36520 (review)

Thanks, removed in the latest push.

enirox001 commented at 3:24 PM on October 30, 2026: contributor

tACK b67d871f95

The fixes in these commits do resolve the problems reported in #36472 and during review of #29409. I applied the changes and confirmed the problem in #36472 is resolved in a functional test.

I think the negative max_height case in hasBlocks and the findAncestorByHash issue look related, since both rely on assumptions about lookup results. hasBlocks can dereference nullptr after GetAncestor receives a negative height, while findAncestorByHash could return true when the starting block was unknown.

I wonder if there is a shared way to handle invalid inputs and failed lookups across the exposed Chain interface in Core, so other methods don't repeat the same mistakes? Probably something for a followup.

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).

ff8ff34d12

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::runtime_error instead of asserting when the
  height is not in the active chain. libmultiprocess sends the exception
  to the IPC client as an error. In-process, the wallet's loadwallet and
  startup error handling catch std::runtime_error, so they also handle a
  reorg that makes the height invalid between getHeight() and
  getBlockHash().
- hasBlocks returns false for a negative max_height instead of
  dereferencing the null block that GetAncestor() returns for it.
- 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,
and the hasBlocks case by cyb3ralbert and xyzconstant.

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

6a59db4cf6

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).

e929e16f98

in src/node/interfaces.cpp:601 in b67d871f95 outdated

 594 | @@ -563,7 +595,9 @@ class ChainImpl : public Chain
 595 |      uint256 getBlockHash(int height) override
 596 |      {
 597 |          LOCK(::cs_main);
 598 | -        return Assert(chainman().ActiveChain()[height])->GetBlockHash();
 599 | +        const CBlockIndex* block{chainman().ActiveChain()[height]};
 600 | +        if (!block) throw std::out_of_range(strprintf("Block height %d is not in the active chain", height));
 601 | +        return block->GetBlockHash();
 602 |      }

ryanofsky commented at 5:48 PM on November 5, 2026:

re: cyb3ralbert #36520 (comment)

In commit "interfaces: Return errors instead of crashing on invalid Chain arguments" (42fcc504e21b6a7c38e696f7a5e4b7b808d624bd)

Good point, out_of_range wasn't chosen for any reason that outweighs this. The latest push throws std::runtime_error instead, so the wallet's existing loadwallet and startup error handling applies if a reorg makes the height invalid between getHeight() and getBlockHash(). IPC clients see the same error message either way.

in src/test/interfaces_tests.cpp:223 in b67d871f95

 218 | +            ++m_tips;
 219 | +            throw ipc::Exception("updatedBlockTip error");
 220 | +        }
 221 | +    };
 222 | +    auto notifications{std::make_shared<ThrowingNotifications>()};
 223 | +    auto handler{m_node.chain->handleNotifications(notifications)};

ryanofsky commented at 5:48 PM on November 5, 2026:

re: cyb3ralbert #36520 (comment)

In commit "interfaces: Catch IPC exceptions thrown by Chain notification handlers" (b67d871f95fa1b9c47fc2f8d08d80e27f0a81e16)

Thanks for the very thorough testing, especially the per-commit reverts and the client-killed-during-a-call case. Agree on the coverage gap: the latest push makes all six Notifications methods throw in this test, so removing any of the Notify wrappers makes it fail.

in src/test/interfaces_tests.cpp:184 in b67d871f95 outdated

 175 | @@ -163,4 +176,58 @@ BOOST_FIXTURE_TEST_CASE(hasBlocks, TestChain100Setup)
 176 |      BOOST_CHECK(!chain->hasBlocks(active.Tip()->GetBlockHash(), 6, 50));
 177 |  }
 178 |  
 179 | +BOOST_FIXTURE_TEST_CASE(invalidArguments, TestChain100Setup)
 180 | +{
 181 | +    // Chain methods can be called over IPC by clients that don't enforce C++
 182 | +    // preconditions, so they should return errors instead of crashing when
 183 | +    // passed invalid arguments.
 184 | +    auto& chain = m_node.chain;

ryanofsky commented at 5:48 PM on November 5, 2026:

re: enirox001 #36520#pullrequestreview-5712899549

In commit "interfaces: Return errors instead of crashing on invalid Chain arguments" (42fcc504e21b6a7c38e696f7a5e4b7b808d624bd)

Thanks for testing. Agree a shared approach would be better than fixing methods one at a time as reports come in, and that it fits a follow-up. A fuzz target that calls Chain methods over IPC with arbitrary arguments might be the most direct way to find the remaining cases.

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

ryanofsky commented at 5:49 PM on November 5, 2026:

re: cyb3ralbert #36520 (comment)

In commit "interfaces: Return errors instead of crashing on invalid Chain arguments" (42fcc504e21b6a7c38e696f7a5e4b7b808d624bd)

Thanks for finding this, and @xyzconstant for checking the other Chain methods and confirming it is the only remaining one. Since it is the same kind of bug as the others here, the latest push fixes it in this PR: hasBlocks now returns false when max_height is negative instead of dereferencing null, and invalidArguments has xyzconstant's test case.

ryanofsky force-pushed on Nov 5, 2026

ryanofsky commented at 5:52 PM on November 5, 2026: contributor

Thanks for the reviews!

Rebased b67d871f95fa1b9c47fc2f8d08d80e27f0a81e16 -> e929e16f98432de82189670112c92218ececa8ac (pr/chainsafe.1 -> pr/chainsafe.2, compare) to fix the hasBlocks crash with a negative max_height (https://github.com/bitcoin/bitcoin/pull/36520#issuecomment-6337747539, #36520#pullrequestreview-5621447619), throw std::runtime_error from getBlockHash so the wallet's existing error handling applies, make the notification test throw from all six Notifications methods, and take the comment and test suggestions from xyzconstant and enirox001