bitcoindevkit / bdk

A modern, lightweight, descriptor-based wallet library written in Rust!
Other
862 stars 310 forks source link

Applied unconfirmed transactions need to be pruned after some time #1666

Open tnull opened 1 week ago

tnull commented 1 week ago

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

LLFourn commented 1 week ago

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 commented 6 days ago

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).