Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

multiprocess: Add bitcoin-gui -ipcconnect option #19461

Draft
wants to merge 21 commits into
base: master
Choose a base branch
from

Conversation

ryanofsky
Copy link
Contributor

@ryanofsky ryanofsky commented Jul 7, 2020

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


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
Copy link
Contributor

DrahtBot commented Jul 7, 2020

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

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:

  • #bitcoin-core/gui/149 (Intro: Have user choose assumevalid by luke-jr)
  • #27711 (kernel: Remove shutdown from kernel library by TheCharlatan)
  • #27636 (kernel: Remove util/system from kernel library, interface_ui from validation. by TheCharlatan)
  • #27632 (Raise on invalid -debug and -loglevel config options by jonatack)
  • #27576 (kernel: Remove args, chainparams, chainparamsbase from kernel library by TheCharlatan)
  • #27375 (net: support unix domain sockets for -proxy and -onion by pinheadmz)
  • #26606 (wallet: Implement independent BDB parser by achow101)
  • #26596 (wallet: Migrate legacy wallets to descriptor wallets without requiring BDB by achow101)
  • #25273 (wallet: Pass through transaction locktime and preset input sequences and scripts to CreateTransaction by achow101)

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.

@DrahtBot DrahtBot removed the CI failed label May 4, 2023
@Sjors
Copy link
Member

Sjors commented May 5, 2023

When I exit the GUI and after that stop bitcoind the latter crashes.

This still happens on 5f91aa3. Same setup as above: node and wallet running on Ubuntu 23.04 with a GUI client connecting from macOS using a unix socket of SSH.

But now I've notice this happens even if you don't connect to it, i.e. starting without -ipcbind.

After letting the chain sync (a few days worth of blocks), I shut it down. At that point the log (on Ubuntu):

2023-05-05T10:30:24Z UpdateTip: new best=000000000000000000010ed2e719320bb8bdea58868cf0bee4897b33b7c20c14 height=788138 version=0x32b96000 log2_work=94.158122 tx=831696670 date='2023-05-03T20:58:19Z' progress=0.999404 cache=515.2MiB(4072240txo)
2023-05-05T10:30:29Z Synchronizing blockheaders, height: 788359 (~100.00%)
^C2023-05-05T10:30:42Z [rpc] Interrupting HTTP RPC server
2023-05-05T10:30:42Z [rpc] Interrupting RPC
2023-05-05T10:30:42Z tor: Thread interrupt
2023-05-05T10:30:42Z torcontrol thread exit
2023-05-05T10:30:42Z addcon thread exit
2023-05-05T10:30:42Z Shutdown: In progress...
2023-05-05T10:30:42Z opencon thread exit
2023-05-05T10:30:42Z [rpc] Stopping HTTP RPC server
2023-05-05T10:30:42Z [rpc] Stopping RPC
2023-05-05T10:30:42Z [rpc] RPC stopped.
terminate called after throwing an instance of 'ipc::Exception'
  what():  kj::Exception: kj/async-io-unix.c++:498: disconnected: ::read(fd, buffer, maxBytes): Connection reset by peer
stack: 7fb82ba25644 7fb82ba34eab 7fb82b9c41b5 7fb82ba1bdb0 7fb82bb95720 7fb82bb8ec20 7fb82bb92df0 7fb82bbf6b30 7fb82bbf1220 7fb82bbe6cc0 7fb82bbe2800 7fb82bbd25d0 7fb82bbd3084 7fb82bbcf610 564eea2664d0 564eea26ad60
Aborted (core dumped)

I then have to restart the node and sync again.

(update: it's unrelated to the commits in this PR, I added a comment on #10102 (comment))

@Sjors
Copy link
Member

Sjors commented May 5, 2023

I'm able to produce another crash that may be related to this PR. When using an external signer that's connected to the Ubuntu machine, and trying to spend with it from the macOs GUI. After I click Sign on device, and hit "send":

Assertion failed: (!complete), function sendButtonClicked, file sendcoinsdialog.cpp, line 525.
zsh: abort      src/bitcoin-gui -ipcconnect=unix:///tmp/remote-node.sock

It also crashes if I create a PSBT (options -> wallet -> enable psbt controls) instead, just a different line

Assertion failed: (!complete), function sendButtonClicked, file sendcoinsdialog.cpp, line 509.

Both nodes were configured with have --enable-external-signer. I'm able to call enumeratesigners from the console.

These asserts happen after calling fillPSBT( without signing (This prevents an external from being called prematurely). complete can't be possibly be true, hence the assert.

@ryanofsky
Copy link
Contributor Author

Thanks Sjors, I'm trying to reproduce the shutdown error but didn't succeed yet. Will try again with a mainnet node. I didn't look into the external signer bug yet, but maybe that is more straightforward. In both cases running with -debug=ipc could make it clearer what's going on, especially in the shutdown case. Probably what is happening in that case is that some thread is trying make an IPC call after the socket is closed, so an ipc::Exception is raised which is uncaught. Fix might be synchronizing the IPC call & socket close, or just catching the exception and handling it

@DrahtBot
Copy link
Contributor

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

@DrahtBot
Copy link
Contributor

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.

@achow101 achow101 marked this pull request as draft September 20, 2023 16:09
@ryanofsky ryanofsky mentioned this pull request Oct 24, 2023
49 tasks
knst pushed a commit to knst/dash that referenced this pull request Jan 16, 2024
…ode (partial revert bitcoin#10244)

519cae8 gui: Delay interfaces::Node initialization (Russell Yanofsky)
102abff gui: Replace interface::Node references with pointers (Russell Yanofsky)
91aced7 gui: Remove unused interfaces::Node references (Russell Yanofsky)
e133631 gui: Partially revert bitcoin#10244 gArgs and Params changes (Russell Yanofsky)

Pull request description:

  This PR is part of the [process separation project](https://github.com/bitcoin/bitcoin/projects/10).

  ---

  This is a partial revert of bitcoin#10244. It changes gui code to go back to using gArgs and Params() functions directly instead of using interfaces::Node to handle arguments.

  These changes were originally pushed as part of bitcoin#19461. Motivation is to support a new GUI process connecting to an already running node process. Details are explained in commit messages, but in addition to spawning a new bitcoin-node process, we want bitcoin-gui to connect to an existing bitcoin-node process. So for that reason it should be able to parse its own parameters, rather than rely on the node.

ACKs for top commit:
  MarcoFalke:
    re-ACK 519cae8, only change is rebase and addressed nits of my previous review last week 🌄

Tree-SHA512: 9c339dd82ba78bcc7b887b84d872f35ccc7dfa3d271691e6eafe8a2048cbbe3bdde1e810ce33d0714d75d048c9de3470e9e9b6f8306a6047d1cb3548f6858dc8
knst added a commit to knst/dash that referenced this pull request Jan 17, 2024
…ode (partial revert bitcoin#10244)

519cae8 gui: Delay interfaces::Node initialization (Russell Yanofsky)
102abff gui: Replace interface::Node references with pointers (Russell Yanofsky)
91aced7 gui: Remove unused interfaces::Node references (Russell Yanofsky)
e133631 gui: Partially revert bitcoin#10244 gArgs and Params changes (Russell Yanofsky)

Pull request description:

  This PR is part of the [process separation project](https://github.com/bitcoin/bitcoin/projects/10).

  ---

  This is a partial revert of bitcoin#10244. It changes gui code to go back to using gArgs and Params() functions directly instead of using interfaces::Node to handle arguments.

  These changes were originally pushed as part of bitcoin#19461. Motivation is to support a new GUI process connecting to an already running node process. Details are explained in commit messages, but in addition to spawning a new bitcoin-node process, we want bitcoin-gui to connect to an existing bitcoin-node process. So for that reason it should be able to parse its own parameters, rather than rely on the node.

ACKs for top commit:
  MarcoFalke:
    re-ACK 519cae8, only change is rebase and addressed nits of my previous review last week 🌄

Tree-SHA512: 9c339dd82ba78bcc7b887b84d872f35ccc7dfa3d271691e6eafe8a2048cbbe3bdde1e810ce33d0714d75d048c9de3470e9e9b6f8306a6047d1cb3548f6858dc8
knst added a commit to knst/dash that referenced this pull request Jan 19, 2024
…ode (partial revert bitcoin#10244)

519cae8 gui: Delay interfaces::Node initialization (Russell Yanofsky)
102abff gui: Replace interface::Node references with pointers (Russell Yanofsky)
91aced7 gui: Remove unused interfaces::Node references (Russell Yanofsky)
e133631 gui: Partially revert bitcoin#10244 gArgs and Params changes (Russell Yanofsky)

Pull request description:

  This PR is part of the [process separation project](https://github.com/bitcoin/bitcoin/projects/10).

  ---

  This is a partial revert of bitcoin#10244. It changes gui code to go back to using gArgs and Params() functions directly instead of using interfaces::Node to handle arguments.

  These changes were originally pushed as part of bitcoin#19461. Motivation is to support a new GUI process connecting to an already running node process. Details are explained in commit messages, but in addition to spawning a new bitcoin-node process, we want bitcoin-gui to connect to an existing bitcoin-node process. So for that reason it should be able to parse its own parameters, rather than rely on the node.

ACKs for top commit:
  MarcoFalke:
    re-ACK 519cae8, only change is rebase and addressed nits of my previous review last week 🌄

Tree-SHA512: 9c339dd82ba78bcc7b887b84d872f35ccc7dfa3d271691e6eafe8a2048cbbe3bdde1e810ce33d0714d75d048c9de3470e9e9b6f8306a6047d1cb3548f6858dc8
knst added a commit to knst/dash that referenced this pull request Jan 20, 2024
…ode (partial revert bitcoin#10244)

519cae8 gui: Delay interfaces::Node initialization (Russell Yanofsky)
102abff gui: Replace interface::Node references with pointers (Russell Yanofsky)
91aced7 gui: Remove unused interfaces::Node references (Russell Yanofsky)
e133631 gui: Partially revert bitcoin#10244 gArgs and Params changes (Russell Yanofsky)

Pull request description:

  This PR is part of the [process separation project](https://github.com/bitcoin/bitcoin/projects/10).

  ---

  This is a partial revert of bitcoin#10244. It changes gui code to go back to using gArgs and Params() functions directly instead of using interfaces::Node to handle arguments.

  These changes were originally pushed as part of bitcoin#19461. Motivation is to support a new GUI process connecting to an already running node process. Details are explained in commit messages, but in addition to spawning a new bitcoin-node process, we want bitcoin-gui to connect to an existing bitcoin-node process. So for that reason it should be able to parse its own parameters, rather than rely on the node.

ACKs for top commit:
  MarcoFalke:
    re-ACK 519cae8, only change is rebase and addressed nits of my previous review last week 🌄

Tree-SHA512: 9c339dd82ba78bcc7b887b84d872f35ccc7dfa3d271691e6eafe8a2048cbbe3bdde1e810ce33d0714d75d048c9de3470e9e9b6f8306a6047d1cb3548f6858dc8
@DrahtBot
Copy link
Contributor

⌛ 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.

knst added a commit to knst/dash that referenced this pull request Jan 24, 2024
…ode (partial revert bitcoin#10244)

519cae8 gui: Delay interfaces::Node initialization (Russell Yanofsky)
102abff gui: Replace interface::Node references with pointers (Russell Yanofsky)
91aced7 gui: Remove unused interfaces::Node references (Russell Yanofsky)
e133631 gui: Partially revert bitcoin#10244 gArgs and Params changes (Russell Yanofsky)

Pull request description:

  This PR is part of the [process separation project](https://github.com/bitcoin/bitcoin/projects/10).

  ---

  This is a partial revert of bitcoin#10244. It changes gui code to go back to using gArgs and Params() functions directly instead of using interfaces::Node to handle arguments.

  These changes were originally pushed as part of bitcoin#19461. Motivation is to support a new GUI process connecting to an already running node process. Details are explained in commit messages, but in addition to spawning a new bitcoin-node process, we want bitcoin-gui to connect to an existing bitcoin-node process. So for that reason it should be able to parse its own parameters, rather than rely on the node.

ACKs for top commit:
  MarcoFalke:
    re-ACK 519cae8, only change is rebase and addressed nits of my previous review last week 🌄

Tree-SHA512: 9c339dd82ba78bcc7b887b84d872f35ccc7dfa3d271691e6eafe8a2048cbbe3bdde1e810ce33d0714d75d048c9de3470e9e9b6f8306a6047d1cb3548f6858dc8
knst added a commit to knst/dash that referenced this pull request Jan 27, 2024
…ode (partial revert bitcoin#10244)

519cae8 gui: Delay interfaces::Node initialization (Russell Yanofsky)
102abff gui: Replace interface::Node references with pointers (Russell Yanofsky)
91aced7 gui: Remove unused interfaces::Node references (Russell Yanofsky)
e133631 gui: Partially revert bitcoin#10244 gArgs and Params changes (Russell Yanofsky)

Pull request description:

  This PR is part of the [process separation project](https://github.com/bitcoin/bitcoin/projects/10).

  ---

  This is a partial revert of bitcoin#10244. It changes gui code to go back to using gArgs and Params() functions directly instead of using interfaces::Node to handle arguments.

  These changes were originally pushed as part of bitcoin#19461. Motivation is to support a new GUI process connecting to an already running node process. Details are explained in commit messages, but in addition to spawning a new bitcoin-node process, we want bitcoin-gui to connect to an existing bitcoin-node process. So for that reason it should be able to parse its own parameters, rather than rely on the node.

ACKs for top commit:
  MarcoFalke:
    re-ACK 519cae8, only change is rebase and addressed nits of my previous review last week 🌄

Tree-SHA512: 9c339dd82ba78bcc7b887b84d872f35ccc7dfa3d271691e6eafe8a2048cbbe3bdde1e810ce33d0714d75d048c9de3470e9e9b6f8306a6047d1cb3548f6858dc8
knst added a commit to knst/dash that referenced this pull request Jan 27, 2024
…ode (partial revert bitcoin#10244)

519cae8 gui: Delay interfaces::Node initialization (Russell Yanofsky)
102abff gui: Replace interface::Node references with pointers (Russell Yanofsky)
91aced7 gui: Remove unused interfaces::Node references (Russell Yanofsky)
e133631 gui: Partially revert bitcoin#10244 gArgs and Params changes (Russell Yanofsky)

Pull request description:

  This PR is part of the [process separation project](https://github.com/bitcoin/bitcoin/projects/10).

  ---

  This is a partial revert of bitcoin#10244. It changes gui code to go back to using gArgs and Params() functions directly instead of using interfaces::Node to handle arguments.

  These changes were originally pushed as part of bitcoin#19461. Motivation is to support a new GUI process connecting to an already running node process. Details are explained in commit messages, but in addition to spawning a new bitcoin-node process, we want bitcoin-gui to connect to an existing bitcoin-node process. So for that reason it should be able to parse its own parameters, rather than rely on the node.

ACKs for top commit:
  MarcoFalke:
    re-ACK 519cae8, only change is rebase and addressed nits of my previous review last week 🌄

Tree-SHA512: 9c339dd82ba78bcc7b887b84d872f35ccc7dfa3d271691e6eafe8a2048cbbe3bdde1e810ce33d0714d75d048c9de3470e9e9b6f8306a6047d1cb3548f6858dc8
PastaPastaPasta pushed a commit to knst/dash that referenced this pull request Jan 31, 2024
…ode (partial revert bitcoin#10244)

519cae8 gui: Delay interfaces::Node initialization (Russell Yanofsky)
102abff gui: Replace interface::Node references with pointers (Russell Yanofsky)
91aced7 gui: Remove unused interfaces::Node references (Russell Yanofsky)
e133631 gui: Partially revert bitcoin#10244 gArgs and Params changes (Russell Yanofsky)

Pull request description:

  This PR is part of the [process separation project](https://github.com/bitcoin/bitcoin/projects/10).

  ---

  This is a partial revert of bitcoin#10244. It changes gui code to go back to using gArgs and Params() functions directly instead of using interfaces::Node to handle arguments.

  These changes were originally pushed as part of bitcoin#19461. Motivation is to support a new GUI process connecting to an already running node process. Details are explained in commit messages, but in addition to spawning a new bitcoin-node process, we want bitcoin-gui to connect to an existing bitcoin-node process. So for that reason it should be able to parse its own parameters, rather than rely on the node.

ACKs for top commit:
  MarcoFalke:
    re-ACK 519cae8, only change is rebase and addressed nits of my previous review last week 🌄

Tree-SHA512: 9c339dd82ba78bcc7b887b84d872f35ccc7dfa3d271691e6eafe8a2048cbbe3bdde1e810ce33d0714d75d048c9de3470e9e9b6f8306a6047d1cb3548f6858dc8
@DrahtBot
Copy link
Contributor

⌛ 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: In progress
Development

Successfully merging this pull request may close these issues.

None yet

8 participants