Bump unconfirmed ancestor transactions to target feerate (wallet)

https://github.com/bitcoin/bitcoin/pull/26152

Host: glozow  -  PR author: Xekyo

The PR branch HEAD was 898ad9d590 at the time of this review club meeting.

Notes

See notes from last weekā€™s meeting.

Questions

  1. The MiniMiner constructor accepts a mempool reference and a list of outpoints the wallet might be interested in spending. Given an outpoint, it may be one of four possible types:

    • a confirmed UTXO
    • a UTXO created by a mempool transaction and has not been spent yet
    • a UTXO created by a mempool transaction and has already been spent by another mempool transaction
    • a UTXO that does not exist in mempool or chainstate (perhaps not yet submitted to mempool)

    How does the MiniMiner constructor detect and handle each case?

  2. MiniMiner builds a descendant_set_by_txid cache, but not an ancestor_set_by_txid cache. Instead, it calculates ancestor sets on the fly. Do you think this approach makes sense? Why or why not?

  3. One potential approach for constructing the block is to define a custom ancestor score comparator for MockMempoolEntry (or even just reuse CompareTxMemPoolEntryByAncestorFee from txmempool like the BlockAssembler does), and then iterate through a list of entries sorted by ancestor score. Why would this approach work for BlockAssembler but not for MiniMiner?

  4. This functionality is only used by the wallet. Instead of adding CalculateBumpFees to the chain interface, should we just add it as a utility function in the wallet?

  5. Describe the approach taken in the ā€œBump unconfirmed parent txs to target feerateā€ commit.

  6. What test cases are included in wallet_spend_unconfirmed.py added in the same commit? Can you think of any other test cases to add?

  7. Two coin selection results may require different fees for bumping ancestors. How does the wallet choose which one to use? (Hint: can you identify how bump fees come into play in GetSelectionWaste())?

  8. How does the PR handle spending unconfirmed UTXOs with overlapping ancestry? (Hint: what does the code here do)?

Meeting Log

  117:00 <glozow> #startmeeting
  217:00 <d33r_gee> hello
  317:00 <rozehnal_paul> hi
  417:00 <glozow> Hi, this is PR review club! Anyone else here?
  517:00 <effexzi> Hi every1
  617:01 <brunoerg> hi
  717:01 <glozow> Today is part 2 of #26152: https://bitcoincore.reviews/26152-2
  817:01 <LarryRuane> hi
  917:01 <glozow> We looked at concept and approach last week. This week, we'll go a bit deeper into the implementation. If you weren't here last week, that's totally fine, notes and logs are here: https://bitcoincore.reviews/26152
 1017:02 <glozow> Have people had a chance to review the PR and/or the notes?
 1117:02 <d33r_gee> yes
 1217:02 <rozehnal_paul> very little tbh
 1317:03 <glozow> Would someone like to summarize what this PR does?
 1417:05 <glozow> ping?
 1517:06 <Murch> Hi
 1617:06 <rozehnal_paul> technical issues?
 1717:06 <d33r_gee> this PR would cluster UTXOs to then calculate the necessary bump fee (?)
 1817:06 <LarryRuane> when the wallet generates a list of spendable outputs to hand to coin selection, it reduces the effective values of mempool transaction outputs, if needed to bring these transactions effective fee rates up to the desired fee rate
 1917:08 <glozow> LarryRuane: yep!
 2017:08 <glozow> le'ts move on to the questions
 2117:09 <glozow> The MiniMiner constructor accepts a mempool reference and a list of outpoints the wallet might be interested in spending. Given an outpoint, what are the 4 possible states?
 2217:10 <LarryRuane> it could be a confirmed UTXO, or unconfirmed (in the mempool), or an outpoint that is already being spent by an existing transaction in the mempool, or an outpoint that we've never heard of
 2317:10 <murchandamus> I couldn't see anyone on matrix
 2417:10 <ishaanam[m]> Hi
 2517:10 <glozow> LarryRuane: bingo
 2617:10 <murchandamus> I think there is a network split or smth
 2717:11 <ishaanam[m]> Is there a meeting today?
 2817:11 <glozow> murchandamus: yeah I wonder if there's a connection issue
 2917:11 <Murch> I think so, but I don't see the chat from IRC
 3017:11 <glozow> ishaanam[m]: yes, we've started but perhaps there is a connection issue and messages aren't going through
 3117:11 <LarryRuane> seems to be okay for me
 3217:12 <sipa> I think we may have a netsplit/delay from the matrix bridge.
 3317:12 <Murch> Yeah, Gloria says she only sees a reduced attendance on her side too
 3417:12 <murchandamus> That message from me on Matrix was ~four multiple minutes ago ^^
 3517:12 <murchandamus> Anyway, it seems like it's mending
 3617:12 <murchandamus> Carry on, please
 3717:13 <glozow> How does the MiniMiner constructor detect and handle each of the 4 cases?
 3817:14 <LarryRuane> a confirmed UTXO is easy, we don't need to "bump its fee" (really bumping our own fee to provide miner incentive to include us and all of our unconfirmed ancestors)
 3917:16 <LarryRuane> an unconfirmed UTXO, if the transaction providing it has a lower feerate than us, then we want to reduce its effective value (aka "bump" its effective feerate) -- this is the main effect of this PR i would say
 4017:17 <LarryRuane> (reduce its effective value as seen by coin selection)
 4117:17 <glozow> LarryRuane: yes, but how does the ctor detect whether a UTXO is confirmed? it doesn't have access to the chainstate, only the mempool!
 4217:17 <glozow> hint: code is here https://github.com/bitcoin-core-review-club/bitcoin/blob/b669fd94f84e679d4549ef0abe1b0483e1406152/src/node/mini_miner.cpp#L26-L49
 4317:18 <Murch> I'm using the web client that's linked on the website now
 4417:18 <LarryRuane> i think if `mempool.exists()` returns false, it's not in the mempool, so it may be confirmed
 4517:19 <glozow> LarryRuane: right
 4617:20 <LarryRuane> in other words, it doesn't ask the question "is this UTXO in the chainstate (coins db)", it asks if it's in the mempool
 4717:20 <glozow> yes exactly
 4817:20 <glozow> What if it's a confirmed UTXO but spent by something in the mempool?
 4917:21 <murchandamus> Well, our wallet should not do that unprompted, so we must be dealing with a replacement attempt
 5017:22 <LarryRuane> then `mempool.GetConflictTx()` will return a handle to that transaction to us (and note there can be at most one!)
 5117:22 <glozow> yup
 5217:22 <murchandamus> LarryRuane: exactly
 5317:22 <LarryRuane> (we would never allow 2 transactions *into the mempool* that both spend the same output)
 5417:23 <glozow> So I guess I should have said 5 states, confirmed and not spent + confirmed and spent by something in the mempool
 5517:23 <glozow> next question. MiniMiner builds a descendant_set_by_txid cache, but not an ancestor_set_by_txid cache. Instead, it calculates ancestor sets on the fly. Do you think this approach makes sense? Why or why not?
 5617:24 <glozow> descendant cache is built here: https://github.com/bitcoin-core-review-club/bitcoin/blob/b669fd94f84e679d4549ef0abe1b0483e1406152/src/node/mini_miner.cpp#L72-L96
 5717:24 <glozow> and ancestors calculated here: https://github.com/bitcoin-core-review-club/bitcoin/blob/b669fd94f84e679d4549ef0abe1b0483e1406152/src/node/mini_miner.cpp#L145-L161
 5817:26 <LarryRuane> Makes sense because ancestor sets can reduce as we add transactions to the block template (those no longer need to be fee-bumped, we sort of pretend they're already mined, no longer our ancestors) -- unsure about this
 5917:26 <LarryRuane> well they ARE our ancestors, but we can ignore them because they're already being "mined"
 6017:27 <glozow> LarryRuane: that's exactly right. our view of ancestors to consider changes while the block is being built
 6117:27 <murchandamus> Descendants can never be included before the transaction itself is included, so they're stable throughout th eblock building until the transaction itself is included
 6217:28 <murchandamus> Although of course, descendants too could change by replacements
 6317:28 <LarryRuane> the code dealing with descendants -- is it only necessary because of replacement?
 6417:29 <LarryRuane> as in, if replacement was never possible, we wouldn't need that code?
 6517:30 <glozow> no, we always need the descendants when building the block template. when a transaction is "added" to the block, we need to update its descendants' ancestor sets
 6617:30 <LarryRuane> oh right, i see, thanks
 6717:31 <LarryRuane> now i understand murch's comment above
 6817:32 <glozow> the next question has a similar theme. One potential approach for constructing the block is to define a custom ancestor score comparator for MockMempoolEntry (or even just reuse CompareTxMemPoolEntryByAncestorFee from txmempool like the BlockAssembler does), and then iterate through a list of entries sorted by ancestor score. Why would this approach work for BlockAssembler but not for MiniMiner?
 6917:33 <glozow> hint: what data structure is modified when BlockAssembler updates an entry for its ancestors getting mined? https://github.com/bitcoin/bitcoin/blob/aeb395dcdbfe2b1a6c77ff218939a18afde3add9/src/node/miner.cpp#L244
 7017:34 <LarryRuane> I'm sorry I didn't get this far, I ended up rabbit-holing mostly in the constructor
 7117:34 <glozow> and what data structure is modified when MiniMiner is doing the equivalent thing? https://github.com/bitcoin-core-review-club/bitcoin/blob/b669fd94f84e679d4549ef0abe1b0483e1406152/src/node/mini_miner.cpp#L168-L179
 7217:38 <murchandamus> Well, BlockAssembler works with a copy of the mempool, but we have just as a set of entries
 7317:38 <glozow> No need to apologize! We are updating the cached ancestor information for each descendant of a transaction "included in the block." In `BlockAssembler`, we do so by updating `mapModifiedTx` and not by writing to `mapTx` itself (so we can iterate in ancestor score order without worrying about it changing). In MiniMiner, we're not working with mapTx but a map of `MockMempoolEntry`s, which we modify directly.
 7417:39 <glozow> So actually, the answer is the same as the answer to the last question. We can't just iterate in ancestor feerate order, because the ancestors change as we're going.
 7517:41 <murchandamus> So, how is that different between MiniMiner and BlockAssembler, though?
 7617:41 <LarryRuane> I noticed that we access the real mempool only in the constructor -- we build our own "private" data structures (the MiniMiner class) from the real mempool. Is this for performance reasons mostly? We don't want to hold the real mempool lock for that long?
 7717:41 <murchandamus> Wouldn't either be affected by ancestors being picked in the block?
 7817:42 <glozow> murchandamus: the difference is BlockAssembler doesn't modify the data structure it's iterating through
 7917:43 <glozow> maybe poor connection again? In `BlockAssembler`, we do so by updating `mapModifiedTx` and not by writing to `mapTx` itself (so we can iterate in ancestor score order without worrying about it changing). In MiniMiner, we're not working with mapTx but a map of `MockMempoolEntry`s, which we modify directly.
 8017:44 <murchandamus> Thanks for the explanation
 8117:45 <glozow> LarryRuane: great observation! Yes, we only grab the lock for that short period of time to make copies of the entries we need
 8217:45 <LarryRuane> that's one of the most contentious locks, i would guess?
 8317:45 <LarryRuane> it's very cool how that works, once i understood it
 8417:46 <glozow> One could say it's not good to let go of the lock since there's a possibility the mempool contents can change while we're constructing the transaction, but haven't yet heard whether that's really problematic
 8517:47 <LarryRuane> let's say (unlikely) .... yes i was just going there ... suppose a new block arrives while we're constructing this tx ... is there some way to start over again?
 8617:47 <glozow> The argument *for* not holding the lock is yeah, theoretically the node should be able to continue doing it's thing while the wallet spends time calculating its bumpfees and whatnot
 8717:47 <LarryRuane> well also (i think this maybe what you were getting at), new transactions could arrive (they would only always be new descendants obviously)
 8817:49 <LarryRuane> "The argument *for* not holding the lock..." I like that design decision .. there will always be the possibility that things change *just after* constructing the tx anyway
 8917:49 <murchandamus> LarryRuane: It could also be new descendants that replace old descendants though
 9017:49 <glozow> LarryRuane: I'm not exactly sure what happens in that case. My imagination is that the tx creation fails and the user needs to manually try again. It's also possible that cs_main is held the whole time so you can't accept a block while a tx is being created haha
 9117:50 <glozow> Not sure, will need to look at the code
 9217:51 <LarryRuane> oh do we hold cs_main the whole time? That seems like an even bigger lock contention than holding the mempool lock (but i'm not sure)
 9317:51 <glozow> LarryRuane: nono, that was just a joke
 9417:52 <LarryRuane> (i missed the "haha" haha)
 9517:52 <LarryRuane> to restate my point more clearly ... no matter what you do, any tx you construct could immediately be non-optimal a millisecond later
 9617:53 <LarryRuane> *become
 9717:53 <glozow> yeah
 9817:54 <glozow> Let's get to some wallet questions...
 9917:54 <glozow>
10017:54 <glozow> Describe the approach taken in the ā€œBump unconfirmed parent txs to target feerateā€ commit : https://github.com/bitcoin/bitcoin/pull/26152/commits/ad8bffe548a2536f925e6911c7d50c1aaab1a59e
10117:55 <LarryRuane> this commit is the main effect of this PR, isn't it?
10217:56 <glozow> yup
10317:56 <glozow> it's the big behavior change commit
10417:56 <murchandamus> Well, all the hard work has now been done by glozow
10517:57 <glozow> heheh, šŸ˜Š
10617:57 <murchandamus> So, what's happening here is that we take the information we've learned about our UTXOs and change their effective values to reflect the amount of fees that go towards bumping their ancestry to the target feerate
10717:57 <murchandamus> And after that we can run coin selection just as before
10817:57 <murchandamus> there is one exception:
10917:58 <murchandamus> If we have some UTXOs that have shared unconfirmed ancestry, each of the UTXOs would bump the whole ancestry again
11017:59 <LarryRuane> yeah so the new tx's fee would be too high then, right?
11117:59 <glozow> murchandamus: thank you for the great explanation! I'll follow up with the last question: How does the PR handle spending unconfirmed UTXOs with overlapping ancestry?
11217:59 <murchandamus> Only if we happen to select two UTXOs that do share ancestry
11317:59 <murchandamus> But yes, it can only be too high, so at least we'll always exceed the target feerate
11417:59 <LarryRuane> oh right .. and that's why it's tricky because you don't know which of these UTXOs (that have the common ancestry) coin selection will choose!
11518:00 <murchandamus> it could however be quite substantial if the ancestry is very heavy
11618:00 <murchandamus> LarryRuane: Exactly!
11718:00 <LarryRuane> that's why you can't just bump one or the other (which was my first thought)
11818:00 <murchandamus> So, we have a post-selection step now, where we check what the total bump fees for a given _set_ of UTXOs would be in sum now
11918:01 <murchandamus> So after we do coin selection, we can check whether we have to give some of the fees back to the change output
12018:01 <LarryRuane> this given set is what coin selection decided on?
12118:01 <murchandamus> Of course that won't work when we created a changeless transaction as easily
12218:01 <murchandamus> in that case, we might drop it to the fees, or need to create a change after all
12318:01 <LarryRuane> so what do we do in the changeless case (sorry i haven't looked at those commits)
12418:02 <murchandamus> either way, such a solution might have a worse waste score, so hopefully wouldn't get picked up anyway
12518:02 <LarryRuane> hey could you explain waste score briefly? i didn't understand tha tpart
12618:02 <LarryRuane> sorry may be too late
12718:02 <murchandamus> LarryRuane: We run multiple different selection attempts in parallel now: via the different UTXO types, and via the different algorithms
12818:02 <murchandamus> hopefully only some of them would be affected and we pick per the best wastescore anyway
12918:02 <LarryRuane> i'm sure there's a stackexchange question on it!
13018:03 <glozow> oo yes we're past the end time, but feel free to keep chatting ofc
13118:03 <murchandamus> LarryRuane: I wrote something for you: https://murch.one/posts/waste-metric/
13218:03 <glozow> #endmeeting