Intended to make bitcoin command line features more discoverable and allow installing new multiprocess binaries in libexec/ instead of bin/ so they don’t cause confusion.
Idea and implementation of this were discussed in #30983
This PR is part of the process separation project.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31375.
See the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
Reviewers, this pull request conflicts with the following ones:
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.
🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/33557375133
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.
pr/wrap.3 -> pr/wrap.4, compare) with fixes for windows and fuzz CI errors, and lint and tidy fixes
Updated 63df9f3deb35be79496f7c240e3303e1d96c6832 -> da108a6e5be220654a65b6613ee7eb2c4ddc8677 (pr/wrap.4 -> pr/wrap.5, compare) fixing windows include error, fs lint error, and previous releases test bug
183+ // Otherwise if exe is installed in a bin/ directory, first look for target
184+ // executable in libexec/
185+ (exe_dir.filename() == "bin" && try_exec(fs::path{exe_dir.parent_path()} / "libexec" / fs::PathFromString(args[0]).filename())) ||
186+ // Otherwise look for target executable next to current exe
187+ try_exec(exe_dir / fs::PathFromString(args[0]).filename()) ||
188+ // Otherwise just look on the system path.
build/src/bitcoin gui to fail. Right now it would launch any bitcoin-qt in my $PATH.
re: #31375 (review)
b06c7ad: this is potentially confusing. If I build without gui, I expect
build/src/bitcoin guito fail. Right now it would launch anybitcoin-qtin my$PATH.
Agree this behavior is potentially confusing, and prevented it for now, but I’m not totally I sure see it as a downside. I like the simplicity of bitcoin daemon being an alias for bitcoind and for the bitcoin wrapper to add a few additional search directories of its own but not otherwise care where other executables are installed on the PATH.
But for now added a check to avoid searching for other executables using the PATH if the bitcoin executable was not invoked using the PATH. Probably will need to keep tweaking this behavior, seeing what works and maybe adding options to control it.
$PATH would be once the interfaces are stable and “public”, at which point folks might install e.g. an alternative GUI implementation.
Concept ACK
I think it would be more clear to move build/src/bitcoin-{node,gui} to build/src/libexec, rather than use a different file organization for CMake builds than for installs.
The “Win64 native, VS 2022” job still seems unhappy.
The “Win64 native, VS 2022” job still seems unhappy.
To turn off deprecation warnings for these functions, define the preprocessor macro
_CRT_NONSTDC_NO_WARNINGS. You can define this macro at the command line by including the option/D_CRT_NONSTDC_NO_WARNINGS.
We have already used this macro:https://github.com/bitcoin/bitcoin/blob/7590e93bc73b3bbac641f05d490fd5c984156b33/cmake/leveldb.cmake#L87-L89
Thanks for the reviews!
Updated da108a6e5be220654a65b6613ee7eb2c4ddc8677 -> 02567bf2bef5997ea5f0765780d196f36d3053e8 (pr/wrap.5 -> pr/wrap.6, compare) to fix windows build warning and making a change to avoid a potentially confusing behavior #31375 (review)
re: #31375#pullrequestreview-2467341532
I think it would be more clear to move
build/src/bitcoin-{node,gui}tobuild/src/libexec, rather than use a different file organization for CMake builds than for installs.
Agree and I think #31161 should allow this to be simplified.
re: #31375 (comment)
We have already used this macro:
It seems like it would be better if this code could be compiled without disabling warnings, especially since if the old names are being deprecated. For now I just added a #define to switch to the recommended name. For leveldb it probably does make sense to disable the warnings to avoid needing to change the code too much.
For reference the CI failure is:
0 subprocess.CalledProcessError: Command '['C:\\hostedtoolcache\\windows\\Python\\3.12.7\\x64\\python.exe', 'D:/a/bitcoin/bitcoin\\contrib\\signet\\miner', "--cli='D:\\a\\bitcoin\\bitcoin\\build\\src\\Release\\bitcoin-cli.exe' -nonamed '-datadir=D:\\a\\_temp\\test_runner_₿_🏃_20241203_223434\\tool_signet_miner_211\\node0'", 'generate', '--address=tb1q2ndfasp67k5wp30fkt63tw9gf465lcjf7rm5fc', "--grind-cmd='D:\\a\\bitcoin\\bitcoin\\build\\src\\Release\\bitcoin-util.exe' grind", '--nbits=1d00ffff', '--set-block-time=1733265821', '--poolnum=99']' returned non-zero exit status 1.
Earlier it says:
0FileNotFoundError: [WinError 2] The system cannot find the file specified
pr/wrap.6 -> pr/wrap.7, compare) to fix argument parsing bug causing a test failure on windows https://github.com/bitcoin/bitcoin/actions/runs/12147886951/job/33875123699?pr=31375#step:12:95
Intended to make bitcoin command line features more discoverable and allow
installing new multiprocess binaries in libexec/ instead of bin/ so they don't
cause confusion.
Idea and implementation of this were discussed in
https://github.com/bitcoin/bitcoin/issues/30983
It's possible to debug this and see what programs it is trying to call by running:
strace -e trace=execve -s 10000 build/src/bitcoin ...
Support new BITCOIN_CMD environment variable in functional test to be able to
test the new bitcoin wrapper executable and run other commands through it
instead of calling them directly.
75@@ -76,6 +76,7 @@ list(APPEND CMAKE_MODULE_PATH ${PROJECT_SOURCE_DIR}/cmake/module)
76 #=============================
77 include(CMakeDependentOption)
78 # When adding a new option, end the <help_text> with a full stop for consistency.
79+option(BUILD_BITCOIN_BIN "Build bitcoin executable." ON)
Executables: in the summary as well?
Can you add this to
Executables:in the summary as well?
Good idea, added now
I cherry-picked the first commit onto #30975 in order to try a (arm64-apple-darwin) Guix build: https://github.com/Sjors/bitcoin/tree/2024/12/multiprocess-guix
This also required adding it to installable targets and Maintenance.cmake (maybe not needed for this PR).
There was a brief discussion on IRC yesterday about whether anything special needs to happen in terms of macOS code signing. The conclusion was: probably not.
https://bitcoin-irc.chaincode.com/bitcoin-core-dev/2024-12-12#1074526
On my M4 I had to codesign -s - ... the binaries from my Guix build just like before, which suggests nothing special is required.
I wrote:
I think it would be more clear to move
build/src/bitcoin-{node,gui}tobuild/src/libexec, rather than use a different file organization for CMake builds than for installs.
But I just realized that the bitcoin binary would be installed to /usr/bin whereas new binaries like bitcoin-node and bitcoin-mine would go to /usr/libexec.
To match this behavior in development mode we could use build/bin and build/libexec. As you mentioned #31161 should allow this. But there’s no need to wait for that.
I sometimes install a user-specific node using -DCMAKE_INSTALL_PREFIX=$HOME. By default $HOME/bin is in the PATH on Ubuntu, but $HOME/libexec isn’t. Looking at f17453ff13ebd0be842396c17bcc99b0c903abb8 it handles this nicely; if the bitcoin executable lives under bin it will try ../libexec.
89@@ -90,6 +90,10 @@ static inline bool exists(const path& p)
90 {
91 return std::filesystem::exists(p);
92 }
93+static inline bool exists(const path& p, std::error_code& ec)
noexcept? https://en.cppreference.com/w/cpp/filesystem/exists
re: #31375 (review)
f17453f nit: can be
noexcept? https://en.cppreference.com/w/cpp/filesystem/exists
Makes sense, added
28+ {gui,daemon,rpc,wallet,test,help}
29+
30+Options:
31+ -m, --multiprocess Run multiprocess binaries bitcoin-node, bitcoin-gui.\n"
32+ -M, --monolothic Run monolithic binaries bitcoind, bitcoin-qt. (Default behavior)\n"
33+ -v, --version Show version information
bitcoin itself too. It currently prints the license, but not the version.
re: #31375 (review)
f17453f nit: would be nice if this worked for
bitcoinitself too. It currently prints the license, but not the version.
Oops, good catch. This should be fixed now
82+ } else if (cmd.command == "test") {
83+ args.emplace_back("test/test_bitcoin");
84+ } else if (cmd.command == "help") {
85+ tfm::format(std::cout, HELP_COMMANDS, argv[0]);
86+ } else if (cmd.command == "mine") { // undocumented, used by tests
87+ args.emplace_back("bitcoin-mine");
re: #31375 (review)
That’s right. It’s just a little easier for me to manage my branches if this is included so the PRs don’t conflict, and it shouldn’t cause any harm.
84+ } else if (cmd.command == "help") {
85+ tfm::format(std::cout, HELP_COMMANDS, argv[0]);
86+ } else if (cmd.command == "mine") { // undocumented, used by tests
87+ args.emplace_back("bitcoin-mine");
88+ } else if (cmd.command == "util") { // undocumented, used by tests
89+ args.emplace_back("bitcoin-util");
bitcoin-util has a grind command which is also used for (custom) signet, see signet/README.md. Doesn’t need to be hidden imo.
re: #31375 (review)
f17453f:
bitcoin-utilhas agrindcommand which is also used for (custom) signet, seesignet/README.md. Doesn’t need to be hidden imo.
Forwarding the util command allows tests to work with minimal changes, but I don’t think having bitcoin util grind command would be a good public interface. I think it would be better to expose a bitcoin grind command that doesn’t require callers to know about underlying binaries. Doing this will require some tweaks to ArgsManager though to separate commands & options correctly so I postponed it for now.
I think it would be better to expose a bitcoin grind command that doesn’t require callers to know about underlying binaries
Ok, that makes sense.
93+ if (!args.empty()) {
94+ args.insert(args.end(), cmd.args.begin(), cmd.args.end());
95+ ExecCommand(args, argv[0]);
96+ }
97+ } catch (const std::exception& e) {
98+ tfm::format(std::cerr, "Error: %s\nTry '%s --help' for more information.", e.what(), argv[0]);
f17453ff13ebd0be842396c17bcc99b0c903abb8: on macOS 15.1.1 this somehow puts a % at the end of the error message, e.g.
0% build-ipc/src/bitcoin utillll
1Error: Unrecognized command: 'utillll'
2Try 'build-ipc/src/bitcoin --help' for more information.%
(the Unknown option error below does not have this issue)
re: #31375 (review)
f17453f: on macOS 15.1.1 this somehow puts a
%at the end of the error message, e.g.
Good catch, that just comes for forgetting to output a trailing newline. Should be fixed now.
131+// the path to this executable is specified `argv0`.
132+//
133+// This function doesn't currently print anything but can be debugged from
134+// the command line using strace like:
135+//
136+// strace -e trace=execve -s 10000 build/src/bitcoin ...
f17453ff13ebd0be842396c17bcc99b0c903abb8: this seems near impossible on macOS. With htop I see that bitcoin daemon causes bitcoind to run, and there’s no lingering bitcoin process. The terminal window process also shows this.
re: #31375 (review)
f17453f: this seems near impossible on macOS. With
htopI see thatbitcoin daemoncausesbitcoindto run, and there’s no lingeringbitcoinprocess. The terminal window process also shows this.
Do you think I should change the comment in some way? The exec behavior you’re describing is expected, and strace comment is just meant to be helpful on platforms where strace works.
In the future we could add more debugging options here that would be more cross-platform than strace, but I’m trying to keep things simple in V1.
The following works on FreeBSD. In case dtrace works on macOS:
0dtrace -n 'proc:::exec-success { trace(curpsinfo->pr_psargs); }' -c 'bitcoin -m gui'
Maybe how to enable dtrace on macOS: https://github.com/suolapeikko/dtrace?tab=readme-ov-file#sample-dtrace-scripts-for-macos
0% dtrace -n 'proc:::exec-success { trace(curpsinfo->pr_psargs); }' -c 'build/src/bitcoin daemon'
1dtrace: system integrity protection is on, some features will not be available
2dtrace: failed to initialize dtrace: DTrace requires additional privileges
With sudo:
0dtrace: system integrity protection is on, some features will not be available
1
2dtrace: invalid probe specifier proc:::exec-success { trace(curpsinfo->pr_psargs); }: probe description proc:::exec-success does not match any probes. System Integrity Protection is on
I haven’t tried disabling System Integrity Protection (SIP) as that link suggests, not necessary a good idea :-) Though the maybe the csrutil enable --without dtrace isn’t too bad.
183+ exe_dir = fs::path{module_path};
184+ } else {
185+ tfm::format(std::cerr, "Warning: Failed to get executable path. Error: %s\n", GetLastError());
186+ }
187+#endif
188+ exe_dir = fs::weakly_canonical(exe_dir, ec).parent_path();
ec is set?
re: #31375 (review)
f17453f: shouldn’t we throw here if
ecis set?
I don’t think we actually want to throw when symlinks can’t be resolved. Should be more robust to just search for executables relative to the original path in that case. But error handling here was not right, because weakly_canonical would return an empty path on failure, so that should be fixed now.
Mostly happy with 365ac72dc7629765afb85bb9cc7c53d153c7c034.
I didn’t test and review Windows code. I also don’t really understand the test changes in a3b92c1f46c14c5c8db1f2833fd869d8a6a7ad4f, hopefully @maflcko does. But it’s useful that we use the wrapper in 365ac72dc7629765afb85bb9cc7c53d153c7c034.
Thanks for the review and testing, really helpful feedback!
Updated 365ac72dc7629765afb85bb9cc7c53d153c7c034 -> 128e110ed7af5f95d2bd16c36eca93c7c9b03573 (pr/wrap.9 -> pr/wrap.10, compare) fixing version and help formatting and weakly_canonical error handling.
Updated 128e110ed7af5f95d2bd16c36eca93c7c9b03573 -> 044c1129db06983da598f427dff85513d8480b3a (pr/wrap.10 -> pr/wrap.11, compare) to fix lint failure https://cirrus-ci.com/task/5251623682834432
🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/34561550110
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.
29+Commands (run help command for more information):
30+ {gui,daemon,rpc,wallet,test,help}
31+
32+Options:
33+ -m, --multiprocess Run multiprocess binaries bitcoin-node, bitcoin-gui.\n"
34+ -M, --monolothic Run monolithic binaries bitcoind, bitcoin-qt. (Default behavior)\n"
typo:
0 -M, --monolithic Run monolithic binaries bitcoind, bitcoin-qt. (Default behavior)\n"
41+%1$s gui [ARGS] Start GUI, equivalent to running 'bitcoin-qt [ARGS]' or 'bitcoin-gui [ARGS]'.
42+%1$s daemon [ARGS] Start daemon, equivalent to running 'bitcoind [ARGS]' or 'bitcoin-node [ARGS]'.
43+%1$s rpc [ARGS] Call RPC method, equivalent to running 'bitcoin-cli -named [ARGS]'.
44+%1$s wallet [ARGS] Call wallet command, equivalent to running 'bitcoin-wallet [ARGS]'.
45+%1$s tx [ARGS] Manipulate hex-encoded transactions, equivalent to running 'bitcoin-tx [ARGS]'.
46+%1$s test [ARGS] Run unit tests, equivalent to running 'test_bitcoin [ARGS]'.
test does not belong here - it is not for users to execute after the installation. test_bitcoin may not even be installed. The tests are normally run either by developers or by users after compile and before install, to verify the result of the compilation is correct.
test_bitcoin - for whatever reason.
36+ -h, --help Show this help message
37+)";
38+
39+static constexpr auto HELP_COMMANDS = R"(Command overview:
40+
41+%1$s gui [ARGS] Start GUI, equivalent to running 'bitcoin-qt [ARGS]' or 'bitcoin-gui [ARGS]'.
So, the user must have a knowledge whether his Bitcoin Core was compiled with or without multiprocess and start either bitcoin -M gui or bitcoin -m gui. Could we spare them this? The bitcoin executable has that knowledge.
Then the user can run just bitcoin gui and under the hood either bitcoin-qt or bitcoin-gui will be executed, depending on how it was compiled? Or maybe we can probe at run time whether the executables bitcoin-qt and bitcoin-gui exist and run whichever is present.
The goal of #31098 is to have bitcoin-node included in the release. For the time being only stratum v2 (or other IPC mining interface consumers) should use that binary. And they will get instructions for that. By default folks should run bitcoind.
Once the multiprocess bundled binaries are mature enough and there’s a broader benefit to using them (e.g. the GUI can connect to the node), we could change the default.
88+ tfm::format(std::cout, HELP_COMMANDS, argv[0]);
89+ } else if (cmd.command == "mine") { // undocumented, used by tests
90+ args.emplace_back("bitcoin-mine");
91+ } else if (cmd.command == "util") { // undocumented, used by tests
92+ args.emplace_back("bitcoin-util");
93+ } else if (!cmd.command.empty()){
0 } else if (!cmd.command.empty()) {
70+ tfm::format(std::cout, HELP_USAGE, argv[0]);
71+ }
72+
73+ std::vector<std::string> args;
74+ if (cmd.command == "gui") {
75+ args.emplace_back(cmd.use_multiprocess ? "qt/bitcoin-gui" : "qt/bitcoin-qt");
Since everything inserted into args is a constant literal, it can be std::vector<std::string_view> and then also ExecCommand() can be:
0- void ExecCommand(const std::vector<std::string>& args, ...
1+ void ExecCommand(const std::vector<std::string_view>& args, ...
96+ if (!args.empty()) {
97+ args.insert(args.end(), cmd.args.begin(), cmd.args.end());
98+ ExecCommand(args, argv[0]);
99+ }
100+ } catch (const std::exception& e) {
101+ tfm::format(std::cerr, "Error: %s\nTry '%s --help' for more information.\n", e.what(), argv[0]);
This adds a trailing newline, but the above 2 tfm::format() calls don’t. I think they should.
I think the tfm::format(..., FormatParagraph(LicenseInfo())) call adds a trailing \n because LicenseInfo() has it, but I did not check.
127+ }
128+ }
129+ return cmd;
130+}
131+
132+// Execute the specified bitcoind, bitcoin-qt or other command line in `args`
ExecCommand(). Would be nice to format the arguments’ descriptions with @param[in] ....
135+//
136+// This function doesn't currently print anything but can be debugged from
137+// the command line using strace like:
138+//
139+// strace -e trace=execve -s 10000 build/src/bitcoin ...
140+void ExecCommand(const std::vector<std::string>& args, std::string_view argv0)
argv0 args[0] args[1] args[2] .... Maybe rename argv0 to parent or wrapper_prog.
195+ exe_dir = exe_dir.parent_path();
196+ // Search for executables on system PATH only if this executable was invoked
197+ // from the PATH, to avoid unintentionally launching system executables in a
198+ // local build
199+ // (https://github.com/bitcoin/bitcoin/pull/31375#discussion_r1861814807)
200+ const bool use_system_path{!argv0_path.has_parent_path()};
argv0.find('/') == std::string_view::npos, right? Would be good to use the same expression in both locations.
205+ (exe_dir.filename() == "src" && try_exec(exe_dir / arg0)) ||
206+ // Otherwise if exe is installed in a bin/ directory, first look for target
207+ // executable in libexec/
208+ (exe_dir.filename() == "bin" && try_exec(fs::path{exe_dir.parent_path()} / "libexec" / arg0.filename())) ||
209+ // Otherwise look for target executable next to current exe
210+ try_exec(exe_dir / arg0.filename(), use_system_path) ||
Why use such an ( && || && ||| ) expression? Would be more readable as:
0if (exe_dir.filename() == "src") {
1 try_exec(exe_dir / arg0);
2}
3if (exe_dir.filename() == "bin") {
4 try_exec(fs::path{exe_dir.parent_path()} / "libexec" / arg0.filename());
5}
171+ if (const char* path_env = std::getenv("PATH")) {
172+ size_t start{0}, end{0};
173+ for (std::string_view paths{path_env}; end != std::string_view::npos; start = end + 1) {
174+ end = paths.find(':', start);
175+ fs::path candidate = fs::path(paths.substr(start, end - start)) / argv0_path;
176+ if (fs::exists(candidate, ec) && fs::is_regular_file(candidate, ec)) {
fs::exists() redundant? fs::is_regular_file() will return true if the file exists and is a regular file.
290@@ -260,7 +291,12 @@ def set_binary_paths(self):
291 "src",
292 binary + self.config["environment"]["EXEEXT"],
293 )
294- setattr(self.options, attribute_name, os.getenv(env_variable_name, default=default_filename))
295+ setattr(paths, attribute_name, os.getenv(env_variable_name, default=default_filename))
296+
297+ # BITCOIN_CMD environment variable can be specified to invoke bitcoin
298+ # binary wrapper binary instead of other executables.
0 # BITCOIN_CMD environment variable can be specified to invoke bitcoin
1 # wrapper binary instead of other executables.