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.
| Type | Reviewers |
|---|---|
| Concept ACK | TheCharlatan |
| Approach ACK | vasild |
| Stale ACK | Sjors |
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
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"
re: #31375 (review)
Thanks, fixed
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.
re: #31375 (review)
Removed from documentation for now since maybe this isn’t something we want to support going forward, and in any case I think it would be better to document internal commands for debugging and testing and external commands intended to be used by typical users separately.
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.
re: #31375 (review)
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.
No this isn’t how it works. Specifying -m or -M is optional, and the current default is to run monolithic binaries. Multiprocess binaries could become the default at some some point in the future but presumably both sets of binaries would still be installed at that point.
I can’t think of a situation where users would encounter a problem or see confusing behavior here, but if I’m missing something, we could implement more complicated behavior like you are suggesting. I don’t see what problem that would solve right now, though.
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()) {
re: #31375 (review)
Thanks, fixed now
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, ...
re: #31375 (review)
Since everything inserted into
argsis a constant literal, it can bestd::vector<std::string_view>
Thanks, I went ahead and changed everything to const char* for now. Unfortuantely std::string_view does not work because these arguments need to be null-terminated in order to be passed to execvp.
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.
re: #31375 (review)
This adds a trailing newline, but the above 2
tfm::format()calls don’t. I think they should.
The output looks right to me currently and trailing newlines are not missing. Adding \n to format strings would add double line breaks to the end of output and not be right, and in general there should be no need to include \n in format strings after substituting variables that already end in newlines.
If the idea is to do some refactoring to move newlines from certain strings into other strings, I could do that, but would helpful to know what goal of refactoring would be. Maybe post a diff if you have a particular change in mind.
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] ....
re: #31375 (review)
Thanks, added doxygen documentation and descriptions.
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.
re: #31375 (review)
I find the names confusing because I would assume this will execute
argv0 args[0] args[1] args[2] .... Maybe renameargv0toparentorwrapper_prog.
Makes sense, and nice suggestion. renamed to wrapper_argv0. Also renamed other related variables to use wrapper_ prefix to be consistent.
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.
re: #31375 (review)
I think this is the same check as above:
argv0.find('/') == std::string_view::npos, right? Would be good to use the same expression in both locations.
Nice catch! Consolidated these
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}
re: #31375 (review)
Why use such an
( && || && ||| )expression? Would be more readable as:
The suggested code is not equivalent unless you assume that try_exec will never return true. In practice, I agree it should work but it would be fragile and look misleading because unless you know that try_exec does a syscall that never returns in certain cases, a plain reading of the code would lead you to believe incorrectly that the return value of try_exec doesn’t matter and that the same executable is run repeatedly.
Using && and || is more explicit and robust because it makes it explicit that if try_exec succeeds, it is not run again, and it makes the code work regardless of whether try_exec halts or returns.
If the boolean operators are a problem, maybe I can try to find another way to write this doesn’t look misleading and rely on try_exec to never return. But IMO, && and || are nice because these operators are a natural convenience in shell scripts and this wrapper program basically is a shell script.
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.
re: #31375 (review)
Isn’t
fs::exists()redundant?fs::is_regular_file()will return true if the file exists and is a regular file.
Good catch, simplified.
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.
re: #31375 (review)
Thanks fixed
106+
107+CommandLine ParseCommandLine(int argc, char* argv[])
108+{
109+ CommandLine cmd;
110+ cmd.args.reserve(argc);
111+ for (int i = 1; i < argc; ++i) {
If I understand correctly passing -m -M will give precedence to -M. Should we really support this? Might it better if providing multiple flags at once be an error? E.g. currently running
0./bitcoin -h -m daemon
will print the help string and run the daemon, which might be confusing, since it is not doing the same for -v.
re: #31375 (review)
If I understand correctly passing
-m -Mwill give precedence to-M. Should we really support this? Might it better if providing multiple flags at once be an error? E.g. currently running0./bitcoin -h -m daemonwill print the help string and run the daemon, which might be confusing, since it is not doing the same for
-v.
Good catch on -h, that should be be fixed now and treated the same as -v and exit when done. In general though I think ability for later command line options to override earlier ones is a standard feature for command line tools and it makes them flexible and composable. For example, in a bash shell you can define a function or alias that calls bitcoin -m to invoke multiprocess binaries by default without losing the ability to append -M and run normal binaries when needed.
I guess bitcoin is intentionally not added to installable_targets because it makes reference to the multiprocess binaries that aren’t shipped?
I made a Windows guix build for 044c1129db06983da598f427dff85513d8480b3a. It got misidentified as Trojan:Script/Wavatac.B1ml again. But since the bitcoin binary isn’t included yet, it can’t be because of the execvp call.
Just to be sure, I also tried a guix build for master @ 62bd61de110b057cbfd6e31e4d0b727d93119c72 that this PR is based on. That didn’t trigger the anti-virus. But maybe it’s just random.
In order to better test on Windows I used this: 26d3a069f94e2cabcc8d1f73fd2cfa3e7c320497. The installer puts bitcoin.exe in C:\Program Files\Bitcoin\daemon. Running it without arguments will show the help. Starting the daemon fails with “Error: Command line contains unexpected token ‘Files\Bitcoin\daemon\bitcoind’” so it seems to trip over the space. It also can’t find bitcoin-qt.exe, which lives in Bitcoin\ instead of daemon\. If I move it to a directory without space, I’m able to start the daemon and I’m able to stop the daemon using bitcoin.exe rpc stop. If I move the bitcoin-qt executable I’m also able to start the GUI. No complaints from virus scanners this time.
Thanks for the reviews and testing!
Rebased 044c1129db06983da598f427dff85513d8480b3a -> 17f25149c680c1fdb2f79dbe373e8cf820e62488 (pr/wrap.11 -> pr/wrap.12, compare) to fix conflicts, also made many suggested updates and some fixes, particularly for windows.
Updated 17f25149c680c1fdb2f79dbe373e8cf820e62488 -> 3d46df16e9cf4a960fedde9e515667030b39bb03 (pr/wrap.12 -> pr/wrap.13, compare) to fix python test bugs after bad rebase
Updated 3d46df16e9cf4a960fedde9e515667030b39bb03 -> c11f7e7336b4a5ea6a8f22dc40dcae1b3ddd81e2 (pr/wrap.13 -> pr/wrap.14, compare) with documentation cleanups
re: #31375 (comment)
I guess
bitcoinis intentionally not added toinstallable_targetsbecause it makes reference to the multiprocess binaries that aren’t shipped?
No that was just an oversight, it should be added now. The wrapper tool is meant to be useful in developer builds and source builds not just official binaries releases, so it doesn’t need to be tied to current options we are building binary releases with.
Starting the daemon fails with “Error: Command line contains unexpected token ‘Files\Bitcoin\daemon\bitcoind’” so it seems to trip over the space. It also can’t find
bitcoin-qt.exe, which lives inBitcoin\instead ofdaemon\. If I move it to a directory without space, I’m able to start the daemon and I’m able to stop the daemon usingbitcoin.exe rpc stop. If I move the bitcoin-qt executable I’m also able to start the GUI. No complaints from virus scanners this time.
Thanks for all the windows testing, this is very helpful. I made attempted fixes for all these issues so we should be able to iterate and resolve them. I added your windows installer commit to this PR with a small tweak to put bitcoin.exe in program files directory instead of the daemon directory, then changed bitcoin.cpp to look for executables in the daemon directory and implemented command line argument escaping for execvp on windows since the windows execvp function doesn’t provide that
On Windows it’s a bit ugly to show the full path:
None of the commands work anymore though, except version and help. The commands daemon and gui do not start anything. When I start the GUI manually through the desktop item, the command bitcoin rpc help doesn’t return anything.
This also doesn’t return anything: .\bitcoin.exe daemon --help
I would expect trying to run the multiprocess versions to throw an error, but nothing happens when I do .\bitcoin.exe daemon -m.
Everything seems to be in place though:
0PS C:\Program Files\Bitcoin> ls
1
2
3 Directory: C:\Program Files\Bitcoin
4
5
6Mode LastWriteTime Length Name
7---- ------------- ------ ----
8d----- 1/20/2025 11:10 AM daemon
9d----- 1/20/2025 11:10 AM share
10-a---- 1/20/2025 11:10 AM 39362576 bitcoin-qt.exe
11-a---- 1/20/2025 11:10 AM 126 bitcoin.conf
12-a---- 1/20/2025 11:10 AM 1221120 bitcoin.exe
13-a---- 1/20/2025 11:10 AM 1142 COPYING.txt
14-a---- 1/20/2025 11:10 AM 846 readme.txt
15-a---- 1/20/2025 11:10 AM 175560 uninstall.exe
16
17ls daemon
18 Directory: C:\Program Files\Bitcoin\daemon
19
20Mode LastWriteTime Length Name
21---- ------------- ------ ----
22-a---- 1/20/2025 11:10 AM 2249728 bitcoin-cli.exe
23-a---- 1/20/2025 11:10 AM 4061184 bitcoin-tx.exe
24-a---- 1/20/2025 11:10 AM 9435648 bitcoin-wallet.exe
25-a---- 1/20/2025 11:10 AM 15302656 bitcoind.exe
26-a---- 1/20/2025 11:10 AM 26467840 test_bitcoin.exe
Updated c11f7e7336b4a5ea6a8f22dc40dcae1b3ddd81e2 -> 0b503d792f0b6255f6dc5b0e4ee7ce2b7b667e90 (pr/wrap.14 -> pr/wrap.15, compare) with fixes for windows.
re: #31375 (comment)
None of the commands work anymore though, except version and help
Thanks, should be fixed now. After struggling with cross compilation and qemu a bit I was able to reproduce this. There was a bug in the ExecVp argv loop that caused string_view to be constructed with a null pointer and for the program to crash. I was a little surprised it would crash without an error, but maybe this is what happens by default if you deference a null pointer. There was also another bug in the same loop that caused arguments to be dropped. Both bugs should be fixed now and now it seems to work as expected and handle arguments with spaces correctly on windows.
On Windows it’s a bit ugly to show the full path:
I could reproduce this with powershell, but I was not seeing this when I tested with cmd.exe. So this behavior seems to vary with the shell, and it looks like this only happens with powershell, not with the classic windows shell or other shells like bash. For these other shells, I think it is probably better to show unmodified argv[0] in usage information instead of some modified or static alternative. But we could do something different in the future, like maybe show original argv[0] path in the Usage: string, and basename of that path in the command overview list.
After struggling with cross compilation and qemu a bit I was able to reproduce this.
I just have a dual boot system and configured sshd on it so I can quickly scp the windows installer from my guix machine.
it looks like this only happens with powershell, not with the classic windows shell
In that case I wouldn’t worry about it.
Just tested: 0b503d792f0b6255f6dc5b0e4ee7ce2b7b667e90 works!
8@@ -9,6 +9,7 @@ import logging
9 import math
10 import os
11 import re
12+import shlex
13 import struct
struct is unused
71+ """
72+ def __init__(self, paths, bin_path=None):
73+ self.paths = paths
74+ self.bin_path = bin_path
75+
76+ def daemon_args(self):
daemon_command_with_args or daemon_argv would make the call sites more readable. Right now it gives the impression that this only provides command arguments, not the command itself. OTOH the documentation does clarify it.
re: #31375 (review)
a1d960e: I think renaming this to
daemon_command_with_argsordaemon_argvwould make the call sites more readable. […]
Makes sense, renamed to daemon_argv, and renamed other methods to match
88+
89+ def wallet_args(self):
90+ "Return argv array that should be used to invoke bitcoin-wallet"
91+ return self.args("wallet", "bitcoinwallet")
92+
93+ def args(self, command, path_attr):
a1d960e03be53517e80d36c257941ff407a6f61f: it took me a while to wrap my head around this. Suggested doc:
Return argv array that should be used to invoke the command. It either uses the bitcoin wrapper executable (if BITCOIN_CMD is set), or the direct binary path (bitcoind, etc). When bin_path is set (by tests calling binaries from previous releases) it always uses the direct path.
(in the future some of the previous releases will have the bitcoin binary, but I don’t expect we’ll need to use it in tests)
re: #31375 (review)
a1d960e: it took me a while to wrap my head around this. Suggested doc:
Nice, added suggested doc.
tACK 0b503d792f0b6255f6dc5b0e4ee7ce2b7b667e90
Also tested on macOS.
The logic in ExecCommand() is easier to follow compared to my previous review.
I did not study the Windows ExecVp implementation in f006565ad220154c6e402c99074d326699d62d3e. I don’t expect anyone to use it until Windows multiprocess works (and is in a release). It would be good to call the wrapper from the Windows CI.
I tried Guix building this branch:
0HOSTS="x86_64-linux-gnu" ./contrib/guix/guix-build
1Consolidate compiler generated dependencies of target bitcoin-qt
2[100%] Built target bitcoin-qt
3Checking binary security...
4/distsrc-base/distsrc-0b503d792f0b-x86_64-linux-gnu/build/src/bitcoin: failed FORTIFY
5make[3]: *** [CMakeFiles/check-security.dir/build.make:71: CMakeFiles/check-security] Error 1
6make[2]: *** [CMakeFiles/Makefile2:565: CMakeFiles/check-security.dir/all] Error 2
7make[1]: *** [CMakeFiles/Makefile2:572: CMakeFiles/check-security.dir/rule] Error 2
8make: *** [Makefile:270: check-security] Error 2
I had only tried the Windows guix build. Getting the same error as @fanquake with x86_64-linux-gnu.
Can also be reproduced on Ubuntu 24.10 with:
0pip install lief==0.13.2
1contrib/devtools/security-check.py build/src/bitcoin
Adding this to security-check.py works, similar to bitcoin-util:
0 # bitcoin does not currently contain any fortified functions
1 if '--monolithic' in binary.strings:
2 return True
Not sure if it’s correct though.
re: #31375 (comment)
Adding this to
security-check.pyworks, similar tobitcoin-util:
I think that makes sense. The new executable is only using c++ vector/string functions and calling execvp so it is probably just not calling any fortified functions, and won’t have any fortified function symbols, IIUC.
fortified functions
I have no idea what those look like.
Rebased 0b503d792f0b6255f6dc5b0e4ee7ce2b7b667e90 -> 171ffdd10e41df9e06ec3f6ef52f1cbbfcaf1748 (pr/wrap.15 -> pr/wrap.16, compare) to avoid conflict. Also added suggestions and security check fix. Additionally made some tweaks like using path::filename() method to avoid long paths #31375 (comment), and adding documentation for internal commands not shown by default.
Updated 171ffdd10e41df9e06ec3f6ef52f1cbbfcaf1748 -> 5e0196c8e0d1687bcf59a9569bb8b7268d1f195d (pr/wrap.16 -> pr/wrap.17, compare) to fix windows compile error https://cirrus-ci.com/task/5239580141551616
fortified functions
I have no idea what those look like.
I know very little about this but I believe they just have a _chk suffix: https://www.gnu.org/software/libc/manual/html_node/Source-Fortification.html
🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/36101379316
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.
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
Co-authored-by: Sjors Provoost <sjors@sprovoost.nl>
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.
Co-authored-by: Sjors Provoost <sjors@sprovoost.nl>
pr/wrap.17 -> pr/wrap.18, compare) to fix conflict
I wonder if python test framework changes here are scaring reviewers from this PR? I potentially could drop the framework integration here and move it into a different PR. Maybe add just a more limited test here instead.
Would be good to know either way.