miner: drop dummy extraNonce in coinbase scriptSig for templates requested via IPC

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

Our miner code adds OP_0 as a dummy extraNonce to the coinbase scriptSig. This has been in place since v0.1.0 and removing it entirely would trigger the bad-cb-length consensus error on test networks for blocks 1 through 16 (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.

This PR adds a include_dummy_extranonce option which is false by default, but for the time being all test code opts in to it. This avoids churning hardcoded block and transaction hashes.

The effect of having the default false for IPC callers is that we remove the dummy extraNonce from the coinbase scriptSig in block templates requested that way. This limits the scriptSig to what is essential for consensus (BIP34) and removes the need for external mining software to remove the dummy, or even ignore the scriptSig we provide and generate it some other way. This could cause problems if a future soft fork requires additional data to be committed here.

test/functional/interface_ipc.py is expanded to verify the new IPC behavior.

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:

#33819 (mining: add getCoinbase() by Sjors)
#33745 (mining: check witness commitment in submitBlock by Sjors)
#32468 (rpc: generateblock to allow multiple outputs by polespinasa)

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.

LLM Linter (✨ experimental)

Possible typos and grammar issues:

- /**\n+ * Whether to include and OP_0 as a dummy extraNonce in the template’s coinbase\n+ */\n -> “and” -> “an” [Correct article: “an OP_0” (singular noun) makes the sentence grammatical and clear.]

drahtbot_id_5_m

DrahtBot added the label Mining on May 5, 2025

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

Sjors force-pushed on May 5, 2025

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.

Sjors force-pushed on May 5, 2025

shahsb commented at 2:48 am on May 6, 2025: none

Concept ACK

DrahtBot added the label Needs rebase on May 9, 2025

ajtowns commented at 4:33 pm on May 10, 2025: contributor

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

Correct me if I’m wrong, please! I think what’s actually going on here is:

scriptSigs in the coinbase have had to be 2 bytes or more since before bitcoin’s git history began
this is perhaps because mainnet bitcoin blocks will usually require some extranonce stuff, and the coinbase tx’s scriptSig is a sensible place to do that
in any event, satoshi set the scriptSig up to contain a push of nBits and a push of the extranonce. this was changed to be time and extranonce (#505) and finally height and extranonce via BIP34 activation.
when BIP34 is applied to blocks 1-16, the coinbase is encoded as a 1-byte OP_1 through OP_16 (this is compliant with the “minimally encoded serialized CScript” part of the spec, though not the “first byte is number of bytes…” part).
this means the only blocks compliant with BIP34 that might potentially have a too short scriptSig are blocks 1 through 16
because mining regtest doesn’t require extranonce stuff, there’s no automatic reason to add extra bytes here, but the cb-length consensus check requires it anyway.
thus, when mining regtest blocks in the functional test framework, we explicitly add an OP_0 in blocktools.py:script_BIP34_coinbase_height for blocks 1 through 16. regtest was introduced a year after BIP 34, so there was never a question of “what did we do before BIP34 was enforced on regtest”
that code was introduced in #16363, at which point BIP34 wasn’t activated in regtest until block height 500; this was lowered to 2 in #16333 at which point the code was used

So I think it’s more fair to say that our miner code adds OP_0 as a dummy extraNonce, expecting it to be incremented as header nonce space runs out, which just never happens in unit tests and regtest.

In the current code, having a dummy extraNonce actually seems sensible to me – in real PoW contexts, we’d need a real extraNonce anyway, so this makes the test environment a little more similar to reality… So I feel a bit -0 on this as a consequence. I wonder if there isn’t a way to have this work for stratumv2 without changing the way the existing code works?

Would it work for the stratumv2 interface (the part of it inside bitcoin core?) to just recognise we supply a dummy extra nonce, and drop it? Even for the first 16 blocks, sv2 miners that supply a non-empty extraNonce of their own, or that include a pool-signature in the coinbase will pass the cb-length check. And after the first 16 blocks, it’s not an issue at all.

Sjors commented at 2:04 pm on May 12, 2025: member

Rebased after #32155.

So I think it’s more fair to say that our miner code adds OP_0 as a dummy extraNonce

Possibly, but extraNonce seems like an implementation detail that should be left to miners. E.g. the Stratum v2 spec defines how to use it here: https://github.com/stratum-mining/sv2-spec/blob/main/05-Mining-Protocol.md

It doesn’t belong in a block template imo.

I wonder if there isn’t a way to have this work for stratumv2 without changing the way the existing code works?

The Template Distribution protocol defines a message NewTemplate which has a coinbase_prefix field, described as:

Up to 8 bytes (not including the length byte) which are to be placed at the beginning of the coinbase field in the coinbase transaction

https://github.com/stratum-mining/sv2-spec/blob/main/07-Template-Distribution-Protocol.md#72-newtemplate-server---client

In the Job Declaration Protocol (which the node doesn’t play a role in) coinbase_tx_prefix is defined as:

Serialized bytes representing the initial part of the coinbase transaction (not including extranonce)

Would it work for the stratumv2 interface … to just recognise we supply a dummy extra nonce, and drop it

Yes but this would be a foot gun if a future soft fork requires an additional commitment. It’s also up to every consumer of our Mining interface to implement that (correctly), not just the one I wrote.

We could implement it somewhere between the mining code and interface, so at least interface users don’t have to deal with this. But currently BlockAssembler::CreateNewBlock() is called pretty much directly without further processing.

Sjors force-pushed on May 12, 2025

DrahtBot removed the label Needs rebase on May 12, 2025

ajtowns commented at 2:55 pm on May 12, 2025: contributor

It doesn’t belong in a block template imo.

Right, but we currently don’t include it in a block template either, so that’s (currently) fine.

Would it work for the stratumv2 interface … to just recognise we supply a dummy extra nonce, and drop it

Yes but this would be a foot gun if a future soft fork requires an additional commitment. It’s also up to every consumer of our Mining interface to implement that (correctly), not just the one I wrote.

I think it’s more likely that any additional commitments required by future soft forks will be in the coinbase tx’s outputs because the coinbase scriptSig’s limited to 100 bytes. The segwit commitment output is designed to allow for this, so additional outputs aren’t needed; signet makes use of this ability for the block signature.

We could implement it somewhere between the mining code and interface, so at least interface users don’t have to deal with this. But currently BlockAssembler::CreateNewBlock() is called pretty much directly without further processing.

Yeah, this was what I was thinking. Maybe adding something like:

 0struct CBlockTemplate
 1{
 2    CBlock block;
 3
 4    std::span<unsigned char> CoinbaseScriptSigPrefix()
 5    {
 6       if (block.vtx.size() == 0 || block.vtx[0].vin.size() == 0 || block.vtx[0].vin[0].scriptSig.size() == 0) return {};
 7       // our scriptSig includes a dummy extraNonce. Drop it here.
 8       std::span<unsigned char> r{block.vtx[0].vin[0].scriptSig};
 9       return r.first(r.size()-1);
10    }
11    ...
12};

and whatever getblocktemplate-ish api we introduce for stratumv2 calls that function to provide the information rather than dumping the scriptSig directly.

Sjors commented at 3:33 pm on May 12, 2025: member

I also don’t think CBlockTemplate should have an extraNonce. Instead it could be added by code that actually does the mining, such as the GenerateBlock block method in rpc/mining.cpp. That seems like a cleaner separation of concerns between template construction and mining.

Although CheckBlock() also needs it to be present for these early blocks, unless we pass an argument in to skip the bad-cb-length check.

achow101 commented at 2:47 pm on October 22, 2025: member

Are you still working on this?

Sjors requested review from ajtowns on Oct 31, 2025

ajtowns commented at 7:11 pm on October 31, 2025: contributor

I also don’t think CBlockTemplate should have an extraNonce.

I think a block should (almost) always have an extraNonce – whether for mainnet to provide sufficient work, or for early blocks on regtest/signet to avoid bad-cb-length, or for test net blocks to more closely simulate mainnet behaviour. Whether the template should also include an extraNonce depends on the user of the template – if they want a template they can just apply work to via nNonce, without further fussing about (like we do in our unit tests), then the template should also include an extraNonce, but otherwise it doesn’t need to.

The current commit descriptions in this PR claim “Our miner code adds OP_0 to the coinbase scriptSig in order to avoid triggering the bad-cb-length consensus error on test networks.” – I don’t think that’s really reflective of the intent behind that code though. It adds an OP_0 because in blocks (not templates) an extraNonce is expected, but only a dummy value is needed. That does avoid the bad-cb-length, but the motivation for it being there is just that the coinbase has always had pushes of two values in it; originally nBits and extraNonce, then nTime and extraNonce, now nHeight and extraNonce.

So rather than special casing early blocks and regtest, how about just making it an explicit option, whether defaulting to true (ie, current behaviour) or false? eg: https://github.com/ajtowns/bitcoin/commits/202510-miner-extranonce/

ajtowns removed review request from ajtowns on Oct 31, 2025

Sjors commented at 7:35 pm on November 1, 2025: member

Indeed the bad-cb-length check has been around since the first commit, of course without a documented reason.

https://github.com/bitcoin/bitcoin/blob/e071a3f6c06f41068ad17134189a4ac3073ef76b/main.h#L454-L457

There was also no documented reason for adding nBits to the coinbase scriptSig, before the extraNonce. I just asked: https://bitcoin.stackexchange.com/questions/129167/why-did-satoshi-put-nbits-in-the-coinbase-scriptsig

https://github.com/bitcoin/bitcoin/blob/e071a3f6c06f41068ad17134189a4ac3073ef76b/main.cpp#L2236-L2244

But as you point out in your historical overview, nBits was swapped for nTime, and then later for the height which became a consensus requirement.

So I agree it makes sense to call it extraNonce and not “dummy zero”. Adding an option include_dummy_extranonce seems a bit overkill, but I guess it’s fine.

[doc] Update comments on dummy extraNonces in tests f514f1aea2

Sjors force-pushed on Nov 3, 2025

Sjors renamed this:
~~miner: don't needlessly append dummy OP_0 to bip34~~
miner: drop dummy extraNonce in coinbase scriptSig for templates requested via IPC
on Nov 3, 2025

Sjors commented at 9:01 am on November 3, 2025: member

I switched to the approach of adding an include_dummy_extranonce, having the tests set it to true while IPC calls use the default false. Expanded test/functional/interface_ipc.py to demonstrate it.

Sjors force-pushed on Nov 3, 2025

[miner] omit dummy extraNonce via IPC

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.

This commit removes the dummy extraNonce from the coinbase scriptSig
in block templates requested via IPC. This limits the scriptSig
to what is essential for consensus (BIP34) and removes the need for
external mining software to remove the dummy, or even ignore
the scriptSig we provide and generate it some other way. This
could cause problems if a future soft fork requires additional
data to be committed here.

A test is added to verify the new IPC behavior.

It achieves this by introducing an include_dummy_extranonce
option which defaults to false with all test code updated to
set it to true. Because this option is not exposed via IPC,
callers will no longer see it.

The caller needs to ensure that for blocks 1 through 16
they pad the scriptSig in order to avoid bad-cb-length.

Co-authored-by: Anthony Towns <aj@erisian.com.au>

b03feedadb

Sjors force-pushed on Nov 3, 2025

Sjors commented at 10:10 am on November 3, 2025: member

Looks like fuzz/process_message.cpp and fuzz/process_messages.cpp were missing include_dummy_extranonce = true.

ajtowns requested review from ajtowns on Nov 8, 2025

DrahtBot added the label Needs rebase on Nov 12, 2025

DrahtBot commented at 5:10 pm on November 12, 2025: contributor

🐙 This pull request conflicts with the target branch and needs rebase.

miner: drop dummy extraNonce in coinbase scriptSig for templates requested via IPC #32420

Code Coverage & Benchmarks

Reviews

Conflicts

LLM Linter (✨ experimental)