ipc: use std::optional for checkSpawned(), add tests and rename arg -ipcfd to -ipcchild #35887

pull ViniciusCestarii wants to merge 3 commits into bitcoin:master from ViniciusCestarii:checkspawned-tests changing 6 files +67 −24

ViniciusCestarii commented at 8:47 PM on August 4, 2026: contributor
Implement proposed follow-ups on dicussion #35084 (review):

<details>

<summary>Discussion</summary>

Sjors:

In https://github.com/bitcoin/bitcoin/commit/2d3f72fd3fa45ab4e399094c39bfa93bb5a87323 ipc, refactor: Update mp::SpawnProcess call: suggested test coverage:

BOOST_AUTO_TEST_CASE(check_spawned_test) { std::unique_ptripc::Process process{ipc::MakeProcess()}; mp::SocketId socket{mp::SocketError}; char arg0[]{"bitcoin-node"}; char arg1[]{"-ipcfd"}; char arg2[]{"invalid"}; char* argv[]{arg0, arg1, arg2}; auto check_error{[](const std::runtime_error& e) { return std::string_view{e.what()}.starts_with("Invalid -ipcfd number 'invalid'"); }}; BOOST_CHECK_EXCEPTION(process->checkSpawned(3, argv, socket), std::runtime_error, check_error); BOOST_CHECK_EQUAL(socket, mp::SocketError); }

ryanofsky:

re: #35084 (review)

This is a good test suggestion that would be a nice followup. Note that after https://github.com/bitcoin-core/libmultiprocess/pull/274, the exception will be a little different and look more like "StartSpawned: invalid connect_info" and after https://github.com/bitcoin-core/libmultiprocess/pull/231 the exception on windows will be "CreateFile(pipe) failed" so it could make sense to just check that an exception is thrown and not try to match the text.

Also:

It would be nice to change checkSpawned signature to return optional<SocketId> instead of using bool and an output parameter. (I believe this code is from before C++17 which added std::optional) Could be good to add another test case that doesn't pass an -ipcfd and ensures checkSpawned returns false without throwing an exception. Could be good to add another test case that just passes -ipcfd as the last argument. Could be nice to rename -ipcfd to something more generic like -ipcspawn or -ipcchild since a named pipe path instead of a file descriptor is passed on windows.

</details>
- Add unit test for checkSpawned().
- Make checkSpawned() return std::optionalmp::SocketId.
- Rename -ipcfd to -ipcchild.
ipc, refactor: return std::optional<mp::SocketId> from checkSpawned() bd3a38f10a
ipc, refactor: rename arg -ipcfd to -ipcchild 8bca7555f8
DrahtBot added the label IPC on Aug 4, 2026
DrahtBot commented at 8:47 PM on August 4, 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/35887.



Reviews

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

Type Reviewers

ACK jeanpablojp

Concept ACK enirox001

If your review is incorrectly listed, please copy-paste <code></code> into the comment that the bot should ignore.



Conflicts

Reviewers, this pull request conflicts with the following ones:
- #31260 (scripted-diff: Type-safe settings retrieval by ryanofsky)
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.



LLM Linter (✨ experimental)

Possible places where comparison-specific test macros should replace generic comparisons:
- src/ipc/test/ipc_tests.cpp BOOST_CHECK_THROW(process->checkSpawned(3, argv), std::runtime_error); -> consider BOOST_CHECK_EXCEPTION(..., std::runtime_error, HasReason(...)) so the test verifies the expected error message, not just the exception type.
<sup>2026-08-18 00:22:58</sup>
enirox001 commented at 9:14 AM on August 11, 2026: contributor

Concept ACK

Would be helpful if you linked the comments for the follow-ups in the PR description as that could aid with review
jeanpablojp commented at 4:54 PM on August 16, 2026: contributor

Approach ACK d01e65ccffe4f195789cf8aa6c7b9d60a72c877d

I have tested the code with IPC enabled and it is ok. I confirm that the follow-ups ryanofsky requested in #35084 were implemented. I left an inline suggestion for one more check_spawned_test case.
in src/ipc/test/ipc_tests.cpp:261 in d01e65ccff outdated
```
 256 | +    }
 257 | +    // -ipcchild combined with other arguments.
 258 | +    {
 259 | +        char* argv[]{arg0, arg_spawn, arg_invalid, arg_other};
 260 | +        BOOST_CHECK(!process->checkSpawned(4, argv));
 261 | +    }
```
jeanpablojp commented at 4:54 PM on August 16, 2026:

I think one case is missing: the negative cases all bail on argc != 3, so the strcmp never really runs. Tested locally without that half and the suite passed. Something like bitcoin-node -regtest -daemon, three arguments without -ipcchild, would cover it:

// three arguments, but the first is not -ipcchild. { char* argv[]{arg0, arg_other, arg_invalid}; BOOST_CHECK(!process->checkSpawned(3, argv)); }
ViniciusCestarii commented at 1:14 PM on August 17, 2026:

Nice catch, thanks for reviewing. Done 60c43329aa0f50c1bc6b801b858ebb13425fd971
ViniciusCestarii force-pushed on Aug 17, 2026
ViniciusCestarii commented at 1:15 PM on August 17, 2026: contributor

Thanks for the reviews! Forced-push 60c43329aa0f50c1bc6b801b858ebb13425fd971 adding a new unit test suggested.
ipc, test: add checkSpawned() unit tests 05e7d57318
ViniciusCestarii force-pushed on Aug 18, 2026
ViniciusCestarii commented at 12:24 AM on August 18, 2026: contributor

Forced push 05e7d573184079720e11d933d24b9c9228477a14 to rerun ci because some failed due to a github incident.
jeanpablojp commented at 10:43 PM on August 19, 2026: contributor

ACK 05e7d573184079720e11d933d24b9c9228477a14
DrahtBot requested review from enirox001 on Aug 19, 2026