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
Concept ACK	sedited
Approach ACK	hebasto

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

Conflicts

No conflicts as of last run.

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-01-20

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

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

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

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.

fac2b1eea6

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

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

Code Coverage & Benchmarks

Reviews

Conflicts

LLM Linter (✨ experimental)