From c548d6f0e8ecc0da6e29256c7085d48d10e10216 Mon Sep 17 00:00:00 2001 From: Sjors Provoost Date: Fri, 21 Nov 2025 16:28:19 +0100 Subject: [PATCH 1/5] test: destroy templates more carefully Prepare template destruction handling for a later commit that checks memory management: - add destroy_template helper which awaits the result and avoids calling destroy() if we never received a template - reverse order and prevent template override. This ensures template and template2 (which don't have transactions) are destroyed last. Additionally, expand the test to demonstrate how setting feeThreshold to MAX_MONEY ignores new mempool transactions. This extra transaction is needed in a later commit (to add coverage for reference counting). --- test/functional/interface_ipc.py | 59 ++++++++++++++++++++++---------- 1 file changed, 40 insertions(+), 19 deletions(-) diff --git a/test/functional/interface_ipc.py b/test/functional/interface_ipc.py index 75d340d5838e3..b9342ad62d869 100755 --- a/test/functional/interface_ipc.py +++ b/test/functional/interface_ipc.py @@ -7,7 +7,13 @@ from io import BytesIO from pathlib import Path import shutil -from test_framework.messages import (CBlock, CTransaction, ser_uint256, COIN) +from test_framework.messages import ( + CBlock, + COIN, + CTransaction, + MAX_MONEY, + ser_uint256, +) from test_framework.test_framework import BitcoinTestFramework from test_framework.util import ( assert_equal, @@ -88,6 +94,12 @@ async def parse_and_deserialize_coinbase_tx(self, block_template, ctx): tx.deserialize(coinbase_data) return tx + async def destroy_template(self, template_obj, ctx): + """Destroy template if we received one.""" + if template_obj is None or template_obj.to_dict() == {}: + return + await template_obj.result.destroy(ctx) + def run_echo_test(self): self.log.info("Running echo test") async def async_routine(): @@ -154,12 +166,18 @@ async def async_routine(): coinbase = CTransaction() coinbase.deserialize(coinbase_data) assert_equal(coinbase.vin[0].prevout.hash, 0) - self.log.debug("Wait for a new template") + self.log.debug("Wait for a new template, get one after the tip updates") waitoptions = self.capnp_modules['mining'].BlockWaitOptions() waitoptions.timeout = timeout - waitoptions.feeThreshold = 1 + # Ignore fee increases, wait only for the tip update + waitoptions.feeThreshold = MAX_MONEY + miniwallet.send_self_transfer(fee_rate=10, from_node=self.nodes[0]) waitnext = template.result.waitNext(ctx, waitoptions) - self.generate(self.nodes[0], 1) + template2 = await waitnext + assert_equal(template2.to_dict(), {}) + waitnext = template.result.waitNext(ctx, waitoptions) + # This mines the transaction, so it won't be in the next template + self.generate(self.nodes[0], 1, sync_fun=self.no_op) template2 = await waitnext block2 = await self.parse_and_deserialize_block(template2, ctx) assert_equal(len(block2.vtx), 1) @@ -167,6 +185,7 @@ async def async_routine(): template3 = await template2.result.waitNext(ctx, waitoptions) assert_equal(template3.to_dict(), {}) self.log.debug("Wait for another, get one after increase in fees in the mempool") + waitoptions.feeThreshold = 1 waitnext = template2.result.waitNext(ctx, waitoptions) miniwallet.send_self_transfer(fee_rate=10, from_node=self.nodes[0]) template4 = await waitnext @@ -212,9 +231,9 @@ async def interrupt_wait(): current_block_height = self.nodes[0].getchaintips()[0]["height"] check_opts = self.capnp_modules['mining'].BlockCheckOptions() - template = await mining.result.createNewBlock(opts) - block = await self.parse_and_deserialize_block(template, ctx) - coinbase = await self.parse_and_deserialize_coinbase_tx(template, ctx) + template8 = await mining.result.createNewBlock(opts) + block = await self.parse_and_deserialize_block(template8, ctx) + coinbase = await self.parse_and_deserialize_coinbase_tx(template8, ctx) balance = miniwallet.get_balance() coinbase.vout[0].scriptPubKey = miniwallet.get_output_script() coinbase.vout[0].nValue = COIN @@ -227,7 +246,7 @@ async def interrupt_wait(): res = await mining.result.checkBlock(block.serialize(), check_opts) assert_equal(res.result, False) assert_equal(res.reason, "bad-version(0x00000000)") - res = await template.result.submitSolution(ctx, block.nVersion, block.nTime, block.nNonce, coinbase.serialize()) + res = await template8.result.submitSolution(ctx, block.nVersion, block.nTime, block.nNonce, coinbase.serialize()) assert_equal(res.result, False) self.log.debug("Submit a valid block") block.nVersion = original_version @@ -238,21 +257,21 @@ async def interrupt_wait(): assert_equal(res.result, True) # The remote template block will be mutated, capture the original: - remote_block_before = await self.parse_and_deserialize_block(template, ctx) + remote_block_before = await self.parse_and_deserialize_block(template8, ctx) self.log.debug("Submitted coinbase must include witness") assert_not_equal(coinbase.serialize_without_witness().hex(), coinbase.serialize().hex()) - res = await template.result.submitSolution(ctx, block.nVersion, block.nTime, block.nNonce, coinbase.serialize_without_witness()) + res = await template8.result.submitSolution(ctx, block.nVersion, block.nTime, block.nNonce, coinbase.serialize_without_witness()) assert_equal(res.result, False) self.log.debug("Even a rejected submitBlock() mutates the template's block") # Can be used by clients to download and inspect the (rejected) # reconstructed block. - remote_block_after = await self.parse_and_deserialize_block(template, ctx) + remote_block_after = await self.parse_and_deserialize_block(template8, ctx) assert_not_equal(remote_block_before.serialize().hex(), remote_block_after.serialize().hex()) self.log.debug("Submit again, with the witness") - res = await template.result.submitSolution(ctx, block.nVersion, block.nTime, block.nNonce, coinbase.serialize()) + res = await template8.result.submitSolution(ctx, block.nVersion, block.nTime, block.nNonce, coinbase.serialize()) assert_equal(res.result, True) self.log.debug("Block should propagate") @@ -271,13 +290,15 @@ async def interrupt_wait(): assert_equal(res.reason, "inconclusive-not-best-prevblk") self.log.debug("Destroy template objects") - template.result.destroy(ctx) - template2.result.destroy(ctx) - template3.result.destroy(ctx) - template4.result.destroy(ctx) - template5.result.destroy(ctx) - template6.result.destroy(ctx) - template7.result.destroy(ctx) + await self.destroy_template(template8, ctx) + await self.destroy_template(template7, ctx) + await self.destroy_template(template6, ctx) + await self.destroy_template(template5, ctx) + await self.destroy_template(template4, ctx) + await self.destroy_template(template3, ctx) + await self.destroy_template(template2, ctx) + await self.destroy_template(template, ctx) + asyncio.run(capnp.run(async_routine())) def run_test(self): From a5eee29fd7d177f57c78da9773cb656a129de839 Mon Sep 17 00:00:00 2001 From: Sjors Provoost Date: Tue, 25 Nov 2025 18:16:43 +0100 Subject: [PATCH 2/5] rpc: move static block_template to node context The getblocktemplate RPC uses a static BlockTemplate, which goes out of scope only after the node completed its shutdown sequence. This becomes a problem when a later commit implements a destructor that uses m_node. --- src/node/context.h | 3 +++ src/rpc/mining.cpp | 2 +- 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/src/node/context.h b/src/node/context.h index debc12212064c..cc88180b33cf8 100644 --- a/src/node/context.h +++ b/src/node/context.h @@ -26,6 +26,7 @@ class ECC_Context; class NetGroupManager; class PeerManager; namespace interfaces { +class BlockTemplate; class Chain; class ChainClient; class Mining; @@ -66,6 +67,8 @@ struct NodeContext { std::unique_ptr addrman; std::unique_ptr connman; std::unique_ptr mempool; + //! Cache latest getblocktemplate result for BIP 22 long polling + std::unique_ptr gbt_template; std::unique_ptr netgroupman; std::unique_ptr fee_estimator; std::unique_ptr peerman; diff --git a/src/rpc/mining.cpp b/src/rpc/mining.cpp index 2639b9161298c..bd497a852f5bd 100644 --- a/src/rpc/mining.cpp +++ b/src/rpc/mining.cpp @@ -858,7 +858,7 @@ static RPCHelpMan getblocktemplate() // Update block static CBlockIndex* pindexPrev; static int64_t time_start; - static std::unique_ptr block_template; + std::unique_ptr& block_template{node.gbt_template}; if (!pindexPrev || pindexPrev->GetBlockHash() != tip || (mempool.GetTransactionsUpdated() != nTransactionsUpdatedLast && GetTime() - time_start > 5)) { From f90ab4ed715d87e7df93e01de55c7f15e9f20023 Mon Sep 17 00:00:00 2001 From: Sjors Provoost Date: Fri, 21 Nov 2025 16:12:27 +0100 Subject: [PATCH 3/5] mining: track non-mempool memory usage IPC clients can hold on to block templates indefinately, which has the same impact as when the node holds a shared pointer to the CBlockTemplate. Because each template in turn tracks CTransactionRefs, transactions that are removed from the mempool will have not have their memory cleared. This commit adds bookkeeping to the block template constructor and destructor that will let us track the resulting memory footprint. --- src/node/context.h | 10 +++++++++- src/node/interfaces.cpp | 20 +++++++++++++++++++- src/node/types.h | 7 +++++++ 3 files changed, 35 insertions(+), 2 deletions(-) diff --git a/src/node/context.h b/src/node/context.h index cc88180b33cf8..613a6579735e3 100644 --- a/src/node/context.h +++ b/src/node/context.h @@ -5,10 +5,13 @@ #ifndef BITCOIN_NODE_CONTEXT_H #define BITCOIN_NODE_CONTEXT_H +#include + #include #include #include #include +#include #include #include @@ -67,7 +70,12 @@ struct NodeContext { std::unique_ptr addrman; std::unique_ptr connman; std::unique_ptr mempool; - //! Cache latest getblocktemplate result for BIP 22 long polling + Mutex template_state_mutex; + //! Track how many templates (which we hold on to on behalf of connected IPC + //! clients) are referencing each transaction. + TxTemplateMap template_tx_refs GUARDED_BY(template_state_mutex); + //! Cache latest getblocktemplate result for BIP 22 long polling. Must be cleared + //! before template_tx_refs. std::unique_ptr gbt_template; std::unique_ptr netgroupman; std::unique_ptr fee_estimator; diff --git a/src/node/interfaces.cpp b/src/node/interfaces.cpp index ac817e01b157c..1c887df81ab6c 100644 --- a/src/node/interfaces.cpp +++ b/src/node/interfaces.cpp @@ -67,6 +67,7 @@ #include #include #include +#include #include #include @@ -862,7 +863,24 @@ class BlockTemplateImpl : public BlockTemplate m_block_template(std::move(block_template)), m_node(node) { - assert(m_block_template); + // Don't track the dummy coinbase, because it can be modified in-place + // by submitSolution() + LOCK(m_node.template_state_mutex); + for (const CTransactionRef& tx : Assert(m_block_template)->block.vtx | std::views::drop(1)) { + m_node.template_tx_refs[tx]++; + } + } + + ~BlockTemplateImpl() + { + LOCK(m_node.template_state_mutex); + for (const CTransactionRef& tx : m_block_template->block.vtx | std::views::drop(1)) { + auto ref_count{m_node.template_tx_refs.find(tx)}; + if (!Assume(ref_count != m_node.template_tx_refs.end())) break; + if (--ref_count->second == 0) { + m_node.template_tx_refs.erase(ref_count); + } + } } CBlockHeader getBlockHeader() override diff --git a/src/node/types.h b/src/node/types.h index 6c2687626c98c..437e5df87b89c 100644 --- a/src/node/types.h +++ b/src/node/types.h @@ -16,7 +16,9 @@ #include #include #include +#include #include +#include #include