Providing a script for the coinbase transaction is only done in test code and for (unoptimized) CPU solo mining.
Production miners use the getblocktemplate RPC which omits the coinbase transaction entirely from its block template, leaving it to external (pool) software to construct it.
This commit removes the script_pub_key argument from createNewBlock() in the Mining interface.
A coinbase script can still be passed via BlockCreateOptions instead. Tests are modified to do so.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31318.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | ryanofsky, TheCharlatan, vasild |
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/33145905317
Try to run the tests locally, according to the documentation. However, a CI failure may still 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.
43@@ -43,6 +44,10 @@ struct BlockCreateOptions {
44 * transaction outputs.
45 */
46 size_t coinbase_output_max_additional_sigops{400};
47+ /**
48+ * Script to put in the coinbase transaction.
49+ */
50+ std::optional<CScript> coinbase_script{};
not sure about making this optional. Even if this is a dummy, it would be good to spell it out.
Using std::optional here is just making code more complex, adding two lines of code in one place to save one line of code in another place.
Also, it may be better to call mention output script in the name.
In production this is always a dummy. CreateNewBlock() sets the script to OP_TRUE and then the getblocktemplate RPC omits the whole coinbase transaction.
It’s not safe for the caller to set an arbitrary dummy, because IIUC if it’s larger than coinbase_max_additional_weight the TestBlockValidity internal check for getblocktemplate will spuriously fail. Which is why I think it’s better for CreateNewBlock to decide what the dummy should be.
mention output script in the name
That makes sense.
re: #31318 (review)
Would it make sense to declare the field as CScript coinbase_output_script{CScript{} << OP_TRUE} instead of std::optional<CScript> coinbase_output_script{}; just to be clearer about what the default value is? Or maybe just add documentation to say what happens if this is not set?
I think the drawback of using std::optional here is that it’s not really clear what happens if the option is not set. It’s also not clear when you would want to set the option here, if ever.
I expanded the comment to explain things a bit more.
I think you should not set this option unless you care what the coinbase output is. For example the block_assemble benchmark uses P2WSH_OP_TRUE. I’m guessing that’s because it wants to look at the performance related to spending from a segwit coinbase.
coinbase_output_script, added warning and discouragement, fixed fuzz binary build.
mining.capnp.
Afaik the only way external users make blocks today is the getblocktemplate RPC, which drops the coinbase transaction from the template it returns.
Similarly in submitSolution, used by Stratum v2, we expect to receive the full coinbase transaction and replace whatever was in the CBlockTemplate.
Unless you’re solo mining, the pool is going to add its own outputs, manipulate the extra nonce, add its own name, etc. Even with a solo pool (a weird concept) you would configure the payout address in that software, not in the node.
There is a way in Stratum v2 to specify a list of outputs that you want in the coinbase. In my current implementation we use this to include the witness commitment, but nothing else.
In the longer run we could expand the interface to specify more outputs.
The special case for that is a single payout address that takes all the money from the block. That could be used for solo mining with a single small ASIC that doesn’t need to grind the extra nonce (up to about 100 TH/s with BIP320). But that’s fairly niche use case. As soon as you use Stratum v1 / v2 software you typically pass a payout address to the the pool software, not to the node.
Rebased to avoid spurious CI failure.
I’ll take a look later if for any of the tests I can drop their custom coinbase output.
Thank you for spelling this out @Sjors. I also like ryanofsky’s idea of adding the OP_TRUE script as the default option instead of making it a std::optional.
Concept ACK
119@@ -120,6 +120,12 @@ std::unique_ptr<CBlockTemplate> BlockAssembler::CreateNewBlock(const CScript& sc
120 pblocktemplate->vTxFees.push_back(-1); // updated at end
121 pblocktemplate->vTxSigOpsCost.push_back(-1); // updated at end
122
123+ // Use dummy coinbase output script if none is provided
124+ CScript coinbase_output_script{CScript() << OP_TRUE};
125+ if (m_options.coinbase_output_script) {
126+ coinbase_output_script = m_options.coinbase_output_script.value();
In commit “Drop script_pub_key arg from createNewBlock” (42f880aeb080070a42229d0bccd517100a3fe7fb)
Would suggest using ‘*’ instead of .value() since this code isn’t intending to trigger the std::bad_optional_access exception, and to make it more obvious a dereference is happening.
coinbase_output_script to no longer be std::optional.
105@@ -106,7 +106,7 @@ void BlockAssembler::resetBlock()
106 nFees = 0;
107 }
108
109-std::unique_ptr<CBlockTemplate> BlockAssembler::CreateNewBlock(const CScript& scriptPubKeyIn)
110+std::unique_ptr<CBlockTemplate> BlockAssembler::CreateNewBlock()
In commit “Drop script_pub_key arg from createNewBlock” (42f880aeb080070a42229d0bccd517100a3fe7fb)
Would it make sense to change the BlockAssembler::CreateNewBlock() signature in a separate followup commit? The test changes in this commit vastly outnumber the non-test changes, which I think make it more likely a bug in the non-test code could be missed.
Maybe this there could be two overloads initially:
0std::unique_ptr<CBlockTemplate> CreateNewBlock()
1{
2 return CreateNewBlock(m_options.coinbase_output_script ? *m_options.coinbase_output_script : CScript{} << OP_TRUE);
3}
4
5std::unique_ptr<CBlockTemplate> CreateNewBlock(const CScript& scriptPubKeyIn);
So test code in could updated in a separate the commit and then the overload could be deleted.
Rebased after #31122.
I changed coinbase_output_script from std::optional to having a default of OP_TRUE.
I split the main commit into three parts:
scriptPubKeyIn arg from CreateNewBlock, using a temporary overload to avoid test changes50+ *
51+ * Should only be used for tests, when the default doesn't suffice.
52+ *
53+ * Note that higher level code like the getblocktemplate RPC may omit the
54+ * coinbase transaction entirely. It's instead constructed by pool software
55+ * using fields like coinbaseaux, coinbasevalue, coinbaseaux and
coinbaseaux too many here?
813@@ -814,11 +814,9 @@ static RPCHelpMan getblocktemplate()
814 time_start = GetTime();
815
816 // Create new block
817- CScript scriptDummy = CScript() << OP_TRUE;
818- block_template = miner.createNewBlock(scriptDummy);
819+ block_template = miner.createNewBlock(CScript() << OP_TRUE);
In commit 59e75470591ce5a36500b649c987898a0c534d20:
Nit: I think we just directly call the new function here?
BlockCreateOptions field.
45@@ -44,8 +46,10 @@ void initialize_process_message()
46 /*chain_type=*/ChainType::REGTEST,
47 {.extra_args = {"-txreconciliation"}});
48 g_setup = testing_setup.get();
49+ BlockAssembler::Options options;
50+ options.coinbase_output_script = CScript() << OP_TRUE;
using node::BlockAssembler declaration entirely by just calling MineBlock(g_setup->m_node, {}).
Squashed the last two commits.
I dropped a few spurious (...)coinbase(_output)_script_pub_key renames from the test commit.
And dropped another unneeded OP_TRUE from tx_pool.
Providing a script for the coinbase transaction is only done in test code
and for CPU solo mining.
Production miners use the getblocktemplate RPC which omits the coinbase
transaction entirely from its block template, leaving it to external (pool)
software to construct it.
A coinbase script can still be passed via BlockCreateOptions instead.
A temporary overload is added so that the test can be modified in the
next commit.
This removes the temporary overload added in the previous commit.
Also drop unneeded custom coinbase output scripts.
2@@ -3,8 +3,8 @@
3 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
4
5 //! @file node/types.h is a home for public enum and struct type definitions
6-//! that are used by internally by node code, but also used externally by wallet
7-//! or GUI code.
8+//! that are used by internally by node code, but also used externally by wallet,
nit, was there before this PR, if you retouch, you can drop the excess “by”:
0//! that are used internally by node code, but also used externally by wallet,
ACK 52fd1511a774d1ff9e747b2ce88aa1fcb778ced8
Looks like a nice cleanup of the interface.
23@@ -24,9 +24,10 @@ COutPoint generatetoaddress(const NodeContext& node, const std::string& address)
24 {
25 const auto dest = DecodeDestination(address);
26 assert(IsValidDestination(dest));
27- const auto coinbase_script = GetScriptForDestination(dest);
28+ BlockAssembler::Options assembler_options;
29+ assembler_options.coinbase_output_script = {GetScriptForDestination(dest)};
The {} look like a typo, because both sides are already the same type (CScript).
Though, TIL that this is a neat way to enable Wnarrowing for the assignment operator.
https://godbolt.org/z/Pxz4KG9To
0int main() {
1 unsigned a;
2 a = {-1}; // error: narrowing conversion of '-1' from 'int' to 'unsigned int' [-Wnarrowing]
3}