build: Enable ENABLE_IPC option by default #33190
Conversation
OpenBSD does not have this package, so recommend building from source for now.
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code Coverage & BenchmarksFor details see: https://corecheck.dev/bitcoin/bitcoin/pulls/33190. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
|
Concept ACK |
d657ada to
732c134
Compare
|
I want to elaborate on why I prefer #31802 to this change. The idea of enabling IPC in source builds before binary releases sounds nice at first, but more thought, stops really making sense. For one thing, this is not how we've rolled out any other features I'm aware of. Most features aren't toggled by build flags, but theoretically any new feature could be gated by a build flag, enabled first in source builds, and later in binary releases. We don't roll out features this way because it's cumbersome and offers no clear benefits. I don't think IPC is different from other features in this respect. Even looking at the last feature that was toggled by a build flag, SQLite, there was never a point where it was enabled in source builds but disabled in depends and binary releases (see #19077). I think some people like the idea of turning IPC on in source builds but leaving it off in depends and binary releases because they think it would lead to more feature testing before inclusion in a binary release. But IPC has already been enabled in the "dev-mode" CMake preset for as long as that preset has existed. And since the only real functionality added by ENABLE_IPC is the third-party mining interface, I doubt changing its default will meaningfully increase testing, unless there's a user or developer who (1) is willing to write or download a program that connects to the interface, (2) builds Bitcoin Core from source with default CMake options, but (3) is unwilling to manually enable ENABLE_IPC. That seems like a narrow group. The only possible benefit I see from changing the default to ON in source builds is that it would confirm IPC builds and tests work on more platforms and with a wider variety of build options beyond what CI covers. That has some value, but I don't see build flag compatibility or platform-specific bugs as major concerns, so putting the feature in a half-enabled state just to get a few more builds and automated tests doesn't seem appealing. I have no objection to this change if it helps us move forward. I just want to explain why I don't think it's very useful on its own, and why I dropped it from #31741 after initially implementing it there. |
|
ACK 732c134
While I also prefer #31802 (and largely agree with everything you said in the rest of your comment), I think there is one additional benefit, namely signalling progress to external projects. I do think this PR would be most impactful if it also included a release note for v30 to inform users this is now on by default when compiling from source, with the expectation it will be fully turned on in the v31 release. |
Yes that's a great point, and reminds me that even without this PR or #33190 we could add a release note saying that in 30.0 the Mining interface has new |
You'll also need to update the valgrind and valgrind fuzz CI jobs (run in qa-assets / elsewhere), otherwise they wont work, after this is merged. |
732c134 to
5a34156
Compare
|
Updated 732c134 -> 5a34156 ( |
|
re: #33190 (comment)
@sipa if you are saying you see this PR as a prerequisite to #31802, that seems ok, but I would be interested to know why. As far as I can tell, there haven't been any other features in bitcoin core enabled by default in non-depends builds but disabled by default by default in depends builds and releases, so it seems like this PR leaves the build configuration in a strange state without a justification for what it is trying to achieve. I could imagine it making sense to enable a feature in ordinary source builds that is disabled in depends builds if the feature changed default behavior of the node or wallet, and we wanted to opt developers into the change before rolling it out more widely. But the IPC feature features are off by default so flipping this switch isn't opting anyone into actually using IPC features. All it is doing is compiling some extra files and enabling some extra automated tests in the non-depends build. Which is nice, but does not seem like a compelling reason to leave this feature in an unusual half-enabled state. Basically the more I think about this PR the less it seems to make sense to me, and I think I'd like to close it, but no problem keeping it open if others think this is a good idea. |
|
Thinking on this further, I think I agree that there isn't a good reason to be making this change. When building from source, we already don't enable several optional features, including the GUI, that we ship in the release binaries. Since the switch to cmake, having the dependencies installed doesn't enable those features automatically, they still need to be enabled explicitly during configuration. Developers should probably be using the dev-mode preset, or similar, which enables a bunch of the optional features so this shouldn't make a difference to them. Since this PR defaults the option to on, it isn't really an optional feature anymore. It can be turned off, but if you just do the default configuration, you will need to have capnproto installed. |
There was a problem hiding this comment.
I still don't understand the value of enabling these binaries in the build tree without also including them in release artifact compared to #31802
|
Thanks everybody who commented! Will close this in favor of #31802. If anybody thinks this should be reopened, I'd be curious and can reopen it. But balance of opinion seems to be in favor of not doing this. |
|
Note: achow101 commented against enabling ENABLE_IPC by default in #31802 (review) and sipa commented in favor in #31802 (comment), so more discussion about this is happening in the other PR. I think the difference comes down to short vs. long term perspective. In the short term it doesn't make a huge amount of sense to turn this one feature on by default while comparable features are turned off. But in the longer term it does not make sense to support and maintain the IPC feature at all if it is not used for more things and turned on by default. |
ce7d94a doc: add release note (Sjors Provoost) 71f29d4 doc: update build and dependencies docs for IPC (Sjors Provoost) 3cbf747 cmake: set ENABLE_IPC by default (Sjors Provoost) 32a90e1 ci: use bitcoin-node for one depends job (Sjors Provoost) b333cc1 ci: build one depends job without multiprocess (Sjors Provoost) 16bce9a build: depends makes libmultiprocess by default (Sjors Provoost) Pull request description: 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 (instructions updated, #33190 does this as a standalone PR) 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) This PR doesn't need to do all of these 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: Sjors#48. See #31756 for discussion of when this should happen. Supersedes #30975. ## Wait what, why? The [Stratum v2 spec](https://stratumprotocol.org/specification) has been around for a few years now, mostly stable but with [ongoing activity](https://github.com/stratum-mining/sv2-spec/commits/main/) to clarify and fix more subtle issues encountered by implementers. Most of the implementation is built in Rust in a project called the Stratum Reference Implementation ([SRI](https://github.com/stratum-mining/stratum)). [Braiins](https://demand.work) added Stratum v2 support to both their (custom) firmware and pool several years ago, though they have fallen behind on recent spec changes (update: it seems they've fixed that). Apparently [new hardware is underway](#31802 (comment)) that supports Stratum v2 without the need for custom firmware. [DMND pool](https://www.dmnd.work) is Stratum v2 native from the start and employs several of the SRI developers (they haven't fully launched though). The industry is rather secretive, but apparently [there is more underway](#31802 (comment)). What does Bitcoin Core have to do with this? Well, in Stratum v2 jargon we are the Template Provider. Or at least, the Template Provider role needs us to make block templates. Initially back in 2023 the plan was to have Bitcoin Core implement this role entirely, see #23049. It would speak the sv2 encrypted message protocol. In fact the spec was designed around this assumption, making sure to only use cryptographic primitives already in our codebase. I took over that effort in late 2023, but during 2024 it became quite clear there was [strong resistance](#29432 (review)) to the idea of including all this new code, opening another network ports, etc. At the same time there was the long running multiprocess / IPC project #10102, and the idea was born to apply that here: instead of including Stratum v2 specific stuff, we offer a general Mining interface via an IPC connection that can e.g. push out fresh block templates as fees rise above a threshold (something not possible and/or very inefficient with `getblocktemplate`). A client sidecar application then sits between the Stratum v2 world and our node. Currently there's only one such sidecar application, maintained by me, and reusing the same codebase from the integrated approach. An attempt has been made to connect to our interface from Rust bitcoin-core/libmultiprocess#174, which would pave the way for SRI include the Template Provider role. Plebhash below indicates he's also working on that: #31802 (comment). So with this new approach in mind, between mid 2024 until spring 2025, I introduced a new Mining interface (#30200 - #31785). At the same time Russ Ryanosky worked on more tight integration of [libmultiprocess](https://github.com/bitcoin-core/libmultiprocess), including making it a subtree in #31741. See [design/multiprocess.md](https://github.com/bitcoin/bitcoin/blob/master/doc/design/multiprocess.md). Meanwhile I've been maintaining a fork of Bitcoin Core that includes the Template Provider, in the original integrated approach (Sjors#68) as well as an IPC + sidecar variant (Sjors#48). I've been shipping [regular releases](https://github.com/Sjors/bitcoin/releases), mostly after bug fixes or major rebases. The SRI team has been testing both variants, though the "official" [instruction on their web page](https://stratumprotocol.org/developers) is to stick to integrated version. Bug reports on [my repo fork](https://github.com/Sjors/bitcoin/issues?q=is%3Aissue) as well as on the [SRI repo](https://github.com/stratum-mining/stratum/issues?q=is%3Aissue%20%20label%3A%22template%20provider%22) are evidence of actual testing happening. But as Pavlenex writes below: > one recurring feedback I kept getting regardless of the size/type of miner is that the need to run a forked version of Bitcoin Core remains a significant barrier to adoption This PR gets rids of that significant barrier. People can download a "pristine" version of Bitcoin Core and the only change is to start it with `bitcoin node -m -ipcconnect=unix` instead of the usual `bitcoind`. Once that's released, I can dramatically simplify my sidecar codebase (Sjors#48) by removing pretty much all Bitcoin Core code that it doesn't need. My plan is to then make that a separate repository, which should be much easier to contribute to. I can then also make (deterministically built) signed releases, while making it clear that sidecar code has nothing to do with Bitcoin Core. Perhaps later on SRI implements the same and I can stop maintaining that project. Conceptually the situation will be a lot clearer; - today: download forked version of `bitcoind` (or a forked version of `bitcoin-node`, plus `bitcoin-mine`), install SRI stuff - tomorrow: download Bitcoin Core v30, install `bitcoin-mine` and SRI - future: download Bitcoin Core v30 and SRI <details> <summary> Guix hashes: </summary> ``` find guix-build-$(git rev-parse --short=12 HEAD)/output/ -type f -print0 | env LC_ALL=C sort -z | xargs -r0 sha256sum 6dbf29baecb1d1593087ef1306ae7c78aa160c8beb04dc016e02549ae2d6d90d guix-build-ce7d94a492e6/output/aarch64-linux-gnu/SHA256SUMS.part 4b465e5e8f9652c176aa57cfe5c289267c28c3a3c684034a9ce471b529b95275 guix-build-ce7d94a492e6/output/aarch64-linux-gnu/bitcoin-ce7d94a492e6-aarch64-linux-gnu-debug.tar.gz 85bc6fa008b83419d96443d9dcc212b46f0992387fd58fd2dda5da76536ee22c guix-build-ce7d94a492e6/output/aarch64-linux-gnu/bitcoin-ce7d94a492e6-aarch64-linux-gnu.tar.gz 5ed9ea52a8bd55361d2d9c01fbd1b25ec9970530c2776e6c1959424ba1689f52 guix-build-ce7d94a492e6/output/arm-linux-gnueabihf/SHA256SUMS.part 2e483011fac64462d3aa000b577c3c05c825506032d879e39612e096d7a6c65b guix-build-ce7d94a492e6/output/arm-linux-gnueabihf/bitcoin-ce7d94a492e6-arm-linux-gnueabihf-debug.tar.gz 7ff1e3ba54944a2be89dd7d68cb91dff6f8950de9d7b521e15dfb746965f81bd guix-build-ce7d94a492e6/output/arm-linux-gnueabihf/bitcoin-ce7d94a492e6-arm-linux-gnueabihf.tar.gz abdf89e701b21b8c1238a8cec46aeaa55e0c3a0b88ad718636e89cde9813ca08 guix-build-ce7d94a492e6/output/arm64-apple-darwin/SHA256SUMS.part fb55cff0296cd5474811fe5cedcf28603628729dd085eeefa007c72582459b33 guix-build-ce7d94a492e6/output/arm64-apple-darwin/bitcoin-ce7d94a492e6-arm64-apple-darwin-codesigning.tar.gz e9aa566b1e79c467d7987b7c68fa608db788e6ddf89c4d90e524cd47b4faaf86 guix-build-ce7d94a492e6/output/arm64-apple-darwin/bitcoin-ce7d94a492e6-arm64-apple-darwin-unsigned.tar.gz bb428fc62a1230a55f49fa3b5c7ba8d588e8fed491357f890d5a6724a38b14e9 guix-build-ce7d94a492e6/output/arm64-apple-darwin/bitcoin-ce7d94a492e6-arm64-apple-darwin-unsigned.zip 5ef4b75e94b2c8265fbc588bbb42467a84438af969fddac0ea61ced3e4113345 guix-build-ce7d94a492e6/output/dist-archive/bitcoin-ce7d94a492e6.tar.gz 4f55d56a108c8f312a502cd5dfdf0840b091861a6d502df31caf4636a203697a guix-build-ce7d94a492e6/output/powerpc64-linux-gnu/SHA256SUMS.part 66c5b1242c60e37098885a00e24efe19baee4afcd2e3d6407207523d8872f055 guix-build-ce7d94a492e6/output/powerpc64-linux-gnu/bitcoin-ce7d94a492e6-powerpc64-linux-gnu-debug.tar.gz d9dbbee7217544eda26e77158cd82caeaef2b40fb9fc7033323e7ffe64264109 guix-build-ce7d94a492e6/output/powerpc64-linux-gnu/bitcoin-ce7d94a492e6-powerpc64-linux-gnu.tar.gz d9b808cc5685c819abcebb4ace65f003ebc4bfedf3fca046b34de37994358782 guix-build-ce7d94a492e6/output/riscv64-linux-gnu/SHA256SUMS.part eeeea470b1cf76515bfae14c779a3ea356d89f719d1fef1a81e8f0d6b04ab747 guix-build-ce7d94a492e6/output/riscv64-linux-gnu/bitcoin-ce7d94a492e6-riscv64-linux-gnu-debug.tar.gz 9993da4eb51618b8bd25ec88cc576496720e5589315e9eba6f3ddab25f9c3e60 guix-build-ce7d94a492e6/output/riscv64-linux-gnu/bitcoin-ce7d94a492e6-riscv64-linux-gnu.tar.gz 1b5a676580e0e79598d182f6ebbb05fb8aee2381edc3c09c042cae2600f448ab guix-build-ce7d94a492e6/output/x86_64-apple-darwin/SHA256SUMS.part 9152122d95a34d5df75305c6883c87707e7b09033fffd08e264d703ed177ef12 guix-build-ce7d94a492e6/output/x86_64-apple-darwin/bitcoin-ce7d94a492e6-x86_64-apple-darwin-codesigning.tar.gz 2793f75730dbef6bdf12b5ed7135e22ed21178abff2926dee92843837d4ab544 guix-build-ce7d94a492e6/output/x86_64-apple-darwin/bitcoin-ce7d94a492e6-x86_64-apple-darwin-unsigned.tar.gz e89aafd7e4a330a41f470e8f0a91ea876fad7d19547b404600867413f1a8ccb7 guix-build-ce7d94a492e6/output/x86_64-apple-darwin/bitcoin-ce7d94a492e6-x86_64-apple-darwin-unsigned.zip 955b27f881927a86da3c566357ad8ca68dbe17e9652bde8c482a57ceacba92cb guix-build-ce7d94a492e6/output/x86_64-linux-gnu/SHA256SUMS.part fd012be97bdf5c75ac12ddef21526bfdb5e17ecc77cde9c34d832194b0dc3293 guix-build-ce7d94a492e6/output/x86_64-linux-gnu/bitcoin-ce7d94a492e6-x86_64-linux-gnu-debug.tar.gz 0ecf7f80e9049369760d0e27fe6c026391ab25eae0f42336bef43e51a2621726 guix-build-ce7d94a492e6/output/x86_64-linux-gnu/bitcoin-ce7d94a492e6-x86_64-linux-gnu.tar.gz 2e8085f5fecc246d841b0bf6f28ecd0684a6cee49252fc88c1019d7586c7b7a2 guix-build-ce7d94a492e6/output/x86_64-w64-mingw32/SHA256SUMS.part c60041e8137eda352557254c5f67fb83eeb97ecfec342ee528451bd44ee4523a guix-build-ce7d94a492e6/output/x86_64-w64-mingw32/bitcoin-ce7d94a492e6-win64-codesigning.tar.gz b1be6b2f4de1c69c2e0e4de6dd97a4891ae9eb50d89435ef47247b5a187915a9 guix-build-ce7d94a492e6/output/x86_64-w64-mingw32/bitcoin-ce7d94a492e6-win64-debug.zip bfe143f41a20c537145c7044aca889b28efe19072b0150042a3bd865983b3d7e guix-build-ce7d94a492e6/output/x86_64-w64-mingw32/bitcoin-ce7d94a492e6-win64-setup-unsigned.exe 94a906b83d84db7b25f7e3cfdce2a2030243f2ee6cc70b1fc088459f0b2f382d guix-build-ce7d94a492e6/output/x86_64-w64-mingw32/bitcoin-ce7d94a492e6-win64-unsigned.zip ``` </details> ACKs for top commit: ryanofsky: Code review ACK ce7d94a. This was just rebased to fix a conflict since last review. josibake: ACK ce7d94a achow101: ACK ce7d94a ismaelsadeeq: ACK ce7d94a and tested again on macOS by building via depends and source. janb84: ACK ce7d94a Tree-SHA512: f7ab72933854e9dfce5746cdf764944bc26eec815f97cd0aa6b54fa499c3cccb1b678861ef5c1c793de28153d46bbb6e4d1b9aa0652163b74262e2d55ec8b813
This change toggles
-DENABLE_IPCdefault value fromOFFtoONso IPC features will be compiled by default in source builds.The main feature this provides is an
-ipcbindoption that lets the node listen on a unix socket and expose a mining interface to support Stratum v2 mining software.This change doesn't affect
bitcoindorbitcoin-qtsince IPC features are implemented in separate binaries accessible through a newbitcoincommand.This PR is a minimal change that just enables IPC by default in cmake. #31802 is more comprehensive and additionally enables IPC by default in depends, uses it in more CI jobs, and updates documentation. If you like this PR, you will love #31802!
I prefer the approach in #31802 of enabling IPC in source builds, release binaries, and CI at the same time, without an intermediate state where source and binary releases are different. But there was a comment in todays irc meeting "i think it'd be nice if we had IPC enabled in from-source builds by default for a while before we add it to releases" and there have been similar comments previously, so maybe this PR will be of interest if we want to take that approach.
This PR is part of the process separation project.