Applied unconfirmed transactions need to be pruned after some time

[tnull]

Describe the bug
In the last BDK Lib call we discussed that likely Wallet::apply_unconfirmed_txs won't ever forget the applied transactions. This is a bug as there is no guarantee that transactions might not eventually be dropped from the mempool, which would leave the wallet with unspendable UTXOs/funds, at least until the wallet is restarted and the state is lost.

Expected behavior
We need a way (read: prune_applied_transactions API or similar) to prune the applied transactions, preferable after a configurable threshold after the last seen, or after some number of blocks. Incidentally this would likely also resolve #849, as locking UTXOs prior to broadcast and locking UTXOs based on the ephemeral mempool state is more or less the same thing.

Additional context
We also discussed in the call that the difference between apply_unconfirmed_txs and insert_tx is unclear, as @stevenroose previously wondered in #1642 (comment)

[LLFourn]

To get the result you want, I think you could have a parameter to canonical txs/UTXOs/balance listing which asks it to ignore the effect of transactions that have a last_seen older than a certain limit.

Any valid transaction that is broadcast could reappear at any time. If the app no longer wants the transaction to exist then they should probably double spend it explicitly or otherwise rebroadcast. I think it's undesirable to "prune" this transaction from any data structure. There's no need to forget about it to get the right answer for any query you want to make about the data.

If the user has not explicitly asked for a tx to be canceled it should be fine to spend its utxos if we also broadcast the tx before broadcasting the child. Maybe in the future the transaction creation pipeline can just give you a "package" of transactions that should be broadcast which contains parents that may have dropped from the mempool.

[tnull]

Any valid transaction that is broadcast could reappear at any time.

Yes, at which point it would simply get synced again / applied again after having been pruned.

If the app no longer wants the transaction to exist then they should probably double spend it explicitly or otherwise rebroadcast.

I disagree. The expected flow here is that the user simply creates, signs, and broadcasts transactions. If these then appear in the mempool and are applied via apply_unconfirmed_txs, the wallet would no longer 'automatically' spend the used UTXOs (ever really). You can't expect a user to write custom logic to independently monitor the mempool and decide when it would become necessary to manually force-double-spend itself because otherwise the funds would become inaccessible via the Wallet. I do consider this a bug, somewhat independently from the discussion around a UTXO locking API (although I'd argue if this bug is properly fixed, we already have such an API available, as there is ~no difference between locking UTXOs before or after broadcast).

As a lot of these behaviors depend on internals and BDK has the best insight into what's going on, it's really the concern of the on-chain wallet and hence BDK needs to handle this by default in a sane and safe way that wouldn't lead to more and more funds becoming unspendable over time.

FWIW, on the call yesterday there even seems to be confusion around whether any API exists that would allow the user to create a conflicting transaction even if they explicitly specified the UTXOs (which users should never be required to do if they simply want to send some funds and leave everything else to BDK's coin selection algorithms, IMO).

[tnull]

Closing this for now as it may in part get completed by UTXO locking (i.e. is already tracked by #849)

[LLFourn]

I disagree. The expected flow here is that the user simply creates, signs, and broadcasts transactions. If these then appear in the mempool and are applied via apply_unconfirmed_txs, the wallet would no longer 'automatically' spend the used UTXOs (ever really). You can't expect a user to write custom logic to independently monitor the mempool and decide when it would become necessary to manually force-double-spend itself because otherwise the funds would become inaccessible via the Wallet. I do consider this a bug, somewhat independently from the discussion around a UTXO locking API (although I'd argue if this bug is properly fixed, we already have such an API available, as there is ~no difference between locking UTXOs before or after broadcast).

Even though the issue is closed I wanted to better understand what you are disagreeing with. The user has to decide the fate of their transactions. And I claim each unconfirmed tx is either in a state of:

1. The user has not broadcasted it (maybe it's still being signed) and wants to confirm it eventually
2. It's broadcasted and the user still wants it to be confirmed
3. It's broadcasted but the user wants to replace it
4. It's not broadcasted and the user no longer wants to broadcast it.

We have to consider the state when creating a new tx: in (1,2) we want to treat the tx as canonical regardless of whether it has been seen in the mempool recently or not and therefore it's UTXOs can be candidate inputs. In (2), if it hasn't been seen for a while this will mean we want the user to broadcast it again along with the new child tx.
In state (3) the application has to treat it as non-canonical and double spend it with the replacing tx. In state (4) it's just non-canonical and remains that way.

I don't think there is a fifth state we have to model where the user is thinking "I broadcasted this tx but you know I don't really care if it gets confirmed. If it hasn't been seen for a while I kinda want to prune it so I might inadvertently double spend it with my next tx...or I might not..whatever".

There are several ways in which these three tx states are not modeled correctly in BDK so there definitely is a design bug but it seems we are not in agreement to the nature of it. I think states (2) and (4) are easy to express, YMWV with (3) and (1) doesn't exist.

[tnull]

Even though the issue is closed I wanted to better understand what you are disagreeing with.

I think I generally agree with you analysis. However, I still think BDK could do a better job API-wise to smooth out the 'default' user experience. In contrast to Core's UX, the BDK wallet doesn't necessarily presuppose to be operated by a human, i.e., be it in a mobile app or on a server, BDK can't really design the API around the assumption that there always is an operator that will monitor the wallet state and take action in particular circumstances.

In this case, I'd argue that BDK should try to find an API/UX that by default allows 'naive' users that only make simple transactions "to do the right thing" by default. For example, BDK should auto-lock UTXOs it creates transactions to disallow double-spending itself when creating transactions without doing a syncing step in-between. However, there might be circumstances where we're positive that we failed to broadcast such a transaction, in which case it might make sense to allow explictly unlocking it again. Apart from that, BDK could offer logic to track, rebroadcast, and RBF-until-confirmed previously created transactions.

In general, as a user I expect all tracking and handling of transactions required for the simplest default use cases to happen in BDK / bdk_wallet. As the wallet has the best information on what it created when and how its syncing status is, etc., I really don't see why any of such logic should be replicated outside of the BDK objects.

Btw. note that when I opened this issue there was generally a lot of confusion as we had just discussed this in the BDK call and none of the attendees could clarify the currently implemented behavior, e.g., whether the state of unconfirmed transactions is persisted across restarts, how insert_tx/apply_unconfirmed_txs differ, whether there is a way to double-spend applied transactions easily at all, etc. So some of that has been clarified in the meantime, and efforts like #1748 seem like steps in the right direction.