Skip to content

refactor: AcceptToMemoryPool #8

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
nully0x wants to merge 2 commits into from
Closed

refactor: AcceptToMemoryPool #8

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9360778
Remove AcceptToMemoryPoolWithTime
lsilva01 Nov 4, 2021
123f5de
Remove calls to global Params() in tx_pool test
lsilva01 Nov 11, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions src/test/fuzz/tx_pool.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ void Finish(FuzzedDataProvider& fuzzed_data_provider, MockedTxPool& tx_pool, CCh
BlockAssembler::Options options;
options.nBlockMaxWeight = fuzzed_data_provider.ConsumeIntegralInRange(0U, MAX_BLOCK_WEIGHT);
options.blockMinFeeRate = CFeeRate{ConsumeMoney(fuzzed_data_provider, /*max=*/COIN)};
auto assembler = BlockAssembler{chainstate, *static_cast<CTxMemPool*>(&tx_pool), ::Params(), options};
auto assembler = BlockAssembler{chainstate, *static_cast<CTxMemPool*>(&tx_pool), chainstate.m_params, options};
auto block_template = assembler.CreateNewBlock(CScript{} << OP_TRUE);
Assert(block_template->block.vtx.size() >= 1);
}
Expand Down Expand Up @@ -224,13 +224,13 @@ FUZZ_TARGET_INIT(tx_pool_standard, initialize_tx_pool)
// Make sure ProcessNewPackage on one transaction works and always fully validates the transaction.
// The result is not guaranteed to be the same as what is returned by ATMP.
const auto result_package = WITH_LOCK(::cs_main,
return ProcessNewPackage(node.chainman->ActiveChainstate(), tx_pool, {tx}, true));
return ProcessNewPackage(chainstate, tx_pool, {tx}, true));
auto it = result_package.m_tx_results.find(tx->GetWitnessHash());
Assert(it != result_package.m_tx_results.end());
Assert(it->second.m_result_type == MempoolAcceptResult::ResultType::VALID ||
it->second.m_result_type == MempoolAcceptResult::ResultType::INVALID);

const auto res = WITH_LOCK(::cs_main, return AcceptToMemoryPool(chainstate, tx_pool, tx, bypass_limits));
const auto res = WITH_LOCK(::cs_main, return AcceptToMemoryPool(tx_pool, chainstate, tx, GetTime(), bypass_limits, /* test_accept= */ false));
const bool accepted = res.m_result_type == MempoolAcceptResult::ResultType::VALID;
SyncWithValidationInterfaceQueue();
UnregisterSharedValidationInterface(txr);
Expand Down Expand Up @@ -330,7 +330,7 @@ FUZZ_TARGET_INIT(tx_pool, initialize_tx_pool)
const auto tx = MakeTransactionRef(mut_tx);
const bool bypass_limits = fuzzed_data_provider.ConsumeBool();
::fRequireStandard = fuzzed_data_provider.ConsumeBool();
const auto res = WITH_LOCK(::cs_main, return AcceptToMemoryPool(node.chainman->ActiveChainstate(), tx_pool, tx, bypass_limits));
const auto res = WITH_LOCK(::cs_main, return AcceptToMemoryPool(tx_pool, chainstate, tx, GetTime(), bypass_limits, /* test_accept= */ false));
const bool accepted = res.m_result_type == MempoolAcceptResult::ResultType::VALID;
if (accepted) {
txids.push_back(tx->GetHash());
Expand Down
31 changes: 11 additions & 20 deletions src/validation.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -349,8 +349,8 @@ void CChainState::MaybeUpdateMempoolForReorg(
while (it != disconnectpool.queuedTx.get<insertion_order>().rend()) {
// ignore validation errors in resurrected transactions
if (!fAddToMempool || (*it)->IsCoinBase() ||
AcceptToMemoryPool(
*this, *m_mempool, *it, true /* bypass_limits */).m_result_type !=
AcceptToMemoryPool(*m_mempool, *this, *it, GetTime(),
/* bypass_limits= */true, /* test_accept= */ false).m_result_type !=
MempoolAcceptResult::ResultType::VALID) {
// If the transaction doesn't make it in to the mempool, remove any
// transactions that depend on it (which would now be orphans).
Expand Down Expand Up @@ -1078,15 +1078,13 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptMultipleTransactions(const std::

} // anon namespace

/** (try to) add transaction to memory pool with a specified acceptance time **/
static MempoolAcceptResult AcceptToMemoryPoolWithTime(const CChainParams& chainparams, CTxMemPool& pool,
CChainState& active_chainstate,
const CTransactionRef &tx, int64_t nAcceptTime,
bool bypass_limits, bool test_accept)
EXCLUSIVE_LOCKS_REQUIRED(cs_main)
MempoolAcceptResult AcceptToMemoryPool(CTxMemPool& pool, CChainState& active_chainstate, const CTransactionRef& tx,
int64_t accept_time, bool bypass_limits, bool test_accept)
EXCLUSIVE_LOCKS_REQUIRED(cs_main)
{
const CChainParams& chainparams{active_chainstate.m_params};
std::vector<COutPoint> coins_to_uncache;
auto args = MemPoolAccept::ATMPArgs::SingleAccept(chainparams, nAcceptTime, bypass_limits, coins_to_uncache, test_accept);
auto args = MemPoolAccept::ATMPArgs::SingleAccept(chainparams, accept_time, bypass_limits, coins_to_uncache, test_accept);
const MempoolAcceptResult result = MemPoolAccept(pool, active_chainstate).AcceptSingleTransaction(tx, args);
if (result.m_result_type != MempoolAcceptResult::ResultType::VALID) {
// Remove coins that were not present in the coins cache before calling
Expand All @@ -1103,12 +1101,6 @@ static MempoolAcceptResult AcceptToMemoryPoolWithTime(const CChainParams& chainp
return result;
}

MempoolAcceptResult AcceptToMemoryPool(CChainState& active_chainstate, CTxMemPool& pool, const CTransactionRef& tx,
bool bypass_limits, bool test_accept)
{
return AcceptToMemoryPoolWithTime(Params(), pool, active_chainstate, tx, GetTime(), bypass_limits, test_accept);
}

PackageMempoolAcceptResult ProcessNewPackage(CChainState& active_chainstate, CTxMemPool& pool,
const Package& package, bool test_accept)
{
Expand Down Expand Up @@ -1161,8 +1153,8 @@ CChainState::CChainState(
ChainstateManager& chainman,
std::optional<uint256> from_snapshot_blockhash)
: m_mempool(mempool),
m_params(::Params()),
m_blockman(blockman),
m_params(::Params()),
m_chainman(chainman),
m_from_snapshot_blockhash(from_snapshot_blockhash) {}

Expand Down Expand Up @@ -3500,7 +3492,7 @@ MempoolAcceptResult ChainstateManager::ProcessTransaction(const CTransactionRef&
state.Invalid(TxValidationResult::TX_NO_MEMPOOL, "no-mempool");
return MempoolAcceptResult::Failure(state);
}
auto result = AcceptToMemoryPool(active_chainstate, *active_chainstate.m_mempool, tx, /*bypass_limits=*/ false, test_accept);
auto result = AcceptToMemoryPool(*active_chainstate.m_mempool, active_chainstate, tx, GetTime(), /* bypass_limits= */ false, test_accept);
active_chainstate.m_mempool->check(active_chainstate.CoinsTip(), active_chainstate.m_chain.Height() + 1);
return result;
}
Expand Down Expand Up @@ -4530,7 +4522,6 @@ static const uint64_t MEMPOOL_DUMP_VERSION = 1;

bool LoadMempool(CTxMemPool& pool, CChainState& active_chainstate, FopenFn mockable_fopen_function)
{
const CChainParams& chainparams = Params();
int64_t nExpiryTimeout = gArgs.GetIntArg("-mempoolexpiry", DEFAULT_MEMPOOL_EXPIRY) * 60 * 60;
FILE* filestr{mockable_fopen_function(gArgs.GetDataDirNet() / "mempool.dat", "rb")};
CAutoFile file(filestr, SER_DISK, CLIENT_VERSION);
Expand Down Expand Up @@ -4568,8 +4559,8 @@ bool LoadMempool(CTxMemPool& pool, CChainState& active_chainstate, FopenFn mocka
}
if (nTime > nNow - nExpiryTimeout) {
LOCK(cs_main);
if (AcceptToMemoryPoolWithTime(chainparams, pool, active_chainstate, tx, nTime, false /* bypass_limits */,
false /* test_accept */).m_result_type == MempoolAcceptResult::ResultType::VALID) {
if (AcceptToMemoryPool(pool, active_chainstate, tx, nTime, /* bypass_limits= */ false,
/* test_accept= */ false).m_result_type == MempoolAcceptResult::ResultType::VALID) {
++count;
} else {
// mempool may contain the transaction already, e.g. from
Expand Down
19 changes: 12 additions & 7 deletions src/validation.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -208,19 +208,23 @@ struct PackageMempoolAcceptResult
};

/**
* Try to add a transaction to the mempool. This is an internal function and is
* exposed only for testing. Client code should use ChainstateManager::ProcessTransaction()
* Try to add a transaction to the mempool. This is an internal function and is exposed only for testing.
* Client code should use ChainstateManager::ProcessTransaction()
*
* @param[in] active_chainstate Reference to the active chainstate.
* @param[in] pool Reference to the node's mempool.
* @param[in] active_chainstate Reference to the active chainstate.
* @param[in] tx The transaction to submit for mempool acceptance.
* @param[in] accept_time The timestamp for adding the transaction to the mempool. Usually
* the current system time, but may be different.
* It is also used to determine when the entry expires.
* @param[in] bypass_limits When true, don't enforce mempool fee and capacity limits.
* @param[in] test_accept When true, run validation checks but don't submit to mempool.
*
* @returns a MempoolAcceptResult indicating whether the transaction was accepted/rejected with reason.
*/
MempoolAcceptResult AcceptToMemoryPool(CChainState& active_chainstate, CTxMemPool& pool, const CTransactionRef& tx,
bool bypass_limits, bool test_accept=false) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
MempoolAcceptResult AcceptToMemoryPool(CTxMemPool& pool, CChainState& active_chainstate, const CTransactionRef& tx,
int64_t accept_time, bool bypass_limits, bool test_accept)
EXCLUSIVE_LOCKS_REQUIRED(cs_main);

/**
* Atomically test acceptance of a package. If the package only contains one tx, package rules still
Expand Down Expand Up @@ -581,8 +585,6 @@ class CChainState
//! Only the active chainstate has a mempool.
CTxMemPool* m_mempool;

const CChainParams& m_params;

//! Manages the UTXO set, which is a reflection of the contents of `m_chain`.
std::unique_ptr<CoinsViews> m_coins_views;

Expand All @@ -591,6 +593,9 @@ class CChainState
//! CChainState instances.
BlockManager& m_blockman;

/** Chain parameters for this chainstate */
const CChainParams& m_params;

//! The chainstate manager that owns this chainstate. The reference is
//! necessary so that this instance can check whether it is the active
//! chainstate within deeply nested method calls.
Expand Down