Add windows support - #231
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline and AI policy for information on the review process.
If your review is incorrectly listed, please copy-paste ConflictsReviewers, this pull request conflicts with the following ones:
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 typos and grammar issues:
2026-08-05 04:48:54 |
|
We have miners using Stratum V2 that are on Windows and would like to see Windows IPC support landing, is there any estimate when they can expect this to materialize? We have a dozen of users who can help with testing if that's a blocker. |
|
@pavlenex testing would definitely be useful. I guess for that to work we'd need a stack of pull requests: this one here in libmultiprocess, one in Bitcoin Core that enables IPC support in the Windows Guix build, and then an SRI pull request that uses it. Testers would then have download (or build themselves) the custom bitcoin core and SRI binaries. I've added Windows support to my v32 wish list: bitcoin/bitcoin#33777 No guarantees obviously. |
|
I'll try to get this PR ready for review this week, split up into smaller commits and with ci passing. From there as Sjors mentioned there is a lot more work to do: more code changes in bitcoin/bitcoin#32387 that need to be made in bitcoin core, enabling IPC into windows builds in bitcoin core, enabling it in windows CI jobs with pycapnp, adding client support, probably adding a windows CI job to this repository. v32 sounds like a good target though and it is very useful to know there is demand for this feature, because it hasn't been a priority so far |
|
PR is split up into commits now and should be reviewable. CI is not passing but failures look like IWYU errors. I also opened bitcoin/bitcoin#35084 with corresponding bitcoin core changes. Windows support for bitcoin core can be tested with bitcoin/bitcoin#32387 which combines both PRs and enables IPC by default in windows builds. Rebased 2975fac -> cb16d2e ( Updated cb16d2e -> e563c96 ( Updated e563c96 -> d9fcac6 ( Added 1 commits d9fcac6 -> a1748e2 ( Updated a1748e2 -> 18fc188 ( Updated 18fc188 -> 7fd5ec4 ( |
e563c96 to
d9fcac6
Compare
|
How realistic is it to add a Windows CI job here? (can be cross-compiled) |
The lifecycle test repeatedly constructs and destroys TPTester to exercise IPC EventLoop teardown. On Windows it hangs intermittently in std::thread::join during teardown of libmultiprocess thread-local state. This is a known libmultiprocess Windows issue, not specific to sv2-tp: bitcoin-core/libmultiprocess#231 bitcoin/bitcoin#32387 The fix lives upstream and rewrites the EventLoop wakeup primitive (raw fd -> KJ stream) and adds shutdownWrite() in ~Connection. Until that lands and is backported into our libmultiprocess subtree, gate this particular test off on _WIN32. The other sv2 unit tests run normally on Windows; only the explicit teardown loop is affected. Assisted-by: GitHub Copilot Assisted-by: Anthropic Claude Opus 4
The template_provider_tests `client_tests`, `fee_timer_blocking_test` and `new_tip_bypasses_fee_timer_test` cases each construct and tear down a TPTester. Like the lifecycle test skipped in the previous commit, that teardown deadlocks intermittently on Windows in std::thread::join during libmultiprocess thread-local state cleanup. This is a known libmultiprocess Windows issue, not specific to sv2-tp: bitcoin-core/libmultiprocess#231 bitcoin/bitcoin#32387 Gate the three TPTester-using cases off on _WIN32 with a TODO. The non-IPC test `block_reserved_weight_floor` continues to run on Windows. Assisted-by: GitHub Copilot Assisted-by: Anthropic Claude Opus 4
Sjors
left a comment
Sjors
left a comment
There was a problem hiding this comment.
Some feedback on the first commit.
| //! Returns child process id and socket id. | ||
| //! | ||
| //! The connection string is just a file descriptor number on unix, and the | ||
| //! child process can call SpawnProcess to parse it. |
There was a problem hiding this comment.
In 37a2190 Improve SpawnProcess API and documentation: did you mean StartSpawned instead of SpawnProcess?
//! The connection string is just a file descriptor number on unix, and the
//! child process can call StartSpawned to get a socket handle from the connection string.
For other reviewers, here's a diagram of how StartSpawned, SpawnProcess and the spawn_argv dance, based on the calculator example:
There was a problem hiding this comment.
re: #231 (comment)
Good catch, fixed StartSpawned references. SpawnProcess and StartSpawned complement each other with SpawnProcess being called by parent and returing socket id, and StartSpawned being called by child and returning socket id.
ConnectStream and ServeStream are also complements of each other with ServeStream wrapping a C++ interface pointer and serving it over a stream, and ConnectStream wrapping a stream and returning a C++ interface pointer.
Usually ConnectStream is paired with SpawnProcess in the parent, and ServeStream is paired with StartSpawned in the child because the parent wants to call C++ methods on some interface provided by the child. But it's also possible to do the reverse and have the parent serve an interface to the child, or to not use Connect and Serve at all and just communicate directly over the socketpair.
| //! Returns child process id and socket id. | ||
| //! | ||
| //! The connection string is just a file descriptor number on unix, and the | ||
| //! child process can call SpawnProcess to parse it. |
There was a problem hiding this comment.
re: #231 (comment)
Good catch, fixed StartSpawned references. SpawnProcess and StartSpawned complement each other with SpawnProcess being called by parent and returing socket id, and StartSpawned being called by child and returning socket id.
ConnectStream and ServeStream are also complements of each other with ServeStream wrapping a C++ interface pointer and serving it over a stream, and ConnectStream wrapping a stream and returning a C++ interface pointer.
Usually ConnectStream is paired with SpawnProcess in the parent, and ServeStream is paired with StartSpawned in the child because the parent wants to call C++ methods on some interface provided by the child. But it's also possible to do the reverse and have the parent serve an interface to the child, or to not use Connect and Serve at all and just communicate directly over the socketpair.
d3d74e7 ipc, refactor: Update mp::g_thread_context references (Ryan Ofsky) 2d3f72f ipc, refactor: Update mp::SpawnProcess call (Ryan Ofsky) e9f1981 ipc, refactor: Add Stream type alias and use it (Ryan Ofsky) 3859805 ipc, refactor: Add SocketId type alias and use it (Ryan Ofsky) 2ee9b69 ipc, refactor: Add ProcessId type alias and use it (Ryan Ofsky) 3449797 ipc: Avoid 'unistd.h' error with MSVC (Ryan Ofsky) dbcc192 ipc, refactor: fix include order (Ryan Ofsky) 7c86d48 ipc, refactor: use native path separators in test (Ryan Ofsky) 00287b9 ipc, refactor: Change Protocol class field order (Ryan Ofsky) 33d37f3 ipc, refactor: Drop connect/listen/serve exe_name parameters (Ryan Ofsky) 7949404 ipc, moveonly: combine ipc_test.cpp and ipc_tests.cpp (Ryan Ofsky) Pull request description: This PR makes Bitcoin Core changes needed to be compatible with bitcoin-core/libmultiprocess#274, which changes the libmultiprocess API to stop using unix-specific types so it is compatible with windows. (Windows support is added in followups: bitcoin-core/libmultiprocess#231 and #32387.) The PR uses some [compatibility shims](https://github.com/ryanofsky/bitcoin/blob/pr/ipc-wins/src/ipc/util.h) so it can be reviewed and merged without needing to merge bitcoin-core/libmultiprocess#274 first and bump the libmultiprocess subtree. These can be deleted when the subtree is updated. --- Review note: All the changes here are refactoring, and you don't really need to know anything about IPC or Windows to review this code. It is also a mostly move-only change (131 lines added, 96 removed, 215 moved) ACKs for top commit: xyzconstant: tACK d3d74e7 enirox001: ACK d3d74e7 Sjors: ACK d3d74e7 ViniciusCestarii: re-ACK d3d74e7 tested locally on Linux Tree-SHA512: cd48708f9fd086ac8127dc75cfaf4bd8f8da81e07d11b2c9e65fd9061ffa33478bffc6fd6fa4b3505e86c6437752578fe6e5bd590c683c3bc9969093103a5608
6101a2e ipc, refactor: Update mp::g_thread_context references (Ryan Ofsky) 8f9f52c ipc, refactor: Update mp::SpawnProcess call (Ryan Ofsky) ff56e7a ipc, refactor: Add Stream type alias and use it (Ryan Ofsky) c0a7490 ipc, refactor: Add SocketId type alias and use it (Ryan Ofsky) 56c0011 ipc, refactor: Add ProcessId type alias and use it (Ryan Ofsky) c4c23f6 ipc: Avoid 'unistd.h' error with MSVC (Ryan Ofsky) 8a9cd6e ipc, refactor: fix include order (Ryan Ofsky) a75df3e ipc, refactor: use native path separators in test (Ryan Ofsky) 83817f3 ipc, refactor: Change Protocol class field order (Ryan Ofsky) e93ffef ipc, refactor: Drop connect/listen/serve exe_name parameters (Ryan Ofsky) 4b41ceb ipc, moveonly: combine ipc_test.cpp and ipc_tests.cpp (Ryan Ofsky) Pull request description: This PR makes Bitcoin Core changes needed to be compatible with bitcoin-core/libmultiprocess#274, which changes the libmultiprocess API to stop using unix-specific types so it is compatible with windows. (Windows support is added in followups: bitcoin-core/libmultiprocess#231 and bitcoin/bitcoin#32387.) The PR uses some [compatibility shims](https://github.com/ryanofsky/bitcoin/blob/pr/ipc-wins/src/ipc/util.h) so it can be reviewed and merged without needing to merge bitcoin-core/libmultiprocess#274 first and bump the libmultiprocess subtree. These can be deleted when the subtree is updated. --- Review note: All the changes here are refactoring, and you don't really need to know anything about IPC or Windows to review this code. It is also a mostly move-only change (131 lines added, 96 removed, 215 moved) ACKs for top commit: xyzconstant: tACK 6101a2e enirox001: ACK 6101a2e Sjors: ACK 6101a2e ViniciusCestarii: re-ACK 6101a2e tested locally on Linux Tree-SHA512: cd48708f9fd086ac8127dc75cfaf4bd8f8da81e07d11b2c9e65fd9061ffa33478bffc6fd6fa4b3505e86c6437752578fe6e5bd590c683c3bc9969093103a5608
Remove recently introduced SpawnConnectInfo and SpawnConnectInfoToArgsFn type aliases since they are the same on all platforms and might obscure the fact that connect info should be treated as an opaque string. Co-authored-by: Sjors Provoost <sjors@sprovoost.nl>
Co-authored-by: Sjors Provoost <sjors@sprovoost.nl>
Add Windows-specific code to support building and running on Windows: - util.h: Guard ProcessId/SocketId/SocketError type aliases with WIN32 ifdefs so they use SOCKET/uintptr_t on Windows and int on Unix. Add winsock2.h include on Windows. - util.cpp: Guard Unix-specific system headers with WIN32 ifdefs. Add Windows-specific includes (windows.h, winsock2.h). Guard MaxFd() with #ifndef WIN32. Add GetCurrentThreadId() branch in ThreadName(). Add win32Socketpair() forward-declare. Add Windows branch in SocketPair() using win32Socketpair(). Add CommandLineFromArgv() helper needed to construct CreateProcess command lines. Add Windows branch in SpawnProcess() using named pipes and WSADuplicateSocket to pass socket to child. Add Windows branch in StartSpawned() reading socket from named pipe. Add Windows branch in WaitProcess() using WaitForSingleObject/GetExitCodeProcess. - proxy.cpp: Add SocketOutputStream class on Windows (analogous to FdOutputStream but using SOCKET/send()). Add Windows branch in EventLoop constructor to create m_post_writer using SocketOutputStream.
There was a problem hiding this comment.
Approach ACK b7e3676.
Looking good, build locally with mingw and ran tests and examples with wine.
I found a couple of issues in SpawnProcess that I think are worth addressing and left some nits.
…calls ConnectNamedPipe synchronously. If the child exits or crashes before connecting, ConnectNamedPipe blocks forever with no recovery path. Fix by opening the pipe with FILE_FLAG_OVERLAPPED and using WaitForMultipleObjects on both the connect event and the child process handle. If the process handle signals first, the child died without connecting and SpawnProcess throws instead of hanging. Since the pipe is now in overlapped mode, WriteFile also requires an OVERLAPPED structure; use GetOverlappedResult with bWait=TRUE to handle both synchronous and asynchronous completion. Add a Windows-only test that spawns a child which exits immediately without opening the named pipe and asserts SpawnProcess does not block. (bitcoin-core#231 (comment)) Co-Authored-By: ViniciusCestarii <viniciuscestari01@gmail.com> Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Allow code to compile without pthreads available, as required for MSVC compatibility. Avoid unconditional POSIX calls (fork, posix_spawn, pthread_getname_np) by moving them into #ifndef WIN32 or HAVE_PTHREAD_* guards. When pthreads is available on Windows (detected via cmake HAVE_PTHREAD_* checks), still use it for thread name reporting since it provides useful information at low cost. Also add Threads::Threads as an explicit dependency of the multiprocess library. proxy.cpp directly uses thread_local, std::this_thread, and std::thread, and the dependency was previously satisfied only through transitive linkage from CapnProto::kj-async. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Replace POSIX-only headers (sys/socket.h, sys/un.h, unistd.h) with Windows equivalents (afunix.h via util.h), guard them with #ifdef WIN32, use TCP sockets instead of Unix sockets for Wine compatibility, replace mkdtemp/unlink/rmdir with std::filesystem equivalents, and use SocketId/SocketError types instead of int/-1 for socket handles so the file compiles and works with MinGW. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
listen_tests.cpp tests fail on Windows with: "expected m_fd != SocketError [18446744073709551615 != 18446744073709551615]" socket(AF_INET, SOCK_STREAM, IPPROTO_TCP) returns INVALID_SOCKET because WSAStartup() has not been called. The mp library calls WSAStartup() only inside ConnectSocketToProcess(), which listen_tests.cpp never reaches — it creates sockets directly using the BSD API. Fix: add a static initializer that calls WSAStartup(MAKEWORD(2,2), ...) at program startup before any test runs. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- shell.nix: add `windows` parameter that selects pkgs.pkgsCross.mingwW64
as the cross target; also change crossPkgs default from import<nixpkgs>{}
to null (cleaner API). When windows=true, add native pkgs.capnproto to
nativeBuildInputs so capnp/capnpc-c++ are in PATH for cmake code generation,
and add wine64Packages.staging so ctest can run mptest.exe via wine.
Change llvmBase to always use pkgs (native) instead of crossPkgs.
- ci/configs/windows.bash: new config that cross-compiles with mingw,
sets CMAKE_SYSTEM_NAME=Windows, CMAKE_FIND_ROOT_PATH_MODE_PROGRAM=NEVER
(so cmake finds native capnp from PATH), CMAKE_CROSSCOMPILING_EMULATOR=wine
(so ctest runs mptest.exe via wine), and sets MPGEN_PRE_BUILD=1.
- ci/scripts/ci.sh: add MPGEN_PRE_BUILD support: when set, build native mpgen
in $CI_DIR-native before the main cross build, then inject
-DMPGEN_EXECUTABLE into CMAKE_ARGS. This is needed because cmake's
add_custom_command does not use CMAKE_CROSSCOMPILING_EMULATOR, so the
cross-compiled mpgen.exe cannot be used as a code generator directly.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…calls ConnectNamedPipe synchronously. If the child exits or crashes before connecting, ConnectNamedPipe blocks forever with no recovery path. Fix by opening the pipe with FILE_FLAG_OVERLAPPED and using WaitForMultipleObjects on both the connect event and the child process handle. If the process handle signals first, the child died without connecting and SpawnProcess throws instead of hanging. Since the pipe is now in overlapped mode, WriteFile also requires an OVERLAPPED structure; use GetOverlappedResult with bWait=TRUE to handle both synchronous and asynchronous completion. Add a Windows-only test that spawns a child which exits immediately without opening the named pipe and asserts SpawnProcess does not block. (bitcoin-core#231 (comment)) Co-Authored-By: ViniciusCestarii <viniciuscestari01@gmail.com> Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Updated b7e3676 -> be00288 (pr/win.14 -> pr/win.15, compare) with suggested changes. Also fixed pthreads include to avoid pthread_self compile error in mingw builds where pthreads is present, as described in #318 (comment)
Updated be00288 -> ad7562e (pr/win.15 -> pr/win.16, compare) to fix IWYU errors: add <unistd.h> to listen_tests.cpp, move <atomic> inside #ifdef WIN32 and add <functional> in spawn_tests.cpp https://github.com/bitcoin-core/libmultiprocess/actions/runs/30972705059/job/92200276090
Rebased ad7562e -> 19f7d31 (pr/win.16 -> pr/win.17, compare) to fix silent conflict with #310 https://github.com/bitcoin-core/libmultiprocess/actions/runs/30974304265/job/92205053288?pr=231
…calls ConnectNamedPipe synchronously. If the child exits or crashes before connecting, ConnectNamedPipe blocks forever with no recovery path. Fix by opening the pipe with FILE_FLAG_OVERLAPPED and using WaitForMultipleObjects on both the connect event and the child process handle. If the process handle signals first, the child died without connecting and SpawnProcess throws instead of hanging. Since the pipe is now in overlapped mode, WriteFile also requires an OVERLAPPED structure; use GetOverlappedResult with bWait=TRUE to handle both synchronous and asynchronous completion. Add a Windows-only test that spawns a child which exits immediately without opening the named pipe and asserts SpawnProcess does not block. (bitcoin-core#231 (comment)) Co-Authored-By: ViniciusCestarii <viniciuscestari01@gmail.com> Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
ad7562e to
19f7d31
Compare
6 participants
Add support for running on windows. These changes make the libmultiprocess API more generic, using stream types instead of file descriptors. All features are supported, including spawning processes with socket connections to the parent process. These changes were originally made in bitcoin/bitcoin#32387