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.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For detailed information about the code coverage, see the test coverage report.
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.
Reviewers, 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.
isTestChain() that’s used by the getblocktemplate RPC.
rpc/mining.cpp or stick to this for now.
855@@ -854,6 +856,13 @@ class MinerImpl : public Miner
856 return TestBlockValidity(state, chainman().GetParams(), chainman().ActiveChainstate(), block, chainman().ActiveChain().Tip(), /*fCheckPOW=*/false, check_merkle_root);
857 }
858
859+ std::unique_ptr<CBlockTemplate> createNewBlock(const CScript& scriptPubKeyIn) override
860+ {
861+ LOCK(::cs_main);
862+ // TODO pass mempool sanely
863+ return BlockAssembler{chainman().ActiveChainstate(), &*(context()->mempool)}.CreateNewBlock(scriptPubKeyIn);
BlockAssembler constructor
re: #30200 (review)
I guess I should change the
BlockAssemblerconstructor
In commit “rpc: call CreateNewBlock via miner interface” (f04cf9f75054573c204245f7bb1e6813cce1fed8)
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.
get().
33@@ -31,6 +34,7 @@ ChainstateManager& EnsureAnyChainman(const std::any& context);
34 CBlockPolicyEstimator& EnsureFeeEstimator(const node::NodeContext& node);
35 CBlockPolicyEstimator& EnsureAnyFeeEstimator(const std::any& context);
36 CConnman& EnsureConnman(const node::NodeContext& node);
37+interfaces::Miner& EnsureMiner(const node::NodeContext& node);
@ryanofsky I’m not sure what the design philophy is behind Ensure and EnsureAny. The other methods here return more low level objects.
Perhaps related to what you said in #29346 (comment):
I think of “Context” as being a passive container for state instead of an object you would call methods on. The top level interfaces like
interfaces::Chaininterface::Nodeinterfaces::Walletare actually stateless themselves, and only point to state stored in other places. (The idea is to let interface instances be created and destroyed freely, since with multiprocess support new processes can connect and create interface pointers that get destroyed on disconnection, and the state of the node shouldn’t be affected too much from connections and disconnections. Anyway, the mining interface could follow this pattern.)
re: #30200 (review)
In commit “Introduce Miner interface” (b30a8aca5191186f56eb1e9bd8f1a1d899a07039)
@ryanofsky I’m not sure what the design philophy is behind
EnsureandEnsureAny. The other methods here return more low level objects.
This looks good to me. Ensure functions are just supposed to make it possible to access node/wallet state from RPCs in a type-safe way. There are similar functions for the wallet in src/wallet/rpc/util.h.
It is true that existing RPC methods just call node and wallet functions directly, instead of going through interface classes like the GUI does, because there hasn’t been a need for preventing RPC methods from accessing node or wallet internals.
But it would be nice if we could prevent Stratum code from accessing node internals, so having a mining interface seems useful, and having the mining RPCs use the interface is a nice way to test it and make sure it is full-featured.
26+
27+ /** If this chain is exclusively used for testing */
28+ virtual bool isTestChain() = 0;
29+
30+ //! Returns the index entry for the tip of this chain, or nullptr if none.
31+ virtual CBlockIndex* getTip() = 0;
In commit “rpc: getblocktemplate getTip() via Miner interface” (d5b354ed93266d76ab9e2dd2af9c9b611583a1ee)
It looks like all the other Miner parameters and return values are serializable except this CBlockIndex pointer.
This is fine for this PR, but if you wanted to support stratum code running in another process and calling this, you could change this return value to a struct containing whatever information the miner needs to know about the tip (block hash, previous block hash, block time, etc), instead returning of this pointer to a tree of blocks, which can’t easily be used from another process.
I replaced getTip() with getTipHash(). Sv2TemplateProvider doesn’t need it, so for now I use it in a few places in rpc/mining.cpp, but then fall back to chainman.m_blockman.LookupBlockIndex(miner.getTipHash()) where we need more info.
A future refactor could move more of getblocktemplate implementation details out of the RPC.
17+namespace interfaces {
18+
19+//! Interface giving clients (RPC, Stratum v2 Template Provider in the future)
20+//! ability to create block templates.
21+
22+class Miner
In commit “Introduce Miner interface” (b30a8aca5191186f56eb1e9bd8f1a1d899a07039)
Not important, but could consider renaming this class.
interfaces::Miner sounds like an interface that would be used to control a miner, which is not really what this is. Maybe interfaces::Mining or interfaces::MiningService would be better.
Mining
695@@ -694,6 +696,7 @@ static RPCHelpMan getblocktemplate()
696 throw JSONRPCError(RPC_DESERIALIZATION_ERROR, "Block decode failed");
697
698 uint256 hash = block.GetHash();
699+
In commit “rpc: call TestBlockValidity via miner interface” (d880aa0ebe374394b2f8c061ebc7d428f899b59b)
Maybe unintended whitespace change here
758@@ -761,7 +759,7 @@ static RPCHelpMan getblocktemplate()
759 else
760 {
761 // NOTE: Spec does not specify behaviour for non-string longpollid, but this makes testing easier
762- hashWatchedChain = active_chain.Tip()->GetBlockHash();
763+ hashWatchedChain = miner.getTip()->GetBlockHash();
In commit “rpc: getblocktemplate getTip() via Miner interface” (d5b354ed93266d76ab9e2dd2af9c9b611583a1ee)
Would suggest introducing a CBlockIndex* tip{miner.getTip()}; variable, using it where possible, and updating it as needed instead of calling miner.getTip() repeatedly in this function.
It seems like the reason it is ok to make multiple getTip() calls is that cs_main is locked for most of this function. But more ideally, this function would not be locking cs_main directly, only calling interface methods which lock cs_main internally. Using a tip variable in this code, instead of just assuming tip cannot change, would get closer to this state.
uint256 tip{miner.getTipHash()}; at the top. For long polls it’s updated after the wait. See d1be135e568605539fa3932da6e7c5032816825b.
Code review ACK 87ba70a2731951a16432ca2d8d55c62bad87dd41
re: #30200 (comment)
Let me know if I should expand the interface to cover more of
rpc/mining.cppor 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.
@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?
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 914c26bbd4d7e87af4e0d0edf9af69ae76668b7e. 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.
You can see the interface in action in https://github.com/bitcoin/bitcoin/pull/29432/commits/5078e51f4ad3c6bd01580d583274dd5faaf01c70, with one additional argument added to createNewBlock in https://github.com/bitcoin/bitcoin/pull/29432/commits/41ac95ea547eef8c79eb8d2e995df8241f53c030
Oops, 914c26bbd4d7e87af4e0d0edf9af69ae76668b7e broke some tests, e.g. test/functional/rpc_generate.py …
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.
705@@ -703,12 +706,14 @@ static RPCHelpMan getblocktemplate()
706 return "duplicate-inconclusive";
707 }
708
709+ BlockValidationState state;
710+ miner.testBlockValidity(state, block, /*check_merkle_root=*/true);
In commit “rpc: call TestBlockValidity via miner interface” (8589e6c44726d6b5760095d72c37934e4be3315c)
Why is the testBlockValidity check being moved before the block.hashPrevBlock != tip check? The previous ordering seemed more straightforward and consistent with “TestBlockValidity only supports blocks built on the current Tip” comment
25@@ -25,6 +26,9 @@ class Mining
26 /** If this chain is exclusively used for testing */
27 virtual bool isTestChain() = 0;
28
29+ //! Returns the hash for the tip of this chain, 0 if none
30+ virtual uint256 getTipHash() = 0;
In commit “rpc: getblocktemplate getTip() via Miner interface” (9c226c5c3818ef157fc50a5f6ecc534a612ac1d0)
Commit message could be updated since this now called getTipHash
201@@ -203,9 +202,6 @@ int64_t UpdateTime(CBlockHeader* pblock, const Consensus::Params& consensusParam
202
203 /** Update an old GenerateCoinbaseCommitment from CreateNewBlock after the block txs have changed */
204 void RegenerateCommitments(CBlock& block, ChainstateManager& chainman);
205-
206-/** Apply -blockmintxfee and -blockmaxweight options from ArgsManager to BlockAssembler options. */
207-void ApplyArgsManOptions(const ArgsManager& gArgs, BlockAssembler::Options& options);
In commit “rpc: call CreateNewBlock via miner interface” (f375fb9a0dff7ccead80626f4aee853db7799e6f)
It might be good to keep this ApplyArgsManOptions function for more consistency with other options parsing code. The node/interfaces.cpp createNewBlock function could call ApplyArgsManOptions instead of parsing options itself. The current approach also seems ok though if you prefer it.
I split f375fb9a0dff7ccead80626f4aee853db7799e6f in two parts:
BlockAssembler constructor without options argumentI changed this second commit to keep ApplyArgsManOptions.
865+
866+ // Block resource limits
867+ options.nBlockMaxWeight = gArgs.GetIntArg("-blockmaxweight", options.nBlockMaxWeight);
868+ if (const auto blockmintxfee{gArgs.GetArg("-blockmintxfee")}) {
869+ if (const auto parsed{ParseMoney(*blockmintxfee)}) options.blockMinFeeRate = CFeeRate{*parsed};
870+ }
In commit “rpc: call CreateNewBlock via miner interface” (f375fb9a0dff7ccead80626f4aee853db7799e6f)
I’m not clear how this implementation lines up with the commit description which says “the pool communicates how many extra bytes it needs for its own outputs (payouts, extra commitments, etc). This needs to be substracted from what the user set as -blockmaxweight.”
Is the commit description just saying that this implementation is not complete and it will need to be extended in the future? Would be helpful to clarify.
@ryanofsky do you have any thoughts on this (for this PR or a followup)?
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?
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?
Maybe the simplest way to do it would be to have a waitTipChanged() method that just blocks waiting for a new tip, and would be interrupted when the node shuts down or the mining interface is destroyed. If needed, the waiting interface could have more features and accept a timeout or return an object with wait() and cancel() methods.
Another alternative would be to provide a callback interface instead of a waiting interface like interfaces::Node:
24@@ -22,6 +25,13 @@ class Mining
25 /** If this chain is exclusively used for testing */
26 virtual bool isTestChain() = 0;
27
28+ /**
29+ * Check a block is completely valid from start to finish.
30+ * Only works on top of our current best block.
31+ * Does not check proof-of-work.
32+ * */
33+ virtual bool testBlockValidity(BlockValidationState& state, const CBlock& block, bool check_merkle_root) = 0;
nit: Since this is an interface, to maximize clarity it might be good to add Doxygen-ey style @param and @returns statements in the function comment, similar to processNewBlock().
Same for createNewBlock().
Also, would it make sense to provide a default value for check_merkle_root (e.g. default true)? Or do we want users of the interface to explicitly state whether or not they want merkle root checking performed?
true to mimic TestBlockValidity in validation.h.
712+ if (block.hashPrevBlock != pindexPrev->GetBlockHash()) {
713 return "inconclusive-not-best-prevblk";
714+ }
715 BlockValidationState state;
716- TestBlockValidity(state, chainman.GetParams(), active_chainstate, block, pindexPrev, false, true);
717+ miner.testBlockValidity(state, block, /*check_merkle_root=*/true);
check_merkle_root is added to testBlockValidity(), this call could be updated as well.
843@@ -844,6 +844,14 @@ class MinerImpl : public Mining
844 return chainman().GetParams().IsTestChain();
845 }
846
847+ uint256 getTipHash() override
848+ {
849+ LOCK(::cs_main);
850+ CBlockIndex* tip{chainman().ActiveChain().Tip()};
851+ if (!tip) return uint256{0};
852+ return tip->GetBlockHash();
std::optional<uint256> instead?
GetBlockHash(), which was called directly before this commit, has an assert that tip must exist. So return uint256{0}; should never happen. An optional does seem more robust though, will look into it.
Update: done
820+ pblocktemplate = miner.createNewBlock(scriptDummy);
821+ if (!pblocktemplate) {
822 throw JSONRPCError(RPC_OUT_OF_MEMORY, "Out of memory");
823+ }
824
825 // Need to update only after we know CreateNewBlock succeeded
CreateNewBlock rather than the new createNewBlock. Feel free to ignore unless touching this file for something else.
ACK 5666af2c6e4924bf8e358ab5121c88d14cb085df Great job laying groundwork for Sv2.
Left some comments (mostly nits). Ran unit and functional tests (passed).
Other than running unit/functional tests and exercising test networks (e.g. regtest for generate calls), any other good ways you’d like to see these changes exercised getblocktemplate() is a pretty crucial RPC, so being extra paranoid here could be prudent.
p.s. May want to update the description, since RPC functions beyond getblocktemplate() and gererateblock() also use the new Mining interface (e.g. submitblock(), generatetodescriptor(), generatetoaddress(), etc.)
any other good ways you’d like to see these changes exercised
getblocktemplate()is a pretty crucial RPC
You could test #27433 on top of the changes here. Or try solo CPU mining on a custom signet, e.g. using pooler/cpuminer and then:
0./minerd -u bitcoin -p bitcoin -o http://127.0.0.1:38332 --coinbase-addr $ADDRESS --no-stratum --algo=sha256d
The latter requires signetchallenge=51 and a patch setting #define GBT_RULES "[\"segwit\", \"signet\"]".
update the description
Done
Pushed an update to add Doxygen comments for functions that take at least one parameter. Also renamed scriptPubKeyIn to script_pub_key.
I changed 38315fc9439ec18f3cb21e2c87c1c590dba78ee2 to have getTipHash() return an optional. Now that there’s only two calls to it, it’s not too tedious to (sanity) check for a missing tip.
Review hint:
0PREV=5666af2 N=9 && git range-diff `git merge-base --all HEAD $PREV`...$PREV HEAD~$N...HEAD
'TestBlockValidity failed: bad-txns-inputs-missingorspent' in rpc_generate.py (changed to testBlockValidity).
Start out with a single method isTestChain() that's used by the getblocktemplate RPC.
This makes the options argument for BlockAssembler constructor mandatory,
dropping implicit use of ArgsManager. The caller i.e. the Mining
interface implementation now handles this.
In a future Stratum v2 change the Options object needs to be
mofified after arguments have been processed. Specifically
the pool communicates how many extra bytes it needs for
its own outputs (payouts, extra commitments, etc). This will need
to be substracted from what the user set as -blockmaxweight.
Such a change can be implemented in createNewBlock, after
ApplyArgsManOptions.
Set tip at the start of the function and only update it for a long poll.
Additionally have getTipHash return an optional, so the
caller can explicitly check that a tip exists.
#includes and classes.
re ACK a9716c53f05082d6d89ebea51a46d4404efb12d7
Re-built and ran unit/functional tests (passed).
Tested #30200 (comment) by mining with cpuminer (https://github.com/pooler/cpuminer) for a custom signet (with signetchallenge=51 set in bitcoin.conf to allow mining without signature).
Mined for 5000 blocks, and created and spent several transactions (one from 135 coinbase outputs and two derivative transactions). Temporarily paused mining, created and broadcasted three additional transactions. Called getblocktemplate to verify that the transactions were included (src/bitcoin-cli -datadir=/home/dev/customsignet getblocktemplate '{"rules":["segwit","signet"]}'), then continued mining to confirm the transactions. All mining and spending was successful.
This Issue from the cpuminer repo was helpful in bootstrapping (thanks @sjors): https://github.com/pooler/cpuminer/issues/285
Useful conf and cli:
bitcoin.conf:
0signet=1
1rpcuser=bitcoin
2rpcpassword=bitcoin
3[signet]
4signetchallenge=51
0src/bitcoin-cli -datadir=/home/dev/customsignet getblocktemplate '{"rules":["segwit","signet"]}'
cpuminer:
0./minerd -u bitcoin -p bitcoin -o http://127.0.0.1:38332 --coinbase-addr tb1qwz06m34zc9e5cy8tjz7f3x5xemega5rn0waq0v --no-stratum --algo=sha256d
41+ *
42+ * @param[in] script_pub_key the coinbase output
43+ * @param[in] use_mempool set false to omit mempool transactions
44+ * @returns a block template
45+ */
46+ virtual std::unique_ptr<node::CBlockTemplate> createNewBlock(const CScript& script_pub_key, bool use_mempool = true) = 0;
45+ */
46+ virtual std::unique_ptr<node::CBlockTemplate> createNewBlock(const CScript& script_pub_key, bool use_mempool = true) = 0;
47+ /**
48+ * Processes new block. A valid new block is automatically relayed to peers.
49+ *
50+ * @param[in] block The block we want to process.
Code review and std-tests ACK a9716c53f05082d6d89ebea51a46d4404efb12d7
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.
33+ * @param[out] state details of why a block failed to validate
34+ * @param[in] block the block to validate
35+ * @param[in] check_merkle_root call CheckMerkleRoot()
36+ * @returns false if any of the checks fail
37+ */
38+ virtual bool testBlockValidity(BlockValidationState& state, const CBlock& block, bool check_merkle_root = true) = 0;
In commit “rpc: call TestBlockValidity via miner interface” (d8a3496b5ad27bea4c79ea0344f595cc1b95f0d3)
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:
0testBlockValidity [@1](/bitcoin-bitcoin/contributor/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 e66b321fd1ddfffd9bfc59d407ad8f03490b873c from #25092 turned that advice into a bigger, broader rule, which I’m not sure is a great change..)
waitTipChanged()
#30200 (comment)
(late) Code Review ACK a9716c53f05082d6d89ebea51a46d4404efb12d7
Also this was good to read to get more familair with the code touched, thanks!
859@@ -859,6 +860,12 @@ class MinerImpl : public Mining
860 return TestBlockValidity(state, chainman().GetParams(), chainman().ActiveChainstate(), block, chainman().ActiveChain().Tip(), /*fCheckPOW=*/false, check_merkle_root);
861 }
862
863+ std::unique_ptr<CBlockTemplate> createNewBlock(const CScript& script_pub_key, bool use_mempool) override
864+ {
865+ LOCK(::cs_main);
