Sjors
commented at 4:04 pm on February 5, 2025:
member
Have depends make libmultiprocess by default. This PR causes the following behavior changes:
bitcoin-node and bitcoin-gui binaries are included in releases, due to ENABLE_IPC option being switched on by default in depends builds
ENABLE_IPC is also switched on by default in non-depends builds
Various changes to CI: switching on ENABLE_IPC on in most configurations and using bitcoin-node binary (bitcoin -m) for functional tests in two of them.
The bitcoin-node and bitcoin-gui are added to Maintenance.cmake (since they’re now in the release)
macOS and Linux installation instructions are updated to install capnp by default
This PR doesn’t need to do all of 3 things at once. However it’s is simpler, avoids code churn (especially in CI), and probably less confusing to make all these changes in the same PR.
Windows and OpenBSD are not supported yet, the latter due to a fairly trivial upstream issue. Therefore for those platforms ENABLE_IPC is off by default.
The initial main use case for IPC is to enable experimental support for the Mining IPC interface. A working example of a Stratum v2 Template Provider client using this interface can be found here: https://github.com/Sjors/bitcoin/pull/48.
See #31756 for discussion of when this should happen. Supersedes #30975.
Guix hashes:
0...
DrahtBot
commented at 4:04 pm on February 5, 2025:
contributor
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
Conflicts
Reviewers, this pull request conflicts with the following ones:
#32262 (build: Restore cross-compilation for Android by hebasto)
#32162 (depends: Switch from multilib to platform-specific toolchains by hebasto)
#31349 (ci: detect outbound internet traffic generated while running tests by vasild)
#30595 (kernel: Introduce initial C header API by TheCharlatan)
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
Needs rebase
on Feb 6, 2025
Sjors force-pushed
on Feb 7, 2025
Sjors force-pushed
on Feb 7, 2025
sipa
commented at 9:03 pm on February 7, 2025:
member
Some chatter from IRC:
017:21:41 < darosior> It might be confusing to release both a bitcoin-wallet utility and a bitcoin-wallet binary as part of multiprocess?
117:24:08 < darosior> We could rename the utility, but then it would be nice to at least have one deprecation cycle. Given recent momentum i estimate it's possible we might release multiprocess in
2 30.0, which means if we want to deprecate the bitcoin-wallet utility name we should do it.. now?
317:33:31 < sipa> bitcoin-wallet-util ?
4...
523:38:57 < _aj_> darosior: bitcoin-wallet-process, bitcoin-gui-process, etc? it's multi *-process!
603:04:34 < Sjors[m]> darosior: at the moment there is no wallet binary if were to enable multiprocess. That won't happen until [#19460](/bitcoin-bitcoin/19460/).
7...
803:05:18 < Sjors[m]> Or maybe already in [#10102](/bitcoin-bitcoin/10102/)
9...
1003:05:50 < Sjors[m]> In any case [#31802](/bitcoin-bitcoin/31802/) only adds bitcoin-node and bitcoin-gui.
11...
1203:07:19 < Sjors[m]> Though if we want to rename the utility eventually, it's always better to do it early.
Sjors
commented at 10:58 am on February 8, 2025:
member
DrahtBot removed the label
Needs rebase
on Feb 13, 2025
Sjors force-pushed
on Feb 17, 2025
Sjors force-pushed
on Feb 17, 2025
DrahtBot added the label
Needs rebase
on Feb 20, 2025
ryanofsky referenced this in commit
83e40d3b52
on Feb 24, 2025
ryanofsky referenced this in commit
8619f03ec2
on Feb 24, 2025
ryanofsky referenced this in commit
cbb7b41c20
on Feb 24, 2025
fanquake referenced this in commit
01f7715766
on Feb 25, 2025
fanquake referenced this in commit
ba0a4391ff
on Feb 25, 2025
pablomartin4btc referenced this in commit
bd2453d134
on Feb 26, 2025
pablomartin4btc referenced this in commit
75d5d235a6
on Feb 26, 2025
Sjors force-pushed
on Feb 27, 2025
DrahtBot removed the label
Needs rebase
on Feb 27, 2025
ngn999 referenced this in commit
fd12bfb763
on Feb 27, 2025
DrahtBot added the label
Needs rebase
on Mar 5, 2025
Sjors force-pushed
on Mar 11, 2025
DrahtBot removed the label
Needs rebase
on Mar 11, 2025
DrahtBot added the label
Needs rebase
on Mar 12, 2025
Sjors force-pushed
on Mar 12, 2025
DrahtBot removed the label
Needs rebase
on Mar 12, 2025
DrahtBot added the label
Needs rebase
on Mar 14, 2025
Sjors force-pushed
on Mar 17, 2025
DrahtBot removed the label
Needs rebase
on Mar 17, 2025
DrahtBot added the label
Needs rebase
on Mar 18, 2025
Sjors force-pushed
on Mar 18, 2025
DrahtBot removed the label
Needs rebase
on Mar 18, 2025
DrahtBot added the label
Needs rebase
on Mar 29, 2025
Sjors force-pushed
on Mar 31, 2025
DrahtBot removed the label
Needs rebase
on Mar 31, 2025
Sjors force-pushed
on Mar 31, 2025
DrahtBot added the label
Needs rebase
on Apr 2, 2025
Sjors force-pushed
on Apr 2, 2025
DrahtBot removed the label
Needs rebase
on Apr 2, 2025
DrahtBot added the label
Needs rebase
on Apr 4, 2025
Sjors
commented at 4:32 pm on April 10, 2025:
member
As discussed in IRC, I’ll turn on IPC in the default cmake build (not just the dev preset). Either here on in the previous PR.
ryanofsky
commented at 5:53 pm on April 10, 2025:
contributor
As discussed in IRC, I’ll turn on IPC in the default cmake build (not just the dev preset). Either here on in the previous PR.
In case it helps I actually have an old commit implementing this: a6d9db48249d5d9bd63ea0d99e15066a1299e5c3 (branch) since I was originally thinking of including it in #31741
Sjors force-pushed
on Apr 11, 2025
DrahtBot removed the label
Needs rebase
on Apr 11, 2025
Sjors marked this as ready for review
on Apr 11, 2025
Sjors
commented at 7:32 pm on April 11, 2025:
member
Marked this as ready for review, but see discussion here: #30975 (comment)
ryanofsky
commented at 8:49 pm on April 11, 2025:
contributor
It seems like there are some MULTIPROCESS=1 settings added earlier in this PR that are not removed later and become ignored, so these would be good to clean up
It could be good to simplify this PR and avoid churn by moving the two CI commits last (if they are still needed) after changing the cmake and depends ENABLE_IPC defaults from true to false.
It could make sense to rename the NO_MULTIPROCESS depends option to NO_IPC to be consistent with the cmake ENABLE_IPC option. There are some other depends/cmake options that seem to share the same names like NO_WALLET/ENABLE_WALLET, NO_ZMQ/WITH_ZMQ, NO_USDT/WITH_USDT, etc
Sjors force-pushed
on Apr 11, 2025
Sjors
commented at 9:58 pm on April 11, 2025:
member
I moved the ci commits last and removed the unneeded stuff. So this PR is now independent of #30975, although it overlaps.
Hopefully didn’t break anything…
Does anyone have a strong preference which CI job(s) should build without multiprocess? Right now the Windows and one i686 cirrus job are.
Any which CI job(s) should use bitcoin-node in the functional tests? I picked CentOS (depends) and macOS native (no depends).
Except for Windows, these choices are fairly arbitrary.
Sjors force-pushed
on Apr 11, 2025
DrahtBot added the label
Needs rebase
on Apr 14, 2025
Sjors force-pushed
on Apr 15, 2025
DrahtBot removed the label
Needs rebase
on Apr 15, 2025
in
depends/README.md:121
in
9b46b7d582outdated
117@@ -118,7 +118,7 @@ The following can be set when running make: `make FOO=bar`
118 - `NO_WALLET`: Don't download/build/cache libs needed to enable the wallet (SQLite)
119 - `NO_BDB`: Don't download/build/cache BerkeleyDB
120 - `NO_USDT`: Don't download/build/cache packages needed for enabling USDT tracepoints
121-- `MULTIPROCESS`: Build libmultiprocess (experimental)
122+- `NO_IPC`: Don't build experimental libmultiprocess (default on Windows and OpenBSD)
In commit “build: depends makes libmultiprocess by default” (9b46b7d582edcf61bbec5b01c8e661af804e22d6)
Think it would be good if this description described the full effects of this option. Would maybe say “NO_IPC: don’t build Cap’n Proto and libmultiprocess packages, and disable building IPC binaries (bitcoin-node, bitcoin-gui)”
Also, since this is the most significant commit in the PR, which controls whether IPC binaries are included in releases, it would be good to mention the effect this has on releases in the commit description.
Commit description could also mention effect this has on CI builds: it cause more depends builds to build IPC binaries, but still the only build running functional tests with them is the i686_multiprocess one.
ryanofsky
commented at 5:35 pm on April 15, 2025:
contributor
Code review ACK5d96e656d72abee468f5f49759b1b08eab445d4d. Changes seem pretty clean and simple now. I think it might be good for PR to clearly mention the 3 changes in behavior it causes:
1 - Causes bitcoin-node and bitcoin-gui binaries to be included in releases and ENABLE_IPC option to be switched on by default in depends builds.
2 - Also causes ENABLE_IPC option to be switched on by default in both non-depends builds
3 - Makes various changes to CI: switching on ENABLE_IPC on in most configurations and using bitcoin-node binary for functional tests in two of them.
It would also be good to point out that that this PR doesn’t need to do all of 3 things at once. It is simpler and probably less confusing to make all these changes in the same PR, but if we want to isolate any of these things and make them separate, that’s possible.
All the choices you made about CI jobs options seem reasonable, and seem to test everything we would want. But @maflcko has best understanding of CI jobs and might have input.
DrahtBot added the label
Needs rebase
on Apr 17, 2025
Sjors force-pushed
on Apr 17, 2025
Sjors
commented at 12:42 pm on April 17, 2025:
member
Rebased after #32271, updated PR description, commit description and comment as suggested here: #31802 (review)
Sjors force-pushed
on Apr 17, 2025
DrahtBot added the label
CI failed
on Apr 17, 2025
in
doc/multiprocess.md:19
in
341b2a99e6outdated
15@@ -16,11 +16,11 @@ Specifying `-DENABLE_IPC=ON` requires [Cap'n Proto](https://capnproto.org/) to b
1617 ### Depends installation
1819-Alternately the [depends system](../depends) can be used to avoid need to install local dependencies. A simple way to get started is to pass the `MULTIPROCESS=1` [dependency option](../depends#dependency-options) to make:
20+Alternatively the [depends system](../depends) can be used to avoid needing to local install dependencies:
Alternatively the depends system can be used to avoid needing to local install dependencies:
—> …avoid needing to install local dependencies:
DrahtBot removed the label
Needs rebase
on Apr 17, 2025
DrahtBot removed the label
CI failed
on Apr 17, 2025
Sjors
commented at 7:32 am on April 18, 2025:
member
This or a followup needs to update doc/dependencies.md and update the build instructions to change opt-in wording to opt-out. I’ll update that here if #32293 (comment) lands earlier.
ryanofsky approved
ryanofsky
commented at 6:12 pm on April 21, 2025:
contributor
Code review ACK341b2a99e646246a9e4d3aa283b2b59442cfad27. Only change since last review is making various improvements to comments and rebasing.
Would be nice to update doc/dependencies.md here since #32293 is no longer doing that but not important.
Sjors force-pushed
on Apr 23, 2025
Sjors
commented at 8:19 am on April 23, 2025:
member
Rebased after #32293, updated the documentation it introduced, and added Cap’n Proto to doc/dependencies.md. Also took the LLM fix.
in
doc/dependencies.md:39
in
ce415e06b1outdated
35@@ -36,3 +36,4 @@ Bitcoin Core requires one of the following compilers.
36 | [SQLite](../depends/packages/sqlite.mk) (wallet) | [link](https://sqlite.org) | [3.38.5](https://github.com/bitcoin/bitcoin/pull/25378) | [3.7.17](https://github.com/bitcoin/bitcoin/pull/19077) | No |
37 | Python (scripts, tests) | [link](https://www.python.org) | N/A | [3.10](https://github.com/bitcoin/bitcoin/pull/30527) | No |
38 | [systemtap](../depends/packages/systemtap.mk) ([tracing](tracing.md)) | [link](https://sourceware.org/systemtap/) | [4.8](https://github.com/bitcoin/bitcoin/pull/26945)| N/A | No |
39+| [Cap'n Proto](../depends/packages/capnp.mk) | [link](https://capnproto.org) | [0.6.1](https://capnproto.org/install.html) | 0.5.3 | Yes |
Fixed. @TheCharlatan why was this set to Yes in the original?
TheCharlatan
commented at 8:23 pm on April 29, 2025:
Fixed. @TheCharlatan why was this set to Yes in the original?
It did not make that much sense to include since it was not part of the release binaries yet, but I thought it could be considered a runtime dependency in case it is not linked statically through depends or some other method.
Sjors force-pushed
on Apr 23, 2025
in
doc/build-osx.md:132
in
1eedff84d7outdated
133-Skip if you do not need IPC functionality.
134-
135-```bash
136-brew install capnp
137-```
138+The depedency on `capnp` can be avoided by compiling without IPC-enabled binaries
Sjors
commented at 9:59 am on April 23, 2025:
member
An alternative approach could be to make Cap’n Proto optional and have -DENABLE_IPC default to ON only if its present. Though I vaguely recall that with CMake we wanted to move away from enabling config options depending on whether a library is found.
Sjors force-pushed
on Apr 23, 2025
TheCharlatan
commented at 8:25 pm on April 29, 2025:
contributor
Concept ACK
An alternative approach could be to make Cap’n Proto optional and have -DENABLE_IPC default to ON only if its present. Though I vaguely recall that with CMake we wanted to move away from enabling config options depending on whether a library is found.
Yeah, this is intentionally not done anymore since the cmake migration. I think the current approach is fine.
DrahtBot added the label
Needs rebase
on May 6, 2025
DrahtBot removed the label
Needs rebase
on May 7, 2025
DrahtBot added the label
CI failed
on May 7, 2025
DrahtBot
commented at 10:38 am on May 7, 2025:
contributor
🚧 At least one of the CI tasks failed.
Task tidy: https://github.com/bitcoin/bitcoin/runs/41784006413
LLM reason (✨ experimental): The CI failed due to clang-tidy errors related to incorrect use of std::move and thread_local.
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
commented at 11:34 am on May 7, 2025:
member
0/ci_container_base/src/ipc/libmultiprocess/include/mp/proxy-io.h:252:16: error: forwarding reference passed to std::move(), which may unexpectedly cause lvalues to be moved; use std::forward() instead [bugprone-move-forwarding-reference,-warnings-as-errors]
1[05:29:23.932] 252 | m_fn = std::move(fn);
2[05:29:23.932] | ^~~~~~~~~
3[05:29:23.932] | std::forward<Fn>
DrahtBot added the label
Needs rebase
on May 7, 2025
DrahtBot removed the label
Needs rebase
on May 15, 2025
ryanofsky referenced this in commit
c1e8c1a028
on May 15, 2025
ryanofsky referenced this in commit
867a3fa894
on May 15, 2025
Sjors force-pushed
on May 16, 2025
DrahtBot added the label
CI failed
on May 16, 2025
DrahtBot removed the label
CI failed
on May 16, 2025
DrahtBot added the label
Needs rebase
on May 22, 2025
build: add bitcoin-{node,gui} to Maintenance.cmakeacd7cf80b5
build: depends makes libmultiprocess by default
This causes IPC binaries (bitcoin-node, bitcoin-gui) to be included
in releases.
The effect on CI is that this causes more depends builds to build IPC
binaries, but still the only build running functional tests with them
is the i686_multiprocess one.
Except for Windows and OpenBSD.
dc653a61b2
ci: build one depends job without multiprocess4ce6310dbb
Sjors force-pushed
on May 28, 2025
ci: use bitcoin-node for one depends job
The bitcoin-node binary is built on all platforms which have
multiprocess enabled, but for functional tests it's only used in
CentOS native (depends) job. The next commit will also add a
non-depends job.
2899ce8157
cmake: enable ENABLE_IPC option by default
Install capnp on non-depends CI jobs.
Use the bitcoin-node binary in the macOS native non-depends job.
Co-authored-by: Ryan Ofsky <ryan@ofsky.org>
2720915977
Sjors force-pushed
on May 28, 2025
Sjors
commented at 8:11 am on May 28, 2025:
member
This is a metadata mirror of the GitHub repository
bitcoin/bitcoin.
This site is not affiliated with GitHub.
Content is generated from a GitHub metadata backup.
generated: 2025-05-29 18:12 UTC
This site is hosted by @0xB10C More mirrored repositories can be found on mirror.b10c.me
