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.

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

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.

Builds on #30940.

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

For detailed information about the code coverage, see the test coverage report.

Reviews

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

Conflicts

No conflicts as of last run.

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

depends: update libmultiprocess library

This update brings in the following changes:
chaincodelabs/libmultiprocess#111: doc: Add internal design section
chaincodelabs/libmultiprocess#113 Add missing include to util.h

9b1bdabe9c

depends: Update libmultiprocess library to fix MSAN failure

This update brings in the following changes:

https://github.com/chaincodelabs/libmultiprocess/pull/116  shutdown bugfix: destroy RPC system before running cleanup callbacks

6380ae88f3

build: depends makes libmultiprocess by default

Except for Windows.

e76fdae837

doc: depends builds multiprocess by default 8ee3076dd6

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

Add multiprocess binaries to release build #30975

Code Coverage

Reviews

Conflicts