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

1. 
   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.
2. ipc, refactor: return std::optional<mp::SocketId> from checkSpawned() bd3a38f10a
3. ipc, refactor: rename arg -ipcfd to -ipcchild 8bca7555f8
4. DrahtBot added the label IPC on Aug 4, 2026
5. 
   DrahtBot commented at 8:47 PM on August 4, 2026: contributor

   <!--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/35887.

   <!--021abf342d371248e50ceaed478a90ca-->

   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>&lt;!--meta-tag:bot-skip--&gt;</code> into the comment that the bot should ignore.

   <!--174a7506f384e20aa4161008e828411d-->

   Conflicts

   No conflicts as of last run.

   <!--5faf32d7da4f0f540f40219e4f7537a3-->

   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.

   ^{2026-08-18 00:22:58}

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

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

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

9. ViniciusCestarii force-pushed on Aug 17, 2026
10. 
    ViniciusCestarii commented at 1:15 PM on August 17, 2026: contributor

    Thanks for the reviews! Forced-push 60c43329aa0f50c1bc6b801b858ebb13425fd971 adding a new unit test suggested.

11. ipc, test: add checkSpawned() unit tests 05e7d57318
12. ViniciusCestarii force-pushed on Aug 18, 2026
13. 
    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.

14. 
    jeanpablojp commented at 10:43 PM on August 19, 2026: contributor

    ACK 05e7d573184079720e11d933d24b9c9228477a14

15. DrahtBot requested review from enirox001 on Aug 19, 2026

Contributors

ViniciusCestariiDrahtBotenirox001jeanpablojp

Review Requested
enirox001