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.

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.

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.

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 (Unique coinbase: Fixes #482 #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 test: Add test for BIP30 duplicate tx #16363, at which point BIP34 wasn't activated in regtest until block height 500; this was lowered to 2 in test: Set BIP34Height = 2 for regtest #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.

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.

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:

struct CBlockTemplate
{
    CBlock block;

    std::span<unsigned char> CoinbaseScriptSigPrefix()
    {
       if (block.vtx.size() == 0 || block.vtx[0].vin.size() == 0 || block.vtx[0].vin[0].scriptSig.size() == 0) return {};
       // our scriptSig includes a dummy extraNonce. Drop it here.
       std::span<unsigned char> r{block.vtx[0].vin[0].scriptSig};
       return r.first(r.size()-1);
    }
    ...
};

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

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.