multiprocess: Add bitcoin wrapper executable - #31375
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code Coverage & BenchmarksFor details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31375. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsReviewers, 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. HintsTry to run the tests locally, according to the documentation. However, a CI failure may still
Leave a comment here, if you need help tracking down a confusing failure. |
Sjors
left a comment
Sjors
left a comment
There was a problem hiding this comment.
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.
We have already used this macro: Lines 87 to 89 in 7590e93 |
ryanofsky
left a comment
ryanofsky
left a comment
There was a problem hiding this comment.
Thanks for the reviews!
Updated da108a6 -> 02567bf (pr/wrap.5 -> pr/wrap.6, compare) to fix windows build warning and making a change to avoid a potentially confusing behavior #31375 (comment)
re: #31375 (review)
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:
Lines 87 to 89 in 7590e93
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: Earlier it says: |
hodlinator
left a comment
hodlinator
left a comment
There was a problem hiding this comment.
Code Review 4e1aae1
Seems like a slight fix would be good in the NSI script, see inline comment.
Nice simplification of ExecVp since previous review.
Tested on NixOS.
Tested redirection of stderr/stdout
./build/bin/bitcoin node > foo
./build/bin/bitcoin node --nonexistentarg 2> err
"foo" stdout file contains expected log output. Confirmed by changing bitcoind.cpp error message that it was properly writing to "err" stderr file.
Tested moving bitcoin executable
Ensured /src was in $PATH and copied bitcoin executable there. Gives expected error messages:
~/bitcoin/src
₿ bitcoin node --nonexistentarg
Error: execvp failed to execute 'bitcoind': No such file or directory
Try 'bitcoin --help' for more information.
~/bitcoin/src
₿ ./bitcoin node --nonexistentarg
Error: execvp failed to execute '/home/hodlinator/bitcoin/src/bitcoind': No such file or directory
Try './bitcoin --help' for more information.
ryanofsky
left a comment
ryanofsky
left a comment
There was a problem hiding this comment.
Updated 4e1aae1 -> 7af6e10 (pr/wrap.32 -> pr/wrap.33, compare) with suggestions adding windows uninstall line, removing no longer used cmake build prefixes, and improving many comments.
|
re-utACK 7af6e10 I didn't retest. |
ismaelsadeeq
left a comment
ismaelsadeeq
left a comment
There was a problem hiding this comment.
Code review 7af6e10
I've also tested this on macOS.
It was running smoothly. I tried shutting it down using the CLI interface, and it worked as expected.
However, I encountered an issue when performing an unclean shutdown using CTRL+C.
The process hung, and I had to pkill bitcoind to terminate it.
I wasn’t running with -debug option, so I couldn’t figure out exactly what went wrong. I tried to reproduce the issue but couldn’t It worked smoothly in all subsequent runs and shutdown even during unclean shutdown.
Because of that, I would encourage additional testing on all supported platforms.
I also have some comments and a suggestion to help prevent mixing up options with commands.
ryanofsky
left a comment
ryanofsky
left a comment
There was a problem hiding this comment.
Thanks for the reviews!
Updated 7af6e10 -> a5ac43d (pr/wrap.33 -> pr/wrap.34, compare) just updating copyright and adding exception to handle an unexpected condition
re: #31375 (review)
However, I encountered an issue when performing an unclean shutdown using CTRL+C.
This could be a real issue but I somewhat doubt this PR could cause something like that. Unless the wrapper executable is printing help or error output should only be making a single exec call, so there is very little it could be doing to influence the next executable that is started.
| if (util::ExecVp(exec_args[0], (char*const*)exec_args.data()) == -1) { | ||
| if (allow_notfound && errno == ENOENT) return false; | ||
| throw std::system_error(errno, std::system_category(), strprintf("execvp failed to execute '%s'", exec_args[0])); | ||
| } | ||
| throw std::runtime_error("execvp returned unexpectedly"); |
There was a problem hiding this comment.
This can be written like:
util::ExecVp(exec_args[0], (char*const*)exec_args.data()); // If it returns then an error has occurred.
if (allow_notfound && errno == ENOENT) return false;
throw std::system_error(errno, std::system_category(), strprintf("execvp failed to execute '%s'", exec_args[0]));it is less indentation and less code - no need for the "this will never be reached" throw.
There was a problem hiding this comment.
re: #31375 (comment)
execvp is documented to return -1 if there is an error (https://linux.die.net/man/3/execvp and https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/execvp-wexecvp) so if it returns something else I think it is better to abort and not just keep going.
| //! array should consist of null terminated strings and be null terminated | ||
| //! itself, like the POSIX function. |
There was a problem hiding this comment.
super nit, but I can't resist ⛑️ :
"null terminated strings" and "null terminated" array -- it is not the same "null":
| //! array should consist of null terminated strings and be null terminated | |
| //! itself, like the POSIX function. | |
| //! array should consist of '\0'-terminated strings and be nullptr-terminated | |
| //! itself, like the POSIX function. |
🏃
There was a problem hiding this comment.
re: #31375 (comment)
Yes I used to refer to them as NUL terminated strings, until I came across https://en.wikipedia.org/wiki/Null-terminated_string and decided there wasn't any real ambiguity and null was probably easier to read. But would be ok changing this if others would prefer
ryanofsky
left a comment
ryanofsky
left a comment
There was a problem hiding this comment.
Thanks for reviewing again!
| if (util::ExecVp(exec_args[0], (char*const*)exec_args.data()) == -1) { | ||
| if (allow_notfound && errno == ENOENT) return false; | ||
| throw std::system_error(errno, std::system_category(), strprintf("execvp failed to execute '%s'", exec_args[0])); | ||
| } | ||
| throw std::runtime_error("execvp returned unexpectedly"); |
There was a problem hiding this comment.
re: #31375 (comment)
execvp is documented to return -1 if there is an error (https://linux.die.net/man/3/execvp and https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/execvp-wexecvp) so if it returns something else I think it is better to abort and not just keep going.
| //! array should consist of null terminated strings and be null terminated | ||
| //! itself, like the POSIX function. |
There was a problem hiding this comment.
re: #31375 (comment)
Yes I used to refer to them as NUL terminated strings, until I came across https://en.wikipedia.org/wiki/Null-terminated_string and decided there wasn't any real ambiguity and null was probably easier to read. But would be ok changing this if others would prefer
theStack
left a comment
theStack
left a comment
There was a problem hiding this comment.
ACK a5ac43d
Tested the wrapper more in-depth on Debian Linux 12, both with monolithic and multiprocess binaries this time, and also tried calling the wrapper without path separator to test PATH searching. Everything worked fine. Reviewed the code, though I only lightly looked at the Windows-specific parts in the exec.cpp module. Left some small refactoring and a comment improvement nit below, nothing blocking.
| } | ||
|
|
||
| //! Execute the specified bitcoind, bitcoin-qt or other command line in `args` | ||
| //! using src, bin and libexec directory paths relative to this executable, where |
There was a problem hiding this comment.
in commit 9c8c688: nit: the directory list seems outdated
| //! using src, bin and libexec directory paths relative to this executable, where | |
| //! using bin and libexec directory paths relative to this executable, where |
(don't know if it's really that important, but could even mentioned the "daemon" directory on Windows)
There was a problem hiding this comment.
re: #31375 (comment)
in commit 9c8c688: nit: the directory list seems outdated
Thanks! I made this change locally so it will be included here if there is another update, or in one of the followups.
ryanofsky
left a comment
ryanofsky
left a comment
There was a problem hiding this comment.
Thanks for the review! This could be ready to merge with another ack
| } | ||
|
|
||
| //! Execute the specified bitcoind, bitcoin-qt or other command line in `args` | ||
| //! using src, bin and libexec directory paths relative to this executable, where |
There was a problem hiding this comment.
re: #31375 (comment)
in commit 9c8c688: nit: the directory list seems outdated
Thanks! I made this change locally so it will be included here if there is another update, or in one of the followups.
ismaelsadeeq
left a comment
ismaelsadeeq
left a comment
There was a problem hiding this comment.
fwiw my last review implied an ACK a5ac43d
Further improvement can come after this as mentioned in the description, and also since I could not and no one reproduced the freeze I encountered in the unclean shutdown it is not a blocker to this.
|
utACK a5ac43d Since my last review this just adds |
hodlinator
left a comment
hodlinator
left a comment
There was a problem hiding this comment.
ACK a5ac43d
Concept: #31375 (review)
git range-diff master 4e1aae1 a5ac43d shows only minor fixups since previous review (#31375 (review)).
|
ACK a5ac43d |
|
There seems a silent conflict with #32396. |
|
|
14 participants
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.
Initial implementation of this feature is deliberately minimal so the UX can evolve in response to feedback and there are not too many details to debate and discuss in a single PR. But many improvements are possible or planned:
bitcoin help subcommandinvokesbitcoin subcommand -h.bitcoin -h subcommandshould also be supported and be equivalent (comment)bitcoin-utilsubcommands. Ideal interface would probably be more likebitcoin grindnotbitcoin util grindbut this has been punted for now. Supporting subcommands directly would require some ArgsManager modificationsBITCOIN_CMDvariable, but this doesn't cover things like the help output and version output, and support for different directory layouts.--multiprocess(-m) /--monolithic(-M) default selection. Right now, default is monolithic but it probably makes sense to chose more intelligently depending on whether -ipc options are enabled and what binaries are available.bitcoin.confand supporting options to control wrapper behavior like custom locations or preferences or aliases.-ah). Allow fuzzy matching of subcommands or suggestions if you misspell. (suggested by stickies in review club)bitcoin-cli namedimplementation used by the wrapper should do a better job disambiguating named arguments from base64 arguments ending in = as pointed out in (comment)This PR is part of the process separation project. A review club meeting for it took place in https://bitcoincore.reviews/31375