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 is not supported yet, so ENABLE_IPC is off by default for it. It can be enabled after #32387.
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.
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:
#32989 (ci: Migrate CI to hosted Cirrus Runners by willcl-ark)
#32162 (depends: Switch from multilib to platform-specific toolchains 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.
LLM Linter (✨ experimental)
Possible typos and grammar issues:
bitcoin-gui(#31802) -> bitcoin-gui (#31802) [missing space before the parenthetical reference]
drahtbot_id_4_m
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
Although #31679 is orthogonal, it’s also worth reviewing. There should be no impact in user experience, because thanks to #31375 the recommended invocation bitcoin -m will find bitcoin-node whether it’s in /usr/local/bin or /usr/local/libexec. But an upgrade would leave a stale version of bitcoin-node in bin, so it’s nicer to get that PR in the same release as this one. Merge order shouldn’t matter.
Sjors marked this as ready for review
on May 30, 2025
TheCharlatan
commented at 1:26 pm on May 30, 2025:
contributor
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>
My apologies for missing this in d4bc5639829f0c98bd2faeddba9be4e8c6667875. We should treat the libmultiprocess subtree the same as all other subtrees and disable the EXPORT_COMPILE_COMMANDS property for all its targets.
We should treat the libmultiprocess subtree the same as all other subtrees and disable the EXPORT_COMPILE_COMMANDS property for all its targets.
This could be reasonable, though I’m not very sure what the benefits of disabling linting here would be. Also, I think even with EXPORT_COMPILE_COMMANDS disabled, errors similar to #31802 (comment) could still happen because that error comes from a header file included via -I, not -isystem
DrahtBot requested review from TheCharlatan
on Jun 2, 2025
fanquake
commented at 4:02 pm on June 5, 2025:
member
Sjors
commented at 5:10 pm on June 11, 2025:
member
Rebased after #32690 and dropped the OpenBSD exception.
I ran the functional test suite for this PR on my OpenBSD vm.
Sjors force-pushed
on Jun 12, 2025
in
depends/README.md:83
in
5f21cb59a9outdated
79@@ -80,7 +80,7 @@ The following can be set when running make: `make FOO=bar`
80 - `NO_ZMQ`: Don't download/build/cache packages needed for enabling ZeroMQ
81 - `NO_WALLET`: Don't download/build/cache libs needed to enable the wallet (SQLite)
82 - `NO_USDT`: Don't download/build/cache packages needed for enabling USDT tracepoints
83-- `MULTIPROCESS`: Build libmultiprocess (experimental)
84+- `NO_IPC`: Don't build Cap’n Proto and libmultiprocess packages, and disable building IPC binaries (bitcoin-node, bitcoin-gui). Default on Windows.
It seems unavoidable to change ci in the same commit.
Previous iterations were changing BUILD_CONFIG, hence the formatting change. But I’ll just drop them now.
Sjors force-pushed
on Jun 12, 2025
DrahtBot added the label
CI failed
on Jun 12, 2025
DrahtBot
commented at 12:05 pm on June 12, 2025:
contributor
🚧 At least one of the CI tasks failed.
Task tidy: https://github.com/bitcoin/bitcoin/runs/43960756779
LLM reason (✨ experimental): The CI failure is caused by errors in clang-tidy checks related to deprecated header includes, which are treated as fatal errors.
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 2:37 pm on June 12, 2025:
member
0 node0 2025-06-12T12:34:42.336592Z (mocktime: 2025-06-12T12:34:46Z)[net][net.cpp:2041][InactivityCheck][net] version handshake timeout, disconnecting peer=0 1 test 2025-06-12T12:34:43.290505Z TestFramework.utils (ERROR): wait_until() failed. Predicate: '''' 2 def test_function():
3if check_connected:
4 assert self.is_connected
5return test_function_in() 6'''
7 node0 2025-06-12T12:34:44.168910Z (mocktime: 2025-06-12T12:34:46Z) [net] [net.cpp:2041] [InactivityCheck] [net] version handshake timeout, disconnecting peer=1
8 node0 2025-06-12T12:34:44.168933Z (mocktime: 2025-06-12T12:34:46Z) [net] [net.cpp:2041] [InactivityCheck] [net] version handshake timeout, disconnecting peer=2
9 node0 2025-06-12T12:34:44.168948Z (mocktime: 2025-06-12T12:34:46Z) [net] [net.cpp:558] [CloseSocketDisconnect] [net] Resetting socket for peer=0
10 test 2025-06-12T12:34:44.169006Z TestFramework (ERROR): Assertion failed
11 Traceback (most recent call last):
12 File "/home/runner/work/bitcoin/bitcoin/test/functional/test_framework/test_framework.py", line 190, in main
13 self.run_test()
14 File "/home/runner/work/bitcoin/bitcoin/build/test/functional/p2p_timeouts.py", line 100, in run_test
15 no_verack_node.wait_for_disconnect(timeout=1)
16 File "/home/runner/work/bitcoin/bitcoin/test/functional/test_framework/p2p.py", line 609, in wait_for_disconnect
17 self.wait_until(test_function, timeout=timeout, check_connected=False)
18 File "/home/runner/work/bitcoin/bitcoin/test/functional/test_framework/p2p.py", line 601, in wait_until
19 wait_until_helper_internal(test_function, timeout=timeout, lock=p2p_lock, timeout_factor=self.timeout_factor, check_interval=check_interval)
20 File "/home/runner/work/bitcoin/bitcoin/test/functional/test_framework/util.py", line 321, in wait_until_helper_internal
21 raise AssertionError("Predicate {} not true after {} seconds".format(predicate_source, timeout))
22 AssertionError: Predicate ''''
23 def test_function():
24 if check_connected:
25 assert self.is_connected
26 return test_function_in()
27 ''' not true after 1 seconds
28 node0 2025-06-12T12:34:44.169056Z (mocktime: 2025-06-12T12:34:46Z)[net][net.cpp:558][CloseSocketDisconnect][net] Resetting socket forpeer=1
DrahtBot removed the label
CI failed
on Jun 13, 2025
Sjors
commented at 11:30 am on June 16, 2025:
member
Added the commit from #32760 to see if bumping capnp (depends) reveals any issues. Marking draft, will rebase after it lands.
I tested this with OpenBSD (x86, as well as aarch64 with 4f10a57671c19cacca630b2401e42a213aacff1b from #32716).
Sjors marked this as a draft
on Jun 16, 2025
ryanofsky approved
ryanofsky
commented at 1:46 pm on June 16, 2025:
contributor
Code review ACK7ecf6bbd84f200dab1ba7cbcae893edc4ed0d581. Since last review rebased, enabled openbsd support by updating capnp, renamed variable and dropped whitespace changes, and avoided c includes to fix tidy modernize-deprecated-headers errors
DrahtBot added the label
CI failed
on Jun 16, 2025
Sjors force-pushed
on Jun 17, 2025
Sjors marked this as ready for review
on Jun 17, 2025
Sjors
commented at 7:31 am on June 17, 2025:
member
Rebased after #32760 (only change is dropping that commit).
DrahtBot removed the label
CI failed
on Jun 17, 2025
ryanofsky approved
ryanofsky
commented at 10:34 am on June 17, 2025:
contributor
Code review ACK5a66729744db797e40f375d62aaebba66884ea6f. No change other than rebase and dropping base commit
vasild approved
vasild
commented at 10:18 am on June 19, 2025:
contributor
ACK5a66729744db797e40f375d62aaebba66884ea6f
Since you are renaming “multiprocess” to “ipc” in some places and you are also renaming 00_setup_env_i686_multiprocess.sh to 00_setup_env_i686_no_multiprocess.sh maybe 00_setup_env_i686_no_ipc.sh would be a better new name of the file?
The last commit refactor: modernize deprecated ipc headers is fine, but I am not sure why it is needed in this PR since those (old) .h includes were not introduced in this PR. Is there not the same problem in master? Or maybe tidy did not look at files in src/ipc/ before? If yes, then maybe that commit should be first, before the one that makes tidy check src/ipc/, so that all intermediate commits are good.
maflcko
commented at 11:16 am on June 19, 2025:
member
The last commit refactor: modernize deprecated ipc headers is fine, but I am not sure why it is needed in this PR since those (old) .h includes were not introduced in this PR. Is there not the same problem in master? Or maybe tidy did not look at files in src/ipc/ before? If yes, then maybe that commit should be first, before the one that makes tidy check src/ipc/, so that all intermediate commits are good.
If you want it to be bisect-nice, I’d guess you’d have to set -DENABLE_IPC=OFF in the tidy CI in the second to last commit and then enable it in the last commit. However I haven’t tested this and I don’t think anyone is bisecting the CI on non-merge commits, so anything should be fine here.
Sjors
commented at 12:21 pm on June 19, 2025:
member
I don’t think anyone is bisecting the CI on non-merge commits
Indeed that’s why I didn’t move it up initially. With this many CI changes it’s hard to be bisect friendly to that level anyway.
@vasild if I need to retouch I’ll move that commit up and rename some more multiprocess things to ipc.
fanquake
commented at 12:53 pm on June 19, 2025:
member
This PR needs release notes.
Note that the Tidy job here is passing, even though it’s generating output (which also seems like a bug?). That should be addressed, to either fix any issues or suppress any false-positives; otherwise it’s going to make the tidy output unusable. i.e (https://cirrus-ci.com/task/6552721135763456):
/usr/include/kj/async-inl.h:609:37: warning: The left operand of '+' is a garbage value [clang-analyzer-core.UndefinedBinaryOperatorResult]
Pretty sure this isn’t a bug. Changing [&] to [] should fix it:
/ci_container_base/src/ipc/libmultiprocess/include/mp/proxy-types.h:134:5: warning: Address of stack memory associated with temporary object of type '(lambda at /ci_container_base/src/ipc/libmultiprocess/include/mp/proxy-types.h:134:51)' is still referred to by a temporary object on the stack upon returning to the caller. This will be a dangling reference [clang-analyzer-core.StackAddressEscape]
This might be a test bug:
/ci_container_base/src/ipc/libmultiprocess/include/mp/type-number.h:55:32: warning: The value '0' provided to the cast expression is not in the valid range of values for 'FooEnum' [clang-analyzer-optin.core.EnumCastOutOfRange]
Marking as ready for review, because the tidy fix doesn’t seem blocking to me - now that a fix is being worked on. Happy to rebase after it lands though.
Sjors marked this as ready for review
on Jun 23, 2025
in
doc/release-notes-31802.md:1
in
314b4282e6outdated
0@@ -0,0 +1 @@
1+- Two new binaries are added to the release: `bitcoin-node` (or `bitcoin -m node`) and `bitcoin-gui` (or `bitcoin -m gui`). These enable IPC functionality, initially only for the (experimental) Mining interface. Building the binaries is controlled by `ENABLE_IPC` (enabled by default, except for Windows). When building from source without depends, `capnp` needs to be installed. (#31802)
In commit “doc: add release note” (314b4282e6b741e38f914246557da5787598aa63)
This seems ok but may want to rephrase it at some point (#31679 could be a good place) to be more about the new IPC feature (-ipcbind) than the new binaries. The binaries are kind of an implementation detail, and there should be no need to use them directly if the bitcoin -m is used.
Yea, these need fleshing out, because they don’t give much useful information to users, and will likely leave them with questions: “What’s IPC?” (never defined), “What’s bitcoin-node; should I use that instead of bitcoind?” (same for the GUI), “Why is this enabled by default if it’s for some experimental feature I (and basically all other users) don’t use?”.
I hope the release notes in #31679 (https://github.com/ryanofsky/bitcoin/blob/pr/libexec/doc/release-notes-31375-31679.md) can be a good starting point for a unified description of all the changes. I think all that might be needed there is a paragraph mentioning that the new bitcoin command has a multiprocess-m option enabling features which allow separate processes to communicate with each other. The only feature currently enabled by -m is an -ipcbind option allowing external tools to connect to the node and efficiently generate block templates for mining. But in the future -m may also allow different Bitcoin Core components (such as the wallet, node, and gui) to run in separate processes and be managed independently. The -m option might also be enabled by default in a future release.
ryanofsky approved
ryanofsky
commented at 1:13 pm on June 23, 2025:
contributor
Code review ACK314b4282e6b741e38f914246557da5787598aa63. Just added release notes and renamed CI job since last review.
I tried to include the changes in bitcoin-core/libmultiprocess#184 to see if they resolve all tidy issues, but that requires additional changes on the Bitcoin Core side (will report on that PR).
DrahtBot requested review from vasild
on Jun 23, 2025
ryanofsky referenced this in commit
980ab41ef8
on Jun 23, 2025
ryanofsky referenced this in commit
97f8435ca9
on Jun 23, 2025
ryanofsky referenced this in commit
5049b81657
on Jun 23, 2025
TheCharlatan
commented at 9:42 pm on June 23, 2025:
contributor
Tangentially related to the topic here, have we discussed adding the capnp files to the releases somewhere yet? I’d be interested what the thoughts are around that. I think at least for the mining interface, where some thought has been spent on how to make it usable for an external project this would make sense. The alternative of having to regularly copy them over to the target project seems less ideal to me.
ryanofsky referenced this in commit
53c532e798
on Jun 25, 2025
ryanofsky referenced this in commit
3e6bcf18e5
on Jun 25, 2025
ryanofsky referenced this in commit
d19ee2cf66
on Jun 25, 2025
ryanofsky referenced this in commit
b7ae258b5e
on Jun 25, 2025
ryanofsky referenced this in commit
26d6232f84
on Jun 25, 2025
ryanofsky referenced this in commit
661caf7c1f
on Jun 25, 2025
ryanofsky referenced this in commit
7abe89cc16
on Jun 25, 2025
ryanofsky referenced this in commit
c00938c333
on Jun 25, 2025
ryanofsky referenced this in commit
72ca2472aa
on Jun 25, 2025
ryanofsky referenced this in commit
060a739269
on Jun 25, 2025
ryanofsky referenced this in commit
4896e7fe51
on Jun 25, 2025
ryanofsky referenced this in commit
76313450c2
on Jun 25, 2025
ryanofsky referenced this in commit
8954cc0377
on Jun 26, 2025
DrahtBot added the label
Needs rebase
on Jul 3, 2025
Sjors force-pushed
on Jul 3, 2025
Sjors
commented at 10:12 am on July 3, 2025:
member
Rebased after trivial conflict with #32290 (it touches ci/test/00_setup_env_i686_multiprocess.sh which this PR renames).
DrahtBot removed the label
Needs rebase
on Jul 3, 2025
ryanofsky referenced this in commit
1e9cf763fb
on Jul 3, 2025
l0rinc referenced this in commit
2b3fa59a85
on Jul 4, 2025
ryanofsky referenced this in commit
91d6e146bb
on Jul 4, 2025
ryanofsky referenced this in commit
4d1bd04326
on Jul 4, 2025
ryanofsky referenced this in commit
cd97905ebc
on Jul 7, 2025
BrandonOdiwuor
commented at 1:59 pm on July 23, 2025:
contributor
In 28af4d727b3eeff3da0a3dfab0260c57fb52111c: This should be squashed into the commit that actually changes the Guix build. Ideally CMake would warn/error here, at least in the Guix build. We don’t want it to continue silently if binaries don’t exist/are missing from these checks.
in
.github/workflows/ci.yml:88
in
969a9472a4outdated
ryanofsky
commented at 1:40 pm on July 24, 2025:
contributor
Code review ACK241e9d14879abbc75a17dba764ef2ad58179db57. Just rebased since last review
DrahtBot requested review from BrandonOdiwuor
on Jul 24, 2025
Sjors force-pushed
on Jul 24, 2025
fanquake
commented at 1:52 pm on July 24, 2025:
member
You could probably split out the renaming, and just get them landed, given that should happen regardless of all the other changes here.
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.
d4f18bb9de
ci: build one depends job without multiprocess3319d5e7bc
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.
0647acb79a
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>
eb5813a3f0
doc: add release note082b416a1b
Sjors force-pushed
on Jul 24, 2025
Sjors
commented at 2:26 pm on July 24, 2025:
member
I went back to having sudo apt-get install in a single line, see #31802 (review).
Squashed the Maintenance.cmake change into the commit that adds these to the guix build, see #31802 (review).
Which renaming do you mean? MULTIPROCESS is replaced with NO_IPC but it’s not really a rename. I could rename MULTIPROCESS to IPC in a separate PR, but I don’t think that would reduce the scope of this PR noticeably.
in
depends/Makefile:42
in
d4f18bb9deoutdated
38@@ -39,7 +39,8 @@ NO_QR ?=
39 NO_WALLET ?=
40 NO_ZMQ ?=
41 NO_USDT ?=
42-MULTIPROCESS ?=
43+# Default NO_IPC value is 1 on Windows
In commit “build: add bitcoin-{node,gui} to Maintenance.cmake” (362e233da72b89723cdb9f4f56342716b60976eb)
Note: Since this PR was opened, full windows support (for both connecting to unix sockets and spawning subprocesses) was implemented in #32387. #32387 is a draft just because I don’t want it to be a review burden while there are other changes like #32345 that I think are more important.
ryanofsky approved
ryanofsky
commented at 3:04 pm on July 24, 2025:
contributor
Code review ACK082b416a1bcbfbbdcafb3a3eb1ae800c93385203. Since last review: squashed commit, updated release notes and reverted ci.yml reformatting.
Maybe it’s just me, but I feel like this PR is more comprehensible now that the squashed depends commit is first. And the new release notes are also much better. (Though I would still like to unify them later with the bitcoin wrapper & libexec notes as suggested #31802 (review)).
I could rename MULTIPROCESS to IPC in a separate PR, but I don’t think that would reduce the scope of this PR noticeably.
Tend to agree, but maybe you could rename MULTIPROCESS to NO_IPC in a separate PR and have it default to 1? But I’m not sure much that would reduce complexity here either. I feel like most of the the complexity in this PR comes from updating the CI jobs, so maybe if we had a separate PR enabling IPC in CI jobs first, and then this PR could just flip the depends and cmake defaults and be really simple. But conceptually it seems nice if release & developer & CI defaults are all in sync, so I tend to like the PR in its current form. But would be happy with any approach.
DrahtBot removed the label
Needs rebase
on Jul 24, 2025
Sjors
commented at 3:22 pm on July 24, 2025:
member
so maybe if we had a separate PR enabling IPC in CI jobs first, and then this PR could just flip the depends
That’s what I did before and people didn’t like that: #30975#pullrequestreview-2761133797
In commit “build: depends makes libmultiprocess by default” (d4f18bb9de76d2caf41b6db05c494b333008a3f5)
Notes to help explain changes:
Variable
Description
$(ipc_packages)
Constant list of packages needed to build with IPC features, defined in depends/packages/packages.mk. Always set regardless of NO_IPC.
$(ipc_packages_)
Same as $(ipc_packages), except empty if NO_IPC is set.
@ipc_packages@
Same as $(ipc_packages_), used in toolchain.cmake.in
Depends and toolchain code checks whether IPC is enabled by checking to see if $(ipc_packages_) and @ipc_packages@ values are non-empty, instead of checking NO_IPC directly.
ryanofsky
commented at 1:08 pm on July 29, 2025:
contributor
I’m a little worried this PR is at risk of missing the next release (#32275), and might be caught in a trap where people don’t want to spend time reviewing the code because the level of conceptual support (discussed in #31756) is unclear, and no one is giving conceptual feedback because there are no ACKs for the code. Having review could be a way out of this.
in
doc/release-notes-31802.md:3
in
082b416a1b
0@@ -0,0 +1,3 @@
1+A new (experimental) Mining interface was introduced to support Stratum v2 or other mining client software, see #31098. When the node is started with `bitcoin node -m -ipcbind=unix` the node will listen on a unix socket for IPC client connections. The `-m` option launches a new binary `bitcoin-node` instead of `bitcoind`.
2+
3+IPC connectivity introduces new dependencies (see [multiprocess.md](multiprocess.md)), which are only included in the `bitcoin-node` and `bitcoin-gui` binaries (not in `bitcoind` and `bitcoin-qt`). Those new binaries are now built by default (controlled by `ENABLE_IPC`). If you don't intend to use IPC, nothing changes and there is no need to use `bitcoin -m`, `bitcoin-node` or `bitcoin-gui`(#31802)
ismaelsadeeq
commented at 1:52 pm on August 1, 2025:
nitty-nit: when you click the link to see the multiprocess docs on github on “display rich diff” it is broken, however this is trivial and will go away once merged.
in
doc/release-notes-31802.md:1
in
082b416a1b
0@@ -0,0 +1,3 @@
1+A new (experimental) Mining interface was introduced to support Stratum v2 or other mining client software, see #31098. When the node is started with `bitcoin node -m -ipcbind=unix` the node will listen on a unix socket for IPC client connections. The `-m` option launches a new binary `bitcoin-node` instead of `bitcoind`.
ismaelsadeeq
commented at 1:57 pm on August 1, 2025:
Nit: perhaps just mention that we have two new binaries and then begin the note as done currently..
Tiny note mining interface addition is not done in this PR however still mentioning it in this release is okay.
It might be helpful to clarify what we mean by “experimental” in this context. i.e I will prefer us being explicit for example, we could mention that the binary has undergone rounds of review and testing by a group of core contributors, but since this is its first release, users should still proceed with caution.
Personally, I’m fully on board with labeling reviewed and tested binaries as “experimental” for initial releases. If things go well over the next few release cycle, the label can be dropped.
This should not necessarily be done in this PR, the release notes can be edited later.
ismaelsadeeq
commented at 2:13 pm on August 1, 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-08-08 09:13 UTC
This site is hosted by @0xB10C More mirrored repositories can be found on mirror.b10c.me
