Bump unconfirmed ancestor transactions to target feerate by murchandamus · Pull Request #26152 · bitcoin/bitcoin

murchandamus · 2022-09-21T20:52:28Z

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

DrahtBot · 2022-09-21T23:24:48Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

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.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#28391 (refactor: Simplify CTxMempool/BlockAssembler fields, remove some external mapTx access by TheCharlatan)
#28335 (RFC: Remove boost usage from kernel api / headers by TheCharlatan)
#27865 (wallet: Track no-longer-spendable TXOs separately by achow101)
#27601 (wallet: don't duplicate change output if already exist by furszy)
#27286 (wallet: Keep track of the wallet's own transaction outputs in memory by achow101)

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.

fanquake

Concept ACK

src/node/mini_miner.cpp

test/functional/wallet_spend_unconfirmed.py

murchandamus · 2022-09-22T19:56:21Z

Thanks @fanquake, I fixed the two issues.

I also added a test for a transaction using subtractfeefromamount

glozow

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

src/txmempool.cpp

jonatack

Concept ACK

src/node/mini_miner.h

t-bast

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.

src/interfaces/chain.h

t-bast · 2022-09-23T15:49:00Z

I ran a first set of tests from within eclair against a07ac02, and everything is looking good 👍

ishaanam

Concept ACK

murchandamus · 2022-09-27T19:19:18Z

@jonatack, @t-bast: Thanks for the review and testing. I made an attempt of getting rid of the getters on MockMempoolEntry, but what I did interfered with the calls made on properties of actual mempool entries. Will have to shift my approach.

murchandamus · 2022-09-27T20:25:04Z

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

glozow

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

src/node/mini_miner.h

src/txmempool.cpp

src/wallet/coinselection.h

test/functional/wallet_spend_unconfirmed.py

murchandamus · 2022-10-27T19:08:57Z

Reordered members vs initialization
Removed special casing of UTXOs without relatives in CalculateBumpFee
Call CalculateBumpFee once for the whole UTXO pool instead of introducing chain-interface dependency on every UTXO

Todos:

Prevent exceeding maxtxfee
Add test for bumpfee RPC
Add secondary modus for CalculateBumpFee that treats the provided UTXOs as being spent together. This allows us to recalculate the bumpfee of all inputs together to resolve our overpayment caveat.

murchandamus · 2022-11-02T16:19:58Z

@glozow: looking more into this, I realized that the 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.

glozow · 2022-11-02T17:42:09Z

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?

achow101 · 2022-11-02T18:38:45Z

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?

I think that -maxtxfee should be expected to behave the same regardless of bumping, it's a context-free check.

murchandamus · 2023-08-30T15:03:40Z

Rebased, and fixed comment to mollify tidy. (Ready for review.)

t-bast

ACK 1b2fb54, I ran my e2e lightning tests on top of that branch and everything is looking good 👍

ismaelsadeeq · 2023-08-31T13:55:59Z

re ACK 1b2fb54d5c18579e013b1dfd061c2b29d8cffdc2

Reviewed up to acb6aaeac49dd4543280653f19d2377ffb175c9e and functional test
As I am not really familiar with the coinselection part of the PR, I lightly review the commits.
I rebased latest master a4e0bcb on this PR, the tests failed and passed after making the branch.

achow101 · 2023-09-06T21:52:20Z

ACK 1b2fb54d5c18579e013b1dfd061c2b29d8cffdc2

src/wallet/spend.cpp

S3RK

Started re-review. Besides few nits, I have a doubt about waste calculation with bump fee and excess. Could you take a look, please?

src/wallet/test/coinselector_tests.cpp

src/wallet/coinselection.cpp

src/wallet/spend.cpp

src/wallet/coinselection.cpp

murchandamus

Addressed @S3RK’s review

src/wallet/spend.cpp

src/wallet/coinselection.cpp

src/wallet/test/coinselector_tests.cpp

Follow-up from bitcoin#27021: In the prior commit, the vector started counting at 0, but the transaction names started with 1. This commit matches the names to the transactions’ vector indices for better readability. Co-authored-by: theStack <[email protected]>

Follow-up from bitcoin#27021. Also included is an ASCII art visualization of the test’s transaction topology by theStack. Co-authored-by: theStack <[email protected]>

Follow-up from bitcoin#27021

Follow-up from bitcoin#27021: accessing of fields in MiniMinerMempoolEntry was done inconsistently. Even though we had a getter, we would directly write to the fields when we needed to update them. This commits sets the fields to private and introduces a method for updating the ancestor information in transactions using the same method name as used for Mempool Entries.

GetSelectionWaste will need to access more context within a selection result, and so should be a private member function rather than a static function. It's only use outside of SelectionResult was for tests which have now been updated to just make a SelectionResult. Co-authored-by: Murch <[email protected]>

When a transaction uses an unconfirmed input, preceding this commit it would not consider the feerate of the parent transaction. Given a parent transaction with a lower ancestor feerate, this resulted in the new transaction's ancestor feerate undershooting the target feerate. This commit changes how we calculate the effective value of unconfirmed UTXOs. The effective value of unconfirmed UTXOs is decreased by the fee necessary to bump its ancestry to the target feerate. This also impacts the calculation of the waste metric: since the estimate for the current fee is increased by the bump fees, unconfirmed UTXOs current fees appear less favorable compared to their unchanged long term fees. This has one caveat: if multiple UTXOs have overlapping ancestries, each of their individual estimates will account for bumping all ancestors.

murchandamus

I pulled the part of the waste test that dealt with bump fees without group discount into the earlier commit and fixed an issue in the waste calculation in that commit. The final code is unchanged.

src/wallet/coinselection.cpp

murchandamus · 2023-09-13T18:54:03Z

I had forgotten to update the AttemptSelection call in src/bench/coin_selection.cpp to the new function header that takes the chain instead of the wallet.

I should have now addressed all feedback from @brunoerg and @S3RK, and be ready for review again.

At the end of coin selection reduce the fees by the difference between the individual bump fee estimates and the collective bump fee estimate.

murchandamus · 2023-09-13T19:51:33Z

@S3RK noticed that SelectionResult::GetBumpFeeDiscount() was now obsolete and no longer used. (AFAIR, this would have been a bi-product of moving GetSelectionWaste into SelectionResult.)

I removed the obsolete method.

S3RK · 2023-09-13T19:54:04Z

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 SelectionResult

murchandamus · 2023-09-13T20:27:08Z

@t-bast, @ismaelsadeeq, @achow101: Changes since 1b2fb54d5c18579e013b1dfd061c2b29d8cffdc2

Pass chain interface to AttemptSelection(…) instead of whole wallet
Remove obsolete method SelectionResult::GetBumpFeeDiscount()
Add waste calculation test for changeless transactions with bump fees
Fix typo

brunoerg

crACK f18f9ef

code lgtm, nice improvement!

ismaelsadeeq · 2023-09-14T11:49:05Z

ACK f18f9ef

t-bast

ACK f18f9ef, I reviewed the latest changes and run e2e tests against eclair, everything looks good 👍

achow101 · 2023-09-14T20:00:42Z

ACK f18f9ef

murchandamus force-pushed the 2022-09-ancestor-aware-funding branch from db2c0f7 to 54fd46b Compare September 21, 2022 21:16

fanquake reviewed Sep 22, 2022

View reviewed changes

src/node/mini_miner.cpp Outdated Show resolved Hide resolved

test/functional/wallet_spend_unconfirmed.py Outdated Show resolved Hide resolved

fanquake requested review from instagibbs and t-bast September 22, 2022 08:34

This was referenced Sep 22, 2022

wallet: Faster transaction creation by removing pre-set-inputs fetching responsibility from Coin Selection #25685

Merged

wallet: Pass through transaction locktime and preset input sequences and scripts to CreateTransaction #25273

Merged

murchandamus force-pushed the 2022-09-ancestor-aware-funding branch from 54fd46b to a07ac02 Compare September 22, 2022 19:55

glozow reviewed Sep 22, 2022

View reviewed changes

DrahtBot mentioned this pull request Sep 22, 2022

wallet: BIP 326 sequence based anti-fee-snipe for taproot inputs #24128

Closed

glozow reviewed Sep 23, 2022

View reviewed changes

src/txmempool.cpp Outdated Show resolved Hide resolved

jonatack reviewed Sep 23, 2022

View reviewed changes

src/txmempool.cpp Outdated Show resolved Hide resolved

jonatack reviewed Sep 23, 2022

View reviewed changes

src/node/mini_miner.h Outdated Show resolved Hide resolved

t-bast reviewed Sep 23, 2022

View reviewed changes

src/interfaces/chain.h Outdated Show resolved Hide resolved

src/interfaces/chain.h Outdated Show resolved Hide resolved

glozow added the Wallet label Sep 23, 2022

ishaanam reviewed Sep 27, 2022

View reviewed changes

murchandamus force-pushed the 2022-09-ancestor-aware-funding branch from a07ac02 to a720b50 Compare September 27, 2022 19:15

murchandamus force-pushed the 2022-09-ancestor-aware-funding branch from a720b50 to 337a24e Compare September 27, 2022 20:25

glozow reviewed Sep 28, 2022

View reviewed changes

murchandamus force-pushed the 2022-09-ancestor-aware-funding branch from 970c848 to f9979ef Compare October 27, 2022 18:53

DrahtBot added the Needs rebase label Oct 27, 2022

murchandamus requested a review from S3RK August 29, 2023 16:28

murchandamus mentioned this pull request Aug 29, 2023

Fix waste calculation in SelectionResult #28366

Merged

t-bast approved these changes Aug 31, 2023

View reviewed changes

DrahtBot mentioned this pull request Sep 2, 2023

refactor: Simplify CTxMempool/BlockAssembler fields, remove some external mapTx access #28391

Merged

brunoerg reviewed Sep 8, 2023

View reviewed changes

src/wallet/spend.cpp Outdated Show resolved Hide resolved

brunoerg reviewed Sep 8, 2023

View reviewed changes

src/wallet/spend.cpp Outdated Show resolved Hide resolved

S3RK reviewed Sep 13, 2023

View reviewed changes

src/wallet/test/coinselector_tests.cpp Outdated Show resolved Hide resolved

src/wallet/coinselection.cpp Outdated Show resolved Hide resolved

src/wallet/spend.cpp Outdated Show resolved Hide resolved

src/wallet/coinselection.cpp Outdated Show resolved Hide resolved

murchandamus commented Sep 13, 2023

View reviewed changes

src/wallet/spend.cpp Outdated Show resolved Hide resolved

src/wallet/coinselection.cpp Outdated Show resolved Hide resolved

src/wallet/test/coinselector_tests.cpp Outdated Show resolved Hide resolved

murchandamus and others added 7 commits September 13, 2023 14:33

Fix calculation of ancestor set feerates in test

d2f90c3

Follow-up from bitcoin#27021. Also included is an ASCII art visualization of the test’s transaction topology by theStack. Co-authored-by: theStack <[email protected]>

Remove unused imports

ac6030e

Follow-up from bitcoin#27021

[node] interface to get bump fees

c57889d

murchandamus commented Sep 13, 2023

View reviewed changes

src/wallet/coinselection.cpp Outdated Show resolved Hide resolved

Amend bumpfee for inputs with overlapping ancestry

f18f9ef

At the end of coin selection reduce the fees by the difference between the individual bump fee estimates and the collective bump fee estimate.

brunoerg approved these changes Sep 13, 2023

View reviewed changes

t-bast approved these changes Sep 14, 2023

View reviewed changes

mzumsande mentioned this pull request Sep 26, 2023

test: Intermittent issue in wallet_resendwallettransactions "Unable to create transaction. Fee exceeds maximum configured by user (e.g. -maxtxfee, maxfeerate) (-4)" #28491

Closed

1 task

glozow mentioned this pull request Oct 13, 2023

validate package transactions with their in-package ancestor sets #26711

Closed

Conversation

murchandamus commented Sep 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Sep 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews

Conflicts

Uh oh!

fanquake left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

murchandamus commented Sep 22, 2022

Uh oh!

glozow left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

t-bast left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

t-bast commented Sep 23, 2022

Uh oh!

ishaanam left a comment

Choose a reason for hiding this comment

Uh oh!

murchandamus commented Sep 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

murchandamus commented Sep 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

glozow left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

murchandamus commented Oct 27, 2022

Uh oh!

murchandamus commented Nov 2, 2022

Uh oh!

glozow commented Nov 2, 2022

Uh oh!

achow101 commented Nov 2, 2022

Uh oh!

murchandamus commented Aug 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

t-bast left a comment

Choose a reason for hiding this comment

Uh oh!

ismaelsadeeq commented Aug 31, 2023

Uh oh!

achow101 commented Sep 6, 2023

Uh oh!

Uh oh!

Uh oh!

S3RK left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

murchandamus left a comment

Choose a reason for hiding this comment

murchandamus commented Sep 21, 2022 •

edited

Loading

DrahtBot commented Sep 21, 2022 •

edited

Loading

glozow left a comment •

edited

Loading

murchandamus commented Sep 27, 2022 •

edited

Loading

murchandamus commented Sep 27, 2022 •

edited

Loading

glozow left a comment •

edited

Loading

murchandamus commented Aug 30, 2023 •

edited

Loading

murchandamus commented Sep 13, 2023 •

edited

Loading