From 1b3a11e126b258fba975ed7c452221608f2c5472 Mon Sep 17 00:00:00 2001 From: glozow Date: Tue, 30 Nov 2021 11:21:02 +0000 Subject: [PATCH 1/5] MOVEONLY: TestLockPointValidity to txmempool --- src/txmempool.cpp | 18 ++++++++++++++++++ src/txmempool.h | 6 ++++++ src/validation.cpp | 18 ------------------ src/validation.h | 5 ----- 4 files changed, 24 insertions(+), 23 deletions(-) diff --git a/src/txmempool.cpp b/src/txmempool.cpp index 502a27dc6b..05b22bb39b 100644 --- a/src/txmempool.cpp +++ b/src/txmempool.cpp @@ -74,6 +74,24 @@ private: const LockPoints& lp; }; +bool TestLockPointValidity(CChain& active_chain, const LockPoints* lp) +{ + AssertLockHeld(cs_main); + assert(lp); + // If there are relative lock times then the maxInputBlock will be set + // If there are no relative lock times, the LockPoints don't depend on the chain + if (lp->maxInputBlock) { + // Check whether active_chain is an extension of the block at which the LockPoints + // calculation was valid. If not LockPoints are no longer valid + if (!active_chain.Contains(lp->maxInputBlock)) { + return false; + } + } + + // LockPoints still valid + return true; +} + CTxMemPoolEntry::CTxMemPoolEntry(const CTransactionRef& tx, CAmount fee, int64_t time, unsigned int entry_height, bool spends_coinbase, int64_t sigops_cost, LockPoints lp) diff --git a/src/txmempool.h b/src/txmempool.h index 85417ac3fc..412c2fbb74 100644 --- a/src/txmempool.h +++ b/src/txmempool.h @@ -14,6 +14,7 @@ #include #include +#include #include #include #include @@ -49,6 +50,11 @@ struct LockPoints { CBlockIndex* maxInputBlock{nullptr}; }; +/** + * Test whether the LockPoints height and time are still valid on the current chain + */ +bool TestLockPointValidity(CChain& active_chain, const LockPoints* lp) EXCLUSIVE_LOCKS_REQUIRED(cs_main); + struct CompareIteratorByHash { // SFINAE for T where T is either a pointer type (e.g., a txiter) or a reference_wrapper // (e.g. a wrapped CTxMemPoolEntry&) diff --git a/src/validation.cpp b/src/validation.cpp index c96adb77ff..322e77afe6 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -212,24 +212,6 @@ bool CheckFinalTx(const CBlockIndex* active_chain_tip, const CTransaction &tx, i return IsFinalTx(tx, nBlockHeight, nBlockTime); } -bool TestLockPointValidity(CChain& active_chain, const LockPoints* lp) -{ - AssertLockHeld(cs_main); - assert(lp); - // If there are relative lock times then the maxInputBlock will be set - // If there are no relative lock times, the LockPoints don't depend on the chain - if (lp->maxInputBlock) { - // Check whether active_chain is an extension of the block at which the LockPoints - // calculation was valid. If not LockPoints are no longer valid - if (!active_chain.Contains(lp->maxInputBlock)) { - return false; - } - } - - // LockPoints still valid - return true; -} - bool CheckSequenceLocks(CBlockIndex* tip, const CCoinsView& coins_view, const CTransaction& tx, diff --git a/src/validation.h b/src/validation.h index 21cd389757..a81188b5f2 100644 --- a/src/validation.h +++ b/src/validation.h @@ -249,11 +249,6 @@ PackageMempoolAcceptResult ProcessNewPackage(CChainState& active_chainstate, CTx */ bool CheckFinalTx(const CBlockIndex* active_chain_tip, const CTransaction &tx, int flags = -1) EXCLUSIVE_LOCKS_REQUIRED(cs_main); -/** - * Test whether the LockPoints height and time are still valid on the current chain - */ -bool TestLockPointValidity(CChain& active_chain, const LockPoints* lp) EXCLUSIVE_LOCKS_REQUIRED(cs_main); - /** * Check if transaction will be BIP68 final in the next block to be created on top of tip. * @param[in] tip Chain tip to check tx sequence locks against. For example, From bedf246f1e2497a3641093c6e8fa11fb34dddac4 Mon Sep 17 00:00:00 2001 From: glozow Date: Tue, 30 Nov 2021 11:15:58 +0000 Subject: [PATCH 2/5] [mempool] only update lockpoints for non-removed entries Each entry's lockpoints are independent of one another, so there isn't any reason to update lockpoints for entries that will be removed. Separating the loops also allows us to move validation logic out and leave lockpoints in txmempool. --- src/txmempool.cpp | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/src/txmempool.cpp b/src/txmempool.cpp index 05b22bb39b..4cde0993a8 100644 --- a/src/txmempool.cpp +++ b/src/txmempool.cpp @@ -642,7 +642,7 @@ void CTxMemPool::removeForReorg(CChainState& active_chainstate, int flags) for (indexed_transaction_set::const_iterator it = mapTx.begin(); it != mapTx.end(); it++) { const CTransaction& tx = it->GetTx(); LockPoints lp = it->GetLockPoints(); - bool validLP = TestLockPointValidity(active_chainstate.m_chain, &lp); + 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)) { @@ -663,15 +663,19 @@ void CTxMemPool::removeForReorg(CChainState& active_chainstate, int flags) } } } - if (!validLP) { - mapTx.modify(it, update_lock_points(lp)); - } } setEntries setAllRemoves; for (txiter it : txToRemove) { 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)) { + mapTx.modify(it, update_lock_points(lp)); + } + } } void CTxMemPool::removeConflicts(const CTransaction &tx) From bb9078ed51159fa162484f16993313ed6cf980e3 Mon Sep 17 00:00:00 2001 From: glozow Date: Wed, 29 Sep 2021 20:30:32 +0100 Subject: [PATCH 3/5] [refactor] put finality and maturity checking into a lambda No behavior change. --- src/txmempool.cpp | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-) diff --git a/src/txmempool.cpp b/src/txmempool.cpp index 4cde0993a8..088a560e15 100644 --- a/src/txmempool.cpp +++ b/src/txmempool.cpp @@ -638,8 +638,13 @@ void CTxMemPool::removeForReorg(CChainState& active_chainstate, int flags) { // Remove transactions spending a coinbase which are now immature and no-longer-final transactions AssertLockHeld(cs); - setEntries txToRemove; - for (indexed_transaction_set::const_iterator it = mapTx.begin(); it != mapTx.end(); it++) { + 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); @@ -648,7 +653,7 @@ void CTxMemPool::removeForReorg(CChainState& active_chainstate, int 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. - txToRemove.insert(it); + should_remove = true; } else if (it->GetSpendsCoinbase()) { for (const CTxIn& txin : tx.vin) { indexed_transaction_set::const_iterator it2 = mapTx.find(txin.prevout.hash); @@ -658,11 +663,17 @@ void CTxMemPool::removeForReorg(CChainState& active_chainstate, int flags) if (m_check_ratio != 0) 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)) { - txToRemove.insert(it); + 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); } setEntries setAllRemoves; for (txiter it : txToRemove) { From 64e4963c635ec3a73a5fa3f32f6ec08e70609f60 Mon Sep 17 00:00:00 2001 From: glozow Date: Wed, 29 Sep 2021 20:55:07 +0100 Subject: [PATCH 4/5] [mempool] always assert coin spent This is an extremely cheap function (just checks that the coin CTxOut isn't null) that doesn't need to be gated on check_ratio. --- src/txmempool.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/txmempool.cpp b/src/txmempool.cpp index 088a560e15..43b15be83d 100644 --- a/src/txmempool.cpp +++ b/src/txmempool.cpp @@ -660,7 +660,7 @@ void CTxMemPool::removeForReorg(CChainState& active_chainstate, int flags) if (it2 != mapTx.end()) continue; const Coin &coin = active_chainstate.CoinsTip().AccessCoin(txin.prevout); - if (m_check_ratio != 0) assert(!coin.IsSpent()); + 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; From a64078e38563ef3ac5e5ec20c07569441c87eeee Mon Sep 17 00:00:00 2001 From: glozow Date: Wed, 29 Sep 2021 20:52:04 +0100 Subject: [PATCH 5/5] 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 43b15be83d..27fbb8acac 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 412c2fbb74..c6e08a3ca5 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 322e77afe6..714eab1f77 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 ab3866d23e..69185090d1 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"