Skip to content

Commit f911bdf

Browse files
stickies-vstickies-v
committed
mempool: use util::Result for CalculateMemPoolAncestors
Avoid using setAncestors outparameter, simplify function signatures and avoid creating unused dummy strings.
1 parent 66e028f commit f911bdf

File tree

8 files changed

+64
-80
lines changed

8 files changed

+64
-80
lines changed

‎src/node/interfaces.cpp

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -672,12 +672,9 @@ class ChainImpl : public Chain
672672
if (!m_node.mempool) return true;
673673
LockPoints lp;
674674
CTxMemPoolEntry entry(tx, 0, 0, 0, false, 0, lp);
675-
CTxMemPool::setEntries ancestors;
676675
const CTxMemPool::Limits& limits{m_node.mempool->m_limits};
677-
std::string unused_error_string;
678676
LOCK(m_node.mempool->cs);
679-
return m_node.mempool->CalculateMemPoolAncestors(
680-
entry, ancestors, limits, unused_error_string);
677+
return m_node.mempool->CalculateMemPoolAncestors(entry, limits).has_value();
681678
}
682679
CFeeRate estimateSmartFee(int num_blocks, bool conservative, FeeCalculation* calc) override
683680
{

‎src/node/miner.cpp

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -394,9 +394,8 @@ void BlockAssembler::addPackageTxs(const CTxMemPool& mempool, int& nPackagesSele
394394
continue;
395395
}
396396

397-
CTxMemPool::setEntries ancestors;
398-
std::string dummy;
399-
mempool.CalculateMemPoolAncestors(*iter, ancestors, CTxMemPool::Limits::NoLimits(), dummy, false);
397+
auto ancestors_result{mempool.CalculateMemPoolAncestors(*iter, CTxMemPool::Limits::NoLimits(), /*fSearchForParents=*/false)};
398+
auto ancestors{std::move(ancestors_result).value_or(CTxMemPool::setEntries{})};
400399

401400
onlyUnconfirmed(ancestors);
402401
ancestors.insert(iter);

‎src/policy/rbf.cpp

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -16,14 +16,13 @@
1616
#include <util/rbf.h>
1717

1818
#include <limits>
19+
#include <utility>
1920
#include <vector>
2021

2122
RBFTransactionState IsRBFOptIn(const CTransaction& tx, const CTxMemPool& pool)
2223
{
2324
AssertLockHeld(pool.cs);
2425

25-
CTxMemPool::setEntries ancestors;
26-
2726
// First check the transaction itself.
2827
if (SignalsOptInRBF(tx)) {
2928
return RBFTransactionState::REPLACEABLE_BIP125;
@@ -37,9 +36,9 @@ RBFTransactionState IsRBFOptIn(const CTransaction& tx, const CTxMemPool& pool)
3736

3837
// If all the inputs have nSequence >= maxint-1, it still might be
3938
// signaled for RBF if any unconfirmed parents have signaled.
40-
std::string dummy;
4139
CTxMemPoolEntry entry = *pool.mapTx.find(tx.GetHash());
42-
pool.CalculateMemPoolAncestors(entry, ancestors, CTxMemPool::Limits::NoLimits(), dummy, false);
40+
auto ancestors_result{pool.CalculateMemPoolAncestors(entry, CTxMemPool::Limits::NoLimits(), /*fSearchForParents=*/false)};
41+
auto ancestors{std::move(ancestors_result).value_or(CTxMemPool::setEntries{})};
4342

4443
for (CTxMemPool::txiter it : ancestors) {
4544
if (SignalsOptInRBF(it->GetTx())) {

‎src/rpc/mempool.cpp

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,8 @@
2323
#include <util/moneystr.h>
2424
#include <util/time.h>
2525

26+
#include <utility>
27+
2628
using kernel::DumpMempool;
2729

2830
using node::DEFAULT_MAX_RAW_TX_FEE_RATE;
@@ -449,19 +451,18 @@ static RPCHelpMan getmempoolancestors()
449451
throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Transaction not in mempool");
450452
}
451453

452-
CTxMemPool::setEntries setAncestors;
453-
std::string dummy;
454-
mempool.CalculateMemPoolAncestors(*it, setAncestors, CTxMemPool::Limits::NoLimits(), dummy, false);
454+
auto ancestors_result{mempool.CalculateMemPoolAncestors(*it, CTxMemPool::Limits::NoLimits(), /*fSearchForParents=*/false)};
455+
auto ancestors{std::move(ancestors_result).value_or(CTxMemPool::setEntries{})};
455456

456457
if (!fVerbose) {
457458
UniValue o(UniValue::VARR);
458-
for (CTxMemPool::txiter ancestorIt : setAncestors) {
459+
for (CTxMemPool::txiter ancestorIt : ancestors) {
459460
o.push_back(ancestorIt->GetTx().GetHash().ToString());
460461
}
461462
return o;
462463
} else {
463464
UniValue o(UniValue::VOBJ);
464-
for (CTxMemPool::txiter ancestorIt : setAncestors) {
465+
for (CTxMemPool::txiter ancestorIt : ancestors) {
465466
const CTxMemPoolEntry &e = *ancestorIt;
466467
const uint256& _hash = e.GetTx().GetHash();
467468
UniValue info(UniValue::VOBJ);

‎src/test/mempool_tests.cpp

Lines changed: 6 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -202,10 +202,9 @@ BOOST_AUTO_TEST_CASE(MempoolIndexingTest)
202202
tx7.vout[1].scriptPubKey = CScript() << OP_11 << OP_EQUAL;
203203
tx7.vout[1].nValue = 1 * COIN;
204204

205-
CTxMemPool::setEntries setAncestorsCalculated;
206-
std::string dummy;
207-
BOOST_CHECK_EQUAL(pool.CalculateMemPoolAncestors(entry.Fee(2'000'000LL).FromTx(tx7), setAncestorsCalculated, CTxMemPool::Limits::NoLimits(), dummy), true);
208-
BOOST_CHECK(setAncestorsCalculated == setAncestors);
205+
auto ancestors_calculated{pool.CalculateMemPoolAncestors(entry.Fee(2000000LL).FromTx(tx7), CTxMemPool::Limits::NoLimits())};
206+
BOOST_REQUIRE(ancestors_calculated.has_value());
207+
BOOST_CHECK(*ancestors_calculated == setAncestors);
209208

210209
pool.addUnchecked(entry.FromTx(tx7), setAncestors);
211210
BOOST_CHECK_EQUAL(pool.size(), 7U);
@@ -261,9 +260,9 @@ BOOST_AUTO_TEST_CASE(MempoolIndexingTest)
261260
tx10.vout[0].scriptPubKey = CScript() << OP_11 << OP_EQUAL;
262261
tx10.vout[0].nValue = 10 * COIN;
263262

264-
setAncestorsCalculated.clear();
265-
BOOST_CHECK_EQUAL(pool.CalculateMemPoolAncestors(entry.Fee(200'000LL).Time(NodeSeconds{4s}).FromTx(tx10), setAncestorsCalculated, CTxMemPool::Limits::NoLimits(), dummy), true);
266-
BOOST_CHECK(setAncestorsCalculated == setAncestors);
263+
ancestors_calculated = pool.CalculateMemPoolAncestors(entry.Fee(200000LL).Time(NodeSeconds{4s}).FromTx(tx10), CTxMemPool::Limits::NoLimits());
264+
BOOST_REQUIRE(ancestors_calculated);
265+
BOOST_CHECK(*ancestors_calculated == setAncestors);
267266

268267
pool.addUnchecked(entry.FromTx(tx10), setAncestors);
269268

‎src/txmempool.cpp

Lines changed: 22 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,8 @@
2525

2626
#include <cmath>
2727
#include <optional>
28+
#include <string_view>
29+
#include <utility>
2830

2931
bool TestLockPointValidity(CChain& active_chain, const LockPoints& lp)
3032
{
@@ -220,11 +222,10 @@ bool CTxMemPool::CheckPackageLimits(const Package& package,
220222
return ancestors.has_value();
221223
}
222224

223-
bool CTxMemPool::CalculateMemPoolAncestors(const CTxMemPoolEntry &entry,
224-
setEntries &setAncestors,
225-
const Limits& limits,
226-
std::string &errString,
227-
bool fSearchForParents /* = true */) const
225+
util::Result<CTxMemPool::setEntries> CTxMemPool::CalculateMemPoolAncestors(
226+
const CTxMemPoolEntry &entry,
227+
const Limits& limits,
228+
bool fSearchForParents /* = true */) const
228229
{
229230
CTxMemPoolEntry::Parents staged_ancestors;
230231
const CTransaction &tx = entry.GetTx();
@@ -238,8 +239,7 @@ bool CTxMemPool::CalculateMemPoolAncestors(const CTxMemPoolEntry &entry,
238239
if (piter) {
239240
staged_ancestors.insert(**piter);
240241
if (staged_ancestors.size() + 1 > static_cast<uint64_t>(limits.ancestor_count)) {
241-
errString = strprintf("too many unconfirmed parents [limit: %u]", limits.ancestor_count);
242-
return false;
242+
return util::Error{Untranslated(strprintf("too many unconfirmed parents [limit: %u]", limits.ancestor_count))};
243243
}
244244
}
245245
}
@@ -250,14 +250,8 @@ bool CTxMemPool::CalculateMemPoolAncestors(const CTxMemPoolEntry &entry,
250250
staged_ancestors = it->GetMemPoolParentsConst();
251251
}
252252

253-
const auto calculated_ancestors{CalculateAncestorsAndCheckLimits(entry.GetTxSize(), /*entry_count=*/1,
254-
staged_ancestors, limits)};
255-
if (!calculated_ancestors.has_value()) {
256-
errString = util::ErrorString(calculated_ancestors).original;
257-
return false;
258-
}
259-
setAncestors = *calculated_ancestors;
260-
return true;
253+
return CalculateAncestorsAndCheckLimits(entry.GetTxSize(), /*entry_count=*/1, staged_ancestors,
254+
limits);
261255
}
262256

263257
void CTxMemPool::UpdateAncestorsOf(bool add, txiter it, setEntries &setAncestors)
@@ -321,9 +315,7 @@ void CTxMemPool::UpdateForRemoveFromMempool(const setEntries &entriesToRemove, b
321315
}
322316
}
323317
for (txiter removeIt : entriesToRemove) {
324-
setEntries setAncestors;
325318
const CTxMemPoolEntry &entry = *removeIt;
326-
std::string dummy;
327319
// Since this is a tx that is already in the mempool, we can call CMPA
328320
// with fSearchForParents = false. If the mempool is in a consistent
329321
// state, then using true or false should both be correct, though false
@@ -343,10 +335,11 @@ void CTxMemPool::UpdateForRemoveFromMempool(const setEntries &entriesToRemove, b
343335
// mempool parents we'd calculate by searching, and it's important that
344336
// we use the cached notion of ancestor transactions as the set of
345337
// things to update for removal.
346-
CalculateMemPoolAncestors(entry, setAncestors, Limits::NoLimits(), dummy, false);
338+
auto ancestors_result{CalculateMemPoolAncestors(entry, Limits::NoLimits(), /*fSearchForParents=*/false)};
339+
auto ancestors{std::move(ancestors_result).value_or(setEntries{})};
347340
// Note that UpdateAncestorsOf severs the child links that point to
348341
// removeIt in the entries for the parents of removeIt.
349-
UpdateAncestorsOf(false, removeIt, setAncestors);
342+
UpdateAncestorsOf(false, removeIt, ancestors);
350343
}
351344
// After updating all the ancestor sizes, we can now sever the link between each
352345
// transaction being removed and any mempool children (ie, update CTxMemPoolEntry::m_parents
@@ -696,15 +689,14 @@ void CTxMemPool::check(const CCoinsViewCache& active_coins_tip, int64_t spendhei
696689
assert(setParentCheck.size() == it->GetMemPoolParentsConst().size());
697690
assert(std::equal(setParentCheck.begin(), setParentCheck.end(), it->GetMemPoolParentsConst().begin(), comp));
698691
// Verify ancestor state is correct.
699-
setEntries setAncestors;
700-
std::string dummy;
701-
CalculateMemPoolAncestors(*it, setAncestors, Limits::NoLimits(), dummy);
702-
uint64_t nCountCheck = setAncestors.size() + 1;
692+
auto ancestors_result{CalculateMemPoolAncestors(*it, Limits::NoLimits())};
693+
auto ancestors{std::move(ancestors_result).value_or(setEntries{})};
694+
uint64_t nCountCheck = ancestors.size() + 1;
703695
uint64_t nSizeCheck = it->GetTxSize();
704696
CAmount nFeesCheck = it->GetModifiedFee();
705697
int64_t nSigOpCheck = it->GetSigOpCost();
706698

707-
for (txiter ancestorIt : setAncestors) {
699+
for (txiter ancestorIt : ancestors) {
708700
nSizeCheck += ancestorIt->GetTxSize();
709701
nFeesCheck += ancestorIt->GetModifiedFee();
710702
nSigOpCheck += ancestorIt->GetSigOpCost();
@@ -859,10 +851,9 @@ void CTxMemPool::PrioritiseTransaction(const uint256& hash, const CAmount& nFeeD
859851
if (it != mapTx.end()) {
860852
mapTx.modify(it, [&nFeeDelta](CTxMemPoolEntry& e) { e.UpdateModifiedFee(nFeeDelta); });
861853
// Now update all ancestors' modified fees with descendants
862-
setEntries setAncestors;
863-
std::string dummy;
864-
CalculateMemPoolAncestors(*it, setAncestors, Limits::NoLimits(), dummy, false);
865-
for (txiter ancestorIt : setAncestors) {
854+
auto ancestors_result{CalculateMemPoolAncestors(*it, Limits::NoLimits(), /*fSearchForParents=*/false)};
855+
auto ancestors{std::move(ancestors_result).value_or(setEntries{})};
856+
for (txiter ancestorIt : ancestors) {
866857
mapTx.modify(ancestorIt, [=](CTxMemPoolEntry& e){ e.UpdateDescendantState(0, nFeeDelta, 0);});
867858
}
868859
// Now update all descendants' modified fees with ancestors
@@ -999,10 +990,9 @@ int CTxMemPool::Expire(std::chrono::seconds time)
999990

1000991
void CTxMemPool::addUnchecked(const CTxMemPoolEntry &entry, bool validFeeEstimate)
1001992
{
1002-
setEntries setAncestors;
1003-
std::string dummy;
1004-
CalculateMemPoolAncestors(entry, setAncestors, Limits::NoLimits(), dummy);
1005-
return addUnchecked(entry, setAncestors, validFeeEstimate);
993+
auto ancestors_result{CalculateMemPoolAncestors(entry, Limits::NoLimits())};
994+
auto ancestors{std::move(ancestors_result).value_or(CTxMemPool::setEntries{})};
995+
return addUnchecked(entry, ancestors, validFeeEstimate);
1006996
}
1007997

1008998
void CTxMemPool::UpdateChild(txiter entry, txiter child, bool add)

‎src/txmempool.h

Lines changed: 2 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -555,20 +555,15 @@ class CTxMemPool
555555
* (these are all calculated including the tx itself)
556556
*
557557
* @param[in] entry CTxMemPoolEntry of which all in-mempool ancestors are calculated
558-
* @param[out] setAncestors Will be populated with all mempool ancestors.
559558
* @param[in] limits Maximum number and size of ancestors and descendants
560-
* @param[out] errString Populated with error reason if any limits are hit
561559
* @param[in] fSearchForParents Whether to search a tx's vin for in-mempool parents, or look
562560
* up parents from mapLinks. Must be true for entries not in
563561
* the mempool
564562
*
565-
* @return true if no limits were hit and all in-mempool ancestors were calculated, false
566-
* otherwise
563+
* @return all in-mempool ancestors, or an error if any ancestor or descendant limits were hit
567564
*/
568-
bool CalculateMemPoolAncestors(const CTxMemPoolEntry& entry,
569-
setEntries& setAncestors,
565+
util::Result<setEntries> CalculateMemPoolAncestors(const CTxMemPoolEntry& entry,
570566
const Limits& limits,
571-
std::string& errString,
572567
bool fSearchForParents = true) const EXCLUSIVE_LOCKS_REQUIRED(cs);
573568

574569
/** Calculate all in-mempool ancestors of a set of transactions not already in the mempool and

‎src/validation.cpp

Lines changed: 22 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -64,6 +64,7 @@
6464
#include <numeric>
6565
#include <optional>
6666
#include <string>
67+
#include <utility>
6768

6869
using kernel::CCoinsStats;
6970
using kernel::CoinStatsHashType;
@@ -870,11 +871,9 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws)
870871
m_limits.descendant_size_vbytes += conflict->GetSizeWithDescendants();
871872
}
872873

873-
std::string errString;
874-
if (!m_pool.CalculateMemPoolAncestors(*entry, ws.m_ancestors, m_limits, errString)) {
875-
ws.m_ancestors.clear();
874+
auto ancestors{m_pool.CalculateMemPoolAncestors(*entry, m_limits)};
875+
if (!ancestors) {
876876
// If CalculateMemPoolAncestors fails second time, we want the original error string.
877-
std::string dummy_err_string;
878877
// Contracting/payment channels CPFP carve-out:
879878
// If the new transaction is relatively small (up to 40k weight)
880879
// and has at most one ancestor (ie ancestor limit of 2, including
@@ -892,14 +891,16 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws)
892891
.descendant_count = m_limits.descendant_count + 1,
893892
.descendant_size_vbytes = m_limits.descendant_size_vbytes + EXTRA_DESCENDANT_TX_SIZE_LIMIT,
894893
};
895-
if (ws.m_vsize > EXTRA_DESCENDANT_TX_SIZE_LIMIT ||
896-
!m_pool.CalculateMemPoolAncestors(*entry, ws.m_ancestors,
897-
cpfp_carve_out_limits,
898-
dummy_err_string)) {
899-
return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "too-long-mempool-chain", errString);
894+
const auto error_message{util::ErrorString(ancestors).original};
895+
if (ws.m_vsize > EXTRA_DESCENDANT_TX_SIZE_LIMIT) {
896+
return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "too-long-mempool-chain", error_message);
900897
}
898+
ancestors = m_pool.CalculateMemPoolAncestors(*entry, cpfp_carve_out_limits);
899+
if (!ancestors) return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "too-long-mempool-chain", error_message);
901900
}
902901

902+
ws.m_ancestors = *ancestors;
903+
903904
// A transaction that spends outputs that would be replaced by it is invalid. Now
904905
// that we have the set of all ancestors we can detect this
905906
// pathological case by making sure ws.m_conflicts and ws.m_ancestors don't
@@ -1114,15 +1115,18 @@ bool MemPoolAccept::SubmitPackage(const ATMPArgs& args, std::vector<Workspace>&
11141115

11151116
// Re-calculate mempool ancestors to call addUnchecked(). They may have changed since the
11161117
// last calculation done in PreChecks, since package ancestors have already been submitted.
1117-
std::string unused_err_string;
1118-
if(!m_pool.CalculateMemPoolAncestors(*ws.m_entry, ws.m_ancestors, m_limits, unused_err_string)) {
1119-
results.emplace(ws.m_ptx->GetWitnessHash(), MempoolAcceptResult::Failure(ws.m_state));
1120-
// Since PreChecks() and PackageMempoolChecks() both enforce limits, this should never fail.
1121-
Assume(false);
1122-
all_submitted = false;
1123-
package_state.Invalid(PackageValidationResult::PCKG_MEMPOOL_ERROR,
1124-
strprintf("BUG! Mempool ancestors or descendants were underestimated: %s",
1125-
ws.m_ptx->GetHash().ToString()));
1118+
{
1119+
auto ancestors{m_pool.CalculateMemPoolAncestors(*ws.m_entry, m_limits)};
1120+
if(!ancestors) {
1121+
results.emplace(ws.m_ptx->GetWitnessHash(), MempoolAcceptResult::Failure(ws.m_state));
1122+
// Since PreChecks() and PackageMempoolChecks() both enforce limits, this should never fail.
1123+
Assume(false);
1124+
all_submitted = false;
1125+
package_state.Invalid(PackageValidationResult::PCKG_MEMPOOL_ERROR,
1126+
strprintf("BUG! Mempool ancestors or descendants were underestimated: %s",
1127+
ws.m_ptx->GetHash().ToString()));
1128+
}
1129+
ws.m_ancestors = std::move(ancestors).value_or(ws.m_ancestors);
11261130
}
11271131
// If we call LimitMempoolSize() for each individual Finalize(), the mempool will not take
11281132
// the transaction's descendant feerate into account because it hasn't seen them yet. Also,

0 commit comments

Comments
 (0)