From 12bae7073cf142a2c5382d1525d1690529514eac Mon Sep 17 00:00:00 2001 From: MarcoFalke Date: Wed, 1 Dec 2021 17:55:44 +0100 Subject: [PATCH] Merge bitcoin/bitcoin#22677: cut the validation <-> txmempool circular dependency 2/2 a64078e38563ef3ac5e5ec20c07569441c87eeee Break validation <-> txmempool circular dependency (glozow) 64e4963c635ec3a73a5fa3f32f6ec08e70609f60 [mempool] always assert coin spent (glozow) bb9078ed51159fa162484f16993313ed6cf980e3 [refactor] put finality and maturity checking into a lambda (glozow) bedf246f1e2497a3641093c6e8fa11fb34dddac4 [mempool] only update lockpoints for non-removed entries (glozow) 1b3a11e126b258fba975ed7c452221608f2c5472 MOVEONLY: TestLockPointValidity to txmempool (glozow) Pull request description: Remove 2 circular dependencies: validation - txmempool and validation - policy/rbf - txmempool Validation should depend on txmempool (e.g. `CChainstateManager` has a mempool and we often need to know what's in our mempool to validate transactions), but txmempool is a data structure that shouldn't really need to know about chain state. - Changes `removeForReorg()` to be parameterized by a callable that returns true/false (i.e. whether the transaction should be removed due to being now immature or nonfinal) instead of a `CChainState`. The mempool really shouldn't need to know about coinbase maturity or lockpoints, it just needs to know which entries to remove. ACKs for top commit: laanwj: Code review ACK a64078e38563ef3ac5e5ec20c07569441c87eeee mjdietzx: reACK a64078e38563ef3ac5e5ec20c07569441c87eeee theStack: re-ACK a64078e38563ef3ac5e5ec20c07569441c87eeee Tree-SHA512: f75995200569c09dfb8ddc09729da66ddb32167ff1e8a7e72f105ec062d2d6a9a390e6b4a2a115e7ad8ad3525f891ee1503f3cd2bed11773abcaf7c3230b1136 --- src/txmempool.cpp | 55 +++++++++++++------------ src/txmempool.h | 11 ++++- src/validation.cpp | 51 ++++++++++++++--------- src/validation.h | 5 --- test/lint/lint-circular-dependencies.sh | 1 - 5 files changed, 70 insertions(+), 53 deletions(-) diff --git a/src/txmempool.cpp b/src/txmempool.cpp index 0f6711aefe3cff..df00c63444cdb9 100644 --- a/src/txmempool.cpp +++ b/src/txmempool.cpp @@ -30,6 +30,24 @@ #include #include +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_count, LockPoints lp) @@ -807,44 +825,27 @@ void CTxMemPool::removeRecursive(const CTransaction &origTx, MemPoolRemovalReaso RemoveStaged(setAllRemoves, false, reason); } -void CTxMemPool::removeForReorg(CChainState& active_chainstate, int flags) EXCLUSIVE_LOCKS_REQUIRED(cs_main) +void CTxMemPool::removeForReorg(CChain& chain, std::function check_final_and_mature) EXCLUSIVE_LOCKS_REQUIRED(cs_main) { // Remove transactions spending a coinbase which are now immature and no-longer-final transactions AssertLockHeld(cs); + AssertLockHeld(::cs_main); + setEntries txToRemove; 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); - 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. - txToRemove.insert(it); - } 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); - 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); - break; - } - } - } - if (!validLP) { - mapTx.modify(it, update_lock_points(lp)); - } + if (check_final_and_mature(it)) txToRemove.insert(it); } setEntries setAllRemoves; for (txiter it : txToRemove) { CalculateDescendants(it, setAllRemoves); } RemoveStaged(setAllRemoves, false, MemPoolRemovalReason::REORG); + 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) diff --git a/src/txmempool.h b/src/txmempool.h index 5193856924ea66..ae46834e5afe8b 100644 --- a/src/txmempool.h +++ b/src/txmempool.h @@ -17,6 +17,7 @@ #include #include #include +#include #include #include #include @@ -59,6 +60,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&) @@ -667,7 +673,10 @@ class CTxMemPool bool removeSpentIndex(const uint256 txhash); 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 removeProTxPubKeyConflicts(const CTransaction &tx, const CKeyID &keyId) EXCLUSIVE_LOCKS_REQUIRED(cs); void removeProTxPubKeyConflicts(const CTransaction &tx, const CBLSLazyPublicKey &pubKey) EXCLUSIVE_LOCKS_REQUIRED(cs); diff --git a/src/validation.cpp b/src/validation.cpp index f32d870d09907d..f16333bacef5e6 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -173,24 +173,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 ::ChainActive() 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, @@ -390,8 +372,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/src/validation.h b/src/validation.h index 5223a6583efec3..2b4e019b52357e 100644 --- a/src/validation.h +++ b/src/validation.h @@ -276,11 +276,6 @@ int GetUTXOConfirmations(CChainState& active_chainstate, const COutPoint& outpoi */ 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, diff --git a/test/lint/lint-circular-dependencies.sh b/test/lint/lint-circular-dependencies.sh index 603f79baf94d3e..d73d844984b5e0 100755 --- a/test/lint/lint-circular-dependencies.sh +++ b/test/lint/lint-circular-dependencies.sh @@ -19,7 +19,6 @@ EXPECTED_CIRCULAR_DEPENDENCIES=( "qt/bitcoingui -> qt/walletframe -> qt/bitcoingui" "qt/recentrequeststablemodel -> qt/walletmodel -> qt/recentrequeststablemodel" "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"