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 third 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 4th 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 and adjusts, adjusting 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

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	ryanofsky
Concept ACK	w0xlt
Stale ACK	enirox001

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:

#34644 (mining: add submitBlock to IPC Mining interface by w0xlt)
#34020 (mining: add getTransactions(ByWitnessID) IPC methods 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 typos and grammar issues:

dummy extranonce -> dummy extraNonce [Typo/inconsistent casing: “extranonce” should match the documented term “extraNonce” for clarity.]

2026-03-25 14:15:58

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).

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.

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.

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

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().

480e3c84ff

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

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.

2544b73f0d

mining: always set dummy_extranonce at low heights

Unconditionally pad the coinbase transaction scriptSig with OP_0
for height up to 16, regardless of include_dummy_extranonce.

This avoids having to expose and document include_dummy_extranonce
to IPC clients.

The dummy OP_0 push is omitted from script_sig_prefix in the
getCoinbaseTx() result. Mining IPC clients are expected to construct
their own coinbase transaction and pass it to submitSolution().
They typically bring a real extraNonce or pool name which satisfies
the coinbase length requirement.

The next commit will drop include_dummy_extranonce entirely.

592d577f00

mining: drop include_dummy_extranonce, update test hashes

Now that the prior commit makes OP_0 padding unconditional at
heights <= 16, include_dummy_extranonce is only used by tests.

Drop the option entirely and regenerate the hardcoded blocks
and transaction hashes throughout the test suite. Also update the
assume_utxo regtest 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.

b45d184c65

mining: add coinbase clarifications in mining.h 0b0aa376ec

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

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

Reviews

Conflicts

LLM Linter (✨ experimental)