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 copy-paste <!--meta-tag:bot-skip--> into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #34184 (mining: add cooldown to createNewBlock() immediately after IBD by Sjors)
• #33966 (refactor: disentangle miner startup defaults from runtime options by Sjors)
• #33965 (mining: fix -blockreservedweight shadows IPC option by Sjors)
• #33922 (mining: add getMemoryLoad() and track template non-mempool memory footprint by Sjors)
• #33819 (mining: getCoinbase() returns struct instead of raw tx 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.

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.

Are you still working on this?

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/

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

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

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.

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.

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

Rebased after #33745 due to import conflict in the test. Also fixed comment typo.

Trivial rebase after #34003.

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