Suggested in #29432 (comment)
An external program that uses the Mining interface may need quick access to some information in the block template, while it can wait a bit longer for the full raw transaction data.
This would be the case for a Stratum v2 Template Provider which needs to send a NewTemplate message message (which doesn’t include transactions) as quickly as possible. It does not include the serialized block transactions.
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 | ryanofsky, itornaza, TheCharlatan, achow101 |
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.
🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/27375256737
Make sure to run all tests locally, according to the documentation.
The failure may happen due to a number of reasons, for example:
Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.
Leave a comment here, if you need help tracking down a confusing failure.
In #29432 (comment) @TheBlueMatt wrote:
Any IPC thing that requires serializing all the transactions for all work is going to be too slow. Bitcoin Core needs to be able to track the templates it gave out and keep them around (to keep the Transaction
shared_ptrs in a block ready to go) until they time out.
This PR returns a BlockTemplate interface which causes the node to hold on to the template and its block.
The goal is to be able to quickly send NewTemplate and after that to copy all transactions to the external application so it can respond to RequestTransactionData and SubmitSolution.
I assume your concern is that this would make SubmitSolution too slow because the external application needs to pass the whole block back via IPC? Or are you even worried that the TP wouldn’t have the full block ready by the time RequestTransactionData comes in?
I could make submitSolution a method on the BlockTemplate interface. This would allow the external application to hold on to the BlockTemplate interface all the way until a solution comes in. This does move the burden of template memory management to the node.
Would it need a way to unilaterally drop templates, or to tell the external application to pick which templates to drop?
Benchmarks would be useful here.
submitSolution to the BlockTemplate interface and updated the description.
🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/27448293750
Make sure to run all tests locally, according to the documentation.
The failure may happen due to a number of reasons, for example:
Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.
Leave a comment here, if you need help tracking down a confusing failure.
getBlockHeader() which the Template Provider needs for the NewTemplate message and a few other places.
This PR returns a BlockTemplate interface which causes the node to hold on to the template and its block. The goal is to be able to quickly send NewTemplate and after that to copy all transactions to the external application so it can respond to RequestTransactionData and SubmitSolution. I assume your concern is that this would make SubmitSolution too slow because the external application needs to pass the whole block back via IPC?
Ah, okay, that was my only real concern. Sending transaction data with the nonce solution requires Bitcoin Core do a metric shitload of allocations and takes ~forever. My only concern was making sure Bitcoin Core held on to the CTransaction share_ptrs for the transaction data that needs to be put in the CBlock.
941 result.pushKV("rules", std::move(aRules));
942 result.pushKV("vbavailable", std::move(vbavailable));
943 result.pushKV("vbrequired", int(0));
944
945- result.pushKV("previousblockhash", pblock->hashPrevBlock.GetHex());
946+ result.pushKV("previousblockhash", block_template->getBlockHeader().hashPrevBlock.GetHex());
In commit “Have createNewBlock return BlockTemplate interface” (4af3d9a483e4107301f0f0a42da781dfa79acb82)
Should be possible to avoid getBlockHeader calls by just using the block variable:
0--- a/src/rpc/mining.cpp
1+++ b/src/rpc/mining.cpp
2@@ -941,7 +941,7 @@ static RPCHelpMan getblocktemplate()
3 result.pushKV("vbavailable", std::move(vbavailable));
4 result.pushKV("vbrequired", int(0));
5
6- result.pushKV("previousblockhash", block_template->getBlockHeader().hashPrevBlock.GetHex());
7+ result.pushKV("previousblockhash", block.hashPrevBlock.GetHex());
8 result.pushKV("transactions", std::move(transactions));
9 result.pushKV("coinbaseaux", std::move(aux));
10 result.pushKV("coinbasevalue", (int64_t)block.vtx[0]->vout[0].nValue);
11@@ -963,8 +963,8 @@ static RPCHelpMan getblocktemplate()
12 if (!fPreSegWit) {
13 result.pushKV("weightlimit", (int64_t)MAX_BLOCK_WEIGHT);
14 }
15- result.pushKV("curtime", block_template->getBlockHeader().GetBlockTime());
16- result.pushKV("bits", strprintf("%08x", block_template->getBlockHeader().nBits));
17+ result.pushKV("curtime", block.GetBlockTime());
18+ result.pushKV("bits", strprintf("%08x", block.nBits));
19 result.pushKV("height", (int64_t)(pindexPrev->nHeight+1));
20
21 if (consensusParams.signet_blocks) {
getBlockHeader() here to test the implementation, but it’s indeed not really useful in this context.
886+ HashWriter hasher{};
887+ hasher << txid1 << txid2;
888+ return hasher.GetHash();
889+ }
890+
891+ std::vector<uint256> getCoinbaseMerklePath() override
In commit “Add getCoinbaseMerklePath() to Mining interface” (1f5f16d166f8944d532aadc382b30f68a06720e2)
Instead of implementing this function in node/interfaces.cpp file it would be better to add a standalone GetCoinBaseMerklePath(const CBlock&block) function or a new CBlock method that the interface method could call. This would keep the interfaces.cpp file simple, and let it just contain glue code instead of more complicated functionality. It would also make the merkle path functionality easier to test, and more accessible to code not using the interface classes.
84@@ -83,3 +85,55 @@ uint256 BlockWitnessMerkleRoot(const CBlock& block, bool* mutated)
85 return ComputeMerkleRoot(std::move(leaves), mutated);
86 }
87
88+uint256 HashTwoTxIDs(const uint256& txid1, const uint256& txid2) {
In commit “Add GetCoinBaseMerklePath helper” (c230cc1a72dc4e4079cb2f7b05586e6f32e844ab)
May want to declare this static to avoid creating an unused link symbol.
845+ explicit BlockTemplateImpl(std::unique_ptr<CBlockTemplate> block_template) : m_block_template(std::move(block_template)) {
846+ m_block_header = m_block_template->block.GetBlockHeader();
847+ }
848+
849+ std::unique_ptr<CBlockTemplate> m_block_template;
850+ CBlockHeader m_block_header;
In commit “Have createNewBlock return BlockTemplate interface” (b4782696a8ec5e21a41db3702d4b4dcc90ea40dd)
Could you move these member declarations to the bottom of the class? It makes it easier to see what state the interface accesses when state is declared at beginning or end of the class, not in the middle mixed with method definitions.
I’m also wondering if it might be more efficient to avoid the call to CBlock::GetBlockHeader():
0--- a/src/node/interfaces.cpp
1+++ b/src/node/interfaces.cpp
2@@ -843,14 +843,10 @@ class BlockTemplateImpl : public BlockTemplate
3 {
4 public:
5 explicit BlockTemplateImpl(std::unique_ptr<CBlockTemplate> block_template) : m_block_template(std::move(block_template)) {
6- m_block_header = m_block_template->block.GetBlockHeader();
7 }
8
9- std::unique_ptr<CBlockTemplate> m_block_template;
10- CBlockHeader m_block_header;
11-
12 CBlockHeader getBlockHeader() override {
13- return m_block_header;
14+ return Assert(m_block_template)->block;
15 }
16
17 CBlock getBlock() override
18@@ -883,6 +879,7 @@ public:
19 return GetWitnessCommitmentIndex(m_block_template->block);
20 }
21
22+ std::unique_ptr<CBlockTemplate> m_block_template;
23 };
24
25 class MinerImpl : public Mining
Do you mean return Assert(m_block_template)->block.GetBlockHeader()?
The reason I made it a member was so that the interface would return it upon constructing BlockTemplate. This would then save multiple round trips over the interface later when we need a bunch of details from the header.
Do you mean return
Assert(m_block_template)->block.GetBlockHeader()?
No I think the diff above should work. There should be no need for calling GetBlockHeader() at all, much less caching the result of the call, because CBlock inherits from CBlockHeader.
I’m not suggesting dropping the getBlockHeader interface method, because that method is useful for avoiding round trips like you suggest.
because CBlock inherits from CBlockHeader
Ah ok
that method is useful for avoiding round trips
The choice is between having the caller be careful to avoid round trips, vs not needing round trips at all because we already got the header at construction time.
That’s likely a tiny difference, so whatever is better code is probably best.
159@@ -160,12 +160,12 @@ static UniValue generateBlocks(ChainstateManager& chainman, Mining& miner, const
160 {
161 UniValue blockHashes(UniValue::VARR);
162 while (nGenerate > 0 && !chainman.m_interrupt) {
163- std::unique_ptr<CBlockTemplate> pblocktemplate(miner.createNewBlock(coinbase_script));
164- if (!pblocktemplate.get())
165+ std::unique_ptr<BlockTemplate> block_template(miner.createNewBlock(coinbase_script));
166+ if (!block_template.get())
In commit “Have createNewBlock return BlockTemplate interface” (6536b345a346670b8733e5372f988452727a2554)
I don’t see any way this condition could be true, now or previously. Would suggest changing this to assert(block_template) or CHECK_NONFATAL(block_template)
Same comment also applies to createNewBlock calls below on lines 374 and 817.
The check here also appears to be dead code. I think it would be good to remove, or replace with an assert, maybe in a separate commit.
The only way this should fail is coinbase_script is invalid (e.g. too large). But that currently results in a throw std::runtime_error inside BlockAssembler::CreateNewBlock.
Changed the three lines to CHECK_NONFATAL.
Added commit to drop the dead code.
838@@ -838,6 +839,49 @@ class ChainImpl : public Chain
839 NodeContext& m_node;
840 };
841
842+class BlockTemplateImpl : public BlockTemplate
843+{
844+public:
845+ explicit BlockTemplateImpl(std::unique_ptr<CBlockTemplate> block_template) : m_block_template(std::move(block_template)) {}
In commit “Have createNewBlock return BlockTemplate interface” (6536b345a346670b8733e5372f988452727a2554)
Would be good to guard against undefined behavior by adding assert(m_block_template); in the constructor body and declaring m_block_template as const unique_ptr to prevent it being reset to null.
If this is done, the Assert in getBlockHeader could also be removed so it is more consistent with the other methods.
I split getCoinbaseMerklePath() and submitSolution() out of this PR and moved them to https://github.com/Sjors/bitcoin/pull/53.
Since it’s astronomically unlikely multiprocess will be in the upcoming v28.0 release, we can add these functions later. They’re both only needed for Stratum v2. getCoinbaseMerklePath() uses the new GetCoinBaseMerklePath which I need to study in more detail myself.
Rebased and addressed comments above: #30440#pullrequestreview-2186655119
Marked as no longer draft.
🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/27740768751
Make sure to run all tests locally, according to the documentation.
The failure may happen due to a number of reasons, for example:
Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.
Leave a comment here, if you need help tracking down a confusing failure.
128@@ -129,7 +129,7 @@ static RPCHelpMan getnetworkhashps()
129 };
130 }
131
132-static bool GenerateBlock(ChainstateManager& chainman, Mining& miner, CBlock& block, uint64_t& max_tries, std::shared_ptr<const CBlock>& block_out, bool process_new_block)
133+static bool GenerateBlock(ChainstateManager& chainman, Mining& miner, CBlock&& block, uint64_t& max_tries, std::shared_ptr<const CBlock>& block_out, bool process_new_block)
In commit “Have createNewBlock return BlockTemplate interface” (f434c8562281dfec1bab37e7232ca35fd759e25f)
While changing this it would make sense to add std::move() on line 148 below so the block data does not need to be copied there.
24 class CScript;
25
26 namespace interfaces {
27
28+//! Block template interface
29+class BlockTemplate
In commit “Have createNewBlock return BlockTemplate interface” (977ab92f8040cd139fc4b969baeec39ef58c00af)
Want to note that at some point, depending on how this interface is used, it might be a good idea to consolidate individual accessor methods into more general ones that return structs or tuples to return multiple values at the same time.
For example, if getTxFees() and getTxSigops() are typically called one after another over IPC it could make sense to combine them into a getTxInfo() function to save a round trip.
getTxFees() and getTxSigops() in stratum v2, or only for the existing stratum v1 RPC calls. Once sv2 code is a bit more mature it’s indeed worth going over the interface once more.
879+ int getWitnessCommitmentIndex() override
880+ {
881+ return GetWitnessCommitmentIndex(m_block_template->block);
882+ }
883+
884+ const std::unique_ptr<CBlockTemplate> m_block_template;
Does this need to be public?
0
1private:
2 const std::unique_ptr<CBlockTemplate> m_block_template;
Does this need to be public?
Convention in interface.cpp files is to make class members public since the classes are private and meant to be a minimal as possible (ideally just containing accessors and forwarding methods). Comment at top of the file mentions this:
823 // Need to update only after we know createNewBlock succeeded
824 pindexPrev = pindexPrevNew;
825 }
826 CHECK_NONFATAL(pindexPrev);
827- CBlock* pblock = &pblocktemplate->block; // pointer for convenience
828+ CBlock block{block_template->getBlock()};
CBlock pointer to using getBlock which returns a value directly going to have much of a performance impact (lots of copying)?
Likely a dumb question, but is switching from a
CBlockpointer to usinggetBlockwhich returns a value directly going to have much of a performance impact (lots of copying)?
Definitely not a dumb question, this change results in an extra copy (only a move is happening on this line, but there is a copy inside the getBlock() function, but a copy is also removed later on due to adding std::move in GenerateBlock, so the net number of copies should be the same.
Definitely not a dumb question, this change results in an extra copy (only a move is happening on this line, but there is a copy inside the
getBlock()function, but a copy is also removed later on due to adding std::move inGenerateBlock, so the net number of copies should be the same.
I don’t understand this comment. This line is part of the getblocktemplate RPC, which is a real RPC (possibly used in production). Whereas the GenerateBlock function is a test-only function for the test-only RPCs generate*.
So it looks like this is adding an extra copy in production code so that a copy can be removed in test-only code.
Generally, production code should not be adjusted to accommodate tests, but rather the other way round. So doing the inverse here would have been better.
Am I missing something obvious?
Edit: I guess the block is later (unconditionally) serialized as UniValue, so adding or removing a copy here shouldn’t change the overall performance.
I split
getCoinbaseMerklePath()andsubmitSolution()out of this PR and moved them to Sjors#53.
I’ve since modified getCoinbaseMerklePath in that PR to reuse BlockMerkleBranch instead of bringing its own GetCoinBaseMerklePath implementation. I’ll move/reopen https://github.com/Sjors/bitcoin/pull/53 to the main repo after this PR is merged. I think it makes sense to keep it separate, because it requires a bit more mining specific knowledge to review than this PR.
4@@ -5,26 +5,44 @@
5 #ifndef BITCOIN_INTERFACES_MINING_H
6 #define BITCOIN_INTERFACES_MINING_H
7
8+#include <consensus/amount.h>
9+#include <primitives/transaction.h>
Nit: Alphabetical order for the includes. There are some other minor clang-format issues, maybe run the commit through clang-format-diff? As far as I know we also don’t have a tool to check the includes of these interface headers, but it is possible to check them manually:
0interfaces/mining.h should add these lines:
1#include <stdint.h> // for int64_t
2#include <vector> // for vector
3namespace node { struct BlockCreateOptions; }
4
5interfaces/mining.h should remove these lines:
6- #include <node/types.h> // lines 11-11
7- class CBlock; // lines 22-22
8- class CBlockHeader; // lines 23-23
9
10The full include-list for interfaces/mining.h:
11#include <consensus/amount.h> // for CAmount
12#include <primitives/block.h> // for CBlock, CBlockHeader
13#include <primitives/transaction.h> // for CTransactionRef
14#include <stdint.h> // for int64_t
15#include <uint256.h> // for uint256
16#include <memory> // for unique_ptr, shared_ptr
17#include <optional> // for optional
18#include <vector> // for vector
19class BlockValidationState; // lines 21-21
20class CScript; // lines 24-24
21namespace node { struct BlockCreateOptions; }
22namespace node { struct NodeContext; } // lines 18-18
23---
Rebased, ran clang-format-diff and updated includes and forward declarations based on #30440 (review)
Except that I kept #include <node/types.h> around because with namespace node { struct BlockCreateOptions; } my clang 15.0.0 compiler would complain:
0 initialization of incomplete type 'const node::BlockCreateOptions'
1 virtual std::unique_ptr<BlockTemplate> createNewBlock(const CScript& script_pub_key, const node::BlockCreateOptions& options = {}) = 0;
Code review ACK 33f5ab32b2651a18c9d56bef1e5b3c69dd26e199. Only changes since last review were include changes and reformatting changes.
I have to say I am not a fan of running clang-format-diff in same commits that introduce real code changes and making unrelated changes to surrounding lines of code. It particularly adds a lot of noise to this PR in the ThresholdState switch statement and makes it harder to review. IMO it is preferable to use clang-format-diff only to format code that is actually changing, not surrounding code, or to run it and commit the changes separately.
@ryanofsky I used the incantation suggested here: https://github.com/bitcoin/bitcoin/blob/master/contrib/devtools/README.md#clang-format-diffpy
I assumed this would only edit lines impacted by the commit. But I see it touched a few other places in rpc/mining.cpp. I removed unrelated changes from the commit.
Concept ACK 23374a22f442939802c0296d6447e0db02d513da
Do you consider updating for cmake? It would be helpful for me to build and run tests locally.
An external program that uses the Mining interface may need quick access to some information in the block template, while it can wait a bit longer for the full raw transaction data.
This would be the case for a Stratum v2 Template Provider which needs to send a NewTemplate message (which doesn't include transactions) as quickly as possible.
Code review ACK a93c171faa7b4426823466e972c8f24260918bbf
Offering an option for the Template Provider to get block header data as soon as possible is a great addition.
Code changes look straight forward. Also, built the pr and successfully run all tests on it locally.
ACK a93c171faa7b4426823466e972c8f24260918bbf
It seems like BlockTemplate is mostly just a wrapper for CBlockTemplate. Presumably there was some discussion about having mining things in a separate process which makes having an interface for it necessary for IPC.