IPC stability bugs

Bug 26

Node started with -disablewallet aborts when a GUI with wallets enabled connects

Status
Open
Symptom
Assertion failure; worst effect seen: assertion failure
Where seen
Branch-build
Trigger
GUI startup
First reported
2025-03-27 (first report)
Fixed
Not yet
In released binaries
Yes
Defect ever on master
Yes

Summary

A bitcoin-node started with -disablewallet aborts on an Assert in NodeImpl::walletLoader when a bitcoin-gui started without -disablewallet connects to it over IPC, and the GUI hangs. The GUI asks the node for its wallet loader, and the node asserts that it has one. That was safe when the GUI and node ran in one process with one set of options, but over IPC the two processes can be configured differently. The trigger needs bitcoin-gui -ipcconnect, which only exists in the unmerged bitcoin#19461, so no release is affected, but the asserting line is still on master. No fix yet.

What happened

Root cause

interfaces::Node::walletLoader() (src/interfaces/node.h, implemented in src/node/interfaces.cpp) returns a reference and asserts that NodeContext::wallet_loader exists:

WalletLoader& walletLoader() override { return *Assert(m_context->wallet_loader); }

In-process this is safe: bitcoin-qt decides whether to use the wallet from the same arguments that decide whether the node creates a WalletLoader, so caller and callee cannot disagree. Over IPC the GUI is a separate process with its own arguments. A GUI that was not given -disablewallet calls walletLoader on a node that has no loader, and the Assert turns a configuration mismatch into a node crash. By comparison, bitcoin-cli wallet RPCs against the same node just return "Method not found".

The assertion predates IPC. bitcoin#19461 builds on bitcoin#19460 ("multiprocess: Add bitcoin-wallet -ipcconnect option"), which also has bitcoin-node listen on a socket with -ipcbind. It is the first code that lets a GUI process connect to a node process it did not spawn, so it is the first place the two can be configured differently.

Fix

No fix yet. In mp#169 ryanofsky proposed either changing walletLoader() to return a pointer (or throw) so the GUI can handle a missing loader, or having the GUI check the node's -disablewallet setting first while still making the IPC method throw instead of asserting. He described the longer-term fix as having bitcoin-gui talk to bitcoin-wallet processes directly, after which bitcoin-node -disablewallet could be dropped.

Next steps

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 different API shape
  • Earliest detector code review (did not exist then)
  • Diagnosis easy
  • Fix design easy
  • Debugging aids a clear assertion message
  • Rust no

Would Rust have prevented it? No. Asserting that the wallet loader exists is an API contract choice; a Rust walletLoader() returning Option would push the caller to handle absence, but an expect() there is just as idiomatic and would panic the same way. More on Rust

Could it have been prevented?

Yes. Node::walletLoader() returns a reference and asserts that the node has a WalletLoader, which in-process is a true precondition: bitcoin-qt decides whether to ask for the wallet from the same arguments that decide whether the node builds one. Once Node is served over IPC to a process with its own arguments, "the node has a wallet loader" is a fact about the peer, and an Assert on a fact about the peer is a remote crash. The rule is the one the bitcoin#33222 case also produced: at an interface that can be reached from another process, preconditions on arguments or on the callee's configuration become validation, returning null or throwing. The API shape ryanofsky proposed, a pointer return or a thrown error the GUI can handle, is what an interface designed for a remote caller would have had from the start. This was knowable when bitcoin#19461 first let a GUI connect to a node it did not spawn, since that is the first moment the two could be configured differently.

Could it have been found sooner?

Yes, by review rather than by a test: the audit "for each method served over IPC, list every Assert reachable from its arguments or from state the caller cannot see" is mechanical and finds this line without running anything. A test would need the configuration matrix (node with -disablewallet, GUI without), which no existing test generates; bitcoin-cli wallet RPCs against the same node already returned "Method not found", so the in-tree precedent for the right behaviour existed. The assertion was added in 2020 for an in-process interface and was correct there; nobody re-read it when the interface went over the wire, which is the usual way this pattern survives.

What made it hard to debug?

Nothing. The report gave the two command lines, ryanofsky reproduced it with -regtest and the assertion text names the file, line and the missing member. The only confusion was in the report itself, whose shell output and prose disagreed about which process died; the reproduction settled it as the node. The fix is unwritten because the branch that exposes it is unmerged, not because it is hard.

Lesson

When an interface starts serving callers from another process, every assertion about the callee's own configuration becomes an assertion about the peer's guess at it; return an error a remote caller can act on, and audit the interface's asserts the day it goes over the wire.

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 branch-build
Scope production
Trigger GUI startup (qt-startup)
Phase steady-state
Side server
Component bitcoin-ipc
Root cause kind assumption-violated
Fix kind none
Detection user-report
Needs a peer that is not libmultiprocess C++ no
Needed a sanitizer to see no
Defect ever on master yes
Released binary
Confidence high