Skip to content

Conversation

Sjors
Copy link
Member

@Sjors Sjors commented May 30, 2024

Introduce a Mining interface for the getblocktemplate, generateblock and other mining RPCs to use now, and for Stratum v2 to use later.

Suggested here: #29346 (comment)

The selection of methods added to the interface is mostly based on what the Template Provider in #29432 uses. It could be expanded further so that rpc/mining.cpp no longer needs EnsureMemPool and EnsureChainman.

This PR should be a pure refactor.

@DrahtBot
Copy link
Contributor

DrahtBot commented May 30, 2024

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type Reviewers
ACK tdb3, itornaza, ryanofsky
Concept ACK TheCharlatan, ismaelsadeeq

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:

  • #30214 (refactor: Improve assumeutxo state representation by ryanofsky)
  • #29700 (kernel, refactor: return error status on all fatal errors by ryanofsky)
  • #28843 ([refactor] Remove BlockAssembler m_mempool member by TheCharlatan)
  • #10102 (Multiprocess bitcoin by ryanofsky)

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.

@Sjors
Copy link
Member Author

Sjors commented May 30, 2024

The first commit is all the boilerplate plus a single method isTestChain() that's used by the getblocktemplate RPC.

@Sjors Sjors force-pushed the 2024/05/miner-interface branch from dc72400 to 87ba70a Compare May 30, 2024 15:08
@Sjors Sjors marked this pull request as ready for review May 30, 2024 15:10
@Sjors
Copy link
Member Author

Sjors commented May 30, 2024

Let me know if I should expand the interface to cover more of rpc/mining.cpp or stick to this for now.

{
LOCK(::cs_main);
// TODO pass mempool sanely
return BlockAssembler{chainman().ActiveChainstate(), &*(context()->mempool)}.CreateNewBlock(scriptPubKeyIn);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

f04cf9f: I guess I should change the BlockAssembler constructor

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

re: #30200 (comment)

I guess I should change the BlockAssembler constructor

In commit "rpc: call CreateNewBlock via miner interface" (f04cf9f)

I don't actually understand the TODO here. The way the mempool is passed seems ok. You could do mempool.get() instead of &*mempool, but both should be equivalent.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, replaced it with get().

@TheCharlatan
Copy link
Contributor

Concept ACK

Copy link
Contributor

@ryanofsky ryanofsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review ACK 87ba70a

re: #30200 (comment)

Let me know if I should expand the interface to cover more of rpc/mining.cpp or stick to this for now.

I think it's just important for the interface to be usable for stratum code so is not too tied to node internals. It should be ok for RPC code to go around the interface and not use it for everything.

{
LOCK(::cs_main);
// TODO pass mempool sanely
return BlockAssembler{chainman().ActiveChainstate(), &*(context()->mempool)}.CreateNewBlock(scriptPubKeyIn);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

re: #30200 (comment)

I guess I should change the BlockAssembler constructor

In commit "rpc: call CreateNewBlock via miner interface" (f04cf9f)

I don't actually understand the TODO here. The way the mempool is passed seems ok. You could do mempool.get() instead of &*mempool, but both should be equivalent.

@DrahtBot DrahtBot requested a review from TheCharlatan June 4, 2024 17:41
@Sjors Sjors force-pushed the 2024/05/miner-interface branch from 87ba70a to d1be135 Compare June 7, 2024 09:55
@Sjors
Copy link
Member Author

Sjors commented Jun 7, 2024

@ryanofsky thanks for the review! I rebased and changed a few things to address comments, see inline.

I'll cherry-pick this into #29432 to see if we need anything more.


Switched back to draft.

I need to add IsInitialBlockDownload, which was the last thing Sv2TemplateProvider still needs ChainstateManager& m_chainman; for.

I also need to be able to pass BlockAssembler::Options into createNewBlock (in a serialisable way).

Forgot to rename miner on NodeContext, and makeMiner to makeMining in init.h.

Additionally the template provider needs to wait, to see if there's a new tip, with g_best_block_cv.wait_until. How would I go about moving that into this interface?

@Sjors Sjors mentioned this pull request Jun 7, 2024
24 tasks
@Sjors Sjors changed the title Introduce Miner interface Introduce Mining interface Jun 7, 2024
@Sjors Sjors marked this pull request as draft June 7, 2024 10:16
@Sjors Sjors force-pushed the 2024/05/miner-interface branch from d1be135 to 0486cec Compare June 7, 2024 13:57
@Sjors
Copy link
Member Author

Sjors commented Jun 7, 2024

All the above should be fixed now, except for g_best_block_cv.wait_until (but that's not blocking).

The biggest change is in 914c26b. It now makes the options argument mandatory for BlockAssembler constructor, dropping implicit handling of ArgsManager. The caller i.e. the Mining interface implementation now handles this.

In Stratum v2 the pool communicates how many extra bytes it needs for its own outputs (payouts, extra commitments, etc). This needs to be subtracted from what the user set as -blockmaxweight. To achieve that the caller would have to pass in an options object, and not forget to also process -blockmintxfee - a mistake I made in #29432.

@Sjors
Copy link
Member Author

Sjors commented Jun 7, 2024

You can see the interface in action in 5078e51, with one additional argument added to createNewBlock in 41ac95e


Oops, 914c26b broke some tests, e.g. test/functional/rpc_generate.py ...

@Sjors Sjors marked this pull request as ready for review June 7, 2024 14:20
@Sjors Sjors force-pushed the 2024/05/miner-interface branch from 0486cec to febd3d1 Compare June 7, 2024 15:04
@Sjors
Copy link
Member Author

Sjors commented Jun 7, 2024

The generateblock RPC relies on not passing the mempool to BlockAssembler, in order to allow manual transaction selection. So I added bool use_mempool = true to createNewBlock.

Copy link
Contributor

@ryanofsky ryanofsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review ACK febd3d1

@Sjors Sjors force-pushed the 2024/05/miner-interface branch from febd3d1 to 4f9c336 Compare June 10, 2024 16:00
Copy link
Contributor

@itornaza itornaza left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review and std-tests ACK a9716c5

Redirected to this PR from #29346 (comment). I really like how clearly the mining interface is layed out after @ryanofsky comment #29346 (comment) to further support @Sjors work on integrating the noise protocol that seems to be needed for stratum v2.

I have closely followed the commit history, built and run all standard unit, functional and extended tests, and all of them pass.

Since src/interface/mining.h is a new source file, I included a couple of non-blocking styling nits.

* @param[in] use_mempool set false to omit mempool transactions
* @returns a block template
*/
virtual std::unique_ptr<node::CBlockTemplate> createNewBlock(const CScript& script_pub_key, bool use_mempool = true) = 0;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

non-blocking nit: In case you revise this file for a more serious reason, maybe consider adding an empty line here for more readability and consistence with the rest of this source file.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in #30335

/**
* Processes new block. A valid new block is automatically relayed to peers.
*
* @param[in] block The block we want to process.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

non-blocking nit: For consistency with the rest of the headers in this file, you may want to convert the tabs after params to spaces.

Copy link
Contributor

@ryanofsky ryanofsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review ACK a9716c5 with one minor suggestion in case you update. Only changes since last review were other small changes to the interface.

* @param[in] check_merkle_root call CheckMerkleRoot()
* @returns false if any of the checks fail
*/
virtual bool testBlockValidity(BlockValidationState& state, const CBlock& block, bool check_merkle_root = true) = 0;
Copy link
Contributor

@ryanofsky ryanofsky Jun 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In commit "rpc: call TestBlockValidity via miner interface" (d8a3496)

Not important, but it might be good to move the state output parameter last. The reason would be to work with the libmultiprocess code generator in case we want to expose this interface over IPC. The code generator only knows how to parse Cap'n Proto declarations, not C++ code, so when it encounters a Cap'n Proto method declaration with output parameters like:

testBlockValidity @1 (block: Data, maxTxFee :Bool) -> (state: ValidationState, result :Bool);

it just assumes a corresponding C++ method exists with the listed input parameters first and output parameters last.

(Also I just noticed information about output parameters was changed in the developer notes. Suggestion to put output parameters last used used to be mentioned in the "internal interface" style section, but it looks like commit e66b321 from #25092 turned that advice into a bigger, broader rule, which I'm not sure is a great change..)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I'll change this if I have to retouch.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made a note to do this along with a PR to add something like waitTipChanged()
#30200 (comment)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, doing this now since I ran into another (minor) issue: #30335

@ryanofsky ryanofsky merged commit 323b0ac into bitcoin:master Jun 24, 2024
@Sjors Sjors deleted the 2024/05/miner-interface branch June 25, 2024 07:05
@AngusP
Copy link
Contributor

AngusP commented Jun 25, 2024

(late) Code Review ACK a9716c5

Also this was good to read to get more familair with the code touched, thanks!

@@ -859,6 +860,12 @@ class MinerImpl : public Mining
return TestBlockValidity(state, chainman().GetParams(), chainman().ActiveChainstate(), block, chainman().ActiveChain().Tip(), /*fCheckPOW=*/false, check_merkle_root);
}

std::unique_ptr<CBlockTemplate> createNewBlock(const CScript& script_pub_key, bool use_mempool) override
{
LOCK(::cs_main);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this lock required?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably not, because BlockAssembler::CreateNewBlock takes a lock itself. As does ChainstateManager::ActiveChainstate().

I think I initially added this because generateblock() in rpc/mining.cpp did it.

I'll add a commit to #30335 to drop it.

@maflcko found something similar

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(unsure if that was indeed the reason)

ryanofsky added a commit that referenced this pull request Jun 27, 2024
…t rpc bug fix

a74b0f9 Have testBlockValidity hold cs_main instead of caller (Sjors Provoost)
f6dc6db refactor: use CHECK_NONFATAL to avoid single-use symbol (Sjors Provoost)
5fb2b70 Drop unneeded lock from createNewBlock (Sjors Provoost)
75ce763 refactor: testBlockValidity make out argument last (Sjors Provoost)
83a9bef Add missing include for mining interface (Sjors Provoost)

Pull request description:

  Followups from #30200

  Fixes:
  - `std::unique_ptr` needs `#include <memory>` (noticed while working on #30332, which has fewer includes than its parent PR that I originally tested with)
  - Drop lock from createNewBlock that was spuriously added
  - Have testBlockValidity hold cs_main instead of caller (also fixes a race condition in test-only code)

  Refactor:
  - Use CHECK_NONFATAL to avoid single-use symbol (refactor)
  - move output argument `state` to the end of `testBlockValidity`, see #30200 (comment)

ACKs for top commit:
  AngusP:
    Code Review ACK a74b0f9
  itornaza:
    Tested ACK a74b0f9
  ryanofsky:
    Code review ACK a74b0f9. Just new error string is added since last review, and a commit message was updated

Tree-SHA512: 805e133bb59303fcee107d6f02b3e2761396c290efb731a85e6a29ae56b4b1b9cd28ada9629e979704dcfd98cf35034e7e6b618e29923049eb1eca2f65630e41
achow101 added a commit that referenced this pull request Sep 23, 2024
…CNotifyBlockChange, drop CRPCSignals & g_best_block

7942951 Remove unused g_best_block (Ryan Ofsky)
e3a560c rpc: use waitTipChanged for longpoll (Ryan Ofsky)
460687a Remove unused CRPCSignals (Sjors Provoost)
dca9231 Replace RPCNotifyBlockChange with waitTipChanged() (Sjors Provoost)
2a40ee1 rpc: check for negative timeout arg in waitfor* (Sjors Provoost)
de7c855 rpc: recommend -rpcclienttimeout=0 for waitfor* (Sjors Provoost)
77ec072 rpc: fix waitfornewblock description (Sjors Provoost)
285fe9f rpc: add test for waitforblock and waitfornewblock (Sjors Provoost)
b94b27c Add waitTipChanged to Mining interface (Sjors Provoost)
7eccdaf node: Track last block that received a blockTip notification (Sjors Provoost)
ebb8215 Rename getTipHash() to getTip() and return BlockRef (Sjors Provoost)
89a8f74 refactor: rename BlockKey to BlockRef (Sjors Provoost)

Pull request description:

  This continues the work in #30200 so that a future Stratum v2 Template Provider (see #29432) can avoid accessing node internals. It needs to know when a new block arrives in order to push new templates to connected clients.

  `waitTipChanged()` uses a new kernel notification `notifications().m_tip_block_mutex`, which this PR also introduces (a previous version used `g_best_block`).

  In order to ensure the new method works as intended, the `waitfornewblock`, `waitforblock` and `waitforblockheight` RPC methods are refactored to use it. This allows removing `RPCNotifyBlockChange`.

  There's a commit to add (direct) tests for the methods that are about to be refactored:
  - `waitfornewblock` was already implicitly tested by `feature_shutdown.py`.
  - `waitforblockheight` by `feature_coinstatsindex.py` and `example_test.py`

  This PR renames `getTipHash()` to `getTip()` and returns a `BlockRef` (renamed from `BlockKey`) so that callers can use either the height or hash.

  The later commits make trivial improvements to the `waitfor*` RPC calls (not needed for this PR).

  The `waitTipChanged()` method could probably also be used for the longpoll functionality in `getblocktemplate`, but I'm a bit reluctant to touch that.

  `RPCServer::OnStarted` no longer does anything and `RPCServer::OnStopped` merely prints a log statement. They were added in #5711 as a refactor. This PR drops them entirely.

  Finally `g_best_block` is also dropped.

ACKs for top commit:
  achow101:
    ACK 7942951
  ryanofsky:
    Code review ACK 7942951. Just rebased since last review
  TheCharlatan:
    Re-ACK 7942951

Tree-SHA512: a5559446b4000c95e07aad33284b7ee2e57aafd87e1ae778b3825d59689566d047a8047e47a10f76e6e341e7dc72fd265a65afbc0a9c011d17c4cafd55031837
achow101 added a commit that referenced this pull request Sep 25, 2024
1a33281 doc: multiprocess documentation improvements (Ryan Ofsky)
d043950 multiprocess: Add serialization code for BlockValidationState (Ryan Ofsky)
33c2eee multiprocess: Add IPC wrapper for Mining interface (Ryan Ofsky)
06882f8 multiprocess: Add serialization code for vector<char> (Russell Yanofsky)
095286f multiprocess: Add serialization code for CTransaction (Russell Yanofsky)
69dfeb1 multiprocess: update common-types.h to use C++20 concepts (Ryan Ofsky)
206c6e7 build: Make bitcoin_ipc_test depend on bitcoin_ipc (Ryan Ofsky)
070e6a3 depends: Update libmultiprocess library for cmake headers target (Ryan Ofsky)

Pull request description:

  Add Cap'n Proto wrapper for the Mining interface introduced in #30200, and its associated types.

  This PR combined with #30509 will allow a separate mining process, like the one being implemented in Sjors#48, to connect to the node over IPC, and create, manage, and submit block templates. (#30437 shows another simpler demo of a process using the Mining interface.)

  ---

  This PR is part of the [process separation project](#28722).

ACKs for top commit:
  achow101:
    ACK 1a33281
  TheCharlatan:
    ACK 1a33281
  itornaza:
    ACK 1a33281

Tree-SHA512: 0791078dd6885dbd81e3d14c75fffff3da8d1277873af379ea6f9633e910c11485bb324e4cde3d936d50d343b16a10b0e8fc1e0fc6d7bdca7f522211da50c01e
@TheCharlatan
Copy link
Contributor

I've been reading over the changes here again and I am not sure what the purpose of using the new processNewBlock interface function in submitblock is. It just wraps the exact same call to the chainman and the chainman is still invoked directly from it. Is it ok for other external users of processNewBlock to skip over the pre-checks and optimizations that are normally done in submitblock?

Reading through this again, because the way I'd like to see most of these RPCs evolving in the future is indeed just calling functions from the various interfaces, which in turn either implement their own logic, or call a kernel function. The kernel library could potentially expose a submitblock function that implements most of the pre-checks here.

@ryanofsky
Copy link
Contributor

Is it ok for other external users of processNewBlock to skip over the pre-checks and optimizations that are normally done in submitblock?

I assumed it was ok, but now that interfaces::BlockTemplate::submitSolution in added in 525e9dc (to deal with the concern in #30440 (comment)) maybe the interfaces::Mining::processNewBlock method could be dropped and RPC code can go back to calling ProcessNewBlock directly (effectively reverting 7b4d324) if that is preferred.

I don't think it matters much to the RPC implementation which function is called. The reason for switching RPCs to use the mining interface is to test the interface and to take advantage of new features and conveniences it adds, which do not seem to be present in this case.

@Sjors Sjors mentioned this pull request Oct 31, 2024
@Sjors
Copy link
Member Author

Sjors commented Oct 31, 2024

I opened #31196 to drop processNewBlock.

ryanofsky added a commit that referenced this pull request Dec 18, 2024
c991cea Remove processNewBlock() from mining interface (Sjors Provoost)
9a47852 Remove getTransactionsUpdated() from mining interface (Sjors Provoost)
bfc4e02 Remove testBlockValidity() from mining interface (Sjors Provoost)

Pull request description:

  There are three methods in the mining interface that can be dropped. The Template Provider doesn't need them and other application should probably not use them either.

  1. `processNewBlock()` was added in 7b4d324, but became unnecessary with the introduction of interfaces::BlockTemplate::submitSolution in 7b4d324.

  Dropping it was suggested in #30200 (comment)

  2. `getTransactionsUpdated()`: this is used in the implementation of #31003 `waitFeesChanged`. It's not very useful generically because the mempool updates very frequently.

  3. `testBlockValidity()`: it might be useful for mining application to have a way to check the validity of a block template they modified, but the Stratum v2 Template Provider doesn't do that, and this method is a bit brittle (e.g. the block needs to build on the tip).

ACKs for top commit:
  TheCharlatan:
    Re-ACK c991cea
  ryanofsky:
    Code review ACK c991cea. Since last review, just rebased to avoid conflicts in surrounding code, and edited a commit message
  tdb3:
    code review ACK c991cea

Tree-SHA512: 2138e54f920b26e01c068b24498c6a210c5c4358138dce0702ab58185d9ae148a18f04c97ac9f043646d40f8031618d80a718a176b1ce4779c237de6fb9c4a67
Sjors added a commit to Sjors/bitcoin that referenced this pull request May 5, 2025
Our miner code adds OP_0 to the coinbase scriptSig in order to avoid
triggering the bad-cb-length consensus error on test networks.

This commit, like blocktools.py, limits that workaround to blocks 1
through 16 where it's actually needed (OP1 through OP_16).

Previously the coinbase transaction generated by our miner code was
not used downstream, because the getblocktemplate RPC excludes it.

Since the Mining IPC interface was introduced in bitcoin#30200 we do expose
this dummy coinbase transaction. In Stratum v2 several parts of it
are communicated downstream, including the scriptSig.
Sjors added a commit to Sjors/bitcoin that referenced this pull request May 5, 2025
Our miner code adds OP_0 to the coinbase scriptSig in order to avoid
triggering the bad-cb-length consensus error on test networks.

This commit, like blocktools.py, limits that workaround to blocks 1
through 16 where it's actually needed (OP1 through OP_16).

Previously the coinbase transaction generated by our miner code was
not used downstream, because the getblocktemplate RPC excludes it.

Since the Mining IPC interface was introduced in bitcoin#30200 we do expose
this dummy coinbase transaction. In Stratum v2 several parts of it
are communicated downstream, including the scriptSig.
Sjors added a commit to Sjors/bitcoin that referenced this pull request May 5, 2025
Our miner code adds OP_0 to the coinbase scriptSig in order to avoid
triggering the bad-cb-length consensus error on test networks.

This commit, like blocktools.py, limits that workaround to blocks 1
through 16 where it's actually needed (OP1 through OP_16).

Previously the coinbase transaction generated by our miner code was
not used downstream, because the getblocktemplate RPC excludes it.

Since the Mining IPC interface was introduced in bitcoin#30200 we do expose
this dummy coinbase transaction. In Stratum v2 several parts of it
are communicated downstream, including the scriptSig.
Sjors added a commit to Sjors/bitcoin that referenced this pull request May 12, 2025
Our miner code adds OP_0 to the coinbase scriptSig in order to avoid
triggering the bad-cb-length consensus error on test networks.

This commit, like blocktools.py, limits that workaround to blocks 1
through 16 where it's actually needed (OP1 through OP_16).

Previously the coinbase transaction generated by our miner code was
not used downstream, because the getblocktemplate RPC excludes it.

Since the Mining IPC interface was introduced in bitcoin#30200 we do expose
this dummy coinbase transaction. In Stratum v2 several parts of it
are communicated downstream, including the scriptSig.
ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Jun 24, 2025
A missing Init::makeMining implementation was causing internal code using the
mining interface (like the `waitforblockheight` RPC method) to not work when
running inside the `bitcoin-gui` binary. It was working the other bitcoin
binaries ('bitocind`, `bitcoin-qt`, and `bitcoin-node`) because they
implmemented `Init::makeMining` methods in commit
8ecb681 from bitcoin#30200, but the `bitcoin-gui`
init class was forgotten in that change.

This bug was reported by Matthew Zipkin <pinheadmz@gmail.com>
bitcoin#32297 (review)
ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Jul 7, 2025
A missing Init::makeMining implementation was causing internal code using the
mining interface (like the `waitforblockheight` RPC method) to not work when
running inside the `bitcoin-gui` binary. It was working the other bitcoin
binaries ('bitocind`, `bitcoin-qt`, and `bitcoin-node`) because they
implmemented `Init::makeMining` methods in commit
8ecb681 from bitcoin#30200, but the `bitcoin-gui`
init class was forgotten in that change.

This bug was reported by Matthew Zipkin <pinheadmz@gmail.com>
bitcoin#32297 (review)
ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Jul 7, 2025
A missing Init::makeMining implementation was causing internal code using the
mining interface (like the `waitforblockheight` RPC method) to not work when
running inside the `bitcoin-gui` binary. It was working the other bitcoin
binaries ('bitocind`, `bitcoin-qt`, and `bitcoin-node`) because they
implmemented `Init::makeMining` methods in commit
8ecb681 from bitcoin#30200, but the `bitcoin-gui`
init class was forgotten in that change.

This bug was reported by Matthew Zipkin <pinheadmz@gmail.com>
bitcoin#32297 (review)
ryanofsky added a commit that referenced this pull request Aug 20, 2025
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants