IPC stability bugs

Bug 13

GUI address book test aborts after two separately merged PRs combine

Status
Fixed
Symptom
Assertion failure; worst effect seen: assertion failure
Where seen
Bitcoin Core unit tests
Trigger
Test setup code
First reported
2021-09-16 (first report)
Fixed
2021-09-16
Fix
bitcoin#22992

Summary

test_bitcoin-qt aborted deterministically in AddressBookTests on an Assert of a null WalletClient after bitcoin#22219 ("multiprocess: Start using init makeNode, makeChain, etc methods"), a refactor for the multiprocess split, merged. The defect was in test code (the address book test fixture never created a wallet client) and did not affect users. Fixed the same day by bitcoin#22992 ("Fix Qt test broken by #22219").

What happened

@fanquake reported in a comment on bitcoin#22219, 15 minutes after it merged on 2021-09-16, that master CI was broken: test_bitcoin-qt failed in AddressBookTests with Assertion `"m_context->wallet_client" && check' failed in NodeImpl::walletClient(), exit status 134.

Root cause

NodeImpl::walletClient() in src/node/interfaces.cpp does Assert(m_context->wallet_client). Before bitcoin#22219, BasicTestingSetup in src/test/util/setup_common.cpp called g_wallet_init_interface.Construct(m_node), which created a WalletClient for every test fixture. bitcoin#22219, in commit e4709c7b5661, changed WalletInit::Construct to build the client through node.init->makeWalletClient() (the Init interface, which in a multiprocess build can create the client in another process), removed that call from the fixture, and added an explicit MakeWalletClient call to src/qt/test/wallettests.cpp, but not to addressbooktests.cpp.

Separately, bitcoin#19101 ("refactor: remove ::vpwallets and related global variables", merged 2021-08-19) had changed TestAddAddressesToSendBook to get its WalletContext from node.walletClient(). With both merged, the address book test ran with m_node.wallet_client never set, and the Assert aborted the test. The fix's PR description names bitcoin#19101 as the likely conflicting PR, which matches the code: each PR passed CI on its own, and only the combination failed.

The code

Quoted from 865ee1af2012^. TestAddAddressesToSendBook in src/qt/test/addressbooktests.cpp sets up a node context but never gives it a wallet client, then asks the node for one:

void TestAddAddressesToSendBook(interfaces::Node& node)
{
    TestChain100Setup test;
    node.setContext(&test.m_node);
    // ...
    WalletContext& context = *node.walletClient().context();

NodeImpl::walletClient() in src/node/interfaces.cpp asserts that the context has one:

WalletClient& walletClient() override
{
    return *Assert(m_context->wallet_client);
}

Until bitcoin#22219 the test fixture filled in m_node.wallet_client for every test. After it, only src/qt/test/wallettests.cpp created one explicitly, so test.m_node.wallet_client stayed null here and the Assert aborted test_bitcoin-qt.

The fix creates the client in the address book test the same way wallettests.cpp already did (865ee1af2012):

@@ -60,6 +60,8 @@ void EditAddressAndSubmit(
 void TestAddAddressesToSendBook(interfaces::Node& node)
 {
     TestChain100Setup test;
+    auto wallet_client = interfaces::MakeWalletClient(*test.m_node.chain, *Assert(test.m_node.args));
+    test.m_node.wallet_client = wallet_client.get();
     node.setContext(&test.m_node);

With m_node.wallet_client set before node.setContext(), walletClient() returns the client instead of asserting, and wallet_client stays alive for the rest of the test.

Fix

Commit 865ee1af2012 adds the same MakeWalletClient call to addressbooktests.cpp and stores the result in m_node.wallet_client, matching wallettests.cpp (+2). The PR description calls the failure a "null WalletClient pointer dereference"; the CI log shows the Assert catching the null pointer and aborting before any dereference.

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 other
  • Earliest detector a unit test (existed then)
  • Diagnosis easy
  • Fix design easy
  • Debugging aids a clear assertion message
  • Rust no

Would Rust have prevented it? No. The missing wallet client is a half-initialized context produced by merging two correct PRs; an Option field would still be None and unwrap would panic exactly where the Assert fires. More on Rust

Could it have been prevented?

Only by process. Each PR was correct against the master it was tested on: bitcoin#19101 made the address book test ask the node for its wallet client, and bitcoin#22219 stopped the test fixture from creating one and added an explicit MakeWalletClient to the one Qt test that needed it at the time. Git merged them without a textual conflict because they touched different files. The prevention is a merge that re-runs CI on the combined tree before landing (a merge queue), or a rebase-and-retest rule for PRs older than a few weeks; neither existed in Bitcoin Core in 2021. Within the code, a fixture that still supplied a default wallet client would have hidden the gap, but removing that default was the point of the refactor.

Could it have been found sooner?

It was found as soon as it could be: test_bitcoin-qt already had the failing test, and fanquake reported the red master CI fifteen minutes after the merge, with the fix merged three hours later. The only earlier moment was a CI run on the merged result before the merge.

What made it hard to debug?

Nothing. Assert(m_context->wallet_client) in NodeImpl::walletClient() turned a null dereference into a message naming the missing pointer, and the fix is the two lines wallettests.cpp already had. The Assert is the useful part: without it the failure would have been a segfault in a Qt test with a less obvious cause.

Lesson

Two correct PRs can combine into a broken tree without a textual conflict; test the merged result, not the branches, and put an assert with a message at the point where a missing dependency is first used so the combination fails loudly.

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 unit-test
Scope test-code
Trigger Test setup (test-fixture)
Phase startup
Side n/a
Component test-harness
Root cause kind uninitialized
Fix kind test-only
Detection ci-deterministic
Needs a peer that is not libmultiprocess C++ no
Needed a sanitizer to see no
Confidence high