util: Remove brittle and confusing sp::Popen(std::string)

maflcko commented at 7:19 am on January 20, 2026: member

The subprocess Popen call that accepts a full std::string has many issues:

It promotes brittle and broken code, where spaces are not properly quoted. Example: #33929 (review)
The internally used util::split function does incorrectly split on spaces, instead of using shlex.split.
It is redundant and not needed, because a vector interface already exists.

Fix all issues by removing it and just using the vector interface.

This pull request should not change any behavior: Note that the command taken from gArgs.GetArg("-signer", "") is still passed through the sp::util::split helper, just like before. Fixing that is left for a follow-up, so that this change here is basically just a refactor.

This also fixes a unit test bug as a side-effect: Fixes #32574.

DrahtBot added the label Utils/log/libs on Jan 20, 2026

DrahtBot commented at 7:19 am on January 20, 2026: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/34349.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	janb84, fjahr, hebasto
Concept ACK	sedited, theStack

If your review is incorrectly listed, please copy-paste <!–meta-tag:bot-skip–> into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#34562 (ThreadPool follow-ups, proactive shutdown and HasReason dependency cleanup by furszy)
#33929 (test: Remove system_tests/run_command runtime dependencies by hebasto)

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.

LLM Linter (✨ experimental)

Possible places where named args for integral literals may be used (e.g. func(x, /*named_arg=*/0) in C++, and func(x, named_arg=0) in Python):

strprintf(“RunCommandParseJSON error: process(%s) returned %s: %s”, util::Join(command, " “), 1, “err”) in src/test/system_tests.cpp

2026-02-17 11:56:03

DrahtBot added the label CI failed on Jan 20, 2026

DrahtBot commented at 8:26 am on January 20, 2026: contributor

🚧 At least one of the CI tasks failed. Task Windows native, VS 2022: https://github.com/bitcoin/bitcoin/actions/runs/21162862470/job/60860861671 LLM reason (✨ experimental): system_tests failed (CTest exit code 8) causing the CI to fail.

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.

maflcko commented at 8:46 am on January 20, 2026: member

Hmm. Looks like all tests passed, except the Windows system unit tests using echo. No idea why that could be. I guess Unlike Bash, the Windows cmd internal echo command does not strip quotes from its arguments, It prints them literally?

Let me just pass them as a single string in the Windows tests. This seems more correct anyway.

maflcko force-pushed on Jan 20, 2026

sedited commented at 8:49 am on January 20, 2026: contributor

Concept ACK

maflcko commented at 9:14 am on January 20, 2026: member

Actually, I don’t think my attempt works, because cmd.exe + echo will double quote here, which is not valid json. I guess I don’t really have a choice than to preserve the exact test case here and manually rip the command into meaningless space-separated tokens, like it is done on current master.

Will push that and add a comment in the unit test.

maflcko force-pushed on Jan 20, 2026

DrahtBot removed the label CI failed on Jan 20, 2026

maflcko commented at 10:34 am on January 20, 2026: member

Looks like i accidentally fixed a bug. To confirm the broken output on current master, the following diff can be used:

 0diff --git a/src/test/system_tests.cpp b/src/test/system_tests.cpp
 1index 0b7cc5b21f..e51b2040cf 100644
 2--- a/src/test/system_tests.cpp
 3+++ b/src/test/system_tests.cpp
 4@@ -67,6 +67,7 @@ BOOST_AUTO_TEST_CASE(run_command)
 5         const std::string expected{"err"};
 6         BOOST_CHECK_EXCEPTION(RunCommandParseJSON(command), std::runtime_error, [&](const std::runtime_error& e) {
 7             const std::string what(e.what());
 8+            std::cout << what << std::endl;
 9             BOOST_CHECK(what.find(strprintf("RunCommandParseJSON error: process(%s) returned", command)) != std::string::npos);
10             BOOST_CHECK(what.find(expected) != std::string::npos);
11             return true;

It will print:

0$ ./bld-cmake/bin/test_bitcoin -t system_tests
1Running 1 test case...
2RunCommandParseJSON error: process(sh -c 'echo err 1>&2 && false') returned 2: err: -c: line 1: unexpected EOF while looking for matching `''
3
4
5*** No errors detected

DrahtBot added the label CI failed on Jan 20, 2026

maflcko force-pushed on Jan 20, 2026

DrahtBot removed the label CI failed on Jan 20, 2026

maflcko commented at 10:56 am on January 21, 2026: member

i dropped the trailing newline from the checked string in the last commit. I guess on Windows it is not called \n. CI is green now.

hebasto commented at 4:55 pm on January 21, 2026: member

Concept ACK.

The Popen constructor that accepts std::vector<std::string> was used in @theStack’s initial approach. Then it was changed.

It’s nice to see it working now.

maflcko commented at 5:08 pm on January 21, 2026: member

It’s nice to see it working now.

Heh, this should be seen more like a refactor and I don’t think this pull request changes any external visible behavior. The unit test bugfix is just an accidental side-effect. If you had quoting issues, i think it would be good to know them, so that a future bugfix that fixes sp::util::split (which is still used here) can be better tested.

maflcko commented at 5:15 pm on January 21, 2026: member

I guess that means porting shlex.split to C++, or something. But this can be done later. I think it is fine to just improve the unit tests for now.

hebasto commented at 5:40 pm on January 21, 2026: member

It’s nice to see it working now.

Heh, this should be seen more like a refactor and I don’t think this pull request changes any external visible behavior. The unit test bugfix is just an accidental side-effect. If you had quoting issues, i think it would be good to know them, so that a future bugfix that fixes sp::util::split (which is still used here) can be better tested.

It seems you have just hacked the quoting issue around and documented it:https://github.com/bitcoin/bitcoin/blob/fac2b1eea6db57d6f0b3671ae1fdb7912253538b/src/test/system_tests.cpp#L31 :)

maflcko commented at 5:47 pm on January 21, 2026: member

Yeah, but I don’t think this has anything to do with subprocess. I am happy to push another version there, if someone finds one that works. But I don’t have Windows to test this locally. Also, I don’t think it is worth it to spend more time with the absurd cmd.exe+echo behavior on Windows,, because #33929 will remove that line of code anyway.

cmd.exe generally having the most absurd behavior.

Source: https://web.archive.org/web/20250323114027/secure.phabricator.com/T13209

hebasto commented at 5:47 pm on January 21, 2026: member

Tested fac2b1eea6db57d6f0b3671ae1fdb7912253538b on OpenIndiana:

0$ uname -a
1SunOS openindiana 5.11 illumos-2671fc51ca i86pc i386 i86pc

hebasto commented at 7:11 pm on January 21, 2026: member

Approach ACK fac2b1eea6db57d6f0b3671ae1fdb7912253538b.

This resolves #33929 (review).

Going to run more tests before final ACK.

hebasto commented at 7:12 pm on January 21, 2026: member

cc @Sjors @theStack

maflcko added the label DrahtBot Guix build requested on Jan 26, 2026

luke-jr commented at 6:01 pm on January 28, 2026: member

There is a fundamental design difference between *nix and Windows in this regard. While *nix passes an array of individual arguments, Windows just passes a complete argument string, and it is merely convention how the called program splits/handles that string. The standard echo on Windows indeed does not handle it like you would expect coming from *nix.

maflcko commented at 7:24 pm on January 28, 2026: member

Joining the args for Windows is responsibility of the subprocess library. This is currently (in this pull and on master) done in quote_argument.

I don’t see another approach here, other than to ifdef WINDOWS every subprocess args related code at every single call site with separate handling for Windows and non-Windows. So removing sp::Popen(std::string) is the correct approach in my eyes.

DrahtBot commented at 3:51 pm on February 1, 2026: contributor

Guix builds (on x86_64) [untrusted test-only build, possibly unsafe, not for production use]

File	commit 4e4fa0199ee2015476218f7f71fe5df31751b767(master)	commit 8906d1789eaae3bfcfb0205f7e3558ea9a1da682(pull/34349/merge)
*-aarch64-linux-gnu-debug.tar.gz	`93a91489a4f56fb5...`	`63eb5bc5344e56df...`
*-aarch64-linux-gnu.tar.gz	`e3392de2c4f745f5...`	`85d6032f9a76aeea...`
*-arm-linux-gnueabihf-debug.tar.gz	`5d6bba5531516d61...`	`6b983c36b8b7161a...`
*-arm-linux-gnueabihf.tar.gz	`a1dc9f203bfcf022...`	`39505f304ac32e50...`
*-arm64-apple-darwin-codesigning.tar.gz	`96c17d2aa8d5541e...`	`3d9f57c65f2c4f89...`
*-arm64-apple-darwin-unsigned.tar.gz	`cd240f043399b74d...`	`fc270db8b8134511...`
*-arm64-apple-darwin-unsigned.zip	`3b6d7f1221d0edd1...`	`cd0cd9bc8a52c046...`
*-powerpc64-linux-gnu-debug.tar.gz	`e5c7d30f4aa0bc2f...`	`1c1591fa3776d603...`
*-powerpc64-linux-gnu.tar.gz	`5577938809b0624f...`	`24ebcd94740263ab...`
*-riscv64-linux-gnu-debug.tar.gz	`62a7ed64f40ca656...`	`28defbefe820d18c...`
*-riscv64-linux-gnu.tar.gz	`90e848f68d1c5516...`	`a21493b1c170856c...`
*-win64-codesigning.tar.gz	`39645ce0101cda0b...`	`caf9fc90e4f60c8a...`
*-win64-debug.zip	`bcece7f3b44d6558...`	`c562ec5ab5e963d0...`
*-win64-setup-unsigned.exe	`2592820766acc39a...`	`a65d04877c09694f...`
*-win64-unsigned.zip	`49ffb3d9e7d06fa6...`	`62f83d464380c4a2...`
*-x86_64-apple-darwin-codesigning.tar.gz	`adac67a7ca464833...`	`1b83ce97bffc5a99...`
*-x86_64-apple-darwin-unsigned.tar.gz	`eb61e5b749d6c87b...`	`ceb48150f57edf15...`
*-x86_64-apple-darwin-unsigned.zip	`4d075d133d794b10...`	`73657b7d00f71eca...`
*-x86_64-linux-gnu-debug.tar.gz	`19ad062fb0aad845...`	`f11d3c382d675f7e...`
*-x86_64-linux-gnu.tar.gz	`bf96d6cfdf5a6bf2...`	`4c213f1838e5c5f3...`
*.tar.gz	`186d8bbf269bc891...`	`621df7bdd8676b42...`
SHA256SUMS.part	`f5aa843d3edc9470...`	`8bc08a4f7634a598...`
guix_build.log	`9644d6364d79c03c...`	`bc9ed458041baee8...`
guix_build.log.diff		`90b3eb7a862af513...`

DrahtBot removed the label DrahtBot Guix build requested on Feb 1, 2026

theStack commented at 4:25 pm on February 4, 2026: contributor

Concept ACK

hebasto approved

hebasto commented at 3:01 pm on February 13, 2026: member

ACK fac2b1eea6db57d6f0b3671ae1fdb7912253538b, I have reviewed the code and it looks OK.

DrahtBot requested review from sedited on Feb 13, 2026

DrahtBot requested review from theStack on Feb 13, 2026

in src/util/subprocess.h:1 in fac2b1eea6 outdated

janb84 commented at 10:20 am on February 17, 2026:

Nit, I think that lines 1016-1017 also can be removed now ?

0  // Command in string format
1  std::string args_;

maflcko commented at 11:57 am on February 17, 2026:

thx, done. Also, this is unrelated to the changes here, so I’ve submitted the same patch upstream.

janb84 commented at 10:30 am on February 17, 2026: contributor

Concept ACK fac2b1eea6db57d6f0b3671ae1fdb7912253538b

Did a code review, left one NIT.

Not sure what’s the current policy is on IWYU but there are some suggestions on changed files: run_command.cpp, external_signer.cpp, external_signer.h (system_tests.cpp mostly false positives on boost)

util: Remove brittle and confusing sp::Popen(std::string) fa626bd143

test: Stricter unit test

Now that the previous commit fixed a unit test bug, make the test
stricter, to prevent this issue from happening again in the future.

fa48d42163

maflcko force-pushed on Feb 17, 2026

maflcko commented at 11:57 am on February 17, 2026: member

Not sure what’s the current policy is on IWYU but there are some suggestions on changed files: run_command.cpp, external_signer.cpp, external_signer.h (system_tests.cpp mostly false positives on boost)

I think for they will be fixed+enforced by CI step-by-step for now. Once they are cleanly enforced globally, they’ll be changed as part of the pull request.

janb84 commented at 2:52 pm on February 17, 2026: contributor

cr ACK fa48d421636c256069010bc03c121c36ed9c0a0c

code review, looks good.

In addition, I have run a windows MSVC build & test, both ran without issues:

 0141/152 Test [#122](/bitcoin-bitcoin/122/): txvalidationcache_tests ..............   Passed   10.55 sec
 1142/152 Test [#151](/bitcoin-bitcoin/151/): walletdb_tests .......................   Passed    0.26 sec
 2143/152 Test [#147](/bitcoin-bitcoin/147/): wallet_crypto_tests ..................   Passed    1.16 sec
 3144/152 Test [#152](/bitcoin-bitcoin/152/): walletload_tests .....................   Passed    0.42 sec
 4145/152 Test [#146](/bitcoin-bitcoin/146/): spend_tests ..........................   Passed    6.60 sec
 5146/152 Test [#118](/bitcoin-bitcoin/118/): txpackage_tests ......................   Passed   25.61 sec
 6147/152 Test   [#3](/bitcoin-bitcoin/3/): secp256k1_noverify_tests .............   Passed   53.55 sec
 7148/152 Test [#130](/bitcoin-bitcoin/130/): validation_block_tests ...............   Passed   28.59 sec
 8149/152 Test [#149](/bitcoin-bitcoin/149/): wallet_tests .........................   Passed   24.26 sec
 9150/152 Test [#138](/bitcoin-bitcoin/138/): coinselector_tests ...................   Passed   27.87 sec
10151/152 Test [#132](/bitcoin-bitcoin/132/): validation_chainstatemanager_tests ...   Passed   33.11 sec
11152/152 Test   [#4](/bitcoin-bitcoin/4/): secp256k1_tests ......................   Passed   83.98 sec
12
13100% tests passed, 0 tests failed out of 152
14
15Total Test time (real) =  84.63 sec
16
17The following tests did not run:
18         90 - script_assets_tests (Skipped)
19PS C:\Users\user\source\repos\bitcoin>

DrahtBot requested review from hebasto on Feb 17, 2026

in src/test/system_tests.cpp:72 in fa48d42163

68@@ -69,7 +69,7 @@ BOOST_AUTO_TEST_CASE(run_command)
69         const std::string expected{"err"};
70         BOOST_CHECK_EXCEPTION(RunCommandParseJSON(command), std::runtime_error, [&](const std::runtime_error& e) {
71             const std::string what(e.what());
72-            BOOST_CHECK(what.find(strprintf("RunCommandParseJSON error: process(%s) returned", util::Join(command, " "))) != std::string::npos);
73+            BOOST_CHECK(what.find(strprintf("RunCommandParseJSON error: process(%s) returned %s: %s", util::Join(command, " "), 1, "err")) != std::string::npos);

fjahr commented at 5:28 pm on February 17, 2026:

nit

0            BOOST_CHECK(what.find(strprintf("RunCommandParseJSON error: process(%s) returned %d: %s", util::Join(command, " "), 1, "err")) != std::string::npos);

maflcko commented at 5:40 pm on February 17, 2026:

I prefer %s in tinyformat, and this will fix itself anyway with std::format in a year or two

fjahr commented at 5:29 pm on February 17, 2026: contributor

Code review ACK fa48d421636c256069010bc03c121c36ed9c0a0c

Change looks like a good improvement.

hebasto approved

hebasto commented at 7:57 am on February 18, 2026: member

re-ACK fa48d421636c256069010bc03c121c36ed9c0a0c.

bitcoin deleted a comment on Feb 18, 2026

fanquake merged this on Feb 18, 2026

fanquake closed this on Feb 18, 2026

maflcko deleted the branch on Feb 18, 2026

fanquake commented at 2:11 pm on February 18, 2026: member

Possible this also fixed #31506.

fanquake commented at 4:18 pm on February 18, 2026: member

and this will fix itself anyway with std::format in a year or two @maflcko can you open an issue to track this / the current std::format blockers?

util: Remove brittle and confusing sp::Popen(std::string) #34349

Code Coverage & Benchmarks

Reviews

Conflicts

LLM Linter (✨ experimental)

Guix builds (on x86_64) [untrusted test-only build, possibly unsafe, not for production use]