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