Skip to content

Commit

Permalink
Merge bitcoin#20750: [Bundle 2/n] Prune g_chainman usage in mempool-r…
Browse files Browse the repository at this point in the history 
…elated validation functions

e8ae1db style-only: Make AcceptToMemoryPool signature readable (Carl Dong)
8f5c100 style-only: Make CheckSequenceLock signature readable (Carl Dong)
8c82481 validation: Use *this in CChainState::LoadMempool (Carl Dong)
0a9a24d validation: Pass in chainstate to UpdateMempoolForReorg (Carl Dong)
7142018 validation: Pass in chainstate to CTxMemPool::removeForReorg (Carl Dong)
71734c6 validation: Pass in chain to ::TestLockPointValidity (Carl Dong)
120aaba tree-wide: Fix erroneous AcceptToMemoryPool replacements (Carl Dong)
417dafc validation: Remove old AcceptToMemoryPool w/o chainstate param (Carl Dong)
3704433 scripted-diff: Invoke ::AcceptToMemoryPool with chainstate (Carl Dong)
229bc37 validation: Pass in chainstate to ::AcceptToMemoryPool (Carl Dong)
d0da7ea validation: Pass in chainstate to ::LoadMempool (Carl Dong)
3a205c4 validation: Pass in chainstate to AcceptToMemoryPoolWithTime (Carl Dong)
d8a8163 validation: Add chainstate member to MemPoolAccept (Carl Dong)
4c15942 validation: Pass in chainstate to ::CheckSequenceLocks (Carl Dong)
577b774 validation: Remove old CheckFinalTx w/o chain tip param (Carl Dong)
7031cf8 scripted-diff: Invoke ::CheckFinalTx with chain tip (Carl Dong)
d015eaa validation: Pass in chain tip to ::CheckFinalTx (Carl Dong)
252b489 validation: Pass in coins tip to CheckInputsFromMempoolAndCache (Carl Dong)
73a6d2b validation: Pass in chainstate to IsCurrentForFeeEstimation (Carl Dong)
d1f932b validation: Pass in coins cache to ::LimitMempoolSize (Carl Dong)

Pull request description:

  Overall PR: bitcoin#20158 (tree-wide: De-globalize ChainstateManager)

  Note to reviewers:
  1. This bundle may _apparently_ introduce usage of `g_chainman` or `::Chain(state|)Active()` globals, but these are resolved later on in the overall PR. [Commits of overall PR](https://github.com/bitcoin/bitcoin/pull/20158/commits)
  2. There may be seemingly obvious local references to `ChainstateManager` or other validation objects which are not being used in callers of the current function in question, this is done intentionally to **_keep each commit centered around one function/method_** to ease review and to make the overall change systematic. We don't assume anything about our callers. Rest assured that once we are considering that particular caller in later commits, we will use the obvious local references. [Commits of overall PR](https://github.com/bitcoin/bitcoin/pull/20158/commits)
  3. When changing a function/method that has many callers (e.g. `LookupBlockIndex` with 55 callers), it is sometimes easier (and less error-prone) to use a scripted-diff. When doing so, there will be 3 commits in sequence so that every commit compiles like so:
  	1. Add `new_function`, make `old_function` a wrapper of `new_function`, divert all calls to `old_function` to `new_function` **in the local module only**
  	2. Scripted-diff to divert all calls to `old_function` to `new_function` **in the rest of the codebase**
  	3. Remove `old_function`

ACKs for top commit:
  glozow:
    reACK bitcoin@e8ae1db via `git range-diff 15f0042...e8ae1db`, only change is fixing ATMP call from conflict
  MarcoFalke:
    ACK e8ae1db 📣

Tree-SHA512: 6af50f04940a69c5c3d3796a24f32f963fa02503cdc1155cc11fff832a99172b407cd163a19793080a5af98580f051b48195b62ec4a797ba2763b4883174153d
  • Loading branch information
MarcoFalke committed Feb 20, 2021
2 parents c46fe4d + e8ae1db commit 828bb77
Show file tree
Hide file tree
Showing 14 changed files with 114 additions and 74 deletions.
2 changes: 1 addition & 1 deletion src/bench/block_assemble.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ static void AssembleBlock(benchmark::Bench& bench)
LOCK(::cs_main); // Required for ::AcceptToMemoryPool.

for (const auto& txr : txs) {
const MempoolAcceptResult res = ::AcceptToMemoryPool(*test_setup.m_node.mempool, txr, false /* bypass_limits */);
const MempoolAcceptResult res = ::AcceptToMemoryPool(::ChainstateActive(), *test_setup.m_node.mempool, txr, false /* bypass_limits */);
assert(res.m_result_type == MempoolAcceptResult::ResultType::VALID);
}
}
Expand Down
4 changes: 2 additions & 2 deletions src/net_processing.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2236,7 +2236,7 @@ void PeerManagerImpl::ProcessOrphanTx(std::set<uint256>& orphan_work_set)
if (orphan_it == mapOrphanTransactions.end()) continue;

const CTransactionRef porphanTx = orphan_it->second.tx;
const MempoolAcceptResult result = AcceptToMemoryPool(m_mempool, porphanTx, false /* bypass_limits */);
const MempoolAcceptResult result = AcceptToMemoryPool(::ChainstateActive(), m_mempool, porphanTx, false /* bypass_limits */);
const TxValidationState& state = result.m_state;

if (result.m_result_type == MempoolAcceptResult::ResultType::VALID) {
Expand Down Expand Up @@ -3258,7 +3258,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
return;
}

const MempoolAcceptResult result = AcceptToMemoryPool(m_mempool, ptx, false /* bypass_limits */);
const MempoolAcceptResult result = AcceptToMemoryPool(::ChainstateActive(), m_mempool, ptx, false /* bypass_limits */);
const TxValidationState& state = result.m_state;

if (result.m_result_type == MempoolAcceptResult::ResultType::VALID) {
Expand Down
2 changes: 1 addition & 1 deletion src/node/interfaces.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -441,7 +441,7 @@ class ChainImpl : public Chain
bool checkFinalTx(const CTransaction& tx) override
{
LOCK(cs_main);
return CheckFinalTx(tx);
return CheckFinalTx(::ChainActive().Tip(), tx);
}
Optional<int> findLocatorFork(const CBlockLocator& locator) override
{
Expand Down
4 changes: 2 additions & 2 deletions src/node/transaction.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ TransactionError BroadcastTransaction(NodeContext& node, const CTransactionRef t
if (max_tx_fee > 0) {
// First, call ATMP with test_accept and check the fee. If ATMP
// fails here, return error immediately.
const MempoolAcceptResult result = AcceptToMemoryPool(*node.mempool, tx, false /* bypass_limits */,
const MempoolAcceptResult result = AcceptToMemoryPool(::ChainstateActive(), *node.mempool, tx, false /* bypass_limits */,
true /* test_accept */);
if (result.m_result_type != MempoolAcceptResult::ResultType::VALID) {
return HandleATMPError(result.m_state, err_string);
Expand All @@ -62,7 +62,7 @@ TransactionError BroadcastTransaction(NodeContext& node, const CTransactionRef t
}
}
// Try to submit the transaction to the mempool.
const MempoolAcceptResult result = AcceptToMemoryPool(*node.mempool, tx, false /* bypass_limits */,
const MempoolAcceptResult result = AcceptToMemoryPool(::ChainstateActive(), *node.mempool, tx, false /* bypass_limits */,
false /* test_accept */);
if (result.m_result_type != MempoolAcceptResult::ResultType::VALID) {
return HandleATMPError(result.m_state, err_string);
Expand Down
2 changes: 1 addition & 1 deletion src/rpc/rawtransaction.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -946,7 +946,7 @@ static RPCHelpMan testmempoolaccept()
result_0.pushKV("txid", tx->GetHash().GetHex());
result_0.pushKV("wtxid", tx->GetWitnessHash().GetHex());

const MempoolAcceptResult accept_result = WITH_LOCK(cs_main, return AcceptToMemoryPool(mempool, std::move(tx),
const MempoolAcceptResult accept_result = WITH_LOCK(cs_main, return AcceptToMemoryPool(::ChainstateActive(), mempool, std::move(tx),
false /* bypass_limits */, /* test_accept */ true));

// Only return the fee and vsize if the transaction would pass ATMP.
Expand Down
12 changes: 6 additions & 6 deletions src/test/miner_tests.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ struct MinerTestingSetup : public TestingSetup {
void TestPackageSelection(const CChainParams& chainparams, const CScript& scriptPubKey, const std::vector<CTransactionRef>& txFirst) EXCLUSIVE_LOCKS_REQUIRED(::cs_main, m_node.mempool->cs);
bool TestSequenceLocks(const CTransaction& tx, int flags) EXCLUSIVE_LOCKS_REQUIRED(::cs_main, m_node.mempool->cs)
{
return CheckSequenceLocks(*m_node.mempool, tx, flags);
return CheckSequenceLocks(::ChainstateActive(), *m_node.mempool, tx, flags);
}
BlockAssembler AssemblerForTest(const CChainParams& params);
};
Expand Down Expand Up @@ -437,7 +437,7 @@ BOOST_AUTO_TEST_CASE(CreateNewBlock_validity)
tx.nLockTime = 0;
hash = tx.GetHash();
m_node.mempool->addUnchecked(entry.Fee(HIGHFEE).Time(GetTime()).SpendsCoinbase(true).FromTx(tx));
BOOST_CHECK(CheckFinalTx(CTransaction(tx), flags)); // Locktime passes
BOOST_CHECK(CheckFinalTx(::ChainActive().Tip(), CTransaction(tx), flags)); // Locktime passes
BOOST_CHECK(!TestSequenceLocks(CTransaction(tx), flags)); // Sequence locks fail
BOOST_CHECK(SequenceLocks(CTransaction(tx), flags, prevheights, CreateBlockIndex(::ChainActive().Tip()->nHeight + 2))); // Sequence locks pass on 2nd block

Expand All @@ -447,7 +447,7 @@ BOOST_AUTO_TEST_CASE(CreateNewBlock_validity)
prevheights[0] = baseheight + 2;
hash = tx.GetHash();
m_node.mempool->addUnchecked(entry.Time(GetTime()).FromTx(tx));
BOOST_CHECK(CheckFinalTx(CTransaction(tx), flags)); // Locktime passes
BOOST_CHECK(CheckFinalTx(::ChainActive().Tip(), CTransaction(tx), flags)); // Locktime passes
BOOST_CHECK(!TestSequenceLocks(CTransaction(tx), flags)); // Sequence locks fail

for (int i = 0; i < CBlockIndex::nMedianTimeSpan; i++)
Expand All @@ -463,7 +463,7 @@ BOOST_AUTO_TEST_CASE(CreateNewBlock_validity)
tx.nLockTime = ::ChainActive().Tip()->nHeight + 1;
hash = tx.GetHash();
m_node.mempool->addUnchecked(entry.Time(GetTime()).FromTx(tx));
BOOST_CHECK(!CheckFinalTx(CTransaction(tx), flags)); // Locktime fails
BOOST_CHECK(!CheckFinalTx(::ChainActive().Tip(), CTransaction(tx), flags)); // Locktime fails
BOOST_CHECK(TestSequenceLocks(CTransaction(tx), flags)); // Sequence locks pass
BOOST_CHECK(IsFinalTx(CTransaction(tx), ::ChainActive().Tip()->nHeight + 2, ::ChainActive().Tip()->GetMedianTimePast())); // Locktime passes on 2nd block

Expand All @@ -474,7 +474,7 @@ BOOST_AUTO_TEST_CASE(CreateNewBlock_validity)
prevheights[0] = baseheight + 4;
hash = tx.GetHash();
m_node.mempool->addUnchecked(entry.Time(GetTime()).FromTx(tx));
BOOST_CHECK(!CheckFinalTx(CTransaction(tx), flags)); // Locktime fails
BOOST_CHECK(!CheckFinalTx(::ChainActive().Tip(), CTransaction(tx), flags)); // Locktime fails
BOOST_CHECK(TestSequenceLocks(CTransaction(tx), flags)); // Sequence locks pass
BOOST_CHECK(IsFinalTx(CTransaction(tx), ::ChainActive().Tip()->nHeight + 2, ::ChainActive().Tip()->GetMedianTimePast() + 1)); // Locktime passes 1 second later

Expand All @@ -483,7 +483,7 @@ BOOST_AUTO_TEST_CASE(CreateNewBlock_validity)
prevheights[0] = ::ChainActive().Tip()->nHeight + 1;
tx.nLockTime = 0;
tx.vin[0].nSequence = 0;
BOOST_CHECK(CheckFinalTx(CTransaction(tx), flags)); // Locktime passes
BOOST_CHECK(CheckFinalTx(::ChainActive().Tip(), CTransaction(tx), flags)); // Locktime passes
BOOST_CHECK(TestSequenceLocks(CTransaction(tx), flags)); // Sequence locks pass
tx.vin[0].nSequence = 1;
BOOST_CHECK(!TestSequenceLocks(CTransaction(tx), flags)); // Sequence locks fail
Expand Down
2 changes: 1 addition & 1 deletion src/test/txvalidation_tests.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ BOOST_FIXTURE_TEST_CASE(tx_mempool_reject_coinbase, TestChain100Setup)
LOCK(cs_main);

unsigned int initialPoolSize = m_node.mempool->size();
const MempoolAcceptResult result = AcceptToMemoryPool(*m_node.mempool, MakeTransactionRef(coinbaseTx),
const MempoolAcceptResult result = AcceptToMemoryPool(::ChainstateActive(), *m_node.mempool, MakeTransactionRef(coinbaseTx),
true /* bypass_limits */);

BOOST_CHECK(result.m_result_type == MempoolAcceptResult::ResultType::INVALID);
Expand Down
2 changes: 1 addition & 1 deletion src/test/txvalidationcache_tests.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ BOOST_FIXTURE_TEST_CASE(tx_mempool_block_doublespend, TestChain100Setup)
const auto ToMemPool = [this](const CMutableTransaction& tx) {
LOCK(cs_main);

const MempoolAcceptResult result = AcceptToMemoryPool(*m_node.mempool, MakeTransactionRef(tx),
const MempoolAcceptResult result = AcceptToMemoryPool(::ChainstateActive(), *m_node.mempool, MakeTransactionRef(tx),
true /* bypass_limits */);
return result.m_result_type == MempoolAcceptResult::ResultType::VALID;
};
Expand Down
2 changes: 1 addition & 1 deletion src/test/util/setup_common.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -303,7 +303,7 @@ CMutableTransaction TestChain100Setup::CreateValidMempoolTransaction(CTransactio
// Add transaction to the mempool
{
LOCK(cs_main);
const MempoolAcceptResult result = AcceptToMemoryPool(*m_node.mempool.get(), MakeTransactionRef(mempool_txn), /* bypass_limits */ false);
const MempoolAcceptResult result = AcceptToMemoryPool(::ChainstateActive(), *m_node.mempool.get(), MakeTransactionRef(mempool_txn), /* bypass_limits */ false);
assert(result.m_result_type == MempoolAcceptResult::ResultType::VALID);
}

Expand Down
2 changes: 1 addition & 1 deletion src/test/validation_block_tests.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -273,7 +273,7 @@ BOOST_AUTO_TEST_CASE(mempool_locks_reorg)
{
LOCK(cs_main);
for (const auto& tx : txs) {
const MempoolAcceptResult result = AcceptToMemoryPool(*m_node.mempool, tx, false /* bypass_limits */);
const MempoolAcceptResult result = AcceptToMemoryPool(::ChainstateActive(), *m_node.mempool, tx, false /* bypass_limits */);
BOOST_REQUIRE(result.m_result_type == MempoolAcceptResult::ResultType::VALID);
}
}
Expand Down
10 changes: 6 additions & 4 deletions src/txmempool.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -503,16 +503,17 @@ void CTxMemPool::removeRecursive(const CTransaction &origTx, MemPoolRemovalReaso
RemoveStaged(setAllRemoves, false, reason);
}

void CTxMemPool::removeForReorg(const CCoinsViewCache *pcoins, unsigned int nMemPoolHeight, int flags)
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++) {
const CTransaction& tx = it->GetTx();
LockPoints lp = it->GetLockPoints();
bool validLP = TestLockPointValidity(&lp);
if (!CheckFinalTx(tx, flags) || !CheckSequenceLocks(*this, tx, flags, &lp, validLP)) {
assert(std::addressof(::ChainstateActive()) == std::addressof(active_chainstate));
bool validLP = TestLockPointValidity(active_chainstate.m_chain, &lp);
if (!CheckFinalTx(active_chainstate.m_chain.Tip(), tx, flags) || !CheckSequenceLocks(active_chainstate, *this, 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);
Expand All @@ -521,8 +522,9 @@ void CTxMemPool::removeForReorg(const CCoinsViewCache *pcoins, unsigned int nMem
indexed_transaction_set::const_iterator it2 = mapTx.find(txin.prevout.hash);
if (it2 != mapTx.end())
continue;
const Coin &coin = pcoins->AccessCoin(txin.prevout);
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;
Expand Down
3 changes: 2 additions & 1 deletion src/txmempool.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
#include <boost/multi_index/sequenced_index.hpp>

class CBlockIndex;
class CChainState;
extern RecursiveMutex cs_main;

/** Fake height value used in Coin to signify they are only in the memory pool (since 0.8) */
Expand Down Expand Up @@ -616,7 +617,7 @@ class CTxMemPool
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(const CCoinsViewCache* pcoins, unsigned int nMemPoolHeight, int flags) EXCLUSIVE_LOCKS_REQUIRED(cs, cs_main);
void removeForReorg(CChainState& active_chainstate, int flags) 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);

Expand Down
Loading

0 comments on commit 828bb77

Please sign in to comment.