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 |
| Concept ACK | TheCharlatan |
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.
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.
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.
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
🐙 This pull request conflicts with the target branch and needs rebase.
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.
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.