IPC stability bugs

Bug 27

Mining client calls during node startup or early shutdown crash the node

Status
Fixed
Symptom
Crash; worst effect seen: nothing observed
Where seen
Never observed
Trigger
Node startup
First reported
2025-02-03 (first report)
Fixed
2025-04-14
Fix
bitcoin#31785
In released binaries
No; the code was in source releases only
Defect ever on master
Yes

Summary

An IPC mining client that called Mining.waitTipChanged or Mining.createNewBlock during node startup, before a chain tip was connected, or during an early shutdown, could crash bitcoin-node: waitTipChanged dereferenced a null Tip() and createNewBlock hit assert(pindexPrev != nullptr). The IPC server starts listening before init waits for the tip, so an early client could reach this window. It was found by reading the code and never seen at runtime. v29.0 source builds with IPC enabled have the bug; fixed by bitcoin#31785 ("Have createNewBlock() wait for tip, make rpc handle shutdown during long poll and wait methods") before v30.0 shipped bitcoin-node.

What happened

No CI failure or user report. @Sjors noticed the window while writing the Stratum v2 Template Provider, whose client already guarded against it by calling waitTipChanged(uint256::ZERO) before createNewBlock(), and opened bitcoin#31785 so other implementers would not have to know to do this (his comment explaining the startup window, 2025-02-03). He pointed out that the IPC server starts listening in AppInitMain before init's wait for a connected tip, so the race was possible for IPC clients. @vasild noted in his review that for RPC it was "mostly theoretical", because RPC warmup ends after the tip is set.

Root cause

MinerImpl::waitTipChanged in src/node/interfaces.cpp waited on m_tip_block_cv until TipBlock() was set and differed from current_tip, or chainman().m_interrupt was set, with a caller-supplied timeout. It then returned BlockRef{chainman().ActiveChain().Tip()->GetBlockHash(), ...} unconditionally. If the wait ended by timeout or interrupt before any tip was connected, Tip() was null and the call dereferenced it. createNewBlock had the same assumption one level down: BlockAssembler::CreateNewBlock (src/node/miner.cpp) does assert(pindexPrev != nullptr) on the chain tip. The interface was written for in-process callers, which run after init has connected a tip; over IPC a client can call at any time.

The fix's commit message says the old waitTipChanged "would return the last known tip during shutdown", which made the start-then-quick-shutdown case "ambiguous". The code shows that holds only once a tip exists: before a tip is connected, the old code dereferenced a null Tip(), as ryanofsky pointed out in review. The PR description, updated later, describes it as a crash.

The code

Quoted from 64a2795fd4fe^. MinerImpl::waitTipChanged in src/node/interfaces.cpp waits for a new tip, a timeout or an interrupt, and then reads the active tip without checking that there is one:

BlockRef waitTipChanged(uint256 current_tip, MillisecondsDouble timeout) override
{
    // ...
    {
        WAIT_LOCK(notifications().m_tip_block_mutex, lock);
        notifications().m_tip_block_cv.wait_for(lock, timeout, [&]() EXCLUSIVE_LOCKS_REQUIRED(notifications().m_tip_block_mutex) {
            // We need to wait for m_tip_block to be set AND for the value
            // to differ from the current_tip value.
            return (notifications().TipBlock() && notifications().TipBlock() != current_tip) || chainman().m_interrupt;
        });
    }
    // Must release m_tip_block_mutex before locking cs_main, to avoid deadlocks.
    LOCK(::cs_main);
    return BlockRef{chainman().ActiveChain().Tip()->GetBlockHash(), chainman().ActiveChain().Tip()->nHeight};
}

createNewBlock goes straight to BlockAssembler::CreateNewBlock in src/node/miner.cpp, which asserts on the same tip:

CBlockIndex* pindexPrev = m_chainstate.m_chain.Tip();
assert(pindexPrev != nullptr);

The IPC server starts accepting connections before init has connected a tip. A client that calls waitTipChanged in that window with a timeout, or during a shutdown that interrupts it, gets a null Tip() dereference; one that calls createNewBlock hits the assert. Either way bitcoin-node dies.

The fix waits for a tip regardless of the timeout, returns std::nullopt on shutdown, and makes createNewBlock wait the same way (64a2795fd4fe, 5315278e7c7f; comments and the RPC callers' nullopt handling are omitted):

@@ -1070,22 +1070,34 @@ public:
-    BlockRef waitTipChanged(uint256 current_tip, MillisecondsDouble timeout) override
+    std::optional<BlockRef> waitTipChanged(uint256 current_tip, MillisecondsDouble timeout) override
+            notifications().m_tip_block_cv.wait(lock, [&]() EXCLUSIVE_LOCKS_REQUIRED(notifications().m_tip_block_mutex) {
+                return notifications().TipBlock() || chainman().m_interrupt;
+            });
+            if (chainman().m_interrupt) return {};
+            notifications().m_tip_block_cv.wait_until(lock, deadline, [&]() EXCLUSIVE_LOCKS_REQUIRED(notifications().m_tip_block_mutex) {
+                return Assume(notifications().TipBlock()) != current_tip || chainman().m_interrupt;
             });
         }
-        // Must release m_tip_block_mutex before locking cs_main, to avoid deadlocks.
-        LOCK(::cs_main);
-        return BlockRef{chainman().ActiveChain().Tip()->GetBlockHash(), chainman().ActiveChain().Tip()->nHeight};
+
+        if (chainman().m_interrupt) return {};
+        return getTip();
@@ -1102,6 +1102,9 @@ public:
     std::unique_ptr<BlockTemplate> createNewBlock(const BlockCreateOptions& options) override
     {
+        if (!waitTipChanged(uint256::ZERO, MillisecondsDouble::max())) return {};

Both methods now read the tip only after TipBlock() has been set, and a shutdown before that returns an empty result instead of touching a null tip.

Fix

No test was added for the startup window.

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 Serving before ready, freeing before done: Serving calls before startup finishes

  • Prevented by a design rule, a different API shape
  • Earliest detector a test with a non-C++ client (did not exist then)
  • Diagnosis easy
  • Fix design moderate
  • Rust partly

Would Rust have prevented it? Partly. The unconnected tip would be an Option and the dereference an unwrap panic rather than undefined behaviour, but the lifecycle gap (serving IPC before the chain is loaded) is an ordering problem the type system does not see. More on Rust

Could it have been prevented?

Yes. waitTipChanged and createNewBlock assumed a connected tip because every in-process caller (RPC after warmup, the miner) runs after init has one; the IPC server starts listening in AppInitMain before that wait, so the assumption became a race the day bitcoin#30510 put Mining on the wire. Two rules would have made it hard to write: do not accept IPC connections until the node is in the state its interfaces assume (the RPC layer's warmup does exactly this, as vasild's review noted), or make every interface method valid in every lifecycle state, which is what the fix does by waiting for a tip and returning std::optional<BlockRef> on shutdown. Sjors' own client already worked around it with waitTipChanged(uint256::ZERO) first, so the gap was understood by the first serious client author; the PR moved that knowledge into the interface.

Could it have been found sooner?

Yes, by a functional test that connects an IPC client during startup and calls each Mining method, or by a fuzz or Antithesis workload that starts clients at random points in the node's life. Neither existed; the fix added no test for the window either, and the same family of startup and shutdown edges was later found by Antithesis (bitcoin#34262, a CHECK_NONFATAL on the null-on-shutdown result this fix introduced) and by a client connecting during startup in the field (the -ipcbind startup assert record). Nothing observed the null dereference itself; it was found by reading init.cpp against the interface.

What made it hard to debug?

Nothing to diagnose, but the fix was not simple: it changed the return type across the RPC long-poll and wait methods (+63/-28), chose "wait for a tip regardless of timeout" over "error before a tip", and introduced a null-on-shutdown contract for createNewBlock that three RPC call sites did not handle. Its companion Assume on negative timeouts became a bug of its own. A lifecycle decision made once at the listener (do not serve until ready) would have been smaller than per-method handling.

Lesson

Before a process accepts external callers, decide which lifecycle states each method may be called in and either gate the listener on readiness or make every method tolerate early and late calls; the in-process callers that never hit the window are not evidence it is closed.

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 crash
Worst effect observed none
Seen in never
Scope production
Trigger Node startup (node-startup)
Phase startup
Side server
Component bitcoin-mining
Root cause kind assumption-violated
Fix kind new-code-path
Detection review
Needs a peer that is not libmultiprocess C++ no
Needed a sanitizer to see no
Defect ever on master yes
Released source
Confidence high