Add multiprocess binaries to release build

Sjors commented at 10:29 am on September 26, 2024: member

This changes the depends system to build libmultiprocess by default. As a result Guix builds now add bitcoin-node and bitcoin-gui to the release binaries. See #30983 for a discussion of variations on this approach.

Combined with #30955 and #31003 this makes it feasible to implement a Stratum v2 Template Provider (or something similar) as an external tool that communicates with the node via IPC.

A complete implementation can be seen in https://github.com/Sjors/bitcoin/pull/48. That implementation builds on top of the Bitcoin Core codebase, but a Template Provider could also be built in Rust from the ground up. Or as a standalone c++ application that uses a library we (might, one day) expose.

IIUC the regular bitcoind, bitcoin-qt, etc binaries are not affected by this change.

It may be useful to also merge #30437 and document its usage.

bitcoin-gui could be omitted, but that doesn’t seem worth complicating the build system.

ci/test/00_setup_env_i686_multiprocess.sh is flipped to no_multiprocess.

TODO:

fix UndefinedBehaviorSanitizer: dynamic-type-mismatch failure) and re-enable multiprocess for ASAN job
fix undefined reference to LLVMFuzzerTestOneInput’` failure and re-enable multiprocess for fuzzer job
fix unknown target CPU 'armv8-a+crc+crypto' failure and re-enable macOS cross
maybe enable multiprocess for test-each-commit job

Followups:

Multiprocess is skipped for the Windows build for now, see https://github.com/chaincodelabs/libmultiprocess/issues/53.

A previous version of this PR changed the Guix build rather than the depends.

DrahtBot commented at 10:29 am on September 26, 2024: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/30975.

Reviews

See the guideline for information on the review process. A summary of reviews will appear here.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#31335 (macOS: swap docs & CI from pkg-config to pkgconf by fanquake)
#31308 (ci, iwyu: Treat warnings as errors for specific targets by hebasto)
#31306 (ci: Update Clang in “tidy” job by hebasto)
#31282 (refactor: Make node_id a const& in RemoveBlockRequest by maflcko)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

DrahtBot added the label Build system on Sep 26, 2024

in contrib/guix/libexec/build.sh:178 in 8b2546c35d outdated

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} \

Sjors commented at 10:31 am on September 26, 2024:

8b2546c35de37ea15aa7e22865943747b3006b61: it turns out that doing make MULTIPROCESS=0 in depends causes libmultiprocess to be built anyway. Not sure if that’s a known behavior or a cmake regression. cc @hebasto

hebasto commented at 10:52 am on September 26, 2024:

8b2546c: it turns out that doing make MULTIPROCESS=0 in depends causes 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.

Sjors commented at 10:34 am on September 26, 2024: member

I’ll look into the linter once it’s clear if the make MULTIPROCESS=0 behaviour above is intentional.

fanquake commented at 11:22 am on September 26, 2024: member

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-gui and bitcoin-wallet to 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

DrahtBot added the label CI failed on Sep 26, 2024

DrahtBot commented at 11:43 am on September 26, 2024: contributor

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

Sjors commented at 12:23 pm on September 26, 2024: member

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

ryanofsky commented at 12:37 pm on September 26, 2024: contributor

It’s good to have this PR open as a draft to see what it looks like but it sounds like there are a number of followups that need to happen for it to be ready. 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. It would also be good to review and merge #30940 which this is currently based on.

Sjors force-pushed on Sep 26, 2024

Sjors commented at 3:40 pm on September 26, 2024: member

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…

Sjors renamed this:
~~guix: add multiprocess binaries~~
Add multiprocess binaries to release build
on Sep 26, 2024

maflcko commented at 4:52 pm on September 26, 2024: member

From CI:

0SUMMARY: MemorySanitizer: use-of-uninitialized-value /usr/src/mp/proxy.cpp:146:25 in mp::Connection::addAsyncCleanup(std::__1::function<void ()>)

ryanofsky commented at 7:46 pm on September 26, 2024: contributor

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.

fanquake commented at 9:26 am on September 27, 2024: member

 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.

DrahtBot added the label Needs rebase on Sep 27, 2024

Sjors force-pushed on Sep 27, 2024

Sjors commented at 11:00 am on September 27, 2024: member

Rebased after #30940

DrahtBot removed the label Needs rebase on Sep 27, 2024

DrahtBot added the label Needs rebase on Sep 30, 2024

Sjors force-pushed on Oct 1, 2024

DrahtBot removed the label Needs rebase on Oct 1, 2024

Sjors force-pushed on Oct 1, 2024

Sjors commented at 7:50 am on October 1, 2024: member

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

Sjors commented at 11:01 am on October 1, 2024: member

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

ryanofsky commented at 2:58 pm on October 1, 2024: contributor

MSAN says https://cirrus-ci.com/task/6248232848719872

Thanks! I created https://github.com/chaincodelabs/libmultiprocess/issues/115 with details about this, and I think I have a potential fix.

ryanofsky commented at 4:42 pm on October 1, 2024: contributor

Could you try cherry-picking following commit to fix the MSAN failure? commit 7363a3eb8a1771b862febf3bd7257ad093887485 (branch)

Sjors force-pushed on Oct 1, 2024

DrahtBot removed the label CI failed on Oct 1, 2024

Sjors commented at 7:15 am on October 2, 2024: member

@ryanofsky that worked!

in src/test/ipc_test.cpp:65 in 03e8fe41c3 outdated

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>(),

Sjors commented at 6:11 pm on October 4, 2024:

03e8fe41c3500c29cd003170f7346872227f3ca5: @ryanofsky now that this breaking change is merged in libmultiprocess it would be good to have a PR to apply this on master.

Sjors force-pushed on Oct 5, 2024

Sjors commented at 12:30 pm on October 5, 2024: member

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.

DrahtBot commented at 3:02 pm on October 5, 2024: contributor

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

DrahtBot added the label CI failed on Oct 5, 2024

Sjors commented at 9:26 am on October 7, 2024: member

I dropped 1ea75505d18eea8e5f00f08d1e9e5b348691c7c0 again and instead opened https://github.com/Sjors/bitcoin/pull/65 to figure out how to best enable multiprocess on non-depends CI. It seems rather non-trivial.

Sjors force-pushed on Oct 7, 2024

DrahtBot removed the label CI failed on Oct 7, 2024

DrahtBot added the label Needs rebase on Oct 21, 2024

fanquake commented at 10:22 am on October 21, 2024: member

See cb3d8038b61440dff10316afd238603bd9f60924 for how I’d rework e76fdae837cea1384ea7fff231dd54ec5fbb0b19. Note that the _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.

Sjors commented at 3:07 pm on October 21, 2024: member

@fanquake thanks, I’ll look into that along with a rebase - might be after TABConf.

Sjors force-pushed on Nov 10, 2024

Sjors commented at 7:35 pm on November 10, 2024: member

@fanquake done and rebased after #31105.

Sjors commented at 8:19 pm on November 10, 2024: member

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?

DrahtBot added the label CI failed on Nov 10, 2024

Sjors force-pushed on Nov 10, 2024

Sjors commented at 8:37 pm on November 10, 2024: member

I set export BITCOIND=bitcoin-node just for the ARM job for now.

DrahtBot removed the label Needs rebase on Nov 10, 2024

DrahtBot removed the label CI failed on Nov 10, 2024

Sjors force-pushed on Nov 14, 2024

Sjors commented at 3:56 pm on November 14, 2024: member

I pulled in the changes from https://github.com/Sjors/bitcoin/pull/65 to enable multiprocess for non-depends builds. But I disabled it for two jobs, see TODO in PR description.

Sjors force-pushed on Nov 14, 2024

DrahtBot added the label Needs rebase on Nov 14, 2024

DrahtBot added the label CI failed on Nov 14, 2024

DrahtBot commented at 7:12 pm on November 14, 2024: contributor

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

Sjors force-pushed on Nov 18, 2024

Sjors commented at 11:34 am on November 18, 2024: member

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.

Sjors force-pushed on Nov 18, 2024

DrahtBot removed the label Needs rebase on Nov 18, 2024

DrahtBot removed the label CI failed on Nov 18, 2024

hebasto commented at 3:34 pm on November 18, 2024: member

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=

Sjors commented at 10:29 am on November 19, 2024: member

@hebasto I don’t understand.

hebasto commented at 10:42 am on November 19, 2024: member

@hebasto I don’t understand.

make -C depends HOST=arm-linux-gnueabihf does not build the libmultiprocess package.

Sjors commented at 11:03 am on November 19, 2024: member

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.

build: depends makes libmultiprocess by default

Except for Windows.

Co-Authored-By: fanquake <fanquake@gmail.com>

e2c085dd07

ci: install libmultiprocess for non-depends cd2225191a

ci: enable multiprocess for non-depends jobs

Except Windows

adab2dcbae

in depends/Makefile:161 in 57d1a7683f outdated

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)

Sjors commented at 11:26 am on November 19, 2024:

This was missing $(host_os)_

Sjors force-pushed on Nov 19, 2024

Sjors commented at 11:46 am on November 19, 2024: member

I wrote:

I set export BITCOIND=bitcoin-node just 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.

Add multiprocess binaries to release build #30975

Code Coverage & Benchmarks

Reviews

Conflicts