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.

Combined with #30955 and https://github.com/Sjors/bitcoin/pull/52 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

Reviewers, this pull request conflicts with the following ones:

#30043 (net: Replace libnatpmp with built-in PCP+NATPMP implementation by laanwj)

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

build: depends makes libmultiprocess by default

Except for Windows.

a1e57166f0

doc: depends builds multiprocess by default 9cd46661ab

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

Add multiprocess binaries to release build #30975

Code Coverage

Reviews

Conflicts