RBF API is awkward (remove allow_shrinking)

[LLFourn]

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 https://github.com/bitcoindevkit/bdk/issues/1342#issuecomment-1940509301

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.

[notmandatory]

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]

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]

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.