IPC stability bugs

Bug 12

GUI aborts at startup after a refactor of node argument setup

Status
Fixed
Symptom
Assertion failure; worst effect seen: assertion failure
Where seen
The production GUI
Trigger
GUI startup
First reported
2021-06-11 (first report)
Fixed
2021-06-11
Fix
gui#361
In released binaries
No
Defect ever on master
Yes

Summary

For about 15 hours on 2021-06-11, bitcoin-qt built from master aborted at startup with Assertion '"node.args" && check' failed in WalletInit::Construct. A refactor that prepared argument setup for the multiprocess split, bitcoin#22216, moved the node.args = &gArgs assignment out of SetupServerArgs into the bitcoind and bitcoin-node startup code, and the GUI's startup function GuiMain never got one. This was not a bug in the IPC mechanism, only in a preparatory refactor, and no release was affected. Fixed the same day by a one-line change in gui#361.

What happened

@hebasto reported a failed assertion when starting bitcoin-qt on Windows 10 a few hours after bitcoin#22216 ("refactor: Make SetupServerArgs callable without NodeContext") was merged, then filed bitcoin#22227 after seeing it with -signet on Linux too. ryanofsky reproduced it with a stack trace ending in WalletInit::Construct at wallet/init.cpp:127.

Root cause

bitcoin#22216 changed SetupServerArgs in src/init.cpp to take an ArgsManager& instead of a NodeContext&. The old version had also set node.args = &gArgs. That assignment moved into the BitcoindInit and BitcoinNodeInit constructors (src/init/bitcoind.cpp, src/init/bitcoin-node.cpp) and the test fixture, but GuiMain in src/qt/bitcoin.cpp was not given one. node_context.args stayed null, and Assert(node.args) in WalletInit::Construct (src/wallet/init.cpp), which sets up the wallet component at startup, fired every time.

The code

Quoted from d7f3b1af21da^. GuiMain in src/qt/bitcoin.cpp creates its NodeContext and goes on to register arguments without ever pointing the context at them:

NodeContext node_context;
std::unique_ptr<interfaces::Node> node = interfaces::MakeNode(&node_context);
// ...
/// 2. Parse command-line options. We do this after qt in order to show an error if there are problems parsing these
// Command-line options take precedence:
SetupServerArgs(gArgs);
SetupUIArgs(gArgs);

bitcoind does it in the BitcoindInit constructor (src/init/bitcoind.cpp):

BitcoindInit(NodeContext& node) : m_node(node)
{
    m_node.args = &gArgs;

Before bitcoin#22216, SetupServerArgs took the NodeContext and set node.args itself, so every caller got it. After the refactor it takes only the ArgsManager, and node_context.args stays null in the GUI. Startup then reaches WalletInit::Construct (src/wallet/init.cpp, called from AppInitInterfaces), which aborts:

void WalletInit::Construct(NodeContext& node) const
{
    ArgsManager& args = *Assert(node.args);

The fix adds the missing assignment to GuiMain (d7f3b1af21da):

@@ -490,6 +490,7 @@ int GuiMain(int argc, char* argv[])
     /// 2. Parse command-line options. We do this after qt in order to show an error if there are problems parsing these
     // Command-line options take precedence:
+    node_context.args = &gArgs;
     SetupServerArgs(gArgs);

bitcoin-qt now sets node.args the same way bitcoind does, before anything reads it, so the Assert holds. The fix's commit title calls the failure a segfault; the code path is this assertion.

Fix

The fix commit d7f3b1af21d adds node_context.args = &gArgs; before SetupServerArgs(gArgs) in GuiMain (+1), merged as gui#361.

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 A contract changed and a caller was left behind: A function's contract changed under its callers

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

Would Rust have prevented it? Partly. A NodeContext whose args field is a non-optional reference cannot be constructed without it, so the GUI entry point would fail to compile; modelled as Option because it is set later (as the C++ pointer was), the unwrap would panic just like the Assert. More on Rust

Could it have been prevented?

Yes. SetupServerArgs had a side effect its name did not advertise, setting node.args = &gArgs, and the refactor in bitcoin#22216 removed it and moved the assignment to each caller: bitcoind, bitcoin-node, the test fixture, and not bitcoin-qt. The API shape that would have made this impossible is a NodeContext that cannot exist without its args, either taken by the constructor or not a pointer at all; a field that starts null and is filled in by convention at some point during startup will be missed by whichever entry point the author is not looking at. Short of that, the checklist is the same as for any function that stops doing something: every caller of the old function is a place that now has to do it, and GuiMain was a caller. The refactor was in service of the multiprocess split rather than IPC itself, so the lesson is about init structure, not the wire.

Could it have been found sooner?

By any automated test that starts bitcoin-qt with the wallet enabled, however small; a smoke test is the closest thing to a unit test of an entry point. CI in 2021 built the GUI and ran test_bitcoin-qt, whose fixture has its own init path and sets args itself, but nothing launched the real GuiMain; the functional tests drive bitcoind only. A smoke test that starts the GUI and exits would have failed on the PR. As it was, a developer ran a master build within twelve hours and the fix merged fifteen hours after the regression, which is the fastest turnaround in the ledger and about the floor without such a test.

What made it hard to debug?

Nothing. The assertion names the null member and the function, ryanofsky posted a stack trace the same afternoon, and the fix is one line placed where bitcoind already had it.

Lesson

When a refactor takes a side effect out of a shared function, every caller inherits the job, and the caller you did not build is the one you will miss; prefer types that cannot be constructed half-initialized, and keep one test that starts each real entry point.

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 production-gui
Scope production
Trigger GUI startup (qt-startup)
Phase startup
Side n/a
Component bitcoin-init
Root cause kind uninitialized
Fix kind one-line-tweak
Detection manual-test
Needs a peer that is not libmultiprocess C++ no
Needed a sanitizer to see no
Defect ever on master yes
Released no
Confidence high