foster/bitcoin-bitcoin-core
0
0
Fork 0
mirror of https://github.com/bitcoin/bitcoin.git synced 2025-02-08 10:31:50 -05:00
Code Issues Activity

refactor: mempool: use CTxMempool::Limits

Browse source 
Simplifies function signatures by removing repetition of all the
ancestor/descendant limits,  and increases readability by being
more verbose by naming the limits, while still reducing the LoC.
This commit is contained in:
stickies-v 2022-09-15 18:11:32 +01:00
parent b85af25f87
commit 3a86f24a4c
No known key found for this signature in database
GPG key ID: 5CB1CE6E5E66A757
7 changed files with 59 additions and 85 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

3
src/node/interfaces.cpp
View file

@ -660,8 +660,7 @@ public:
std::string unused_error_string;
LOCK(m_node.mempool->cs);
return m_node.mempool->CalculateMemPoolAncestors(
entry, ancestors, limits.ancestor_count, limits.ancestor_size_vbytes,
limits.descendant_count, limits.descendant_size_vbytes, unused_error_string);
entry, ancestors, limits, unused_error_string);
}
CFeeRate estimateSmartFee(int num_blocks, bool conservative, FeeCalculation* calc) override
{

3
src/node/miner.cpp
View file

@ -396,9 +396,8 @@ void BlockAssembler::addPackageTxs(const CTxMemPool& mempool, int& nPackagesSele
}
CTxMemPool::setEntries ancestors;
uint64_t nNoLimit = std::numeric_limits<uint64_t>::max();
std::string dummy;
mempool.CalculateMemPoolAncestors(*iter, ancestors, nNoLimit, nNoLimit, nNoLimit, nNoLimit, dummy, false);
mempool.CalculateMemPoolAncestors(*iter, ancestors, CTxMemPool::Limits::NoLimits(), dummy, false);
onlyUnconfirmed(ancestors);
ancestors.insert(iter);

3
src/policy/rbf.cpp
View file

@ -36,10 +36,9 @@ RBFTransactionState IsRBFOptIn(const CTransaction& tx, const CTxMemPool& pool)
// If all the inputs have nSequence >= maxint-1, it still might be
// signaled for RBF if any unconfirmed parents have signaled.
uint64_t noLimit = std::numeric_limits<uint64_t>::max();
std::string dummy;
CTxMemPoolEntry entry = *pool.mapTx.find(tx.GetHash());
pool.CalculateMemPoolAncestors(entry, ancestors, noLimit, noLimit, noLimit, noLimit, dummy, false);
pool.CalculateMemPoolAncestors(entry, ancestors, CTxMemPool::Limits::NoLimits(), dummy, false);
for (CTxMemPool::txiter it : ancestors) {
if (SignalsOptInRBF(it->GetTx())) {

3
src/rpc/mempool.cpp
View file

@ -449,9 +449,8 @@ static RPCHelpMan getmempoolancestors()
}
CTxMemPool::setEntries setAncestors;
uint64_t noLimit = std::numeric_limits<uint64_t>::max();
std::string dummy;
mempool.CalculateMemPoolAncestors(*it, setAncestors, noLimit, noLimit, noLimit, noLimit, dummy, false);
mempool.CalculateMemPoolAncestors(*it, setAncestors, CTxMemPool::Limits::NoLimits(), dummy, false);
if (!fVerbose) {
UniValue o(UniValue::VARR);

57
src/txmempool.cpp
View file

@ -187,10 +187,7 @@ bool CTxMemPool::CalculateAncestorsAndCheckLimits(size_t entry_size,
size_t entry_count,
setEntries& setAncestors,
CTxMemPoolEntry::Parents& staged_ancestors,
uint64_t limitAncestorCount,
uint64_t limitAncestorSize,
uint64_t limitDescendantCount,
uint64_t limitDescendantSize,
const Limits& limits,
std::string &errString) const
{
size_t totalSizeWithAncestors = entry_size;
@ -203,14 +200,14 @@ bool CTxMemPool::CalculateAncestorsAndCheckLimits(size_t entry_size,
staged_ancestors.erase(stage);
totalSizeWithAncestors += stageit->GetTxSize();
if (stageit->GetSizeWithDescendants() + entry_size > limitDescendantSize) {
errString = strprintf("exceeds descendant size limit for tx %s [limit: %u]", stageit->GetTx().GetHash().ToString(), limitDescendantSize);
if (stageit->GetSizeWithDescendants() + entry_size > static_cast<uint64_t>(limits.descendant_size_vbytes)) {
errString = strprintf("exceeds descendant size limit for tx %s [limit: %u]", stageit->GetTx().GetHash().ToString(), limits.descendant_size_vbytes);
return false;
} else if (stageit->GetCountWithDescendants() + entry_count > limitDescendantCount) {
errString = strprintf("too many descendants for tx %s [limit: %u]", stageit->GetTx().GetHash().ToString(), limitDescendantCount);
} else if (stageit->GetCountWithDescendants() + entry_count > static_cast<uint64_t>(limits.descendant_count)) {
errString = strprintf("too many descendants for tx %s [limit: %u]", stageit->GetTx().GetHash().ToString(), limits.descendant_count);
return false;
} else if (totalSizeWithAncestors > limitAncestorSize) {
errString = strprintf("exceeds ancestor size limit [limit: %u]", limitAncestorSize);
} else if (totalSizeWithAncestors > static_cast<uint64_t>(limits.ancestor_size_vbytes)) {
errString = strprintf("exceeds ancestor size limit [limit: %u]", limits.ancestor_size_vbytes);
return false;
}
@ -222,8 +219,8 @@ bool CTxMemPool::CalculateAncestorsAndCheckLimits(size_t entry_size,
if (setAncestors.count(parent_it) == 0) {
staged_ancestors.insert(parent);
}
if (staged_ancestors.size() + setAncestors.size() + entry_count > limitAncestorCount) {
errString = strprintf("too many unconfirmed ancestors [limit: %u]", limitAncestorCount);
if (staged_ancestors.size() + setAncestors.size() + entry_count > static_cast<uint64_t>(limits.ancestor_count)) {
errString = strprintf("too many unconfirmed ancestors [limit: %u]", limits.ancestor_count);
return false;
}
}
@ -233,10 +230,7 @@ bool CTxMemPool::CalculateAncestorsAndCheckLimits(size_t entry_size,
}
bool CTxMemPool::CheckPackageLimits(const Package& package,
uint64_t limitAncestorCount,
uint64_t limitAncestorSize,
uint64_t limitDescendantCount,
uint64_t limitDescendantSize,
const Limits& limits,
std::string &errString) const
{
CTxMemPoolEntry::Parents staged_ancestors;
@ -247,8 +241,8 @@ bool CTxMemPool::CheckPackageLimits(const Package& package,
std::optional<txiter> piter = GetIter(input.prevout.hash);
if (piter) {
staged_ancestors.insert(**piter);
if (staged_ancestors.size() + package.size() > limitAncestorCount) {
errString = strprintf("too many unconfirmed parents [limit: %u]", limitAncestorCount);
if (staged_ancestors.size() + package.size() > static_cast<uint64_t>(limits.ancestor_count)) {
errString = strprintf("too many unconfirmed parents [limit: %u]", limits.ancestor_count);
return false;
}
}
@ -260,8 +254,7 @@ bool CTxMemPool::CheckPackageLimits(const Package& package,
setEntries setAncestors;
const auto ret = CalculateAncestorsAndCheckLimits(total_size, package.size(),
setAncestors, staged_ancestors,
limitAncestorCount, limitAncestorSize,
limitDescendantCount, limitDescendantSize, errString);
limits, errString);
// It's possible to overestimate the ancestor/descendant totals.
if (!ret) errString.insert(0, "possibly ");
return ret;
@ -269,10 +262,7 @@ bool CTxMemPool::CheckPackageLimits(const Package& package,
bool CTxMemPool::CalculateMemPoolAncestors(const CTxMemPoolEntry &entry,
setEntries &setAncestors,
uint64_t limitAncestorCount,
uint64_t limitAncestorSize,
uint64_t limitDescendantCount,
uint64_t limitDescendantSize,
const Limits& limits,
std::string &errString,
bool fSearchForParents /* = true */) const
{
@ -287,8 +277,8 @@ bool CTxMemPool::CalculateMemPoolAncestors(const CTxMemPoolEntry &entry,
std::optional<txiter> piter = GetIter(tx.vin[i].prevout.hash);
if (piter) {
staged_ancestors.insert(**piter);
if (staged_ancestors.size() + 1 > limitAncestorCount) {
errString = strprintf("too many unconfirmed parents [limit: %u]", limitAncestorCount);
if (staged_ancestors.size() + 1 > static_cast<uint64_t>(limits.ancestor_count)) {
errString = strprintf("too many unconfirmed parents [limit: %u]", limits.ancestor_count);
return false;
}
}
@ -302,8 +292,7 @@ bool CTxMemPool::CalculateMemPoolAncestors(const CTxMemPoolEntry &entry,
return CalculateAncestorsAndCheckLimits(entry.GetTxSize(), /*entry_count=*/1,
setAncestors, staged_ancestors,
limitAncestorCount, limitAncestorSize,
limitDescendantCount, limitDescendantSize, errString);
limits, errString);
}
void CTxMemPool::UpdateAncestorsOf(bool add, txiter it, setEntries &setAncestors)
@ -347,7 +336,6 @@ void CTxMemPool::UpdateForRemoveFromMempool(const setEntries &entriesToRemove, b
{
// For each entry, walk back all ancestors and decrement size associated with this
// transaction
const uint64_t nNoLimit = std::numeric_limits<uint64_t>::max();
if (updateDescendants) {
// updateDescendants should be true whenever we're not recursively
// removing a tx and all its descendants, eg when a transaction is
@ -390,7 +378,7 @@ void CTxMemPool::UpdateForRemoveFromMempool(const setEntries &entriesToRemove, b
// mempool parents we'd calculate by searching, and it's important that
// we use the cached notion of ancestor transactions as the set of
// things to update for removal.
CalculateMemPoolAncestors(entry, setAncestors, nNoLimit, nNoLimit, nNoLimit, nNoLimit, dummy, false);
CalculateMemPoolAncestors(entry, setAncestors, Limits::NoLimits(), dummy, false);
// Note that UpdateAncestorsOf severs the child links that point to
// removeIt in the entries for the parents of removeIt.
UpdateAncestorsOf(false, removeIt, setAncestors);
@ -744,9 +732,8 @@ void CTxMemPool::check(const CCoinsViewCache& active_coins_tip, int64_t spendhei
assert(std::equal(setParentCheck.begin(), setParentCheck.end(), it->GetMemPoolParentsConst().begin(), comp));
// Verify ancestor state is correct.
setEntries setAncestors;
uint64_t nNoLimit = std::numeric_limits<uint64_t>::max();
std::string dummy;
CalculateMemPoolAncestors(*it, setAncestors, nNoLimit, nNoLimit, nNoLimit, nNoLimit, dummy);
CalculateMemPoolAncestors(*it, setAncestors, Limits::NoLimits(), dummy);
uint64_t nCountCheck = setAncestors.size() + 1;
uint64_t nSizeCheck = it->GetTxSize();
CAmount nFeesCheck = it->GetModifiedFee();
@ -908,9 +895,8 @@ void CTxMemPool::PrioritiseTransaction(const uint256& hash, const CAmount& nFeeD
mapTx.modify(it, [&nFeeDelta](CTxMemPoolEntry& e) { e.UpdateModifiedFee(nFeeDelta); });
// Now update all ancestors' modified fees with descendants
setEntries setAncestors;
uint64_t nNoLimit = std::numeric_limits<uint64_t>::max();
std::string dummy;
CalculateMemPoolAncestors(*it, setAncestors, nNoLimit, nNoLimit, nNoLimit, nNoLimit, dummy, false);
CalculateMemPoolAncestors(*it, setAncestors, Limits::NoLimits(), dummy, false);
for (txiter ancestorIt : setAncestors) {
mapTx.modify(ancestorIt, [=](CTxMemPoolEntry& e){ e.UpdateDescendantState(0, nFeeDelta, 0);});
}
@ -1049,9 +1035,8 @@ int CTxMemPool::Expire(std::chrono::seconds time)
void CTxMemPool::addUnchecked(const CTxMemPoolEntry &entry, bool validFeeEstimate)
{
setEntries setAncestors;
uint64_t nNoLimit = std::numeric_limits<uint64_t>::max();
std::string dummy;
CalculateMemPoolAncestors(entry, setAncestors, nNoLimit, nNoLimit, nNoLimit, nNoLimit, dummy);
CalculateMemPoolAncestors(entry, setAncestors, Limits::NoLimits(), dummy);
return addUnchecked(entry, setAncestors, validFeeEstimate);
}

32
src/txmempool.h
View file

@ -526,6 +526,8 @@ public:
typedef std::set<txiter, CompareIteratorByHash> setEntries;
using Limits = kernel::MemPoolLimits;
uint64_t CalculateDescendantMaximum(txiter entry) const EXCLUSIVE_LOCKS_REQUIRED(cs);
private:
typedef std::map<txiter, setEntries, CompareIteratorByHash> cacheMap;
@ -554,10 +556,7 @@ private:
size_t entry_count,
setEntries& setAncestors,
CTxMemPoolEntry::Parents &staged_ancestors,
uint64_t limitAncestorCount,
uint64_t limitAncestorSize,
uint64_t limitDescendantCount,
uint64_t limitDescendantSize,
const Limits& limits,
std::string &errString) const EXCLUSIVE_LOCKS_REQUIRED(cs);
public:
@ -576,8 +575,6 @@ public:
const bool m_require_standard;
const bool m_full_rbf;
using Limits = kernel::MemPoolLimits;
const Limits m_limits;
/** Create a new CTxMemPool.
@ -670,36 +667,31 @@ public:
/** Try to calculate all in-mempool ancestors of entry.
* (these are all calculated including the tx itself)
* limitAncestorCount = max number of ancestors
* limitAncestorSize = max size of ancestors
* limitDescendantCount = max number of descendants any ancestor can have
* limitDescendantSize = max size of descendants any ancestor can have
* limits = maximum number and size of ancestors and descendants
* errString = populated with error reason if any limits are hit
* fSearchForParents = whether to search a tx's vin for in-mempool parents, or
* look up parents from mapLinks. Must be true for entries not in the mempool
*/
bool CalculateMemPoolAncestors(const CTxMemPoolEntry& entry, setEntries& setAncestors, uint64_t limitAncestorCount, uint64_t limitAncestorSize, uint64_t limitDescendantCount, uint64_t limitDescendantSize, std::string& errString, bool fSearchForParents = true) const EXCLUSIVE_LOCKS_REQUIRED(cs);
bool CalculateMemPoolAncestors(const CTxMemPoolEntry& entry,
setEntries& setAncestors,
const Limits& limits,
std::string& errString,
bool fSearchForParents = true) const EXCLUSIVE_LOCKS_REQUIRED(cs);
/** Calculate all in-mempool ancestors of a set of transactions not already in the mempool and
* check ancestor and descendant limits. Heuristics are used to estimate the ancestor and
* descendant count of all entries if the package were to be added to the mempool. The limits
* are applied to the union of all package transactions. For example, if the package has 3
* transactions and limitAncestorCount = 25, the union of all 3 sets of ancestors (including the
* transactions and limits.ancestor_count = 25, the union of all 3 sets of ancestors (including the
* transactions themselves) must be <= 22.
* @param[in] package Transaction package being evaluated for acceptance
* to mempool. The transactions need not be direct
* ancestors/descendants of each other.
* @param[in] limitAncestorCount Max number of txns including ancestors.
* @param[in] limitAncestorSize Max virtual size including ancestors.
* @param[in] limitDescendantCount Max number of txns including descendants.
* @param[in] limitDescendantSize Max virtual size including descendants.
* @param[in] limits Maximum number and size of ancestors and descendants
* @param[out] errString Populated with error reason if a limit is hit.
*/
bool CheckPackageLimits(const Package& package,
uint64_t limitAncestorCount,
uint64_t limitAncestorSize,
uint64_t limitDescendantCount,
uint64_t limitDescendantSize,
const Limits& limits,
std::string &errString) const EXCLUSIVE_LOCKS_REQUIRED(cs);
/** Populate setDescendants with all in-mempool descendants of hash.

43
src/validation.cpp
View file

@ -424,11 +424,13 @@ namespace {
class MemPoolAccept
{
public:
explicit MemPoolAccept(CTxMemPool& mempool, Chainstate& active_chainstate) : m_pool(mempool), m_view(&m_dummy), m_viewmempool(&active_chainstate.CoinsTip(), m_pool), m_active_chainstate(active_chainstate),
m_limit_ancestors(m_pool.m_limits.ancestor_count),
m_limit_ancestor_size(m_pool.m_limits.ancestor_size_vbytes),
m_limit_descendants(m_pool.m_limits.descendant_count),
m_limit_descendant_size(m_pool.m_limits.descendant_size_vbytes) {
explicit MemPoolAccept(CTxMemPool& mempool, Chainstate& active_chainstate) :
m_pool(mempool),
m_view(&m_dummy),
m_viewmempool(&active_chainstate.CoinsTip(), m_pool),
m_active_chainstate(active_chainstate),
m_limits{m_pool.m_limits}
{
}
// We put the arguments we're handed into a struct, so we can pass them
@ -660,13 +662,7 @@ private:
Chainstate& m_active_chainstate;
// The package limits in effect at the time of invocation.
const size_t m_limit_ancestors;
const size_t m_limit_ancestor_size;
// These may be modified while evaluating a transaction (eg to account for
// in-mempool conflicts; see below).
size_t m_limit_descendants;
size_t m_limit_descendant_size;
CTxMemPool::Limits m_limits;
/** Whether the transaction(s) would replace any mempool transactions. If so, RBF rules apply. */
bool m_rbf{false};
@ -879,12 +875,12 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws)
assert(ws.m_iters_conflicting.size() == 1);
CTxMemPool::txiter conflict = *ws.m_iters_conflicting.begin();
m_limit_descendants += 1;
m_limit_descendant_size += conflict->GetSizeWithDescendants();
m_limits.descendant_count += 1;
m_limits.descendant_size_vbytes += conflict->GetSizeWithDescendants();
}
std::string errString;
if (!m_pool.CalculateMemPoolAncestors(*entry, ws.m_ancestors, m_limit_ancestors, m_limit_ancestor_size, m_limit_descendants, m_limit_descendant_size, errString)) {
if (!m_pool.CalculateMemPoolAncestors(*entry, ws.m_ancestors, m_limits, errString)) {
ws.m_ancestors.clear();
// If CalculateMemPoolAncestors fails second time, we want the original error string.
std::string dummy_err_string;
@ -899,8 +895,16 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws)
// to be secure by simply only having two immediately-spendable
// outputs - one for each counterparty. For more info on the uses for
// this, see https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2018-November/016518.html
CTxMemPool::Limits cpfp_carve_out_limits{
.ancestor_count = 2,
.ancestor_size_vbytes = m_limits.ancestor_size_vbytes,
.descendant_count = m_limits.descendant_count + 1,
.descendant_size_vbytes = m_limits.descendant_size_vbytes + EXTRA_DESCENDANT_TX_SIZE_LIMIT,
};
if (ws.m_vsize > EXTRA_DESCENDANT_TX_SIZE_LIMIT ||
!m_pool.CalculateMemPoolAncestors(*entry, ws.m_ancestors, 2, m_limit_ancestor_size, m_limit_descendants + 1, m_limit_descendant_size + EXTRA_DESCENDANT_TX_SIZE_LIMIT, dummy_err_string)) {
!m_pool.CalculateMemPoolAncestors(*entry, ws.m_ancestors,
cpfp_carve_out_limits,
dummy_err_string)) {
return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "too-long-mempool-chain", errString);
}
}
@ -976,8 +980,7 @@ bool MemPoolAccept::PackageMempoolChecks(const std::vector<CTransactionRef>& txn
{ return !m_pool.exists(GenTxid::Txid(tx->GetHash()));}));
std::string err_string;
if (!m_pool.CheckPackageLimits(txns, m_limit_ancestors, m_limit_ancestor_size, m_limit_descendants,
m_limit_descendant_size, err_string)) {
if (!m_pool.CheckPackageLimits(txns, m_limits, err_string)) {
// This is a package-wide error, separate from an individual transaction error.
return package_state.Invalid(PackageValidationResult::PCKG_POLICY, "package-mempool-limits", err_string);
}
@ -1121,9 +1124,7 @@ bool MemPoolAccept::SubmitPackage(const ATMPArgs& args, std::vector<Workspace>&
// Re-calculate mempool ancestors to call addUnchecked(). They may have changed since the
// last calculation done in PreChecks, since package ancestors have already been submitted.
std::string unused_err_string;
if(!m_pool.CalculateMemPoolAncestors(*ws.m_entry, ws.m_ancestors, m_limit_ancestors,
m_limit_ancestor_size, m_limit_descendants,
m_limit_descendant_size, unused_err_string)) {
if(!m_pool.CalculateMemPoolAncestors(*ws.m_entry, ws.m_ancestors, m_limits, unused_err_string)) {
results.emplace(ws.m_ptx->GetWitnessHash(), MempoolAcceptResult::Failure(ws.m_state));
// Since PreChecks() and PackageMempoolChecks() both enforce limits, this should never fail.
Assume(false);