#36105 bitcoin wrapper: Fix Windows exec so wrapper waits for child process
| Reviewability | Reviews | Agreement | Size | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| #36105 bitcoin wrapper: Fix Windows exec so wrapper waits for child process | Ready | 1 | Strong | 15 + 14 tests | |||||||||
|
|
|
|
| |||||||||
Reviewability: ReadyStalePaused Agreement: StrongPositivePositive w/ caveatsNeutralMildDisputedBlockedCrickets Reviews: current code-review ACKs, then (+stale ACKs) and -NACKs; greener = more ACKs. Size: lines added or modified outside tests, then in tests; greener = smaller. | |||||||||||||
Goal
Fix `util::ExecVp` on Windows to wait for child processes to finish and forward their exit codes instead of having the wrapper process exit immediately. Use `std::generic_category()` instead of `std::system_category()` when reporting POSIX `errno` errors from `ExecVp` failures on Windows. Re-enable the `tool_bitcoin.py` and non-vcpkg `interface_gui.py` functional tests on Windows.
Problem: On Windows, the `bitcoin.exe` wrapper calls CRT `_execvp`, which spawns the child process and immediately exits the parent wrapper with code 0. External callers, scripts, and the Python test framework cannot wait for the child, capture output, or inspect the exit status.
Evidence of importance:
- Description and commit 4453ead327: 'Switch from _execvp to _spawnvp(_P_NOWAIT) + _cwait so bitcoin.exe waits for the child process to finish and forwards its exit code. Previously _execvp would exit the parent as soon as the child started, making it impossible for anything waiting on bitcoin.exe (such as a test framework) to track whether the child succeeded or failed.'
- Commit 7fc08852e4 removes skips in `tool_bitcoin.py` and `interface_gui.py` that were previously introduced in #33229 and #35551 due to this exact Windows behavior.
Risk of a mistake: low (tooling, tests). A flaw would only affect Windows process execution within the bitcoin multi-process wrapper binary and test harness.
Changed since the previous assessment: Dropped the workaround handling msvcrt-specific EINVAL returns when probing for non-existent candidate binaries in src/bitcoin.cpp, keeping only the std::generic_category errno exception fix.
Reviewability
Ready
Nothing known that would invalidate a review now.
Waiting on (from the thread read):
- nothing: no one is asked for anything; all raised items are answered or accepted
Agreement
Strong
substantive support, no open objection (cyb3ralbert)
Thread notes: Added cyb3ralbert's Tested ACK (c:5930586612), updated cyb3ralbert's stance to support, and marked suggestion c:5714437254 as accepted.
Objections, suggestions, and questions:
| Reviewer | Type | Harm, request, or question | Status | Must be | Author replied | Quote |
|---|---|---|---|---|---|---|
| hebasto (member) | suggestion, scope | Drop the commit fixing MSVCRT regressions once #33593 merges and removes MSVCRT support | fixed · fix pushed · author dropped the MSVCRT workaround commit in push ad98271f after #33593 merged | answered | yes | 2026-08-27: 'renders the "bitcoin: Fix msvcrt regressions from _execvp to _spawnvp switch" commit unnecessary' Status from: 2026-09-23: ryanofsky: 'Thanks dropped 68ae4a0f84b12587b9e2b53d3e306460fad4244a workaround.' |
| hodlinator (member) | question, interface | asks whether the runtime difference in error codes between MSVCRT and UCRT is documented elsewhere | accepted · author confirmed it was only observed in CI and added tests/comments; reviewer accepted in rc:3982725901 | answered | yes | 2026-08-31: 'Checked https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/spawnvp-wspawnvp?view=msvc-170 but this difference in error codes between runtimes is not documented there. Is it admitted els' Status from: 2026-09-10: hodlinator: 'Impressive testing by @ryan8or. ^^ (https://github.com/ryanofsky/bitcoin/actions/runs/34367754505)' |
| hodlinator (member) | question, approach | asks why interface_gui.py is skipped on vcpkg builds instead of fixing QT_PLUGIN_PATH to make it run | answered · author explained that fixing bitcoin-qt execution is better handled in a separate PR and improved documentation in the meantime | answered | yes | 2026-09-10: 'So why would you be changing the functional test to skip something which should be working?' Status from: 2026-09-23: ryanofsky: 'I think this is probably better to address in a separate pr since this PR is supposed be a bitcoin.exe bugfix, not chang' |
| cyb3ralbert (contributor) | objection, interface | MSVCRT builds report an unhelpful 'Invalid argument' error instead of 'No such file or directory' when the target executable is missing | accepted · fix pushed · reviewer withdrew the concern in c:5714437254 after #33593 dropped MSVCRT support, and author dropped the workaround commit in r:5296577726 | answered | yes | 2026-09-13: 'When the target executable is missing, the error the user sees differs between runtimes: MSVCRT: Error: execvp failed to execute '...\bitcoind': Invalid argument' Status from: 2026-09-17: cyb3ralbert: 'My own comment above about mapping EINVAL to ENOENT goes away with it, for the same reason.' |
| cyb3ralbert (contributor) | suggestion, scope | drop the MSVCRT workaround commit 68ae4a0f upon rebasing on #33593 while keeping generic errno handling commit 2b877984 | accepted · fix pushed · reviewer confirmed commit 68ae4a0f was dropped as suggested and provided a Tested ACK | answered | yes | 2026-09-17: 'Suggestion for the rebase on #33593: drop 68ae4a0f, keep 2b877984. They read as one story, but only the first one is MSVCRT-specific.' Status from: 2026-10-01: cyb3ralbert: '68ae4a0f is dropped, as suggested earlier: https://github.com/bitcoin/bitcoin/pull/36105#issuecomment-5714437254' |
| maflcko (member) | suggestion, style | Use prose attribution instead of Co-authored-by tags for AI assistance | answered · author agreed to switch to prose attribution in future commits | answered | yes | 2026-09-17: 'Otherwise, the co-author tags are only useful to you, but not really useful to anyone else.' Status from: 2026-09-23: ryanofsky: 'Thanks, added a regex to switch to prose attribution in future commits.' |
Support:
- hebasto (member) (Concept ACK, 2026-08-27): Concept ACK. [not substantive]
- hodlinator (member) (Concept ACK, 2026-08-31): Concept ACK 30ae05a21bdb30e63400385cf6d9dfd87dccfc70 [not substantive]
- cyb3ralbert (contributor) (Tested ACK, 2026-10-01) [tested: test/functional/tool_bitcoin.py]: 68ae4a0f was dropped as suggested; tested tool_bitcoin.py on Linux and Windows cross build + native test on windows-2022
Participants:
- hebasto (member, support, 4 statements 2026-08-27 to 2026-09-16): Concept ACK; suggested dropping MSVCRT workaround commit once #33593 merged
- hodlinator (member, support, 8 statements 2026-08-31 to 2026-09-10): Concept ACK; reviewed code and investigated Windows test behavior
- cyb3ralbert (contributor, support, 3 statements 2026-09-13 to 2026-10-01): Tested ACK on ad98271f after MSVCRT workaround was dropped; previous error message objection was withdrawn
- maflcko (member, neutral, 2 statements 2026-09-17 to 2026-09-17): commented on commit attribution style regarding AI policy
Dependencies
Files
33 lines under test/bench/ci.
- test/functional/interface_gui.py +6/-7
- src/util/exec.cpp +11/-1
- test/functional/tool_bitcoin.py +0/-11
- src/bitcoin.cpp +4/-1
- src/qt/test/CMakeLists.txt +3/-1
- test/functional/test_framework/test_framework.py +4/-0
- test/config.ini.in +1/-0
Card
The short text the ranking pass compares across PRs.
Fixes util::ExecVp on Windows so the bitcoin.exe wrapper blocks until its child process exits and forwards the child exit status. Previously, _execvp exited the parent wrapper immediately upon spawning the child, preventing scripts and test harnesses on Windows from capturing process output or checking exit codes. The change switches to _spawnvp(_P_NOWAIT) followed by _cwait, fixes errno reporting on exec failures, and re-enables tool_bitcoin.py and interface_gui.py on Windows.
Data
thread read through 2026-10-01 (27 statements), head ad98271f51
labels: none
Categories
IPC / multiprocess
P2 · platform fix
Fixes Windows process execution for the `bitcoin` wrapper so it properly waits for child processes and returns exit codes. The wrapper was exiting immediately with code 0 on Windows, which broke external callers and forced functional tests like `interface_gui.py` to be skipped.
Membership: Changes the `bitcoin` wrapper executable and process management in `src/bitcoin.cpp`, explicitly covered under IPC.
Factors: security/stability 1, bug 2, performance 0, user value 1, leverage 1
Tools and scripts
P2 · platform fix
Resolves a broken execution model for the `bitcoin.exe` CLI wrapper on Windows. Previously, `_execvp` exited the wrapper immediately with status 0 while the child ran asynchronously, preventing callers from waiting on completion or checking return codes.
Membership: Modifies the behavior of the `bitcoin` wrapper executable (`src/bitcoin.cpp`), explicitly covered under tools.
Factors: security/stability 1, bug 2, performance 0, user value 1, leverage 1
Utilities (logging, arguments, libraries)
P3 · platform fix
Corrects platform-specific process execution behavior and error category mapping in `util::ExecVp`. While helpful for callers of `ExecVp`, its use is limited to wrapper process invocation.
Membership: Modifies `util::ExecVp` in `src/util/exec.cpp` to correctly wait and forward exit codes on Windows, as well as fixing POSIX errno category reporting.
Factors: security/stability 1, bug 2, performance 0, user value 1, leverage 1