miner: don’t needlessly append dummy OP_0 to bip34 #32420

1. 
   Sjors commented at 12:01 pm on May 5, 2025: member

   Our miner code adds OP_0 to the coinbase scriptSig in order to avoid triggering the bad-cb-length consensus error on test networks.

   This commit, like blocktools.py, limits that workaround to blocks 1 through 16 where it’s actually needed (OP1 through OP_16).

   Previously the coinbase transaction generated by our miner code was not used downstream, because the getblocktemplate RPC excludes it.

   Since the Mining IPC interface was introduced in #30200 we do expose this dummy coinbase transaction. In Stratum v2 several parts of it are communicated downstream, including the scriptSig.

   To avoid churning various hardcoded hashes in test files, this PR continues to add OP_0 for regtest. This can be changed if we ever need to change them anyway, such as in #32155.

   There’s a few places where the tests generate their own coinbase transactions. Those are modified to either match the new behavior, or a code comment is added.

2. 
   DrahtBot commented at 12:01 pm on May 5, 2025: contributor

   The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

   Code Coverage & Benchmarks

   For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32420.

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   Concept ACK	shahsb

   If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

   Conflicts

   Reviewers, this pull request conflicts with the following ones:

   • #32155 (miner: timelock the coinbase to the mined block’s height by darosior)

   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.

3. DrahtBot added the label Mining on May 5, 2025
4. in src/node/miner.cpp:166 in 9a6dc0db85 outdated

   160@@ -161,7 +161,12 @@ std::unique_ptr<CBlockTemplate> BlockAssembler::CreateNewBlock()
   161     coinbaseTx.vout.resize(1);
   162     coinbaseTx.vout[0].scriptPubKey = m_options.coinbase_output_script;
   163     coinbaseTx.vout[0].nValue = nFees + GetBlockSubsidy(nHeight, chainparams.GetConsensus());
   164-    coinbaseTx.vin[0].scriptSig = CScript() << nHeight << OP_0;
   165+    coinbaseTx.vin[0].scriptSig = CScript() << nHeight;
   166+    // IsTestChain() can be dropped if hardcoded block hashes in tests are regenerated
   167+    if (nHeight <= 16 || chainparams.IsTestChain()) {
   

   ---

   ---

   

   maflcko commented at 12:12 pm on May 5, 2025:

   nit: Would be good to limit this to regtest, assuming this is the only test network that “needs” it. Otherwise this can’t be tested outside of mainnet?

   ---

   

   Sjors commented at 12:28 pm on May 5, 2025:

   I’ll try to narrow it down to regtest.

   ---

   

   ajtowns commented at 1:08 pm on May 5, 2025:

   Match the comment to the updated code too?

   ---

   

   Sjors commented at 1:33 pm on May 5, 2025:

   Fixed
5. Sjors force-pushed on May 5, 2025
6. 
   Sjors commented at 1:03 pm on May 5, 2025: member

   I limited the exception to regtest. I also found two tests that for some reason implement their own coinbase construction code. I adjusted those for consistently with the first commit.

   ---

   CreateBlockChain is used by the utxo_snapshot fuzzer which relies on hardcoded block hashes, so I just added a comment instead of changing it.

7. Sjors force-pushed on May 5, 2025
8. miner: don't needlessly append dummy OP_0 to bip34

   Our miner code adds OP_0 to the coinbase scriptSig in order to avoid
   triggering the bad-cb-length consensus error on test networks.
   
   This commit, like blocktools.py, limits that workaround to blocks 1
   through 16 where it's actually needed (OP1 through OP_16).
   
   Previously the coinbase transaction generated by our miner code was
   not used downstream, because the getblocktemplate RPC excludes it.
   
   Since the Mining IPC interface was introduced in #30200 we do expose
   this dummy coinbase transaction. In Stratum v2 several parts of it
   are communicated downstream, including the scriptSig.

   6e2ac27b91
9. test: don't needlessly append dummy OP_0 to bip34

   Some tests implement their own miner code. Where possible, update
   those similar to the previous commit.

   ca8a52f4f6
10. Sjors force-pushed on May 5, 2025
11. 
    shahsb commented at 2:48 am on May 6, 2025: none

    Concept ACK

Contributors
Sjors DrahtBot maflcko ajtowns shahsb

Labels
Mining