foster/bitcoin-bitcoin-core
0
0
Fork 0
mirror of https://github.com/bitcoin/bitcoin.git synced 2025-02-02 09:46:52 -05:00
Code Issues Activity

Break validation <-> txmempool circular dependency

Browse source 
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.
This commit is contained in:
glozow 2021-09-29 20:52:04 +01:00
parent 64e4963c63
commit a64078e385
4 changed files with 37 additions and 38 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

35
src/txmempool.cpp
View file

@ -16,7 +16,6 @@
#include <util/moneystr.h>
#include <util/system.h>
#include <util/time.h>
#include <validation.h>
#include <validationinterface.h>
#include <cmath>
@ -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<bool(txiter)> 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)) {

5
src/txmempool.h
View file

@ -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<bool(txiter)> check_final_and_mature) EXCLUSIVE_LOCKS_REQUIRED(cs, cs_main);
void removeConflicts(const CTransaction& tx) EXCLUSIVE_LOCKS_REQUIRED(cs);
void removeForBlock(const std::vector<CTransactionRef>& vtx, unsigned int nBlockHeight) EXCLUSIVE_LOCKS_REQUIRED(cs);

33
src/validation.cpp
View file

@ -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,

2
test/lint/lint-circular-dependencies.sh
View file

@ -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"