From a64078e38563ef3ac5e5ec20c07569441c87eeee Mon Sep 17 00:00:00 2001 From: glozow Date: Wed, 29 Sep 2021 20:52:04 +0100 Subject: [PATCH] Break validation <-> txmempool circular dependency No behavior change. Parameterize removeForReorg using a CChain and callable that encapsulates validation logic. The mempool shouldn't need to know a bunch of details about coinbase maturity and lock finality. Instead, just pass in a callable function that says true/false. Breaks circular dependency by removing txmempool's dependency on validation. --- src/txmempool.cpp | 35 +------------------------ src/txmempool.h | 5 +++- src/validation.cpp | 33 ++++++++++++++++++++++- test/lint/lint-circular-dependencies.sh | 2 -- 4 files changed, 37 insertions(+), 38 deletions(-) diff --git a/src/txmempool.cpp b/src/txmempool.cpp index 43b15be83de..27fbb8acac7 100644 --- a/src/txmempool.cpp +++ b/src/txmempool.cpp @@ -16,7 +16,6 @@ #include #include #include -#include #include #include @@ -634,43 +633,12 @@ void CTxMemPool::removeRecursive(const CTransaction &origTx, MemPoolRemovalReaso RemoveStaged(setAllRemoves, false, reason); } -void CTxMemPool::removeForReorg(CChainState& active_chainstate, int flags) +void CTxMemPool::removeForReorg(CChain& chain, std::function check_final_and_mature) { // Remove transactions spending a coinbase which are now immature and no-longer-final transactions AssertLockHeld(cs); AssertLockHeld(::cs_main); - const auto check_final_and_mature = [this, &active_chainstate, flags](txiter it) - EXCLUSIVE_LOCKS_REQUIRED(cs, ::cs_main) { - bool should_remove = false; - AssertLockHeld(cs); - AssertLockHeld(::cs_main); - const CTransaction& tx = it->GetTx(); - LockPoints lp = it->GetLockPoints(); - const bool validLP = TestLockPointValidity(active_chainstate.m_chain, &lp); - CCoinsViewMemPool view_mempool(&active_chainstate.CoinsTip(), *this); - if (!CheckFinalTx(active_chainstate.m_chain.Tip(), tx, flags) - || !CheckSequenceLocks(active_chainstate.m_chain.Tip(), view_mempool, tx, flags, &lp, validLP)) { - // Note if CheckSequenceLocks fails the LockPoints may still be invalid - // So it's critical that we remove the tx and not depend on the LockPoints. - should_remove = true; - } else if (it->GetSpendsCoinbase()) { - for (const CTxIn& txin : tx.vin) { - indexed_transaction_set::const_iterator it2 = mapTx.find(txin.prevout.hash); - if (it2 != mapTx.end()) - continue; - const Coin &coin = active_chainstate.CoinsTip().AccessCoin(txin.prevout); - assert(!coin.IsSpent()); - unsigned int nMemPoolHeight = active_chainstate.m_chain.Tip()->nHeight + 1; - if (coin.IsSpent() || (coin.IsCoinBase() && ((signed long)nMemPoolHeight) - coin.nHeight < COINBASE_MATURITY)) { - should_remove = true; - break; - } - } - } - return should_remove; - }; - setEntries txToRemove; for (indexed_transaction_set::const_iterator it = mapTx.begin(); it != mapTx.end(); it++) { if (check_final_and_mature(it)) txToRemove.insert(it); @@ -680,7 +648,6 @@ void CTxMemPool::removeForReorg(CChainState& active_chainstate, int flags) CalculateDescendants(it, setAllRemoves); } RemoveStaged(setAllRemoves, false, MemPoolRemovalReason::REORG); - auto chain = active_chainstate.m_chain; for (indexed_transaction_set::const_iterator it = mapTx.begin(); it != mapTx.end(); it++) { LockPoints lp = it->GetLockPoints(); if (!TestLockPointValidity(chain, &lp)) { diff --git a/src/txmempool.h b/src/txmempool.h index 412c2fbb743..c6e08a3ca57 100644 --- a/src/txmempool.h +++ b/src/txmempool.h @@ -589,7 +589,10 @@ public: void addUnchecked(const CTxMemPoolEntry& entry, setEntries& setAncestors, bool validFeeEstimate = true) EXCLUSIVE_LOCKS_REQUIRED(cs, cs_main); void removeRecursive(const CTransaction& tx, MemPoolRemovalReason reason) EXCLUSIVE_LOCKS_REQUIRED(cs); - void removeForReorg(CChainState& active_chainstate, int flags) EXCLUSIVE_LOCKS_REQUIRED(cs, cs_main); + /** After reorg, check if mempool entries are now non-final, premature coinbase spends, or have + * invalid lockpoints. Update lockpoints and remove entries (and descendants of entries) that + * are no longer valid. */ + void removeForReorg(CChain& chain, std::function check_final_and_mature) EXCLUSIVE_LOCKS_REQUIRED(cs, cs_main); void removeConflicts(const CTransaction& tx) EXCLUSIVE_LOCKS_REQUIRED(cs); void removeForBlock(const std::vector& vtx, unsigned int nBlockHeight) EXCLUSIVE_LOCKS_REQUIRED(cs); diff --git a/src/validation.cpp b/src/validation.cpp index 322e77afe6c..714eab1f77f 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -350,8 +350,39 @@ void CChainState::MaybeUpdateMempoolForReorg( // the disconnectpool that were added back and cleans up the mempool state. m_mempool->UpdateTransactionsFromBlock(vHashUpdate); + const auto check_final_and_mature = [this, flags=STANDARD_LOCKTIME_VERIFY_FLAGS](CTxMemPool::txiter it) + EXCLUSIVE_LOCKS_REQUIRED(m_mempool->cs, ::cs_main) { + bool should_remove = false; + AssertLockHeld(m_mempool->cs); + AssertLockHeld(::cs_main); + const CTransaction& tx = it->GetTx(); + LockPoints lp = it->GetLockPoints(); + bool validLP = TestLockPointValidity(m_chain, &lp); + CCoinsViewMemPool view_mempool(&CoinsTip(), *m_mempool); + if (!CheckFinalTx(m_chain.Tip(), tx, flags) + || !CheckSequenceLocks(m_chain.Tip(), view_mempool, tx, flags, &lp, validLP)) { + // Note if CheckSequenceLocks fails the LockPoints may still be invalid + // So it's critical that we remove the tx and not depend on the LockPoints. + should_remove = true; + } else if (it->GetSpendsCoinbase()) { + for (const CTxIn& txin : tx.vin) { + auto it2 = m_mempool->mapTx.find(txin.prevout.hash); + if (it2 != m_mempool->mapTx.end()) + continue; + const Coin &coin = CoinsTip().AccessCoin(txin.prevout); + assert(!coin.IsSpent()); + unsigned int nMemPoolHeight = m_chain.Tip()->nHeight + 1; + if (coin.IsSpent() || (coin.IsCoinBase() && ((signed long)nMemPoolHeight) - coin.nHeight < COINBASE_MATURITY)) { + should_remove = true; + break; + } + } + } + return should_remove; + }; + // We also need to remove any now-immature transactions - m_mempool->removeForReorg(*this, STANDARD_LOCKTIME_VERIFY_FLAGS); + m_mempool->removeForReorg(m_chain, check_final_and_mature); // Re-limit mempool size, in case we added any transactions LimitMempoolSize( *m_mempool, diff --git a/test/lint/lint-circular-dependencies.sh b/test/lint/lint-circular-dependencies.sh index ab3866d23e4..69185090d12 100755 --- a/test/lint/lint-circular-dependencies.sh +++ b/test/lint/lint-circular-dependencies.sh @@ -15,12 +15,10 @@ EXPECTED_CIRCULAR_DEPENDENCIES=( "index/base -> validation -> index/blockfilterindex -> index/base" "index/coinstatsindex -> node/coinstats -> index/coinstatsindex" "policy/fees -> txmempool -> policy/fees" - "policy/rbf -> txmempool -> validation -> policy/rbf" "qt/addresstablemodel -> qt/walletmodel -> qt/addresstablemodel" "qt/recentrequeststablemodel -> qt/walletmodel -> qt/recentrequeststablemodel" "qt/sendcoinsdialog -> qt/walletmodel -> qt/sendcoinsdialog" "qt/transactiontablemodel -> qt/walletmodel -> qt/transactiontablemodel" - "txmempool -> validation -> txmempool" "wallet/fees -> wallet/wallet -> wallet/fees" "wallet/wallet -> wallet/walletdb -> wallet/wallet" "node/coinstats -> validation -> node/coinstats"