test: Check bitcoin wrapper child exit status on windows #36190

Follow-up to #36105, which makes the Windows bitcoin.exe wrapper wait for its child and forward the child's exit code. This adds the functional test for that.

The case worth covering is exit code -1. The first version of the wrapper used _spawnvp(_P_WAIT), which returns -1 both when the spawn fails and when the child started fine and exited with -1, so bitcoin.exe reported a startup error and exited 1 instead of passing -1 through. #36105 now uses _P_NOWAIT + _cwait, which keeps the spawn result and the child status separate. Against the old launcher this test fails at not(1 == 4294967295).

The test copies bitcoin.exe into a temp dir next to a copy of cmd.exe named bitcoind.exe, so bitcoin -M node /d /c exit N runs the fake child, and checks 0, 1 and -1 come back unchanged. Windows reports exit codes as unsigned 32-bit, so -1 arrives as 4294967295. It also checks the wrapper adds nothing to stdout/stderr. The exit code test is Windows only.

ryanofsky preferred keeping the runtime fix in #36105 and reviewing the test here (https://github.com/bitcoin/bitcoin/pull/36190#issuecomment-5603692520), so this is the same commit he briefly carried in his branch. The branch still contains #36105's commits and should land after it.

Checked on a native Windows 11 clang/UCRT release build, wallet/GUI/IPC off:

cmake --build build --target bitcoin bitcoind bitcoin-cli
python test/functional/tool_bitcoin.py --configfile=build/test/config.ini --tmpdir=build/tool-bitcoin-exit-status

Fails against the pre-fix launcher, passes against the fixed one, and the rest of tool_bitcoin.py still passes. Use a fresh --tmpdir when re-running.

<!--e57a25ab6845829454e8d69fc972939a-->

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

<!--006a51241073e994b41acfe9ec718e94-->

Code Coverage & Benchmarks

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

<!--021abf342d371248e50ceaed478a90ca-->

Reviews

See the guideline and AI policy for information on the review process.

If your review is incorrectly listed, please copy-paste <code>&lt;!--meta-tag:bot-skip--&gt;</code> into the comment that the bot should ignore.

<!--174a7506f384e20aa4161008e828411d-->

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #36106 (bitcoin wrapper: respect CMAKE_INSTALL_BINDIR/LIBEXECDIR by ryanofsky)
• #36022 (test: add coverage for bitcoin wrapper argument handling by cyb3ralbert)

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.

<!--5faf32d7da4f0f540f40219e4f7537a3-->

Thanks @Bortlesboat! I added your fix and test to #36105 since I think it makes more sense to change the execvp code once instead of multiple times. I think with those changes it probably makes sense to close this PR. I can also revert or partially revert though if you'd prefer this PR to be separate.

Thanks Ryan. I'd prefer to keep the exit-status regression here as a test-only follow-up, with the runtime fix staying in #36105. Would you be happy to leave the test and its imports out of your branch? I can rebase this PR once that's done.

re: #36190 (comment)

Would you be happy to leave the test and its imports out of your branch?

No problem, dropped the test and happy to review it here

Tested the test commit (b78e0a9d) on both Linux and a native Windows runner. It does what it says: reverting _P_NOWAIT + _cwait back to _P_WAIT is caught, and the -1 case is the one that catches it.

One gap I'd suggest closing while this is still open.

The bug the commit message refers to was actually a conflation: _spawnvp(_P_WAIT) returns -1 both when the spawn fails and when the child exits with -1. The test covers one side of that — a child exiting with -1 is forwarded — but not the other: a failed spawn must still be reported as a wrapper error rather than forwarded as a child status.

Nothing in tool_bitcoin.py currently covers the failure path, and the branch carries a commit dedicated to it (68ae4a0f, where MSVCRT returns EINVAL instead of ENOENT for a path with a directory component).

Leaving bitcoind out of the wrapper directory is enough, since the wrapper is invoked by absolute path and therefore does not fall back to a PATH search:

        # The other half of what _P_NOWAIT + _cwait separates: a child that
        # could not be launched at all must be reported as a wrapper error, not
        # forwarded as a child exit status. Leave the internal executable out so
        # the lookup fails. The wrapper is invoked by absolute path, so it does
        # not fall back to searching PATH.
        self.log.info("Ensure bitcoin reports a launch failure instead of a child exit status")
        missing_dir = self.nodes[0].datadir_path / "exit_status_missing"
        missing_dir.mkdir()
        missing_wrapper = missing_dir / "bitcoin.exe"
        shutil.copyfile(self.get_binaries().paths.bitcoin_bin, missing_wrapper)
        result = subprocess.run([str(missing_wrapper), "-M", "node", "-version"],
                                capture_output=True, timeout=30)
        assert_equal(result.returncode, 1)
        assert b"failed to execute" in result.stderr

I ran this on a windows-2022 runner against the cross-built executables from this branch, with both CRTs, and it passes: the wrapper exits with 1 and reports its own error instead of forwarding a child status.

I also rebuilt the branch with ExecVp reverted to _spawnvp(_P_WAIT) and ran the test against that, with both CRTs: it fails at AssertionError: not(1 == 4294967295), as described. Worth noting that the launch-failure case is never reached there — the old code did report a failed spawn correctly, so that side genuinely was left uncovered.

One more thing that fell out of running the above: the message the user gets differs between CRTs.

MSVCRT: Error: execvp failed to execute '...\bitcoind': Invalid argument
UCRT:   Error: execvp failed to execute '...\bitcoind': No such file or directory

That is the EINVAL from 68ae4a0f surfacing: the intermediate lookups treat it as "not found" and move on, but the last attempt has allow_notfound=false, so errno goes into the std::system_error as-is, and an MSVCRT build reports "Invalid argument" for a file that simply isn't there. Not a problem with this test — if anything it's an argument for covering the case — and the fix would belong in #36105.

Environment:

• Linux: Debian, GCC 12.2.0, Release, BUILD_TESTS=OFF, ENABLE_IPC=OFF, Python 3.11 — tool_bitcoin.py passes; the Windows case is skipped.
• Windows: windows-2022 runner with the UCRT and MSVCRT cross-built binaries from this branch — tool_bitcoin.py passes on both.

re: #36190 (comment)

I had trouble understanding the comment at first, but it's just suggesting adding a second test to accompany the existing test in b78e0a9d3e004988c54a5438df0fee16d6abc7f3. The existing test confirms exit codes (0, 1, -1) from bitcoind all get passed through the bitcoin wrapper. But it doesn't confirm that an execvp failed to execute error appears if bitcoind can't be started at all.

This seems like a nice thing to check. The original bug in 80fc82c4aed020afc0f8e2230886733282f9b755 fixed in 6bce70ad0330467abc38e5ee23f3fc3b4379112b was that the execvp failed to execute error previously trigger incorrectly when bitcoind returned -1, and now it will correctly exit with -1 instead. But it would be good to check that the error still does trigger in cases where it is supposed to trigger.

re: #36190 (comment)

Thanks for restating my points more clearly than my original comment. That's exactly what was needed.

ACK f2a1beb29873643ad382deb5d1848bd8572f4105

Ran tool_bitcoin.py against this head on Linux (Debian, GCC 12.2.0) and on a windows-2022 runner with the cross-built binaries, both CRTs. It passes in all three, and the new case is reached in each. @Bortlesboat thanks for catching the copyfile → copy issue — my snippet only ever ran under the Windows branch, so it got away without the executable bit. Checking this on every platform is better too.

Code review ACK f2a1beb29873643ad382deb5d1848bd8572f4105 (last 2 commits only). Both tests are well written and add useful coverage.

Converted this to a draft since it depends on #36105