mining: always pad scriptSig at low heights, drop include_dummy

Sjors commented at 8:52 AM on March 19, 2026: member

Blocks 0-16 on any new chain require mining code to be careful not to violate the bad-cb-length rule, which states the coinbase transaction scriptSig must be at least 2 bytes.

Our mining code deals with that by padding the scriptSig with a 0 extraNonce. It does this for every height. As a result IPC clients would get an unnecessary 0 in the scriptSigPrefix field of CoinbaseTx. #32420 fixed that by introducing a include_dummy_extranonce option in BlockCreateOptions and turning that off for IPC clients.

A minor issue was missed though: createNewBlock() now fails with bad-cb-length. An easy workaround is to use the generate RPC for the first 16 blocks, as demonstrated in the 2nd commit.

The real fix is to have the miner code always pad the scriptSig at lower heights, but to not include that in the scriptSigPrefix field of CoinbaseTx (introduced in #33819). This is what the 3rd commit implements.

Now that we set scriptSigPrefix independent of what our internal miner code does - to get past CheckBlock() - the original motivation for include_dummy_extranonce goes away and we can just drop it entirely. The last commit drops it, while the 4th commit adjusts the tests and hardcoded block and assume utxo hashes.

This last change does not break IPC clients, because include_dummy_extranonce was never exposed in mining.capnp.

Instead of adjusting the hardcoded hashes, an alternative approach would be to just always pad the scriptSig internally, since we exclude the padding from scriptSigPrefix anyway. However, IPC clients can also call getBlock() to get the raw block and might be confused about the difference. The miner code is also easier to understand if we limit the exception (coinbase_tx.script_sig_prefix != coinbaseTx.vin[0].scriptSig) to nHeight <= 16, where the explanation is based purely on consensus rules rather than historical test suite reasons.

The first two commits are preperation test changes:

extract assert_capnp_failed helper for macOS (also part of #34727)
use script_BIP34_coinbase_height in IPC mining test (existing code in interface_ipc_mining.py was incorrect for low height

Fixes #35126

DrahtBot added the label Mining on Mar 19, 2026

DrahtBot commented at 8:52 AM on March 19, 2026: contributor

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	sedited
Concept ACK	w0xlt
Stale ACK	enirox001, ryanofsky

If your review is incorrectly listed, please copy-paste <code></code> into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#34644 (mining: add submitBlock to IPC Mining interface by w0xlt)
#34020 (mining: add getTransactions(ByWitnessID) IPC methods by Sjors)
#33966 (refactor: disentangle miner startup defaults from runtime options by Sjors)
#33421 (node: add BlockTemplateCache by ismaelsadeeq)
#32468 (rpc: generateblock to allow multiple outputs by polespinasa)
#30437 (ipc: add bitcoin-mine test program by ryanofsky)

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 places where comparison-specific test macros should replace generic comparisons:

[test/functional/interface_ipc_mining.py] assert template is not None -> use assert_not_equal(template, None) or a direct null-check helper if available.

2026-04-27 10:18:28

Sjors commented at 8:54 AM on March 19, 2026: member

Given the easy workaround, I don't think it's necessary to backport this. The only downside of that workaround, and the reason I noticed the issue while working on another project, is that the generate RPC methods can't be used to create a deterministic regtest chain. With createNewBlock() -> submitSolution() the IPC client can pick specific (cached) nonce and nTime values.

Historical note: the introduction of a CoinbaseTx struct for IPC clients in #33819 probably made the include_dummy_extranonce approach in #32420 unnecessary, since we can now give clients a different scriptSigPrefix than what we use in the block template in order to get past CheckBlock().

maflcko commented at 9:40 AM on March 19, 2026: member

A nice benefit of this change is that it's now possible to get rid of the include_dummy_extranonce option entirely, without breaking IPC clients because it's not exposed in mining.capnp, but it would involve significant test churn

I don't think test churn is a reason to leave deprecated/unused/confusing stuff in the real code. Test code should always follow the real code, not the other way round.

Sjors commented at 9:43 AM on March 19, 2026: member

@maflcko I'll push a commit to this PR to drop it entirely, updating the hashes where needed.

fanquake commented at 9:49 AM on March 19, 2026: member

without breaking IPC clients

Until the interface stabilizes, and is marked as such, "breaking clients" should be much less of an issue? I don't think we should have an interface that is marked as brand new/experimental, but then also not have the freedom to iterate/change it, because we are also half-pretending that it is meant to be stable/backwards compatible.

Sjors commented at 9:55 AM on March 19, 2026: member

"breaking clients" should be much less of an issue?

Indeed, I just observed that it's nice that we never exposed include_dummy_extranonce to clients in the first place, so dropping it is an internal change to Bitcoin Core that has no impact on IPC clients. But if it was a breaking change, we could indeed just do that as long as the interface is marked experimental.

Sjors force-pushed on Mar 19, 2026

Sjors renamed this:
~~mining: always set dummy_extranonce at low heights~~
mining: always pad scriptSig at low heights, drop include_dummy_extranonce
on Mar 19, 2026

Sjors commented at 12:33 PM on March 19, 2026: member

The ancestor commits failure seems unrelated, but the fuzzer failures seem relevant. Will investigate.

Sjors marked this as a draft on Mar 19, 2026

DrahtBot added the label CI failed on Mar 19, 2026

DrahtBot commented at 1:00 PM on March 19, 2026: contributor

🚧 At least one of the CI tasks failed. Task Windows native, fuzz, VS: https://github.com/bitcoin/bitcoin/actions/runs/23294168432/job/67737082288 LLM reason (✨ experimental): Fuzz test failure: fuzz.exe crashed on utxo_total_supply due to an assertion mismatch (scriptSig != duplicate_coinbase_script, utxo_total_supply.cpp:161).

<details><summary>Hints</summary>

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

</details>

Sjors force-pushed on Mar 19, 2026

Sjors commented at 1:30 PM on March 19, 2026: member

The fuzzer was still adding the dummy extraNonce, should be fixed now.

DrahtBot removed the label CI failed on Mar 19, 2026

Sjors marked this as ready for review on Mar 19, 2026

in src/node/miner.cpp:192 in d6f9ab4d08

 188 | +    // Set script_sig_prefix here, so IPC mining clients are not affected by
 189 | +    // the optional scriptSig padding below. They provide their own extraNonce,
 190 | +    // and in a typical setup a pool name or realistic extraNonce already makes
 191 | +    // the scriptSig long enough.
 192 | +    coinbase_tx.script_sig_prefix = coinbaseTx.vin[0].scriptSig;
 193 | +    if (nHeight <= 16 || m_options.include_dummy_extranonce) {

enirox001 commented at 2:43 PM on March 21, 2026:

In commit "mining: always set dummy_extranonce at low heights" (https://github.com/bitcoin/bitcoin/pull/34860/changes/d6f9ab4d082dc12a9df976668689d7c75cfe34cd):

The condition nHeight <= 16 technically includes height 0. But height 0 is caught by the Assert below. Would it be cleaner to write if (nHeight <= 16 && nHeight > 0)? or move the Assert above the condition?

Sjors commented at 10:24 AM on March 23, 2026:

I think they're separate issues:

if we ever supported regtest or signets with a custom genesis block, and if we decided such custom genesis blocks are subject to validation, then the bad-cb-length would apply. Either way it wouldn't cause a crash if we padded it.
our mining code is not expected to be used on a genesis block. The nHeight - 1 logic below would crash if that changes. So I think the assert is in the right place to make that that clear.

in src/node/miner.cpp:189 in d6f9ab4d08 outdated

 183 | @@ -184,14 +184,19 @@ std::unique_ptr<CBlockTemplate> BlockAssembler::CreateNewBlock()
 184 |      // increasing its length would reduce the space they can use and may break
 185 |      // existing clients.
 186 |      coinbaseTx.vin[0].scriptSig = CScript() << nHeight;
 187 | -    if (m_options.include_dummy_extranonce) {
 188 | +    // Set script_sig_prefix here, so IPC mining clients are not affected by
 189 | +    // the optional scriptSig padding below. They provide their own extraNonce,
 190 | +    // and in a typical setup a pool name or realistic extraNonce already makes

enirox001 commented at 2:48 PM on March 21, 2026:

In commit "mining: always set dummy_extranonce at low heights" (https://github.com/bitcoin/bitcoin/pull/34860/changes/d6f9ab4d082dc12a9df976668689d7c75cfe34cd):

The comment says "in a typical setup a pool name or realistic extraNonce already makes the scriptSig long enough" but this puts the responsibility entirely on the client at heights ≤ 16. I tested this by removing the extra nonce in run_low_height_test() and it causes submitSolution() to silently return False with no explanation. Should we at minimum document in the code or the IPC interface that at heights ≤ 16 the client must provide at least 1 byte of extra nonce?

Sjors commented at 10:30 AM on March 23, 2026:

#34672 adds a failure reason to submitSolution()

I'm tempted to not document this rule so implementers actually have to handle failure correctly to learn it :-)

enirox001 commented at 10:49 AM on March 23, 2026:

Good to know #34672 addresses this. Fair point on not documenting it, clients should handle failures regardless.

Sjors commented at 11:04 AM on March 23, 2026:

Documented in e5ec15d3a229fe933313b46e5e2af9cbc54c9247.

in src/node/miner.cpp:191 in d6f9ab4d08 outdated

 183 | @@ -184,14 +184,19 @@ std::unique_ptr<CBlockTemplate> BlockAssembler::CreateNewBlock()
 184 |      // increasing its length would reduce the space they can use and may break
 185 |      // existing clients.
 186 |      coinbaseTx.vin[0].scriptSig = CScript() << nHeight;
 187 | -    if (m_options.include_dummy_extranonce) {
 188 | +    // Set script_sig_prefix here, so IPC mining clients are not affected by
 189 | +    // the optional scriptSig padding below. They provide their own extraNonce,
 190 | +    // and in a typical setup a pool name or realistic extraNonce already makes
 191 | +    // the scriptSig long enough.
 192 | +    coinbase_tx.script_sig_prefix = coinbaseTx.vin[0].scriptSig;

enirox001 commented at 3:02 PM on March 21, 2026:

In commit "mining: always set dummy_extranonce at low heights" (https://github.com/bitcoin/bitcoin/pull/34860/changes/d6f9ab4d082dc12a9df976668689d7c75cfe34cd):

At heights ≤ 16, getBlock() and getCoinbaseTx() return different scriptSig data since script_sig_prefix is snapshotted here before the OP_0 padding is added. I verified this in the test. getBlock() returns 5100 ([OP_1, OP_0]) while scriptSigPrefix returns 51 ([OP_1] only). Would an IPC client comparing both would be confused? Should this discrepancy be documented in the IPC interface or the code?

Sjors commented at 10:34 AM on March 23, 2026:

They would indeed be confused, but only for these lower heights.

Might indeed be worth documenting in getBlock(), more generally that it contains a dummy coinbase that should not be used and may not match getCoinbaseTx()

Sjors commented at 11:04 AM on March 23, 2026:

Documented in e5ec15d3a229fe933313b46e5e2af9cbc54c9247.

ryanofsky commented at 6:05 PM on April 22, 2026:

In commit "mining: always pad script_sig_prefix at heights <= 16" (26d28e3cfdcf77d5b16465cecc6a5db49dd3a8ad)

Change looks good and thanks for the updates! I think the commit message saying "always pad script_sig_prefix" is misleading since actually this never pads it. I think i'd rewrite it to focus on the main change and list the other changes later like:

mining: pad coinbase to fix createNewBlock at heights <=16

Since [#32420](/bitcoin-bitcoin/32420/), createNewBlock has thrown `bad-cb-length` errors when called at
low block heights because `OP_0` padding stopped being added to coinbase
transactions. (#32420 did add an `include_dummy_extranonce` option which could
bypass this, but it was not exposed to IPC clients.) Fix the problem by padding
coinbase transactions with `OP_0` when necessary to produce valid blocks.

Additionally this commit stops adding `OP_0` padding to the template
`script_sig_prefix` field when `include_dummy_extranonce` is true. This is safe
because non-IPC clients don't use this field, and IPC clients could never set
the option to true, and are expected to add their own nonces in any case.

This also improves documentation about the `script_sig_prefix` field and
`getCoinbaseTx` method.

Sjors commented at 11:36 AM on April 23, 2026:

Taken

enirox001 commented at 3:19 PM on March 21, 2026: contributor

Concept ACK. Changes look good to me. Left a few questions about some of the changes made.

I also tested that the boundary works correctly across all heights up to 17. At heights 1-16 the discrepancy between getBlock() and scriptSigPrefix exists as expected, and disappears exactly at height 17

w0xlt commented at 8:07 AM on March 22, 2026: contributor

Concept ACK

Sjors force-pushed on Mar 23, 2026

DrahtBot added the label CI failed on Mar 23, 2026

DrahtBot removed the label CI failed on Mar 23, 2026

enirox001 commented at 6:57 PM on March 23, 2026: contributor

ACK 647407c - Thanks for applying the documentation suggestions.

DrahtBot added the label Needs rebase on Mar 24, 2026

Sjors force-pushed on Mar 25, 2026

Sjors commented at 1:41 PM on March 25, 2026: member

Rebased after #34727, which included the original variant of 7881e298c7491f070e1c10318b84655a894d8889.

Sjors marked this as a draft on Mar 25, 2026

Sjors force-pushed on Mar 25, 2026

Sjors commented at 2:16 PM on March 25, 2026: member

It also introduced a new make_mining_ctx method.

Sjors marked this as ready for review on Mar 25, 2026

DrahtBot added the label CI failed on Mar 25, 2026

DrahtBot removed the label Needs rebase on Mar 25, 2026

DrahtBot removed the label CI failed on Mar 25, 2026

ryanofsky referenced this in commit 6cb64bb963 on Mar 31, 2026

in test/functional/interface_ipc_mining.py:452 in 2544b73f0d

 448 | @@ -412,6 +449,9 @@ def run_test(self):
 449 |          self.run_coinbase_and_submission_test()
 450 |          self.run_ipc_option_override_test()
 451 |  
 452 | +        # Needs to run last

ryanofsky commented at 7:32 PM on April 6, 2026:

In commit "test: bad-cb-length for createNewBlock() at low heights" (2544b73f0d9db1736c6fee70351ad73a3fd3b40d)

Maybe add it needs to run last because it resets the chain

ryanofsky approved

ryanofsky commented at 8:55 PM on April 6, 2026: contributor

Code review ACK 0b0aa376ec51f86c9cf3fb170ccde7abd64db353.

I think the end state here is an improvement, but the PR feels a bit broader than it needs to be, and I wonder if it would be nicer to split it up.

As I understand it, the PR is doing three things:

It removes the createNewBlock dummy_extranonce option introduced in #32420. That option created a footgun because createNewBlock could build invalid blocks at heights <= 16 when the option was false. Over IPC there was no way to set dummy_extranonce=true, and no way to disable test_block_validity, so createNewBlock was effectively unusable below height 16 because it would always throw TestBlockValidity exceptions.
It implements a different fix for the usability problem #32420 was trying to address. Instead of returning a coinbase_template.script_sig_prefix that may contain a dummy OP_0 which clients need to strip off before adding their own extra nonce, it now always returns the unpadded prefix.
It changes the scriptSig << OP_0 behavior so the dummy OP_0 is only appended at heights <= 16 instead of at all heights. This is externally visible beyond IPC, including through RPC, and requires a number of hash updates.

I think all three changes are reasonable, but (3) seems separate from fixing the IPC usability problem. and not strictly necessary, so would seem good to move it into separate commit at the end, so it is not mixed in with changes in (1) and (2).

I also think it would be good if (2) were the first non-test change in the series, and if the script_sig_prefix documentation were updated in the same commit to clarify that clients may need to add extra data at low heights for the resulting scriptSig to be valid, not just that they may append extra data in general.

DrahtBot requested review from enirox001 on Apr 6, 2026

test: use script_BIP34_coinbase_height in IPC mining test

Needed in a later commit to correctly derive the BIP34 prefix
for heights <= 16.

Add a padding parameter to script_BIP34_coinbase_height() that
controls whether the OP_0 dummy extranonce is appended for
heights <= 16.

Use this helper with padding=False in the IPC mining test's
build_coinbase_test().

a2682ed95c

Sjors force-pushed on Apr 22, 2026

Sjors commented at 9:54 AM on April 22, 2026: member

@ryanofsky I rewrote the commit history (first two test commits are the same, just rebased and with your comment added).

Sjors force-pushed on Apr 22, 2026

Sjors commented at 10:01 AM on April 22, 2026: member

Replaced rmtree(node.chain_path) with self.cleanup_folder as requested by the linter.

DrahtBot added the label CI failed on Apr 22, 2026

DrahtBot removed the label CI failed on Apr 22, 2026

in src/test/fuzz/utxo_total_supply.cpp:110 in 13cb2989d0

 106 | @@ -107,8 +107,13 @@ FUZZ_TARGET(utxo_total_supply)
 107 |      // Assuming that the fuzzer will mine relatively short chains (less than 200 blocks), we want the duplicate coinbase to be not too high.
 108 |      // Up to 300 seems reasonable.
 109 |      int64_t duplicate_coinbase_height = fuzzed_data_provider.ConsumeIntegralInRange(0, 300);
 110 | -    // Always pad with OP_0 at the end to avoid bad-cb-length error
 111 | -    const CScript duplicate_coinbase_script = CScript() << duplicate_coinbase_height << OP_0;
 112 | +    // Build the scriptSig exactly as CreateNewBlock does for the given height:

ryanofsky commented at 6:12 PM on April 22, 2026:

In commit "mining: always pad script_sig_prefix at heights <= 16" (26d28e3cfdcf77d5b16465cecc6a5db49dd3a8ad)

Is it important this is building the scriptSig in the exact same way as CreateNewBlock? If not maybe say "similarly to" instead of "exactly as" to avoid giving this impression

Sjors commented at 11:36 AM on April 23, 2026:

It's not, it just needs to avoid avoid bad-cb-length error, updated the comment.

ryanofsky approved

ryanofsky commented at 6:42 PM on April 22, 2026: contributor

Code review ACK 48c32df97f7a6197e912e22eb7de3e2780d89404. Nice fix, and everything seems pretty clearly spelled out in the PR description.

I do think the alternate approach of always adding padding instead of only adding it at low heights would be a good fix as well, since it would restore previous behavior prior to #32420 and simplify this PR by avoiding the need to update hashes, pruning offsets, and feature_assumetxo data offsets. Both both approaches seem reasonable.

Sjors force-pushed on Apr 23, 2026

in test/functional/interface_ipc_mining.py:441 in f6e546b89b

 436 | +                block.vtx[0] = coinbase
 437 | +                block.hashMerkleRoot = block.calc_merkle_root()
 438 | +                block.solve()
 439 | +                submitted = (await template.submitSolution(ctx, block.nVersion, block.nTime, block.nNonce, coinbase.serialize())).result
 440 | +                assert_equal(submitted, True)
 441 | +                assert_equal(node.getblockcount(), 1)

sedited commented at 10:21 AM on April 26, 2026:

Nit, maybe do this for 17 iterations to exercise the boundary condition?

Sjors commented at 10:18 AM on April 27, 2026:

Done (required some additional test changes to limit churn between 55c3104a739252bee86b77a880f03227079988bc and 3ed62f3ff0f8f2a9a8cd42b08d9d1d5d0bc6a322)

sedited approved

sedited commented at 10:22 AM on April 26, 2026: contributor

ACK f6e546b89bd4ebeca9091cdc7cce389d2751fe9a

DrahtBot requested review from ryanofsky on Apr 26, 2026

test: bad-cb-length for createNewBlock() at low heights

On new regtest / signet chains (heights <= 16), createBlock() fails
internally with bad-cb-length. Since mining.capnp does not expose
include_dummy_extranonce, IPC clients can't work around this.

Add a functional test to illustrate this issue and use RPC to
work around it. The next commit introduces a fix.

55c3104a73

mining: pad coinbase to fix createNewBlock at heights <=16

Since #32420, createNewBlock has thrown `bad-cb-length` errors when called at
low block heights because `OP_0` padding stopped being added to coinbase
transactions. (#32420 did add an `include_dummy_extranonce` option which could
bypass this, but it was not exposed to IPC clients.) Fix the problem by padding
coinbase transactions with `OP_0` when necessary to produce valid blocks.

Additionally this commit stops adding `OP_0` padding to the template
`script_sig_prefix` field when `include_dummy_extranonce` is true. This is safe
because non-IPC clients don't use this field, and IPC clients could never set
the option to true, and are expected to add their own nonces in any case.

This also improves documentation about the `script_sig_prefix` field and
`getCoinbaseTx` method.

3ed62f3ff0

mining: only pad with OP_0 at heights <= 16

Drop the include_dummy_extranonce branch from the OP_0 padding
condition in CreateNewBlock(), so that the dummy extraNonce is
only appended when consensus actually requires it (heights <= 16,
where the BIP34 height push alone would yield a 1-byte scriptSig
and trigger bad-cb-length).

The include_dummy_extranonce option struct field is now unused by
the miner and is removed in the next commit. Callers still set it,
so that this commit compiles.

Regenerate the hardcoded coinbase / block hashes throughout the
unit and functional test suites and update the regtest assumeutxo
snapshot in chainparams.

Additional side-effects:

- Without the dummy extranonce, coinbase scriptSigs are 1 byte
  shorter at heights > 16, making every block 1 byte smaller.
  This shifts where block files wrap and therefore where pruning
  boundaries land.

- feature_assumeutxo malleation cases:
  - case 1: error message changes due to UTXO reordering, similar
            to 8f2078af6a55448c003b3f7f3021955fbb351caa
  - case 4: the corruption byte is swapped from \x82 to \x83
            because \x82 happened to be the actual value at that
            offset in the new snapshot.

cb6e85a53f

mining: drop unused include_dummy_extranonce option

The previous commit made CreateNewBlock() ignore the
include_dummy_extranonce flag (OP_0 is now appended only at heights
<= 16 to satisfy bad-cb-length, regardless of caller-supplied options).

Remove the now-unused field from BlockCreateOptions and clean up all
the call sites that still set it (RPC, bench, fuzz, test setups).

c4c701ca3e

Sjors force-pushed on Apr 27, 2026

sedited approved

sedited commented at 11:39 AM on April 27, 2026: contributor

Re-ACK c4c701ca3efa5ed4d0c4207cd8a4d9b950c5d58b

mining: always pad scriptSig at low heights, drop include_dummy_extranonce #34860

Reviews

Conflicts

LLM Linter (✨ experimental)