From 53ad845fb9ebb4da39f7d55415d69647600478e5 Mon Sep 17 00:00:00 2001 From: Sjors Provoost Date: Thu, 13 Mar 2025 11:08:52 +0100 Subject: [PATCH 1/2] test: check fees and sigops in getblocktemplate --- test/functional/mining_basic.py | 57 ++++++++++++++++++++++++++++++++- 1 file changed, 56 insertions(+), 1 deletion(-) diff --git a/test/functional/mining_basic.py b/test/functional/mining_basic.py index fea7ced375..63fb7002c1 100755 --- a/test/functional/mining_basic.py +++ b/test/functional/mining_basic.py @@ -40,7 +40,10 @@ from test_framework.util import ( assert_raises_rpc_error, get_fee, ) -from test_framework.wallet import MiniWallet +from test_framework.wallet import ( + MiniWallet, + MiniWalletMode, +) DIFFICULTY_ADJUSTMENT_INTERVAL = 144 @@ -93,6 +96,57 @@ class MiningTest(BitcoinTestFramework): self.restart_node(0) self.connect_nodes(0, 1) + def test_fees_and_sigops(self): + self.log.info("Test fees and sigops in getblocktemplate result") + node = self.nodes[0] + + # Generate a coinbases with p2pk transactions for its sigops. + wallet_sigops = MiniWallet(node, mode=MiniWalletMode.RAW_P2PK) + self.generate(wallet_sigops, 1, sync_fun=self.no_op) + + # Mature with regular coinbases to prevent inteference with other tests + self.generate(self.wallet, 100, sync_fun=self.no_op) + + # Generate three transactions that must be mined in sequence + # + # tx_a (1 sat/vbyte) + # | + # | + # tx_b (2 sat/vbyte) + # | + # | + # tx_c (3 sat/vbyte) + # + tx_a = wallet_sigops.send_self_transfer(from_node=node, + fee_rate=Decimal("0.00001")) + tx_b = wallet_sigops.send_self_transfer(from_node=node, + fee_rate=Decimal("0.00002"), + utxo_to_spend=tx_a["new_utxo"]) + tx_c = wallet_sigops.send_self_transfer(from_node=node, + fee_rate=Decimal("0.00003"), + utxo_to_spend=tx_b["new_utxo"]) + + # Generate transaction without sigops. It will go first because it pays + # higher fees (100 sat/vbyte) and descends from a different coinbase. + tx_d = self.wallet.send_self_transfer(from_node=node, + fee_rate=Decimal("0.00100")) + + block_template_txs = node.getblocktemplate(NORMAL_GBT_REQUEST_PARAMS)['transactions'] + + block_template_fees = [tx['fee'] for tx in block_template_txs] + assert_equal(block_template_fees, [ + tx_d["fee"] * COIN, + tx_a["fee"] * COIN, + tx_b["fee"] * COIN, + tx_c["fee"] * COIN + ]) + + block_template_sigops = [tx['sigops'] for tx in block_template_txs] + assert_equal(block_template_sigops, [0, 4, 4, 4]) + + # Clear mempool + self.generate(self.wallet, 1, sync_fun=self.no_op) + def test_blockmintxfee_parameter(self): self.log.info("Test -blockmintxfee setting") self.restart_node(0, extra_args=['-minrelaytxfee=0', '-persistmempool=0']) @@ -533,6 +587,7 @@ class MiningTest(BitcoinTestFramework): node.submitheader(hexdata=CBlockHeader(bad_block_root).serialize().hex()) assert_equal(node.submitblock(hexdata=block.serialize().hex()), 'duplicate') # valid + self.test_fees_and_sigops() self.test_blockmintxfee_parameter() self.test_block_max_weight() self.test_timewarp() From 226d81f8b708ea1c3a39ec58446e291fe7440fdd Mon Sep 17 00:00:00 2001 From: Sjors Provoost Date: Fri, 21 Feb 2025 09:11:17 +0100 Subject: [PATCH 2/2] mining: drop unused -nFees and sigops from CBlockTemplate For the coinbase vTxFees used a dummy value of -nFees. This value was never returned by the RPC or used in a test. Similarly the fist vTxSigOpsCost entry was calculated from the dummy coinbase transaction. Drop both and add code comments to prevent confusion. --- src/interfaces/mining.h | 3 +++ src/node/miner.cpp | 7 ++----- src/node/miner.h | 2 ++ src/rpc/mining.cpp | 2 +- 4 files changed, 8 insertions(+), 6 deletions(-) diff --git a/src/interfaces/mining.h b/src/interfaces/mining.h index 435151c5dd..298a5c92dc 100644 --- a/src/interfaces/mining.h +++ b/src/interfaces/mining.h @@ -34,9 +34,12 @@ public: virtual ~BlockTemplate() = default; virtual CBlockHeader getBlockHeader() = 0; + // Block contains a dummy coinbase transaction that should not be used. virtual CBlock getBlock() = 0; + // Fees per transaction, not including coinbase transaction. virtual std::vector getTxFees() = 0; + // Sigop cost per transaction, not including coinbase transaction. virtual std::vector getTxSigops() = 0; virtual CTransactionRef getCoinbaseTx() = 0; diff --git a/src/node/miner.cpp b/src/node/miner.cpp index 33eeaf91fb..c24debd1e6 100644 --- a/src/node/miner.cpp +++ b/src/node/miner.cpp @@ -124,10 +124,9 @@ std::unique_ptr BlockAssembler::CreateNewBlock() pblocktemplate.reset(new CBlockTemplate()); CBlock* const pblock = &pblocktemplate->block; // pointer for convenience - // Add dummy coinbase tx as first transaction + // Add dummy coinbase tx as first transaction. It is skipped by the + // getblocktemplate RPC and mining interface consumers must not use it. pblock->vtx.emplace_back(); - pblocktemplate->vTxFees.push_back(-1); // updated at end - pblocktemplate->vTxSigOpsCost.push_back(-1); // updated at end LOCK(::cs_main); CBlockIndex* pindexPrev = m_chainstate.m_chain.Tip(); @@ -165,7 +164,6 @@ std::unique_ptr BlockAssembler::CreateNewBlock() coinbaseTx.vin[0].scriptSig = CScript() << nHeight << OP_0; pblock->vtx[0] = MakeTransactionRef(std::move(coinbaseTx)); pblocktemplate->vchCoinbaseCommitment = m_chainstate.m_chainman.GenerateCoinbaseCommitment(*pblock, pindexPrev); - pblocktemplate->vTxFees[0] = -nFees; LogPrintf("CreateNewBlock(): block weight: %u txs: %u fees: %ld sigops %d\n", GetBlockWeight(*pblock), nBlockTx, nFees, nBlockSigOpsCost); @@ -174,7 +172,6 @@ std::unique_ptr BlockAssembler::CreateNewBlock() UpdateTime(pblock, chainparams.GetConsensus(), pindexPrev); pblock->nBits = GetNextWorkRequired(pindexPrev, pblock, chainparams.GetConsensus()); pblock->nNonce = 0; - pblocktemplate->vTxSigOpsCost[0] = WITNESS_SCALE_FACTOR * GetLegacySigOpCount(*pblock->vtx[0]); BlockValidationState state; if (m_options.test_block_validity && !TestBlockValidity(state, chainparams, m_chainstate, *pblock, pindexPrev, diff --git a/src/node/miner.h b/src/node/miner.h index 56101561b1..c09e9eb5d1 100644 --- a/src/node/miner.h +++ b/src/node/miner.h @@ -37,7 +37,9 @@ static const bool DEFAULT_PRINT_MODIFIED_FEE = false; struct CBlockTemplate { CBlock block; + // Fees per transaction, not including coinbase transaction (unlike CBlock::vtx). std::vector vTxFees; + // Sigops per transaction, not including coinbase transaction (unlike CBlock::vtx). std::vector vTxSigOpsCost; std::vector vchCoinbaseCommitment; /* A vector of package fee rates, ordered by the sequence in which diff --git a/src/rpc/mining.cpp b/src/rpc/mining.cpp index b527e686a4..060adee8c6 100644 --- a/src/rpc/mining.cpp +++ b/src/rpc/mining.cpp @@ -893,7 +893,7 @@ static RPCHelpMan getblocktemplate() } entry.pushKV("depends", std::move(deps)); - int index_in_template = i - 1; + int index_in_template = i - 2; entry.pushKV("fee", tx_fees.at(index_in_template)); int64_t nTxSigOps{tx_sigops.at(index_in_template)}; if (fPreSegWit) {