From 3ddafceb9afd9d493b927bc91dae324225ed8e32 Mon Sep 17 00:00:00 2001 From: Pieter Wuille Date: Sat, 10 Jan 2026 13:03:36 -0500 Subject: [PATCH] txgraph: initialize Ref in AddTransaction (preparation) Instead of returning a TxGraph::Ref from TxGraph::AddTransaction(), pass in a TxGraph::Ref& which is updated to refer to the new transaction in that graph. This cleans up the usage somewhat, avoiding the need for dummy Refs in CTxMemPoolEntry constructor calls, but the motivation is that a future commit will allow a callback to passed to MakeTxGraph to define a fallback order on the transaction objects. This does not work when a Ref is created separately from the CTxMemPoolEntry it ends up living in, as passing the newly-created Ref to the callback would be UB before it's emplaced in its final CTxMemPoolEntry. --- src/bench/blockencodings.cpp | 2 +- src/bench/mempool_ephemeral_spends.cpp | 2 +- src/bench/mempool_eviction.cpp | 2 +- src/bench/mempool_stress.cpp | 2 +- src/bench/rpc_mempool.cpp | 2 +- src/bench/txgraph.cpp | 5 +++-- src/kernel/mempool_entry.h | 5 ++--- src/test/fuzz/txgraph.cpp | 13 ++++++------ src/test/fuzz/util/mempool.cpp | 2 +- src/test/txgraph_tests.cpp | 29 +++++++++++++------------- src/test/util/txmempool.cpp | 2 +- src/txgraph.cpp | 15 ++++++------- src/txgraph.h | 13 ++++++------ src/txmempool.cpp | 5 +++-- 14 files changed, 49 insertions(+), 50 deletions(-) diff --git a/src/bench/blockencodings.cpp b/src/bench/blockencodings.cpp index 3f6be56b464..ce968dbc651 100644 --- a/src/bench/blockencodings.cpp +++ b/src/bench/blockencodings.cpp @@ -22,7 +22,7 @@ static void AddTx(const CTransactionRef& tx, const CAmount& fee, CTxMemPool& pool) EXCLUSIVE_LOCKS_REQUIRED(cs_main, pool.cs) { LockPoints lp; - TryAddToMempool(pool, CTxMemPoolEntry(TxGraph::Ref(), tx, fee, /*time=*/0, /*entry_height=*/1, /*entry_sequence=*/0, /*spends_coinbase=*/false, /*sigops_cost=*/4, lp)); + TryAddToMempool(pool, CTxMemPoolEntry(tx, fee, /*time=*/0, /*entry_height=*/1, /*entry_sequence=*/0, /*spends_coinbase=*/false, /*sigops_cost=*/4, lp)); } namespace { diff --git a/src/bench/mempool_ephemeral_spends.cpp b/src/bench/mempool_ephemeral_spends.cpp index 2f89f0da208..f0d8eb0bea7 100644 --- a/src/bench/mempool_ephemeral_spends.cpp +++ b/src/bench/mempool_ephemeral_spends.cpp @@ -29,7 +29,7 @@ static void AddTx(const CTransactionRef& tx, CTxMemPool& pool) EXCLUSIVE_LOCKS_R unsigned int sigOpCost{4}; uint64_t fee{0}; LockPoints lp; - TryAddToMempool(pool, CTxMemPoolEntry(TxGraph::Ref(), + TryAddToMempool(pool, CTxMemPoolEntry( tx, fee, nTime, nHeight, sequence, spendsCoinbase, sigOpCost, lp)); } diff --git a/src/bench/mempool_eviction.cpp b/src/bench/mempool_eviction.cpp index ad3ab08a994..37bb7215be0 100644 --- a/src/bench/mempool_eviction.cpp +++ b/src/bench/mempool_eviction.cpp @@ -27,7 +27,7 @@ static void AddTx(const CTransactionRef& tx, const CAmount& nFee, CTxMemPool& po bool spendsCoinbase = false; unsigned int sigOpCost = 4; LockPoints lp; - TryAddToMempool(pool, CTxMemPoolEntry(TxGraph::Ref(), + TryAddToMempool(pool, CTxMemPoolEntry( tx, nFee, nTime, nHeight, sequence, spendsCoinbase, sigOpCost, lp)); } diff --git a/src/bench/mempool_stress.cpp b/src/bench/mempool_stress.cpp index 9cddeb7815f..1f582081ea9 100644 --- a/src/bench/mempool_stress.cpp +++ b/src/bench/mempool_stress.cpp @@ -29,7 +29,7 @@ static void AddTx(const CTransactionRef& tx, CTxMemPool& pool, FastRandomContext bool spendsCoinbase = false; unsigned int sigOpCost = 4; LockPoints lp; - TryAddToMempool(pool, CTxMemPoolEntry(TxGraph::Ref(), tx, det_rand.randrange(10000)+1000, nTime, nHeight, sequence, spendsCoinbase, sigOpCost, lp)); + TryAddToMempool(pool, CTxMemPoolEntry(tx, det_rand.randrange(10000)+1000, nTime, nHeight, sequence, spendsCoinbase, sigOpCost, lp)); } struct Available { diff --git a/src/bench/rpc_mempool.cpp b/src/bench/rpc_mempool.cpp index b27a7e72cc4..f319c961a04 100644 --- a/src/bench/rpc_mempool.cpp +++ b/src/bench/rpc_mempool.cpp @@ -22,7 +22,7 @@ static void AddTx(const CTransactionRef& tx, const CAmount& fee, CTxMemPool& pool) EXCLUSIVE_LOCKS_REQUIRED(cs_main, pool.cs) { LockPoints lp; - TryAddToMempool(pool, CTxMemPoolEntry(TxGraph::Ref(), tx, fee, /*time=*/0, /*entry_height=*/1, /*entry_sequence=*/0, /*spends_coinbase=*/false, /*sigops_cost=*/4, lp)); + TryAddToMempool(pool, CTxMemPoolEntry(tx, fee, /*time=*/0, /*entry_height=*/1, /*entry_sequence=*/0, /*spends_coinbase=*/false, /*sigops_cost=*/4, lp)); } static void RpcMempool(benchmark::Bench& bench) diff --git a/src/bench/txgraph.cpp b/src/bench/txgraph.cpp index 078f77b4256..7257a043777 100644 --- a/src/bench/txgraph.cpp +++ b/src/bench/txgraph.cpp @@ -67,7 +67,7 @@ void BenchTxGraphTrim(benchmark::Bench& bench) for (int chaintx = 0; chaintx < NUM_TX_PER_TOP_CHAIN; ++chaintx) { int64_t fee = rng.randbits<27>() + 100; FeePerWeight feerate{fee, 1}; - top_refs.push_back(graph->AddTransaction(feerate)); + graph->AddTransaction(top_refs.emplace_back(), feerate); // Add internal dependencies linking the chain transactions together. if (chaintx > 0) { graph->AddDependency(*(top_refs.rbegin()), *(top_refs.rbegin() + 1)); @@ -85,7 +85,8 @@ void BenchTxGraphTrim(benchmark::Bench& bench) // Construct the transaction. int64_t fee = rng.randbits<27>() + 100; FeePerWeight feerate{fee, 1}; - auto bottom_tx = graph->AddTransaction(feerate); + TxGraph::Ref bottom_tx; + graph->AddTransaction(bottom_tx, feerate); // Determine the number of dependencies this transaction will have. int deps = std::min(NUM_DEPS_PER_BOTTOM_TX, top_components.size()); for (int dep = 0; dep < deps; ++dep) { diff --git a/src/kernel/mempool_entry.h b/src/kernel/mempool_entry.h index 58824f7a80c..dd0c23a2cde 100644 --- a/src/kernel/mempool_entry.h +++ b/src/kernel/mempool_entry.h @@ -84,12 +84,11 @@ private: public: virtual ~CTxMemPoolEntry() = default; - CTxMemPoolEntry(TxGraph::Ref&& ref, const CTransactionRef& tx, CAmount fee, + CTxMemPoolEntry(const CTransactionRef& tx, CAmount fee, int64_t time, unsigned int entry_height, uint64_t entry_sequence, bool spends_coinbase, int64_t sigops_cost, LockPoints lp) - : TxGraph::Ref(std::move(ref)), - tx{tx}, + : tx{tx}, nFee{fee}, nTxWeight{GetTransactionWeight(*tx)}, nUsageSize{RecursiveDynamicUsage(tx)}, diff --git a/src/test/fuzz/txgraph.cpp b/src/test/fuzz/txgraph.cpp index 195c2e05025..6d5ac88218d 100644 --- a/src/test/fuzz/txgraph.cpp +++ b/src/test/fuzz/txgraph.cpp @@ -137,15 +137,16 @@ struct SimTxGraph return simmap[pos].get(); } - /** Add a new transaction to the simulation. */ - void AddTransaction(TxGraph::Ref&& ref, const FeePerWeight& feerate) + /** Add a new transaction to the simulation and the specified real graph. */ + void AddTransaction(TxGraph& txgraph, const FeePerWeight& feerate) { assert(graph.TxCount() < MAX_TRANSACTIONS); auto simpos = graph.AddTransaction(feerate); real_is_optimal = false; MakeModified(simpos); assert(graph.Positions()[simpos]); - simmap[simpos] = std::make_shared(std::move(ref)); + simmap[simpos] = std::make_shared(); + txgraph.AddTransaction(*simmap[simpos], feerate); auto ptr = simmap[simpos].get(); simrevmap[ptr] = simpos; // This may invalidate our cached oversized value. @@ -455,10 +456,8 @@ FUZZ_TARGET(txgraph) size = provider.ConsumeIntegralInRange(1, 0xff); } FeePerWeight feerate{fee, size}; - // Create a real TxGraph::Ref. - auto ref = real->AddTransaction(feerate); - // Create a shared_ptr place in the simulation to put the Ref in. - top_sim.AddTransaction(std::move(ref), feerate); + // Create the transaction in the simulation and the real graph. + top_sim.AddTransaction(*real, feerate); break; } else if ((block_builders.empty() || sims.size() > 1) && top_sim.GetTransactionCount() + top_sim.removed.size() > 1 && command-- == 0) { // AddDependency. diff --git a/src/test/fuzz/util/mempool.cpp b/src/test/fuzz/util/mempool.cpp index 241a4d559c7..a6a28f94006 100644 --- a/src/test/fuzz/util/mempool.cpp +++ b/src/test/fuzz/util/mempool.cpp @@ -27,5 +27,5 @@ CTxMemPoolEntry ConsumeTxMemPoolEntry(FuzzedDataProvider& fuzzed_data_provider, const auto entry_height{fuzzed_data_provider.ConsumeIntegralInRange(0, max_height)}; const bool spends_coinbase = fuzzed_data_provider.ConsumeBool(); const unsigned int sig_op_cost = fuzzed_data_provider.ConsumeIntegralInRange(0, MAX_BLOCK_SIGOPS_COST); - return CTxMemPoolEntry{TxGraph::Ref(), MakeTransactionRef(tx), fee, time, entry_height, entry_sequence, spends_coinbase, sig_op_cost, {}}; + return CTxMemPoolEntry{MakeTransactionRef(tx), fee, time, entry_height, entry_sequence, spends_coinbase, sig_op_cost, {}}; } diff --git a/src/test/txgraph_tests.cpp b/src/test/txgraph_tests.cpp index 26cf2b3a970..31fefd9ce18 100644 --- a/src/test/txgraph_tests.cpp +++ b/src/test/txgraph_tests.cpp @@ -46,11 +46,11 @@ BOOST_AUTO_TEST_CASE(txgraph_trim_zigzag) refs.reserve(NUM_TOTAL_TX); // First all bottom transactions: the i'th bottom transaction is at position i. for (unsigned int i = 0; i < NUM_BOTTOM_TX; ++i) { - refs.push_back(graph->AddTransaction(FeePerWeight{200 - i, 100})); + graph->AddTransaction(refs.emplace_back(), FeePerWeight{200 - i, 100}); } // Then all top transactions: the i'th top transaction is at position NUM_BOTTOM_TX + i. for (unsigned int i = 0; i < NUM_TOP_TX; ++i) { - refs.push_back(graph->AddTransaction(FeePerWeight{100 - i, 100})); + graph->AddTransaction(refs.emplace_back(), FeePerWeight{100 - i, 100}); } // Create the zigzag dependency structure. @@ -109,9 +109,9 @@ BOOST_AUTO_TEST_CASE(txgraph_trim_flower) refs.reserve(NUM_TOTAL_TX); // Add all transactions. They are in individual clusters. - refs.push_back(graph->AddTransaction({1, 100})); + graph->AddTransaction(refs.emplace_back(), {1, 100}); for (unsigned int i = 0; i < NUM_TOP_TX; ++i) { - refs.push_back(graph->AddTransaction(FeePerWeight{500 + i, 100})); + graph->AddTransaction(refs.emplace_back(), FeePerWeight{500 + i, 100}); } graph->SanityCheck(); @@ -196,8 +196,8 @@ BOOST_AUTO_TEST_CASE(txgraph_trim_huge) // Use random fees, size 1. int64_t fee = rng.randbits<27>() + 100; FeePerWeight feerate{fee, 1}; - top_refs.push_back(graph->AddTransaction(feerate)); - // Add internal dependencies linked the chain transactions together. + graph->AddTransaction(top_refs.emplace_back(), feerate); + // Add internal dependencies linking the chain transactions together. if (chaintx > 0) { graph->AddDependency(*(top_refs.rbegin()), *(top_refs.rbegin() + 1)); } @@ -215,7 +215,8 @@ BOOST_AUTO_TEST_CASE(txgraph_trim_huge) // Construct the transaction. int64_t fee = rng.randbits<27>() + 100; FeePerWeight feerate{fee, 1}; - auto bottom_tx = graph->AddTransaction(feerate); + TxGraph::Ref bottom_tx; + graph->AddTransaction(bottom_tx, feerate); // Determine the number of dependencies this transaction will have. int deps = std::min(NUM_DEPS_PER_BOTTOM_TX, top_components.size()); for (int dep = 0; dep < deps; ++dep) { @@ -271,7 +272,7 @@ BOOST_AUTO_TEST_CASE(txgraph_trim_big_singletons) // The 88th transaction is oversized. // Every 20th transaction is oversized. const FeePerWeight feerate{500 + i, (i == 88 || i % 20 == 0) ? MAX_CLUSTER_SIZE + 1 : 100}; - refs.push_back(graph->AddTransaction(feerate)); + graph->AddTransaction(refs.emplace_back(), feerate); } graph->SanityCheck(); @@ -334,23 +335,23 @@ BOOST_AUTO_TEST_CASE(txgraph_chunk_chain) // everytime adding a transaction, test the chunk status // [A] - refs.push_back(graph->AddTransaction(feerateA)); + graph->AddTransaction(refs.emplace_back(), feerateA); BOOST_CHECK_EQUAL(graph->GetTransactionCount(TxGraph::Level::TOP), 1); block_builder_checker({{&refs[0]}}); // [A, B] - refs.push_back(graph->AddTransaction(feerateB)); + graph->AddTransaction(refs.emplace_back(), feerateB); graph->AddDependency(/*parent=*/refs[0], /*child=*/refs[1]); BOOST_CHECK_EQUAL(graph->GetTransactionCount(TxGraph::Level::TOP), 2); block_builder_checker({{&refs[0]}, {&refs[1]}}); // [A, BC] - refs.push_back(graph->AddTransaction(feerateC)); + graph->AddTransaction(refs.emplace_back(), feerateC); graph->AddDependency(/*parent=*/refs[1], /*child=*/refs[2]); BOOST_CHECK_EQUAL(graph->GetTransactionCount(TxGraph::Level::TOP), 3); block_builder_checker({{&refs[0]}, {&refs[1], &refs[2]}}); // [ABCD] - refs.push_back(graph->AddTransaction(feerateD)); + graph->AddTransaction(refs.emplace_back(), feerateD); graph->AddDependency(/*parent=*/refs[2], /*child=*/refs[3]); BOOST_CHECK_EQUAL(graph->GetTransactionCount(TxGraph::Level::TOP), 4); block_builder_checker({{&refs[0], &refs[1], &refs[2], &refs[3]}}); @@ -383,7 +384,7 @@ BOOST_AUTO_TEST_CASE(txgraph_staging) // everytime adding a transaction, test the chunk status // [A] - refs.push_back(graph->AddTransaction(feerateA)); + graph->AddTransaction(refs.emplace_back(), feerateA); BOOST_CHECK_EQUAL(graph->HaveStaging(), false); BOOST_CHECK_EQUAL(graph->GetTransactionCount(TxGraph::Level::TOP), 1); @@ -392,7 +393,7 @@ BOOST_AUTO_TEST_CASE(txgraph_staging) BOOST_CHECK_EQUAL(graph->GetTransactionCount(TxGraph::Level::TOP), 1); // [A, B] - refs.push_back(graph->AddTransaction(feerateB)); + graph->AddTransaction(refs.emplace_back(), feerateB); BOOST_CHECK_EQUAL(graph->GetTransactionCount(TxGraph::Level::MAIN), 1); BOOST_CHECK_EQUAL(graph->GetTransactionCount(TxGraph::Level::TOP), 2); BOOST_CHECK_EQUAL(graph->Exists(refs[0], TxGraph::Level::TOP), true); diff --git a/src/test/util/txmempool.cpp b/src/test/util/txmempool.cpp index a019c830e1e..3c7eb87ccc2 100644 --- a/src/test/util/txmempool.cpp +++ b/src/test/util/txmempool.cpp @@ -38,7 +38,7 @@ CTxMemPoolEntry TestMemPoolEntryHelper::FromTx(const CMutableTransaction& tx) co CTxMemPoolEntry TestMemPoolEntryHelper::FromTx(const CTransactionRef& tx) const { - return CTxMemPoolEntry{TxGraph::Ref(), tx, nFee, TicksSinceEpoch(time), nHeight, m_sequence, spendsCoinbase, sigOpCost, lp}; + return CTxMemPoolEntry{tx, nFee, TicksSinceEpoch(time), nHeight, m_sequence, spendsCoinbase, sigOpCost, lp}; } std::optional CheckPackageMempoolAcceptResult(const Package& txns, diff --git a/src/txgraph.cpp b/src/txgraph.cpp index f60d8ca2a66..d9f5900f306 100644 --- a/src/txgraph.cpp +++ b/src/txgraph.cpp @@ -753,7 +753,7 @@ public: // Implementations for the public TxGraph interface. - Ref AddTransaction(const FeePerWeight& feerate) noexcept final; + void AddTransaction(Ref& arg, const FeePerWeight& feerate) noexcept final; void RemoveTransaction(const Ref& arg) noexcept final; void AddDependency(const Ref& parent, const Ref& child) noexcept final; void SetTransactionFee(const Ref&, int64_t fee) noexcept final; @@ -2115,20 +2115,19 @@ void TxGraphImpl::MakeAllAcceptable(int level) noexcept GenericClusterImpl::GenericClusterImpl(uint64_t sequence) noexcept : Cluster{sequence} {} -TxGraph::Ref TxGraphImpl::AddTransaction(const FeePerWeight& feerate) noexcept +void TxGraphImpl::AddTransaction(Ref& arg, const FeePerWeight& feerate) noexcept { Assume(m_main_chunkindex_observers == 0 || GetTopLevel() != 0); Assume(feerate.size > 0); - // Construct a new Ref. - Ref ret; + Assume(GetRefGraph(arg) == nullptr); // Construct a new Entry, and link it with the Ref. auto idx = m_entries.size(); m_entries.emplace_back(); auto& entry = m_entries.back(); entry.m_main_chunkindex_iterator = m_main_chunkindex.end(); - entry.m_ref = &ret; - GetRefGraph(ret) = this; - GetRefIndex(ret) = idx; + entry.m_ref = &arg; + GetRefGraph(arg) = this; + GetRefIndex(arg) = idx; // Construct a new singleton Cluster (which is necessarily optimally linearized). bool oversized = uint64_t(feerate.size) > m_max_cluster_size; auto cluster = CreateEmptyCluster(1); @@ -2146,8 +2145,6 @@ TxGraph::Ref TxGraphImpl::AddTransaction(const FeePerWeight& feerate) noexcept clusterset.m_oversized = true; clusterset.m_group_data = std::nullopt; } - // Return the Ref. - return ret; } void TxGraphImpl::RemoveTransaction(const Ref& arg) noexcept diff --git a/src/txgraph.h b/src/txgraph.h index 0e1f2c79d07..385db0147cc 100644 --- a/src/txgraph.h +++ b/src/txgraph.h @@ -67,12 +67,14 @@ public: /** Virtual destructor, so inheriting is safe. */ virtual ~TxGraph() = default; - /** Construct a new transaction with the specified feerate, and return a Ref to it. + /** Initialize arg (which must be an empty Ref) to refer to a new transaction in this graph + * with the specified feerate. + * * If a staging graph exists, the new transaction is only created there. feerate.size must be - * strictly positive. In all further calls, only Refs created by AddTransaction() are allowed + * strictly positive. In all further calls, only Refs passed to AddTransaction() are allowed * to be passed to this TxGraph object (or empty Ref objects). Ref objects may outlive the - * TxGraph they were created for. */ - [[nodiscard]] virtual Ref AddTransaction(const FeePerWeight& feerate) noexcept = 0; + * TxGraph they were added to. */ + virtual void AddTransaction(Ref& arg, const FeePerWeight& feerate) noexcept = 0; /** Remove the specified transaction. If a staging graph exists, the removal only happens * there. This is a no-op if the transaction was already removed. * @@ -235,8 +237,7 @@ public: /** Index into the Graph's m_entries. Only used if m_graph != nullptr. */ GraphIndex m_index = GraphIndex(-1); public: - /** Construct an empty Ref. Non-empty Refs can only be created using - * TxGraph::AddTransaction. */ + /** Construct an empty Ref. It can be initialized through TxGraph::AddTransaction. */ Ref() noexcept = default; /** Destroy this Ref. If it is not empty, the corresponding transaction is removed (in both * main and staging, if it exists). */ diff --git a/src/txmempool.cpp b/src/txmempool.cpp index 55c332769d4..58262e7f2f9 100644 --- a/src/txmempool.cpp +++ b/src/txmempool.cpp @@ -1000,8 +1000,9 @@ CTxMemPool::ChangeSet::TxHandle CTxMemPool::ChangeSet::StageAddition(const CTran CAmount delta{0}; m_pool->ApplyDelta(tx->GetHash(), delta); - TxGraph::Ref ref(m_pool->m_txgraph->AddTransaction(FeePerWeight(fee, GetSigOpsAdjustedWeight(GetTransactionWeight(*tx), sigops_cost, ::nBytesPerSigOp)))); - auto newit = m_to_add.emplace(std::move(ref), tx, fee, time, entry_height, entry_sequence, spends_coinbase, sigops_cost, lp).first; + FeePerWeight feerate(fee, GetSigOpsAdjustedWeight(GetTransactionWeight(*tx), sigops_cost, ::nBytesPerSigOp)); + auto newit = m_to_add.emplace(tx, fee, time, entry_height, entry_sequence, spends_coinbase, sigops_cost, lp).first; + m_pool->m_txgraph->AddTransaction(const_cast(*newit), feerate); if (delta) { newit->UpdateModifiedFee(delta); m_pool->m_txgraph->SetTransactionFee(*newit, newit->GetModifiedFee());