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:
#32888 (ci: Use optimized Debug build type in test-each-commit by maflcko)
#32162 (depends: Switch from multilib to platform-specific toolchains by hebasto)
#31349 (ci: detect outbound internet traffic generated while running tests by vasild)
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
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.
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
build: add bitcoin-{node,gui} to Maintenance.cmake28af4d727b
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.
827b2ef843
ci: build one depends job without multiprocess0b20ea2166
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.
6fe3c78d1e
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>
7213994675
doc: add release note1d9ba616c9
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
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-07-11 09:13 UTC
This site is hosted by @0xB10C More mirrored repositories can be found on mirror.b10c.me
