Skip to content

Commit 43ae5ee

Browse files
laanwjlaanwj
committed
Merge #12634: [refactor] Make TransactionWithinChainLimit more flexible
f77e1d3 test: Add MempoolAncestryTests (Karl-Johan Alm) a08d76b mempool: Calculate descendant maximum thoroughly (Karl-Johan Alm) 6d35683 wallet: Switch to using ancestor/descendant limits (Karl-Johan Alm) 6888195 wallet: Strictly greater than for ancestor caps (Karl-Johan Alm) 322b12a Remove deprecated TransactionWithinChainLimit (Karl-Johan Alm) 4784751 Switch to GetTransactionAncestry() in OutputEligibleForSpending (Karl-Johan Alm) 475a385 Add GetTransactionAncestry to CTxMemPool for general purpose chain limit checking (Karl-Johan Alm) 46847d6 mempool: Fix max descendants check (Karl-Johan Alm) b9ef21d mempool: Add explicit max_descendants (Karl-Johan Alm) Pull request description: Currently, `TransactionWithinChainLimit` is restricted to single-output use, and needs to be called every time for different limits. If it is replaced with a chain limit value calculator, that can be called once and reused, and is generally more flexible (see e.g. #12257). Update: this PR now corrects usage of max ancestors / max descendants, including calculating the correct max descendant value, as advertised for the two limits. ~~This change also makes `nMaxAncestors` signed, as the replacement method will return `-1` for "not in the mempool", which is different from "0", which means "no ancestors/descendants in mempool".~~ ~~This is a subset of #12257.~~ Tree-SHA512: aa59c849360542362b3126c0e29d44d3d58f11898e277d38c034dc4b86a5b4500f77ac61767599ce878c876b5c446fec9c02699797eb2fa41e530ec863a00cf9
2 parents 3f0f394 + f77e1d3 commit 43ae5ee

File tree

5 files changed

+224
-11
lines changed

5 files changed

+224
-11
lines changed

‎src/test/mempool_tests.cpp

Lines changed: 178 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -571,4 +571,182 @@ BOOST_AUTO_TEST_CASE(MempoolSizeLimitTest)
571571
SetMockTime(0);
572572
}
573573

574+
inline CTransactionRef make_tx(std::vector<CAmount>&& output_values, std::vector<CTransactionRef>&& inputs=std::vector<CTransactionRef>(), std::vector<uint32_t>&& input_indices=std::vector<uint32_t>())
575+
{
576+
CMutableTransaction tx = CMutableTransaction();
577+
tx.vin.resize(inputs.size());
578+
tx.vout.resize(output_values.size());
579+
for (size_t i = 0; i < inputs.size(); ++i) {
580+
tx.vin[i].prevout.hash = inputs[i]->GetHash();
581+
tx.vin[i].prevout.n = input_indices.size() > i ? input_indices[i] : 0;
582+
}
583+
for (size_t i = 0; i < output_values.size(); ++i) {
584+
tx.vout[i].scriptPubKey = CScript() << OP_11 << OP_EQUAL;
585+
tx.vout[i].nValue = output_values[i];
586+
}
587+
return MakeTransactionRef(tx);
588+
}
589+
590+
#define MK_OUTPUTS(amounts...) std::vector<CAmount>{amounts}
591+
#define MK_INPUTS(txs...) std::vector<CTransactionRef>{txs}
592+
#define MK_INPUT_IDX(idxes...) std::vector<uint32_t>{idxes}
593+
594+
BOOST_AUTO_TEST_CASE(MempoolAncestryTests)
595+
{
596+
size_t ancestors, descendants;
597+
598+
CTxMemPool pool;
599+
TestMemPoolEntryHelper entry;
600+
601+
/* Base transaction */
602+
//
603+
// [tx1]
604+
//
605+
CTransactionRef tx1 = make_tx(MK_OUTPUTS(10 * COIN));
606+
pool.addUnchecked(tx1->GetHash(), entry.Fee(10000LL).FromTx(tx1));
607+
608+
// Ancestors / descendants should be 1 / 1 (itself / itself)
609+
pool.GetTransactionAncestry(tx1->GetHash(), ancestors, descendants);
610+
BOOST_CHECK_EQUAL(ancestors, 1ULL);
611+
BOOST_CHECK_EQUAL(descendants, 1ULL);
612+
613+
/* Child transaction */
614+
//
615+
// [tx1].0 <- [tx2]
616+
//
617+
CTransactionRef tx2 = make_tx(MK_OUTPUTS(495 * CENT, 5 * COIN), MK_INPUTS(tx1));
618+
pool.addUnchecked(tx2->GetHash(), entry.Fee(10000LL).FromTx(tx2));
619+
620+
// Ancestors / descendants should be:
621+
// transaction ancestors descendants
622+
// ============ =========== ===========
623+
// tx1 1 (tx1) 2 (tx1,2)
624+
// tx2 2 (tx1,2) 2 (tx1,2)
625+
pool.GetTransactionAncestry(tx1->GetHash(), ancestors, descendants);
626+
BOOST_CHECK_EQUAL(ancestors, 1ULL);
627+
BOOST_CHECK_EQUAL(descendants, 2ULL);
628+
pool.GetTransactionAncestry(tx2->GetHash(), ancestors, descendants);
629+
BOOST_CHECK_EQUAL(ancestors, 2ULL);
630+
BOOST_CHECK_EQUAL(descendants, 2ULL);
631+
632+
/* Grand-child 1 */
633+
//
634+
// [tx1].0 <- [tx2].0 <- [tx3]
635+
//
636+
CTransactionRef tx3 = make_tx(MK_OUTPUTS(290 * CENT, 200 * CENT), MK_INPUTS(tx2));
637+
pool.addUnchecked(tx3->GetHash(), entry.Fee(10000LL).FromTx(tx3));
638+
639+
// Ancestors / descendants should be:
640+
// transaction ancestors descendants
641+
// ============ =========== ===========
642+
// tx1 1 (tx1) 3 (tx1,2,3)
643+
// tx2 2 (tx1,2) 3 (tx1,2,3)
644+
// tx3 3 (tx1,2,3) 3 (tx1,2,3)
645+
pool.GetTransactionAncestry(tx1->GetHash(), ancestors, descendants);
646+
BOOST_CHECK_EQUAL(ancestors, 1ULL);
647+
BOOST_CHECK_EQUAL(descendants, 3ULL);
648+
pool.GetTransactionAncestry(tx2->GetHash(), ancestors, descendants);
649+
BOOST_CHECK_EQUAL(ancestors, 2ULL);
650+
BOOST_CHECK_EQUAL(descendants, 3ULL);
651+
pool.GetTransactionAncestry(tx3->GetHash(), ancestors, descendants);
652+
BOOST_CHECK_EQUAL(ancestors, 3ULL);
653+
BOOST_CHECK_EQUAL(descendants, 3ULL);
654+
655+
/* Grand-child 2 */
656+
//
657+
// [tx1].0 <- [tx2].0 <- [tx3]
658+
// |
659+
// \---1 <- [tx4]
660+
//
661+
CTransactionRef tx4 = make_tx(MK_OUTPUTS(290 * CENT, 250 * CENT), MK_INPUTS(tx2), MK_INPUT_IDX(1));
662+
pool.addUnchecked(tx4->GetHash(), entry.Fee(10000LL).FromTx(tx4));
663+
664+
// Ancestors / descendants should be:
665+
// transaction ancestors descendants
666+
// ============ =========== ===========
667+
// tx1 1 (tx1) 4 (tx1,2,3,4)
668+
// tx2 2 (tx1,2) 4 (tx1,2,3,4)
669+
// tx3 3 (tx1,2,3) 4 (tx1,2,3,4)
670+
// tx4 3 (tx1,2,4) 4 (tx1,2,3,4)
671+
pool.GetTransactionAncestry(tx1->GetHash(), ancestors, descendants);
672+
BOOST_CHECK_EQUAL(ancestors, 1ULL);
673+
BOOST_CHECK_EQUAL(descendants, 4ULL);
674+
pool.GetTransactionAncestry(tx2->GetHash(), ancestors, descendants);
675+
BOOST_CHECK_EQUAL(ancestors, 2ULL);
676+
BOOST_CHECK_EQUAL(descendants, 4ULL);
677+
pool.GetTransactionAncestry(tx3->GetHash(), ancestors, descendants);
678+
BOOST_CHECK_EQUAL(ancestors, 3ULL);
679+
BOOST_CHECK_EQUAL(descendants, 4ULL);
680+
pool.GetTransactionAncestry(tx4->GetHash(), ancestors, descendants);
681+
BOOST_CHECK_EQUAL(ancestors, 3ULL);
682+
BOOST_CHECK_EQUAL(descendants, 4ULL);
683+
684+
/* Make an alternate branch that is longer and connect it to tx3 */
685+
//
686+
// [ty1].0 <- [ty2].0 <- [ty3].0 <- [ty4].0 <- [ty5].0
687+
// |
688+
// [tx1].0 <- [tx2].0 <- [tx3].0 <- [ty6] --->--/
689+
// |
690+
// \---1 <- [tx4]
691+
//
692+
CTransactionRef ty1, ty2, ty3, ty4, ty5;
693+
CTransactionRef* ty[5] = {&ty1, &ty2, &ty3, &ty4, &ty5};
694+
CAmount v = 5 * COIN;
695+
for (uint64_t i = 0; i < 5; i++) {
696+
CTransactionRef& tyi = *ty[i];
697+
tyi = make_tx(MK_OUTPUTS(v), i > 0 ? MK_INPUTS(*ty[i-1]) : std::vector<CTransactionRef>());
698+
v -= 50 * CENT;
699+
pool.addUnchecked(tyi->GetHash(), entry.Fee(10000LL).FromTx(tyi));
700+
pool.GetTransactionAncestry(tyi->GetHash(), ancestors, descendants);
701+
BOOST_CHECK_EQUAL(ancestors, i+1);
702+
BOOST_CHECK_EQUAL(descendants, i+1);
703+
}
704+
CTransactionRef ty6 = make_tx(MK_OUTPUTS(5 * COIN), MK_INPUTS(tx3, ty5));
705+
pool.addUnchecked(ty6->GetHash(), entry.Fee(10000LL).FromTx(ty6));
706+
707+
// Ancestors / descendants should be:
708+
// transaction ancestors descendants
709+
// ============ =================== ===========
710+
// tx1 1 (tx1) 5 (tx1,2,3,4, ty6)
711+
// tx2 2 (tx1,2) 5 (tx1,2,3,4, ty6)
712+
// tx3 3 (tx1,2,3) 5 (tx1,2,3,4, ty6)
713+
// tx4 3 (tx1,2,4) 5 (tx1,2,3,4, ty6)
714+
// ty1 1 (ty1) 6 (ty1,2,3,4,5,6)
715+
// ty2 2 (ty1,2) 6 (ty1,2,3,4,5,6)
716+
// ty3 3 (ty1,2,3) 6 (ty1,2,3,4,5,6)
717+
// ty4 4 (y1234) 6 (ty1,2,3,4,5,6)
718+
// ty5 5 (y12345) 6 (ty1,2,3,4,5,6)
719+
// ty6 9 (tx123, ty123456) 6 (ty1,2,3,4,5,6)
720+
pool.GetTransactionAncestry(tx1->GetHash(), ancestors, descendants);
721+
BOOST_CHECK_EQUAL(ancestors, 1ULL);
722+
BOOST_CHECK_EQUAL(descendants, 5ULL);
723+
pool.GetTransactionAncestry(tx2->GetHash(), ancestors, descendants);
724+
BOOST_CHECK_EQUAL(ancestors, 2ULL);
725+
BOOST_CHECK_EQUAL(descendants, 5ULL);
726+
pool.GetTransactionAncestry(tx3->GetHash(), ancestors, descendants);
727+
BOOST_CHECK_EQUAL(ancestors, 3ULL);
728+
BOOST_CHECK_EQUAL(descendants, 5ULL);
729+
pool.GetTransactionAncestry(tx4->GetHash(), ancestors, descendants);
730+
BOOST_CHECK_EQUAL(ancestors, 3ULL);
731+
BOOST_CHECK_EQUAL(descendants, 5ULL);
732+
pool.GetTransactionAncestry(ty1->GetHash(), ancestors, descendants);
733+
BOOST_CHECK_EQUAL(ancestors, 1ULL);
734+
BOOST_CHECK_EQUAL(descendants, 6ULL);
735+
pool.GetTransactionAncestry(ty2->GetHash(), ancestors, descendants);
736+
BOOST_CHECK_EQUAL(ancestors, 2ULL);
737+
BOOST_CHECK_EQUAL(descendants, 6ULL);
738+
pool.GetTransactionAncestry(ty3->GetHash(), ancestors, descendants);
739+
BOOST_CHECK_EQUAL(ancestors, 3ULL);
740+
BOOST_CHECK_EQUAL(descendants, 6ULL);
741+
pool.GetTransactionAncestry(ty4->GetHash(), ancestors, descendants);
742+
BOOST_CHECK_EQUAL(ancestors, 4ULL);
743+
BOOST_CHECK_EQUAL(descendants, 6ULL);
744+
pool.GetTransactionAncestry(ty5->GetHash(), ancestors, descendants);
745+
BOOST_CHECK_EQUAL(ancestors, 5ULL);
746+
BOOST_CHECK_EQUAL(descendants, 6ULL);
747+
pool.GetTransactionAncestry(ty6->GetHash(), ancestors, descendants);
748+
BOOST_CHECK_EQUAL(ancestors, 9ULL);
749+
BOOST_CHECK_EQUAL(descendants, 6ULL);
750+
}
751+
574752
BOOST_AUTO_TEST_SUITE_END()

‎src/txmempool.cpp

Lines changed: 28 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1055,11 +1055,36 @@ void CTxMemPool::TrimToSize(size_t sizelimit, std::vector<COutPoint>* pvNoSpends
10551055
}
10561056
}
10571057

1058-
bool CTxMemPool::TransactionWithinChainLimit(const uint256& txid, size_t chainLimit) const {
1058+
uint64_t CTxMemPool::CalculateDescendantMaximum(txiter entry) const {
1059+
// find parent with highest descendant count
1060+
std::vector<txiter> candidates;
1061+
setEntries counted;
1062+
candidates.push_back(entry);
1063+
uint64_t maximum = 0;
1064+
while (candidates.size()) {
1065+
txiter candidate = candidates.back();
1066+
candidates.pop_back();
1067+
if (!counted.insert(candidate).second) continue;
1068+
const setEntries& parents = GetMemPoolParents(candidate);
1069+
if (parents.size() == 0) {
1070+
maximum = std::max(maximum, candidate->GetCountWithDescendants());
1071+
} else {
1072+
for (txiter i : parents) {
1073+
candidates.push_back(i);
1074+
}
1075+
}
1076+
}
1077+
return maximum;
1078+
}
1079+
1080+
void CTxMemPool::GetTransactionAncestry(const uint256& txid, size_t& ancestors, size_t& descendants) const {
10591081
LOCK(cs);
10601082
auto it = mapTx.find(txid);
1061-
return it == mapTx.end() || (it->GetCountWithAncestors() < chainLimit &&
1062-
it->GetCountWithDescendants() < chainLimit);
1083+
ancestors = descendants = 0;
1084+
if (it != mapTx.end()) {
1085+
ancestors = it->GetCountWithAncestors();
1086+
descendants = CalculateDescendantMaximum(it);
1087+
}
10631088
}
10641089

10651090
SaltedTxidHasher::SaltedTxidHasher() : k0(GetRand(std::numeric_limits<uint64_t>::max())), k1(GetRand(std::numeric_limits<uint64_t>::max())) {}

‎src/txmempool.h

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -498,6 +498,7 @@ class CTxMemPool
498498

499499
const setEntries & GetMemPoolParents(txiter entry) const EXCLUSIVE_LOCKS_REQUIRED(cs);
500500
const setEntries & GetMemPoolChildren(txiter entry) const EXCLUSIVE_LOCKS_REQUIRED(cs);
501+
uint64_t CalculateDescendantMaximum(txiter entry) const EXCLUSIVE_LOCKS_REQUIRED(cs);
501502
private:
502503
typedef std::map<txiter, setEntries, CompareIteratorByHash> cacheMap;
503504

@@ -619,8 +620,11 @@ class CTxMemPool
619620
/** Expire all transaction (and their dependencies) in the mempool older than time. Return the number of removed transactions. */
620621
int Expire(int64_t time);
621622

622-
/** Returns false if the transaction is in the mempool and not within the chain limit specified. */
623-
bool TransactionWithinChainLimit(const uint256& txid, size_t chainLimit) const;
623+
/**
624+
* Calculate the ancestor and descendant count for the given transaction.
625+
* The counts include the transaction itself.
626+
*/
627+
void GetTransactionAncestry(const uint256& txid, size_t& ancestors, size_t& descendants) const;
624628

625629
unsigned long size()
626630
{

‎src/wallet/wallet.cpp

Lines changed: 9 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -2469,8 +2469,11 @@ bool CWallet::OutputEligibleForSpending(const COutput& output, const CoinEligibi
24692469
if (output.nDepth < (output.tx->IsFromMe(ISMINE_ALL) ? eligibility_filter.conf_mine : eligibility_filter.conf_theirs))
24702470
return false;
24712471

2472-
if (!mempool.TransactionWithinChainLimit(output.tx->GetHash(), eligibility_filter.max_ancestors))
2472+
size_t ancestors, descendants;
2473+
mempool.GetTransactionAncestry(output.tx->GetHash(), ancestors, descendants);
2474+
if (ancestors > eligibility_filter.max_ancestors || descendants > eligibility_filter.max_descendants) {
24732475
return false;
2476+
}
24742477

24752478
return true;
24762479
}
@@ -2582,16 +2585,17 @@ bool CWallet::SelectCoins(const std::vector<COutput>& vAvailableCoins, const CAm
25822585
++it;
25832586
}
25842587

2585-
size_t nMaxChainLength = std::min(gArgs.GetArg("-limitancestorcount", DEFAULT_ANCESTOR_LIMIT), gArgs.GetArg("-limitdescendantcount", DEFAULT_DESCENDANT_LIMIT));
2588+
size_t max_ancestors = (size_t)std::max<int64_t>(1, gArgs.GetArg("-limitancestorcount", DEFAULT_ANCESTOR_LIMIT));
2589+
size_t max_descendants = (size_t)std::max<int64_t>(1, gArgs.GetArg("-limitdescendantcount", DEFAULT_DESCENDANT_LIMIT));
25862590
bool fRejectLongChains = gArgs.GetBoolArg("-walletrejectlongchains", DEFAULT_WALLET_REJECT_LONG_CHAINS);
25872591

25882592
bool res = nTargetValue <= nValueFromPresetInputs ||
25892593
SelectCoinsMinConf(nTargetValue - nValueFromPresetInputs, CoinEligibilityFilter(1, 6, 0), vCoins, setCoinsRet, nValueRet, coin_selection_params, bnb_used) ||
25902594
SelectCoinsMinConf(nTargetValue - nValueFromPresetInputs, CoinEligibilityFilter(1, 1, 0), vCoins, setCoinsRet, nValueRet, coin_selection_params, bnb_used) ||
25912595
(m_spend_zero_conf_change && SelectCoinsMinConf(nTargetValue - nValueFromPresetInputs, CoinEligibilityFilter(0, 1, 2), vCoins, setCoinsRet, nValueRet, coin_selection_params, bnb_used)) ||
2592-
(m_spend_zero_conf_change && SelectCoinsMinConf(nTargetValue - nValueFromPresetInputs, CoinEligibilityFilter(0, 1, std::min((size_t)4, nMaxChainLength/3)), vCoins, setCoinsRet, nValueRet, coin_selection_params, bnb_used)) ||
2593-
(m_spend_zero_conf_change && SelectCoinsMinConf(nTargetValue - nValueFromPresetInputs, CoinEligibilityFilter(0, 1, nMaxChainLength/2), vCoins, setCoinsRet, nValueRet, coin_selection_params, bnb_used)) ||
2594-
(m_spend_zero_conf_change && SelectCoinsMinConf(nTargetValue - nValueFromPresetInputs, CoinEligibilityFilter(0, 1, nMaxChainLength), vCoins, setCoinsRet, nValueRet, coin_selection_params, bnb_used)) ||
2596+
(m_spend_zero_conf_change && SelectCoinsMinConf(nTargetValue - nValueFromPresetInputs, CoinEligibilityFilter(0, 1, std::min((size_t)4, max_ancestors/3), std::min((size_t)4, max_descendants/3)), vCoins, setCoinsRet, nValueRet, coin_selection_params, bnb_used)) ||
2597+
(m_spend_zero_conf_change && SelectCoinsMinConf(nTargetValue - nValueFromPresetInputs, CoinEligibilityFilter(0, 1, max_ancestors/2, max_descendants/2), vCoins, setCoinsRet, nValueRet, coin_selection_params, bnb_used)) ||
2598+
(m_spend_zero_conf_change && SelectCoinsMinConf(nTargetValue - nValueFromPresetInputs, CoinEligibilityFilter(0, 1, max_ancestors-1, max_descendants-1), vCoins, setCoinsRet, nValueRet, coin_selection_params, bnb_used)) ||
25952599
(m_spend_zero_conf_change && !fRejectLongChains && SelectCoinsMinConf(nTargetValue - nValueFromPresetInputs, CoinEligibilityFilter(0, 1, std::numeric_limits<uint64_t>::max()), vCoins, setCoinsRet, nValueRet, coin_selection_params, bnb_used));
25962600

25972601
// because SelectCoinsMinConf clears the setCoinsRet, we now add the possible inputs to the coinset

‎src/wallet/wallet.h

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -662,8 +662,10 @@ struct CoinEligibilityFilter
662662
const int conf_mine;
663663
const int conf_theirs;
664664
const uint64_t max_ancestors;
665+
const uint64_t max_descendants;
665666

666-
CoinEligibilityFilter(int conf_mine, int conf_theirs, uint64_t max_ancestors) : conf_mine(conf_mine), conf_theirs(conf_theirs), max_ancestors(max_ancestors) {}
667+
CoinEligibilityFilter(int conf_mine, int conf_theirs, uint64_t max_ancestors) : conf_mine(conf_mine), conf_theirs(conf_theirs), max_ancestors(max_ancestors), max_descendants(max_ancestors) {}
668+
CoinEligibilityFilter(int conf_mine, int conf_theirs, uint64_t max_ancestors, uint64_t max_descendants) : conf_mine(conf_mine), conf_theirs(conf_theirs), max_ancestors(max_ancestors), max_descendants(max_descendants) {}
667669
};
668670

669671
class WalletRescanReserver; //forward declarations for ScanForWalletTransactions/RescanFromTime

0 commit comments

Comments
 (0)