Includes some commits to address follow-ups from #27021: #27021 (comment)
Reduces the effective value of unconfirmed UTXOs by the fees necessary to bump their ancestor transactions to the same feerate.
While the individual UTXOs always account for their full ancestry before coin-selection, we can correct potential overestimates with a second pass where we establish the ancestry and bump fee for the whole input set collectively.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | S3RK, brunoerg, ismaelsadeeq, t-bast, achow101 |
| Concept ACK | fanquake, glozow, jonatack, ishaanam, andrewtoth, stickies-v, LarryRuane, josibake |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
Reviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
70+
71+ // Remove the to-be-replaced transactions and build the descendant_set_by_txid cache.
72+ for (const auto& txiter : cluster) {
73+ const auto& txid = txiter->GetTx().GetHash();
74+ // Cache descendants for future use. Unlike the real mempool, a descendant MockMempoolEntry
75+ // will not exist wihout its ancestor MockMempoolEntry, so these sets won't be invalidated.
0 // will not exist without its ancestor MockMempoolEntry, so these sets won't be invalidated.
0@@ -0,0 +1,297 @@
1+#!/usr/bin/env python3
0File "test/functional/wallet_spend_unconfirmed.py" contains a shebang line, but has the file permission 644 instead of the expected executable permission 755. Do "chmod 755 test/functional/wallet_spend_unconfirmed.py" (or remove the shebang line).
1ERROR: There were 1 failed tests in the lint-files.py lint test. Please resolve the above errors.
This also needs to be added to the list of tests in test_runner.py. Which should deal with:
0�[1mWARNING!�[0m The following scripts are not being run: ['wallet_spend_unconfirmed.py']. Check the test lists in test_runner.py.
Thanks @fanquake, I fixed the two issues.
I also added a test for a transaction using subtractfeefromamount
HUGE Concept ACK obviously 🥳 🥳
~This needs to obey -maxtxfee, so I’d suggest that coin selection keeps track of what ancestors the tx has and how much of the fees is ancestors vs this transaction. And we’d want a test where the target feerate is just below maxtxfee and there are low-feerate ancestors to bump, and so total fees paid / size of this tx is higher than maxtxfee, but actually you’re just paying to bump.~
~Also, given that there is a (small) chance of overpayment, it would be good to check that -maxtxfee protects against something drastic. For example, a test where all the coins share 1 large parent? It will overestimate and -maxtxfee should make sure the tx isn’t sent?~ edit: this is totally wrong
1240+ parent_entry.GetCountWithDescendants() == 1) {
1241+ // We don't need to process this entry. Just add it to the processed
1242+ // transactions and skip.
1243+ std::swap(cluster[i+1], cluster.back());
1244+ cluster[i+1] = parent_it;
1245+ ++i;
Sorry, lint error is my fault here
0 ++i;
1243+ std::swap(cluster[i+1], cluster.back());
1244+ cluster[i+1] = parent_it;
1245+ ++i;
1246+ } else {
1247+ cluster.push_back(parent_it);
1248+ // we still need to process this
whitespace linter
0 // we still need to process this
Edit (you can set up your editor to highlight these or run test/lint/lint-whitespace.py as part of your local scripted prechecks before pushing, with clang-format, etc.):
0--- a/src/txmempool.cpp
1+++ b/src/txmempool.cpp
2@@ -1242,10 +1242,10 @@ std::vector<CTxMemPool::txiter> CTxMemPool::CalculateCluster(const std::vector<u
3 // transactions and skip.
4 std::swap(cluster[i+1], cluster.back());
5 cluster[i+1] = parent_it;
6- ++i;
7+ ++i;
8 } else {
9 cluster.push_back(parent_it);
10- // we still need to process this
11+ // we still need to process this
12 ++unprocessed_count;
13 }
14 }
15@@ -1259,10 +1259,10 @@ std::vector<CTxMemPool::txiter> CTxMemPool::CalculateCluster(const std::vector<u
16 // transactions and skip.
17 std::swap(cluster[i+1], cluster.back());
18 cluster[i+1] = child_it;
19- ++i;
20+ ++i;
21 } else {
22 cluster.push_back(child_it);
23- // we still need to process this
24+ // we still need to process this
25 ++unprocessed_count;
26 }
27 }
19+ CAmount fee_with_ancestors;
20+ int64_t vsize_individual;
21+ int64_t vsize_with_ancestors;
22+ const CTransaction& tx;
23+
24+public:
All struct members are public by default, so can either drop public here (and removed the getters), or make the struct a class if any of the data members above are intended to be private (for instance, tx and fee_individual have public getters and could be private, or just be public and drop the getter).
At first look it seems some of the data members need to be public, in which case their getters should be used or removed. This builds:
0-struct MockMempoolEntry {
1+class MockMempoolEntry
2+{
3 CAmount fee_individual;
4- CAmount fee_with_ancestors;
5- int64_t vsize_individual;
6- int64_t vsize_with_ancestors;
7 const CTransaction& tx;
8
9 public:
10+ CAmount fee_with_ancestors;
11+ int64_t vsize_individual;
12+ int64_t vsize_with_ancestors;
13 explicit MockMempoolEntry(CTxMemPool::txiter entry) :
14 fee_individual{entry->GetModifiedFee()},
15 fee_with_ancestors{entry->GetModFeesWithAncestors()},
MiniMiner.
For background, I originally was trying to align the interface with CTxMemPoolEntry like CTxMemPoolModifiedEntry to reuse the CompareTxMemPoolEntryByAncestorFee comparator in a multi index container, but then realized using simple std::maps was enough.
221+ // mempool, it is assumed that the goal is to replace that transaction. As such, the
222+ // calculation will exclude the to-be-replaced transaction, but will include the fee-bumping
223+ // cost. If bump fees of descendants of the to-be-replaced transaction are requested, the value
224+ // will be 0. Fee-related RBF rules are not included as they are logically distinct.
225+ //
226+ // Any outpoints that otherwise unavailable from the mempool (e.g. UTXOs from confirmed
nit:
0 // Any outpoints that are otherwise unavailable from the mempool (e.g. UTXOs from confirmed
225+ //
226+ // Any outpoints that otherwise unavailable from the mempool (e.g. UTXOs from confirmed
227+ // transactions or transactions not yet broadcast by the wallet) are given a bump fee of 0.
228+ //
229+ // If multiple outpoints come from the same transaction (this should be very rare because
230+ // the transaction essentially multiple change outputs or paid the same wallet using multiple
nit:
0 // it means the transaction has multiple change outputs or paid the same wallet using multiple
@glozow: Maybe for
This needs to obey -maxtxfee
Maybe we can add a maxtxfee check to the filter introduced in #25729 for max weight after input sets are produced for different subsets of the available coins. Perhaps a separate PR that builds both on this one here and #25729.
@jonatack, @t-bast:
Fixed whitespace issues, applied the propose change to a class for the struct MockMempoolEntry, amended comments in Chain interface. Thanks!
26+ explicit MockMempoolEntry(CTxMemPool::txiter entry) :
27+ fee_individual{entry->GetModifiedFee()},
28+ fee_with_ancestors{entry->GetModFeesWithAncestors()},
29+ vsize_individual(entry->GetTxSize()),
30+ vsize_with_ancestors(entry->GetSizeWithAncestors()),
31+ tx{entry->GetTx()}
in 9b33f5db095c232ac83304c91524f48bf799802f
These need to match the order of the members, CI -Wreorder-ctor says
1240+ parent_entry.GetCountWithDescendants() == 1) {
1241+ // We don't need to process this entry. Just add it to the processed
1242+ // transactions and skip.
1243+ std::swap(cluster[i+1], cluster.back());
1244+ cluster[i+1] = parent_it;
1245+ ++i;
in f40b9fe6a83b6668a625e28520456350f2cd98f0
As discussed offline, this case can be deleted since it will never be hit (and is also incorrect).
1257+ child_entry.GetCountWithDescendants() == 1) {
1258+ // We don't need to process this entry. Just add it to the processed
1259+ // transactions and skip.
1260+ std::swap(cluster[i+1], cluster.back());
1261+ cluster[i+1] = child_it;
1262+ ++i;
70@@ -66,7 +71,10 @@ struct COutput {
71 /** The fee required to spend this output at the consolidation feerate. */
72 CAmount long_term_fee{0};
73
74- COutput(const COutPoint& outpoint, const CTxOut& txout, int depth, int input_bytes, bool spendable, bool solvable, bool safe, int64_t time, bool from_me, const std::optional<CFeeRate> feerate = std::nullopt)
75+ /** The fee necessary to bump this UTXO's ancestor transactions to the target feerate */
76+ CAmount ancestor_bump_fees{0};
77+
78+ COutput(const COutPoint& outpoint, const CTxOut& txout, int depth, int input_bytes, bool spendable, bool solvable, bool safe, int64_t time, bool from_me, const std::optional<CFeeRate> feerate = std::nullopt, const std::optional<std::reference_wrapper<interfaces::Chain>> chain_interface = std::nullopt)
I have 2 concerns with this approach, please let me know what you think:
(1) The COutput constructor really shouldn’t need to have a reference the chain interface; it smells a bit weird that coinselection (which I interpret to be a relatively well-modularized component thus far since it doesn’t even depend on wallet) has a new dependency on interfaces/chain.h. It’s unclear to me why CalculateBumpFees() needs to be called inside the constructor instead of just having CAmount ancestor_bump_fees be a parameter?
(2) This means CalculateBumpFees() will be called over and over again for each COutput constructed within the AvailableCoins loop. There’s not a lot of duplicated work if all the outputs are from independent transactions, but it would definitely be faster to call it once with all the outpoints at once.
Approach-wise, I think it makes more sense to call CalculateBumpFees() just once, with the full list of outpoints. It should be fairly straightforward with preset inputs since you have the list already. For AvailableCoins, since they’re constructed as you iterate through mapWallet, you could populate each output’s ancestor_bump_fees values at the end. If you want to construct them as-is and then not mutate afterwards, then maybe do 2 passes for filtering and constructing?
313+
314+ # Actual test: Spend chain with low grandparent lower parent
315+ self.test_chain_of_high_low_below_target_feerate()
316+
317+ # Actual test: Check that fee is calculated correctly when bumping while subtracting fee from output
318+ self.test_target_feerate_unconfirmed_low_sffo()
in 337a24e6e4fd77d2fc3bf94e87c2a172c39a8fb0
Missing a test for bumpfee RPC?
This needs to obey -maxtxfee
Maybe we can add a maxtxfee check to the filter introduced in #25729 for max weight after input sets are produced for different subsets of the available coins. Perhaps a separate PR that builds both on this one here and #25729.
~Not sure if this is scope creep, but seems like breaking maxtxfee would be a bug and should probably be done in the same PR? Why not build this on top of #25729?~ ignore
Todos:
maxtxfee
maxtxfee refers to an absolute fee, not a feerate. maxtxfee is checked after a transaction is built, so I don’t see how ancestor aware funding changes anything in regard to maxtxfee—we still check at the end whether the amount of fee is allowed, regardless how we calculated the fee. If you meant maxtxfeerate, that is used to check raw transactions on submission in sendrawtx, so it doesn’t apply here either.
maxtxfee refers to an absolute fee, not a feerate. maxtxfee is checked after a transaction is built, so I don’t see how ancestor aware funding changes anything in regard to maxtxfee
Ah for some reason I thought it was a feerate, apologies. Question: is it better to only enforce -maxtxfee on the fees paid for the tx itself and not on the fees used to bump its ancestors? Or would the user expect that it’s applied to any tx, bumping or not?
Question: is it better to only enforce
-maxtxfeeon the fees paid for the tx itself and not on the fees used to bump its ancestors? Or would the user expect that it’s applied to any tx, bumping or not?
I think that -maxtxfee should be expected to behave the same regardless of bumping, it’s a context-free check.
Latest changes:
bumpfee RPCRemaining Todo:
Update:
Caveat: If multiple UTXOs share ancestry, this implementation will overpay by bumping shared ancestors once per descendant.
After calculating an input set candidate, we recalculate the bumpfee for the collective set of inputs and correct our fee estimation if there was an overestimate due to overlapping ancestries.
Left to do: • rebase • clean up tests • clean up commits • touch up commit messages
Cleaned up tests, redrew commits, touched up commit messages, rebased.
Ready for review. :partying_face:
232+ for (const auto& outpoint : requested_outpoints) {
233+ const auto& txid = outpoint.hash;
234+ // Skip any ancestors that have a higher minerscore already
235+ if (in_block.find(txid) != in_block.end()) continue;
236+ auto iter = entries_by_txid.find(outpoint.hash);
237+ assert(iter != entries_by_txid.end());
In 8a474b7675d552a9f2951d7cbc59ca36d3a10f11 “Add CalculateTotalBumpFee for overlapping ancestry”
I think the following change might make sense because that way even a MiniMiner that was initialized with confirmed outpoints could run CalculateTotalBumpFees. I think it would be better if CalculateTotalBumpFees could handle unconfirmed outpoints for uniformity with CalculateBumpFees.
0 if (iter == entries_by_txid.end()) continue;
58+ // txids of to-be-replaced transactions
59+ std::set<uint256> to_be_replaced;
60+
61+ // After using the outpoints to figure out which transactions are to be replaced, we can just
62+ // work with txids (each outpoint from a single tx should have the same bumpfee independently).
63+ // Cache which outpoint are needed for each tx so we don't have to look up all the outputs.
nit:
0 // Cache which outpoints are needed for each tx so we don't have to look up all the outputs.
102+ self.assert_spends_only_parent(ancestor_aware_tx, parent_txid)
103+
104+ self.assert_beats_target(ancestor_aware_tx)
105+ resulting_ancestry_fee_rate = self.calc_set_fee_rate([parent_tx, ancestor_aware_tx])
106+ assert_greater_than_or_equal(resulting_ancestry_fee_rate, self.target_fee_rate)
107+ assert_greater_than_or_equal(self.target_fee_rate*1.5, resulting_ancestry_fee_rate)
test_two_low_feerate_unconfirmed_parents() 1.1)?
84+ std::vector<COutPoint> reused_inputs;
85+ for (const CTxIn& txin : wtx.tx->vin) {
86+ reused_inputs.push_back(txin.prevout);
87+ }
88+
89+ std::map<COutPoint, CAmount> bump_fees = wallet.chain().CalculateBumpFees(reused_inputs, newFeerate);
CalculateTotalBumpFees() instead of CalculateBumpFees() here, given that all these inputs are from the same transaction and thus most certainly overlap in ancestry?
223+ // If the outpoint comes from an unconfirmed transaction that is already above the target
224+ // feerate or bumped by its descendant(s) already, it does not need to be bumped. Its bump fee
225+ // is 0. Likewise, if any of the transaction's ancestors are already bumped, they are not
226+ // included in the transaction's bump fee.
227+ //
228+ // This includes fee-bumping in RBFs. If an outpoint conflicts with another transaction in the
in RBFs. Perhaps This includes fee-bumping using RBF for any conflicting transactions.?
952@@ -952,6 +953,24 @@ CTxMemPool::setEntries CTxMemPool::GetIterSet(const std::set<uint256>& hashes) c
953 return ret;
954 }
955
956+std::vector<CTxMemPool::txiter> CTxMemPool::GetIterVec(const std::vector<uint256>& txids) const
957+{
958+ std::vector<txiter> ret;
AssertLockHeld(cs)?
964+ ret.push_back(*it);
965+ } else {
966+ // return empty vector to let the caller know this failed.
967+ std::vector<txiter> empty_vector;
968+ return empty_vector;
969+ }
could be a bit more concise:
0 if (!it) return {}; // return empty vector to let the caller know this failed
1 ret.push_back(*it);
2 }
I would also add a docstring to assert(it) that even though we’re currently asserting this, the interface is that upon failure an empty vector is returned, so this assertion may be removed in the future without breaking the interface?
GetIter for some other usecase and introduce a crash bug in the worst case.
952@@ -952,6 +953,24 @@ CTxMemPool::setEntries CTxMemPool::GetIterSet(const std::set<uint256>& hashes) c
953 return ret;
954 }
955
956+std::vector<CTxMemPool::txiter> CTxMemPool::GetIterVec(const std::vector<uint256>& txids) const
957+{
958+ std::vector<txiter> ret;
959+ ret.reserve(txids.size());
960+ for (const auto& txid : txids) {
961+ const auto it = GetIter(txid);
nit:
0 const auto it{GetIter(txid)};
1219+{
1220+ AssertLockHeld(cs);
1221+ std::vector<txiter> cluster{GetIterVec(txids)};
1222+ assert(cluster.size() == txids.size());
1223+ cluster.reserve(std::accumulate(cluster.cbegin(), cluster.cend(), 0, [](size_t sum, const auto it) {
1224+ return sum + it->GetCountWithAncestors() + it->GetCountWithDescendants(); }));
it->GetCountWithAncestors() and it->GetCountWithDescendants() both include the current transaction, so in that case this should be decreased with 1?
1218+std::vector<CTxMemPool::txiter> CTxMemPool::CalculateCluster(const std::vector<uint256>& txids) const
1219+{
1220+ AssertLockHeld(cs);
1221+ std::vector<txiter> cluster{GetIterVec(txids)};
1222+ assert(cluster.size() == txids.size());
1223+ cluster.reserve(std::accumulate(cluster.cbegin(), cluster.cend(), 0, [](size_t sum, const auto it) {
1227+ WITH_FRESH_EPOCH(m_epoch);
1228+ for (const auto& it : cluster) {
1229+ visited(it);
1230+ }
1231+ // i = index of where the list of unprocessed starts
1232+ for (size_t i{0}, unprocessed_count{txids.size()}; i < unprocessed_count; ++i) {
I think unprocessed_count is incorrect, shouldn’t this be e.g. to_process_count?
0 for (size_t i{0}, to_process_count{txids.size()}; i < to_process_count; ++i) {
1226+ // Use epoch: visiting an entry means we have added it to the cluster vector.
1227+ WITH_FRESH_EPOCH(m_epoch);
1228+ for (const auto& it : cluster) {
1229+ visited(it);
1230+ }
1231+ // i = index of where the list of unprocessed starts
1244+ if (!visited(child_it)) {
1245+ cluster.push_back(child_it);
1246+ // we still need to process this
1247+ ++unprocessed_count;
1248+ }
1249+ }
This can be deduplicated. Also, I think curr is not really more helpful than cluster[i], so I’d just remove that varariable.
0 auto family{cluster[i]->GetMemPoolParents()};
1 family.merge(cluster[i]->GetMemPoolChildren());
2 for (const CTxMemPoolEntry& entry : family) {
3 const auto tx_iter = mapTx.iterator_to(entry);
4 if (!visited(tx_iter)) {
5 cluster.push_back(tx_iter);
6 // we still need to process this
7 ++unprocessed_count;
8 }
9 }
GetMemPoolChildrenConst to GetMemPoolChildren and getting a mutable reference to m_parents?
merge is modifying the entry’s m_parents to now include its children as well. You can make a separate set that copies in the iterators from GetMemPoolParentsConst and GetMemPoolChildrenConst, but I’m not sure that’s worth the lines of code reduction.
700@@ -694,6 +701,9 @@ class CTxMemPool
701 std::string& errString,
702 bool fSearchForParents = true) const EXCLUSIVE_LOCKS_REQUIRED(cs);
703
704+ /** Get entire list of connected transactions for all transactions in txids. */
@pre indicating that all txids need to be in mempool?
@pre statement
19+ CAmount fee_individual;
20+ const CTransaction& tx;
21+
22+public:
23+ CAmount fee_with_ancestors;
24+ int64_t vsize_individual;
int32_t?
33+
34+ CAmount GetModifiedFee() const { return fee_individual; }
35+ CAmount GetModFeesWithAncestors() const { return fee_with_ancestors; }
36+ int64_t GetTxSize() const { return vsize_individual; }
37+ int64_t GetSizeWithAncestors() const { return vsize_with_ancestors; }
38+ const CTransaction& GetTx() const { return tx; }
0 const CTransaction& GetTx() const LIFETIMEBOUND { return tx; }
20+ const CTransaction& tx;
21+
22+public:
23+ CAmount fee_with_ancestors;
24+ int64_t vsize_individual;
25+ int64_t vsize_with_ancestors;
private instead?
fee_with_ancestors and vsize_with_ancestors get set directly in some code of mini_miner.cpp, so I think they cannot be private without also adding setters. I did make vsize_individual private
14+namespace node {
15+
16+// Container for tracking updates to ancestor feerate as we include ancestors in the "block"
17+class MockMempoolEntry
18+{
19+ CAmount fee_individual;
If tx is const I think this should be too? And same for vsize_individual?
0 const CAmount fee_individual;
const does compile.
77+ CAmount total_fees{0};
78+ int64_t total_vsize{0};
79+
80+ /** Main data structure holding the entries, can be indexed by txid */
81+ std::map<uint256, MockMempoolEntry> entries_by_txid;
82+ using MockEntryMap = decltype(entries_by_txid);
Since we’re only using the ::iterator attribute, could shorten it a bit more to
0 using MockEntryMapIter = decltype(entries_by_txid)::iterator;
50+ }
51+};
52+
53+/** A minimal version of BlockAssembler. Allows us to run the mining algorithm on a subset of
54+ * mempool transactions, ignoring consensus rules, to calculate mining scores. */
55+class MiniMiner
MiniMiner sounds catchier, would MiniBlockAssembler be a more appropriate/accurate name?
63+
64+ // After using the outpoints to figure out which transactions are to be replaced, we can just
65+ // work with txids (each outpoint from a single tx should have the same bumpfee independently).
66+ // Cache which outpoint are needed for each tx so we don't have to look up all the outputs.
67+ // Excludes to-be-replaced and unavailable transactions (set to 0).
68+ std::map<uint256, std::vector<COutPoint>> outpoints_needed_by_txid;
naming consistency
0 std::map<uint256, std::vector<COutPoint>> requested_outpoints_by_txid;
53+/** A minimal version of BlockAssembler. Allows us to run the mining algorithm on a subset of
54+ * mempool transactions, ignoring consensus rules, to calculate mining scores. */
55+class MiniMiner
56+{
57+ // Original outpoints requested
58+ std::vector<COutPoint> requested_outpoints;
requested_outpoints? I think this overlaps entirely with the keys of bump_fees? Feel free to ignore/keep it very brief if it’s a dumb remark, I don’t fully understand the PR yet.
m_ prefix?
m_
requested_outpoints in favor of the keys of bump_fees breaks tests, so I assume they’re not overlapping exactly.
40+ if (it != outpoints_needed_by_txid.end()) {
41+ it->second.push_back(outpoint);
42+ } else {
43+ std::vector<COutPoint> outpoints_of_tx({outpoint});
44+ outpoints_needed_by_txid.emplace(std::make_pair(outpoint.hash, outpoints_of_tx));
45+ // Instead of operating on the entire mempool, just run the mining algorithm on the
32+
33+ if (!mempool.exists(GenTxid::Txid(outpoint.hash))) {
34+ // This UTXO is either confirmed or not yet submitted to mempool.
35+ // In the former case, no bump fee is required.
36+ // In the latter case, we have no information, so just return 0.
37+ this->bump_fees.emplace(std::make_pair(outpoint, 0));
0 bump_fees.emplace(outpoint, 0);
214+ for (const auto& outpoint : outpoints) {
215+ bump_fees.emplace(std::make_pair(outpoint, bump_fee));
216+ }
217+ }
218+ }
219+ return this->bump_fees;
0 return bump_fees;
40+ auto it = outpoints_needed_by_txid.find(outpoint.hash);
41+ if (it != outpoints_needed_by_txid.end()) {
42+ it->second.push_back(outpoint);
43+ } else {
44+ std::vector<COutPoint> outpoints_of_tx({outpoint});
45+ outpoints_needed_by_txid.emplace(std::make_pair(outpoint.hash, outpoints_of_tx));
0 outpoints_needed_by_txid.emplace(outpoint.hash, outpoints_of_tx);
Can make a similar change elsewhere in this file.
std::make_pair throughout
51+ // Calculate the cluster and construct the entry map.
52+ std::vector<uint256> txids_needed;
53+ std::transform(outpoints_needed_by_txid.cbegin(),
54+ outpoints_needed_by_txid.cend(),
55+ std::back_inserter(txids_needed),
56+ [](const auto& pair) { return pair.first; });
0 for (const auto& [txid, outpoints] : outpoints_needed_by_txid) {
1 txids_needed.push_back(txid);
2 }
(nit, simpler)
212@@ -213,6 +213,42 @@ class Chain
213 //! Calculate mempool ancestor and descendant counts for the given transaction.
214 virtual void getTransactionAncestry(const uint256& txid, size_t& ancestors, size_t& descendants, size_t* ancestorsize = nullptr, CAmount* ancestorfees = nullptr) = 0;
215
216+ //! For each outpoint, calculate the fee-bumping cost to spend this outpoint at the specified
217+ // feerate, including bumping its ancestors. For example, if the target feerate is 10sat/vbyte
218+ // and this outpoint refers to a mempool transaction at 5sat/vbyte, the bump fee includes the
219+ // cost to bump the mempool transaction to 10sat/vbyte (i.e. 5 * mempooltx.vsize). If that
0 // cost to bump the mempool transaction to 10sat/vbyte (i.e. (10-5) * mempooltx.vsize). If that
and / or maybe use 6 instead of 5. (It’s slightly unclear as is because 10-5 == 5.)
62+ std::set<uint256> to_be_replaced;
63+
64+ // After using the outpoints to figure out which transactions are to be replaced, we can just
65+ // work with txids (each outpoint from a single tx should have the same bumpfee independently).
66+ // Cache which outpoint are needed for each tx so we don't have to look up all the outputs.
67+ // Excludes to-be-replaced and unavailable transactions (set to 0).
0 // If multiple argument outpoints correspond to the same transaction, cache them together in
1 // a single entry indexed by txid. Then we can just work with txids since all outpoints from
2 // the same tx will have the same bumpfee. Excludes non-mempool transactions.
This is how I read the code, but check my understanding!
240+ // independently, i.e. as if only one of them is spent. This may result in double-fee-bumping. This
241+ // caveat can be rectified per use of the sister-function CalculateTotalBumpFees(…).
242+ virtual std::map<COutPoint, CAmount> CalculateBumpFees(const std::vector<COutPoint>& outpoints, const CFeeRate& target_feerate) = 0;
243+
244+ //! Calculate the shared bump fees for a given set of outpoints per the
245+ // same strategy as in CalculateBumpFees(…). Other than the above call,
0 // same strategy as in CalculateBumpFees(…). Unlike the above call,
221+ // includes the cost to bump the parent (i.e. 9 * parentmempooltx.vsize).
222+ //
223+ // If the outpoint comes from an unconfirmed transaction that is already above the target
224+ // feerate or bumped by its descendant(s) already, it does not need to be bumped. Its bump fee
225+ // is 0. Likewise, if any of the transaction's ancestors are already bumped, they are not
226+ // included in the transaction's bump fee.
they are not included
Is this true? IIUC, I don’t think it works this way, or it shouldn’t. If there are two outpoints sharing the same ancestor transaction, we don’t know which of those two outpoints coin selection will choose if it chooses only one. If coin selection chooses the one for which we didn’t bump to account for the ancestor, then our fee will be too low. I thought we bump both, and then after coin selection, we make the adjustment.
63+ } else {
64+ auto outpoints_it = outpoints_needed_by_txid.find(txiter->GetTx().GetHash());
65+ if (outpoints_it != outpoints_needed_by_txid.end()) {
66+ for (const auto& outpoint : outpoints_it->second) {
67+ this->bump_fees.emplace(std::make_pair(outpoint, 0));
68+ }
0 for (const auto& outpoint : outpoints_it->second) {
1 bump_fees.emplace(outpoint, 0);
2 }
3 outpoints_needed_by_txid.erase(outpoints_it);
I’m not sure about this, but doing the erase here would be consistent with the !mempool.exists() case above (an entry for this transaction, none of whose outpoints we will use, is not added to outpoints_needed_by_txid).
92+ } else {
93+ cached_descendants.push_back(desc_it);
94+ }
95+ }
96+ }
97+ if (!remove) descendant_set_by_txid.emplace(std::make_pair(txid, cached_descendants));
0 if (!remove) descendant_set_by_txid.emplace(std::make_pair(txid, std::move(cached_descendants)));
11+#include <optional>
12+#include <stdint.h>
13+
14+namespace node {
15+
16+// Container for tracking updates to ancestor feerate as we include ancestors in the "block"
suggest adding:
0// This class must be constructed while holding mempool.cs. After construction, the object's
1// methods can be called without holding that lock.
Or maybe a better place for this comment would be just before the constructor itself.
12+#include <stdint.h>
13+
14+namespace node {
15+
16+// Container for tracking updates to ancestor feerate as we include ancestors in the "block"
17+class MockMempoolEntry
MiniMempoolEntry?
MiniMinerMempoolEntry, although I don’t feel strongly about the name.
15+
16+// Container for tracking updates to ancestor feerate as we include ancestors in the "block"
17+class MockMempoolEntry
18+{
19+ CAmount fee_individual;
20+ const CTransaction& tx;
I’m not at all sure about this, but this is a reference variable so it’s (sort of) a pointer, right? We continue to hold this reference after releasing mempool.cs – is it possible for the transaction to go away (its memory deallocated) if, for example, it gets mined, thereby invalidating our reference? I wonder if this might be better:
0 const CTransaction tx;
The debugger shows sizeof(node::MockMempoolEntry) is only 40 bytes with tx being a reference, and 152 if it’s not a reference, which makes sense because sizeof(CTransaction) is 120.
Or, maybe it should be const CTransactionRef tx; because then it’s a shared pointer, so if the tx gets removed from the mempool, our reference will remain valid until we’re done with it. The advantage of this would be we wouldn’t be making a full copy of the transaction.
227+ //
228+ // This includes fee-bumping in RBFs. If an outpoint conflicts with another transaction in the
229+ // mempool, it is assumed that the goal is to replace that transaction. As such, the
230+ // calculation will exclude the to-be-replaced transaction, but will include the fee-bumping
231+ // cost. If bump fees of descendants of the to-be-replaced transaction are requested, the value
232+ // will be 0. Fee-related RBF rules are not included as they are logically distinct.
Would it be safer not to make this assumption? The cluster would be larger, but since the algorithms are efficient, the performance difference shouldn’t be a problem. The MiniMiner constructor would be simpler too. Even if currently we’re never given a outpoint that conflicts with a transaction not being replaced, could that possibly change in the future?
Initially, I thought that was I’m suggesting here wouldn’t work, because suppose that the transaction that our outpoint refers to (let’s call it the parent) has a low feerate, but the transaction we’re replacing (the parent’s existing child) has a very high feerate. If, contrary to what the code currently does, we ignore the fact that we’re replacing the child (we keep the child in the cluster, as if we’re spending a different output of the parent), then we might conclude that we don’t need to bump the parent’s fee – whereas we actually do, because the child will no longer exist!
But I think this won’t happen because rule 6 of our replacement policy requires the replacement transaction to have a higher feerate than all that it’s replacing.
- The replacement transaction’s feerate is greater than the feerates of all directly conflicting transactions.
So it seems like even if we (mistakenly) think that the existing child will remain and that it will bump the parent somewhat, it won’t bump it enough to meet our requested feerate, so we’ll still bump it (as required by our desired feerate).
137+ auto best_iter = entries.begin();
138+ assert(best_iter != entries.end());
139+ const auto ancestor_package_size = (*best_iter)->second.GetSizeWithAncestors();
140+ const auto ancestor_package_fee = (*best_iter)->second.GetModFeesWithAncestors();
141+ // Stop here. Everything that didn't "make it into the block" has bumpfee.
142+ if (best_iter == entries.end() || ancestor_package_fee < target_feerate.GetFee(ancestor_package_size)) {
Simplification, we just asserted that it’s not entries.end()
0 if (ancestor_package_fee < target_feerate.GetFee(ancestor_package_size)) {
74+ for (const auto& txiter : cluster) {
75+ const auto& txid = txiter->GetTx().GetHash();
76+ // Cache descendants for future use. Unlike the real mempool, a descendant MockMempoolEntry
77+ // will not exist without its ancestor MockMempoolEntry, so these sets won't be invalidated.
78+ std::vector<MockEntryMap::iterator> cached_descendants;
79+ cached_descendants.emplace_back(entries_by_txid.find(txid));
find() could be entries_by_txid.end() (not found), because in the previous loop (also over cluster), if the transaction is found in to_be_replaced (so we take the else path), then the transaction is not added to entries_by_txid. I think it turns out to be harmless, but I just wanted to point it out because it looks like it may be unintentional.
Also noticed this and was a bit confused. In the case where remove is true, cache_descendants never gets added to the descendants_by_txid map, which is fine. In the case where remove is false, then txid should be in entries_by_txid, so it will get added to cache_descendants, along with all of descendants.
So what gets added to descendants_by_txid is txidA: [txidA, txidB, txidC...], basically the parent + all of its children, which seems incorrect?
emplace_back() shouldn’t be here at all, because each mempool entry is a member of its own descendants’ list. So this “main” transaction gets added to cached_descendants below. This was causing the fee_with_ancestors and vsize_with_ancestors to go negative near the end of BuildMockTemplate() below.
… which seems incorrect?
No, I think it’s a convention in the strange Land of Mempool that a transaction’s ancestor and descendant lists both include the transaction itself. This relates directly to my comment elsewhere that the emplace_back() should be removed; as it’s currently written, the list would be: txidA: [txidA, txidA, txidB, txidC...]. This later causes a major accounting problem!
emplace_back() on !remove.
188+ // Build a block template until the target feerate is hit.
189+ BuildMockTemplate(target_feerate);
190+ assert(in_block.empty() || CFeeRate(total_fees, total_vsize) >= target_feerate);
191+
192+ // Each transaction that "made it into the block" has a bumpfee of 0, i.e. they are part of an
193+ // ancestor package that exceeds the target feerate and don't need to be bumped.
0 // ancestor package with at least the target feerate and don't need to be bumped.
(if the feerates are equal, no bump is required)
957+{
958+ std::vector<txiter> ret;
959+ ret.reserve(txids.size());
960+ for (const auto& txid : txids) {
961+ const auto it = GetIter(txid);
962+ assert(it);
if GetIter(txid) cannot find the txid in mapTx, it will return a std::nullopt, which will then cause the node to crash. this seems really dangerous.
wouldn’t it be better to remove the assert and just let GetIterVec return early with an empty vector if it’s passed a txid which isn’t in the mempool? based on the comment, that seems to be the intention of this code, so im not sure what good the assert is doing here
else branch is dead since the assert would hit.
1213@@ -1196,3 +1214,40 @@ const std::string RemovalReasonToString(const MemPoolRemovalReason& r) noexcept
1214 }
1215 assert(false);
1216 }
1217+
1218+std::vector<CTxMemPool::txiter> CTxMemPool::CalculateCluster(const std::vector<uint256>& txids) const
1219+{
1220+ AssertLockHeld(cs);
1221+ std::vector<txiter> cluster{GetIterVec(txids)};
1222+ assert(cluster.size() == txids.size());
GetIterVec is written, it will return an empty vector if any of the txids are not found in the mempool, so wouldn’t it be better to check for an empty vector here and return an error to the user letting them know one of the txids they sent wasn’t found in the mempool? the way it’s written now, if an empty vector is returned the node will crash
654
655+ /** Translate a list of hashes into a list of mempool iterators to avoid repeated lookups.
656+ * The nth element in txids becomes the nth element in the returned vector. If any of the txids
657+ * don't actually exist in the mempool, returns an empty vector. */
658+ std::vector<txiter> GetIterVec(const std::vector<uint256>& txids) const EXCLUSIVE_LOCKS_REQUIRED(cs);
659+
GetIterVec and CalculateCluster, this is not accurate, unless I am misunderstanding something. if a txid does not exist in the mempool, GetIter returns a nullopt, which then causes GetIterVec to crash. if we remove that assert, then the assert in CalculateCluster checking that txids.size() == cluster.size() will crash the node due to cluster being an empty vector
25+ // Anything otherwise unavailable just has a bump fee of 0
26+ for (const auto& outpoint : outpoints) {
27+ if (const auto ptx{mempool.GetConflictTx(outpoint)}) {
28+ // The conflicting transaction is to-be-replaced
29+ to_be_replaced.insert(ptx->GetHash());
30+ }
It could be my own unfamiliarity with the mempool, but I’m not really sure what is happening here. To check my understanding:
outpoints here refers to a set of unconfirmed UTXOs that our wallet would like to spend, meaning they are tx outputs. GetConflictTx checks if the outpoint exists in mapNextTx, which means it is the prevout (or input) into another tx in the mempool. If there is another transaction in the mempool spending one of these outputs, we put the tx which is spending the outpointt into to_be_replaced. This is because we plan to construct a transaction which would then replace the conflicting tx, so we want to ignore doing anything with the conflicting txs for now?
It might be helpful to be a little more explicit than just outpoints (or write a more detailed comment for the function) as this can refer to either outputs or prevouts, which in this context makes things confusing to follow.
To check my understanding:
outpoints here refers to a set of unconfirmed UTXOs that our wallet would like to spend, meaning they are tx outputs.
They are not necessarily unconfirmed UTXOs, just some specified outpoints/prevouts. They may refer to confirmed or unconfirmed UTXOs. They may refer to outputs that the node doesn’t think exist.
GetConflictTx checks if the outpoint exists in mapNextTx, which means it is the prevout (or input) into another tx in the mempool. If there is another transaction in the mempool spending one of these outputs, we put the tx which is spending the outpointt into to_be_replaced.
correct, GetConflictTx gives you another mempool transaction that spends the same tx.
This is because we plan to construct a transaction which would then replace the conflicting tx, so we want to ignore doing anything with the conflicting txs for now?
We want to ensure we provide a bump fee for this UTXO, but ensure we calculate the bump fees exluding the to-be-replaced transaction(s). Once they’re replaced, they won’t be there to bump their ancestors. So if we’re replacing the child of a CPFP (e.g. to increase the bump even more), we want the correct fee to bump that parent without the original child there.
Thanks, this explanation really helps!
They are not necessarily unconfirmed UTXOs, just some specified outpoints/prevouts. They may refer to confirmed or unconfirmed UTXOs. They may refer to outputs that the node doesn’t think exist.
Is it correct to say these outpoints are intended to be spent in a new transaction, as in the wallet has a set of UTXOs that it wants to spend in TxB where TxB can be a replacement of an already existing TxA or it can be an entirely new tx? From there, these outpoints can be a mix of confirmed/unconfirmed, but it’s assumed at least one outpoint is unconfirmed
outpoint has any conflicting spends in the mempool before we check if the outpoint itself is in the mempool. It seems we are inferring that the outpoint is in the mempool, otherwise it would not be present in mapNextTx. Is this the safest way to do this? Seems fine because mapNextTx is updated every time a tx is removed from the mempool, but figured I’d ask anyway in case there is a race condition / code path I’m not seeing which could make this unreliable in an edge case
Is it correct to say these outpoints are intended to be spent in a new transaction, as in the wallet has a set of UTXOs that it wants to spend in TxB where TxB can be a replacement of an already existing TxA or it can be an entirely new tx?
Yes, exactly. If we’re constructing MiniMiner to CalculateBumpFees(), these outpoints are basically all the coins owned by the wallet, i.e. AvailableCoins. If we’re constructing MiniMiner to CalculateTotalBumpFees(), these are the outpoints we’ve decided to use to fund txB.
From there, these outpoints can be a mix of confirmed/unconfirmed, but it’s assumed at least one outpoint is unconfirmed
Not exactly. They could all be confirmed / nonexistent - in that case this constructor will end up not constructing any MockMempoolEntrys. When you call CalculateBumpFees() afterwards, it should just return the already-existing map of all-0 bump fees. This is the first test case in miniminer_tests.cpp.
Concept ACK
Still in the process of reviewing (have reviewed up to and most of b669fd94f84e679d4549ef0abe1b0483e1406152), but left a few comments.
Some general feedback:
I noticed a lot of asserts being used. As I understand it, this is going to be a functionality exposed to the wallet, so wouldn’t it be better to replace asserts with error messages that can be returned to the wallet? Seems better than crashing the node, IMO.
Also, the MiniMiner function is quite complicated. It might be worth adding a high-level overview to the PR description for the MiniMiner::MiniMiner as it seems like this is where a lot of the logic is. In particular, it seems to be working with several caches, so perhaps describing the general flow would be helpful. Take this feedback with a grain of salt, tho, as I’m not super familiar with the mempool and that could be why I’m struggling to follow the steps :sweat_smile:
Hopefully this is helpful and not annoying, but here’s a branch that makes the first couple commits less assert-happy and applies some of the suggestions to miniminer: https://github.com/glozow/bitcoin/tree/26152-fixups
lgtm!
110+ const CFeeRate a_feerate(a->second.GetModFeesWithAncestors(), a->second.GetSizeWithAncestors());
111+ const CFeeRate b_feerate(b->second.GetModFeesWithAncestors(), b->second.GetSizeWithAncestors());
112+ if (a_feerate != b_feerate) {
113+ return a_feerate > b_feerate;
114+ }
115+ return &(*a) > &(*b);
return a_feerate >= b_feerate or something like that?
Two thoughts:
I think this could be split up into two PRs; one for MiniMiner and another for the wallet logic. My reasoning here is the MiniMiner code seems like it could be useful outside of the wallet use case and thus could be merged independently and both parts by themselves seem complex enough that splitting it into two might make things easier to review.
per @LarryRuane ’s comment #26152 (comment), I’m also not totally convinced we need the to_be_replaced logic in MiniMiner. I took out all of the code relating to to_be_replaced and the unit and functional tests still pass. It’s possible this is due to insufficient test coverage, in which case having a test case to demonstrate why this logic is necessary would be super helpful. I have a few more thoughts regarding whether MiniMiner or the wallet should be handling the conflicting transactions logic, but I’ll try to add a concrete example in a test case to first convince myself and also make the conversation more productive
I’m also not totally convinced we need the to_be_replaced logic in MiniMiner. I took out all of the code relating to to_be_replaced and the unit and functional tests still pass. It’s possible this is due to insufficient test coverage
I would say this is at least 95% due to insufficient coverage in the unit tests (pretty sparse at the moment). I don’t think it’s possible to hit very much of the replacement-related logic through functional tests, because our (1) RBF rules currently do not allow any additional unconfirmed inputs so we won’t actually ask MiniMiner to calculate much, (2) our wallet will never try to replace something with descendants and (3) our wallet will always use all the inputs from the replacee (so the size of to_be_replaced is always 1 and the set of unconfirmed ancestors is identical before and after).
But maybe this is a good reason to remove it! If it never executes because of how our wallet operates, and the wallet is the only client of this interface, then it would be complexity for no reason. We could just use CalculateTotalBumpFees() for a replacement (since that’s all the unconfirmed inputs we’ll use) and only add logic for replacements if (1) ever changes in the future.
But maybe this is a good reason to remove it! If it never executes because of how our wallet operates, and the wallet is the only client of this interface, then it would be complexity for no reason.
I think this is a good reason to leave it out for now. I started writing a unit test to try and cover different replacement scenarios, but it felt a little silly to be writing tests for scenarios that would never actually happen to due the RBF rules and how the wallet behaves.
I also haven’t been able to think of a scenario where MiniMiner would return an incorrect bumpfee if it didn’t have the “to be replaced logic” if MiniMiner was given a set of “reasonable” outpoints in the first place. @Xekyo perhaps you have an example that I haven’t thought of?
tldr; imo we should prefer a simpler implementation which makes as few assumptions as possible and add in more complexity if/when it is needed.
154+ assert(iter != to_process.end());
155+ const CTransaction& tx = (*iter)->second.GetTx();
156+ for (const auto& input : tx.vin) {
157+ if (auto parent_it{entries_by_txid.find(input.prevout.hash)}; parent_it != entries_by_txid.end()) {
158+ to_process.insert(parent_it);
159+ ancestors.insert(parent_it);
The benchmark (part of my branch) experienced a near-infinite loop without this change. The reason is that the benchmark test mempool has such a high degree of fan-out and fan-in that a single transaction could be added to the to_process set over and over. Of course, std::set doesn’t allow duplicates, but I added some debug logging that shows the same tx being added, removed, processed, then added again, many times. This change fixed the problem completely.
0 if (!ancestors.count(parent_it)) {
1 to_process.insert(parent_it);
2 ancestors.insert(parent_it);
3 }
244+ assert(iter != to_process.end());
245+ const CTransaction& tx = (*iter)->second.GetTx();
246+ for (const auto& input : tx.vin) {
247+ if (auto parent_it{entries_by_txid.find(input.prevout.hash)}; parent_it != entries_by_txid.end()) {
248+ to_process.insert(parent_it);
249+ ancestors.insert(parent_it);
Same change as above – this is much more efficient with a large cluster, for example, running the benchmark.
0 if (!ancestors.count(parent_it)) {
1 to_process.insert(parent_it);
2 ancestors.insert(parent_it);
3 }
170+ for (const auto& anc : ancestors) {
171+ in_block.insert(anc->second.GetTx().GetHash());
172+ total_fees += anc->second.GetModifiedFee();
173+ total_vsize += anc->second.GetTxSize();
174+ auto it = descendant_set_by_txid.find(anc->second.GetTx().GetHash());
175+ assert(it != descendant_set_by_txid.end());
0 // each entry's descendant set includes itself
1 assert(it != descendant_set_by_txid.end());
(it took me a little while to figure out why this assertion is valid)
173+ total_vsize += anc->second.GetTxSize();
174+ auto it = descendant_set_by_txid.find(anc->second.GetTx().GetHash());
175+ assert(it != descendant_set_by_txid.end());
176+ for (const auto& descendant : it->second) {
177+ descendant->second.vsize_with_ancestors -= anc->second.GetTxSize();
178+ descendant->second.fee_with_ancestors -= anc->second.GetModifiedFee();
You may want to assert that these don’t go negative, I would actually do it before the decrement, like this:
0assert(descendant->second.vsize_with_ancestors >= anc->second.GetTxSize());
1descendant->second.vsize_with_ancestors -= anc->second.GetTxSize();
Assume instead.
169+ // "Mine" all transactions in this ancestor set.
170+ for (const auto& anc : ancestors) {
171+ in_block.insert(anc->second.GetTx().GetHash());
172+ total_fees += anc->second.GetModifiedFee();
173+ total_vsize += anc->second.GetTxSize();
174+ auto it = descendant_set_by_txid.find(anc->second.GetTx().GetHash());
simpler:
0 auto it = descendant_set_by_txid.find(anc->first);
166+ assert(ancestor_package_fee == std::accumulate(ancestors.cbegin(), ancestors.cend(), 0,
167+ [](CAmount sum, const auto it) {return sum + it->second.GetModifiedFee();}));
168+
169+ // "Mine" all transactions in this ancestor set.
170+ for (const auto& anc : ancestors) {
171+ in_block.insert(anc->second.GetTx().GetHash());
0 in_block.insert(anc->first);
(functionally the same, just simpler)
MiniMiner); feel free to cherry-pick this commit. It also seems useful as a stress test – most of my comments below are the result of running the benchmark.
WIP: I’ve addressed all comments up to #26152 (comment)
I am currently evaluating whether splitting this PR in two makes sense, and still need to investigate @LarryRuane’s alternative branch.
Redesignating this PR to draft to signal that it’s not ready for review at the moment.
Rebased on latest version of #27021, then rebased on master to resolve merge conflicts.
Needed to reintroduce access to the Chain interface in ChooseSelectionResult().
589+ }
590+ CAmount bump_fee_overestimate = summed_bump_fees - grouped_bump_fees.value();
591+ if (bump_fee_overestimate) {
592+ result.SetBumpFeeDiscount(bump_fee_overestimate);
593+ // Update waste
594+ result.ComputeAndSetWaste(coin_selection_params.min_viable_change, coin_selection_params.m_cost_of_change, coin_selection_params.m_change_fee);
in “Amend bumpfee for inputs with overlapping ancestry” 56139fd808250fcb693b5f185e7e510804a33470
nit: while you’re here, should we drop earlier calls to ComputeAndSetWaste and just call it once for all eligible results before selecting the best one?
SelectCoinsBnB(). It does pass the coin selection tests. (I’ll let the fuzzer run over night to see whether that finds something else.)
In theory the waste score should be the same.
Because not all of the params are available within BnB we call
result.ComputeAndSetWaste(cost_of_change, cost_of_change, 0);
instead of
result.ComputeAndSetWaste(min_viable_change, cost_of_change, change_fee);
I believe this should be fine as BnB will not produce results with more than cost_of_change excess and therefore GetChange() will always return 0 and then change_fee doesn’t matter.
195@@ -195,6 +196,7 @@ util::Result<PreSelectedInputs> FetchSelectedInputs(const CWallet& wallet, const
196
197 /* Set some defaults for depth, spendable, solvable, safe, time, and from_me as these don't matter for preset inputs since no selection is being done. */
198 COutput output(outpoint, txout, /*depth=*/ 0, input_bytes, /*spendable=*/ true, /*solvable=*/ true, /*safe=*/ true, /*time=*/ 0, /*from_me=*/ false, coin_selection_params.m_effective_feerate);
199+ output.ApplyBumpFee(map_of_bump_fees.at(output.outpoint));
test_preset_input_cpfp testing this?
6@@ -7,6 +7,7 @@
7 #include <common/system.h>
8 #include <consensus/amount.h>
9 #include <consensus/consensus.h>
10+#include <interfaces/chain.h>
In 232edb7e5d630d9d687ba3089aa4028f8e0380a4 " Bump unconfirmed parent txs to target feerate "
Why was this added?
350@@ -346,6 +351,18 @@ CoinsResult AvailableCoins(const CWallet& wallet,
351 }
352 }
353
354+ if (feerate.has_value()) {
355+ std::map<COutPoint, CAmount> map_of_bump_fees = wallet.chain().CalculateBumpFees(outpoints, feerate.value());
356+
357+ for (auto& [_, outputs] : result.coins) {
in “Bump unconfirmed parent txs to target feerate” 8e3d924f02ee28043e8844a301389915823e5893
nit: suggestion
0for (auto& output : result.coins.All()) {
1 output.ApplyBumpFee(map_of_bump_fees.at(output.outpoint));
2}
CoinsResult.All() produces a copy. Reverting this.
519@@ -512,7 +520,12 @@ CAmount SelectionResult::GetSelectedValue() const
520
521 CAmount SelectionResult::GetSelectedEffectiveValue() const
522 {
523- return std::accumulate(m_selected_inputs.cbegin(), m_selected_inputs.cend(), CAmount{0}, [](CAmount sum, const auto& coin) { return sum + coin->GetEffectiveValue(); });
524+ return std::accumulate(m_selected_inputs.cbegin(), m_selected_inputs.cend(), CAmount{0}, [](CAmount sum, const auto& coin) { return sum + coin->GetEffectiveValue(); }) + bump_fee_group_discount;
525+}
526+
527+CAmount SelectionResult::GetBumpFeeDiscount() const
in “Amend bumpfee for inputs with overlapping ancestry” 56139fd808250fcb693b5f185e7e510804a33470
nit: you can drop this method if you decide to have GetTotalBumpFee() instead
GetSummedBumpFees(), but I have yet to find a neat way how I can drop the discount. I was experimenting with having both a function GetSummedBumpFees() and a function GetCombinedBumpFee() on SelectionResult, but it made it more complicated because I now had to get the Chain interface and target feerate into SelectionResult. I am tending to leaving it as it is now, unless I come up with a more elegant way to transition.
bump_fee_group_discount within GetTotalBumpFee() function. Probably I’m missing something but I don’t know what. Why wouldn’t it just work?
GetTotalBumpFee() does not either return the sum of individual bump fees or the combined bump fee for the input set depending on when it is called. I’d be worried that it would be hard to understand later and might be a source of future bugs.
149@@ -148,8 +150,7 @@ util::Result<SelectionResult> AttemptSelection(const CAmount& nTargetValue, Outp
150 * or (2) an specific error message if there was something particularly wrong (e.g. a selection
151 * result that surpassed the tx max weight size).
152 */
153-util::Result<SelectionResult> ChooseSelectionResult(const CAmount& nTargetValue, Groups& groups, const CoinSelectionParams& coin_selection_params);
154-
155+util::Result<SelectionResult> ChooseSelectionResult(const CWallet& wallet, const CAmount& nTargetValue, Groups& groups, const CoinSelectionParams& coin_selection_params);
in “Amend bumpfee for inputs with overlapping ancestry” 56139fd808250fcb693b5f185e7e510804a33470
nit: we don’t really need the whole wallet there, better to pass just chain interface
610@@ -611,6 +611,28 @@ util::Result<SelectionResult> ChooseSelectionResult(const CAmount& nTargetValue,
611 return errors.empty() ? util::Error() : errors.front();
612 }
613
614+ // If the chosen input set has unconfirmed inputs, check for synergies from overlapping ancestry
615+ for (auto& result : results) {
616+ std::vector<COutPoint> outpoints;
617+ CAmount summed_bump_fees = 0;
in “Amend bumpfee for inputs with overlapping ancestry” 56139fd808250fcb693b5f185e7e510804a33470
nit: you can use SelectionResult::GetTotalBumpFee() if you decide to introduce it
GetSummedBumpFees()
1024@@ -1008,7 +1025,9 @@ static util::Result<CreatedTransactionResult> CreateTransactionInternal(
1025 // and in the spirit of "smallest possible change from prior
1026 // behavior."
1027 const uint32_t nSequence{coin_control.m_signal_bip125_rbf.value_or(wallet.m_signal_rbf) ? MAX_BIP125_RBF_SEQUENCE : CTxIn::MAX_SEQUENCE_NONFINAL};
1028+ CAmount total_bump_fees{0};
in “Bump unconfirmed parent txs to target feerate” 8e3d924f02ee28043e8844a301389915823e5893
nit: we can encapsulate bump fee calculation within CoinSelectionResult::GetTotalBumpFee(). It will come handy in the next commit as well
70+ def test_target_feerate_unconfirmed_high(self):
71+ self.log.info("Start test feerate with high feerate unconfirmed input")
72+ wallet = self.setup_and_fund_wallet("unconfirmed_high_wallet")
73+
74+ # Send unconfirmed transaction with high feerate to testing wallet
75+ parent_txid = wallet.sendtoaddress(address=wallet.getnewaddress(), amount=1, fee_rate=100)
in “Bump unconfirmed parent txs to target feerate” 8e3d924f02ee28043e8844a301389915823e5893
nit: better use a multiple of target_fee_rate
67+ self.generate(self.nodes[0], 1)
68+ wallet.unloadwallet()
69+
70+ def test_target_feerate_unconfirmed_high(self):
71+ self.log.info("Start test feerate with high feerate unconfirmed input")
72+ wallet = self.setup_and_fund_wallet("unconfirmed_high_wallet")
in “Bump unconfirmed parent txs to target feerate” 8e3d924f02ee28043e8844a301389915823e5893
I think all the tests would be easier to understand if we start with an empty testing wallet and send unconfirmed txs to it from def_wallet.
Now we start with the testing wallet with already one confirmed input, but why if we test spending mostly unconfirmed txs? Current setup requires reader to do more coin tracking in mind.
I’m not sure I see a big difference between setting up the wallet with a confirmed UTXO from which I send an unconfirmed transaction with a varying feerate, or setting up the wallet to have an unconfirmed UTXO with varying feerate. The former may have the advantage that when I need multiple UTXOs, it’s easier to understand what’s different.
Currently expecting to keep it this way
80+ ancestor_aware_tx = wallet.gettransaction(txid=ancestor_aware_txid, verbose=True)
81+
82+ self.assert_spends_only_parent(ancestor_aware_tx, parent_txid)
83+
84+ self.assert_beats_target(ancestor_aware_tx)
85+ resulting_ancestry_fee_rate = self.calc_set_fee_rate([parent_tx, ancestor_aware_tx])
350@@ -346,6 +351,15 @@ CoinsResult AvailableCoins(const CWallet& wallet,
351 }
352 }
353
354+ if (feerate.has_value()) {
355+ std::map<COutPoint, CAmount> map_of_bump_fees = wallet.chain().CalculateIndividualBumpFees(outpoints, feerate.value());
356+
357+ for (auto& output : result.All()) {
358+ output.ApplyBumpFee(map_of_bump_fees.at(output.outpoint));
In f2daebf8d1ac3829faa6443425cb97a763e9f780 “Bump unconfirmed parent txs to target feerate”
This doesn’t actually apply the bumpfees to all of the outputs in result. All()` will return a copy of all of the outputs, not references to them, so this is just applying the bumpfees to a temporary.
Because of this, the tests added in this commit fail. The entire PR does not have failing tests because of the adjustment to the fee that is done in the following commit.
411+ self.log.info("Starting UnconfirmedInputTest!")
412+ self.target_fee_rate = 30
413+ self.def_wallet = self.nodes[0].get_wallet_rpc(self.default_wallet_name)
414+ self.generate(self.nodes[0], 110)
415+
416+ # Test that assumptions about meeting feerate and being able to test it hold
In f2daebf8d1ac3829faa6443425cb97a763e9f780 “Bump unconfirmed parent txs to target feerate”
nit: I prefer that comments about what a test does are attached to the test implementation itself rather than the caller.
85+ reused_inputs.reserve(mtx.vin.size());
86+ for (const CTxIn& txin : mtx.vin) {
87+ reused_inputs.push_back(txin.prevout);
88+ }
89+
90+ std::map<COutPoint, CAmount> bump_fees = wallet.chain().CalculateBumpFees(reused_inputs, newFeerate);
In f2daebf8d1ac3829faa6443425cb97a763e9f780 “Bump unconfirmed parent txs to target feerate”
Compilation failure in this commit.
0wallet/feebumper.cpp: In function ‘wallet::feebumper::Result wallet::CheckFeeRate(const CWallet&, const CMutableTransaction&, const CFeeRate&, int64_t, CAmount, std::vector<bilingual_str>&)’:
1wallet/feebumper.cpp:89:61: error: ‘class interfaces::Chain’ has no member named ‘CalculateBumpFees’
2 89 | std::map<COutPoint, CAmount> bump_fees = wallet.chain().CalculateBumpFees(reused_inputs, newFeerate);
3 | ^~~~~~~~~~~~~~~~~
This function was renamed to CalculateIndividualBumpFees
94+
95+ parent_txid = wallet.sendtoaddress(address=wallet.getnewaddress(), amount=1, fee_rate=1)
96+ parent_tx = wallet.gettransaction(txid=parent_txid, verbose=True)
97+
98+ self.assert_undershoots_target(parent_tx)
99+ resulting_fee_rate_funding = self.calc_fee_rate(parent_tx)
assert_undershoots_target
I tried to run my own set of external e2e tests against https://github.com/bitcoin/bitcoin/pull/26152/commits/43b86e49351b3160c2e39807997a9373cd88e173 and they all hit the following assertion error:
0bitcoind: wallet/coinselection.cpp:495: void wallet::SelectionResult::SetBumpFeeDiscount(CAmount): Assertion `discount >= 0' failed
Reading through recent comments, it seems somewhat expected that this version isn’t yet fully working, let me know when you want me to run my tests again!
Reading through recent comments, it seems somewhat expected that this version isn’t yet fully working, let me know when you want me to run my tests again!
Sorry about that, I was a bit in a rush on Friday. I’ve fixed a number of issues, if this latest push’s checks turn up green, it might be more fruitful to try now.
62+
63+ ancestor_aware_txid = wallet.sendtoaddress(address=self.def_wallet.getnewaddress(), amount=0.5, fee_rate=self.target_fee_rate)
64+ ancestor_aware_tx = wallet.gettransaction(txid=ancestor_aware_txid, verbose=True)
65+ self.assert_beats_target(ancestor_aware_tx)
66+
67+ self.generate(self.nodes[0], 1)