Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RBF API is awkward (remove allow_shrinking) #1374

Closed
LLFourn opened this issue Mar 15, 2024 · 3 comments · Fixed by #1386
Closed

RBF API is awkward (remove allow_shrinking) #1374

LLFourn opened this issue Mar 15, 2024 · 3 comments · Fixed by #1386
Labels
api A breaking API change bug Something isn't working module-wallet
Milestone

Comments

@LLFourn
Copy link
Contributor

LLFourn commented Mar 15, 2024

A fee bump shouldn't try to interpret the existing transaction and replicate it -- it should just create a tx with the appropriate fee that spends one or more of the existing transaction's outputs.

The builder API should just be tx_builder.replace(tx). You could call it several times to replace several transactions.

Originally posted by @LLFourn in #1342 (comment)

NB I think our RBF code is probably wrong but that's another issue. I've done a lot of work in the coin selection crate to get this correct. Getting the API right here and removing footgun allow_shrinking is what's important.

@LLFourn LLFourn changed the title RBF API is awkward RBF API is awkward (remove allow_shrinking) Mar 15, 2024
@notmandatory notmandatory added this to the 1.0.0-alpha milestone Mar 15, 2024
@notmandatory
Copy link
Member

notmandatory commented Mar 15, 2024

If we remove allow_shrinking() does this mean a user would manually have to reduce the amount of an output if they don't have enough inputs to pay the fee? I'd be okay with that as long as we make it easy for the user to know how much fee they're short. Our InsufficientFunds error already let's users know how much they are short (eg. InsufficientFunds { needed: 50130, available: 50000 }). Should we make it easier to shrink the amount to a particular address? or we can leave that until we get some feed back on if users need it.

@notmandatory notmandatory added module-wallet api A breaking API change labels Mar 15, 2024
@notmandatory
Copy link
Member

What do you mean by "it should just create a tx with the appropriate fee that spends one or more of the existing transaction's outputs"? I assume you mean one or more of the existing tx inputs, but then how does the user tell us which ones? Do we add a parameter with range of input indexes? And then in the TxParams we'd need to keep track of the original transactions fee rate and total absolute fees to enforce the rule that those are higher also right?

How about for bdk_wallet 1.0 and pre-1.0 we just remove allow_shrinking() and keep the current build_fee_bump() builder behavior of keeping inputs and outputs the same as the original tx? Then we can tackle this issue for adding a new and improved build fee bump type function in a 1.1 release and where we'd keep but deprecate the current build_fee_bump()?

@LLFourn
Copy link
Contributor Author

LLFourn commented Mar 17, 2024

What do you mean by "it should just create a tx with the appropriate fee that spends one or more of the existing transaction's outputs"? I assume you mean one or more of the existing tx inputs,

Yes.

but then how does the user tell us which ones?

We don't allow them. Just choose one. In the future even run coin selection with multiple times to figure out which is optimal to use to replace.

And then in the TxParams we'd need to keep track of the original transactions fee rate and total absolute fees to enforce the rule that those are higher also right?

yes

Then we can tackle this issue for adding a new and improved build fee bump type function in a 1.1 release and where we'd keep but deprecate the current build_fee_bump()

I think this might work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api A breaking API change bug Something isn't working module-wallet
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

2 participants