From fc987908699086161c4d735d7ac3bcc7110a5fa0 Mon Sep 17 00:00:00 2001 From: ismaelsadeeq Date: Mon, 22 Jun 2026 10:53:41 +0100 Subject: [PATCH 1/2] test: `TestChunkBlockLimits` uses incorrect weight for comparison addChunks() passes the chunk's sigops-adjusted fee rate to TestChunkBlockLimits() and the adjusted weight is used for block weight limit check while `nBlockWeight` accumulates real transaction weight. A sigop-dense chunk that fits the block by real weight is therefore checked against block_max_weight by its larger adjusted weight and wrongly skipped. Add a transaction sized to fit by real weight; but is left out of the template (only the coinbase is mined) due to this issue. The next commit fixes this and flips the assertion. --- src/test/miner_tests.cpp | 56 +++++++++++++++++++++++++++++++--------- 1 file changed, 44 insertions(+), 12 deletions(-) diff --git a/src/test/miner_tests.cpp b/src/test/miner_tests.cpp index 1c02a91104b..fc80e7dd8ad 100644 --- a/src/test/miner_tests.cpp +++ b/src/test/miner_tests.cpp @@ -61,6 +61,7 @@ struct MinerTestingSetup : public TestingSetup { void TestPackageSelection(const CScript& scriptPubKey, const std::vector& txFirst) EXCLUSIVE_LOCKS_REQUIRED(::cs_main); void TestBasicMining(const CScript& scriptPubKey, const std::vector& txFirst, int baseheight) EXCLUSIVE_LOCKS_REQUIRED(::cs_main); void TestPrioritisedMining(const CScript& scriptPubKey, const std::vector& txFirst) EXCLUSIVE_LOCKS_REQUIRED(::cs_main); + void TestSigOpsAdjustedWeightChunkLimit(const CScript& scriptPubKey, const std::vector& txFirst) EXCLUSIVE_LOCKS_REQUIRED(::cs_main); bool TestSequenceLocks(const CTransaction& tx, CTxMemPool& tx_mempool) EXCLUSIVE_LOCKS_REQUIRED(::cs_main) { CCoinsViewMemPool view_mempool{&m_node.chainman->ActiveChainstate().CoinsTip(), tx_mempool}; @@ -299,6 +300,22 @@ void MinerTestingSetup::TestPackageSelection(const CScript& scriptPubKey, const BOOST_CHECK(block.vtx[8]->GetHash() == hashLowFeeTx2); } +// One sigop-dense tx spending `input`: num_outputs bare CHECKMULTISIG outputs, +// i.e. 20 * num_outputs legacy sigops (OP_NOP forces the inaccurate max-20 count). +static CMutableTransaction CreateBigSigOpsTx(const COutPoint& input, unsigned int num_outputs) +{ + CMutableTransaction tx; + tx.vin.resize(1); + tx.vin[0].prevout = input; + tx.vin[0].scriptSig = CScript() << OP_1; + tx.vout.resize(num_outputs); + for (auto& out : tx.vout) { + out.nValue = 0; + out.scriptPubKey = CScript() << OP_0 << OP_0 << OP_0 << OP_NOP << OP_CHECKMULTISIG << OP_1; + } + return tx; +} + std::vector CreateBigSigOpsCluster(const CTransactionRef& first_tx) { std::vector ret; @@ -325,22 +342,35 @@ std::vector CreateBigSigOpsCluster(const CTransactionRef& first // Tx2-51 has 400 sigops: 1 input, 20 CHECKMULTISIG outputs // Total: 1000 CHECKMULTISIG + 1 for (unsigned int i = 0; i < 50; ++i) { - auto tx2 = tx; - tx2.vin.resize(1); - tx2.vin[0].prevout.hash = parent_tx->GetHash(); - tx2.vin[0].prevout.n = i; - tx2.vin[0].scriptSig = CScript() << OP_1; - tx2.vout.resize(20); - tx2.vout[0].nValue = parent_tx->vout[i].nValue - CENT; - for (auto &out : tx2.vout) { - out.nValue = 0; - out.scriptPubKey = CScript() << OP_0 << OP_0 << OP_0 << OP_NOP << OP_CHECKMULTISIG << OP_1; - } - ret.push_back(MakeTransactionRef(tx2)); + ret.push_back(MakeTransactionRef(CreateBigSigOpsTx(COutPoint{parent_tx->GetHash(), i}, /*num_outputs=*/20))); } return ret; } +void MinerTestingSetup::TestSigOpsAdjustedWeightChunkLimit(const CScript& scriptPubKey, const std::vector& txFirst) +{ + auto mining{MakeMining()}; + BOOST_REQUIRE(mining); + + CTxMemPool& tx_mempool{MakeMempool()}; + LOCK(tx_mempool.cs); + TestMemPoolEntryHelper entry; + + const auto tx{CreateBigSigOpsTx(COutPoint{txFirst[0]->GetHash(), 0}, /*num_outputs=*/50)}; + const auto sigop_entry{entry.Fee(COIN).SpendsCoinbase(true).SigOpsCost(GetLegacySigOpCount(CTransaction(tx)) * WITNESS_SCALE_FACTOR).FromTx(tx)}; + BOOST_REQUIRE(sigop_entry.GetAdjustedWeight() > sigop_entry.GetTxWeight()); + BOOST_REQUIRE(sigop_entry.GetSigOpCost() < MAX_BLOCK_SIGOPS_COST); + TryAddToMempool(tx_mempool, sigop_entry); + + BlockCreateOptions options{ + // +1 because TestChunkBlockLimits rejects on >= (exact fit doesn't count). + .block_max_weight = DEFAULT_BLOCK_RESERVED_WEIGHT + sigop_entry.GetTxWeight() + 1, + .coinbase_output_script = scriptPubKey, + }; + const CBlock block{mining->createNewBlock(options, /*cooldown=*/false)->getBlock()}; + BOOST_CHECK_EQUAL(block.vtx.size(), 1U); +} + void MinerTestingSetup::TestBasicMining(const CScript& scriptPubKey, const std::vector& txFirst, int baseheight) { FakeNodeClock clock{}; @@ -905,6 +935,8 @@ BOOST_AUTO_TEST_CASE(CreateNewBlock_validity) m_node.chainman->ActiveChain().Tip()->nHeight--; TestPrioritisedMining(scriptPubKey, txFirst); + + TestSigOpsAdjustedWeightChunkLimit(scriptPubKey, txFirst); } BOOST_AUTO_TEST_SUITE_END() From 5be248341a8e9ad97143647e7f480eecf1f90eb4 Mon Sep 17 00:00:00 2001 From: ismaelsadeeq Date: Mon, 22 Jun 2026 10:53:54 +0100 Subject: [PATCH 2/2] bugfix: compare real chunk weight against block weight limit TestChunkBlockLimits() compared the chunk's sigops-adjusted weight against block_max_weight, while nBlockWeight tracks real transaction weight. This over-counted sigop-dense chunks and could skip ones that actually fit, losing fees; the block sigop limit is enforced separately on the next line. Pass the chunk's real weight (sum of GetTxWeight()) instead, and update the test to show the chunk is now included. --- src/node/miner.cpp | 8 +++++--- src/node/miner.h | 2 +- src/test/miner_tests.cpp | 3 ++- 3 files changed, 8 insertions(+), 5 deletions(-) diff --git a/src/node/miner.cpp b/src/node/miner.cpp index 32a21440c4e..b3fd020820e 100644 --- a/src/node/miner.cpp +++ b/src/node/miner.cpp @@ -243,11 +243,11 @@ std::unique_ptr BlockAssembler::CreateNewBlock() return std::move(pblocktemplate); } -bool BlockAssembler::TestChunkBlockLimits(FeePerWeight chunk_feerate, int64_t chunk_sigops_cost) const +bool BlockAssembler::TestChunkBlockLimits(int64_t chunk_weight, int64_t chunk_sigops_cost) const { // block_max_weight has been flattened before block assembly limit checks. Assert(m_options.block_max_weight); - if (nBlockWeight + chunk_feerate.size >= *m_options.block_max_weight) { + if (nBlockWeight + chunk_weight >= m_options.block_max_weight) { return false; } if (nBlockSigOpsCost + chunk_sigops_cost >= MAX_BLOCK_SIGOPS_COST) { @@ -310,12 +310,14 @@ void BlockAssembler::addChunks() } int64_t chunk_sig_ops = 0; + int64_t chunk_weight = 0; for (const auto& tx : selected_transactions) { chunk_sig_ops += tx.get().GetSigOpCost(); + chunk_weight += tx.get().GetTxWeight(); } // Check to see if this chunk will fit. - if (!TestChunkBlockLimits(chunk_feerate, chunk_sig_ops) || !TestChunkTransactions(selected_transactions)) { + if (!TestChunkBlockLimits(chunk_weight, chunk_sig_ops) || !TestChunkTransactions(selected_transactions)) { // This chunk won't fit, so we skip it and will try the next best one. m_mempool->SkipBuilderChunk(); ++nConsecutiveFailed; diff --git a/src/node/miner.h b/src/node/miner.h index af327307329..08fa3f1d2af 100644 --- a/src/node/miner.h +++ b/src/node/miner.h @@ -108,7 +108,7 @@ private: // helper functions for addChunks() /** Test if a new chunk would "fit" in the block */ - bool TestChunkBlockLimits(FeePerWeight chunk_feerate, int64_t chunk_sigops_cost) const; + bool TestChunkBlockLimits(int64_t chunk_weight, int64_t chunk_sigops_cost) const; /** Perform locktime checks on each transaction in a chunk: * This check should always succeed, and is here * only as an extra check in case of a bug */ diff --git a/src/test/miner_tests.cpp b/src/test/miner_tests.cpp index fc80e7dd8ad..b91ec8b8b6f 100644 --- a/src/test/miner_tests.cpp +++ b/src/test/miner_tests.cpp @@ -368,7 +368,8 @@ void MinerTestingSetup::TestSigOpsAdjustedWeightChunkLimit(const CScript& script .coinbase_output_script = scriptPubKey, }; const CBlock block{mining->createNewBlock(options, /*cooldown=*/false)->getBlock()}; - BOOST_CHECK_EQUAL(block.vtx.size(), 1U); + BOOST_CHECK_EQUAL(block.vtx.size(), 2U); + BOOST_CHECK(block.vtx[1]->GetHash() == tx.GetHash()); } void MinerTestingSetup::TestBasicMining(const CScript& scriptPubKey, const std::vector& txFirst, int baseheight)