These changes make IPC features work on windows, resolving https://github.com/bitcoin-core/libmultiprocess/issues/53 and https://github.com/bitcoin-core/libmultiprocess/issues/114
PR is a draft since this is less of a priority compared to other multiprocess PR’s (see tracking issue #28722). Also it would probably make sense to split this up into separate commits. But the code is working and clean, and this could be reviewed and tested.
This PR is part of the process separation project.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32387.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| Concept ACK | hebasto |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
Reviewers, this pull request conflicts with the following ones:
Parse(U)Int* by maflcko)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.
Possible typos and grammar issues:
pr/ipc-win.1 -> pr/ipc-win.2, compare) fixing accidentally disabled tests
🚧 At least one of the CI tasks failed.
Task multiprocess, i686, DEBUG: https://github.com/bitcoin/bitcoin/runs/41420704031
LLM reason (✨ experimental): The CI failure is caused by an assertion failure in the IPC test suite due to a nullptr dereference.
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.
Updated 87432b6a4325e09a13c912d77b386daa3832b34d -> 0a35e21103b7fed6e46c809e9029446277b6f6ee (pr/ipc-win.2 -> pr/ipc-win.3, compare) to fix test CI failure https://cirrus-ci.com/task/5583224107171840 due to bad std::optional access
Updated 0a35e21103b7fed6e46c809e9029446277b6f6ee -> 27328195136754babe8d8f5d99f4f0b6a5ab2e55 (pr/ipc-win.3 -> pr/ipc-win.4, compare) to fix CI test failure https://cirrus-ci.com/task/5668412652781568 in rpc_misc.py echoipc call due to CLOEXEC flag
re: #32387 (comment)
Agree changes in #31756 should make this easier to test (though I might be able to make a minimal change here to turn on multiprocess windows build).
Having #30437 or #32297 merged first would also make test coverage more meaningful because they both add functional tests for -ipcbind and -ipcconect options.
In general there is more work to do here and I hope other PRs can be merged before this one.
Updated 27328195136754babe8d8f5d99f4f0b6a5ab2e55 -> f215e742045401ab386f0cd67d06b358558ecf89 (pr/ipc-win.4 -> pr/ipc-win.5, compare) fixing many windows bugs.
With this update, IPC code is mostly working on windows: mpexample and mptest programs work, test_bitcoin IPC calls over socketpairs work, bitcoin-node echoipc command is able to create a subprocess and successfully make calls to it, and bitcoin-node -ipcbind option creates a unix socket file. Some problems remain:
bitcoin-mine -ipcconnect seems unable to find the unix socket, failing with “Error: The system cannot find the file specified” even though the socket exists. Two test_bitcoin unix socket tests also give similar errors.test_bitcoin parse address tests fails because some checks are looking for / not \ separators.
Updated f215e742045401ab386f0cd67d06b358558ecf89 -> 5f272b7cc3b72fe4a59f590825b68dc2c342baca (pr/ipc-win.5 -> pr/ipc-win.6, compare) with more fixes.
Updated 5f272b7cc3b72fe4a59f590825b68dc2c342baca -> 13aef5ad7669e0302e27f96876615907dc98c3c3 (pr/ipc-win.6 -> pr/ipc-win.7, compare) improving comments.
With the new fixes, IPC is fully working on windows: both creating child processes and communicating over socket pairs, and listening and making connections over unix sockets. (Latter currently requires adding a SetupNetworking call to bitcoin-mine in #30437). The code changes are also mostly straightforward and clean, although I want to split commits to make them easier to review.
Will leave this PR in a draft state for now since other multiprocess PRs should have higher priority (see tracking issue #28722)
🐙 This pull request conflicts with the target branch and needs rebase.
Labels
Needs rebase