Bump unconfirmed ancestor transactions to target feerate #26152
Conversation
db2c0f7 to
54fd46b
Compare
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsReviewers, 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. |
54fd46b to
a07ac02
Compare
|
Thanks @fanquake, I fixed the two issues. I also added a test for a transaction using |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Concept ACK, thanks a lot for working on this, it will be very helpful!
And this set of internal utility functions will very likely be useful for many things in the future.
I'll create a set of E2E tests in eclair to run against this branch when I have a bit more time, I'll let you know the results.
|
I ran a first set of tests from within eclair against a07ac02, and everything is looking good 👍 |
a07ac02 to
a720b50
Compare
a720b50 to
337a24e
Compare
|
@glozow: Maybe for
Maybe we can add a @jonatack, @t-bast: |
There was a problem hiding this comment.
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
970c848 to
f9979ef
Compare
Todos:
|
|
@glozow: looking more into this, I realized that the |
Ah for some reason I thought it was a feerate, apologies. Question: is it better to only enforce |
I think that |
|
@S3RK noticed that I removed the obsolete method. |
|
ACK f18f9ef Great work! This PR not only makes our wallet work properly with unconfirmed inputs, but it also improved the factoring of the code. Happy to see waste calculations as a member function of |
|
@t-bast, @ismaelsadeeq, @achow101: Changes since 1b2fb54d5c18579e013b1dfd061c2b29d8cffdc2
|
|
ACK f18f9ef |
|
ACK f18f9ef |
bd34dd8 Use `exact_target` shorthand in coinselector_tests (Murch) 7aa7e30 Fold GetSelectionWaste() into ComputeAndSetWaste() (Murch) Pull request description: PR #26152 moved waste calculation into SelectionResult to be able to correct the waste score on basis of the bump_fee_group_discount for overlapping ancestries. This left two functions with largely overlapping purpose, where one was simply a wrapper of the other. This PR cleans up the overlap, and fixes the double-meaning of `change_cost` where the `GetChange()` function assumed that no change was created when `change_cost` was set to 0. This behavior was exploited in a bunch of tests, but is problematic, because a `change_cost` of 0 is permitted with custom settings for feerate and discard_feerate (i.e. when they’re both 0). ACKs for top commit: achow101: ACK bd34dd8 furszy: Code ACK bd34dd8 ismaelsadeeq: Code Review ACK bd34dd8 Tree-SHA512: 83a2688d45d719dc61a64b5180fe136107faccf401a59df65245c05d701748a03e85ed56fde8c9b7ef39a3ab54374dd3718c559bda5b3f55dafedfd7fed25161
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.
Fixes #9645
Fixes #9864
Fixes #15553