Add bitcoin-{node,gui} to release binaries for IPC #31802

pull Sjors wants to merge 7 commits into bitcoin:master from Sjors:2025/02/ipc-yea changing 23 files +152 −111
  1. Sjors commented at 4:04 pm on February 5, 2025: member

    Have depends make libmultiprocess by default. This PR causes the following behavior changes:

    1. bitcoin-node and bitcoin-gui binaries are included in releases, due to ENABLE_IPC option being switched on by default in depends builds
    2. ENABLE_IPC is also switched on by default in non-depends builds
    3. 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.
    4. The bitcoin-node and bitcoin-gui are added to Maintenance.cmake (since they’re now in the release)
    5. 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...
    
  2. 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.

    Code Coverage & Benchmarks

    For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31802.

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    Concept ACK TheCharlatan
    Stale ACK ryanofsky

    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.

  3. DrahtBot added the label Needs rebase on Feb 6, 2025
  4. Sjors force-pushed on Feb 7, 2025
  5. Sjors force-pushed on Feb 7, 2025
  6. 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.
    
  7. Sjors commented at 10:58 am on February 8, 2025: member
    @sipa I opened #31827
  8. onlinesipahimithu commented at 1:26 am on February 9, 2025: none
    GOOD lesson from
  9. bitcoin deleted a comment on Feb 9, 2025
  10. Sjors force-pushed on Feb 10, 2025
  11. Sjors force-pushed on Feb 13, 2025
  12. DrahtBot removed the label Needs rebase on Feb 13, 2025
  13. Sjors force-pushed on Feb 17, 2025
  14. Sjors force-pushed on Feb 17, 2025
  15. DrahtBot added the label Needs rebase on Feb 20, 2025
  16. ryanofsky referenced this in commit 83e40d3b52 on Feb 24, 2025
  17. ryanofsky referenced this in commit 8619f03ec2 on Feb 24, 2025
  18. ryanofsky referenced this in commit cbb7b41c20 on Feb 24, 2025
  19. fanquake referenced this in commit 01f7715766 on Feb 25, 2025
  20. fanquake referenced this in commit ba0a4391ff on Feb 25, 2025
  21. pablomartin4btc referenced this in commit bd2453d134 on Feb 26, 2025
  22. pablomartin4btc referenced this in commit 75d5d235a6 on Feb 26, 2025
  23. Sjors force-pushed on Feb 27, 2025
  24. DrahtBot removed the label Needs rebase on Feb 27, 2025
  25. ngn999 referenced this in commit fd12bfb763 on Feb 27, 2025
  26. DrahtBot added the label Needs rebase on Mar 5, 2025
  27. Sjors force-pushed on Mar 11, 2025
  28. DrahtBot removed the label Needs rebase on Mar 11, 2025
  29. DrahtBot added the label Needs rebase on Mar 12, 2025
  30. Sjors force-pushed on Mar 12, 2025
  31. DrahtBot removed the label Needs rebase on Mar 12, 2025
  32. DrahtBot added the label Needs rebase on Mar 14, 2025
  33. Sjors force-pushed on Mar 17, 2025
  34. DrahtBot removed the label Needs rebase on Mar 17, 2025
  35. DrahtBot added the label Needs rebase on Mar 18, 2025
  36. Sjors force-pushed on Mar 18, 2025
  37. DrahtBot removed the label Needs rebase on Mar 18, 2025
  38. DrahtBot added the label Needs rebase on Mar 29, 2025
  39. Sjors force-pushed on Mar 31, 2025
  40. DrahtBot removed the label Needs rebase on Mar 31, 2025
  41. Sjors force-pushed on Mar 31, 2025
  42. DrahtBot added the label Needs rebase on Apr 2, 2025
  43. Sjors force-pushed on Apr 2, 2025
  44. DrahtBot removed the label Needs rebase on Apr 2, 2025
  45. DrahtBot added the label Needs rebase on Apr 4, 2025
  46. 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.
  47. 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

  48. Sjors force-pushed on Apr 11, 2025
  49. DrahtBot removed the label Needs rebase on Apr 11, 2025
  50. Sjors marked this as ready for review on Apr 11, 2025
  51. Sjors commented at 7:32 pm on April 11, 2025: member
    Marked this as ready for review, but see discussion here: #30975 (comment)
  52. ryanofsky commented at 8:49 pm on April 11, 2025: contributor

    Code review 1ed02d5bf4a4bcf58cad0addaaaf9c171e29d8c6

    • 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
  53. Sjors force-pushed on Apr 11, 2025
  54. 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.

  55. Sjors force-pushed on Apr 11, 2025
  56. DrahtBot added the label Needs rebase on Apr 14, 2025
  57. Sjors force-pushed on Apr 15, 2025
  58. DrahtBot removed the label Needs rebase on Apr 15, 2025
  59. in depends/README.md:121 in 9b46b7d582 outdated
    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)
    


    ryanofsky commented at 5:17 pm on April 15, 2025:

    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.


    Sjors commented at 12:42 pm on April 17, 2025:
    Expanded the comment and commit description.
  60. ryanofsky approved
  61. ryanofsky commented at 5:35 pm on April 15, 2025: contributor

    Code review ACK 5d96e656d72abee468f5f49759b1b08eab445d4d. 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.


    re: #31802 (comment)

    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.

  62. DrahtBot added the label Needs rebase on Apr 17, 2025
  63. Sjors force-pushed on Apr 17, 2025
  64. 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)
  65. Sjors force-pushed on Apr 17, 2025
  66. DrahtBot added the label CI failed on Apr 17, 2025
  67. in doc/multiprocess.md:19 in 341b2a99e6 outdated
    15@@ -16,11 +16,11 @@ Specifying `-DENABLE_IPC=ON` requires [Cap'n Proto](https://capnproto.org/) to b
    16 
    17 ### Depends installation
    18 
    19-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:
    


    DrahtBot commented at 2:02 pm on April 17, 2025:

    (✨ LLM generated, experimental)

    Alternatively the depends system can be used to avoid needing to local install dependencies: —> …avoid needing to install local dependencies:

  68. DrahtBot removed the label Needs rebase on Apr 17, 2025
  69. DrahtBot removed the label CI failed on Apr 17, 2025
  70. 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.
  71. ryanofsky approved
  72. ryanofsky commented at 6:12 pm on April 21, 2025: contributor

    Code review ACK 341b2a99e646246a9e4d3aa283b2b59442cfad27. 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.

  73. Sjors force-pushed on Apr 23, 2025
  74. 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.
  75. in doc/dependencies.md:39 in ce415e06b1 outdated
    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 |
    


    fanquake commented at 8:56 am on April 23, 2025:
    0| [Cap'n Proto](../depends/packages/capnp.mk) | [link](https://capnproto.org) | [0.6.1](https://capnproto.org/install.html) | 0.5.3 | No |
    

    Sjors commented at 9:54 am on April 23, 2025:
    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.

  76. Sjors force-pushed on Apr 23, 2025
  77. in doc/build-osx.md:132 in 1eedff84d7 outdated
    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
    


    maflcko commented at 9:58 am on April 23, 2025:
    depedency → dependency
  78. 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.
  79. Sjors force-pushed on Apr 23, 2025
  80. 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.

  81. DrahtBot added the label Needs rebase on May 6, 2025
  82. Sjors force-pushed on May 7, 2025
  83. Sjors commented at 9:04 am on May 7, 2025: member
    Rebased after #32086.
  84. DrahtBot removed the label Needs rebase on May 7, 2025
  85. DrahtBot added the label CI failed on May 7, 2025
  86. 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.

  87. Sjors commented at 11:34 am on May 7, 2025: member

    @ryanofsky CI complains about std::move in libmultiprocess: https://cirrus-ci.com/task/6187773452877824, e.g.:

    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>
    
  88. DrahtBot added the label Needs rebase on May 7, 2025
  89. Sjors commented at 2:52 pm on May 7, 2025: member
    Rebased after #28710.
  90. Sjors force-pushed on May 7, 2025
  91. DrahtBot removed the label Needs rebase on May 7, 2025
  92. ryanofsky referenced this in commit 28632dc782 on May 12, 2025
  93. ryanofsky commented at 6:10 pm on May 12, 2025: contributor

    re: Sjors #31802 (comment)

    @ryanofsky CI complains about std::move in libmultiprocess: https://cirrus-ci.com/task/6187773452877824, e.g.:

    Thanks for catching this. I implemented a fix for that error and the other errors in https://github.com/bitcoin-core/libmultiprocess/pull/172

    If you wanted to test the fix here you could do:

    0git fetch https://github.com/bitcoin-core/libmultiprocess 8d8e9f65f1d9faaafe91ca90910c1f96da9d094d
    1git subtree merge --prefix=src/ipc/libmultiprocess --squash 8d8e9f65f1d9faaafe91ca90910c1f96da9d094d
    

    I’ll also try to open a subtree update PR in the next week or so that this PR could be rebased on.

  94. Sjors force-pushed on May 13, 2025
  95. Sjors commented at 9:03 am on May 13, 2025: member
    @ryanofsky I added your libmultiprocess patch (and rebased).
  96. DrahtBot removed the label CI failed on May 13, 2025
  97. DrahtBot added the label Needs rebase on May 15, 2025
  98. Sjors force-pushed on May 15, 2025
  99. Sjors commented at 4:52 pm on May 15, 2025: member
    Rebased after #31895. Also marking draft given the dependency on https://github.com/bitcoin-core/libmultiprocess/pull/172.
  100. Sjors marked this as a draft on May 15, 2025
  101. DrahtBot removed the label Needs rebase on May 15, 2025
  102. ryanofsky referenced this in commit c1e8c1a028 on May 15, 2025
  103. ryanofsky referenced this in commit 867a3fa894 on May 15, 2025
  104. Sjors force-pushed on May 16, 2025
  105. DrahtBot added the label CI failed on May 16, 2025
  106. DrahtBot removed the label CI failed on May 16, 2025
  107. DrahtBot added the label Needs rebase on May 22, 2025
  108. build: add bitcoin-{node,gui} to Maintenance.cmake acd7cf80b5
  109. 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
  110. ci: build one depends job without multiprocess 4ce6310dbb
  111. Sjors force-pushed on May 28, 2025
  112. 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
  113. 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
  114. Sjors force-pushed on May 28, 2025
  115. Sjors commented at 8:11 am on May 28, 2025: member
  116. DrahtBot added the label CI failed on May 28, 2025
  117. Squashed 'src/ipc/libmultiprocess/' changes from 35944ffd23..57a65b8546
    57a65b8546 clang-tidy: Suppress bitcoin-nontrivial-threadlocal error
    3a96cdc18f clang-tidy: Fix bugprone-move-forwarding-reference error
    c1e8c1a028 clang-tidy: Fix bugprone-move-forwarding-reference errors
    0d8012f656 Merge chaincodelabs/libmultiprocess#165: clang-tidy: fix warnings introduced in version 19
    aa19285303 use ranges transform
    a78137ca73 make member function const
    ca3226ec8a replace custom tuple unpacking code with `std::apply`
    949fe85fc9 replace SFINAE trick with `if constexpr`
    
    git-subtree-dir: src/ipc/libmultiprocess
    git-subtree-split: 57a65b854664c13c9801788eeaf71bdb3e597c81
    88fe80ab1b
  118. Merge commit '88fe80ab1bf8a85adb532e6fced51f5e28b173d8' into 2025/02/ipc-yea 04fdcc5a26
  119. Sjors force-pushed on May 28, 2025
  120. DrahtBot removed the label Needs rebase on May 28, 2025
  121. DrahtBot removed the label CI failed on May 28, 2025

github-metadata-mirror

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