Use subprocess library for notifications

laanwj commented at 2:55 pm on May 19, 2025: member

Builds on #29868 (to have windows support).

This switches the -*notify options to use the subprocess library, which is currently used for only the external signer. The main advantage of this is that file descriptor leaks are avoided (#32343). This could also add future flexibility in notifications, for example to bypass the shell (and launch commands with their arguments directly[1]), or do something with command output (like log it). But for now, this is meant to be one-to-one.

Generalize ENABLE_EXTERNAL_SIGNER build option to ENABLE_SUBPROCESS.
Unify everything dealing with external commands in common/run_command.cpp. Use the new functions RunShell and RunShellInThread.
Remove HAVE_SYSTEM.

[1] There’s a long-running issue on Windows where it can’t pass the wallet name because there’s no way to escape it for the shell.

laanwj added the label Build system on May 19, 2025

laanwj added the label Utils/log/libs on May 19, 2025

DrahtBot commented at 2:55 pm on May 19, 2025: 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/32566.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	fanquake, hebasto, theStack

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#32528 (rpc: Round verificationprogress to 1 for a recent tip by maflcko)
#29641 (scripted-diff: Use LogInfo over LogPrintf [WIP, NOMERGE, DRAFT] by maflcko)
#28792 (Embed default ASMap as binary dump header file by fjahr)

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.

laanwj force-pushed on May 19, 2025

DrahtBot added the label CI failed on May 19, 2025

DrahtBot commented at 3:00 pm on May 19, 2025: contributor

🚧 At least one of the CI tasks failed. Task lint: https://github.com/bitcoin/bitcoin/runs/42487791631 LLM reason (✨ experimental): The CI failure is due to a lint check indicating unused includes of the bitcoin-build-config.h header.

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.

laanwj force-pushed on May 19, 2025

fanquake commented at 3:47 pm on May 19, 2025: member

Concept ACK. Seems like there is some overlap here with #32380, but that this PR should go first, otherwise #32380 is going to be refactoring code, that is being deleted here.

laanwj commented at 4:06 pm on May 19, 2025: member

i’m not 100% sure ENABLE_SUBPROCESS needs to exist as an option at all, btw, it may be something to always enable, but maybe not on mobile/sandboxed platforms so wasn’t sure whether to make that decision here.

Edit: most notably, the ENABLE_SUBPROCESS=OFF path isn’t tested in the CI at all, it probably should? but in what run?

hebasto commented at 4:16 pm on May 19, 2025: member

Concept ACK.

Seems like there is some overlap here with #32380, but that this PR should go first, otherwise #32380 is going to be refactoring code, that is being deleted here.

I agree. And we are not going to switch to C++26 any time soon when #32380 will be required.

theStack commented at 5:02 pm on May 19, 2025: contributor

Concept ACK

laanwj force-pushed on May 19, 2025

laanwj force-pushed on May 20, 2025

DrahtBot removed the label CI failed on May 20, 2025

laanwj force-pushed on May 20, 2025

in src/util/subprocess.h:1134 in 2193963779 outdated

1129   for (auto arg : this->vargs_) {
1130+    if (!first_arg) {
1131+      command_line += L" ";
1132+    } else {
1133+      first_arg = false;
1134+    }

hebasto commented at 10:57 am on May 20, 2025:

Backported from upstream in #32567.

DrahtBot added the label CI failed on May 20, 2025

DrahtBot added the label Needs rebase on May 20, 2025

in src/common/system.cpp:52 in 3e2b4660b8 outdated

46-{
47-    if (strCommand.empty()) return;
48-#ifndef WIN32
49-    int nErr = ::system(strCommand.c_str());
50-#else
51-    int nErr = ::_wsystem(std::wstring_convert<std::codecvt_utf8_utf16<wchar_t>,wchar_t>().from_bytes(strCommand).c_str());

hebasto commented at 2:00 pm on May 20, 2025:

3e2b4660b84af872d114afc1556649f3dd93a1af:

#include <codecvt> might be removed now.

laanwj commented at 2:51 pm on May 20, 2025:

Done, thanks

laanwj force-pushed on May 20, 2025

in src/common/run_command.cpp:14 in ceb1783c1a outdated

10 #include <tinyformat.h>
11 #include <univalue.h>
12+#include <util/string.h>
13 
14-#ifdef ENABLE_EXTERNAL_SIGNER
15+#ifdef ENABLE_SUBPROCESS

fanquake commented at 3:38 pm on May 20, 2025:

I think you can just drop these #ifdefs around our own headers. Same for in RunCommandParseJSON below.

laanwj commented at 10:45 am on May 21, 2025:

Wasn’t sure about this. One reason for setting ENABLE_SUBPROCESS=OFF in the first place would be that the operating system doesn’t support subprocesses. As subprocess is a header-only library, including the subprocess.hpp header would mean use of fork execv CreateProcessW etc. So it could break the compile.

laanwj commented at 10:47 am on May 21, 2025:

If we don’t care about supporting such operating systems (only iOS comes to mind), it would imo be preferable to remove the option entirely (and with it, all the ifdefs).

fanquake commented at 9:06 am on May 22, 2025:

I think I agree. Given we don’t actually test or ship anything for iOS, it seems like we can clean up the code now, and figure out what to do if/when this might be an issue later.

DrahtBot removed the label Needs rebase on May 20, 2025

DrahtBot removed the label CI failed on May 20, 2025

in src/common/run_command.cpp:39 in ceb1783c1a outdated

36+#ifdef WIN32
37+        // Emulate _system behavior.
38+        auto c = sp::Popen("cmd.exe /c " + str_command);
39+#else
40+        // Emulate system(3) behavior, but don't leak file descriptors.
41+        auto c = sp::Popen({"/bin/sh", "-c", str_command}, sp::close_fds{true});

laanwj commented at 10:57 am on May 21, 2025:

TIL that upstream subprocess has a shell option that does the equivalent (https://github.com/arun11299/cpp-subprocess/blob/master/cpp-subprocess/subprocess.hpp#L1630) . But we removed it.

Unfortunately, currently it doesn’t support Windows. Could revert the option and switch to using that, when it does. Or, unless they had reason to not do it in the trivial way we do, it seems easy to add.

hebasto commented at 6:21 am on May 23, 2025:

A related discussion takes place at #32577.

hebasto commented at 6:28 am on May 23, 2025:

This is the first use of the Popen(std::initializer_list<const char*> cmd_args, Args&& ...args) ctor. Before this, it was a candidate for cleanup.

fanquake referenced this in commit 0a56ed1ac8 on May 21, 2025

build: Rename ENABLE_EXTERNAL_SIGNER to ENABLE_SUBPROCESS

Generalize it, because we're going to use subprocess for other things.

9930a5b77d

test: Rename external_signer methods to subprocess ff41f99f12

common: Change std::system usage to use subprocess 33ac946374

build: Remove HAVE_SYSTEM

It is no longer used.

82aeba60c2

laanwj force-pushed on May 21, 2025

laanwj commented at 11:42 am on May 21, 2025: member

Rebased for #32567: ceb1783c1af07f59885f51fdd15e7143fefdd821 → 82aeba60c2b3a166302e10fbf6e3eaf0593fab4d, which dropped one commit.

DrahtBot added the label Needs rebase on May 27, 2025

DrahtBot commented at 11:50 pm on May 27, 2025: contributor

🐙 This pull request conflicts with the target branch and needs rebase.

DrahtBot commented at 0:20 am on August 24, 2025: contributor

⌛ There hasn’t been much activity lately and the patch still needs rebase. What is the status here?

Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
Is it no longer relevant? ➡️ Please close.
Did the author lose interest or time to work on this? ➡️ Please close it and mark it ‘Up for grabs’ with the label, so that it can be picked up in the future.

achow101 commented at 2:55 pm on October 22, 2025: member

Are you still working on this?

maflcko commented at 3:05 pm on October 22, 2025: member

The main advantage of this is that file descriptor leaks are avoided (#32343).

For ref, this was reverted in #33063 (temporarily)

Use subprocess library for notifications #32566

Code Coverage & Benchmarks

Reviews

Conflicts