This builds most of the CI jobs with multiprocess enabled.
Two jobs run the functional test suite, one with depends and one with the regular build.
The Windows job does not use multiprocess since that’s not supported yet.
The previous version of this PR changed the depends system to build libmultiprocess by default as one of the final steps for #31098 - but based discussion here and on IRC it was decided it’s too early to ship it.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/30975.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| Stale ACK | ryanofsky |
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:
CMAKE_CXX_FLAGS with APPEND_CXXFLAGS by hebasto)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.
174@@ -171,6 +175,7 @@ make -C depends --jobs="$JOBS" HOST="$HOST" \
175 ${SOURCES_PATH+SOURCES_PATH="$SOURCES_PATH"} \
176 ${BASE_CACHE+BASE_CACHE="$BASE_CACHE"} \
177 ${SDK_PATH+SDK_PATH="$SDK_PATH"} \
178+ ${MAYBE_MULTIPROCESS} \
8b2546c: it turns out that doing
make MULTIPROCESS=0independscauses libmultiprocess to be built anyway.
Yes, any non-empty value is considered “set/enabled”:https://github.com/bitcoin/bitcoin/blob/e13da501db9e123ec56adfcb6f01b811445f9ce7/depends/Makefile#L165
Not sure if that’s a known behavior or a cmake regression. cc @hebasto
Definitely, not the latter.
make MULTIPROCESS=0 behaviour above is intentional.
If multiprocess is (currently) unsupported on Windows, then that should be fixed in depends, i.e (for now) don’t even try building the packages/functionality for that platform, not leaked into Guix with a workaround.
More generally, if the project decides to start shipping multiprocess as part of it’s releases, then there’s a number of other things that need to happen first. We should be switching multiprocess from an opt-in in depends, to being built/used by default, as well as testing all of this, on all relevant platforms in our CIs, not just enabling it for some platforms as part of the release process.
There are also documentation/website/release process updates that will be needed, as this change is going to produce a release tarball, which will contain:
0bitcoin-cli
1bitcoin-gui
2bitcoin-node
3bitcoin-qt
4bitcoin-tx
5bitcoin-util
6bitcoin-wallet
7bitcoind
8test_bitcoin
with no explanation of why there is bitcoind & bitcoin-node, as well as bitcoin-qt & bitcoin-gui, what the difference is betweem them, or which one someone should use. I’d think that even if we started doing multiprocess, we should still only be shipping a bitcoind and bitcoin-qt (which contain the functionality, not multiple different binaries).
and as a result add
bitcoin-node,bitcoin-guiandbitcoin-walletto the release binaries.
bitcoin-wallet already exists in the release binaries:
0ls bitcoin-28.0rc2/bin
1bitcoin-cli
2bitcoin-qt
3bitcoin-tx
4bitcoin-util
5bitcoin-wallet
6bitcoind
7test_bitcoin
🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/30698173359
Make sure to run all tests locally, according to the documentation.
The failure may 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.
I would also prefer it if bitcoind (and bitcoin-qt) had multiprocess built in. That way the instruction for miners is “Install Bitcoin Core as you always do, just add ipcbind=unix to you config”. cc @ryanofsky
Another approach, if touching the bitcoind and bitcoin-qt binaries is something people are worried about doing too soon, is to move bitcoin-node and bitcoin-qt into an ipc folder. A document explaining all the different binaries would still be needed.
that should be fixed in depends, i.e (for now) don’t even try building the packages/functionality for that platform, not leaked into Guix with a workaround.
I’ll look into that.
there’s a number of other things that need to happen first. We should be switching multiprocess from an opt-in in depends, to being built/used by default, as well as testing all of this, on all relevant platforms in our CIs, not just enabling it for some platforms as part of the release process.
I’ll add some commits here to do these things. Those can be split into separate PRs later for a more incremental approach.
bitcoin-walletalready exists in the release binaries:
Ah, I either misremembered or things changed. But in #19460 it’s not / no longer the plan to have bitcoin-node spawn a bitcoin-wallet process. Updated the description.
I moved the Windows build skip workaround from Guix to depends. This made me excited about the prospect of moving that bit of Autotools stuff to CMake as well. :-)
Made some minimal documentation changes.
I didn’t change the non-depends build (yet).
All CI instances that use depends now use multiprocess, the others don’t (yet). Let’s see if that breaks anything…
From CI:
0SUMMARY: MemorySanitizer: use-of-uninitialized-value /usr/src/mp/proxy.cpp:146:25 in mp::Connection::addAsyncCleanup(std::__1::function<void ()>)
For my part, I need to fix the windows build and also open an issue listing different ways it would be possible to make releases including multiprocess support in short or long term.
Update: currently working on getting mingw32 build working, and I created issue #30983 to lay out different multiprocess release options.
0ci_container_base/src/ipc/capnp/mining.cpp -DARENA_DEBUG -DDEBUG_LOCKORDER -DDEBUG_LOCKCONTENTION -D_LIBCPP_REMOVE_TRANSITIVE_INCLUDES
1In file included from /ci_container_base/src/ipc/capnp/mining.cpp:5:
2In file included from /ci_container_base/src/ipc/capnp/mining-types.h:9:
3In file included from /ci_container_base/ci/scratch/build-x86_64-pc-linux-gnu/src/ipc/capnp/common.capnp.proxy-types.h:6:
4In file included from /ci_container_base/ci/scratch/build-x86_64-pc-linux-gnu/src/ipc/capnp/common.capnp.proxy.h:7:
5In file included from /ci_container_base/depends/x86_64-pc-linux-gnu/include/mp/proxy.h:8:
6/ci_container_base/depends/x86_64-pc-linux-gnu/include/mp/util.h:207:20: error: no template named 'function' in namespace 'std'; did you mean 'kj::Function'?
7 207 | using FdToArgsFn = std::function<std::vector<std::string>(int fd)>;
8 | ^~~~~
9/ci_container_base/depends/x86_64-pc-linux-gnu/include/kj/exception.h:34:29: note: 'kj::Function' declared here
10 34 | template <typename T> class Function;
11 | ^
121 error generated.
TSAN compile failure fixed in https://github.com/chaincodelabs/libmultiprocess/pull/113.
Rebased after #30043.
TSAN compile failure fixed in chaincodelabs/libmultiprocess#113.
Added a commit to bump libmultiprocess to include this (can be its own PR later).
MSAN says https://cirrus-ci.com/task/6248232848719872:
0SUMMARY: MemorySanitizer: use-of-uninitialized-value /usr/src/mp/proxy.cpp:146:25 in mp::Connection::addAsyncCleanup(std::__1::function<void ()>)
1[10:00:27.923] Exiting
Thanks! I created https://github.com/chaincodelabs/libmultiprocess/issues/115 with details about this, and I think I have a potential fix.
61@@ -62,7 +62,7 @@ void IpcPipeTest()
62
63 auto connection_client = std::make_unique<mp::Connection>(loop, kj::mv(pipe.ends[0]));
64 auto foo_client = std::make_unique<mp::ProxyClient<gen::FooInterface>>(
65- connection_client->m_rpc_system.bootstrap(mp::ServerVatId().vat_id).castAs<gen::FooInterface>(),
66+ connection_client->m_rpc_system->bootstrap(mp::ServerVatId().vat_id).castAs<gen::FooInterface>(),
Added a commit to enable multiprocess on more CI environments.
Tidy fails with: https://cirrus-ci.com/task/4938161187454976
0[12:32:04.351] CMake Error at src/ipc/CMakeLists.txt:12 (target_capnp_sources):
1[12:32:04.351] Unknown CMake command "target_capnp_sources".
That makes sense; I’ll have to actually install capnp and multiprocess on the non-depends CI jobs.
🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/31118275242
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.
_multiprocess suffix could be dropped from the i686 job, however, we should have atleast one job building with NO_MULTIPROCESS, so maybe that could be reused for that with _no_multiprocess.
It looks like the no multiprocess, i686, DEBUG functional tests are still trying to check multiprocess stuff despite NO_MULTIPROCESS=1.
FileNotFoundError: [Errno 2] No such file or directory: ‘bitcoin-node’
https://cirrus-ci.com/task/6626990688567296
I forgot to adjust export BITCOIND=bitcoin-node. Do we want to use that for all builds, or just a few?
export BITCOIND=bitcoin-node just for the ARM job for now.
🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/32995736139
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.
I disabled ARM and macOS-cross as well, added a link to their failures to PR description TODO. These failures are all over my head, so will wait for someone else to figure them out…
I think the p2p_i2p_ports.py failure in the previous releases job is spurious, so leaving that job in.
TODO:
- fix
arm-linux-gnueabihf-g++: error: unrecognized command-line option ‘-mpclmul’failure and re-enable multiprocess for arm job
I don’t think the problem description is correct.
In my opinion, the root of the issue is:
0$ make --no-print-directory -C depends HOST=arm-linux-gnueabihf print-multiprocess_packages
1multiprocess_packages=
multiprocess_packages= seems to be empty for all hosts. It turns out the first commit isn’t actually working, as I can confirm locally.
Given that the arm job uses depends, it was incorrect to set BITCOIN_CONFIG=-DWITH_MULTIPROCESS=ON. It should simply follow what depends sets. I’ll fix that in the next push.
I dropped the arm todo.
156@@ -157,21 +157,19 @@ sqlite_packages_$(NO_SQLITE) = $(sqlite_packages)
157 wallet_packages_$(NO_WALLET) = $(bdb_packages_) $(sqlite_packages_)
158
159 zmq_packages_$(NO_ZMQ) = $(zmq_packages)
160-multiprocess_packages_$(MULTIPROCESS) = $(multiprocess_packages)
161+
162+multiprocess_packages_$(NO_MULTIPROCESS) = $(multiprocess_packages)
$(host_os)_
I wrote:
I set export
BITCOIND=bitcoin-nodejust for the ARM job for now.
I think it should be enough to do this for one depends job and one non-depends job. I’ll adjust that later.
Update 2024-11-22: I picked macOS native
16@@ -17,4 +17,8 @@ export XCODE_BUILD_ID=15A240d
17 export RUN_UNIT_TESTS=false
18 export RUN_FUNCTIONAL_TESTS=false
19 export GOAL="deploy"
20-export BITCOIN_CONFIG="-DBUILD_GUI=ON -DREDUCE_EXPORTS=ON"
21+export BITCOIN_CONFIG="\
22+-DWITH_MULTIPROCESS=OFF \
TODO:
- fix
unknown target CPU 'armv8-a+crc+crypto'failure and re-enable macOS cross
Similar to #30975 (comment), I do think that -DWITH_MULTIPROCESS=OFF can be dropped now as depends are fixed.
I re-enabled multiprocess for two more depends build machines: macOS cross and centos. Let’s see how those go.
I also enabled it for the native fuzz with valgrind job and the native valgrind job, let’s see how that goes.
For all non-depends builds I now explicitly set -DWITH_MULTIPROCESS to either ON or OFF for clarity.
I also moved a few things that were in the wrong commit.
Added a commit to enable the native_asan and native_valgrind jobs, to get a more update to date log.
CI / ASan + LSan + UBsan log: https://github.com/bitcoin/bitcoin/actions/runs/11970857809/job/33374462331?pr=30975
Trivial rebase after #31399.
I disabled ASan + LSan + UBsan again, see previous failure log: https://github.com/bitcoin/bitcoin/actions/runs/11970857809/job/33374462331?pr=30975
(this drops fda0d98237b4a82a94a5672f6bd263bca04adf35 and adjusts 0b5c84bc988e600484012f43df0e9950e7c444f5 to enable multiprocess for ci/test/00_setup_env_native_valgrind.sh)
I disabled ASan + LSan + UBsan again, see previous failure log: https://github.com/bitcoin/bitcoin/actions/runs/11970857809/job/33374462331?pr=30975
I think https://github.com/chaincodelabs/libmultiprocess/pull/121 should fix this, but I want to test the fix a little more and it will require another depends bump like 90b405516f7f3be522ced3e0c4d23b3892df0661 to fix the problem in CI.
If there are more bugs feel free to post to https://github.com/chaincodelabs/libmultiprocess/issues/new or just ping me.
🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/34105951022
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.
Rebased, opened chaincodelabs/libmultiprocess#124
Fixed in #31480.
Rebased after #31480, hopefully Tidy succeeds! (narrator: it did)
Updated PR description to track the two remaining CI crash.
I’ll track https://github.com/chaincodelabs/libmultiprocess/issues/122 separately in #31098.
Rebased to see if nothing new breaks. Clarified the PR description to indicate Windows support (probably) won’t make it into this PR.
As described in the TODO, before marking this as ready review, there’s one UndefinedBehaviorSanitizer issue left to resolve upstream in libmultiprocess. I might also wait for #31375 and #31161 to land first (added to description).
Pushed a commit to test https://github.com/chaincodelabs/libmultiprocess/pull/121
@ryanofsky getting another(?) UndefinedBehaviorSanitizer: dynamic-type-mismatch
https://github.com/bitcoin/bitcoin/actions/runs/12714674558/job/35445294089?pr=30975#step:7:5107
Oh no, this job doesn’t use depends.
Still fails:
0git clone https://github.com/ryanofsky/libmultiprocess.git
1Cloning into 'libmultiprocess'...
2cd libmultiprocess
3git checkout pr/ucon
4...
5SUMMARY: UndefinedBehaviorSanitizer: dynamic-type-mismatch /usr/local/include/mp/proxy.h:95:45
https://github.com/bitcoin/bitcoin/actions/runs/12715313812/job/35447236983?pr=30975#step:7:5113
It seems to be for different reasons, see https://github.com/chaincodelabs/libmultiprocess/issues/125
Update 2025-14-01: will wait for a fix for this new issue, and then replace the last commit to test it.
Rebased and trying https://github.com/chaincodelabs/libmultiprocess/pull/127.
Update: works!
The first commit now bumps the multiprocess dependency. @ryanofsky feel free to extract into its own PR if you want to land it faster.
I don’t think it’s necessary to wait for #31375 and #31161 so I’ve marked this ready for review. But if folks disagree, I can make it draft again.
21-multiprocess_native_packages = native_libmultiprocess native_capnp
22+multiprocess_linux_packages = libmultiprocess capnp
23+multiprocess_darwin_packages = libmultiprocess capnp
24+
25+multiprocess_native_linux_packages = native_libmultiprocess native_capnp
26+multiprocess_native_darwin_packages = native_libmultiprocess native_capnp
In commit “build: depends makes libmultiprocess by default” (a77a88a7583e7f07626b55347cb6d3f31be8bffb)
It seems like this change is forcing multiprocess to be disabled for windows (ignoring the NO_MULTIPROCESS setting) instead of just defaulting it to off for windows. If that is really the intention here, I think it would be good to have a comment pointing that out, otherwise it’s not clear why this code is explicitly mentioning linux and darwin.
I also wonder if a better alternative instead of changing package definitions and ignoring the NO_MULTIPROCESS setting on certain platforms is just to change the default NO_MULTIPROCESS value depending on the platform. Like maybe:
0# Default NO_MULTIPROCESS value is 1 unless host platform is linux or darnwin due to lack of support on other platforms currently
1NO_MULTIPROCESS ?= $(if $(findstring linux,$(HOST))$(findstring darwin,$(HOST)),,1)
or
0# Default NO_MULTIPROCESS value is 1 if host platform is windows because it doesn't currently build on windows
1NO_MULTIPROCESS ?= $(if $(findstring mingw32,$(HOST)),1,)
freebsd.
21-multiprocess_darwin_packages = libmultiprocess capnp
22-
23-multiprocess_native_linux_packages = native_libmultiprocess native_capnp
24-multiprocess_native_darwin_packages = native_libmultiprocess native_capnp
25+multiprocess_packages = libmultiprocess capnp
26+multiprocess_linux_packages = native_libmultiprocess native_capnp
7ab858765d90470f941851d8cf55d616014ee2df:
Why are native packages being removed?
🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/35764046539
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.
https://github.com/bitcoin/bitcoin/actions/runs/12826202676/job/35765674846?pr=30975#step:7:3078:
0 node0 2025-01-17T09:33:34.664692Z [capnp-loop] [ipc/capnp/protocol.cpp:35] [IpcLogFn] [ipc] {bitcoin-node-25673/b-capnp-loop-71206} EventLoop::loop done, cancelling event listeners.
1 node0 2025-01-17T09:33:34.664706Z [capnp-loop] [ipc/capnp/protocol.cpp:35] [IpcLogFn] [ipc] {bitcoin-node-25673/b-capnp-loop-71206} EventLoop::loop bye.
2 test 2025-01-17T09:33:34.712000Z TestFramework (ERROR): Assertion failed
3 Traceback (most recent call last):
4 File "/Users/runner/work/bitcoin/bitcoin/test/functional/test_framework/test_framework.py", line 135, in main
5 self.run_test()
6 ~~~~~~~~~~~~~^^
7 File "/Users/runner/work/bitcoin/bitcoin/ci/scratch/build-aarch64-apple-darwin23.6.0/test/functional/rpc_misc.py", line 83, in run_test
8 self.restart_node(0, ["-txindex", "-blockfilterindex", "-coinstatsindex"])
9 ~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
10 File "/Users/runner/work/bitcoin/bitcoin/test/functional/test_framework/test_framework.py", line 617, in restart_node
11 self.stop_node(i)
12 ~~~~~~~~~~~~~~^^^
13 File "/Users/runner/work/bitcoin/bitcoin/test/functional/test_framework/test_framework.py", line 603, in stop_node
14 self.nodes[i].stop_node(expected_stderr, wait=wait)
15 ~~~~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^
16 File "/Users/runner/work/bitcoin/bitcoin/test/functional/test_framework/test_node.py", line 409, in stop_node
17 self.wait_until_stopped(expected_stderr=expected_stderr)
18 ~~~~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
19 File "/Users/runner/work/bitcoin/bitcoin/test/functional/test_framework/test_node.py", line 444, in wait_until_stopped
20 self.wait_until(lambda: self.is_node_stopped(**kwargs), timeout=timeout)
21 ~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
22 File "/Users/runner/work/bitcoin/bitcoin/test/functional/test_framework/test_node.py", line 840, in wait_until
23 return wait_until_helper_internal(test_function, timeout=timeout, timeout_factor=self.timeout_factor, check_interval=check_interval)
24 File "/Users/runner/work/bitcoin/bitcoin/test/functional/test_framework/util.py", line 310, in wait_until_helper_internal
25 if predicate():
26 ~~~~~~~~~^^
27 File "/Users/runner/work/bitcoin/bitcoin/test/functional/test_framework/test_node.py", line 444, in <lambda>
28 self.wait_until(lambda: self.is_node_stopped(**kwargs), timeout=timeout)
29 ~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^
30 File "/Users/runner/work/bitcoin/bitcoin/test/functional/test_framework/test_node.py", line 423, in is_node_stopped
31 assert return_code == expected_ret_code, self._node_msg(
32 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
33 AssertionError: [node 0] Node returned unexpected exit code (-6) vs (0) when stopping
34 test 2025-01-17T09:33:34.716000Z TestFramework (DEBUG): Closing down network thread
35 test 2025-01-17T09:33:34.792000Z TestFramework (INFO): Stopping nodes
36 test 2025-01-17T09:33:34.792000Z TestFramework.node0 (DEBUG): Stopping node
37
38 node0 stderr libc++abi: terminating due to uncaught exception of type std::__1::system_error: mutex lock failed: Invalid argument
@fanquake I’m unable to reproduce that locally. Filed an issue on the multiprocess repo to track: https://github.com/chaincodelabs/libmultiprocess/issues/128
Rebased after #31675 and fix the confusion between freebsd and openbsd.
40@@ -41,7 +41,10 @@ NO_SQLITE ?=
41 NO_WALLET ?=
42 NO_ZMQ ?=
43 NO_USDT ?=
44-MULTIPROCESS ?=
45+# Default NO_MULTIPROCESS value is 1 on unsupported host platforms:
46+# - Windows
47+# - OpenBSB: https://github.com/capnproto/capnproto/pull/1907
In commit “build: depends makes libmultiprocess by default” (48268facc9bb4f3e4f98cff5c21e1fe490b232e2)
OpenBSB typo
159@@ -157,21 +160,19 @@ sqlite_packages_$(NO_SQLITE) = $(sqlite_packages)
160 wallet_packages_$(NO_WALLET) = $(bdb_packages_) $(sqlite_packages_)
161
162 zmq_packages_$(NO_ZMQ) = $(zmq_packages)
163-multiprocess_packages_$(MULTIPROCESS) = $(multiprocess_packages)
164+
165+multiprocess_packages_$(NO_MULTIPROCESS) = $(multiprocess_$(host_os)_packages)
166+multiprocess_native_packages_$(NO_MULTIPROCESS) = $(multiprocess_native_$(host_os)_packages)
In commit “build: depends makes libmultiprocess by default” (48268facc9bb4f3e4f98cff5c21e1fe490b232e2)
It doesn’t seem right to include host_os in these variables anymore. I think this is is causing multiprocess packages not to be built right now. If I run make print-multiprocess_packages_ the variable is empty and the toolchain file does not seem to have multiprocess enabled. But if I drop host_os these things are fixed:
0-multiprocess_packages_$(NO_MULTIPROCESS) = $(multiprocess_$(host_os)_packages)
1-multiprocess_native_packages_$(NO_MULTIPROCESS) = $(multiprocess_native_$(host_os)_packages)
2+multiprocess_packages_$(NO_MULTIPROCESS) = $(multiprocess_packages)
3+multiprocess_native_packages_$(NO_MULTIPROCESS) = $(multiprocess_native_packages)
Separately I also think the multiprocess_native_packages_ assignment above is unnecessary and can be dropped with other simplifications suggested below.
168 usdt_packages_$(NO_USDT) = $(usdt_$(host_os)_packages)
169
170-packages += $($(host_arch)_$(host_os)_packages) $($(host_os)_packages) $(boost_packages_) $(libevent_packages_) $(qt_packages_) $(wallet_packages_) $(usdt_packages_)
171-native_packages += $($(host_arch)_$(host_os)_native_packages) $($(host_os)_native_packages)
172+packages += $($(host_arch)_$(host_os)_packages) $($(host_os)_packages) $(boost_packages_) $(libevent_packages_) $(qt_packages_) $(wallet_packages_) $(usdt_packages_) $(multiprocess_packages_)
173+native_packages += $($(host_arch)_$(host_os)_native_packages) $($(host_os)_native_packages) $(multiprocess_native_packages_)
In commit “build: depends makes libmultiprocess by default” (48268facc9bb4f3e4f98cff5c21e1fe490b232e2)
Would suggest reverting this change to simplify. Requires also reverting the ifeq ($(multiprocess_packages_),) change below.
I’m confused. What should depends/Makefile look like?
I’ll push a new version with just the above host bugfix for now.
I’m confused. What should depends/Makefile look like?
I think the only changes that need to be made to this file in this PR are:
0--- a/depends/Makefile
1+++ b/depends/Makefile
2@@ -41,7 +41,10 @@ NO_SQLITE ?=
3 NO_WALLET ?=
4 NO_ZMQ ?=
5 NO_USDT ?=
6-MULTIPROCESS ?=
7+# Default NO_MULTIPROCESS value is 1 on unsupported host platforms:
8+# - Windows
9+# - OpenBSB: https://github.com/capnproto/capnproto/pull/1907
10+NO_MULTIPROCESS ?= $(if $(findstring mingw32,$(HOST))$(findstring openbsd,$(HOST)),1,)
11 LTO ?=
12 NO_HARDEN ?=
13 FALLBACK_DOWNLOAD_PATH ?= https://bitcoincore.org/depends-sources
14@@ -157,7 +160,7 @@ sqlite_packages_$(NO_SQLITE) = $(sqlite_packages)
15 wallet_packages_$(NO_WALLET) = $(bdb_packages_) $(sqlite_packages_)
16
17 zmq_packages_$(NO_ZMQ) = $(zmq_packages)
18-multiprocess_packages_$(MULTIPROCESS) = $(multiprocess_packages)
19+multiprocess_packages_$(NO_MULTIPROCESS) = $(multiprocess_packages)
20 usdt_packages_$(NO_USDT) = $(usdt_$(host_os)_packages)
21
22 packages += $($(host_arch)_$(host_os)_packages) $($(host_os)_packages) $(boost_packages_) $(libevent_packages_) $(qt_packages_) $(wallet_packages_) $(usdt_packages_)
23@@ -167,7 +170,7 @@ ifneq ($(zmq_packages_),)
24 packages += $(zmq_packages)
25 endif
26
27-ifeq ($(multiprocess_packages_),)
28+ifneq ($(multiprocess_packages_),)
29 packages += $(multiprocess_packages)
30 native_packages += $(multiprocess_native_packages)
31 endif
32@@ -230,7 +233,7 @@ $(host_prefix)/toolchain.cmake : toolchain.cmake.in $(host_prefix)/.stamp_$(fina
33 -e 's|@sqlite_packages@|$(sqlite_packages_)|' \
34 -e 's|@usdt_packages@|$(usdt_packages_)|' \
35 -e 's|@no_harden@|$(NO_HARDEN)|' \
36- -e 's|@multiprocess@|$(MULTIPROCESS)|' \
37+ -e 's|@multiprocess_packages@|$(multiprocess_packages_)|' \
38 $< > $@
39 touch $@
40
All the other changes just add noise and make things more complicated.
EDIT: Forgot a change above
178
179-ifeq ($(multiprocess_packages_),)
180-packages += $(multiprocess_packages)
181-native_packages += $(multiprocess_native_packages)
182-endif
183-
In commit “build: depends makes libmultiprocess by default” (https://github.com/bitcoin/bitcoin/commit/48268facc9bb4f3e4f98cff5c21e1fe490b232e2)
Would suggest reverting this change to simplify the PR.
17@@ -18,3 +18,4 @@ export GOAL="install"
18 # -Wno-psabi is to disable ABI warnings: "note: parameter passing for argument of type ... changed in GCC 7.1"
19 # This could be removed once the ABI change warning does not show up by default
20 export BITCOIN_CONFIG="-DREDUCE_EXPORTS=ON -DCMAKE_CXX_FLAGS='-Wno-psabi -Wno-error=maybe-uninitialized'"
21+export BITCOIND=bitcoin-node # Used in functional tests
In commit “build: depends makes libmultiprocess by default” (48268facc9bb4f3e4f98cff5c21e1fe490b232e2)
This isn’t actually doing anything because RUN_FUNCTIONAL_TESTS is false above
00_setup_env_arm.sh)
Code review 9a90f964a3c4fe75477926d34b7f1813b144449d.
It looks like latest depends changes are not actually enabling multiprocess binaries anymore due to a referencing a bad $(multiprocess_$(host_os)_packages) variable that should no longer include $(host_os). Also it is concerning that there doesn’t seem to be any test coverage for this because in current version of this multiprocess binaries are only used in functional tests in a non-depends build. There’s no depends build using multiprocess binaries in functional tests.
Also suggested more simplifications to the depends Makefile that should make this easier to understand and review.
There’s no depends build using multiprocess binaries in functional tests.
I added BITCOIND=bitcoin-node to “CentOS, dash, gui”. So after the host bug fix (https://github.com/bitcoin/bitcoin/pull/30975#discussion_r1920342619), then it should use depends, should include multiprocess and run the functional tests.
bitcoin-node.
I pushed an additional commit which adds bitcoin-node and bitcoin-gui to Maintenance.cmake. Before this PR they were already part of installable_targets, conditional on being built. So it makes sense to run the binary security, symbol and dynamic library checks on them.
I clarified the PR description on this topic.
If people prefer to defer including these binaries, e.g. for discussion in another PR, I could also drop this commit and instead remove them from installable_targets. IIUC that still allows us to use those binaries in CI for functional tests.
0arm64
1e95710441e4f8eef100b22f776556bf30e31456083c1344a1c05af6430354755 guix-build-ba454202d9d9/output/aarch64-linux-gnu/SHA256SUMS.part
272601fc3ef0b732c97b0e958666308427963f45eabda241c857d08a0e968b204 guix-build-ba454202d9d9/output/aarch64-linux-gnu/bitcoin-ba454202d9d9-aarch64-linux-gnu-debug.tar.gz
302e150ab991818959ab96d114b9ef1379cf9564539dc8bdb16b349856f71c3e6 guix-build-ba454202d9d9/output/aarch64-linux-gnu/bitcoin-ba454202d9d9-aarch64-linux-gnu.tar.gz
4b6e846f36fe3a18f82ba7f0fd7576eaadcf04df4836964ab908a9ae49c4d835f guix-build-ba454202d9d9/output/arm-linux-gnueabihf/SHA256SUMS.part
52b698f74bb116d1a67eb6d78f79e17ced03a19a60430447da23443506e76c908 guix-build-ba454202d9d9/output/arm-linux-gnueabihf/bitcoin-ba454202d9d9-arm-linux-gnueabihf-debug.tar.gz
67550a61a6de85a65f4ef16b2b1f4fb676ea1b6447b11b1dc96cf4feda6d11d78 guix-build-ba454202d9d9/output/arm-linux-gnueabihf/bitcoin-ba454202d9d9-arm-linux-gnueabihf.tar.gz
7c947153396b2ce2c491afbb4249773786d8063f71cba7dbda8e422a576908dc5 guix-build-ba454202d9d9/output/arm64-apple-darwin/SHA256SUMS.part
8f99a634754a12d526b8c8a9c31da332baf42c8f1bbe26f924b7975abe9e3c809 guix-build-ba454202d9d9/output/arm64-apple-darwin/bitcoin-ba454202d9d9-arm64-apple-darwin-unsigned.tar.gz
95d0a6c2e838b9de3d2e9d709070abc6ed7c7e01b099695d420fa82058d98c55a guix-build-ba454202d9d9/output/arm64-apple-darwin/bitcoin-ba454202d9d9-arm64-apple-darwin-unsigned.zip
10d25a91a58e0df323cb2dab0b1b4a45a9012a2b7a12801e604610d6e77409edee guix-build-ba454202d9d9/output/arm64-apple-darwin/bitcoin-ba454202d9d9-arm64-apple-darwin.tar.gz
119a9886b36aabf1d3b15714f405f6ab9c041128dd7f4c75b752e7c1466f18b801 guix-build-ba454202d9d9/output/dist-archive/bitcoin-ba454202d9d9.tar.gz
128cbe8cf0739228e2a438f71ef226c6ba6514da120a23c439e41d937a16cf8424 guix-build-ba454202d9d9/output/powerpc64-linux-gnu/SHA256SUMS.part
13a645877f0015af46f0a79c23f39278280240c5a2770bf0a5dbdd310ef9fc39c1 guix-build-ba454202d9d9/output/powerpc64-linux-gnu/bitcoin-ba454202d9d9-powerpc64-linux-gnu-debug.tar.gz
145f937a1ad002cd741169fd426485cb94ce39210c0305fe7fdf89fd9ed917bbd1 guix-build-ba454202d9d9/output/powerpc64-linux-gnu/bitcoin-ba454202d9d9-powerpc64-linux-gnu.tar.gz
150c1b8759cc95cce471355e7cdead2c0a0175d619ce8b4c2b30bf8d5a4528cf02 guix-build-ba454202d9d9/output/riscv64-linux-gnu/SHA256SUMS.part
1649da925b050bcd681b97740ee5939312e7dc226aaecfeb6fef2cea10b3b5db24 guix-build-ba454202d9d9/output/riscv64-linux-gnu/bitcoin-ba454202d9d9-riscv64-linux-gnu-debug.tar.gz
177c35536663565a56686eaf7da4cb0a124806155104c6e2543ee5d66a3b352e3f guix-build-ba454202d9d9/output/riscv64-linux-gnu/bitcoin-ba454202d9d9-riscv64-linux-gnu.tar.gz
183f14d420fb059e8a9f37780ccaebc284fa5a4cb14b02cf97a9e8aece9e3d08da guix-build-ba454202d9d9/output/x86_64-apple-darwin/SHA256SUMS.part
192f41d12d8fd777b5fe2a4924848f31ae2ecc6bb94b4d506941f5a5ff7f3e8fe6 guix-build-ba454202d9d9/output/x86_64-apple-darwin/bitcoin-ba454202d9d9-x86_64-apple-darwin-unsigned.tar.gz
2009f091bdfb043ff495b1dc4fb2777d08da3511303e7dacf2cf9e97c372a39f14 guix-build-ba454202d9d9/output/x86_64-apple-darwin/bitcoin-ba454202d9d9-x86_64-apple-darwin-unsigned.zip
2140aa2b846bf08d670096bd5b1be5e12bea29f6bcae3b49413acde525bfc14da4 guix-build-ba454202d9d9/output/x86_64-apple-darwin/bitcoin-ba454202d9d9-x86_64-apple-darwin.tar.gz
22ade2e5dcc2154d3485e21592e5d67e2658827fa32782c8222dc0d7f9495a0679 guix-build-ba454202d9d9/output/x86_64-linux-gnu/SHA256SUMS.part
2308d687298736fb1899b8f0b9aadbd9fd4bb1c4e318331d2fc5d3a322650dcd74 guix-build-ba454202d9d9/output/x86_64-linux-gnu/bitcoin-ba454202d9d9-x86_64-linux-gnu-debug.tar.gz
24171795917c798510a12b73f406099503153125246425290a4578822dda9dd7f4 guix-build-ba454202d9d9/output/x86_64-linux-gnu/bitcoin-ba454202d9d9-x86_64-linux-gnu.tar.gz
25136da1d75dc0218f80a6359e111d84c997dc0ad29202c122758aa67920f64f6c guix-build-ba454202d9d9/output/x86_64-w64-mingw32/SHA256SUMS.part
26a7c990deeb03ac1f30ba418573795ab305bb3499018d5fbe37ad9dd463c4749c guix-build-ba454202d9d9/output/x86_64-w64-mingw32/bitcoin-ba454202d9d9-win64-debug.zip
27a42f81b5df2300f2af01505b8838ae1632c3a68fd4d1e30426a279acac62afac guix-build-ba454202d9d9/output/x86_64-w64-mingw32/bitcoin-ba454202d9d9-win64-setup-unsigned.exe
2873cf8353f7839c300ca11b2b17bc0029fe502e2e55fe3dd80c492e173cb829b6 guix-build-ba454202d9d9/output/x86_64-w64-mingw32/bitcoin-ba454202d9d9-win64-unsigned.tar.gz
295cfb28b72c3e17a7df2db4ac1e7823307e108a72ea20b65bf156300b6d00621c guix-build-ba454202d9d9/output/x86_64-w64-mingw32/bitcoin-ba454202d9d9-win64.zip
CI fails with
0[19:02:40.881] node0 stderr terminate called after throwing an instance of 'kj::ExceptionImpl'
1[19:02:40.881] what(): mp/proxy.cpp:242: disconnected: write(m_post_fd, &buffer, 1): Broken pipe
2[19:02:40.881] stack: 5620c5490f21 5620c549127d 7fee54edc063 7fee54bcede8 7fee54c3f8fb `
Another CI failure on rpc_misc.py this time on centOS. I’m unsure if it’s the same issue:
https://cirrus-ci.com/task/6027377219731456?logs=ci#L4105
0raise AssertionError("Unexpected stderr {} != {}".format(stderr, expected_stderr))
1AssertionError: Unexpected stderr terminate called after throwing an instance of 'kj::ExceptionImpl'
2what(): mp/proxy.cpp:242: disconnected: write(m_post_fd, &buffer, 1): Broken pipe
Pushing again with only the last commit date changed to see if it’s consistent. But it needs a fix in any case.
Code review ACK 4b5b8d4f63a9f9154ac8e37861c2b3781bb7fd2d. Since last review, this was rebased and a new commit was added to run symbol-check and security-check on bitcoin-node and bitcoin-gui binaries.
I’m not sure how frequently rpc_misc.py CI errors that are reported https://github.com/chaincodelabs/libmultiprocess/issues/128 happen, but if I understand correctly if this PR is merged we would expect these errors to happen more because more multiprocess tests will be run more? This might be a reason to delay merging this PR.
104@@ -105,3 +105,14 @@ if [ -n "$XCODE_VERSION" ] && [ ! -d "${DEPENDS_DIR}/SDKs/${OSX_SDK_BASENAME}" ]
105 fi
106
107 git config --global ${CFG_DONE} "true"
108+
109+# Install libmultiprocess
git config --global ${CFG_DONE} "true" does? I’ll move it up.
71@@ -72,7 +72,7 @@ jobs:
72 run: |
73 # Run tests on commits after the last merge commit and before the PR head commit
74 # Use clang++, because it is a bit faster and uses less memory than g++
75- git rebase --exec "echo Running test-one-commit on \$( git log -1 ) && CC=clang CXX=clang++ cmake -B build -DWERROR=ON -DWITH_ZMQ=ON -DBUILD_GUI=ON -DBUILD_BENCH=ON -DBUILD_FUZZ_BINARY=ON -DWITH_BDB=ON -DWITH_USDT=ON -DCMAKE_CXX_FLAGS='-Wno-error=unused-member-function' && cmake --build build -j $(nproc) && ctest --output-on-failure --stop-on-failure --test-dir build -j $(nproc) && ./build/test/functional/test_runner.py -j $(( $(nproc) * 2 )) --combinedlogslen=99999999" ${{ env.TEST_BASE }}
76+ git rebase --exec "echo Running test-one-commit on \$( git log -1 ) && CC=clang CXX=clang++ cmake -B build -DWERROR=ON -DWITH_MULTIPROCESS=OFF -DWITH_ZMQ=ON -DBUILD_GUI=ON -DBUILD_BENCH=ON -DBUILD_FUZZ_BINARY=ON -DWITH_BDB=ON -DWITH_USDT=ON -DCMAKE_CXX_FLAGS='-Wno-error=unused-member-function' && cmake --build build -j $(nproc) && ctest --output-on-failure --stop-on-failure --test-dir build -j $(nproc) && ./build/test/functional/test_runner.py -j $(( $(nproc) * 2 )) --combinedlogslen=99999999" ${{ env.TEST_BASE }}
01_base_install.sh. But agree it would be nicer to include multiprocess, so might be worth refactoring the scripts.
But agree it would be nicer to include multiprocess, so might be worth refactoring the scripts.
Not sure why this needs a refactor. Just don’t turn it off in this line and install the dep?
Given that merging this PR for 29.x is going to be discussed further, I wanted to leave a few general thoughts here.
Looking at #31098, there are still un-merged changes in the “To complete the interface list” (also unclear if there are more PRs missing from that list). It seems premature to do this if the thing that is meant to be utilising it isn’t even finished.
Big new features / changes are done early after a branch off, so they have plenty of time to sit, be tested, devs can adjust etc. If this is merged now, it’s close to the last minute, with basically no real testing from inside the project.
I don’t think we should be starting to ship / turning something on by default, when there are multiple known intermittent test failures, or other outstanding issues. i.e
Some of these are months old, without a fix. That’s just opting the project into random CI failures.
Basic usage of building libmultiprocess yourself, on the current Ubuntu LTS release (24.04), for use with Core, doesn’t currently work:
0<libmultiprocess compiled & installed following instructions>
1cmake -B build -DWITH_MULTIPROCESS=ON
2-- The CXX compiler identification is GNU 13.3.0
3-- Detecting CXX compiler ABI info
4-- Detecting CXX compiler ABI info - done
5-- Check for working CXX compiler: /usr/bin/c++ - skipped
6-- Detecting CXX compile features
7-- Detecting CXX compile features - done
8-- Found SQLite3: /usr/include (found suitable version "3.45.1", minimum required is "3.7.17")
9-- Looking for __atomic_load_8 in atomic
10-- Looking for __atomic_load_8 in atomic - not found
11CMake Error at /usr/lib/aarch64-linux-gnu/cmake/CapnProto/CapnProtoConfig.cmake:108 (message):
12 libatomic not found
13Call Stack (most recent call first):
14 /usr/share/cmake-3.28/Modules/CMakeFindDependencyMacro.cmake:76 (find_package)
15 /usr/local/lib/cmake/Libmultiprocess/LibmultiprocessConfig.cmake:38 (find_dependency)
16 CMakeLists.txt:146 (find_package)
17
18-- Configuring incomplete, errors occurred!
Having to work around this was also mentioned here: #10102 (comment). We can likely work around this ourselves, but something this basic should be fixed before requiring developers to use this library. Along with reporting & upstreaming fixes to what are now new (hard) dependencies for us.
I think if we are going to do this, we should be vendoring libmultiprocess the same as secp256k1 or leveldb. Otherwise the dev experience is awkward, most will likely be building without, or might be forced to use depends (see above) etc.
WITH_MULTIPROCESS=ON should become the default, and a missing library should be an error (unless WITH_MULTIPROCESS=OFF is passed). Changing the default in depends is not enough, as we just end up with this awkward middle ground, where devs or outside builders are likely no-longer even building/testing what we are actually shipping to users.
#31375 I don’t think delaying figuring that out before we start using/shipping it, is a good idea. I think it’s exactly the kind of thing we should have a rought/good idea about, before making substantial change to what the project is doing.
Libmultiprocess was written, and is currently maintained by Russ. It is unclear how many of the other developers in this project have ever (seriously) looked at the code. My understanding is it might be 1 or 2 others. For something that is going to become a core part of the binaries we ship, and external interfaces we provide, it’d be great if that wasn’t the case.
Is the mining interface exposed on bitcoin-node now considered a (stable) external API? If this ships, what sort of guarantees are we giving to consumers?
It’d be good to have a better idea of where the demand is coming from, such that we need to ship this now.
Big new features / changes are done early after a branch off, so they have plenty of time to sit, be tested, devs can adjust etc. If this is merged now, it’s close to the last minute, with basically no real testing from inside the project.
Can you be more specific about what impacts this could practically have?
I don’t think most developers are running depends builds so developers should be unaffected unless this is causing CI errors.
I also don’t think users should be affected by this because should have no effect on existing binaries, it should only cause two new binaries to be built which (with #31679) should be in libexec/ and not even on the path.
The only real effects of this PR should just be to increase coverage of multiprocess code in CI and make to a new binary available for a mining feature.
All the other concerns you raise are valid and I think are reasons to take things one step at a time and not do things like not making WITH_MULTIPROCESS=ON the default yet and not declaring the API stable yet. But I don’t see a reason not to take the step this PR is taking, which should be a small step.
Based on todays IRC discussion, it probably makes most sense for me to drop bitcoin-node and bitcoin-gui from installable_targets so they won’t end up in v29 (if this is merged before that release)
Additionally / alternatively I could add -DWITH_MULTIPROCESS=OFF to CONFIGFLAGS in the guix build, or have it build depends with NO_MULTIPROCESS=1. That way it’s not built at all during the release process, but we do get the increased CI coverage.
There was some support for vendoring libmultiprocess. Since this PR increases test coverage, it might make sense to merge it first. But I can also rebase on a vendored version.
Marking draft pending:
I agree it’s currently tedious to use as a developer without vendoring. That also means that if a developer triggers a CI regression related to multiprocess, they may have difficulty reproducing it locally. That’s an argument for merging vendoring first.
Once vendored, making WITH_MULTIPROCESS=ON should be fine?
@fanquake wrote:
The mining interface isn’t finished yet?
I try to keep #31098 up to date.
Regarding the mining interface itself, the only new method I need for complete Stratum v2 support is #31283. It stands a reasonable chance of making the feature freeze deadline.
https://github.com/chaincodelabs/libmultiprocess/issues/122 is an edge case: Stratum v2 clients won’t hit it, but any other software using the interface could, which is why I included in the list of things that should be fixed before we ship the interface.
Note that #31564 is not needed for Stratum v2* and can go in any future release, if we merge it at all. (I have other interface method ideas, which I haven’t opened PRs for to reduce distraction)
Is the mining interface exposed on bitcoin-node now considered a (stable) external API? If this ships, what sort of guarantees are we giving to consumers?
For the next few releases it should be considered unstable. I maintain the only current client that uses it. Perhaps SRI will develop a Rust client, which would remove the need for a c++ based sidecar. I can communicate any interface changes to them for the foreseeable future.
* = not for the sidecar, there’s other places where it might be useful, but that can wait
The discussion on IRC also covered the question of whether me or SRI can ship IPCC binaries ourselves, at least for some time. I’m reluctant to do that, because if I have to tell people to download two binaries from me (bitcoin-node and bitcoin-miner), this seems a confusing experience. I’d rather tell people to get the official Bitcoin Core release, and then only a small (and easy to audit) bitcoin-miner binary from me.
However there may be some intermediate solution where I do ship bitcoin-node, but it’s a guix build from a release tag (maybe master). For that to work it should be possible to toggle multiprocess when invoking guix-build.
The “test each commit” task timed out, the rest passed.
Rebased and took the latest version of https://github.com/chaincodelabs/libmultiprocess/pull/129.
Pushed another commit that should build libmultiprocess with Tsan (on the ‘TSan, depends, gui’ worker).
10@@ -11,6 +11,11 @@ export CI_IMAGE_NAME_TAG="docker.io/ubuntu:24.04"
11 export APT_LLVM_V="19"
12 export PACKAGES="clang-${APT_LLVM_V} llvm-${APT_LLVM_V} libclang-rt-${APT_LLVM_V}-dev libc++abi-${APT_LLVM_V}-dev libc++-${APT_LLVM_V}-dev python3-zmq"
13 export DEP_OPTS="CC=clang-${APT_LLVM_V} CXX='clang++-${APT_LLVM_V} -stdlib=libc++'"
14+export LIBMULTIPROCESS_CMAKE_OPTIONS="-DCMAKE_BUILD_TYPE=Tsan"
Debug, Release, MinSizeRel and RelWithDebInfo are allowed, no? What is this Tsan build type doing? git grep Tsan in the top directory yields no results.
The depends build log seems to be suppressed. From my local machine:
0Preprocessing native_libmultiprocess...
1...
2-- The CXX compiler identification is AppleClang 16.0.0.16000026
3-- Detecting CXX compiler ABI info
4-- Detecting CXX compiler ABI info - done
5-- Check for working CXX compiler: /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/clang++ - skipped
6-- Detecting CXX compile features
7-- Detecting CXX compile features - done
8-- Performing Test CMAKE_HAVE_LIBC_PTHREAD
9-- Performing Test CMAKE_HAVE_LIBC_PTHREAD - Success
10-- Found Threads: TRUE
11-- Performing Test HAVE_PTHREAD_GETNAME_NP
12-- Performing Test HAVE_PTHREAD_GETNAME_NP - Success
13-- Performing Test HAVE_PTHREAD_THREADID_NP
14-- Performing Test HAVE_PTHREAD_THREADID_NP - Success
15-- Performing Test HAVE_PTHREAD_GETTHREADID_NP
16-- Performing Test HAVE_PTHREAD_GETTHREADID_NP - Failed
17-- Configuring done (0.5s)
18-- Generating done (0.1s)
So I think it’s doing something…
-DCMAKE_BUILD_TYPE=Tsan.
The difference can be seen at the install stage:
0-- Install configuration: ""
vs
0-- Install configuration: "Tsan"
https://cmake.org/cmake/help/latest/variable/CMAKE_BUILD_TYPE.html
Typical values include
Debug,Release,RelWithDebInfoandMinSizeRel, but custom build types can also be defined.
However, I do not think that our build has defined a custom build type Tsan. I checked that -DCMAKE_BUILD_TYPE=Tsan uses the same compile flags as -DCMAKE_BUILD_TYPE=FooBarBaz which are not the same as any of Debug, Release, RelWithDebInfo or MinSizeRel.
I think it would be better to drop this and leave it to the default or if needed, explicitly specify any of Debug, Release, RelWithDebInfo or MinSizeRel, or if needed actually implement a new build type Tsan in CMakeLists.txt.
This was based on the suggestion by @maflcko:
You could build libmultiprocess with tsan (edit: in the existing tsan task) to get a better check.
So I guess libmultiprocess needs to explicitly support this? @ryanofsky thoughts?
-fsanitize=thread. Maybe using cmake -DCMAKE_CXX_FLAGS=-fsanitize=thread when compiling libmiltiprocess would suffice (untested).
cmake --build . --verbose) that this actually uses -fsanitize=thread
LGTM c5b62d51dff10ff78ffdbe5af31f5f097c293b65 modulo:
The issue that this commit tries to address: 67417894dd [WIP]: try libmultiprocess fix
This PR installs two new binaries: bin/bitcoin-gui and bin/bitcoin-node, I agree with @fanquake above:
no explanation of why there is
bitcoind&bitcoin-node, as well asbitcoin-qt&bitcoin-gui, what the difference is betweem them, or which one someone should use
@ryanofsky the latest multiprocess master causes CI failures:
0 /home/runner/work/bitcoin/bitcoin/src/ipc/capnp/protocol.cpp:76:56: error: no member named 'cleanup' in 'mp::ProxyContext'
1 76 | mp::ProxyTypeRegister::types().at(type)(iface).cleanup.emplace_back(std::move(cleanup));
I’ll probably just let you PR the latest master yourself and then rebase.
(I also missed chaincodelabs/libmultiprocess#94)
Rebased on #31740.
🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/36231988070
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.
#31740 landed, now trying on top #31741 (not working yet).
I haven’t changed anything else yet.
What I’ll probably do is keep bitcoin-node (and bitcoin-gui) in installable_targets, but drop the default change. The focus of this PR will then be to increase CI coverage.
Changing the default can be done once we’re ready to include multiprocess in a release. The main thing I need until that time is to be able to do a MULTIPROCESS=1 ./contrib/guix/guix-build so I have the option of shipping this binaries myself (without further patches).
🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/36367376193
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.
I dropped eb41f92c0889708045ce0a30c6d0f8d43cecd6d5 if favor of setting MULTIPROCESS=1 for most depends jobs (9a20dc4f2ebbb4179dfabd6426aa442069649907).
Once #31741 lands (and this build succeeds) I’ll probably open a fresh PR to avoid confusion.
Also dropped the CI code that builds from chaincodelabs/libmultiprocess.
The “test each commit” job jails on 36db25f0bcaab6af9358faaad9214d5b77a71cb2 which is part of #31741:
0Start 5: mptest
1Unable to find executable: /home/runner/work/bitcoin/bitcoin/build/src/ipc/libmultiprocess/test/mptest
The ASan + LSan job also fails over missing mptest.
The macOS 14 native no depends job fails with https://github.com/chaincodelabs/libmultiprocess/issues/138.
The tidy job found several issues:
0[13:27:31.569] /ci_container_base/src/ipc/libmultiprocess/example/example.cpp:5:1: warning: included header filesystem is not used directly [misc-include-cleaner]
1[13:27:31.569] 5 | #include <filesystem>
2[13:27:31.569] | ^~~~~~~~~~~~~~~~~~~~~
3[13:27:31.569] 6 | #include <fstream>
4[13:27:31.569] /ci_container_base/src/ipc/libmultiprocess/example/example.cpp:7:10: error: 'init.capnp.h' file not found [clang-diagnostic-error]
5[13:27:31.569] 7 | #include <init.capnp.h>
The previous releases job seems have trouble downloading native_capnp during depends build.
Now that the scope of this PR has been reduced to only changing CI, I’ll need something like #31763 to more easily ship bitcoin-node binaries to (an advanced subset of) testers.
re: #30975 (comment)
Thanks I started looking into all of these but for “unable to find executable” errors, I think fix should be:
0--- a/src/ipc/CMakeLists.txt
1+++ b/src/ipc/CMakeLists.txt
2@@ -9,6 +9,8 @@ if (NOT WITH_LIBMULTIPROCESS)
3 # build rules can find the libmultiprocess include directory and avoid
4 # "error: Import failed: /mp/proxy.capnp" errors from capnp.
5 set(MP_INCLUDE_DIR "${MP_INCLUDE_DIR}" PARENT_SCOPE)
6+ # Add mptest unit tests to "all" target so ctest can run them
7+ set_target_properties(mptest PROPERTIES EXCLUDE_FROM_ALL OFF)
8 endif()
9
10 add_library(bitcoin_ipc STATIC EXCLUDE_FROM_ALL
Which i will add to base PR.
It didn’t work for the “test each commit” job, but that makes sense because the fix itself isn’t introduced early enough. That should go away once the base PR is merged.
Looks like we’re left with the tidy failure and https://github.com/chaincodelabs/libmultiprocess/issues/138.
(Windows is still running though…)
You mentioned the previous releases job was failing and now it is passing? Was this also the mptest bug?
No, it was having difficulty downloading capnp, maybe it was spurious.
@ryanofsky getting a 403 again this time on a different job: https://cirrus-ci.com/task/6262892556713984?logs=ci#L1459
This might just be an availability issue?
@ryanofsky getting a 403 again this time on a different job: https://cirrus-ci.com/task/6262892556713984?logs=ci#L1459
Thank you and good find. The error actual error there is make: *** [funcs.mk:320: /ci_container_base/depends/sources/download-stamps/.stamp_fetched-native_libmultiprocess-.hash] Error 22 which indicates a bug in the depends makefile in the base PR.
It looks like make is trying to download the libmultiprocess library when it should be using a subtree shouldn’t be trying to download it. There is also an earlier error Checksum missing or mismatched for native_libmultiprocess source. Forcing re-download. which partly explains why it is trying to do the download. I will debug more.
The macOS 14 native no depends job fails with chaincodelabs/libmultiprocess#138.
Seems to be passing now so I guess the attempted fix in f3f4685bc4f8d42c6ef6dbd223de25da235155ee is working. Will go ahead and merge that and add it to the base PR.
I also have a fix for the 403 download / “Checksum missing or mismatched for native_libmultiprocess” errors.
0--- a/depends/funcs.mk
1+++ b/depends/funcs.mk
2@@ -40,12 +40,16 @@ define fetch_file
3 endef
4
5 define fetch_local_dir_sha256
6- if ! [ -f $($(1)_fetched) ] || ! [ -f $($(1)_source) ] || [ $($(1)_source) -ot $($(1)_local_dir) ]; then \
7- mkdir -p $(dir $($(1)_fetched)); \
8+ if ! [ -f $($(1)_source) ] || [ $($(1)_source) -ot $($(1)_local_dir) ]; then \
9+ mkdir -p $(dir $($(1)_source)) && \
10 $(build_TAR) -c -f $($(1)_source) -C $($(1)_local_dir) . && \
11+ rm -f $($(1)_fetched); \
12+ fi && \
13+ if ! [ -f $($(1)_fetched) ]; then \
14+ mkdir -p $(dir $($(1)_fetched)) && \
15 $(build_SHA256SUM) $($(1)_source) > $($(1)_fetched); \
16- fi; \
17- cut -d" " -f1 $($(1)_fetched);
18+ fi && \
19+ cut -d" " -f1 $($(1)_fetched)
20 endef
21
22 define int_get_build_recipe_hash
That bug was caused by native_libmultiprocess and libmultiprocess package intentionally sharing the same source tarball but unintentionally generating the source tarball twice, causing one of the checksums to sometimes be invalid if tarball was not generated identically.
I think the only errors that leaves should be the clang-tidy errors, which I can fix and also add to the base PR.
also have a fix for the 403 download
Let’s see how that goes.
Update: it doesn’t, at least not for centos: https://github.com/bitcoin/bitcoin/pull/30975/checks?check_run_id=36436390795
Update: it doesn’t, at least not for centos: https://github.com/bitcoin/bitcoin/pull/30975/checks?check_run_id=36436390795
Thanks I don’t see what else could be causing it and I can’t reproduce it locally anymore. If we can add a few debug lines that might provide some insight:
0--- a/depends/Makefile
1+++ b/depends/Makefile
2@@ -251,8 +251,9 @@ endef
3
4 define check_or_remove_sources
5 mkdir -p $($(package)_source_dir); cd $($(package)_source_dir); \
6- test -f $($(package)_fetched) && ( $(build_SHA256SUM) -c $($(package)_fetched) >/dev/null 2>/dev/null || \
7+ test -f $($(package)_fetched) && ( $(build_SHA256SUM) -c $($(package)_fetched) >/dev/null || \
8 ( echo "Checksum missing or mismatched for $(package) source. Forcing re-download."; \
9+ set -x; cat $($(package)_fetched); : $($(package)_sha256_hash); ls -ld --full-time $(CURDIR)/$($(package)_local_dir) $($(package)_fetched) $($(package)_source); \
10 rm -f $($(package)_all_sources) $($(1)_fetched))) || true
11 endef
12
13--- a/depends/funcs.mk
14+++ b/depends/funcs.mk
15@@ -40,7 +40,7 @@ define fetch_file
16 endef
17
18 define fetch_local_dir_sha256
19- if ! [ -f $($(1)_source) ] || [ $($(1)_source) -ot $($(1)_local_dir) ]; then \
20+ set -x; if ! [ -f $($(1)_source) ] || [ $($(1)_source) -ot $($(1)_local_dir) ]; then \
21 mkdir -p $(dir $($(1)_source)) && \
22 $(build_TAR) -c -f $($(1)_source) -C $($(1)_local_dir) . && \
23 rm -f $($(1)_fetched); \
At this point (edc1fd44fa0d78168847b199d44ae107e33861e4) it seems like there are 2 failures left. One is the 403 download failure [1], [2], [3]
The other is the clang-tidy failure which I’m in the middle of fixing (slowly, since there a lot of warnings so I wanted to figure out how to get clang-tidy and iwyu running locally).
The test-each-commit job is also failing, but that would stop happening if commits were rearranged to make the fix for that 563d28502fb601d7eaf31af3341d06585d20df69 the first fix after the base PR.
The debug output from 403 download failures looks like
0make: Entering directory '/ci_container_base/depends'
1+ [ -f /ci_container_base/depends/sources/src-ipc-libmultiprocess.tar ]
2+ [ /ci_container_base/depends/sources/src-ipc-libmultiprocess.tar -ot ../src/ipc/libmultiprocess ]
3+ mkdir -p /ci_container_base/depends/sources/
4+ tar -c -f /ci_container_base/depends/sources/src-ipc-libmultiprocess.tar -C ../src/ipc/libmultiprocess .
5+ rm -f /ci_container_base/depends/sources/download-stamps/.stamp_fetched-libmultiprocess-.hash
6+ [ -f /ci_container_base/depends/sources/download-stamps/.stamp_fetched-libmultiprocess-.hash ]
7+ mkdir -p /ci_container_base/depends/sources/download-stamps/
8+ sha256sum /ci_container_base/depends/sources/src-ipc-libmultiprocess.tar
9+ cut -d -f1 /ci_container_base/depends/sources/download-stamps/.stamp_fetched-libmultiprocess-.hash
10+ [ -f /ci_container_base/depends/sources/src-ipc-libmultiprocess.tar ]
11+ [ /ci_container_base/depends/sources/src-ipc-libmultiprocess.tar -ot ../src/ipc/libmultiprocess ]
12+ [ -f /ci_container_base/depends/sources/download-stamps/.stamp_fetched-native_libmultiprocess-.hash ]
13+ cut -d -f1 /ci_container_base/depends/sources/download-stamps/.stamp_fetched-native_libmultiprocess-.hash
14sha256sum: WARNING: 1 computed checksum did NOT match
15Checksum missing or mismatched for native_libmultiprocess source. Forcing re-download.
16+ cat /ci_container_base/depends/sources/download-stamps/.stamp_fetched-native_libmultiprocess-.hash
17eb87cd36bebd5718a93bdb03b78f7855d8b64c41713e68f942e34625d9f5654e /ci_container_base/depends/sources/src-ipc-libmultiprocess.tar
18+ : eb87cd36bebd5718a93bdb03b78f7855d8b64c41713e68f942e34625d9f5654e
19+ ls -ld --full-time /ci_container_base/depends/../src/ipc/libmultiprocess /ci_container_base/depends/sources/download-stamps/.stamp_fetched-native_libmultiprocess-.hash /ci_container_base/depends/sources/src-ipc-libmultiprocess.tar
20drwxr-xr-x 9 root root 13 2025-01-31 07:55:09.538771383 +0000 /ci_container_base/depends/../src/ipc/libmultiprocess
21-rw-r--r-- 1 root root 129 2025-01-29 16:52:37.013133343 +0000 /ci_container_base/depends/sources/download-stamps/.stamp_fetched-native_libmultiprocess-.hash
22-rw-r--r-- 1 root root 296960 2025-01-31 07:57:04.733337218 +0000 /ci_container_base/depends/sources/src-ipc-libmultiprocess.tar
23+ rm -f /ci_container_base/depends/sources/download-stamps/.stamp_fetched-native_libmultiprocess-.hash
24Extracting native_capnp...
25/ci_container_base/depends/sources/capnproto-cxx-1.1.0.tar.gz: OK
26Preprocessing native_capnp...
27Configuring native_capnp...
28Building native_capnp...
29Staging native_capnp...
30Postprocessing native_capnp...
31Caching native_capnp...
32Fetching from
33curl: (3) URL rejected: No host part in the URL
34Fetching from https://bitcoincore.org/depends-sources
35 % Total % Received % Xferd Average Speed Time Time Time Current
36 Dload Upload Total Spent Left Speed
37
38 0 0 0 0 0 0 0 0 --:--:-- --:--:-- --:--:-- 0
39 0 308k 0 0 0 0 0 0 --:--:-- --:--:-- --:--:-- 0
40curl: (22) The requested URL returned error: 403
41make: *** [funcs.mk:324: /ci_container_base/depends/sources/download-stamps/.stamp_fetched-native_libmultiprocess-.hash] Error 22
42make: Leaving directory '/ci_container_base/depends'
43
44Exit status: 2
From the debug output that was added there I can see something going wrong, but I’m not sure what the cause is (EDIT: never mind, figured out the fix as I was writing this, it is at the bottom). It might have to do with some kind of caching. The debug output from the first fetch_local_dir_sha256 call for the cross-compiled libmultiprocess package looks perfect:
0+ [ -f /ci_container_base/depends/sources/src-ipc-libmultiprocess.tar ]
1+ [ /ci_container_base/depends/sources/src-ipc-libmultiprocess.tar -ot ../src/ipc/libmultiprocess ]
2+ mkdir -p /ci_container_base/depends/sources/
3+ tar -c -f /ci_container_base/depends/sources/src-ipc-libmultiprocess.tar -C ../src/ipc/libmultiprocess .
4+ rm -f /ci_container_base/depends/sources/download-stamps/.stamp_fetched-libmultiprocess-.hash
5+ [ -f /ci_container_base/depends/sources/download-stamps/.stamp_fetched-libmultiprocess-.hash ]
6+ mkdir -p /ci_container_base/depends/sources/download-stamps/
7+ sha256sum /ci_container_base/depends/sources/src-ipc-libmultiprocess.tar
8+ cut -d -f1 /ci_container_base/depends/sources/download-stamps/.stamp_fetched-libmultiprocess-.hash
It shows the tarball being created then the .stamp_fetched hash file being written and formatted for output.
But something wrong is happening after that in the fetch_local_dir_sha256 call for the native_libmultiprocess package:
0+ [ -f /ci_container_base/depends/sources/src-ipc-libmultiprocess.tar ]
1+ [ /ci_container_base/depends/sources/src-ipc-libmultiprocess.tar -ot ../src/ipc/libmultiprocess ]
2+ [ -f /ci_container_base/depends/sources/download-stamps/.stamp_fetched-native_libmultiprocess-.hash ]
3+ cut -d -f1 /ci_container_base/depends/sources/download-stamps/.stamp_fetched-native_libmultiprocess-.hash
This shows the tarball creation being skipped as expected, because the tarball already exists and has a fresh timestamp. But then the .stamp_fetched-native_libmultiprocess-.hash hash file NOT being created after that, apparently because the file already exists somehow. I think this might have something to do with caching, but I don’t know anything (not even the basics) of how caching works in CI. The later debug output from the check-sources rule confirms this to be the case:
0drwxr-xr-x 9 root root 13 2025-01-31 07:55:09.538771383 +0000 /ci_container_base/depends/../src/ipc/libmultiprocess
1-rw-r--r-- 1 root root 129 2025-01-29 16:52:37.013133343 +0000 /ci_container_base/depends/sources/download-stamps/.stamp_fetched-native_libmultiprocess-.hash
2-rw-r--r-- 1 root root 296960 2025-01-31 07:57:04.733337218 +0000 /ci_container_base/depends/sources/src-ipc-libmultiprocess.tar
Which shows tarball and directory both having a current mtime, but hash file having an mtime from 2 days ago.
Luckily, I think the fix for this can be pretty simple:
0- if ! [ -f $($(1)_fetched) ]; then \
1+ if ! [ -f $($(1)_fetched) ] || [ $($(1)_fetched) -ot $($(1)_source) ]; then \
The “clang-tidy” CI job shouldn’t process sources in the src/ipc/libmultiprocess/example/ directory:
0[07:11:14.250] [ 13/719][0.1s] clang-tidy-19 -p=/ci_container_base/ci/scratch/build-x86_64-pc-linux-gnu -quiet -load=/tidy-build/libbitcoin-tidy.so /ci_container_base/ci/scratch/build-x86_64-pc-linux-gnu/src/ipc/libmultiprocess/example/printer.capnp.proxy-client.c++
1[07:11:14.250] error: no input files [clang-diagnostic-error]
2[07:11:14.250] error: no such file or directory: '/ci_container_base/ci/scratch/build-x86_64-pc-linux-gnu/src/ipc/libmultiprocess/example/printer.capnp.proxy-client.c++' [clang-diagnostic-error]
The “clang-tidy” CI job shouldn’t process sources in the
src/ipc/libmultiprocess/example/directory
Maybe not, but I didn’t see an easy way to exclude them, so I thought I would just fix the clang-tidy errors. I fixed the missing inputs problem by just adding necessary targets back to all:
0--- a/src/ipc/CMakeLists.txt
1+++ b/src/ipc/CMakeLists.txt
2@@ -9,6 +9,10 @@ if (NOT WITH_LIBMULTIPROCESS)
3 # build rules can find the libmultiprocess include directory and avoid
4 # "error: Import failed: /mp/proxy.capnp" errors from capnp.
5 set(MP_INCLUDE_DIR "${MP_INCLUDE_DIR}" PARENT_SCOPE)
6+ # Add tests to "all" target so ctest can run them
7+ set_target_properties(mptests PROPERTIES EXCLUDE_FROM_ALL OFF)
8+ # Add examples to "all" target so clang-tidy can check them
9+ set_target_properties(mpexamples PROPERTIES EXCLUDE_FROM_ALL OFF)
10 endif()
11
12 add_library(bitcoin_ipc STATIC EXCLUDE_FROM_ALL
Tidy is happy!
But now macOS 14 native trips over building those examples (ld: library 'stdc++fs' not found).
https://github.com/bitcoin/bitcoin/actions/runs/13073304594/job/36479449027?pr=30975#step:7:2141
The “clang-tidy” CI job shouldn’t process sources in the
src/ipc/libmultiprocess/example/directoryMaybe not, but I didn’t see an easy way to exclude them, so I thought I would just fix the clang-tidy errors. I fixed the missing inputs problem by just adding necessary targets back to all:
0--- a/src/ipc/CMakeLists.txt 1+++ b/src/ipc/CMakeLists.txt 2@@ -9,6 +9,10 @@ if (NOT WITH_LIBMULTIPROCESS) 3 # build rules can find the libmultiprocess include directory and avoid 4 # "error: Import failed: /mp/proxy.capnp" errors from capnp. 5 set(MP_INCLUDE_DIR "${MP_INCLUDE_DIR}" PARENT_SCOPE) 6+ # Add tests to "all" target so ctest can run them 7+ set_target_properties(mptests PROPERTIES EXCLUDE_FROM_ALL OFF) 8+ # Add examples to "all" target so clang-tidy can check them 9+ set_target_properties(mpexamples PROPERTIES EXCLUDE_FROM_ALL OFF) 10 endif() 11 12 add_library(bitcoin_ipc STATIC EXCLUDE_FROM_ALL
Maybe something like this:
0--- a/src/ipc/CMakeLists.txt
1+++ b/src/ipc/CMakeLists.txt
2@@ -11,6 +11,7 @@ if (NOT WITH_LIBMULTIPROCESS)
3 set(MP_INCLUDE_DIR "${MP_INCLUDE_DIR}" PARENT_SCOPE)
4 # Add mptest unit tests to "all" target so ctest can run them
5 set_target_properties(mptest PROPERTIES EXCLUDE_FROM_ALL OFF)
6+ set_target_properties(mpcalculator mpprinter mpexample PROPERTIES EXPORT_COMPILE_COMMANDS OFF)
7 endif()
8
9 add_library(bitcoin_ipc STATIC EXCLUDE_FROM_ALL
?
All green! 🎉
(the test each commit job is limited to MAX_COUNT=6 which is why it’s happy now)
All green! 🎉
This is nice! But I am surprised to see clang-tidy build succeeding. There were so much other clang-tidy output in the previous failing job aside from the missing input file errors fixed by the cmake change: https://cirrus-ci.com/task/4607289014878208?logs=ci#L7174. But I guess all the other new clang-tidy output was warnings not errors, so they don’t cause the job to fail. Still planning on cleaning up the warnings anyway since I already fixed a lot of them locally.
But I am surprised to see clang-tidy build succeeding.
So am I.
There were so much other clang-tidy output in the previous failing job aside from the missing input file errors fixed by the cmake change: https://cirrus-ci.com/task/4607289014878208?logs=ci#L7174. But I guess all the other new clang-tidy output was warnings not errors, so they don’t cause the job to fail.
It could be the result of the absence of WarningsAsErrors: '*' in src/ipc/libmultiprocess/.clang-tidy.
Still planning on cleaning up the warnings anyway since I already fixed a lot of them locally.
I’ve just read it. My apologies for overstepping.
Bring in a few cmake changes to improve cmake support
https://github.com/chaincodelabs/libmultiprocess/pull/140 build: don't clobber user/superproject c++ version
https://github.com/chaincodelabs/libmultiprocess/pull/142 build: add option for external mpgen binary
https://github.com/chaincodelabs/libmultiprocess/pull/143 cleanup: initialize vars in the EventLoop constructor in the correct order
https://github.com/chaincodelabs/libmultiprocess/pull/146 cmake: Suppress compiler warnings from capnproto headers
https://github.com/chaincodelabs/libmultiprocess/pull/147 cmake: EXTERNAL_MPGEN cleanups
https://github.com/chaincodelabs/libmultiprocess/pull/148 util: fix -Wpessimizing-move warning
https://github.com/chaincodelabs/libmultiprocess/pull/145 CTest: Module must be included at the top level
https://github.com/chaincodelabs/libmultiprocess/pull/149 Avoid `-Wundef` compiler warnings
https://github.com/chaincodelabs/libmultiprocess/pull/152 refactor: Fix compiler and clang-tidy warnings
Rebased on the latest #31741, replaced -DBUILD_MULTIPROCESS with -DENABLE_IPC.
I dropped 8a7bcdbaa5a45b40ac451439106e78ade7194e37 “ci: build multiprocess with Tsan” since this is a non-depends job, so IIUC it should also build multiprocess with the same flags as the rest of the code.
I also dropped 743ae9b4e2746f48d707d49fc2c8ba908995034d “build: add bitcoin-{node,gui} to Maintenance.cmake” which can go in a followup that turns it on by default.
On my own macOS 15.3 with Xcode 16.2 machine building fails:
0git clean -dfx
1cmake -B build -DENABLE_IPC=ON
2cmake --build build
3...
4/Users/sjors/dev/bitcoin/src/ipc/libmultiprocess/src/mp/util.cpp:22:5: error: '__linux__' is not defined, evaluates to 0 [-Werror,-Wundef]
5 22 | #if __linux__
6 | ^
7/Users/sjors/dev/bitcoin/src/ipc/libmultiprocess/src/mp/util.cpp:62:5: error: '__linux__' is not defined, evaluates to 0 [-Werror,-Wundef]
8 62 | #if __linux__
9 | ^
102 errors generated.
Rebased on the latest #31741, replaced
-DBUILD_MULTIPROCESSwith-DENABLE_IPC.I dropped 8a7bcdb “ci: build multiprocess with Tsan” since this is a non-depends job, so IIUC it should also build multiprocess with the same flags as the rest of the code.
I also dropped 743ae9b “build: add bitcoin-{node,gui} to Maintenance.cmake” which can go in a followup that turns it on by default.
On my own macOS 15.3 with Xcode 16.2 machine building fails:
0git clean -dfx 1cmake -B build -DENABLE_IPC=ON 2cmake --build build 3... 4/Users/sjors/dev/bitcoin/src/ipc/libmultiprocess/src/mp/util.cpp:22:5: error: '__linux__' is not defined, evaluates to 0 [-Werror,-Wundef] 5 22 | #if __linux__ 6 | ^ 7/Users/sjors/dev/bitcoin/src/ipc/libmultiprocess/src/mp/util.cpp:62:5: error: '__linux__' is not defined, evaluates to 0 [-Werror,-Wundef] 8 62 | #if __linux__ 9 | ^ 102 errors generated.
Fixed in https://github.com/chaincodelabs/libmultiprocess/pull/149.
🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/36637228220
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.
The ASan build fails with a long and cryptic error: https://github.com/bitcoin/bitcoin/actions/runs/13131433348/job/36637228850?pr=30975
0/usr/bin/ld: CMakeFiles/mputil.dir/src/mp/util.cpp.o: in function `asan.module_ctor':
1util.cpp:(.text.asan.module_ctor[asan.module_ctor]+0x6): undefined reference to `__asan_init'
2/usr/bin/ld: util.cpp:(.text.asan.module_ctor[asan.module_ctor]+0xb): undefined reference to `__asan_version_mismatch_check_v8'
3/usr/bin/ld: util.cpp:(.text.asan.module_ctor[asan.module_ctor]+0x25): undefined reference to `__asan_register_elf_globals'
4/usr/bin/ld: CMakeFiles/mputil.dir/src/mp/util.cpp.o: in function `asan.module_dtor':
5util.cpp:(.text.asan.module_dtor[asan.module_dtor]+0x1b): undefined reference to `__asan_unregister_elf_globals'
6clang++-19: error: linker command failed with exit code 1 (use -v to see invocation)
7gmake[2]: *** [src/ipc/libmultiprocess/CMakeFiles/mpgen.dir/build.make:104: src/ipc/libmultiprocess/mpgen] Error 1
Pushed @hebasto’s fix for macOS builds.
macOS cross compile fails (in a different way): https://cirrus-ci.com/task/5052270577975296
0[09:45:29.772] /bin/sh: 1: /ci_container_base/depends/x86_64-apple-darwin/bin/capnp: Exec format error
macOS cross compile fails (in a different way): https://cirrus-ci.com/task/5052270577975296
0[09:45:29.772] /bin/sh: 1: /ci_container_base/depends/x86_64-apple-darwin/bin/capnp: Exec format error
It should call /ci_container_base/depends/x86_64-apple-darwin/native/bin/capnp.
macOS cross compile fails (in a different way): https://cirrus-ci.com/task/5052270577975296
0[09:45:29.772] /bin/sh: 1: /ci_container_base/depends/x86_64-apple-darwin/bin/capnp: Exec format error
Here is a fix:
0--- a/depends/toolchain.cmake.in
1+++ b/depends/toolchain.cmake.in
2@@ -155,6 +155,8 @@ endif()
3 if("@multiprocess@" STREQUAL "1")
4 set(ENABLE_IPC ON CACHE BOOL "")
5 set(Libmultiprocess_EXTERNAL_MPGEN "@depends_prefix@/native/bin/mpgen" CACHE PATH "")
6+ set(CAPNP_EXECUTABLE "${CMAKE_CURRENT_LIST_DIR}/native/bin/capnp" CACHE FILEPATH "")
7+ set(CAPNPC_CXX_EXECUTABLE "${CMAKE_CURRENT_LIST_DIR}/native/bin/capnpc-c++" CACHE FILEPATH "")
8 else()
9 set(ENABLE_IPC OFF CACHE BOOL "")
10 endif()
152@@ -153,9 +153,8 @@ else()
153 endif()
154
155 if("@multiprocess@" STREQUAL "1")
156- set(WITH_MULTIPROCESS ON CACHE BOOL "")
157- set(Libmultiprocess_ROOT "${CMAKE_CURRENT_LIST_DIR}" CACHE PATH "")
158- set(LibmultiprocessNative_ROOT "${CMAKE_CURRENT_LIST_DIR}/native" CACHE PATH "")
159+ set(ENABLE_IPC ON CACHE BOOL "")
160+ set(Libmultiprocess_EXTERNAL_MPGEN "@depends_prefix@/native/bin/mpgen" CACHE PATH "")
@hebasto macOS cross works!
Let’s see if only ASan remains… https://github.com/bitcoin/bitcoin/actions/runs/13138410284/job/36659308174?pr=30975
re: #30975 (comment)
Let’s see if only ASan remains… https://github.com/bitcoin/bitcoin/actions/runs/13138410284/job/36659308174?pr=30975
Following might fix the asan error:
0--- a/src/ipc/CMakeLists.txt
1+++ b/src/ipc/CMakeLists.txt
2@@ -9,6 +9,7 @@ if (NOT WITH_LIBMULTIPROCESS)
3 # Apply core_interface compile options to libmultiprocess runtime library.
4 target_link_libraries(multiprocess PUBLIC $<BUILD_INTERFACE:core_interface>)
5 target_link_libraries(mputil PUBLIC $<BUILD_INTERFACE:core_interface>)
6+ target_link_libraries(mpgen PUBLIC $<BUILD_INTERFACE:core_interface>)
7 # Share MP_INCLUDE_DIR variable with parent scope so target_capnp_sources
8 # build rules can find the libmultiprocess include directory and avoid
9 # "error: Import failed: /mp/proxy.capnp" errors from capnp.
Error is about mpgen failing to link because mputil library it depends on is compiled with asan/ubsan symbols. So mpgen probably needs to depend on core_interface to be built with same options.
I think that worked, and then found another issue:
0/home/runner/work/_temp/src/ipc/libmultiprocess/src/mp/gen.cpp:633:26: error: comparison of integers of different signs: 'size_t' (aka 'unsigned long') and 'int' [-Werror,-Wsign-compare]
1 633 | for (size_t i = 4; i < argc; ++i) {
2 | ~ ^ ~~~~
31 error generated.
https://github.com/bitcoin/bitcoin/actions/runs/13139156520/job/36661780168?pr=30975
Same issue on “[macOS 14 native, arm64, no depends, sqlite only, gui]”: https://github.com/bitcoin/bitcoin/actions/runs/13139156520/job/36661779305?pr=30975#step:7:1009
And on Tidy, so it looks like 0316528791d6218d2cb48d760593f89fb59f26b4 had a much broader impact.
… then found another issue:
0/home/runner/work/_temp/src/ipc/libmultiprocess/src/mp/gen.cpp:633:26: error: comparison of integers of different signs: 'size_t' (aka 'unsigned long') and 'int' [-Werror,-Wsign-compare] 1 633 | for (size_t i = 4; i < argc; ++i) { 2 | ~ ^ ~~~~ 31 error generated.
Fixed in https://github.com/chaincodelabs/libmultiprocess/pull/151.
git-subtree-dir: src/ipc/libmultiprocess
git-subtree-split: 9558ceb0d47ac1f62f88d29ec55f8f3099e32b20
Rename WITH_MULTIPROCESS to ENABLE_IPC, because ENABLE_IPC is a more accurate
name for the feature. It controls whether the src/ipc/ directory is built and
whether IPC features like -ipcbind, -ipcconnect, and -ipcfd are available. It
does NOT currently enable multiprocess features which are implemented in #10102
building on top of the IPC features. It will also no longer (as of the next
commit), control whether a find_package is call is made so the "WITH_" prefix
is also inappropriate.
-BEGIN VERIFY SCRIPT-
git grep -l WITH_MULTIPROCESS | xargs sed -i s/WITH_MULTIPROCESS/ENABLE_IPC/g
-END VERIFY SCRIPT-
When ENABLE_IPC option is on, build with libmultiprocess subtree and
`add_subdirectory(src/ipc/libmultiprocess)` instead of external package
and `find_package(Libmultiprocess)` by default.
Behavior can be toggled with `WITH_LIBMULTIPROCESS` option. Using a subtree
should be more convenient for most bitcoin developers, but using an external
package is more convenient for developing in the libmultiprocess repository.
The `WITH_LIBMULTIPROCESS` option is also used to avoid needing to changing the
depends build here. But in later commits, the depends build is switched to use
the add_subdirectory build as well.
Co-authored-by: Cory Fields <cory-nospam-@coryfields.com>
This change is technically not needed to add libmultiprocess as a subtree, but
it avoids a CI failure in followup PR #30975 which enables multiprocess build
option in more CI jobs. In that PR, several jobs fail due to the mptest
executable not being built by default, as reported
https://github.com/bitcoin/bitcoin/pull/30975#issuecomment-2623801480
This change is technically not needed to add libmultiprocess as a subtree, but
it avoids a CI failure in followup PR #30975 which enables multiprocess build
option in more CI jobs. In that PR, the "macOS 14 native no depends job" fails
due to warnings in boost headers treated as errors, reported in
https://github.com/bitcoin/bitcoin/pull/30975#issuecomment-2623801480 and
https://github.com/chaincodelabs/libmultiprocess/issues/138
This change is technically not needed to add libmultiprocess as a subtree, but
it avoids a CI failure in followup PR #30975 which enables multiprocess build
option in more CI jobs. In that PR, clang-tidy job fails due to missing
generated example files as reported
https://github.com/bitcoin/bitcoin/pull/30975#issuecomment-2627116832
Different fixes were suggested
https://github.com/bitcoin/bitcoin/pull/30975#issuecomment-2627152623 and
https://github.com/bitcoin/bitcoin/pull/30975#issuecomment-2627403382
Co-authored-by: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com>
Co-authored-by: Cory Fields <cory-nospam-@coryfields.com>
Without this change linter produces errors about:
- Use of std::filesystem the libmultiprocess example program.
- Use of locale-dependent functions in example program, in the build time code
generator, and in the runtime library for debug logging.
- Include guards not beginning with BITCOIN_
Move parts of the int_get_build_id into a new int_get_build_properties
function. There is no change in behavior. This just organizes assignments
better so some build properties can be used to help compute build ids in the
next commit.
With newly introduced libmultiprocess subtree, there's no need for depends
system to download and track changes to the upstream repository.
Note that adding the libmultiprocess subtree does not allow dropping
libmultiprocess packages from the depends build, because libmultiprocess
includes a code generation tool called mpgen, and in cross-compiled builds,
bitcoin core's cmake build system doesn't have access to a native toolchain and
can't build mpgen itself, so the depends system (or the native environment if
not using depends) needs to supply it.
Co-authored-by: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com>
Except:
1. i686, DEBUG (changed to "no multiprocess")
2. Windows due to lack of support
Install capnp where needed.
The bitcoin-node binary is built on all platforms which have
multiprocess enabled, but for functional tests it's only used in
the macOS native (no depends) and CentOS native (depends) jobs.
