Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Wallet::insert_tx should not have position input #1446

Closed
evanlinjin opened this issue May 16, 2024 · 2 comments · Fixed by #1416
Closed

Wallet::insert_tx should not have position input #1446

evanlinjin opened this issue May 16, 2024 · 2 comments · Fixed by #1416
Assignees
Labels
api A breaking API change new feature New feature or request
Milestone

Comments

@evanlinjin
Copy link
Member

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
Copy link
Contributor

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:

  • We can build many txs with the same coins
  • Inserting unbroadcast txs don't affect the balance calculation
  • Inserting txs and manually marking them seen can affect the balance and available coins
  • Test the value returned by try_get_chain_position for different txs at different local chain heights.

@notmandatory notmandatory added this to the 1.0.0-alpha milestone May 30, 2024
@notmandatory notmandatory added this to BDK May 30, 2024
@notmandatory notmandatory moved this to Todo in BDK May 30, 2024
@ValuedMammal
Copy link
Contributor

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.

@notmandatory notmandatory added the api A breaking API change label Jun 1, 2024
@notmandatory notmandatory moved this from Todo to In Progress in BDK Jun 18, 2024
@notmandatory notmandatory moved this from In Progress to Needs Review in BDK Jun 18, 2024
@github-project-automation github-project-automation bot moved this from Needs Review to Done in BDK Jul 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api A breaking API change new feature New feature or request
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

3 participants