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.

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