Closes #24907.
This PR is based on theStack’s work.
The subprocess.hpp header has been sourced from the upstream repo with the only modification being the removal of convenience functions, which are not utilized in our codebase.
Windows-related changes will be addressed in subsequent follow-ups.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For detailed information about the code coverage, see the test coverage report.
See the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
Reviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
11@@ -12,7 +12,7 @@
12 from subprocess import check_output, STDOUT, CalledProcessError
13
14 IGNORE_WORDS_FILE = 'test/lint/spelling.ignore-words.txt'
15-FILES_ARGS = ['git', 'ls-files', '--', ":(exclude)build-aux/m4/", ":(exclude)contrib/seeds/*.txt", ":(exclude)depends/", ":(exclude)doc/release-notes/", ":(exclude)src/leveldb/", ":(exclude)src/crc32c/", ":(exclude)src/qt/locale/", ":(exclude)src/qt/*.qrc", ":(exclude)src/secp256k1/", ":(exclude)src/minisketch/", ":(exclude)contrib/guix/patches"]
16+FILES_ARGS = ['git', 'ls-files', '--', ":(exclude)build-aux/m4/", ":(exclude)contrib/seeds/*.txt", ":(exclude)depends/", ":(exclude)doc/release-notes/", ":(exclude)src/leveldb/", ":(exclude)src/crc32c/", ":(exclude)src/qt/locale/", ":(exclude)src/qt/*.qrc", ":(exclude)src/secp256k1/", ":(exclude)src/minisketch/", ":(exclude)src/util/subprocess.hpp", ":(exclude)contrib/guix/patches"]
Is the plan to also delete any unused code from this header?
I haven’t consider it yet.
it maybe already doesn’t work with c++20?
CI jobs that uses C++20 are passing so far.
Is the plan to also delete any unused code from this header?
If at all possible, I’d prefer for it to be subtreed, so that updating to newer upstream versions is easy.
I was actually thinking the opposite. I think a subtree is overkill here, and, upstream isn’t exactly active (some random merges but no development), so I’m not sure if there will be many new versions.
I’d rather take the small(er) portion of any code we need, and have something better integrated/reviewed. Hopefully that will help avoid randomly pulling code from upstream, that isn’t well developed or reviewed.
I’d rather take the small(er) portion of any code we need, and have something better integrated/reviewed.
Maybe a subtree from our own fork? (to avoid bringing testing stuff to the main repo)
Maybe a subtree from our own fork?
I think that would mostly just re-introduce the awkwardness we ended up with with univalue, for not much benefit, and for code that shouldn’t be critical (i.e compared to leveldb & libsecp256k1), this is for an optional feature, that is not used by a large proportion of the people running this software.
I’d rather take the small(er) portion of any code we need, and have something better integrated/reviewed.
Looking at the coverage report of the header, there seems to be some potential for pruning out (89.2% function coverage and 67.4% line coverage),
I switched to the Popen constructor that accepts const std::string& instead of std::vector<std::string> to address quoting issues on Windows.
@theStack
Since you initially chose to use a vector, do you have any objections to this approach?
Here are the Windows functional tests – https://github.com/hebasto/bitcoin/actions/runs/7072075125/job/19250502576
I switched to the
Popenconstructor that acceptsconst std::string&instead ofstd::vector<std::string>to address quoting issues on Windows.@theStack Since you initially chose to use a vector, do you have any objections to this approach?
No objections at all. I would have expected that passing the arguments as a single string is more prone to quoting issues than if it’s done in a list, where quoting shouldn’t be needed at all, but if it works, all the better :) (Maybe that unverified assumption made me use the std::vector Popen ctor back then, but I honestly don’t really remember).
I switched to the
Popenconstructor that acceptsconst std::string&instead ofstd::vector<std::string>to address quoting issues on Windows. @theStack Since you initially chose to use a vector, do you have any objections to this approach?No objections at all. I would have expected that passing the arguments as a single string is more prone to quoting issues than if it’s done in a list, where quoting shouldn’t be needed at all, but if it works, all the better :) (Maybe that unverified assumption made me use the std::vector
Popenctor back then, but I honestly don’t really remember).
To support the string-based approach, let’s consider echo-ing a JSON-parsable string on Windows:
0>echo {"success": true}
1{"success": true}
and reproducing it using Python’s subprocess module, which interface is mimicked in cpp-subprocess:
0>>> import subprocess
1>>> subprocess.run("cmd.exe /c echo {\"success\": true}")
2{"success": true}
3CompletedProcess(args='cmd.exe /c echo {"success": true}', returncode=0)
It becomes intricate when using a list of strings:
0>>> subprocess.run(["cmd.exe", "/c", "echo", "{\"success\": true}"])
1"{\"success\": true}"
or
0>>> subprocess.run(["cmd.exe", "/c", "echo", "{\"success\":", "true}"])
1{\"success\": true}
or
0>>> subprocess.run(['cmd.exe', '/c', 'echo', '{"success":', 'true}'])
1{\"success\": true}
Maybe a subtree from our own fork?
I think that would mostly just re-introduce the awkwardness we ended up with with univalue, for not much benefit, and for code that shouldn’t be critical (i.e compared to leveldb & libsecp256k1), this is for an optional feature, that is not used by a large proportion of the people running this software.
Which means it should be an entirely external dependency we just check for in configure…
The only thing worse than a subtree, is entirely vendoring it like this PR currently does.
The code changes for external signing look pretty minimal, so that’s nice. I have no informed opinion on the difference between Boost Process and cpp-process. Except that the latter seems self contained.
Which means it should be an entirely external dependency we just check for in configure…
In theory that makes sense to me as well, e.g. a fork under the bitcoin-core project, with functionality (mostly) limited to what we need. However in practice it means having to package it and get into distros. Or force anyone* compiling Bitcoin to also clone and build this library from source themselves. The latter could be less of a problem, if we make it easier to build and use individual /depends packages. But that’s not going to be anytime soon.
* = who either needs the feature or just doesn’t want to accidentally break it
Tested on Ubuntu 23.10.
This slightly changes the way strings are escaped, please use this:
0--- a/src/external_signer.cpp
1+++ b/src/external_signer.cpp
2@@ -60,43 +60,43 @@ bool ExternalSigner::Enumerate(const std::string& command, std::vector<ExternalS
3 return true;
4 }
5
6 UniValue ExternalSigner::DisplayAddress(const std::string& descriptor) const
7 {
8- return RunCommandParseJSON(m_command + " --fingerprint " + m_fingerprint + NetworkArg() + " displayaddress --desc \"" + descriptor + "\"");
9+ return RunCommandParseJSON(m_command + " --fingerprint " + m_fingerprint + NetworkArg() + " displayaddress --desc " + descriptor);
10 }
11
12 const std::string command = m_command + " --stdin --fingerprint " + m_fingerprint + NetworkArg();
13- const std::string stdinStr = "signtx \"" + EncodeBase64(ssTx.str()) + "\"";
14+ const std::string stdinStr = "signtx " + EncodeBase64(ssTx.str());
That’s probably for the better, but it’s important to manually test the HWI address verify and PSBT sign feature.
For anyone testing with HWI, consider also testing #24313.
Rebased and implemented @Sjors’s suggestions.
Thank you @Sjors for testing!
For anyone testing with HWI, consider also testing #24313.
Here is the combined 231130-replace-bp+pr24313 branch.
40@@ -41,6 +41,9 @@
41 /* Define this symbol to enable ZMQ functions */
42 #define ENABLE_ZMQ 1
43
44+/* define if external signer support is enabled (requires Boost::Process) */
(requires Boost::Process)
tACK b67b6ab3b69d45034b17f4704a1f3cedb07e9af6
Tested on macOS 14.2.1, Windows 11 Home (using cross compiled Guix build) and Ubuntu 23.10.
I didn’t test Windows native. It might be worth splitting off the last commit to get this merged faster. I’m assuming it’s more important to get rid of boost process than it is to support native Windows builds.
Commit f6a3b5ef643d4993f0ae3faa0c5ff1ae62ad71d8 which switches the external signer code to use cpp-subprocess looks correct.
I perused the cpp-subprocess implementation. It seems well structured and documented, but I didn’t study it in great detail.
Guix hashes:
0ed62c504b8ba300d0e360593542226d23a72f89f8ff001fbecad56ef86c1a225 guix-build-b67b6ab3b69d/output/aarch64-linux-gnu/SHA256SUMS.part
1dcc6a5b947c8a8394bf20c8ed227181532e7b146317bf28c0e44d8fa1367d1a2 guix-build-b67b6ab3b69d/output/aarch64-linux-gnu/bitcoin-b67b6ab3b69d-aarch64-linux-gnu-debug.tar.gz
2fba2a71b5075029b038b3047db382e4f353004b7b43de98f83ff6882e635b977 guix-build-b67b6ab3b69d/output/aarch64-linux-gnu/bitcoin-b67b6ab3b69d-aarch64-linux-gnu.tar.gz
3d634f4eeb5d640fccfaaa3e773f43ba10557a71539183fb465af1d2d25e60982 guix-build-b67b6ab3b69d/output/arm-linux-gnueabihf/SHA256SUMS.part
49e3d0d439913581884eee878b5bc8bb170564888de64b58963247139bdbf7c2c guix-build-b67b6ab3b69d/output/arm-linux-gnueabihf/bitcoin-b67b6ab3b69d-arm-linux-gnueabihf-debug.tar.gz
566343b988d3eb57f3c2d07050bd5331933256a4651b34ff460096fab665c537f guix-build-b67b6ab3b69d/output/arm-linux-gnueabihf/bitcoin-b67b6ab3b69d-arm-linux-gnueabihf.tar.gz
6e08ed79e9fc6cac757815b243a4ceef063ac341eee2429058c7c8fd405061286 guix-build-b67b6ab3b69d/output/arm64-apple-darwin/SHA256SUMS.part
7df21ea533af06af18b1c4740714d03cda0eecfda27c390ac757cea92517a9690 guix-build-b67b6ab3b69d/output/arm64-apple-darwin/bitcoin-b67b6ab3b69d-arm64-apple-darwin-unsigned.tar.gz
854d61884c4c6b719463ced19e9fa4e224b3655b4cc90d768f63f5a9ac6f27163 guix-build-b67b6ab3b69d/output/arm64-apple-darwin/bitcoin-b67b6ab3b69d-arm64-apple-darwin-unsigned.zip
99aa8688e1ac55a6384e931e6217ec890a4b241abfa7ad67ba2bdd46c88b7f8c9 guix-build-b67b6ab3b69d/output/arm64-apple-darwin/bitcoin-b67b6ab3b69d-arm64-apple-darwin.tar.gz
103e8d01ddad51524c3fa8a4e5ab1ae6aff1813064d97cce1920c23d16dfbb5769 guix-build-b67b6ab3b69d/output/dist-archive/bitcoin-b67b6ab3b69d.tar.gz
119cf279f28118da5fc995c97c8f463273ade38a9da7ca9a2bfcab755e1c08ac46 guix-build-b67b6ab3b69d/output/powerpc64-linux-gnu/SHA256SUMS.part
1246d7ef9595cba43a1ce4772b060f0b360bccb5eea16180aeed74d5e8f3f4801a guix-build-b67b6ab3b69d/output/powerpc64-linux-gnu/bitcoin-b67b6ab3b69d-powerpc64-linux-gnu-debug.tar.gz
13c25331d7919b78c8820f23ad47b142a334fbfc51bac0dac385305feb09c03272 guix-build-b67b6ab3b69d/output/powerpc64-linux-gnu/bitcoin-b67b6ab3b69d-powerpc64-linux-gnu.tar.gz
14df0d7a7ccc5c8ae08958f3b2fba7aa3e59ac6402959b4eafbf333d3e4566aad2 guix-build-b67b6ab3b69d/output/powerpc64le-linux-gnu/SHA256SUMS.part
157dbf5c8e10f5b80805b7962ac79bb67feabd2d5deaa15a028a61181fc5d3e5d9 guix-build-b67b6ab3b69d/output/powerpc64le-linux-gnu/bitcoin-b67b6ab3b69d-powerpc64le-linux-gnu-debug.tar.gz
1654969541c8ea83279d55bd3e31b2929257266bf6e8e4f985e59a29a2f513c44c guix-build-b67b6ab3b69d/output/powerpc64le-linux-gnu/bitcoin-b67b6ab3b69d-powerpc64le-linux-gnu.tar.gz
17331854be69ed4374aeae930840286e5fc090bf1e4535022b6d8226aa52b6e815 guix-build-b67b6ab3b69d/output/riscv64-linux-gnu/SHA256SUMS.part
18d16dbf428e6443ba542f0d031cbde863b7dbb12795ab88264832a0e368415ff3 guix-build-b67b6ab3b69d/output/riscv64-linux-gnu/bitcoin-b67b6ab3b69d-riscv64-linux-gnu-debug.tar.gz
1922676d48bbd363a2cd2e6390f0182c8eb9181ee4c4bd3d11565a0cbecec827cf guix-build-b67b6ab3b69d/output/riscv64-linux-gnu/bitcoin-b67b6ab3b69d-riscv64-linux-gnu.tar.gz
20ae3a2eaabc84ed05603403d922e61d7339af1c0392141bb42181cd2fd2449971 guix-build-b67b6ab3b69d/output/x86_64-apple-darwin/SHA256SUMS.part
218e4e1f5adb4a6883907f79c5864d24e39d590de736345d3d6422d0675d50e60a guix-build-b67b6ab3b69d/output/x86_64-apple-darwin/bitcoin-b67b6ab3b69d-x86_64-apple-darwin-unsigned.tar.gz
224463c30865215881b2a9a91d3a844577e57985160a6e8f0f1407853354891132 guix-build-b67b6ab3b69d/output/x86_64-apple-darwin/bitcoin-b67b6ab3b69d-x86_64-apple-darwin-unsigned.zip
235061a82bdcd43d4cc82b2b004c4d415bb6146be083384d6ef7fc26c5717ff49f guix-build-b67b6ab3b69d/output/x86_64-apple-darwin/bitcoin-b67b6ab3b69d-x86_64-apple-darwin.tar.gz
24ed378935976c4faeb84b37d16eff95ccb42877911e1e5894b7dc5c7f3a5fc15d guix-build-b67b6ab3b69d/output/x86_64-linux-gnu/SHA256SUMS.part
25c47db6b7abfe300f5545d0e0db1e202bd72c0e27e213601267e25fbfacf9a29c guix-build-b67b6ab3b69d/output/x86_64-linux-gnu/bitcoin-b67b6ab3b69d-x86_64-linux-gnu-debug.tar.gz
26a4941c606ecad6e0131fdb7c2785014f4edfcca9f9848add9702209bbbf42895 guix-build-b67b6ab3b69d/output/x86_64-linux-gnu/bitcoin-b67b6ab3b69d-x86_64-linux-gnu.tar.gz
27d5bdc32703c9c42ef179e14581881db261c2164c0674b6ee0b1a842ac57ba0ba guix-build-b67b6ab3b69d/output/x86_64-w64-mingw32/SHA256SUMS.part
28fbbd9e65fe60ae69bd523043eaedcad09b282dde180775f0248e8f14f1f1867a guix-build-b67b6ab3b69d/output/x86_64-w64-mingw32/bitcoin-b67b6ab3b69d-win64-debug.zip
29fdadf9a1fa10fc1269ca5bcd1550f32c194b86f086b9d7a10a84c71bfd1ed8e8 guix-build-b67b6ab3b69d/output/x86_64-w64-mingw32/bitcoin-b67b6ab3b69d-win64-setup-unsigned.exe
30013e27014e44607b46d4db6defedbf7aac3f3d3ee8da861be6509291d134dd34 guix-build-b67b6ab3b69d/output/x86_64-w64-mingw32/bitcoin-b67b6ab3b69d-win64-unsigned.tar.gz
3177621aa403995bd958d8c9560bf7fe4852e945de739dbe94d993b6207966f444 guix-build-b67b6ab3b69d/output/x86_64-w64-mingw32/bitcoin-b67b6ab3b69d-win64.zip
Nit: if you have to retouch, could update the commit hash reference in the first commit body (the last upstream patch https://github.com/arun11299/cpp-subprocess/pull/98 was merged a few weeks ago). Regarding the TODO in the same message, are there any more changes included here which would make sense to upstream? Comparing cpp-subprocess master branch with the version included here still shows some small differences:
0--- /home/thestack/bitcoin/cpp-subprocess/subprocess.hpp Tue Feb 27 14:46:14 2024
1+++ ./src/util/subprocess.hpp Tue Feb 27 14:41:21 2024
2@@ -61,7 +61,7 @@
3
4 extern "C" {
5 #ifdef __USING_WINDOWS__
6- #include <Windows.h>
7+ #include <windows.h>
8 #include <io.h>
9 #include <cwchar>
10
11@@ -155,7 +155,7 @@
12 //--------------------------------------------------------------------
13
14 //Environment Variable types
15-#ifndef _MSC_VER
16+#ifndef __USING_WINDOWS__
17 using env_string_t = std::string;
18 using env_char_t = char;
19 #else
20@@ -184,7 +184,7 @@
21 //
22
23 if (force == false && argument.empty() == false &&
24- argument.find_first_of(L" \t\n\v\"") == argument.npos) {
25+ argument.find_first_of(L" \t\n\v") == argument.npos) {
26 command_line.append(argument);
27 }
28 else {
29@@ -1057,6 +1057,7 @@
30 public:
31 Communication(Streams* stream): stream_(stream)
32 {}
33+ Communication(const Communication&) = default;
34 void operator=(const Communication&) = delete;
35 public:
36 int send(const char* msg, size_t length);
37@@ -1094,6 +1095,7 @@
38 {
39 public:
40 Streams():comm_(this) {}
41+ Streams(const Streams&) = default;
42 void operator=(const Streams&) = delete;
43
44 public:
45@@ -1426,6 +1428,10 @@
46
47 inline int Popen::poll() noexcept(false)
48 {
49+#ifndef __USING_WINDOWS__
50+ if (!child_created_) return -1; // TODO: ??
51+#endif
52+
53 #ifdef __USING_WINDOWS__
54 int ret = WaitForSingleObject(process_handle_, 0);
55 if (ret != WAIT_OBJECT_0) return -1;
56@@ -1439,8 +1445,6 @@
57
58 return retcode_;
59 #else
60- if (!child_created_) return -1; // TODO: ??
61-
62 int status;
63
64 // Returns zero if child is still running
I didn’t test Windows native. It might be worth splitting off the last commit to get this merged faster. I’m assuming it’s more important to get rid of boost process than it is to support native Windows builds.
I’ve reworked this PR to remove all Windows-specific changes. Any remaining Windows-related modifications will be addressed in follow-ups. @theStack
Nit: if you have to retouch, could update the commit hash reference in the first commit body (the last upstream patch arun11299/cpp-subprocess#98 was merged a few weeks ago). Regarding the TODO in the same message, are there any more changes included here which would make sense to upstream? Comparing cpp-subprocess master branch with the version included here still shows some small differences:
I’ve reworked this PR to remove all Windows-specific changes. Therefore, diffs you mentioned are not needed in this PR. However, I’ll continue to work on submitting Windows-related fixes to the upstream. @fanquake
Concept ACK on removing Boost Process. Is the plan to also delete any unused code from this header? I don’t think copy-pasting dead/untested code into this repository is a good idea, and only serves to complicate review, and potentially cause issues in future (it maybe already doesn’t work with c++20?)
I’ve removed the entire “Convenience Functions” section. While other removals, such as unused API overloads, are still possible, I’ve opted to keep them for now to facilitate the reviewing process.
This PR is based on #29489 now, so please review it first :)
re-ACK 86e1f24f20335f9790db95f253c8f493a7f63983
Mainly just drops windows support since my last review. I briefly re-tested on Ubuntu.
Light ACK 86e1f24f20335f9790db95f253c8f493a7f63983
Didn’t review any of the code in cpp-subprocess, but the library seems to do its job at least on POSIX systems (tested on OpenBSD 7.4 and Ubuntu 22.04.3 LTS).
I checked for remaining boost::process leftovers/references via git grep -i boost.*process and found a depends patch that can likely also be removed: https://github.com/theStack/bitcoin/commit/cf78210018ad1ea83b5b035c15949045fc69c583 (it’s the only depends patch for boost)
ACK 86e1f24f20335f9790db95f253c8f493a7f63983
Briefly tested on Arch, and it works. The changes to our code are straightforward and just modifying to use the new API. I looked at cpp-subprocess itself, and there definitely is a lot of code that we don’t use that could be pruned in a followup. For unix systems, it indeed does the expected fork-exec. I did not look at the Windows specific code as that is currently unused. @TheCharlatan @fanquake @dergoegge Since you Concept ACK’d, would you mind also giving this a review?
Upstream repo: https://github.com/arun11299/cpp-subprocess
Commit: 4025693decacaceb9420efedbf4967a04cb028e7
The "Convenience Functions" section is unused in our codebase, so it has
been removed.
This primarily affects the `RunCommandParseJSON` utility function.
reACK d5a715536e497c160a2520f81334aab6c7490213
Checked diff is just for resolving rebase conflicts.
Light re-ACK d5a715536e497c160a2520f81334aab6c7490213
(as per git range-diff 86e1f24...d5a7155)
re-tACK d5a715536e497c160a2520f81334aab6c7490213
Tested display address from the GUI on a Trezor on Ubuntu.
ACK d5a715536e497c160a2520f81334aab6c7490213 - with the expectation that this code is going to be maintained as our own. Next PRs should: