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 also includes python test changes which are identical to changes in #31866 and could be reviewed there to simplify this PR.
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.
-m|-M is not mandatory and bitcoin gui has a reasonable default of running bitcoin-qt (as if bitcoin -M gui has been run). Makes sense, thanks for the explanation!
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.
Ok, I see your arguments. Feel free to ignore this comment of mine and leave the code as it is. I do not see it as a blocker.
Yes, I assumed that try_exec() will never return true and I guessed that try_exec() does an exec syscall without reading its code. Both Windows and Linux docs are clear that execvp() only returns if it couldn’t execute the program. try_exec() could be:
0 auto try_exec = [&](fs::path exe_path, bool allow_notfound = true) {
1 std::string exe_path_str{fs::PathToString(exe_path)};
2 exec_args[0] = exe_path_str.c_str();
3- if (ExecVp(exec_args[0], (char*const*)exec_args.data()) == -1) {
4- if (allow_notfound && errno == ENOENT) return false;
5+ ExecVp(exec_args[0], (char*const*)exec_args.data());
6+ // failed
7+ if (!allow_notfound || errno != ENOENT) {
8 throw std::system_error(errno, std::system_category(), strprintf("execvp failed to execute '%s'", exec_args[0]));
9 }
10- return true; // Will not actually be reached if execvp succeeds
11+ return false;
12 };
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.
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.
239+ // Whether to use system PATH variable to locate wrapper executable and
240+ // search for other executables. Only search PATH if wrapper executable was
241+ // invoked using the PATH, to avoid unintentionally launching system
242+ // executables in a local build.
243+ // (https://github.com/bitcoin/bitcoin/pull/31375#discussion_r1861814807)
244+ const bool search_system_path{!wrapper_argv0_path.has_parent_path()};
C:\bitcoin\src (which is not in PATH) and the user types bitcoin (not .\bitcoin) and presses <enter> and if C:\bitcoin\src\bitcoin exists then it will be executed. This means that the variable search_system_path will be wrongly set to true in this case.
re: #31375 (review)
(note: renamed this variable to fallback_os_search in the latest push)
This means that the variable search_system_path will be wrongly set to true in this case.
It will be set to true, but I don’t think it will be “wrongly” set to true. This is a heuristic deciding whether or not to let operating system look for executables if we can’t find them ourselves. It’s very possible different behaviors may be more or less convenient on windows, just as they may be more or less convenient on unix.
We need to do something reasonable here, and if it turns out there are ways we can improve the behavior or make it more configurable we should consider doing that. But you would have to tell me what behavior you think is more convenient here because there is not a right or wrong way to do this.
210+//! @note This function doesn't currently print anything but can be debugged
211+//! from the command line using strace or dtrace like:
212+//!
213+//! strace -e trace=execve -s 10000 build/src/bitcoin ...
214+//! dtrace -n 'proc:::exec-success { trace(curpsinfo->pr_psargs); }' -c ...
215+void ExecCommand(const std::vector<const char*>& args, std::string_view wrapper_argv0)
Is only used in this file:
0static void ExecCommand(const std::vector<const char*>& args, std::string_view wrapper_argv0)
re: #31375 (review)
Thanks, applied suggestion
281+ // Otherwise check the "daemon" subdirectory in a windows install.
282+ try_exec(wrapper_dir / "daemon" / arg0.filename()) ||
283+ // Otherwise look for target executable next to current wrapper
284+ try_exec(wrapper_dir / arg0.filename(), search_system_path) ||
285+ // Otherwise just look on the system path.
286+ (search_system_path && try_exec(arg0.filename(), false));
This is equivalent to:
0 try_exec(wrapper_dir / arg0.filename(), search_system_path) ||
1 // Otherwise just look on the system path.
2 try_exec(arg0.filename(), false);
because try_exec() would only return if search_system_path is true and if that is the case then search_system_path && A is equivalent to just A.
This is equivalent to […] because
try_exec()would only return ifsearch_system_pathistrue
IMO, this code should work correctly even if try_exec() returns true when it succeeds. There’s no reason a future implementation of try_exec() might not return true when it succeeds or that someone reading this code should automatically know try_exec() doesn’t just act like a normal function and return true when it succeeds.
The current version of this code with search_system_path && also makes it more immediately obvious that the try_exec call following it will not be executed if search_system_path is false.
try_exec() could fork() and then exec() in which case it will return in case of success.
279+ // target executable in libexec/
280+ (wrapper_dir.filename() == "bin" && try_exec(fs::path{wrapper_dir.parent_path()} / "libexec" / arg0.filename())) ||
281+ // Otherwise check the "daemon" subdirectory in a windows install.
282+ try_exec(wrapper_dir / "daemon" / arg0.filename()) ||
283+ // Otherwise look for target executable next to current wrapper
284+ try_exec(wrapper_dir / arg0.filename(), search_system_path) ||
search_system_path is used as allow_notfound. Those are two different things.
search_system_path=true to try_exec() is misleading because try_exec() uses execvp(3) which will search the system path but not based on the search_system_path=true argument but based on whether there is / in the executable: https://linux.die.net/man/3/execvp. Maybe rename search_system_path to allow_notfound?
re: #31375 (review)
(note: renamed search_system_path to fallback_os_search in the latest push)
I don’t get it why
search_system_pathis used asallow_notfound. Those are two different things.
Exactly, they are two different things.
If search_system_path is true, then we want to allow the operating system to search the system path. In order for that to happen we need our own searches to not throw a fatal error, therefore allow_notfound should also be true.
If search_system_path is false, then do not want to allow the operating system to search the system path, and we want our own searches to throw a fatal error if they fail, so allow_notfound should be false.
So search_system_path and allow_notfound are two different variables in two different functions with two different meanings. Just because one value is used to initialize the other one (once) doesn’t mean they should have the same name.
I think I am starting to grasp: fallback_os_search (or previously search_system_path) is about allowing this code:
0181 // Decide whether to fall back to the operating system to search for the
1182 // specified executable. Avoid doing this if it looks like the wrapper
2183 // executable was invoked by path, rather than by search, to avoid
3184 // unintentionally launching system executables in a local build.
4185 // (https://github.com/bitcoin/bitcoin/pull/31375#discussion_r1861814807)
5186 const bool fallback_os_search{!fs::PathFromString(std::string{wrapper_argv0}).has_parent_path()};
6187
7188 // If wrapper is in a CMake build tree, first look for target executable
8189 // relative to it.
9190 (wrapper_dir.filename() == "src" && try_exec(wrapper_dir / arg0)) ||
10191 // Otherwise if wrapper is installed in a bin/ directory, look for
11192 // target executable in libexec/
12193 (wrapper_dir.filename() == "bin" && try_exec(fs::path{wrapper_dir.parent_path()} / "libexec" / arg0.filename())
13194 // Otherwise check the "daemon" subdirectory in a windows install.
14195 try_exec(wrapper_dir / "daemon" / arg0.filename()) ||
15196 // Otherwise look for target executable next to current wrapper
16197 try_exec(wrapper_dir / arg0.filename(), fallback_os_search) ||
17198 // Otherwise just look on the system path.
18199 (fallback_os_search && try_exec(arg0.filename(), false));
to reach the last try_exec() call on line 199 where it would search the system path because we pass a first argument that does not contain /. It is not about individual try_exec() calls falling back to $PATH search.
I still find line 197 confusing because it gives the impression that we want to instruct that individual try_exec() call to search $PATH if fallback_os_search is true, whereas it will never search $PATH because the first argument we supply contains /.
I think the code is correct and works as intended, even if I personally find it confusing. So, feel free to ignore.
46+
47+static constexpr auto HELP_INTERNAL = R"(
48+Additional internal commands:
49+ %1$s bench [ARGS] Run util command, equivalent to running 'bench_bitcoin [ARGS]'"
50+ %1$s test [ARGS] Run unit test, equivalent to running 'test_bitcoin [ARGS]'"
51+ %1$s test-gui [ARGS] Run unit test, equivalent to running 'test_bitcoin-qt [ARGS]'"
0 %1$s bench [ARGS] Run bench command, equivalent to running 'bench_bitcoin [ARGS]'"
1 %1$s test [ARGS] Run unit tests, equivalent to running 'test_bitcoin [ARGS]'"
2 %1$s test-gui [ARGS] Run GUI unit tests, equivalent to running 'test_bitcoin-qt [ARGS]'"
re: #31375 (review)
Thanks! Applied
198+//! using src, bin and libexec directory paths relative to this executable, where
199+//! the path to this executable is specified in `wrapper_argv0`.
200+//!
201+//! @param args Command line arguments to execute, where first argument should
202+//! be a relative path to a bitcoind, bitcoin-qt or other executable
203+//! that will be located on the PATH or relative to this executable.
0//! that will be located on the PATH or relative to wrapper_argv0.
re: #31375 (review)
Thanks! Applied
209+//
210+//! @note This function doesn't currently print anything but can be debugged
211+//! from the command line using strace or dtrace like:
212+//!
213+//! strace -e trace=execve -s 10000 build/src/bitcoin ...
214+//! dtrace -n 'proc:::exec-success { trace(curpsinfo->pr_psargs); }' -c ...
The dtrace example would print all exec calls on the system, not just the ones by the given program.
0//! dtrace -n 'proc:::exec-success /pid == $target/ { trace(curpsinfo->pr_psargs); }' -c ...
re: #31375 (review)
The
dtraceexample would print all exec calls on the system, not just the ones by the given program.
Wow previous behavior would not be great, applied your suggestion.
140+}
141+
142+//! Cross-platform wrapper for POSIX execvp function
143+int ExecVp(const char *file, char *const argv[])
144+{
145+#ifndef WIN32
I think the following two commits should be squashed into one commit:
bde036879d multiprocess: Add bitcoin wrapper executable
b7213977ed multiprocess: Add bitcoin wrapper windows support
because otherwise there is an intermediate state where the code is broken on Windows (would try to use execvp() which does not exist, I guess it will not compile on Windows).
re: #31375 (review)
intermediate state where the code is broken on Windows
Thanks, restructured so this is no longer the case. I didn’t think anyone would care if an intermediate non-merge commit failed to compile on windows and I thought code would be easier to understand looking at the unix version first. But in retrospect a better way to separate unix and windows code is just to move the windows code to another file, so have now done that in the latest push (src/util/exec.cpp)
git-bisect(1).
IIRC we had some guidelines somewhere that all intermediate commits should be legit - compiling and tests passing. This would matter during
git-bisect(1).
Yes, but the only enforcement is with the test-each-commit CI job, so I would be surprised if there weren’t many other intermediate commits where there is some build or test error on windows. Also:
In this case the error is in a new executable that didn’t exist before this PR, so if the bisect test is testing this executable., it means the test needs either some special case for this PR anyway, or it is inefficient and testing a lot of things it doesn’t need to.
The git bisect --first-parent option skips intermediate commits and avoids problems like this. I would always recommend using --first-parent anyway because it speeds up bisection, makes it easier to follow, and makes it more reliable because intermediate commits have less CI coverage and testing in general than merge commits.
In any case, I believe every commit will compile on windows now (though I didn’t test this).
Approach ACK 0e4ee158ca.
I wonder if python test framework changes here are scaring reviewers from this PR?
Moving the python changes to another PR would probably speed the review of this one and probably delay the review of the other one (with the python changes).
227+ throw std::system_error(errno, std::system_category(), strprintf("execvp failed to execute '%s'", exec_args[0]));
228+ }
229+ return true; // Will not actually be reached if execvp succeeds
230+ };
231+
232+ // Try to figure out where wrapper executable is located. This does a
execvp(). That is, reduce ExecCommand() to just 1 - 5 lines of code and deal with the consequences of it…
re: #31375 (review)
I wonder if this can be made dumb stupid and reduce it to just call
execvp(). That is, reduceExecCommand()to just 1 - 5 lines of code and deal with the consequences of it…
It’s not really clear what you could be suggesting specifically.
The only thing ExecCommand() is doing is trying to execute a few different versions of the provided command line with some prefixes added. It is basically just emulating a simple shell script. It is true that a simple shell script can turn into a 40-50 lines of code when you try to write it in c++ and make it cross platform. I think this is just what shell code in c++ looks like and that this should not present an actual problem.
Thanks for the reviews!
Rebased 0e4ee158cadc3eb8f6af1b33440ae9a95fe19487 -> edb8a72226b362ee60e4184fd8f56d1e588ce5b6 (pr/wrap.18 -> pr/wrap.19, compare) due to conflict with #31818
re: #31375#pullrequestreview-2612338457
Moving the python changes to another PR would probably speed the review of this one and probably delay the review of the other one (with the python changes).
Thanks, I guess for now it should be easy enough for reviewers not review the python changes if they aren’t interested and only ack the C++ ones.
These functions are just meant to serve the needs of the bitcoin wrapper
executable, and are intentionally not very general purpose so they can be
simple.
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>
Add new TestNode.binaries object to manage paths to bitcoin binaries.
Having this object makes it possible for the test framework to exercise the
bitcoin wrapper executable introduced in
https://github.com/bitcoin/bitcoin/pull/31375 and also makes it easier to add
new binaries and options and environment variables controlling how they are
invoked, because logic for invoking them that was previously spread out is now
consolidated in one place.
Co-authored-by: Sjors Provoost <sjors@sprovoost.nl>
Pass bitcoin binary command lines from test framework to signet/miner utility
using shell escaping so they are unambigous and don't get mangled if they
contain spaces.
This change is not needed for tests to pass currently, but is a useful change
to avoid CI failures in followup PR
https://github.com/bitcoin/bitcoin/pull/31375 and to avoid other bugs.
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.19 -> pr/wrap.20, compare) due to silent conflict with #31844
Updated 27bb348388a6708e8748d7073e352c047e8fb95c -> 6f0405177f89377ed272bae42becec1c75b8943c (pr/wrap.20 -> pr/wrap.21, compare) just splitting up test commits internally so they are easier to review
re-ACK 6f0405177f89377ed272bae42becec1c75b8943c.
Lightly reviewed the code changes since my previous review. It’s nice that ExecCommand is now a bit shorter, as a result of moving some helper functionality to src/util/exec.h.
The Python changes are also easier to follow now thanks to splitting the commit.
I made a guix build for 6f0405, but didn’t try including the IPC binaries with #31802 since there’s too many build system changes there. That will be easier to test after #31741 lands.
Retested a bit on Apple Silicon macOS and on Windows.
pr/wrap.21 -> pr/wrap.22, compare) just incorporating some python cleanups from #31866
11+
12+namespace util {
13+//! Cross-platform wrapper for POSIX execvp function.
14+int ExecVp(const char *file, char *const argv[]);
15+//! Return path to current_executable.
16+fs::path GetExePath(std::string_view argv0);
0//! Return path to argv0.
1fs::path GetExePath(std::string_view argv0);
re: #31375 (review)
Thanks, fixed!
9+#include <util/exec.h>
10+#include <util/strencodings.h>
11+#include <util/translation.h>
12+
13+#include <iostream>
14+#include <optional>
19+const TranslateFn G_TRANSLATION_FUN{nullptr};
20+
21+static constexpr auto HELP_USAGE = R"(Usage: %2$s [OPTIONS] COMMAND...
22+
23+Commands (run help command for more information):
24+ {gui,daemon,rpc,wallet,test,help}
tx is missing and test is part of the internal commands, so:
0 {gui,daemon,rpc,wallet,tx,help}
23+Commands (run help command for more information):
24+ {gui,daemon,rpc,wallet,test,help}
25+
26+Options:
27+ -m, --multiprocess Run multiprocess binaries bitcoin-node, bitcoin-gui.\n"
28+ -M, --monolithic Run monolithic binaries bitcoind, bitcoin-qt. (Default behavior)\n"
Some leftovers, I guess from previous incarnation of this which did not use R"(...)":
0 -m, --multiprocess Run multiprocess binaries bitcoin-node, bitcoin-gui.
1 -M, --monolithic Run monolithic binaries bitcoind, bitcoin-qt. (Default behavior)
re: #31375 (review)
Some leftovers, I guess from previous incarnation of this which did not use
R"(...)":
Thanks, fixed!
28+ -M, --monolithic Run monolithic binaries bitcoind, bitcoin-qt. (Default behavior)\n"
29+ -v, --version Show version information
30+ -h, --help Show this help message
31+
32+Command overview:
33+ %1$s gui [ARGS] Start GUI, equivalent to running 'bitcoin-qt [ARGS]' or 'bitcoin-gui [ARGS]'.
This prints “bitcoin gui”. One might get the impression that the command is “bitcoin gui”, whereas it is just “gui”. For comparison:
0$ git help
1usage: git ... <command> [<args>]
2...
3 clone Clone a repository into a new directory
4 init Create an empty Git repository or reinitialize an existing one
5...
So maybe:
0 gui [ARGS] Start GUI, equivalent to running 'bitcoin-qt [ARGS]' or 'bitcoin-gui [ARGS]'.
re: #31375 (review)
This prints “bitcoin gui”. One might get the impression that the command is “bitcoin gui”, whereas it is just “gui”.
Thanks for pointing that out. Changed to just print the commands like git.
Thanks for the review!
Updated 185c6a416df679ddc7b38750aa4d181c0b043d5b -> 623c0f0df878968dabbaa271a7654c638603f369 (pr/wrap.22 -> pr/wrap.23, compare) cleaning up comments and help with suggestions