Use subprocess library for notifications #32566

laanwj · 2025-05-19T14:55:35Z

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.

DrahtBot · 2025-05-19T14:55:38Z

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.

DrahtBot · 2025-05-19T15:00:47Z

🚧 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.}

Hints

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.

fanquake · 2025-05-19T15:47:08Z

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 · 2025-05-19T16:06:40Z

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 · 2025-05-19T16:16:02Z

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 · 2025-05-19T17:02:11Z

Concept ACK

src/util/subprocess.h

src/common/system.cpp

fanquake · 2025-05-20T15:38:49Z

src/common/run_command.cpp

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

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.

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).

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.

laanwj · 2025-05-21T10:57:32Z

src/common/run_command.cpp

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.

A related discussion takes place at #32577.

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.

e63a703 subprocess: Don't add an extra whitespace at end of Windows command line (laanwj) Pull request description: A list of the backported PRs: - arun11299/cpp-subprocess#119 The following PRs were skipped for backporting: - arun11299/cpp-subprocess#118 because there is no changes in the header code. Required for #32566. ACKs for top commit: laanwj: Code review ACK e63a703 Tree-SHA512: 69a74aa7f9c611a9ec910e27161c5e9e147067d37f8335953cd3875fcc88dc840a2f7b206bb603f22507159e406b1449f1dc4702fffe890bb824672641b4feed

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

It is no longer used.

laanwj · 2025-05-21T11:42:46Z

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

DrahtBot · 2025-05-27T23:50:01Z

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

DrahtBot · 2025-08-24T00:20:37Z

⌛ 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 · 2025-10-22T14:55:56Z

Are you still working on this?

maflcko · 2025-10-22T15:05:48Z

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

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

53e4951 Switch to ANSI Windows API in `fsbridge::fopen()` function (Hennadii Stepanov) dbe770d Switch to ANSI Windows API in `Win32ErrorString()` function (Hennadii Stepanov) 06d0be4 Remove no longer necessary `WinCmdLineArgs` class (Hennadii Stepanov) f366408 cmake: Set process code page to UTF-8 on Windows (Hennadii Stepanov) dccbb17 Set minimum supported Windows version to 1903 (May 2019 Update) (Hennadii Stepanov) Pull request description: The main goal is to remove [deprecated](#32361) code (removed in C++26). This PR employs Microsoft's modern [approach](https://learn.microsoft.com/en-us/windows/apps/design/globalizing/use-utf8-code-page) to handling UTF-8: > Until recently, Windows has emphasized "Unicode" -W variants over -A APIs. However, recent releases have used the ANSI code page and -A APIs as a means to introduce UTF-8 support to apps. If the ANSI code page is configured for UTF-8, then -A APIs typically operate in UTF-8. This model has the benefit of supporting existing code built with -A APIs without any code changes. TODO: - [x] Handle application manifests properly when building with MSVC. - [x] Bump the minimum supported Windows version to 1903 (May 2019 Update). - [x] Remove all remaining use cases of the deprecated `std:wstring_convert`. - The instance in `subprocess.h` will be addressed in a follow-up PR, as additional tests are likely needed. - The usage in `common/system.cpp` is handled in #32566. Resolves partially #32361. ACKs for top commit: laanwj: re-ACK 53e4951 hodlinator: re-ACK 53e4951 davidgumberg: untested crACK 53e4951 Tree-SHA512: 0dbe9badca8b979ac2b4814fea6e4a7e53c423a1c96cb76ce894253137d3640a87631a5b22b9645e8f0c2a36a107122eb19ed8e92978c17384ffa8b9ab9993b5

laanwj added Build system Utils/log/libs labels May 19, 2025

laanwj force-pushed the 2025-05-subprocess-system branch from 397ff54 to 7b25522 Compare May 19, 2025 14:59

DrahtBot added the CI failed label May 19, 2025

laanwj force-pushed the 2025-05-subprocess-system branch 3 times, most recently from 8aebdab to e32bfc4 Compare May 19, 2025 15:43

laanwj force-pushed the 2025-05-subprocess-system branch 4 times, most recently from 9069ed3 to 722cd1d Compare May 19, 2025 17:44

DrahtBot mentioned this pull request May 19, 2025

Modernize use of UTF-8 in Windows code #32380

Merged

3 tasks

laanwj force-pushed the 2025-05-subprocess-system branch 3 times, most recently from ec5c0c0 to 27d4ae1 Compare May 19, 2025 20:44

DrahtBot mentioned this pull request May 19, 2025

Reintroduce external signer support for Windows #29868

Merged

laanwj force-pushed the 2025-05-subprocess-system branch from 27d4ae1 to 02d7fb6 Compare May 19, 2025 22:03

This was referenced May 19, 2025

scripted-diff: Use LogInfo over LogPrintf #29641

Merged

build: Embedded ASMap [3/3]: Build binary dump header file #28792

Open

laanwj force-pushed the 2025-05-subprocess-system branch from 02d7fb6 to 56fb60a Compare May 20, 2025 07:45

DrahtBot removed the CI failed label May 20, 2025

laanwj force-pushed the 2025-05-subprocess-system branch 2 times, most recently from 9de7029 to 2193963 Compare May 20, 2025 09:13

hebasto mentioned this pull request May 20, 2025

subprocess: Backport upstream changes #32567

Merged

hebasto reviewed May 20, 2025

View reviewed changes

src/util/subprocess.h Outdated Show resolved Hide resolved

DrahtBot added CI failed Needs rebase labels May 20, 2025

hebasto reviewed May 20, 2025

View reviewed changes

src/common/system.cpp Outdated Show resolved Hide resolved

laanwj force-pushed the 2025-05-subprocess-system branch 2 times, most recently from 14362bc to ceb1783 Compare May 20, 2025 14:51

fanquake reviewed May 20, 2025

View reviewed changes

DrahtBot removed Needs rebase CI failed labels May 20, 2025

DrahtBot mentioned this pull request May 20, 2025

rpc: Round verificationprogress to 1 for a recent tip #32528

Merged

laanwj commented May 21, 2025

View reviewed changes

laanwj added 4 commits May 21, 2025 13:40

build: Rename ENABLE_EXTERNAL_SIGNER to ENABLE_SUBPROCESS

9930a5b

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

test: Rename external_signer methods to subprocess

ff41f99

common: Change std::system usage to use subprocess

33ac946

build: Remove HAVE_SYSTEM

82aeba6

It is no longer used.

laanwj force-pushed the 2025-05-subprocess-system branch from ceb1783 to 82aeba6 Compare May 21, 2025 11:41

laanwj mentioned this pull request May 21, 2025

subprocess: Let shell parse command on non-Windows systems #32577

Draft

DrahtBot added the Needs rebase label May 27, 2025

Use subprocess library for notifications #32566

Are you sure you want to change the base?

Use subprocess library for notifications #32566

Conversation

laanwj commented May 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented May 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage & Benchmarks

Reviews

Conflicts

Uh oh!

DrahtBot commented May 19, 2025

Uh oh!

fanquake commented May 19, 2025

Uh oh!

laanwj commented May 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hebasto commented May 19, 2025

Uh oh!

theStack commented May 19, 2025

Uh oh!

Uh oh!

Uh oh!

fanquake May 20, 2025

Choose a reason for hiding this comment

Uh oh!

laanwj May 21, 2025

Choose a reason for hiding this comment

Uh oh!

laanwj May 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fanquake May 22, 2025

Choose a reason for hiding this comment

Uh oh!

laanwj May 21, 2025

Choose a reason for hiding this comment

Uh oh!

hebasto May 23, 2025

Choose a reason for hiding this comment

Uh oh!

hebasto May 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laanwj commented May 21, 2025

Uh oh!

DrahtBot commented May 27, 2025

Uh oh!

DrahtBot commented Aug 24, 2025

Uh oh!

achow101 commented Oct 22, 2025

Uh oh!

maflcko commented Oct 22, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

laanwj commented May 19, 2025 •

edited

Loading

DrahtBot commented May 19, 2025 •

edited

Loading

laanwj commented May 19, 2025 •

edited

Loading

laanwj May 21, 2025 •

edited

Loading

hebasto May 23, 2025 •

edited

Loading