bitcoindevkit / bdk

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

`Wallet::insert_tx` should not have `position` input #1446

Closed evanlinjin closed 4 months ago

evanlinjin commented 5 months ago

Describe the problem

The Wallet::insert_tx method allows us to add a transaction and instantly confirm it by anchoring the transaction to the wallet tip. This is dangerous as the user is not aware of how it works internally, and may anchor the transaction to the wrong tip (which will result in nonsensical history later on).

The correct way to insert confirmed or in-mempool transactions is via a chain-source which creates a wallet::Update.

I propose changing this method to only insert transactions without anchors or last seen. Therefore, I think this will be tackled in #1416 (assuming that #1416 is a good idea). In other words, this method will just insert transactions that have not left "application memory", or unbroadcasted transactions.

ValuedMammal commented 5 months ago

Thinking about this I questioned whether inserting txs through the wallet should even scan and index them - the worry being that if you insert several txs into the wallet with no intention of broadcasting them, will this put the wallet's balance and utxo pool into a bad state with no way to roll it back? I would think inserting an unbroadcast tx should not affect the balance or available coins if we follow the strict definition that unbroadcast txs don't have a canonical chain position.

In the draft PR I exposed a tx_graph_mut for Wallet which makes it possible to insert a tx into the inner TxGraph without indexing it. However we have wallet tests that are supposed to simulate inserting a tx that is either confirmed or in mempool, and for that we do need to index it and make sure it has a chain position. It's still worth having a warning or disclaimer that manually altering the inner TxGraph is not the recommended way to add new tx data, but rather to sync with a chain source and apply the resulting updates.

I think unbroadcast txs are subject to the same conflict handling rules, only that a tx without a last_seen has no precedence over any other tx, and as long as it doesn't affect the available coin pool then replacing it is not a problem - just broadcasting the real tx is enough to replace any potential conflicts.

So what if we do want to return non-canonical transactions and txouts? The caller can apply a filter to the result of TxGraph::full_txs for example, or we can add methods that take a filter_map as a closure and return an iterator of filtered graph data.

We should have tests that show:

ValuedMammal commented 5 months ago

Additional note about an issue related to temporarily locking coins used to create a new tx (which seems counter to the above comments regarding the available balance) - I'm in favor of having #849 as a feature request, so attempting to resolve the current issue should try to accommodate a future where it's possible to lock utxos "in flight".

Also, working on #1416 made me realize it may be necessary to change update_last_seen_unconfirmed to accept a ChainOracle impl which was already suggested by Evan in a past discussion. The reason is that if a tx is anchored to a block that is reorged out and later re-submitted to the mempool, under the current implementation it would not have its last_seen time updated when it arguably should.