-
Notifications
You must be signed in to change notification settings - Fork 37.7k
Add bitcoin-{node,gui} to release binaries for IPC #31802
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
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/31802. 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. |
ef1e5b0
to
527a77e
Compare
Some chatter from IRC:
|
f43ff56
to
63dffe1
Compare
63dffe1
to
1cb3ff9
Compare
Thanks to all who reviewed, tested, and commented on this. This has been very useful and should lead to a lot of good followups. I think all concerns can be addressed in future PRs even if they have not been addressed here and I think there are clear next steps to address all the issues listed in the review summary |
Post-merge ACK ce7d94a |
🎉 thanks for all the reviews! I'll stay on top of followup PR's over the next few weeks. |
OSS-Fuzz is now failing, as explained above in #31802 (comment) (with possible fixes). https://oss-fuzz-build-logs.storage.googleapis.com/log-66a59cf3-d2f6-4bf1-b4e1-0a5ef44fbaaf.txt
I am still waiting for the oss-fuzz maintainers to give the ok for google/oss-fuzz#13018, so I guess in the meantime one of the above mentioned workarounds could be used. |
@@ -35,6 +35,7 @@ Bitcoin Core requires one of the following compilers. | |||
|
|||
| Dependency | Releases | Minimum required | | |||
| --- | --- | --- | | |||
| [Cap'n Proto](../depends/packages/capnp.mk) | [link](https://capnproto.org) | [0.7.1](https://capnproto.org/install.html) | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
my understanding is that this should refer to the pull introducing the min required, which would be [0.7.1](https://github.com/bitcoin/bitcoin/pull/28907)
via bitcoin-core/libmultiprocess@414542f
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in #33233
@fanquake @maflcko I opened a PR to temporarily disable IPC build for oss-fuzz, please take a look: google/oss-fuzz#13854 |
bitcoin/bitcoin#31802 builds libmultiprocess and enables IPC by default, but my understanding is that this breaks oss-fuzz. One proposed solution is #13018. This PR builds depends with `NO_IPC=1`, which disables the feature, for now while we work on an actual fix.
…inaries for IPC" de65c86 doc: capnproto instruction for Alpine and Arch (Sjors Provoost) 49d1a1a doc: add capnproto-devel to Fedora build instruction (Sjors Provoost) eab5518 doc: mark bitcoin-{node,gui} installed in files.md (Sjors Provoost) 2a815d1 doc: link to capnp version bump PR (Sjors Provoost) Pull request description: - have `dependencies.md` link to the PR that updated the capnp version: #31802 (comment) ACKs for top commit: maflcko: lgtm ACK de65c86 janb84: re ACK de65c86 Tree-SHA512: 842d7a89ef18a8c597ca05720c41a72e67500bc93430cf2c3b074cb2f4b936f1df58b5b1e99010e1ea5c1f8a9f8875fb9c20398f915feeacecee9b2fed3cb03c
Required since bitcoin/bitcoin#31802 was merged.
Required since bitcoin/bitcoin#31802 was merged.
Required since bitcoin/bitcoin#31802 was merged.
Required since bitcoin/bitcoin#31802 was merged.
Required since bitcoin/bitcoin#31802 was merged.
Required since bitcoin/bitcoin#31802 was merged.
Required since bitcoin/bitcoin#31802 was merged.
The sidecar Template Provider application now has its own repo: https://github.com/Sjors/sv2-tp (release coming soon) I suspect that the SRI team will have a Rust alternative soon(tm) as well. |
Have depends make libmultiprocess by default. This PR causes the following behavior changes:
ENABLE_IPC
option being switched on by default in depends buildsENABLE_IPC
is also switched on by default in non-depends builds (instructions updated, build: Enable ENABLE_IPC option by default #33190 does this as a standalone PR)ENABLE_IPC
on in most configurations and usingbitcoin-node
binary (bitcoin -m
) for functional tests in two of them.bitcoin-node
andbitcoin-gui
are added toMaintenance.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 has been around for a few years now, mostly stable but with ongoing activity 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).
Braiins 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 that supports Stratum v2 without the need for custom firmware.
DMND pool 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.
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 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, including making it a subtree in #31741. See 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, mostly after bug fixes or major rebases. The SRI team has been testing both variants, though the "official" instruction on their web page is to stick to integrated version. Bug reports on my repo fork as well as on the SRI repo are evidence of actual testing happening.
But as Pavlenex writes below:
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 usualbitcoind
.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;
bitcoind
(or a forked version ofbitcoin-node
, plusbitcoin-mine
), install SRI stuffbitcoin-mine
and SRIGuix hashes: