Skip to content

multiprocess: Add bitcoin-wallet -ipcconnect option #19460

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.

Sign up for GitHub

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

Jump to bottom
Draft
wants to merge 18 commits into
base: master
Choose a base branch
from
Draft

multiprocess: Add bitcoin-wallet -ipcconnect option #19460

wants to merge 18 commits into from

Conversation

ryanofsky
Copy link
Contributor

@ryanofsky ryanofsky commented Jul 7, 2020 •
edited
Loading

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

Building on #10102, this adds an -ipcconnect option to bitcoin-wallet and an -ipcbind option to bitcoin-node (both enabled by default in multiprocess builds) so bitcoin node will listen on a <datadir>/sockets/node.sock unix socket, and bitcoin-wallet will connect to it.

The idea is that bitcoin-wallet can be extended in the future to have some online functionality. For example, there could be a bitcoin-wallet sync command that will update balances and sync latest transactions to an unloaded wallet, or a bitcoin-wallet serve subcommand that loads a wallet and serves RPC requests, or a bitcoin-wallet shell subcommand that allows running RPC methods interactively like the GUI console, or just general support for bitcoin-wallet <rpc method> <rpc params> invocations suggested #13926 (comment).

This PR is small and doesn't do much. The only visible change is that bitcoin-wallet now checks whether a node socket exists on startup and prints "Connected to IPC address" if it can connect it it.

The default bitcoin-wallet connect option is -ipcconnect=auto, which connects if possible as described above, and proceeds offline if not possible. Other supported options are -noipcconnect to disable ipc, -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.

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.

@ryanofsky ryanofsky mentioned this pull request Jul 7, 2020
Draft
@DrahtBot
Copy link
Contributor

DrahtBot commented Jul 7, 2020 •
edited
Loading

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/19460.

Reviews

See the guideline for information on the review process.
A summary of reviews will appear here.

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.

This was referenced Jul 7, 2020
Merged
Merged
Merged
Merged
Closed
Merged
Merged
Merged
Merged
Closed
Merged
Merged
Merged
Closed
Merged
Closed
Draft
Closed
Closed
Merged
@ryanofsky
multiprocess: Expose Chain interface
4fa7f72 
Expose Chain interface to external processes spawning or connecting to
bitcoin-node.
@Sjors Sjors mentioned this pull request Feb 10, 2025
Closed
@DrahtBot DrahtBot mentioned this pull request Mar 18, 2025
Merged
@ryanofsky
Copy link
Contributor Author

ryanofsky commented Mar 19, 2025 •
edited
Loading

Updated 240bc47 -> ce32dc3 (pr/ipc-connect.42 -> pr/ipc-connect.43, compare) to fix CI failure in tool_wallet.py https://cirrus-ci.com/task/4513424845045760 where bitcoin-wallet throws an exception because the temporary directory name is too long and it causes a "Unix address path "..." exceeded maximum socket path length" error
Rebased ce32dc3 -> 511f5a9 (pr/ipc-connect.43 -> pr/ipc-connect.44, compare) on top of updated base pr/ipc.216
Rebased 511f5a9 -> 217d194 (pr/ipc-connect.44 -> pr/ipc-connect.45, compare) on top of updated base pr/ipc.217 to fix std::span conflict https://cirrus-ci.com/task/5166515827245056

ryanofsky and others added 14 commits March 21, 2025 18:40
@ryanofsky
Merge remote-tracking branch 'origin/pull/29409/head'
b5cca2e
@ryanofsky
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.
@ryanofsky
test: Fix multiprocess test for unclean shutdown on kill
548540c
@ryanofsky
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.
@ryanofsky
multiprocess: Add capnp serialization code for bitcoin types
ccd0a20
@ryanofsky
multiprocess: Add capnp wrapper for Wallet interface
d903a6c
@ryanofsky
multiprocess: Add capnp wrapper for Node interface
7b906e9
@ryanofsky
multiprocess: Make bitcoin-gui spawn a bitcoin-node process
7244a61 
Spawn node subprocess instead of running node code internally
@ryanofsky
multiprocess: Make bitcoin-node spawn a bitcoin-wallet process
f4fe8c3 
Spawn wallet subprocess instead of running wallet code internally
@ryanofsky
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.
@ryanofsky
doc: Multiprocess misc doc and comment updates
bbcbe85
@ryanofsky
combine_logs: Handle multiprocess wallet log files
c9a4963
@ryanofsky
Merge remote-tracking branch 'origin/pull/10102/head'
ec1b264
@ryanofsky
multiprocess: Add bitcoin-wallet -ipcconnect option
217d194 
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
This was referenced Mar 26, 2025
Open
Merged
@DrahtBot
Copy link
Contributor

DrahtBot commented Apr 2, 2025

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

@jimhashhq jimhashhq mentioned this pull request Apr 9, 2025
Open
@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
Reviewers
2 more reviewers

@RonSherfey RonSherfey RonSherfey approved these changes

@Korosiua1206 Korosiua1206 Korosiua1206 approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
Build system Needs rebase Utils/log/libs Wallet
Projects
Process Separation
Status: In progress
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@ryanofsky @DrahtBot @jonasschnelli @RonSherfey @Korosiua1206 @ajtowns @maflcko @hebasto