miner: don't needlessly append dummy OP

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.

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

No conflicts as of last run.

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.

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.

eac6135a5b

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.

c91ebececc

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.

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

Code Coverage & Benchmarks

Reviews

Conflicts