multiprocess: Don't require bitcoin -m argument when IPC options are used

ryanofsky commented at 8:08 pm on August 20, 2025: contributor

This change makes the bitcoin command respect IPC command line options and bitcoin.conf settings, so IPC listening can be enabled by just running bitcoin node -ipcbind=unix or bitcoin node with ipcbind=unix in the configuration file, and there is no longer a need to specify a multiprocess -m option like bitcoin -m node [...]

sipa and theuni in #31802 pointed out that users shouldn’t be exposed to multiprocess implementation details just to use IPC features, so current need to specify the bitcoin -m option in conjunction with -ipcbind could be seen as a design mistake and not just a usage inconvenience.

This PR also adds a dedicated functional test for the bitcoin wrapper command and to make sure it calls the right binaries and test the new functionality.

This PR is part of the process separation project.

DrahtBot added the label interfaces on Aug 20, 2025

DrahtBot commented at 8:08 pm on August 20, 2025: contributor

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	Sjors
Stale ACK	sipa, TheCharlatan

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:

#28802 (ArgsManager: support subcommand-specific options by ajtowns)

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.

LLM Linter (✨ experimental)

Possible typos and grammar issues:

“Warning: failed to parse subcommand command line options: %s\n” -> “Warning: failed to parse subcommand’s command-line options: %s\n” [the phrase “subcommand command line” is awkward/confusing; “subcommand’s command-line options” makes the relationship and meaning clear]
“// Process help and version before taking care about datadir” -> “// Process help and version before taking care of the datadir” [use “taking care of” instead of “taking care about” for grammatical correctness]

drahtbot_id_5_m

DrahtBot added the label CI failed on Aug 20, 2025

DrahtBot commented at 9:13 pm on August 20, 2025: contributor

🚧 At least one of the CI tasks failed. Task lint: https://github.com/bitcoin/bitcoin/runs/48525913081 LLM reason (✨ experimental): The CI failure is caused by lint errors related to unused imports and formatting issues in Python code.

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

Sjors commented at 7:09 am on August 21, 2025: member

Concept ACK

This also makes it easier to document instructions for miners, they just need to drop the “d” from bitcoind and then -ipcbind behaves like any other setting.

The help text should explain that -m is assumed if any IPC arguments are passed.

in src/bitcoin.cpp:169 in 5a28d0b027 outdated

164+    }
165+    args.SelectConfigNetwork(args.GetChainTypeString());
166+
167+    // If any -ipc* options are set these need to be processed by a
168+    // multiprocess-capable binary.
169+    return args.IsArgSet("-ipcbind") || args.IsArgSet("-ipcconnect") || args.IsArgSet("-ipcfd");

Sjors commented at 1:17 pm on September 1, 2025:

5a28d0b027fdca07c55c853a550324c0aa1c450f: did you mean to add -ipcconnect and -ipcfd here?

ryanofsky commented at 6:50 pm on September 3, 2025:

re: #33229 (review)

5a28d0b: did you mean to add -ipcconnect and -ipcfd here?

Yeah they are not necessary but listed to be more future-proof. It makes sense for any IPC options to choose the IPC binary.

Sjors commented at 4:28 pm on September 5, 2025:

I asked because it triggered a missing documentation error.

Sjors commented at 1:18 pm on September 1, 2025: member

The following patches fix the linter:

 0iff --git a/src/bitcoin.cpp b/src/bitcoin.cpp
 1index c1a5fce33a..aa74f3e54e 100644
 2--- a/src/bitcoin.cpp
 3+++ b/src/bitcoin.cpp
 4@@ -166,7 +166,7 @@ bool UseMultiprocess(const CommandLine& cmd)
 5 
 6     // If any -ipc* options are set these need to be processed by a
 7     // multiprocess-capable binary.
 8-    return args.IsArgSet("-ipcbind") || args.IsArgSet("-ipcconnect") || args.IsArgSet("-ipcfd");
 9+    return args.IsArgSet("-ipcbind");
10 }
11 
12 //! Execute the specified bitcoind, bitcoin-qt or other command line in `args`
13diff --git a/test/functional/tool_bitcoin.py b/test/functional/tool_bitcoin.py
14index 7ceb6c85b3..a6cce2339a 100755
15--- a/test/functional/tool_bitcoin.py
16+++ b/test/functional/tool_bitcoin.py
17@@ -3,7 +3,6 @@
18 # Distributed under the MIT software license, see the accompanying
19 # file COPYING or http://www.opensource.org/licenses/mit-license.php.
20 """Test the IPC  interface."""
21-from pathlib import Path
22 from test_framework.test_framework import BitcoinTestFramework
23 from test_framework.util import (
24     append_config,
25@@ -63,7 +62,7 @@ class ToolBitcoinTest(BitcoinTestFramework):
26         self.test_args([], ["-ipcbind=unix"], expect_exe="bitcoin-node")
27 
28         self.log.info("Ensure bitcoin recognizes -ipcbind in config file")
29-        append_config(node.datadir_path, [f"ipcbind=unix"])
30+        append_config(node.datadir_path, ["ipcbind=unix"])
31         self.test_args([], [], expect_exe="bitcoin-node")

As for the macOS error I can’t reproduce:

 02025-08-20T20:26:26.298984Z TestFramework (INFO): Ensure bitcoin -m invokes bitcoin-node
 12025-08-20T20:26:26.315700Z TestFramework (ERROR): Unexpected exception
 2Traceback (most recent call last):
 3  File "/Users/runner/work/bitcoin/bitcoin/test/functional/test_framework/test_framework.py", line 195, in main
 4    self.run_test()
 5    ~~~~~~~~~~~~~^^
 6  File "/Users/runner/work/bitcoin/bitcoin/ci/scratch/build-aarch64-apple-darwin23.6.0/test/functional/tool_bitcoin.py", line 54, in run_test
 7    self.test_args(["-m"], [], expect_exe="bitcoin-node")
 8    ~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 9  File "/Users/runner/work/bitcoin/bitcoin/ci/scratch/build-aarch64-apple-darwin23.6.0/test/functional/tool_bitcoin.py", line 41, in test_args
10    assert_equal(get_exe_name(out), expect_exe.encode())
11                 ~~~~~~~~~~~~^^^^^
12  File "/Users/runner/work/bitcoin/bitcoin/ci/scratch/build-aarch64-apple-darwin23.6.0/test/functional/tool_bitcoin.py", line 90, in get_exe_name
13    return re.match(rb".*\s(\S+)\s*(?:\n|$)", version_str).group(1)
14           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
15AttributeError: 'NoneType' object has no attribute 'group'

However you might need the same workaround for long -ipcbind paths as in https://github.com/bitcoin/bitcoin/pull/30437/files#diff-7f22a082e3a80ca2f212e36193f87dd80237035afedb7f15b116ef7fa265f3eeR139

in src/init/bitcoin-qt.cpp:19 in 231fba1f06 outdated

15@@ -16,6 +16,8 @@
16 
17 namespace init {
18 namespace {
19+const char* EXE_NAME = "bitcoin-node";

Sjors commented at 1:29 pm on September 1, 2025:

231fba1f067211b889080bd97c8f58df1500ecf1: I’m confused, should this be bitcoin-qt?

I don’t see it appear when I run build/bin/bitcoin gui --version

ryanofsky commented at 6:35 pm on September 3, 2025:

re: #33229 (review)

Good catch, changed to bitcoin-qt. Also good to know text doesn’t appear in gui –version output but I think that is ok as long as the test is only calling bitcoin node. I do think it’s probably good for the Init::exeName method to return the right name for consistency even it’s not called right now.

in test/functional/tool_bitcoin.py:42 in 75cdfb49b2 outdated

37+            ret, out, err = get_node_output(node)
38+            assert_equal(get_exe_name(out), expect_exe.encode())
39+            assert_equal(err, b"")
40+
41+    def run_test(self):
42+        self.log.info("Ensure bitcoin command invokes bitcoind by default")

Sjors commented at 1:30 pm on September 1, 2025:

bitcoin node command?

ryanofsky commented at 6:47 pm on September 3, 2025:

re: #33229 (review)

bitcoin node command?

Thanks, updated

ryanofsky force-pushed on Sep 3, 2025

ryanofsky force-pushed on Sep 4, 2025

in test/functional/test_framework/test_node.py:449 in 18423cd075 outdated

445@@ -446,7 +446,7 @@ def is_node_stopped(self, *, expected_stderr="", expected_ret_code=0):
446         self.stderr.seek(0)
447         stderr = self.stderr.read().decode('utf-8').strip()
448         if stderr != expected_stderr:
449-            raise AssertionError("Unexpected stderr {} != {}".format(stderr, expected_stderr))
450+            print("Unexpected stderr {} != {}".format(stderr, expected_stderr))

Sjors commented at 6:46 am on September 5, 2025:

No access to self.logger here?

ryanofsky commented at 12:38 pm on September 5, 2025:

No access to self.logger here?

Yeah sorry, this is just a workaround for debugging. This test is just not working on windows and I was trying to debug it using CI to avoid needing to setup a windows build and vm and see if the issue happens there.

So far I’ve fixed one issue where unicode path CI uses was not properly handled by bitcoin wrapper on windows, but there is another issue where stdout/stderr process of the bitcoind do not seem to be captured by python when it is invoked through bitcoin node. Have more things I want to try to debug this and some ideas for fixing this, so will continue to use this PR to debug a little while, but if it doesn’t work out I’ll just disable the new test on windows.

Sjors commented at 6:29 am on September 8, 2025: member

Looks like Windows CI is still unhappy:

0 File "D:\a\bitcoin\bitcoin/test/functional/tool_bitcoin.py", line 62, in run_test
1                                       self.test_args([], [], expect_exe="bitcoind")

Maybe @hebasto has an idea?

I would be nice to get this in v30 so we don’t have to teach people to use -m only for that to become unnecessary in the next release. But it’s not a huge deal.

ryanofsky commented at 9:57 am on September 8, 2025: contributor

re: #33229 (comment)

I would be nice to get this in v30 so we don’t have to teach people to use -m only for that to become unnecessary in the next release. But it’s not a huge deal.

Yes I’d like to include / backport it in v30.

The code auto-setting -m does work, it’s just that this PR added a new functional test and the new test doesn’t work on windows because it depends on checking bitcoin node -version output and for whatever reason python fails to capture any stdout/stderr output from the child process on windows (unrelated to whether -m is passed or not). I think the problem is caused by an implementation quirk of _wexecvp or a problem with state of stdout/stderr handles before making that call, and I’ve been trying to use the PR to debug by adding debug commits. But since the fix does not seem to be straightforward, I’ll drop the debug commits and just disable the test on windows and debug separately.

ryanofsky force-pushed on Sep 8, 2025

ryanofsky commented at 4:03 pm on September 8, 2025: contributor

All ci jobs are passing now that the new tool_bitcoin.py test is disabled on windows, so this PR should be ready to review. I’ll set up a vm and address the windows test problem in a followup PR, since I think the _wexecvp/stdout issue might not be trivial to fix.

Updated 0c82805f75438c38f5a7f942df2a96ec3d61d68e -> f9685d6a1389938b0cceb31d9eef201ab3dd2e86 (pr/ipc-auto.23 -> pr/ipc-auto.24, compare) with some cleanups in test code.

Sjors referenced this in commit 148ced15c3 on Sep 9, 2025

Sjors commented at 9:04 am on September 9, 2025: member

ACK f9685d6a1389938b0cceb31d9eef201ab3dd2e86

We could simplify the functional tests a bit with the last commit of https://github.com/Sjors/bitcoin/pull/104 which drops BITCOIN_CMD and simplifies the _argv logic.

(probably better to leave as a followup)

ryanofsky added this to the milestone 30.0 on Sep 9, 2025

sipa commented at 2:07 pm on September 9, 2025: member

utACK f9685d6a1389938b0cceb31d9eef201ab3dd2e86

ryanofsky commented at 2:08 pm on September 9, 2025: contributor

Updated the description and added this as a 30.0 miestone in case that makes sense. I’m not exactly sure how release milestones are used so we can remove it if this is inappropriate

With this change, release notes https://github.com/bitcoin-core/bitcoin-devwiki/wiki/v30.0-Release-Notes-Draft#ipc-mining-interface could be simplified a little to not mention -m and a sentence could be dropped (“The -m option launches a new internal binary and is currently required but will become optional in the future.”)

in src/interfaces/init.h:41 in 47721451a9 outdated

37@@ -38,6 +38,7 @@ class Init
38     virtual std::unique_ptr<Echo> makeEcho() { return nullptr; }
39     virtual Ipc* ipc() { return nullptr; }
40     virtual bool canListenIpc() { return false; }
41+    virtual const char* exeName() { return nullptr; }

TheCharlatan commented at 11:29 am on September 10, 2025:

I’m a bit confused about this first commit. What exactly is it supposed to fix? FWIW the -help for bitcoin-node is still:

 0./build_dev_mode_clang/bin/bitcoin-node -help | head -n 12                                                                                PIPE|0 ✔  13:28:55 
 1Bitcoin Core daemon version v30.99.0-2c8a478db4b8
 2
 3The Bitcoin Core daemon (bitcoind) is a headless program that connects to the Bitcoin network to validate and relay transactions and blocks, as well as relaying addresses.
 4
 5It provides the backbone of the Bitcoin network and its RPC, REST and ZMQ services can provide various transaction, block and address-related services.
 6
 7There is an optional wallet component which provides transaction services.
 8
 9It can be used in a headless environment or as part of a server setup.
10
11Usage: bitcoind [options]

ryanofsky commented at 11:46 am on September 10, 2025:

I’m a bit confused about this first commit. What exactly is it supposed to fix?

Just to clarify here (I can also update the commit message) this first commit adds the binary name to -version output (-help output may be different) as a way of giving the functional test an easy way to detect whether correct binary was called.

Sjors commented at 11:59 am on September 10, 2025:

This seems useful enough even outside the test suite. E.g. when a user files a bug report it’s good to know whether they ran bitcoind or bitcoin-node, and instructing them to do bitcoin node -version is a lot easier than to look at process names.

OTOH that wouldn’t be useful if e.g. they had an RPC error, since bitcoin rpc -version won’t tell you what node is running, we’d have to provide that via RPC.

I would be fine with also adding it to -help, but there it’s less relevant.

TheCharlatan commented at 12:09 pm on September 10, 2025:

Right, I think I missed the context, because the changed lines also run when processing the help command. Should we fix the output?

 0diff --git a/src/bitcoind.cpp b/src/bitcoind.cpp
 1index a4373dafdf..e046be19aa 100644
 2--- a/src/bitcoind.cpp
 3+++ b/src/bitcoind.cpp
 4@@ -140 +140,3 @@ static bool ProcessInitCommands(interfaces::Init& init, ArgsManager& args)
 5-        if (const char* exe_name{init.exeName()}) {
 6+        const char* exe_name_ptr = init.exeName();
 7+        std::string exe_name = exe_name_ptr ? exe_name_ptr : "bitcoind";
 8+        if (exe_name_ptr) {
 9@@ -150 +152 @@ static bool ProcessInitCommands(interfaces::Init& init, ArgsManager& args)
10-                "The " CLIENT_NAME " daemon (bitcoind) is a headless program that connects to the Bitcoin network to validate and relay transactions and blocks, as well as relaying addresses.\n\n"
11+                "The " CLIENT_NAME " daemon (" + exe_name + ") is a headless program that connects to the Bitcoin network to validate and relay transactions and blocks, as well as relaying addresses.\n\n"
12@@ -155 +157 @@ static bool ProcessInitCommands(interfaces::Init& init, ArgsManager& args)
13-                "Usage: bitcoind [options]\n"
14+                "Usage: " + exe_name + " [options]\n"

ryanofsky commented at 8:37 am on September 12, 2025:

re: #33229 (review)

I think instead of expanding this PR I’m inclined to keep the first commit minimal and limited to (1) adding binary name to bitcoind -version and bitcoin-node -version output so the test can work, and (2) returning the right binary names from interfaces::Init::exeName() methods.

There are a few related followups that would be nice to make but aren’t needed by this PR:

Updating QT -version output so it includes binary names like bitcoind output.
Updating -help output to mention binary names as suggested above (instead of just the CLIENT_NAME generically).
Updating -help usage lines. One option would be to have them just reflect the command line that was used. I.e. show Usage: bitcoin node [options] or Usage: bitcoin gui [options] or Usage: bitcoind [options] depending on how the program was started. (Wrapper could set an environment variable to indicate this.) Another option could be to show both usages, or both usages only when bitcoind is started like:

0  Usage: bitcoin node [options]
1     or: bitcoind [options]
2
3  Options:
4
5    -alertnotify=<cmd>
6         [...]

ryanofsky removed the label CI failed on Sep 11, 2025

TheCharlatan approved

TheCharlatan commented at 8:54 am on September 16, 2025: contributor

ACK f9685d6a1389938b0cceb31d9eef201ab3dd2e86

init: add exe name to bitcoind, bitcoin-node -version output to be able to distinguish these in tests 0972f55040

test: add tool_bitcoin to test bitcoin wrapper behavior 29e836fae6

bitcoin: Make wrapper not require -m

Choose the right binary by default if an IPC option is specified

453b0fa286

DrahtBot added the label Needs rebase on Sep 17, 2025

ryanofsky force-pushed on Sep 17, 2025

ryanofsky commented at 1:04 pm on September 17, 2025: contributor

Unfortunately this conflicted with #33407 due to cmake change on adjacent line, so needed to rebase.

Rebased f9685d6a1389938b0cceb31d9eef201ab3dd2e86 -> 453b0fa286e5dce0af682b7b73684dd6415a50de (pr/ipc-auto.24 -> pr/ipc-auto.25, compare) due to conflict with #33407

DrahtBot removed the label Needs rebase on Sep 17, 2025

Sjors commented at 4:18 pm on September 17, 2025: member

re-ACK 453b0fa286e5dce0af682b7b73684dd6415a50de

Just a rebase. Briefly retested on macOS that build/bin/bitcoin node -ipcbind=unix launches bitcoin-node.

DrahtBot requested review from TheCharlatan on Sep 17, 2025

DrahtBot requested review from sipa on Sep 17, 2025

multiprocess: Don’t require bitcoin -m argument when IPC options are used #33229

Code Coverage & Benchmarks

Reviews

Conflicts

LLM Linter (✨ experimental)