Open
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline for information on the review process. 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-04-22 21:07:39 |
This was referenced Jan 8, 2026
pavlenex
added a commit
to pavlenex/stratumprotocol.org
that referenced
this pull request
Mar 24, 2026
|
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. |
18 tasks
Member
|
@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. |
Collaborator
Author
|
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 |
Add ProcessId = int type alias and apply it to WaitProcess, SpawnProcess (pid output argument), and callers.
Add SocketId = int and SocketError = -1 type aliases and apply SocketId to SpawnProcess (return type and callback parameter) and callers.
Add ConnectInfo type alias to pass socket handle from parent process to child process in more platform independent way.
Collaborator
Author
|
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 ( |
ryanofsky
force-pushed
the
pr/win
branch
2 times, most recently
from
e563c96 to
d9fcac6
Compare
This was referenced Apr 17, 2026
gen.cpp used fork() directly via <unistd.h> to invoke the capnp compiler as a subprocess, but fork() is not available on Windows, so shouldn't be used in application code. Add an ExecProcess(const std::vector<std::string>& args) function to util.h/util.cpp that spawns a process and returns its ProcessId, leaving the caller responsible for WaitProcess. On POSIX it uses fork() (via KJ_SYSCALL) + execvp; on Windows it can use CreateProcess. Update gen.cpp to replace the inline fork/exec/wait with mp::WaitProcess(mp::ExecProcess(args)). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Extract socket pair creation from SpawnProcess into a standalone SocketPair() function, and use it to replace the inline socketpair() call. No behavior change.
kj::AsyncIoStream::getFd() was added in capnproto 0.9 (commit d27bfb8a4175b32b783de68d93dd1dbafadddea5, first released in 0.9.0). The code now uses getFd() in proxy.cpp, so 0.7 is no longer a sufficient minimum. Set olddeps version to 0.9.2, which is the patched 0.9.x release for CVE-2022-46149. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…m objects Instead of accepting raw file descriptor integers and wrapping them internally, ConnectStream and ServeStream now accept kj::Own<kj::AsyncIoStream> directly. This removes the assumption that the transport is always a local unix fd, making the API easier to adapt to other I/O types (e.g. Windows handles). The Stream type alias (kj::Own<kj::AsyncIoStream>) is added as a convenience, along with StreamSocketId() to extract the underlying fd from a Stream when needed. Callers are updated to wrap their fd with wrapSocketFd() before calling.
Flush pending Cap'n Proto release messages before closing the stream. When one side of a socket pair closes, the other side does not receive an onDisconnect event, so it relies on receiving release messages from the closing side to free its ProxyServer objects and shut down cleanly. Without this, Server objects are not freed by Cap'n Proto on disconnection.
MSVC error when building multiprocess.vcxproj:
mp/util.h(146,46): error C2280:
'std::variant<T *,T>::variant(const std::variant<T *,T> &)':
attempting to reference a deleted function [with T=mp::Lock]
The PtrOrValue constructor used a ternary expression to initialize data:
data(ptr ? ptr : std::variant<T*, T>{std::in_place_type<T>, args...})
Both arms are prvalues of type std::variant<T*,T>, so under C++17's
mandatory copy elision no copy/move constructor should be invoked. GCC
and Clang apply this correctly. MSVC does not apply guaranteed copy
elision to ternary expressions in this context: it materializes the
temporary and then attempts to copy-construct data from it. Since
std::variant<Lock*,Lock> has a deleted copy constructor (Lock holds a
std::unique_lock which is move-only), MSVC fails.
Fix by initializing data to hold T*=ptr in the member initializer list,
then emplacing T in-place in the constructor body if ptr is null. This
avoids the ternary entirely and requires only the in-place constructor
of T, not any variant copy or move.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Member
|
How realistic is it to add a Windows CI job here? (can be cross-compiled) |
This was referenced Apr 20, 2026
Draft
Sjors
added a commit
to Sjors/sv2-tp
that referenced
this pull request
Apr 20, 2026
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
Sjors
added a commit
to Sjors/sv2-tp
that referenced
this pull request
Apr 20, 2026
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
added a commit
to Sjors/sv2-tp
that referenced
this pull request
Apr 20, 2026
The sv2 template_provider_tests pass on Windows; only TPTester teardown intermittently deadlocks in std::thread::join during libmultiprocess thread-local cleanup. This is a known libmultiprocess Windows issue, not specific to sv2-tp: bitcoin-core/libmultiprocess#231 bitcoin/bitcoin#32387 Instead of skipping the cases, exercise them normally and intentionally leak the TPTester so its destructor never runs. The OS reclaims the remaining loop thread and IPC state at process exit. A MAKE_TPTESTER helper macro keeps the tests source-compatible across platforms; on non-Windows it is just a stack-allocated TPTester as before. The lifecycle test in sv2_tester_lifecycle_tests is left skipped on Windows because its whole purpose is to exercise repeated construction and destruction of TPTester; leaking would defeat it. Assisted-by: GitHub Copilot Assisted-by: Anthropic Claude Opus 4
Collaborator
Author
I think this does need a windows CI job to exist in order to be merged, otherwise windows support is very likely to break with future changes, so thanks for opening #272. I think it may also make sense to split this PR up to separate the commits which are needed to support windows but don't actually add any windows code, from the one commit which actually does add windows code. |
Member
Yes it would be good to land those changes to keep this PR focussed. |
MSVC warns (C4305, treated as error) about truncation from 'int' to 'const bool' when initializing static const bool members from integer bitwise-and expressions. Use constexpr bool with explicit != 0 to make the boolean conversion unambiguous. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ldField on MSVC MSVC cannot parse 'typename decltype(expr)::Member' syntax and fails with a hard error (C2039, C2146) instead of a SFINAE substitution failure. Use Decay<> wrapper to provide the extra template indirection that MSVC needs, consistent with the unique_ptr and shared_ptr overloads of CustomBuildField which already use this pattern. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This repo has introduced API changes to add Windows support to libmultiprocess (HANDLE-based IPC alongside the existing fd-based IPC). These changes require corresponding updates to Bitcoin Core, which are pending in bitcoin/bitcoin#35084. Until that PR merges, the Bitcoin Core CI jobs fail against master because Bitcoin Core has not yet been updated to use the new API. Switch the Bitcoin Core checkout in both jobs to use refs/pull/35084/merge so CI tests against the compatible version. A BITCOIN_CORE_REF env var is introduced at the top of the file; once (and keep the var in place for any future API compatibility cycles). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ibraries
On macOS, when libcapnp is built as a dynamic library and Bitcoin Core
REDUCE_EXPORT option is used the RTTI typeinfo for kj::Exception has a
different address in libcapnp.dylib versus the calling binary. This
means catch (const kj::Exception& e) in the calling binary silently
fails to match exceptions thrown by capnp, so the DISCONNECTED exception
from shutdownWrite() propagates as a fatal uncaught exception instead of
being suppressed as intended.
This causes the Bitcoin Core macOS native CI job to fail with:
Fatal uncaught kj::Exception: kj/async-io-unix.c++:491: disconnected:
shutdown(fd, SHUT_WR): Socket is not connected
The fix is to use kj::runCatchingExceptions/kj::throwRecoverableException,
which use KJ's own thread-level exception interception mechanism rather
than C++ RTTI-based matching, and therefore work correctly across dynamic
library boundaries. This is the same approach used elsewhere in the
codebase (proxy.cpp EventLoop::post, type-context.h server request handler)
for the same reason.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
On MSVC, std::terminate() does not print the exception message before calling abort()/fastfail, so exceptions thrown during mpgen execution appear as a bare 0xC0000409 exit code with no diagnostic output. Wrap main() in a try-catch to explicitly print the error to stderr and return 1 instead of crashing. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Bitcoin Core linter rejects it: https://github.com/bitcoin/bitcoin/actions/runs/24568789956/job/71835997334?pr=32387
…bols Use target_compile_definitions on mpgen to expose CAPNP_EXECUTABLE, CAPNPC_CXX_EXECUTABLE (via $<TARGET_FILE:...> generator expressions on the CapnProto::capnp_tool and CapnProto::capnpc_cpp imported targets), and CAPNP_INCLUDE_DIRS (from the CAPNP_INCLUDE_DIRS variable set by find_package). gen.cpp uses these directly instead of constructing paths from capnp_PREFIX. Remove capnp_PREFIX from config.h.in as it is no longer needed there. Add compat fallbacks in compat_config.cmake to synthesize the tool imported targets and CAPNP_INCLUDE_DIRS from older variables when using an older CapnProto package. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Ubuntu Noble's libcapnp-dev 1.0.1 cmake config file is installed under /usr/lib/x86_64-linux-gnu/cmake/CapnProto/ but its _IMPORT_PREFIX calculation goes up only 3 directory levels to /usr/lib instead of 4 levels to /usr, so IMPORTED_LOCATION for CapnProto::capnp_tool is set to /usr/lib/bin/capnp (non-existent) rather than /usr/bin/capnp. The previous compat_config.cmake fallback only fired when the target didn't exist at all (NOT TARGET), so it didn't catch this case where the target exists but has a wrong path. Add a validation pass that iterates over both tool targets after they are created (either by the package or by our own fallback). For each target, check whether any IMPORTED_LOCATION (config-specific or generic) resolves to an existing file. If none do, use find_program (with capnp_PREFIX/bin as a hint) to locate the actual binary and override all stored locations on that target. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
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-io.h: Add Windows branch in StreamSocketId() using getWin32Handle(). - 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.
Remove POSIX and pthread calls from util.cpp to avoid relying on MinGW's POSIX compatibility layer. This lets code be compiled with MSVC.
Sjors
added a commit
to Sjors/sv2-tp
that referenced
this pull request
Apr 23, 2026
Sjors
added a commit
to Sjors/sv2-tp
that referenced
this pull request
Apr 23, 2026
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
Sjors
added a commit
to Sjors/sv2-tp
that referenced
this pull request
Apr 23, 2026
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
Sjors
added a commit
to Sjors/sv2-tp
that referenced
this pull request
Apr 23, 2026
Tearing down a TPTester (and with it the IPC EventLoop / per-thread state) intermittently deadlocks on Windows in std::thread::join during libmultiprocess thread-local cleanup. 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, paper over it in the test harness so the Windows CI job stays useful: - TPTesterHandle now heap-allocates and intentionally leaks the tester on Windows; the OS reclaims the remaining loop thread and IPC state at process exit. This only disables test cleanup, not the tests themselves. Non-Windows builds continue to own the tester by value. - sv2_tester_lifecycle_tests is gated off on _WIN32, since its whole purpose is to exercise repeated TPTester construction and destruction; leaking would defeat the test. - src/test/main.cpp installs a Windows-only Boost global fixture whose destructor runs at module teardown (after every test case has completed and Boost has tallied results). It flushes stdout/stderr and calls _exit() with 0 or 1 based on the Boost results, bypassing static destructors entirely so the leaked threads cannot fault and turn a green run into exit code 139. - lint-includes.py: allowlist boost/test/results_collector.hpp, required by the new fixture. References: bitcoin-core/libmultiprocess#231 bitcoin/bitcoin#32387 Assisted-by: GitHub Copilot Assisted-by: Anthropic Claude Opus 4 Assisted-by: Anthropic Claude Opus 4.7
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR is based on #274
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