IPC stability bugs

Bug 42

Mining client that leaves block options unset crashes the node

Status
Fixed
Symptom
Assertion failure; worst effect seen: assertion failure
Where seen
Bitcoin Core functional tests
Trigger
A client that does not use libmultiprocess, such as a Python or Rust client
First reported
2025-08-19 (first report)
Fixed
2025-08-21
Fix
bitcoin#33222
In released binaries
No; the code was in source releases only
Defect ever on master
Yes

Summary

bitcoin-node aborted on an Assert when an IPC client called Mining.createNewBlock with BlockCreateOptions fields out of range, including a client that left the fields at their Cap'n Proto zero defaults. Any non-C++ client (Python, Rust) that did not set every field could crash the node. Fixed by bitcoin#33222 ("miner: clamp options instead of asserting"), which clamps the values instead, before v30.0 shipped bitcoin-node.

What happened

@sipa reported in the bitcoin#33222 description that "calling Mining::createNewBlock with a default-constructed BlockCreateOptions will right now instantly crash the bitcoin node", and called it "unergonomical to have the node crash while developing against the interface". No CI or user crash was posted.

A default-constructed C++ BlockCreateOptions has valid values, so the crashing struct was the Cap'n Proto one with every field zero, which is what pycapnp's BlockCreateOptions() produces. sipa had opened bitcoin#33201 ("Add functional test for IPC interface"), the first Python (pycapnp) IPC functional test, four days earlier, and its final version sets all three option fields explicitly. The crash was most likely hit while writing that test; the PR does not say so directly.

Root cause

ClampOptions() in src/node/miner.cpp checked client-supplied options with asserts:

These held for every in-process caller, which built the options from validated -blockreservedweight settings, and for C++ IPC clients, whose BlockCreateOptions initializers (src/node/types.h) fill every field with valid defaults. Over IPC the struct comes from the client, and the BlockCreateOptions fields in mining.capnp have no defaults, so a client that leaves them unset sends blockReservedWeight = 0. That is below the minimum, and the lower-bound Assert aborts the node.

The code

Quoted from 7392b8b084be^. The BlockAssembler constructor passes the caller's options through ClampOptions in src/node/miner.cpp, which asserts that they are in range:

static BlockAssembler::Options ClampOptions(BlockAssembler::Options options)
{
    Assert(options.block_reserved_weight <= MAX_BLOCK_WEIGHT);
    Assert(options.block_reserved_weight >= MINIMUM_BLOCK_RESERVED_WEIGHT);
    Assert(options.coinbase_output_max_additional_sigops <= MAX_BLOCK_SIGOPS_COST);

In C++ the fields start from valid defaults (src/node/types.h):

size_t block_reserved_weight{DEFAULT_BLOCK_RESERVED_WEIGHT};
// ...
size_t coinbase_output_max_additional_sigops{400};

but over IPC the values come from the client, and the schema in src/ipc/capnp/mining.capnp gives the fields no defaults:

struct BlockCreateOptions $Proxy.wrap("node::BlockCreateOptions") {
    useMempool @0 :Bool $Proxy.name("use_mempool");
    blockReservedWeight @1 :UInt64 $Proxy.name("block_reserved_weight");
    coinbaseOutputMaxAdditionalSigops @2 :UInt64 $Proxy.name("coinbase_output_max_additional_sigops");
}

A Python or Rust client that builds BlockCreateOptions without setting blockReservedWeight sends 0, which is below MINIMUM_BLOCK_RESERVED_WEIGHT. Assert is checked in release builds too, so createNewBlock aborts bitcoin-node. An out-of-range value set on purpose does the same.

The fix clamps both fields into range instead (7392b8b084be):

@@ -77,9 +77,8 @@ void RegenerateCommitments(CBlock& block, ChainstateManager& chainman)
 static BlockAssembler::Options ClampOptions(BlockAssembler::Options options)
 {
-    Assert(options.block_reserved_weight <= MAX_BLOCK_WEIGHT);
-    Assert(options.block_reserved_weight >= MINIMUM_BLOCK_RESERVED_WEIGHT);
-    Assert(options.coinbase_output_max_additional_sigops <= MAX_BLOCK_SIGOPS_COST);
+    options.block_reserved_weight = std::clamp<size_t>(options.block_reserved_weight, MINIMUM_BLOCK_RESERVED_WEIGHT, MAX_BLOCK_WEIGHT);
+    options.coinbase_output_max_additional_sigops = std::clamp<size_t>(options.coinbase_output_max_additional_sigops, 0, MAX_BLOCK_SIGOPS_COST);

Any value a client sends is now accepted and moved to the nearest valid one, so client input can no longer reach an assertion; an unset reserved weight becomes the minimum rather than the default.

Fix

Commit 7392b8b084be replaces the asserts with std::clamp on both fields (+2/-3). Reviewers (ryanofsky, @Sjors, @stickies-v) would have preferred returning an error to the caller and discussed setting sentinel defaults in the schema (ryanofsky's comment, Sjors's comment); clamping was accepted as "much better than crashing".

Timeline

Lessons

What would have prevented this bug, found it sooner, or made it easier to debug. The patterns page groups these lessons across all bugs.

Pattern An in-process interface trusting what now comes from another process: An assertion on what is now input from a peer

  • Prevented by a design rule, a review checklist item
  • Earliest detector a test with a non-C++ client (did not exist then)
  • Diagnosis easy
  • Rust no

Would Rust have prevented it? No. An assert! on a field received from the wire panics in Rust just as Assert aborts in C++; treating peer input as already validated is a contract mistake, not a memory one. More on Rust

Could it have been prevented?

Yes. ClampOptions() asserted ranges on BlockCreateOptions that were preconditions when every caller was in-process C++ building the struct from validated settings. The moment bitcoin#30510 put Mining over IPC (2024-09-25) those same fields became untrusted input from a peer that may not even share the C++ defaults, and an Assert on untrusted input is a remote abort. The fix is the rule: at an interface that can be reached over the wire, argument checks are validation (clamp or return an error), never assertions. This was knowable when the wrapper was added, and the review checklist item is mechanical: for each method exposed over IPC, list every Assert/Assume reachable from its arguments. The reviewers' preferred shape, returning an error or giving the schema sentinel defaults, was not taken; clamping silently changes an unset weight to the minimum rather than the C++ default, which is the residue of the choice.

Could it have been found sooner?

Yes, by any client that does not share the C++ headers. The C++ client's struct initializers filled every field, so the C++ functional tests could not send a zero, and no Python or Rust client existed against master until sipa's pycapnp test (bitcoin#33201, 2025-08-15) found it within days, 397 days after the first assert and 190 after the lower-bound one that a zeroed struct trips. A fuzzer at the IPC boundary (fuzzamoto found the malformed-argument sibling, record 57, a month later) would also have hit it on its first zero.

What made it hard to debug?

Nothing. The assert is deterministic and names the failed expression; the PR that reported it was the fix, two days from open to merge. The only work was deciding what to do instead of asserting, and that discussion (error vs clamp vs schema defaults) is the part still worth having.

Lesson

When an in-process interface is exposed to another process, every precondition on its arguments turns into input validation; audit the asserts on the day the wrapper lands, and test with a client that does not share your headers, because your own client will always send what you expect.

Notes

Classification

The survey's classification of this bug. The explorer charts these fields across all bugs; the method page explains them.

Field Value
Symptom assert
Worst effect observed assert
Seen in functional-test
Scope production
Trigger Non-C++ client (non-cpp-client)
Phase steady-state
Side server
Component bitcoin-mining
Root cause kind assumption-violated
Fix kind one-line-tweak
Detection new-test
Needs a peer that is not libmultiprocess C++ yes
Needed a sanitizer to see no
Defect ever on master yes
Released source
Confidence high