Add transactions to txgraph, but without cluster dependencies
What the developer wrote
Authored by Suhas Daftuar
Add transactions to txgraph, but without cluster dependencies
Effectively this is treating all transactions in txgraph as being in a cluster
of size 1.
What changed, and why it matters
This commit is a step in a larger Bitcoin Core project to replace the mempool's internal transaction graph with a new component called TxGraph. The change starts tracking every mempool transaction inside TxGraph, but deliberately ignores parent-child dependencies between transactions, treating each one as if it were isolated. It also restructures how mempool locks are held during transaction acceptance so the new graph can be updated consistently. There is no claim in the commit that this fixes a security bug; it reads like an incremental refactor or feature-in-progress.
Treat this as a normal code-review item in a refactor series. Verify that TxGraph::AddTransaction, RemoveTransaction, SetTransactionFee, StartStaging, AbortStaging, and CommitStaging maintain invariants when exceptions occur, when the same transaction is staged twice, and when PrioritiseTransaction is called while a ChangeSet is active. Confirm that holding m_pool.cs across the new wrapper methods does not increase lock contention or deadlock risk with cs_main. No immediate security patch action is indicated by the commit itself.
Security signals we found
New mempool subsystem (TxGraph) is being integrated and now receives all add/remove/fee-update events
Locking discipline changed: mempool lock is acquired earlier and held across single/multiple transaction acceptance wrappers
Staging/abort/commit pattern added for batched mempool mutations; abort path is in destructor
Fee-delta propagation added to graph, which may affect mempool eviction/mining priority calculations
No input validation, bounds checking, or resource-limit changes visible in the diff
Evidence from the diff
The patch moves m_txgraph from a private member to a public member of CTxMemPool and wires it into the mempool’s ChangeSet staging/apply workflow. StageAddition now calls m_txgraph->AddTransaction with a fee-per-weight value and stores the returned TxGraph::Ref in the new entry. StageRemoval now calls m_txgraph->RemoveTransaction. PrioritiseTransaction propagates fee deltas into the graph. ChangeSet constructor/destructor manage a staging mode on the graph (StartStaging/AbortStaging), and Apply calls CommitStaging. In validation.cpp, AcceptSingleTransaction and AcceptMultipleTransactions are renamed to internal variants that require the mempool lock already be held, and new wrapper methods take the lock and clear subpackage state. The commit message explicitly says cluster dependencies are not yet handled, so the graph currently treats every transaction as its own size-1 cluster.
Changed components
src/txmempool.cppsrc/txmempool.hsrc/validation.cppCTxMemPool::ChangeSetCTxMemPool::ApplyCTxMemPool::PrioritiseTransactionMemPoolAccept transaction acceptance pathsInspect captured patch +55 / −17
diff --git a/src/txmempool.cpp b/src/txmempool.cpp
index c931b361..dc8ec8ed 100644
--- a/src/txmempool.cpp
+++ b/src/txmempool.cpp
@@ -435,6 +435,8 @@ void CTxMemPool::AddTransactionsUpdated(unsigned int n)
void CTxMemPool::Apply(ChangeSet* changeset)
{
AssertLockHeld(cs);
+ m_txgraph->CommitStaging();
+
RemoveStaged(changeset->m_to_remove, false, MemPoolRemovalReason::REPLACED);
for (size_t i=0; i<changeset->m_entry_vec.size(); ++i) {
@@ -891,7 +893,10 @@ void CTxMemPool::PrioritiseTransaction(const Txid& hash, const CAmount& nFeeDelt
delta = SaturatingAdd(delta, nFeeDelta);
txiter it = mapTx.find(hash);
if (it != mapTx.end()) {
+ // PrioritiseTransaction calls stack on previous ones. Set the new
+ // transaction fee to be current modified fee + feedelta.
mapTx.modify(it, [&nFeeDelta](CTxMemPoolEntry& e) { e.UpdateModifiedFee(nFeeDelta); });
+ m_txgraph->SetTransactionFee(*it, it->GetModifiedFee());
// Now update all ancestors' modified fees with descendants
auto ancestors{AssumeCalculateMemPoolAncestors(__func__, *it, Limits::NoLimits(), /*fSearchForParents=*/false)};
for (txiter ancestorIt : ancestors) {
@@ -1387,15 +1392,28 @@ CTxMemPool::ChangeSet::TxHandle CTxMemPool::ChangeSet::StageAddition(const CTran
{
LOCK(m_pool->cs);
Assume(m_to_add.find(tx->GetHash()) == m_to_add.end());
- auto newit = m_to_add.emplace(TxGraph::Ref(), tx, fee, time, entry_height, entry_sequence, spends_coinbase, sigops_cost, lp).first;
+
CAmount delta{0};
m_pool->ApplyDelta(tx->GetHash(), delta);
- if (delta) m_to_add.modify(newit, [&delta](CTxMemPoolEntry& e) { e.UpdateModifiedFee(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;
+ if (delta) {
+ m_to_add.modify(newit, [&delta](CTxMemPoolEntry& e) { e.UpdateModifiedFee(delta); });
+ m_pool->m_txgraph->SetTransactionFee(*newit, newit->GetModifiedFee());
+ }
m_entry_vec.push_back(newit);
return newit;
}
+void CTxMemPool::ChangeSet::StageRemoval(CTxMemPool::txiter it)
+{
+ LOCK(m_pool->cs);
+ m_pool->m_txgraph->RemoveTransaction(*it);
+ m_to_remove.insert(it);
+}
+
void CTxMemPool::ChangeSet::Apply()
{
LOCK(m_pool->cs);
diff --git a/src/txmempool.h b/src/txmempool.h
index bc6eb36c..acf55984 100644
--- a/src/txmempool.h
+++ b/src/txmempool.h
@@ -371,6 +371,7 @@ public:
* the mempool is consistent with the new chain tip and fully populated.
*/
mutable RecursiveMutex cs;
+ std::unique_ptr<TxGraph> m_txgraph GUARDED_BY(cs);
indexed_transaction_set mapTx GUARDED_BY(cs);
using txiter = indexed_transaction_set::nth_index<0>::type::const_iterator;
@@ -382,7 +383,6 @@ public:
uint64_t CalculateDescendantMaximum(txiter entry) const EXCLUSIVE_LOCKS_REQUIRED(cs);
private:
- std::unique_ptr<TxGraph> m_txgraph GUARDED_BY(cs);
typedef std::map<txiter, setEntries, CompareIteratorByHash> cacheMap;
@@ -822,8 +822,14 @@ public:
*/
class ChangeSet {
public:
- explicit ChangeSet(CTxMemPool* pool) : m_pool(pool) {}
- ~ChangeSet() EXCLUSIVE_LOCKS_REQUIRED(m_pool->cs) { m_pool->m_have_changeset = false; }
+ explicit ChangeSet(CTxMemPool* pool) : m_pool(pool) { m_pool->m_txgraph->StartStaging(); }
+ ~ChangeSet() EXCLUSIVE_LOCKS_REQUIRED(m_pool->cs) {
+ AssertLockHeld(m_pool->cs);
+ if (m_pool->m_txgraph->HaveStaging()) {
+ m_pool->m_txgraph->AbortStaging();
+ }
+ m_pool->m_have_changeset = false;
+ }
ChangeSet(const ChangeSet&) = delete;
ChangeSet& operator=(const ChangeSet&) = delete;
@@ -831,7 +837,8 @@ public:
using TxHandle = CTxMemPool::txiter;
TxHandle StageAddition(const CTransactionRef& tx, const CAmount fee, int64_t time, unsigned int entry_height, uint64_t entry_sequence, bool spends_coinbase, int64_t sigops_cost, LockPoints lp);
- void StageRemoval(CTxMemPool::txiter it) { m_to_remove.insert(it); }
+
+ void StageRemoval(CTxMemPool::txiter it);
const CTxMemPool::setEntries& GetRemovals() const { return m_to_remove; }
diff --git a/src/validation.cpp b/src/validation.cpp
index 7f97f99b..25831db6 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -591,14 +591,26 @@ public:
void CleanupTemporaryCoins() EXCLUSIVE_LOCKS_REQUIRED(cs_main, m_pool.cs);
// Single transaction acceptance
- MempoolAcceptResult AcceptSingleTransaction(const CTransactionRef& ptx, ATMPArgs& args) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
+ MempoolAcceptResult AcceptSingleTransactionAndCleanup(const CTransactionRef& ptx, ATMPArgs& args) EXCLUSIVE_LOCKS_REQUIRED(cs_main) {
+ LOCK(m_pool.cs);
+ MempoolAcceptResult result = AcceptSingleTransactionInternal(ptx, args);
+ ClearSubPackageState();
+ return result;
+ }
+ MempoolAcceptResult AcceptSingleTransactionInternal(const CTransactionRef& ptx, ATMPArgs& args) EXCLUSIVE_LOCKS_REQUIRED(cs_main, m_pool.cs);
/**
* Multiple transaction acceptance. Transactions may or may not be interdependent, but must not
* conflict with each other, and the transactions cannot already be in the mempool. Parents must
* come before children if any dependencies exist.
*/
- PackageMempoolAcceptResult AcceptMultipleTransactions(const std::vector<CTransactionRef>& txns, ATMPArgs& args) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
+ PackageMempoolAcceptResult AcceptMultipleTransactionsAndCleanup(const std::vector<CTransactionRef>& txns, ATMPArgs& args) EXCLUSIVE_LOCKS_REQUIRED(cs_main) {
+ LOCK(m_pool.cs);
+ PackageMempoolAcceptResult result = AcceptMultipleTransactionsInternal(txns, args);
+ ClearSubPackageState();
+ return result;
+ }
+ PackageMempoolAcceptResult AcceptMultipleTransactionsInternal(const std::vector<CTransactionRef>& txns, ATMPArgs& args) EXCLUSIVE_LOCKS_REQUIRED(cs_main, m_pool.cs);
/**
* Submission of a subpackage.
@@ -1423,10 +1435,10 @@ bool MemPoolAccept::SubmitPackage(const ATMPArgs& args, std::vector<Workspace>&
return all_submitted;
}
-MempoolAcceptResult MemPoolAccept::AcceptSingleTransaction(const CTransactionRef& ptx, ATMPArgs& args)
+MempoolAcceptResult MemPoolAccept::AcceptSingleTransactionInternal(const CTransactionRef& ptx, ATMPArgs& args)
{
AssertLockHeld(cs_main);
- LOCK(m_pool.cs); // mempool "read lock" (held through m_pool.m_opts.signals->TransactionAddedToMempool())
+ AssertLockHeld(m_pool.cs);
Workspace ws(ptx);
const std::vector<Wtxid> single_wtxid{ws.m_ptx->GetWitnessHash()};
@@ -1515,9 +1527,10 @@ MempoolAcceptResult MemPoolAccept::AcceptSingleTransaction(const CTransactionRef
effective_feerate, single_wtxid);
}
-PackageMempoolAcceptResult MemPoolAccept::AcceptMultipleTransactions(const std::vector<CTransactionRef>& txns, ATMPArgs& args)
+PackageMempoolAcceptResult MemPoolAccept::AcceptMultipleTransactionsInternal(const std::vector<CTransactionRef>& txns, ATMPArgs& args)
{
AssertLockHeld(cs_main);
+ AssertLockHeld(m_pool.cs);
// These context-free package limits can be done before taking the mempool lock.
PackageValidationState package_state;
@@ -1529,8 +1542,6 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptMultipleTransactions(const std::
[](const auto& tx) { return Workspace(tx); });
std::map<Wtxid, MempoolAcceptResult> results;
- LOCK(m_pool.cs);
-
// Do all PreChecks first and fail fast to avoid running expensive script checks when unnecessary.
for (Workspace& ws : workspaces) {
if (!PreChecks(args, ws)) {
@@ -1681,11 +1692,11 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptSubPackage(const std::vector<CTr
AssertLockHeld(m_pool.cs);
auto result = [&]() EXCLUSIVE_LOCKS_REQUIRED(::cs_main, m_pool.cs) {
if (subpackage.size() > 1) {
- return AcceptMultipleTransactions(subpackage, args);
+ return AcceptMultipleTransactionsInternal(subpackage, args);
}
const auto& tx = subpackage.front();
ATMPArgs single_args = ATMPArgs::SingleInPackageAccept(args);
- const auto single_res = AcceptSingleTransaction(tx, single_args);
+ const auto single_res = AcceptSingleTransactionInternal(tx, single_args);
PackageValidationState package_state_wrapped;
if (single_res.m_result_type != MempoolAcceptResult::ResultType::VALID) {
package_state_wrapped.Invalid(PackageValidationResult::PCKG_TX, "transaction failed");
@@ -1863,8 +1874,10 @@ MempoolAcceptResult AcceptToMemoryPool(Chainstate& active_chainstate, const CTra
CTxMemPool& pool{*active_chainstate.GetMempool()};
std::vector<COutPoint> coins_to_uncache;
+
auto args = MemPoolAccept::ATMPArgs::SingleAccept(chainparams, accept_time, bypass_limits, coins_to_uncache, test_accept);
- MempoolAcceptResult result = MemPoolAccept(pool, active_chainstate).AcceptSingleTransaction(tx, args);
+ MempoolAcceptResult result = MemPoolAccept(pool, active_chainstate).AcceptSingleTransactionAndCleanup(tx, args);
+
if (result.m_result_type != MempoolAcceptResult::ResultType::VALID) {
// Remove coins that were not present in the coins cache before calling
// AcceptSingleTransaction(); this is to prevent memory DoS in case we receive a large
@@ -1897,7 +1910,7 @@ PackageMempoolAcceptResult ProcessNewPackage(Chainstate& active_chainstate, CTxM
AssertLockHeld(cs_main);
if (test_accept) {
auto args = MemPoolAccept::ATMPArgs::PackageTestAccept(chainparams, GetTime(), coins_to_uncache);
- return MemPoolAccept(pool, active_chainstate).AcceptMultipleTransactions(package, args);
+ return MemPoolAccept(pool, active_chainstate).AcceptMultipleTransactionsAndCleanup(package, args);
} else {
auto args = MemPoolAccept::ATMPArgs::PackageChildWithParents(chainparams, GetTime(), coins_to_uncache, client_maxfeerate);
return MemPoolAccept(pool, active_chainstate).AcceptPackage(package, args);
Why this scored 23/100
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.