Mining Interface doesn't allow for Bitcoin Core to create blocks when it wants

TheBlueMatt commented at 1:29 pm on October 17, 2024: contributor

Finally getting around to reviewing the mining interface, and sadly its missing some critical features that a new mining protocol should have. Specifically, one of the key goals of replacing getblocktemplate is that Bitcoin Core should be able to push work to clients at opportune times. This includes the ability to create a new block in between validating a new block and updating the mempool for a full CNB run. Sadly, forcing the client of the interface to explicitly call CNB makes this impossible.

laanwj added the label Mining on Oct 20, 2024

Sjors commented at 3:13 pm on October 21, 2024: member

This includes the ability to create a new block in between validating a new block and updating the mempool for a full CNB run.

The interface isn’t final so that could be added. But aren’t we holding cs_main throughout the process? cc @TheCharlatan, @ryanofsky

TheCharlatan commented at 8:13 am on October 22, 2024: contributor

But aren’t we holding cs_main throughout the process?

There are brief times where the lock is not held once the ActivateBestChain stage is reached during validation. EDIT: When processing transactions the lock is held the entire time.

TheBlueMatt commented at 6:15 pm on October 22, 2024: contributor

But aren’t we holding cs_main throughout the process?

Doesn’t mean we can’t build a new block. There’s several ways we might build a new block here - (a) we might precalculate an oversized next-next-block in advance and just walk through and quickly remove conflicts to get us mining on a non-empty template as fast as possible, (b) we might delay updating the mempool entirely waiting on new block building - if we’re mining the most important thing is the new work (and relay might work without delay sometimes anyway thanks to HB compact blocks), (c) we might just build new work immediately after updating the mempool in the same cs_main lock to avoid letting anything else take the lock before we get a chance to, etc.

The point is the protocol should support this, Bitcoin Core certainly doesn’t have to do anything with it yet, but if the protocol is structured around it we don’t have to change the protocol when we eventually optimize Bitcoin Core to be smarter. The other nice thing is this way the protocol is fairly DoS resistant natively, making it pretty easy to (in the future) expose out over the wire to semi-trusted clients, rather than relying on the client to not DoS the node.

Sjors commented at 6:15 pm on October 31, 2024: member

I made a note to add waitNewPowBlock() or something similar to the interface, but I think it can wait for a later release. And we should probably get some performance benchmarks to see if it’s even worth it. It’s possible that cluster mempool updates stuff so quickly we can just wait for it.

Adding a new method to the interface just requires assigning it a unique number in mining.capnp. No versioning required.

TheBlueMatt commented at 6:46 pm on October 31, 2024: contributor

I think there’s about four reasons to want to switch around the mining interface to be push-based (rather than the current request-based):

reduces (tail) latency when requesting a block. In most cases it won’t matter, but immediately after finding a new block there is potentially a lot of cs_main contention with other peers sending us copies of the block header or peers requesting the block’s transactions. Even ignoring the on-block stuff, you don’t want to find yourself waiting for some relatively large transaction to fetch a bunch of UTXO entries off disk to get into the mempool to build your new block!
allows for Bitcoin Core to decide when it wants to push a new block, enabling predictive block templates for the next block,
allows Bitcoin Core to avoid updating the mempool or flushing to disk before pushing the next block (or doing so in parallel). Indeed, with cluster mempool the mempool part may matter less, but there may also be a future where UTXO state flushing can happen in parallel while block building happens. Just generally it means anything that happens in Bitcoin Core between when a block has been verified and when we are able to make a round-trip to another process and get cs_main again can be completely elided,
enables a future where we move towards exposing the mining interface over the wire.

Of course it doesn’t have to happen in a particular version, but it’d be good to be sooner rather than later :).

Also good to do it sooner rather than later as some stuff outside Core (at least your work) will use the interface, and ideally all the cruft of the current interface gets removed once the interface is push-based, breaking existing clients but enabling the interface to go over the wire.

sipa commented at 6:49 pm on October 31, 2024: member

I think it would (eventually) make sense that Bitcoin Core can decide when a new block template is generated, as it is at the source of the information that determines whether that’s worth doing.

ryanofsky commented at 7:19 pm on October 31, 2024: contributor

Maybe a flexible way to support this would be to add wait options to the createNewBlock() method so instead of creating a new block template right away, it could optionally wait for conditions to be reached. Those conditions could be set by the client or determined by the node.

Something like:

 0--- a/src/interfaces/mining.h
 1+++ b/src/interfaces/mining.h
 2@@ -90,9 +90,13 @@ public:
 3      *
 4      * [@param](/bitcoin-bitcoin/contributor/param/)[in] script_pub_key the coinbase output
 5      * [@param](/bitcoin-bitcoin/contributor/param/)[in] options options for creating the block
 6+     * [@param](/bitcoin-bitcoin/contributor/param/)[in] wait_options options for waiting to create the block.
 7+     *            If specified, will wait for fees to reach a threshold or for
 8+     *            the tip to change before returning a new block template. If
 9+     *            unspecified, will return a block template without waiting.
10      * [@returns](/bitcoin-bitcoin/contributor/returns/) a block template
11      */
12-    virtual std::unique_ptr<BlockTemplate> createNewBlock(const CScript& script_pub_key, const node::BlockCreateOptions& options = {}) = 0;
13+    virtual std::unique_ptr<BlockTemplate> createNewBlock(const CScript& script_pub_key, const node::BlockCreateOptions& options = {}, const std::optional<node::BlockWaitOptions>& wait_options = {}) = 0;
14 
15     /**
16      * Processes new block. A valid new block is automatically relayed to peers.
17--- a/src/node/types.h
18+++ b/src/node/types.h
19@@ -44,6 +44,26 @@ struct BlockCreateOptions {
20      */
21     size_t coinbase_output_max_additional_sigops{400};
22 };
23+
24+struct BlockWaitOptions {
25+    /**
26+     * If specified, return block template from createNewBlock() as soon as fees
27+     * in the block exceed the specified threshold.
28+     */
29+    CAmount fee_threshold{-1};
30+
31+    /**
32+     * If specified, return template from createNewBlock() as soon as chain tip
33+     * no longer matches specified block hash.
34+     */
35+    uint256 current_tip;
36+
37+    /**
38+     * Maximum time to wait before timing out from createNewBlock() and
39+     * returning null.
40+     */
41+    MillisecondsDouble timeout{MillisecondsDouble::max()};
42+};
43 } // namespace node
44 
45 #endif // BITCOIN_NODE_TYPES_H

Sjors commented at 8:53 pm on October 31, 2024: member

@ryanofsky that approach makes sense to me. In that case perhaps waitFeesChanged() won’t be needed as its own method #31003? That has a nice side-effect in the current implementation of letting us return the last generated block template (I hope eventually we can get the next-block-fees without generating templates at all).

ryanofsky commented at 2:13 pm on November 1, 2024: contributor

Yep, the idea is to drop waitFeesChanged().

Thinking about this more, though, I think a better design that would be simpler for clients and the node implementation would not be to add waiting options to the Mining::createNewBlock() method, but to add a new BlockTemplate::waitNext() method which waits until a better block than the current one can be generated, and then returns a std::unique_ptr<BlockTemplate> pointing to the new block.

This would be simpler for clients because they would not need to worry about choosing timeouts or fee increments (although these could be options to waitNext() if clients do want more control). Clients could just call Mining::createNewBlock() when they first connect to a node, and then run a simple loop calling BlockTemplate::waitNext() after that to get new blocks.

For the server, adding a BlockTemplate::waitNext() method should be simpler than adding waiting options to Mining::createNewBlock() because the BlockTemplate object provides information about the last block sent to the client. The server could use that information to avoid needing to maintain state between calls or needing to trust the client to provide block hashes or information about fees.

Sjors commented at 3:20 pm on November 8, 2024: member

because the BlockTemplate object provides information about the last block sent to the client.

Good point, the lack of such a reference has been a source of headaches.

I plan to look into this next week.

Sjors commented at 12:32 pm on November 13, 2024: member

See #31283.

TheBlueMatt commented at 8:09 pm on December 12, 2024: contributor

That doesn’t do much towards addressing this issue.

Sjors commented at 4:30 am on December 13, 2024: member

This includes the ability to create a new block in between validating a new block and updating the mempool for a full CNB run.

As I said in #31109 (comment) this seems like an optimization that can wait. @plebhash and @GitGab19 are working a stratum v2 benchmarking. Hopefully that will shine some light on the main speed bottlenecks when it comes to switching work to a new tip:

Network latency to the pool
Slow / overload pool machines
ASIC time to stop and switch to a new template (vs. pre-loading it)
etc (?)

From what I’ve seen so far (1) and (2) seem to be hundred of milliseconds at the moment (combined). If that’s true then we already get an order of magnitude improvement by making the template locally and (unilaterally) switching to it (based on submitSolution() or a compact block from our peers, whichever arrives first).

I have a branch that implements createFutureBlock() that can push templates templates for a future block, but no PR yet. It adds some complexity, so imo does need some justification from benchmarks. That includes knowing how fast cluster mempool can update the mempool when a new block comes in and how much it can speed up template generation.

With all that in place a new method waitNewPowBlock() could make sense to shave off a few more milliseconds. Adding a new IPC method doesn’t break older clients. See #31109 (comment)

Sjors commented at 3:08 am on December 20, 2024: member

@gitgab19 and I did a bit of benchmarking for (3) on a S19k Pro. We sent the machine a new job template every second. The proxy software was modified to keep the share difficulty very low.

The time between shares roughly varied from 2-16ms. Presumably following a Poison distribution, but we didn’t analyze that.

With clean_jobs [0] enabled we found that between sending a new template and getting the first share typically 22-65ms went by.

With clean_jobs disabled the delay was 49-65ms. But more importantly, there was a 36-44 ms delay between the last share of the previous job and the first share of this new job.

From this it’s a bit unclear how much time the ASIC spends on loading the job, versus switching to it. Perhaps with stratum v2 native firmware the difference could be more pronounced. Maybe @jakubtrnka has some insights into this?

[0] https://en.bitcoin.it/wiki/Stratum_mining_protocol#mining.notify

TheBlueMatt commented at 4:43 pm on January 9, 2025: contributor

As I said in #31109 (comment) this seems like an optimization that can wait.

Right, and in the next comment I replied with 4 reasons why its relevant, only two of which are specifically performance-related :). Sure, things can be built when they’re built, but just adding a way to wait for the next block does not meaningfully address the reasons why we want this.

ryanofsky commented at 5:01 pm on January 9, 2025: contributor

Right, and in the next comment I replied with 4 reasons why its relevant, only two of which are specifically performance-related :). Sure, things can be built when they’re built, but just adding a way to wait for the next block does not meaningfully address the reasons why we want this.

I think I don’t understand what you are saying. If clients can connect to the node and call waitNext() to wait for block templates, then the node can “decide when it wants to push a new block, enabling predictive block templates for the next block” as you wrote in the linked message and “allow for Bitcoin Core to create blocks when it wants” as you wrote in the title of this issue. And these things can happen without any interface changes. Am I missing something?

TheBlueMatt commented at 9:20 pm on January 14, 2025: contributor

I was referencing the comment at #31109 (comment) The changes proposed do not accomplish point 1, 3, or 4, only point 2. Point 2 is possible more important than 1 or 3, but 4 is also quite important.

sipa commented at 9:26 pm on January 14, 2025: member

@TheBlueMatt I don’t follow. A “wait until you have something for me” interface is push-based, and (depending on the implementation) has the potential to accomplish all points as far as I can tell.

TheBlueMatt commented at 2:43 am on January 15, 2025: contributor

reduces (tail) latency when requesting a block. In most cases it won’t matter, but immediately after finding a new block there is potentially a lot of cs_main contention with other peers sending us copies of the block header or peers requesting the block’s transactions. Even ignoring the on-block stuff, you don’t want to find yourself waiting for some relatively large transaction to fetch a bunch of UTXO entries off disk to get into the mempool to build your new block!

An interface to request notification for when a new block is available does not accomplish this. After the notification fires, the miner calling back in to request a CreateNewBlock run will now have to wait behind cs_main for random P2P stuff, a slow RPC, or, really, post-block-connected stuff, which can take quite some time. An interface that pushes the new work itself, rather than a notification, would not have this issue.

allows Bitcoin Core to avoid updating the mempool or flushing to disk before pushing the next block (or doing so in parallel). Indeed, with cluster mempool the mempool part may matter less, but there may also be a future where UTXO state flushing can happen in parallel while block building happens. Just generally it means anything that happens in Bitcoin Core between when a block has been verified and when we are able to make a round-trip to another process and get cs_main again can be completely elided,

Similar to the above, but instead of just waiting for cs_main, we could actually prepare the block before we’ve finished updating the mempool/releasing cs_main at all during the block-connection process! Again, an interface that notifies the external process, but then requires the external process call back in to trigger CreateNewBlock does not allow for this (in an easy way, I mean the result could just be cached, but if we’re gonna do that just push it!).

enables a future where we move towards exposing the mining interface over the wire.

In an ideal world, whatever mining protocol ends up being used by miners to fetch work from Bitcoin Core should be trivially able to be run over the wire. Miners (and pools, though they currently don’t have much other choice) are often…skittish when it comes to being responsible for selecting transactions themselves. Having the flexibility to take it away from the pool while still outsourcing it may end up being quite important to decentralizing transaction selection. Still, in the short term, its unlikely to be a Thing.

If the interface being used by local processes has DoS risks (beyond just questions about vulns message framing or decoding), that requires a rather complicated sidecar process that translates the protocol and is likely to end up going unmaintained and unused…until its needed, at which point it won’t really be available. Ideally, the protocol as defined for local processes is DoS-risk-free (again, beyond questions about vulns in message framing or decoding), which a simple “hey, when you get a block give it to me!” interface is, but where a “hey, please call CreateNewBlock” isn’t.

A great side-effect of this is that it makes Bitcoin Core more robust in general! A buggy pool server can’t cause Bitcoin Core to grind to a halt calling CreateNewBlock every millisecond, which is even more important when we’re talking about small operations that may not have as active monitoring as a larger pool.

ryanofsky commented at 4:52 am on January 15, 2025: contributor

After the notification fires, the miner calling back in to request a CreateNewBlock run will now have to wait behind cs_main for random P2P stuff

I don’t see why this would be true. There’s no reason Mining.createNewBlock or BlockTemplate.waitNext methods need to use cs_main. cs_main is an implementation detail and not part of the interface. For example we could have:

0std::shared_ptr<CBlockTemplate> g_block_template;
1std::mutex g_block_template_mutex;
2std::condition_variable g_block_template_cv;

And node code could freely update g_block_template and notify the condition variable, and createNewBlock/waitNext methods could return g_block_template and only use g_block_template_mutex / g_block_template_cv to wait for new blocks, and not rely on cs_main.

An interface that pushes the new work itself, rather than a notification, would not have this issue.

waitNext is providing an interface that pushes the new work itself. A reference to the new block is in the return value.

Ideally, the protocol as defined for local processes is DoS-risk-free (again, beyond questions about vulns in message framing or decoding), which a simple “hey, when you get a block give it to me!” interface is, but where a “hey, please call CreateNewBlock” isn’t.

I think you are right that the capnproto interface is inherently more vulnerable to DoS risk than a custom protocol would be. And this isn’t specific to the waitNext method, but true in general because capnproto is asynchronous, and clients can fire off as many requests as they want to the server simultaneously, and it is up to the server to decide how to handle them. There are probably ways this can be addressed within capnproto but I suspect it would probably be easier to switch to a simpler protocol if DoS is a real concern.

The nice thing about the Mining interface is that it is just a plain c++ interface and not tied to capnproto in any way. capnproto and libmultiprocess just provides a way of automatically exposing that interface to other processes without needing to write more code. Having the Mining interface should only make it easier to implement support for other protocols because they can use it without needing to interact with validation code.

Sjors commented at 8:32 am on January 15, 2025: member

Having the Mining interface should only make it easier to implement support for other protocols because they can use it without needing to interact with validation code.

Which can be seen in action in RPC code that calls these methods.

An interface that pushes the new work itself, rather than a notification, would not have this issue.

waitNext is providing an interface that pushes the new work itself. A reference to the new block is in the return value. @TheBlueMatt when you opened this issue the proposed waitFeesChanged() still returned a bool:https://github.com/bitcoin/bitcoin/pull/31003/files#r1804784541. Its successor waitNext() returns a template #31283.

waitTipChanged doesn’t return a template, but it’s typically only used during startup.

TheBlueMatt commented at 8:00 pm on January 18, 2025: contributor

There’s no reason Mining.createNewBlock or BlockTemplate.waitNext methods need to use cs_main. cs_main is an implementation detail and not part of the interface. For example we could have:

Sure, my point wasn’t that it has to be exposed in the interface, rather that the implementation details here matter, and the interface should reflect that.

@TheBlueMatt when you opened this issue the proposed waitFeesChanged() still returned a bool:https://github.com/bitcoin/bitcoin/pull/31003/files#r1804784541. Its successor waitNext() returns a template #31283.

Ah, okay. I appear to be blind and have missed it in #31283.

I think you are right that the capnproto interface is inherently more vulnerable to DoS risk than a custom protocol would be.

This actually wasn’t my point, but good to know :). My point was more broadly that the structure of the interface should be such that if we do change the interface to go over a different protocol, but with the same fundamental interface, it would be nice if that were suddenly relatively DoS-resistant.

That means, basically, that we need an interface where methods are basically free (i.e. don’t call CreateNextBlock for an individual user and don’t allocate a ton of memory for a whole block per-client).

Mining Interface doesn’t allow for Bitcoin Core to create blocks when it wants #31109