IPC stability bugs

Bug 15

Node aborts when a separate GUI process opens confirmed transaction details

Status
Fixed
Symptom
Assertion failure; worst effect seen: assertion failure
Where seen
Branch-build
Trigger
Normal use; no special event is needed
First reported
2022-09-12 (first report)
Fixed
2023-11-13
Fix
bitcoin#28721
In released binaries
No; the code was in source releases only
Defect ever on master
Yes

Summary

In the multiprocess GUI, where bitcoin-gui runs in its own process and talks to bitcoin-node over IPC, opening the details of a confirmed transaction could kill the node process. Node::getUnspentOutput left its Coin& output parameter in the spent state when the coin was not found, and the IPC server serialized that parameter anyway, hitting assert(!IsSpent()) in Coin::Serialize. The interface contract dates from 2018 and was in releases v22 through v26, but no released binary could reach it, because the in-process GUI checks the return value; only the out-of-process GUI of the unmerged bitcoin#10102 ("Multiprocess bitcoin") branch crashed. Fixed on master in bitcoin#28721 ("multiprocess compatibility updates").

What happened

@Sjors reported on bitcoin#10102 (2022-09-09) a bitcoin-gui crash with Taproot addresses while testing the branch, caused by a different serialization problem (CTxDestination). While fixing it, ryanofsky tested sending and receiving and replied (2022-09-12) that he "fixed problems with CScript and Coin serialization that were causing similar crashes". The Coin fix is this one; its commit message says the crash "could be triggered by double clicking and viewing transaction details in the GUI transaction list". The fix was split out of bitcoin#10102 and merged to master fourteen months later in bitcoin#28721. The CScript and CTxDestination fixes stayed in the unmerged branch.

Root cause

interfaces::Node::getUnspentOutput(const COutPoint&, Coin& coin) returned bool and left coin as a default, spent Coin when the outpoint was not in the UTXO set. In-process that is harmless: TransactionDesc::toHTML (src/qt/transactiondesc.cpp) checked the return value before reading coin. Over IPC the generated proxy server, the node-side code that runs the method for the GUI and sends the results back, serializes every output parameter after the call regardless of the return value. So the untouched Coin reached Coin::Serialize (src/coins.h), which starts with assert(!IsSpent()), and bitcoin-node aborted.

toHTML looks up each input's previous output, which is spent by the transaction itself once it confirms, so the lookup fails for every confirmed transaction. The lookup only runs in the details dialog's "Debug information" section, which is shown when any debug log category is enabled (node.getLogCategories() != BCLog::NONE); multiprocess testing with -debug=ipc enables one, which is most likely why it was hit.

The fix's commit message calls the crash a segfault. The code at the time shows an assertion failure in Coin::Serialize instead; either way the node process died.

The code

Quoted from 156f49d682ef^. NodeImpl::getUnspentOutput in src/node/interfaces.cpp reports "not found" through its return value and leaves the coin output argument spent:

bool getUnspentOutput(const COutPoint& output, Coin& coin) override
{
    LOCK(::cs_main);
    return chainman().ActiveChainstate().CoinsTip().GetCoin(output, coin);
}

The GUI caller, TransactionDesc::toHTML in src/qt/transactiondesc.cpp, checks the return value before reading prev, which is enough in-process:

Coin prev;
if(node.getUnspentOutput(prevout, prev))
{

Across processes, the generated server code for Node sends every output argument back to the client after the call, whatever the method returned. For an input whose previous output is already spent, that means serializing a spent Coin, and Coin::Serialize in src/coins.h refuses:

void Serialize(Stream &s) const {
    assert(!IsSpent());

So opening the details of any confirmed transaction aborted bitcoin-node.

The fix returns std::optional<Coin> instead of filling an output argument (156f49d682ef; the matching src/interfaces/node.h and src/qt/transactiondesc.cpp changes are omitted):

@@ -328,10 +328,12 @@ public:
-    bool getUnspentOutput(const COutPoint& output, Coin& coin) override
+    std::optional<Coin> getUnspentOutput(const COutPoint& output) override
     {
         LOCK(::cs_main);
-        return chainman().ActiveChainstate().CoinsTip().GetCoin(output, coin);
+        Coin coin;
+        if (chainman().ActiveChainstate().CoinsTip().GetCoin(output, coin)) return coin;
+        return {};
     }

"Not found" is now an empty optional, so the only Coin that ever crosses the process boundary is one GetCoin returned as unspent.

Fix

Commit 156f49d682ef changes the method to std::optional<Coin> getUnspentOutput(const COutPoint&) (src/interfaces/node.h, src/node/interfaces.cpp), so "not found" is an empty optional and no spent Coin is ever serialized, and adjusts the one caller in src/qt/transactiondesc.cpp (+8/-8). bitcoin#28721 describes its changes as refactoring that does not change the behavior of current code, which is accurate for master, where no IPC wrapper for Node existed yet.

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 interface that only works inside one process

  • Prevented by a different API shape, the type system
  • Earliest detector a unit test (did not exist then)
  • Diagnosis easy
  • Fix design easy
  • Debugging aids a clear assertion message
  • Rust partly

Would Rust have prevented it? Partly. Option<Coin> is the idiomatic Rust signature because bool-plus-out-parameter is awkward to write, so the shape that carries the contract is the default; a serializer that asserts on a spent coin would still panic if that shape were bypassed. More on Rust

Could it have been prevented?

Yes. bool getUnspentOutput(const COutPoint&, Coin&) is the ordinary C++ idiom "the out-parameter is valid only if I return true", and the generated IPC server cannot know that: it serializes every output after the call, and Coin::Serialize asserts !IsSpent(). The fix (156f49d682ef) is the API shape that carries the contract in the type, std::optional<Coin>, so there is no invalid value to serialize. A rule for interfaces that will be wrapped, "every output must be serializable in every return case; use optional or a result struct instead of a bool plus out-parameter", was statable in 2018 when bitcoin#10244 defined the interface for the process split, and it is checkable by reading the signatures.

Could it have been found sooner?

Yes, by a test that calls each wrapped method over IPC with its failure inputs (here an outpoint that is not in the UTXO set). The Node wrapper existed only on the bitcoin#10102 branch, whose testing was a person clicking through bitcoin-gui, so it waited for someone with a wallet that had confirmed transactions, debug logging on (the lookup only runs in the dialog's "Debug information" section) and a reason to open the details. That was ryanofsky in September 2022, chasing the different serialization crash Sjors had reported; the CScript and CTxDestination fixes from the same session are the same class.

What made it hard to debug?

Nothing. The assert in Coin::Serialize names the invariant, and the stack shows the generated server serializing the output of getUnspentOutput. The commit message calls it a segfault, so even the exact failure was not important to the fix. The delay was organizational: the fix was authored 2022-09-12 and merged fourteen months later in bitcoin#28721, as a refactor with no behavior change on master, because master had no Node wrapper to show the bug.

Lesson

A serialization layer sends what the signature says, not what the return value means; when an in-process interface is going to be marshalled, encode "no result" in the return type rather than in a convention the caller is expected to know.

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 Normal use (ordinary-path)
Phase steady-state
Side server
Component bitcoin-ipc
Root cause kind api-misuse
Fix kind local-fix
Detection manual-test
Needs a peer that is not libmultiprocess C++ no
Needed a sanitizer to see no
Defect ever on master yes
Released source
Confidence high