multiprocess: Add bitcoin-gui -ipcconnect option #19461

ryanofsky · 2020-07-07T19:00:11Z

This is a draft PR because it is based on #29409 + #10102 + #19460. The non-base commits are:

dbced82bbafd multiprocess: Add bitcoin-gui -ipcconnect option

Building on #10102, this adds an -ipcconnect option to bitcoin-gui that connects the GUI to an existing bitcoin-node process already running in the background instead of spawning a new bitcoin-node process. This allows the GUI to be started and stopped independently of the node. By default with this change, bitcoin-gui will check if a <datadir>/sockets/node.sock socket exists and try to connect to that. If that doesn't work, it will spawn a new node process and start up the same way it did before this PR.

The default bitcoin-gui connect option is -ipcconnect=auto, which tries to connect if possible as described above, and spawns a new bitcoin-node process if not possible. Other supported options are -noipcconnect to never connect to an existing node and always spawn a new one, -ipcconnect to require a connection and fail if it can't be established, and -ipcconnect=unix:<socket> to require a connection and use a custom socket path.

With this PR, basic functionality works and gui instances can connect and disconnect from a running node. But there are rough edges: If a gui process doesn't shut down cleanly, the node can see unhandled IpcExceptions, and if node command line options are passed to bitcoin-gui and bitcoin-gui connects to an exiting bitcoin-node process instead of spawning a new one, the node options will be silently ignored.

These changes require multiprocess support and this PR has no effect unless bitcoin is configured with --enable-multiprocess as described in doc/multiprocess.md

This PR is part of the process separation project.

DrahtBot · 2020-07-07T21:06:18Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/19461.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	laanwj, meshcollider

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#32176 (net: Prevent accidental circuit sharing when using Tor stream isolation by laanwj)
#31895 (doc: Improve dependencies.md by NicolaLS)
#31672 (rpc: add cpu_load to getpeerinfo by vasild)
#31410 (qa: Fix wallet_multiwallet.py by hebasto)
#30997 (build: Switch to Qt 6 by hebasto)
#29700 (kernel, refactor: return error status on all fatal errors by ryanofsky)
#27052 (test: rpc: add last block announcement time to getpeerinfo result by LarryRuane)
#26022 (Add util::ResultPtr class by ryanofsky)
#25665 (refactor: Add util::Result failure values, multiple error and warning messages by ryanofsky)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

Needed because BlockConnected notifications are a lot slower with the wallet running in separate process.

Make default constructor more generic so it doesn't only work with void types.

Spawn node subprocess instead of running node code internally

Spawn wallet subprocess instead of running wallet code internally

Add .wallet/.gui suffixes to log files created by bitcoin-gui and bitcoin-wallet processes so they don't clash with bitcoin-node log file.

Add `-ipcconnect` option to `bitcoin-wallet` to allow connecting to a bitcoin node process over IPC. The `bitcoin-wallet` tool doesn't really do anything with its connection to the node yet, but it could potentially run or serve RPCs that require being online. Example usage: src/bitcoin-node -regtest -debug -ipcbind=unix src/bitcoin-wallet -regtest -ipcconnect=unix info

Add `-ipcconnect` option to `bitcoin-gui` to allow connecting gui to an existing node instead of starting a new node process, so it is possible to start and stop the gui independently of the node. This change doesn't add an -ipcbind option to the bitcoin-wallet (which would allow the gui to control wallet processes without going through the node), but this would be a logical extension to add in the future.

jimhashhq · 2025-03-27T00:25:02Z

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:

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

DrahtBot · 2025-04-02T14:25:56Z

🐙 This pull request conflicts with the target branch and needs rebase.

ryanofsky · 2025-04-07T15:58:09Z

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.

Thanks for the report. I created bitcoin-core/libmultiprocess#169 to track it. It sounds like you are using the correct branch, and as long as you are using any branch containing the changes in this PR, following those steps should not crash and the fact that they do is a bug. I'm trying to reproduce the crash now and will add a stack trace to bitcoin-core/libmultiprocess#169 if I can. I'd expect fixing this to be pretty simple after that.

jimhashhq · 2025-04-08T14:36:16Z

I created bitcoin-core/libmultiprocess#169 to track it.

Thank you; glad to hear you were able to easily reproduce. I did not report an issue myself only because:

Didn't want to sound alarmist; this is isolated to the experimental multiprocess feature.
Calling this just an issue might avoid addressing deeper process control hierarchy challenges,
prioritizing a temporary implementation over longer term design (below).
I'm not currently qualified to opine on any potential pitfalls of mixing/matching legacy Boost
threading/synchronization with KJ cooperative multitasking, coroutines, etc.

Comment by @ryanofsky bitcoin-core/libmultiprocess pr-169:
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.

Saw one other statement to this effect, but can't place it RN, will reference here if I can.

I also would also be interested in seeing multiprocess achieve the goal of not having the node act as a go-between for wallet RPCs, and might be personally interested on working on any formal software "interface rationalization" spec to that end, if one doesn't exist already.

DrahtBot · 2025-07-06T01:18:50Z

⌛ There hasn't been much activity lately and the patch still needs rebase. What is the status here?

Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
Is it no longer relevant? ➡️ Please close.
Did the author lose interest or time to work on this? ➡️ Please close it and mark it 'Up for grabs' with the label, so that it can be picked up in the future.

DrahtBot added Build system Docs GUI P2P RPC/REST/ZMQ Tests Utils/log/libs Wallet labels Jul 7, 2020

ryanofsky removed the Needs rebase label Mar 19, 2025

ryanofsky and others added 16 commits March 21, 2025 18:40

Merge remote-tracking branch 'origin/pull/29409/head'

b5cca2e

test: Increase feature_block.py and feature_taproot.py timeouts

d91a9d8

Needed because BlockConnected notifications are a lot slower with the wallet running in separate process.

test: Fix multiprocess test for unclean shutdown on kill

548540c

util: Add util::Result workaround to be compatible with libmultiprocess

883659e

Make default constructor more generic so it doesn't only work with void types.

multiprocess: Add capnp serialization code for bitcoin types

ccd0a20

multiprocess: Add capnp wrapper for Wallet interface

d903a6c

multiprocess: Add capnp wrapper for Node interface

7b906e9

multiprocess: Make bitcoin-gui spawn a bitcoin-node process

7244a61

Spawn node subprocess instead of running node code internally

multiprocess: Make bitcoin-node spawn a bitcoin-wallet process

f4fe8c3

Spawn wallet subprocess instead of running wallet code internally

multiprocess: Add debug.log .wallet/.gui suffixes

a504fcc

Add .wallet/.gui suffixes to log files created by bitcoin-gui and bitcoin-wallet processes so they don't clash with bitcoin-node log file.

doc: Multiprocess misc doc and comment updates

bbcbe85

combine_logs: Handle multiprocess wallet log files

c9a4963

Merge remote-tracking branch 'origin/pull/10102/head'

ec1b264

Merge remote-tracking branch 'origin/pull/19460/head'

cb7b512

ryanofsky force-pushed the pr/ipc-gui branch from 9706a5c to dbced82 Compare March 25, 2025 18:48

DrahtBot removed the CI failed label Mar 25, 2025

DrahtBot mentioned this pull request Mar 26, 2025

wallet, rpc: remove settxfee and paytxfee #32138

Open

DrahtBot mentioned this pull request Mar 31, 2025

net: Prevent accidental circuit sharing when using Tor stream isolation #32176

Merged

DrahtBot added the Needs rebase label Apr 2, 2025

ryanofsky mentioned this pull request Apr 7, 2025

Bitcoin crash when -ipcconnect and -disablewallet are used together bitcoin-core/libmultiprocess#169

Open

jimhashhq mentioned this pull request Apr 9, 2025

RFC: Adding bitcoin-{node,gui} binaries for IPC in 30.0 release #31756

Open

ryanofsky mentioned this pull request Apr 10, 2025

Bitcoin multiprocess questions & concerns bitcoin-core/libmultiprocess#170

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

multiprocess: Add bitcoin-gui -ipcconnect option #19461

multiprocess: Add bitcoin-gui -ipcconnect option #19461

Uh oh!

ryanofsky commented Jul 7, 2020 •

edited

Loading

Uh oh!

DrahtBot commented Jul 7, 2020 •

edited

Loading

Uh oh!

jimhashhq commented Mar 27, 2025 •

edited

Loading

Uh oh!

DrahtBot commented Apr 2, 2025

Uh oh!

ryanofsky commented Apr 7, 2025

Uh oh!

jimhashhq commented Apr 8, 2025

Uh oh!

DrahtBot commented Jul 6, 2025

Uh oh!

Uh oh!

multiprocess: Add bitcoin-gui -ipcconnect option #19461

Are you sure you want to change the base?

multiprocess: Add bitcoin-gui -ipcconnect option #19461

Uh oh!

Conversation

ryanofsky commented Jul 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Jul 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage & Benchmarks

Reviews

Conflicts

Uh oh!

jimhashhq commented Mar 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Apr 2, 2025

Uh oh!

ryanofsky commented Apr 7, 2025

Uh oh!

jimhashhq commented Apr 8, 2025

Uh oh!

DrahtBot commented Jul 6, 2025

Uh oh!

Uh oh!

ryanofsky commented Jul 7, 2020 •

edited

Loading

DrahtBot commented Jul 7, 2020 •

edited

Loading

jimhashhq commented Mar 27, 2025 •

edited

Loading