Bitcoin crash when -ipcconnect and -disablewallet are used together

[ryanofsky]

Originally posted by @jimhashhq in bitcoin/bitcoin#19461 (comment)

As promised in #10102 comments, I tried out -ipcconnect using rebased pr-19641 (thank you); everything seemed to work consistent with my new understanding of multiprocess interactions (thanks again). I did notice one issue:

1. If bitcoin-node is run with -disablewallet subsequent start of a bitcoin-gui with-ipcconnect=auto, but without -disablewallet will crash bitcoin-node. This is obviously an operator error, but may be worth handling w/o node crash. Error is:

23329 Segmentation fault (core dumped) bitcoin-gui -conf=regtest-guisolo.conf -ipcconnect=auto -debuglogfile=bitcoin-gui-date +"%Y-%m-%d".log 2>&1

Note: bitcoin-guiand bitcoin-node work just fine if bitcoin-gui correctly started with -disablewallet.
Of interest, bitcoin-cli wallet-related RPC commands run against a bitcoin-node with -disablewallet (again, an operator error) correctly just return a Method not found RPC error and do not cause node crash.

Please reference -disablewallet notes at docs/developer-notes.md#wallet

Interestingly, a prior revision to docs/developer-notes.md#wallet expands on this and references an obsolete/superceded test:

   - *Rationale*: In RPC code that conditionally use the wallet (such as
     `validateaddress`) it is easy to forget that global pointer `pwalletMain`
     can be NULL. See `qa/rpc-tests/disablewallet.py` for functional tests
     exercising the API with `-disablewallet`

I did not yet try and debug the crash as I'm not totally sure I built from the correct integration branch for this?
Hopefully some of this is useful, thank you.
See also: #19460

[ryanofsky]

This is not technically a libmultiprocess bug but a bitcoin IPC issue exposed by bitcoin/bitcoin#19461. Creating a separate issue to debug and track.

I think ideal fix in long run would be for bitcoin-wallet processes to handle wallet RPC without relying on bitcoin-node, and for bitcoin-gui to communicate with bitcoin-wallet processes directly without going through bitcoin-node. At that point, the bitcoin-node -disablewallet option could be dropped.

Before that point though, the segfault should be avoided or replaced with a clean error message.

I could reproduce easily by building bitcoin/bitcoin#19461 and running:

gdb -ex run --args build/bin/bitcoin-node -regtest -debug=ipc -disablewallet -ipcbind=unix
gdb -ex run --args build/bin/bitcoin-gui -regtest -debug=ipc -printtoconsole -ipcconnect=unix

in two terminals. Doing that, the bitcoin-gui process appears to hang, while the bitcoin-node process has an assertion failure node/interfaces.cpp:377 walletLoader: Assertion m_context->wallet_loader' failed.which gdb shows happening in anode::(anonymous namespace)::NodeImpl::walletLoader` call from an external thread created by the GUI.

A reasonable fix could be to change the interfaces::Node::walletLoader() method to return a pointer rather than a reference and return a null value instead of crashing, and the GUI could handle the null value. Or it could throw an exception which the GUI could catch.

Alternately on startup the GUI could check if the node was started with -disablewallet and avoid calling the walletLoader method if that is the case. If this is done, the walletLoader IPC method should also be changed to throw an exception in stead of asserting so the IPC call does not crash the node.