Bump unconfirmed ancestor transactions to target feerate #26152

        // will not exist without its ancestor MockMempoolEntry, so these sets won't be invalidated.

File "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).
ERROR: 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:

�[1mWARNING!�[0m The following scripts are not being run: ['wallet_spend_unconfirmed.py']. Check the test lists in test_runner.py.

Sorry, lint error is my fault here

                        ++i;

whitespace linter

                        // 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.):

--- a/src/txmempool.cpp
+++ b/src/txmempool.cpp
@@ -1242,10 +1242,10 @@ std::vector<CTxMemPool::txiter> CTxMemPool::CalculateCluster(const std::vector<u
                         // transactions and skip.
                         std::swap(cluster[i+1], cluster.back());
                         cluster[i+1] = parent_it;
-                        ++i; 
+                        ++i;
                     } else {
                         cluster.push_back(parent_it);
-                        // we still need to process this 
+                        // we still need to process this
                         ++unprocessed_count;
                     }
                 }
@@ -1259,10 +1259,10 @@ std::vector<CTxMemPool::txiter> CTxMemPool::CalculateCluster(const std::vector<u
                         // transactions and skip.
                         std::swap(cluster[i+1], cluster.back());
                         cluster[i+1] = child_it;
-                        ++i; 
+                        ++i;
                     } else {
                         cluster.push_back(child_it);
-                        // we still need to process this 
+                        // we still need to process this
                         ++unprocessed_count;
                     }
                 }

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:

-struct MockMempoolEntry {
+class MockMempoolEntry
+{
     CAmount fee_individual;
-    CAmount fee_with_ancestors;
-    int64_t vsize_individual;
-    int64_t vsize_with_ancestors;
     const CTransaction& tx;
 
 public:
+    CAmount fee_with_ancestors;
+    int64_t vsize_individual;
+    int64_t vsize_with_ancestors;
     explicit MockMempoolEntry(CTxMemPool::txiter entry) :
         fee_individual{entry->GetModifiedFee()},
         fee_with_ancestors{entry->GetModFeesWithAncestors()},

I'd say just remove the getters, it's fine to keep these public. This struct is only used by 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.

nit:

    //  Any outpoints that are otherwise unavailable from the mempool (e.g. UTXOs from confirmed

nit:

    //  it means the transaction has multiple change outputs or paid the same wallet using multiple

in 9b33f5db095c232ac83304c91524f48bf799802f

These need to match the order of the members, CI -Wreorder-ctor says

in f40b9fe6a83b6668a625e28520456350f2cd98f0

As discussed offline, this case can be deleted since it will never be hit (and is also incorrect).

this as well in f40b9fe6a83b6668a625e28520456350f2cd98f0

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?

Now applying the bumpfees to each output after constructing the output.

in 337a24e6e4fd77d2fc3bf94e87c2a172c39a8fb0

Missing a test for bumpfee RPC?

Added!

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.

        if (iter == entries_by_txid.end()) continue;

Yes, that makes sense, thank you

nit:

    // Cache which outpoints are needed for each tx so we don't have to look up all the outputs.

Fixed, thanks!

Where does the 1.5 come from here (and in later tests e.g. test_two_low_feerate_unconfirmed_parents() 1.1)?

It's just to check that the fees are at least what was expected, but do not overshoot too far, e.g. because we selected more inputs than expected.

Would it maybe make sense to use CalculateTotalBumpFees() instead of CalculateBumpFees() here, given that all these inputs are from the same transaction and thus most certainly overlap in ancestry?

Yes, in fact not accounting for overlapping ancestries here might have been a bug.

It's unclear to me what the first sentence here is saying with in RBFs. Perhaps This includes fee-bumping using RBF for any conflicting transactions.?

Thanks, I've updated the comment.

Should this function start with AssertLockHeld(cs)?

Thanks, fixed

could be a bit more concise:

        if (!it) return {}; // return empty vector to let the caller know this failed
        ret.push_back(*it);
        }

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?

I'd say we should either assert (and document that assumption) or return an empty vector, but not both. In the current form, this seems dangerous - in the future someone might read the doc for the function, use the generic GetIter for some other usecase and introduce a crash bug in the worst case.

Thanks, removed the assert and simplified as suggested by @stickies-v

nit:

        const auto it{GetIter(txid)};

Fixed, thanks

I think it->GetCountWithAncestors() and it->GetCountWithDescendants() both include the current transaction, so in that case this should be decreased with 1?

That's true, -1 makes sense. Though note this is an approximation rather than an exact reservation. It may overestimate because transactions may share ancestors/descendants, and may underestimate because the cluster may include more than just ancestors and descendants.

That's true, but since this is just allocating enough space for the iterators we are adding to the vector later, I don't think having one slot more than we might need is going to cause any issues.

nit: I'm not sure it'd be a wortwhile improvement, but reserving before assigning could be a slight performance improvement so you only need to size the vector once instead of twice?

I do not understand this question. Could you elaborate?

We don't know the cluster size before, so this doesn’t allow us to optimize here

I think unprocessed_count is incorrect, shouldn't this be e.g. to_process_count?

        for (size_t i{0}, to_process_count{txids.size()}; i < to_process_count; ++i) {

Updated the name to your suggestion

I don't think this is correct?

Updated the comment to correct

This can be deduplicated. Also, I think curr is not really more helpful than cluster[i], so I'd just remove that varariable.

            auto family{cluster[i]->GetMemPoolParents()};
            family.merge(cluster[i]->GetMemPoolChildren());
            for (const CTxMemPoolEntry& entry : family) {
                const auto tx_iter = mapTx.iterator_to(entry);
                if (!visited(tx_iter)) {
                    cluster.push_back(tx_iter);
                    // we still need to process this
                    ++unprocessed_count;
                }
            }

Is this equivalent, given it's changing from GetMemPoolChildrenConst to GetMemPoolChildren and getting a mutable reference to m_parents?

TODO: Tried this, but it broke all sorts of things, gonna revisit later.

Mutable reference = the 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.

Gonna skip this for now, unless there is more demand for it

Would it be helpful to add a @pre indicating that all txids need to be in mempool?

Agree, this should be part of its documentation. I think it means that the caller must ensure that nothing can get removed from the mempool in between preparing the list of txids and calling this function, which seems important.

Added an @pre statement

As per #23962, perhaps better to make these int32_t?

Sure, fixed here and in other instances

    const CTransaction& GetTx() const LIFETIMEBOUND { return tx; }

Since this is a reference on an object held in mempool, and we have a lock on mempool, I think this is not necessary.

Should these be 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

If tx is const I think this should be too? And same for vsize_individual?

    const CAmount fee_individual;

This suggestion makes sense since these two individual fields shouldn't need to change (the ancestor fields do change). Making both const does compile.

Thanks, made the two individuals const

Since we're only using the ::iterator attribute, could shorten it a bit more to

    using MockEntryMapIter = decltype(entries_by_txid)::iterator;

I could not get this to work, could you elaborate what you propose?

Although MiniMiner sounds catchier, would MiniBlockAssembler be a more appropriate/accurate name?

Mh, I'll consider it, but it's also an easy change to make later.

naming consistency

    std::map<uint256, std::vector<COutPoint>> requested_outpoints_by_txid;

Thanks, I followed your suggestion

Perhaps I'll answer my own question as I progress with my review, but do we need 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.

Should all these class variable names have the m_ prefix?

I do not know from the top of my head, I will need to look more into this

Yes, yes, they should.

Renamed all MiniMiner class variables to be prefixed with m_

Removing requested_outpoints in favor of the keys of bump_fees breaks tests, so I assume they’re not overlapping exactly.

is that comment meant to be here? I can't see the relation to this else branch, and mapModifiedTx doesn't exist in the MiniMiner.

You are right, I could not place the comment either and removed it.

            bump_fees.emplace(outpoint, 0);

Thanks, I used your suggestion.

    return bump_fees;

Fixed

                outpoints_needed_by_txid.emplace(outpoint.hash, outpoints_of_tx);

Can make a similar change elsewhere in this file.

Thanks, I removed the unnecessary std::make_pair throughout

    for (const auto& [txid, outpoints] : outpoints_needed_by_txid) {
        txids_needed.push_back(txid);
    }

(nit, simpler)

Thanks, looks great

    //  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.)

Good idea, clarified the example

    // If multiple argument outpoints correspond to the same transaction, cache them together in
    // a single entry indexed by txid. Then we can just work with txids since all outpoints from
    // the same tx will have the same bumpfee. Excludes non-mempool transactions.

This is how I read the code, but check my understanding!

Adopted your phrasing of the comment

    //  same strategy as in CalculateBumpFees(…). Unlike the above call,

Sgtm, thanks

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.

This refers to transactions already bumped by other transactions in our mempool, not bumped by our calculation of bump fees. I've amended the phrase to clarify.

                for (const auto& outpoint : outpoints_it->second) {
                    bump_fees.emplace(outpoint, 0);
                }
                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).

Sounds right to me

        if (!remove) descendant_set_by_txid.emplace(std::make_pair(txid, std::move(cached_descendants)));

Good idea

suggest adding:

// This class must be constructed while holding mempool.cs. After construction, the object's
// methods can be called without holding that lock.

Or maybe a better place for this comment would be just before the constructor itself.

Added before the constructor

Could a better word than "Mock" be used to name this class? I initially thought this class had to do with testing. Since this is the MiniMiner, maybe MiniMempoolEntry?

Renamed to MiniMinerMempoolEntry, although I don't feel strongly about the name.

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:

    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.

Thanks for diving in this deeply, that sounds correct to me.

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.

6. 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).

I’m not sure I follow your train of thought here. I think you have since reviewed this further, do you think this still needs to be addressed?

Thinking more about this, I think the assumption is valid and I don't expect it to change soon. It is also documented here, so if it changes, people should notice the conflict. We will add a test for this.

Simplification, we just asserted that it's not entries.end()

        if (ancestor_package_fee < target_feerate.GetFee(ancestor_package_size)) {

Good catch, fixed

898ad9d5904f1b689d18d94f20d92500cf443758 The result of this 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?

Something more important: this 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.

@josibake

... 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!

Note to self: TODO

Yes, it would be more correct to gate the emplace_back() on !remove.

    // ancestor package with at least the target feerate and don't need to be bumped.

(if the feerates are equal, no bump is required)

Thanks, corrected

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

Ah good catch, that else branch is dead since the assert would hit.

Fixed

the way 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

Fixed

with the assert statements in 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

Fixed

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

Another thought I had while looking at this: we check if an 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.

what is the significance here? why not just have return a_feerate >= b_feerate or something like that?

I expect this to be used to make the sorting order stable without introducing a gameable tie-breaker

Added a comment

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.

                    if (!ancestors.count(parent_it)) {
                        to_process.insert(parent_it);
                        ancestors.insert(parent_it);
                    }

Great, thanks

Same change as above -- this is much more efficient with a large cluster, for example, running the benchmark.

                if (!ancestors.count(parent_it)) {
                    to_process.insert(parent_it);
                    ancestors.insert(parent_it);
                }

            // each entry's descendant set includes itself
            assert(it != descendant_set_by_txid.end());

(it took me a little while to figure out why this assertion is valid)

You may want to assert that these don't go negative, I would actually do it before the decrement, like this:

assert(descendant->second.vsize_with_ancestors >= anc->second.GetTxSize());
descendant->second.vsize_with_ancestors -= anc->second.GetTxSize();

Thanks, I’ve adopted your suggestion, although with an Assume instead.

simpler:

            auto it = descendant_set_by_txid.find(anc->first);

            in_block.insert(anc->first);

(functionally the same, just simpler)

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?

Still open

Staring more at this, I am starting to wonder whether this would produce the same waste score for all possible BnB solutions, since BnB calculates the waste score with different parameters at the end of SelectCoinsBnB(). It does pass the coin selection tests. (I’ll let the fuzzer run over night to see whether that finds something else.)

Did about 40M executions, and no issues! Gonna calculate the Waste score below here as discussed.

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.

It would be good to have a test for the application of ancestor bump fees with external pre-selected inputs.

Isn't test_preset_input_cpfp testing this?

In that test don't all of the pre-selected inputs belong to the wallet?

ah, yes, you're right

In 232edb7e5d630d9d687ba3089aa4028f8e0380a4 " Bump unconfirmed parent txs to target feerate "

Why was this added?

Ah thanks, that belongs to the commit one later, where we need the chain interface to amend the bump fees in case of overlapping ancestries.

in "Bump unconfirmed parent txs to target feerate" 8e3d924f02ee28043e8844a301389915823e5893

nit: suggestion

for (auto& output : result.coins.All()) {
    output.ApplyBumpFee(map_of_bump_fees.at(output.outpoint));
}

Thanks, went with your suggestion

This is incorrect, see #26152 (review)

Oops, sorry about misleading you 🙄

Ah drats, I did not realize that CoinsResult.All() produces a copy. Reverting this.

in "Amend bumpfee for inputs with overlapping ancestry" 56139fd808250fcb693b5f185e7e510804a33470

nit: you can drop this method if you decide to have GetTotalBumpFee() instead

I added a function 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.

My idea was to add bump_fee_group_discount within GetTotalBumpFee() function. Probably I'm missing something but I don't know what. Why wouldn't it just work?

I would prefer that the method 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.

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

Done

in "Amend bumpfee for inputs with overlapping ancestry" 56139fd808250fcb693b5f185e7e510804a33470

nit: you can use SelectionResult::GetTotalBumpFee() if you decide to introduce it

Added GetSummedBumpFees()

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

I tried this change, and remembered why I put in the "discount" in the first place: I think I have incorporated the bump fees in the effective values before, so I need to know whether there is a difference between the sum of the bump fees and the combined inputs’ bump fee. I have an idea how to incorporate your suggestion, but I gotta try tomorrow.

in "Bump unconfirmed parent txs to target feerate" 8e3d924f02ee28043e8844a301389915823e5893

nit: better use a multiple of target_fee_rate

Used 3×target_fee_rate instead

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

do we want to check naïve ancestry fee rate or the fee_rate at which it will be accepted at a block (i.e. ancestry score)?

Thanks, that check doesn’t make sense, I’ve removed it.

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.

Thanks for catching this. Fixed.

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.

Moved all comments about what the tests do to the implementations of the tests

In f2daebf8d1ac3829faa6443425cb97a763e9f780 "Bump unconfirmed parent txs to target feerate"

Compilation failure in this commit.

wallet/feebumper.cpp: In function ‘wallet::feebumper::Result wallet::CheckFeeRate(const CWallet&, const CMutableTransaction&, const CFeeRate&, int64_t, CAmount, std::vector<bilingual_str>&)’:
wallet/feebumper.cpp:89:61: error: ‘class interfaces::Chain’ has no member named ‘CalculateBumpFees’
   89 |     std::map<COutPoint, CAmount> bump_fees = wallet.chain().CalculateBumpFees(reused_inputs, newFeerate);
      |                                                             ^~~~~~~~~~~~~~~~~

This function was renamed to CalculateIndividualBumpFees

Oops, I only fixed the rename in the next commit. Fixing.

that seems like a duplicate check of assert_undershoots_target

Yeah, you’re right. Removed duplicate check.

nit: since you create new wallet for every test and never reuse any addresses do you really need to generate blocks? I tried to delete and the tests pass

That’s right, I’ll remove the unnecessary block generation

nit: here and other tests add self.assert_spends_only_parent

Added

nit: add self.assert_spends_only_parent

Here and other tests.I'm not sure the magic number is needed. I tried setting the multiplier to 1 and the tests still pass. Maybe just check that resulting fee rate is exactly equal to target fee rate?

Every once in a while we’ll get a signature that’s gonna be one byte smaller, so a little bit of a buffer will prevent intermittent failures. I reduced the buffers to 1%, though.

In 0f6c13665c4ab7d4928ff0fa63c4e755667f7fd6 "Amend bumpfee for inputs with overlapping ancestry"

nit: I'd prefer if variables were not renamed if they don't need to be, it makes review of this commit slightly harder and unnecessarily breaks git blame. new_total_fee is still a valid description of what this variable contains.

Okay, will be reverting to new_total_fee

In 0f6c13665c4ab7d4928ff0fa63c4e755667f7fd6 "Amend bumpfee for inputs with overlapping ancestry"

GetBumpFeeDiscount is only used here. It doesn't seem like it's useful to require the caller to calculate the final bumpfee rather than having SelectionResult just do that when the the discount is applied and have a single method to return the bumpfees required for a particular selection.

I’ve been doing some staring at this today. It seems to me that the waste score of finished input sets should also be adjusted according to the bump fee discount. Gotta do more pondering, will get back to this.

Yes, the discount value is a function of the effective fee rate, therefore it has to be taken into account in waste calculation. The problem is that GetSelectionWaste doesn't use SelectionResult.GetEffectiveValue() which already accounts for bump fee discoun

Okay, I replaced GetSummedBumpFees() with GetTotalBumpFees() which deducts the bump_fee_discount if it has been set.

In 0f6c13665c4ab7d4928ff0fa63c4e755667f7fd6 "Amend bumpfee for inputs with overlapping ancestry"

nit: this comment seems to be misplaced?