From 788f390fb09a5e3d1dd406413b9e0acf72466d1b Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Sun, 17 May 2026 11:21:20 -0400 Subject: [PATCH] Merge bitcoin/bitcoin#34860: mining: always pad scriptSig at low heights, drop include_dummy_extranonce 8544537f41df568e6e1cb94eeab9e699b9190427 mining: drop unused include_dummy_extranonce option (Sjors Provoost) 58eeab790d9825a777f907e3e912a2da78cbc76d mining: only pad with OP_0 at heights <= 16 (Sjors Provoost) 00d22328b0560bf9773417495123ae68bb89e4d1 mining: pad coinbase to fix createNewBlock at heights <=16 (Sjors Provoost) 605ff37403f0b50c5664bb4e2d6eaf84022d53e4 test: bad-cb-length for createNewBlock() at low heights (Sjors Provoost) 1966621b76885257b4b4e44aab4712e9f84313e6 test: refactor IPC mining test to use script_BIP34_coinbase_height (Sjors Provoost) Pull request description: 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 ACKs for top commit: ryanofsky: Code review ACK 8544537f41df568e6e1cb94eeab9e699b9190427. Just rebased to fix silent conflict and applied some minor suggestions since last review. As part of rereviewing I left some more minor suggestions that are fine to ignore. sedited: Re-ACK 8544537f41df568e6e1cb94eeab9e699b9190427 Tree-SHA512: a01d48842bf4bcc1a9c51a89ef9d750766db7d04edb4dcd6b3a8bf195c6b4fa07445256a49367ff0db00ab489a52a3d7ff6a5c3ab9290ecb1fcb82f532552e9b (cherry picked from commit 7802e578c3f1e9a5d9b57fb003349d0e032bb43b) --- src/bench/block_assemble.cpp | 1 - src/interfaces/mining.h | 7 ++++++- src/kernel/chainparams.cpp | 1 + src/node/miner.cpp | 13 ++++++++----- src/node/types.h | 8 +++----- src/test/fuzz/cmpctblock.cpp | 1 - src/test/fuzz/package_eval.cpp | 1 - src/test/fuzz/process_message.cpp | 1 - src/test/fuzz/process_messages.cpp | 1 - src/test/fuzz/tx_pool.cpp | 2 -- src/test/fuzz/utxo_total_supply.cpp | 9 ++++++--- src/test/miner_tests.cpp | 4 ---- src/test/peerman_tests.cpp | 1 - src/test/testnet4_miner_tests.cpp | 1 - src/test/util/mining.cpp | 2 -- src/test/util/setup_common.cpp | 3 +-- src/test/validation_block_tests.cpp | 2 -- src/test/validation_tests.cpp | 1 + 18 files changed, 26 insertions(+), 33 deletions(-) diff --git a/src/bench/block_assemble.cpp b/src/bench/block_assemble.cpp index 702f2c093668..297465be80f5 100644 --- a/src/bench/block_assemble.cpp +++ b/src/bench/block_assemble.cpp @@ -30,7 +30,6 @@ static void AssembleBlock(benchmark::Bench& bench) witness.stack.push_back(WITNESS_STACK_ELEM_OP_TRUE); BlockAssembler::Options options; options.coinbase_output_script = P2WSH_OP_TRUE; - options.include_dummy_extranonce = true; // Collect some loose transactions that spend the coinbases of our mined blocks constexpr size_t NUM_BLOCKS{200}; diff --git a/src/interfaces/mining.h b/src/interfaces/mining.h index f4c42e204a7c..943d34e75d9b 100644 --- a/src/interfaces/mining.h +++ b/src/interfaces/mining.h @@ -34,7 +34,8 @@ class BlockTemplate virtual ~BlockTemplate() = default; virtual CBlockHeader getBlockHeader() = 0; - // Block contains a dummy coinbase transaction that should not be used. + // Block contains a dummy coinbase transaction that should not be used and + // it may not match a transaction constructed from getCoinbaseTx(). virtual CBlock getBlock() = 0; // Fees per transaction, not including coinbase transaction. @@ -64,6 +65,10 @@ class BlockTemplate * @note unlike the submitblock RPC, this method does NOT add the * coinbase witness automatically. * + * @note for heights <= 16, the BIP34 height push in getCoinbaseTx().script_sig_prefix + * is only one byte long, so the coinbase scriptSig needs at least + * one additional byte of data to avoid bad-cb-length. + * * @returns if the block was processed, does not necessarily indicate validity. * * @note Returns true if the block is already known, which can happen if diff --git a/src/kernel/chainparams.cpp b/src/kernel/chainparams.cpp index 5d96e5e3bb0d..b63ce1119e7a 100644 --- a/src/kernel/chainparams.cpp +++ b/src/kernel/chainparams.cpp @@ -537,6 +537,7 @@ class CRegTestParams : public CChainParams m_is_mockable_chain = true; + chainTxData = ChainTxData{ .nTime = 0, .tx_count = 0, diff --git a/src/node/miner.cpp b/src/node/miner.cpp index 836b828b5c42..7fc19deeb79a 100644 --- a/src/node/miner.cpp +++ b/src/node/miner.cpp @@ -184,14 +184,18 @@ std::unique_ptr BlockAssembler::CreateNewBlock() // increasing its length would reduce the space they can use and may break // existing clients. coinbaseTx.vin[0].scriptSig = CScript() << nHeight; - if (m_options.include_dummy_extranonce) { + // Set script_sig_prefix here, so IPC mining clients are not affected by + // the optional scriptSig padding below. They provide their own extraNonce, + // and in a typical setup a pool name or realistic extraNonce already makes + // the scriptSig long enough. + coinbase_tx.script_sig_prefix = coinbaseTx.vin[0].scriptSig; + if (nHeight <= 16) { // For blocks at heights <= 16, the BIP34-encoded height alone is only // one byte. Consensus requires coinbase scriptSigs to be at least two - // bytes long (bad-cb-length), so tests and regtest include a dummy - // extraNonce (OP_0) + // bytes long (bad-cb-length), so an OP_0 is always appended at those + // heights. coinbaseTx.vin[0].scriptSig << OP_0; } - coinbase_tx.script_sig_prefix = coinbaseTx.vin[0].scriptSig; Assert(nHeight > 0); coinbaseTx.nLockTime = static_cast(nHeight - 1); coinbase_tx.lock_time = coinbaseTx.nLockTime; @@ -221,7 +225,6 @@ std::unique_ptr BlockAssembler::CreateNewBlock() pblock->nNonce = 0; if (m_options.test_block_validity) { - // if nHeight <= 16, and include_dummy_extranonce=false this will fail due to bad-cb-length. if (BlockValidationState state{TestBlockValidity(m_chainstate, *pblock, /*check_pow=*/false, /*check_merkle_root=*/false)}; !state.IsValid()) { throw std::runtime_error(strprintf("TestBlockValidity failed: %s", state.ToString())); } diff --git a/src/node/types.h b/src/node/types.h index 01e74528e06f..d228fa41db6a 100644 --- a/src/node/types.h +++ b/src/node/types.h @@ -72,10 +72,6 @@ struct BlockCreateOptions { * coinbase_max_additional_weight and coinbase_output_max_additional_sigops. */ CScript coinbase_output_script{CScript() << OP_TRUE}; - /** - * Whether to include an OP_0 as a dummy extraNonce in the template's coinbase - */ - bool include_dummy_extranonce{false}; }; struct BlockWaitOptions { @@ -125,7 +121,9 @@ struct CoinbaseTx { * Prefix which needs to be placed at the beginning of the scriptSig. * Clients may append extra data to this as long as the overall scriptSig * size is 100 bytes or less, to avoid the block being rejected with - * "bad-cb-length" error. + * "bad-cb-length" error. At heights <= 16 the BIP 34 height push is only + * one byte long, so clients must append at least one additional byte to + * meet the consensus minimum scriptSig length of two bytes. * * Currently with BIP 34, the prefix is guaranteed to be less than 8 bytes, * but future soft forks could require longer prefixes. diff --git a/src/test/fuzz/cmpctblock.cpp b/src/test/fuzz/cmpctblock.cpp index 6af9883a2e9a..85e3bedfe924 100644 --- a/src/test/fuzz/cmpctblock.cpp +++ b/src/test/fuzz/cmpctblock.cpp @@ -123,7 +123,6 @@ void ResetChainmanAndMempool(TestingSetup& setup) node::BlockAssembler::Options options; options.coinbase_output_script = P2WSH_OP_TRUE; - options.include_dummy_extranonce = true; g_mature_coinbase.clear(); diff --git a/src/test/fuzz/package_eval.cpp b/src/test/fuzz/package_eval.cpp index 93cd8ad6bbdb..1cc2caa396e9 100644 --- a/src/test/fuzz/package_eval.cpp +++ b/src/test/fuzz/package_eval.cpp @@ -46,7 +46,6 @@ void initialize_tx_pool() BlockAssembler::Options options; options.coinbase_output_script = P2WSH_EMPTY; - options.include_dummy_extranonce = true; for (int i = 0; i < 2 * COINBASE_MATURITY; ++i) { COutPoint prevout{MineBlock(g_setup->m_node, options)}; diff --git a/src/test/fuzz/process_message.cpp b/src/test/fuzz/process_message.cpp index 5f54857ea5fa..d8a91d7f90a7 100644 --- a/src/test/fuzz/process_message.cpp +++ b/src/test/fuzz/process_message.cpp @@ -43,7 +43,6 @@ void ResetChainman(TestingSetup& setup) setup.LoadVerifyActivateChainstate(); for (int i = 0; i < 2 * COINBASE_MATURITY; i++) { node::BlockAssembler::Options options; - options.include_dummy_extranonce = true; MineBlock(setup.m_node, options); } } diff --git a/src/test/fuzz/process_messages.cpp b/src/test/fuzz/process_messages.cpp index 9f33b0df24bb..314388fac07e 100644 --- a/src/test/fuzz/process_messages.cpp +++ b/src/test/fuzz/process_messages.cpp @@ -37,7 +37,6 @@ void ResetChainman(TestingSetup& setup) setup.m_make_chainman(); setup.LoadVerifyActivateChainstate(); node::BlockAssembler::Options options; - options.include_dummy_extranonce = true; for (int i = 0; i < 2 * COINBASE_MATURITY; i++) { MineBlock(setup.m_node, options); } diff --git a/src/test/fuzz/tx_pool.cpp b/src/test/fuzz/tx_pool.cpp index bb15552723af..f70dd710b356 100644 --- a/src/test/fuzz/tx_pool.cpp +++ b/src/test/fuzz/tx_pool.cpp @@ -48,7 +48,6 @@ void initialize_tx_pool() BlockAssembler::Options options; options.coinbase_output_script = P2WSH_OP_TRUE; - options.include_dummy_extranonce = true; for (int i = 0; i < 2 * COINBASE_MATURITY; ++i) { COutPoint prevout{MineBlock(g_setup->m_node, options)}; @@ -98,7 +97,6 @@ void Finish(FuzzedDataProvider& fuzzed_data_provider, MockedTxPool& tx_pool, Cha BlockAssembler::Options options; options.nBlockMaxWeight = fuzzed_data_provider.ConsumeIntegralInRange(0U, MAX_BLOCK_WEIGHT); options.blockMinFeeRate = CFeeRate{ConsumeMoney(fuzzed_data_provider, /*max=*/COIN)}; - options.include_dummy_extranonce = true; auto assembler = BlockAssembler{chainstate, &tx_pool, options}; auto block_template = assembler.CreateNewBlock(); Assert(block_template->block.vtx.size() >= 1); diff --git a/src/test/fuzz/utxo_total_supply.cpp b/src/test/fuzz/utxo_total_supply.cpp index 1275fd45c43c..b281f20028e7 100644 --- a/src/test/fuzz/utxo_total_supply.cpp +++ b/src/test/fuzz/utxo_total_supply.cpp @@ -46,7 +46,6 @@ FUZZ_TARGET(utxo_total_supply) }; BlockAssembler::Options options; options.coinbase_output_script = CScript() << OP_FALSE; - options.include_dummy_extranonce = true; const auto PrepareNextBlock = [&]() { // Use OP_FALSE to avoid BIP30 check from hitting early auto block = PrepareBlock(node, options); @@ -107,8 +106,12 @@ FUZZ_TARGET(utxo_total_supply) // Assuming that the fuzzer will mine relatively short chains (less than 200 blocks), we want the duplicate coinbase to be not too high. // Up to 300 seems reasonable. int64_t duplicate_coinbase_height = fuzzed_data_provider.ConsumeIntegralInRange(0, 300); - // Always pad with OP_0 at the end to avoid bad-cb-length error - const CScript duplicate_coinbase_script = CScript() << duplicate_coinbase_height << OP_0; + // Avoid bad-cb-length error at heights <= 16. Pad the BIP34-encoded height + // with OP_0 to satisfy the minimum 2-byte coinbase scriptSig length. + CScript duplicate_coinbase_script = CScript() << duplicate_coinbase_height; + if (duplicate_coinbase_height <= 16) { + duplicate_coinbase_script << OP_0; + } // Mine the first block with this duplicate current_block = PrepareNextBlock(); StoreLastTxo(); diff --git a/src/test/miner_tests.cpp b/src/test/miner_tests.cpp index 6b4e85a59e45..124ef08500ce 100644 --- a/src/test/miner_tests.cpp +++ b/src/test/miner_tests.cpp @@ -117,7 +117,6 @@ void MinerTestingSetup::TestPackageSelection(const CScript& scriptPubKey, const auto mining{MakeMining()}; BlockAssembler::Options options; options.coinbase_output_script = scriptPubKey; - options.include_dummy_extranonce = true; LOCK(tx_mempool.cs); BOOST_CHECK(tx_mempool.size() == 0); @@ -336,7 +335,6 @@ void MinerTestingSetup::TestBasicMining(const CScript& scriptPubKey, const std:: BlockAssembler::Options options; options.coinbase_output_script = scriptPubKey; - options.include_dummy_extranonce = true; { CTxMemPool& tx_mempool{MakeMempool()}; @@ -663,7 +661,6 @@ void MinerTestingSetup::TestPrioritisedMining(const CScript& scriptPubKey, const BlockAssembler::Options options; options.coinbase_output_script = scriptPubKey; - options.include_dummy_extranonce = true; CTxMemPool& tx_mempool{MakeMempool()}; LOCK(tx_mempool.cs); @@ -753,7 +750,6 @@ BOOST_AUTO_TEST_CASE(CreateNewBlock_validity) CScript scriptPubKey = CScript() << "04678afdb0fe5548271967f1a67130b7105cd6a828e03909a67962e0ea1f61deb649f6bc3f4cef38c4f35504e51ec112de5c384df7ba0b8d578a4c702b6bf11d5f"_hex << OP_CHECKSIG; BlockAssembler::Options options; options.coinbase_output_script = scriptPubKey; - options.include_dummy_extranonce = true; // Create and check a simple template std::unique_ptr block_template = mining->createNewBlock(options, /*cooldown=*/false); diff --git a/src/test/peerman_tests.cpp b/src/test/peerman_tests.cpp index e391e8b9c3fa..36a66b14eb58 100644 --- a/src/test/peerman_tests.cpp +++ b/src/test/peerman_tests.cpp @@ -20,7 +20,6 @@ static void mineBlock(const node::NodeContext& node, std::chrono::seconds block_ { auto curr_time = GetTime(); node::BlockAssembler::Options options; - options.include_dummy_extranonce = true; SetMockTime(block_time); // update time so the block is created with it CBlock block = node::BlockAssembler{node.chainman->ActiveChainstate(), nullptr, options}.CreateNewBlock()->block; while (!CheckProofOfWork(block.GetHash(), block.nBits, node.chainman->GetConsensus())) ++block.nNonce; diff --git a/src/test/testnet4_miner_tests.cpp b/src/test/testnet4_miner_tests.cpp index 1bfc19db2707..c501b4eb0860 100644 --- a/src/test/testnet4_miner_tests.cpp +++ b/src/test/testnet4_miner_tests.cpp @@ -36,7 +36,6 @@ BOOST_AUTO_TEST_CASE(MiningInterface) BOOST_REQUIRE(mining); BlockAssembler::Options options; - options.include_dummy_extranonce = true; std::unique_ptr block_template; // Set node time a few minutes past the testnet4 genesis block diff --git a/src/test/util/mining.cpp b/src/test/util/mining.cpp index c4823bce3515..05f1be77ee29 100644 --- a/src/test/util/mining.cpp +++ b/src/test/util/mining.cpp @@ -29,7 +29,6 @@ COutPoint generatetoaddress(const NodeContext& node, const std::string& address) assert(IsValidDestination(dest)); BlockAssembler::Options assembler_options; assembler_options.coinbase_output_script = GetScriptForDestination(dest); - assembler_options.include_dummy_extranonce = true; return MineBlock(node, assembler_options); } @@ -140,7 +139,6 @@ std::shared_ptr PrepareBlock(const NodeContext& node, const CScript& coi { BlockAssembler::Options assembler_options; assembler_options.coinbase_output_script = coinbase_scriptPubKey; - assembler_options.include_dummy_extranonce = true; ApplyArgsManOptions(*node.args, assembler_options); return PrepareBlock(node, assembler_options); } diff --git a/src/test/util/setup_common.cpp b/src/test/util/setup_common.cpp index 85612708b357..ed1741987861 100644 --- a/src/test/util/setup_common.cpp +++ b/src/test/util/setup_common.cpp @@ -380,7 +380,7 @@ TestChain100Setup::TestChain100Setup( LOCK(::cs_main); assert( m_node.chainman->ActiveChain().Tip()->GetBlockHash().ToString() == - "0c8c5f79505775a0f6aed6aca2350718ceb9c6f2c878667864d5c7a6d8ffa2a6"); + "0ee6e270d6594249e548110619f7bd690695beb219b915da4a2e84e2b61ed60f"); } } @@ -402,7 +402,6 @@ CBlock TestChain100Setup::CreateBlock( { BlockAssembler::Options options; options.coinbase_output_script = scriptPubKey; - options.include_dummy_extranonce = true; CBlock block = BlockAssembler{chainstate, nullptr, options}.CreateNewBlock()->block; Assert(block.vtx.size() == 1); diff --git a/src/test/validation_block_tests.cpp b/src/test/validation_block_tests.cpp index 09a6f85e0d2f..9ede19a18057 100644 --- a/src/test/validation_block_tests.cpp +++ b/src/test/validation_block_tests.cpp @@ -68,7 +68,6 @@ std::shared_ptr MinerTestingSetup::Block(const uint256& prev_hash) BlockAssembler::Options options; options.coinbase_output_script = CScript{} << i++ << OP_TRUE; - options.include_dummy_extranonce = true; auto ptemplate = BlockAssembler{m_node.chainman->ActiveChainstate(), m_node.mempool.get(), options}.CreateNewBlock(); auto pblock = std::make_shared(ptemplate->block); pblock->hashPrevBlock = prev_hash; @@ -337,7 +336,6 @@ BOOST_AUTO_TEST_CASE(witness_commitment_index) pubKey << 1 << OP_TRUE; BlockAssembler::Options options; options.coinbase_output_script = pubKey; - options.include_dummy_extranonce = true; auto ptemplate = BlockAssembler{m_node.chainman->ActiveChainstate(), m_node.mempool.get(), options}.CreateNewBlock(); CBlock pblock = ptemplate->block; diff --git a/src/test/validation_tests.cpp b/src/test/validation_tests.cpp index 07b28b9cd4b4..e5c8b4c554a6 100644 --- a/src/test/validation_tests.cpp +++ b/src/test/validation_tests.cpp @@ -127,6 +127,7 @@ BOOST_AUTO_TEST_CASE(signet_parse_tests) BOOST_CHECK(!CheckSignetBlockSolution(block, signet_params->GetConsensus())); } + BOOST_AUTO_TEST_CASE(block_malleation) { // Test utilities that calls `IsBlockMutated` and then clears the validity