feat(wallet): add TxBuilder::replace_tx

[ValuedMammal]

This PR adds a new method TxBuilder::replace_tx that offers a simple way of creating replacements without relying on the mechanics of build_fee_bump. See #1374 for context and motivating discussion.

In summary, there are a few limitations with the current build_fee_bump:

1. It doesn't prevent the wallet from accidentally selecting the outputs of the replaced tx as inputs to the replacement
2. The UX is somewhat rigid because it assumes all of the same inputs and outputs should appear in the replacement. (however despite this second point we somehow neglect to reuse the original drain script?)

In contrast replace_tx is useful when we only need to create a conflict and flexible enough to further tweak the parameters as desired.

Notes to the reviewers

• TxBuilder::add_utxos is modified to look for potentially spent outputs, provided that the given outpoint exists in the replaceable set.

Changelog notice

• Added method TxBuilder::replace_tx
• Added method TxBuilder::previous_fee

Checklists

All Submissions:

• I've signed all my commits
• I followed the contribution guidelines
• I ran cargo fmt and cargo clippy before committing

New Features:

• I've added tests for the new feature
• I've added docs for the new feature

[evanlinjin]

ConceptACK, ApproachACK.

Just need to make sure we don't use descendant outputs from tx we are trying to replace.

It's out of the scope of this PR, the inability to set both feerate/fee amount constraints together really bothers me. We should probably switch to the bdk_coin_select crate soon.

[ValuedMammal]

I made a few small changes including a test for unspendable descendants. I'm still open to discussing the right UX for replace_tx.

[notmandatory]

Since still some open ux design questions I removed this from the 1.1 milestone.

[ValuedMammal]

Since replace_tx doesn't currently let you to choose which input to replace, another idea could be to have replace_tx internally call a new method replace_tx_input with an extra argument input_index: Option<usize>.

[notmandatory]

utACK dae5d73

My 2 sats on adding replace_tx_input is it should wait until if/when someone has a need for it.

[tvpeter]

tACK dae5d73

nit: When the discussion is finalized, it would be great to consider adding a test case for replace_tx in the tests/wallet, as it is an important wallet feature.

[oleonardolima]

utACK dae5d73

[notmandatory]

@evanlinjin your above comments were addressed ok to resolve those comments so we can merge this one in 1.2?

[evanlinjin]

Looks pretty good to me on a first glance. I'll double check over it tomorrow to make sure and I'll ACK/merge.

[evanlinjin]

I guess we changed this so that we can call it in replace_tx without having it return an error?

However, when we do RBF, it doesn't mean we want to be able to replace every spend.

To avoid this potential footgun, the ideal approach would be to use something like CanonicalizationParams::assume_not_canonical in #1809 (which is not helpful here).

Another idea would be to NOT modify add_utxos and use something custom for replace_tx.

However, add_utxos is problematic anyway because "these have priority over the unspendable utxos". So we can still create txs that selects "outputs of the replaced tx as inputs to the replacement".

Would it make more sense to change our logic to make "unspendables" have precedence? It does seem to be safer in this regard.

Ideally, we want to change add_utxos to disregard the is_spent of outputs that are inputs of the tx-to-replace. We can do this by observing the "unspendables" list and if the output is spent by something of the unspendables, we consider it unspent. We can do this by keeping track of the spends of the tx-to-replace.

[ValuedMammal]

We can do this by keeping track of the spends of the tx-to-replace.

I went with this idea and pushed an update f1f57d4.

[evanlinjin]

There are still a bunch of issues with this implementation that may result in footguns for the caller. However, I'm not sure if it is worth tackling here because the users really want this now and the only way to solve everything correctly is to use CanonicalizationParams which will not be available until bdk_wallet 2.0. Let me list them out below:

---

Given 2 unconfirmed txs (A, B), where B spends A. If the caller does replace_tx(A), then calls add_utxo(some-output-of-B), we will create a tx that simultaneously replaces A and spends from a descendant of A.

---

Given 2 unconfirmed txs (A, B), where B spends A. If the caller does replace_tx on both A and B, we will be creating a replacement of A & B that tries to spend from A.

---

Doing this logic in TxBuilder means introducing more footguns.

I would prefer if we just create a no-bdk-wallet dependency function that takes in a bunch of params to create an RBF tx. create_rbf(params: RBFParams) -> Result<Psbt, Error>. This way this feature is release-agnostic, does not introduce footguns and can be used without a bdk_wallet dependency.

I created a some proposed specifications for this create_rbf function.

[thunderbiscuit]

I’m glad to see this getting solid attention in review and and hope we find a good API for users (good meaning usable with minimal footguns, but also existing!).

I am, however, hesitant about shelving an approach that has working code and could be immediately useful, especially in favour of an alternative that, while promising on paper, currently has:

• No working implementation (still at the spec stage, so edge cases may yet surface)
• No tests
• An unclear timeline
• And potentially a dependency on 2.0?

I’m not trying to block progress or be antagonistic—I just want to advocate a bit for users of bdk_wallet. They form our biggest user base, and shipping something that’s imperfect but works today often ends up being more valuable for them than waiting on something ideal that might still be quite a ways off.

[evanlinjin]

@thunderbiscuit there is a working implementation.
bitcoindevkit/bdk-tx#1

I did this in a single day. Check /tests/synopsis.rs. Give me a few more days (a week at most) and can make it feature complete.

We won't need to wait until v2.0 to have this since it lives outside of bdk_wallet.

In terms of tests, a bunch of the heavy lifting is in bdk_coin_select and that is well tested. Also some of the tests in the old wallet-tx-builder are for making sure the policy module works. We're using the planning module in the new implementation.

The old codebase is a mess and is hard to reason about. I spent half a day reviewing this PR. I prefer putting my time into fixing it up for good.

[thunderbiscuit]

I want to give an update for anyone following this PR/thread.

I had a really good offline conversation with @evanlinjin, and it gave me an idea of what the next steps are for me.

The gist of my hesitation above revolves around the giving up of a useful feature that's ready to go (this PR) in exchange for an alternative for which I don't have a clear understanding of "readiness". Are we at the design phase? The hammering the details phase? How probable is it that users could leverage it in the 1.3/1.4 timeline? (June/August respectively).

This new crate (bdk-tx) has 2 PRs. It's fairly new. But also super promising. Doing away with code that is hard to reason about and leveraging the lessons we've learned over the past 4 years of using the TxBuilder is very appealing. I want to be on board, at least to understand better where this is going.

Here is what I think is missing at this point for bdk-tx to make a strong case for itself:

• There are no core/root issue on the bdk or bdk_wallet repositories making the sales pitch for it. What is it for? Is it intended as the complete replacement of TxBuilder? Can it be done within the 1.0 versions, or will it break anything? Could bdk_wallet maybe start using it one bit at a time?
• I hear here and elsewhere that the TxBuilder "is broken" and "is hard to reason about", and that we should not put more time into building new features into it, but rather focus on a new, better approach. But this TxBuilder has been with BDK since the beginning, and a migration away from it is no small affair. If this is where we're going, a clear outline of what is wrong with it, the issues we hope to fix in a replacement for it, and lessons learned should be written somewhere. May I suggest an ADR for it?

Here is what I commit to:

1. I want to understand better this alternative bdk-tx, and will be an ally for it moving forward in a few ways, namely:
2. Exploring how to use it in conjunction with bdk_wallet. You can find PRs here (bdk-tx) and here (bdk_wallet) if you'd like to follow along my poking around these APIs.
3. I'll open some of the issues I mention above that are missing to drive the discussion forward.
4. I will bring this up directly on dev calls and reach out to people to discuss it more and understand better the solution space for this.