Transactions inserted with Wallet::insert_tx will be replaced by the wallet when creating a new tx.

[evanlinjin]

Describe the bug

When a caller manually inserts a transaction into wallet via Wallet::insert_tx, it is an intuitive assumption that the transaction inserted will not be replaced by a new transaction created with Wallet::build_tx.

However, the current default behavior of Wallet::build_tx replaces the transaction inserted with Wallet::insert_tx since purely inserting a transaction without a last_seen or Anchor to the best chain means that the tx will not be part of the canonical chain.

Thank you @stevenroose for bringing this to my attention. Am I missing anything here?

To Reproduce

Some simple steps for reproducing.

• Create a wallet with a single UTXO.
• Create a tx (T1) from that wallet and send to foreign address A.
• Insert T1 into the wallet via Wallet::insert_tx.
• Create another tx (T2) that sends to foreign address B.
• Compare inputs of A & B. They should not be the same - but they are.

Proposed solutions

1. Add another parameter to insert_tx: last_seen: Option<u64>, and document that we need the last_seen value in order for the tx to be seen in the canonical chain.
2. Have wallet keep track of which transactions it has created. We shouldn't be replacing transactions that we create.

I think 1. should be the solution for now. 2. will need more thinking.

[stevenroose]

Yeah nothing missing. Few short points

• I expect my wallet to never double spend any txs that I explicitly made with it, unless explicitly indicated to double spend (RBF or otherwise).
• What is the current purpose of insert_tx and how does it compare to apply_unconfirmed_txs? I don't see insert_tx used anywhere internally.
• I think it would make sense if insert_tx would just add the current timestamp as last_seen. It has, after all, been seen because the user provides it. In fact it would make sense if the last_seen would be updated to the latest "now" if called twice for the same tx.

[evanlinjin]

What is the current purpose of insert_tx and how does it compare to apply_unconfirmed_txs? I don't see insert_tx used anywhere internally.

It's intended to be an external API. We didn't consider the tx-replacing problem though. Also our wallet examples are too basic to require it. I.e. relying on syncing with the chain source was good enough for them.

I think it would make sense if insert_tx would just add the current timestamp as last_seen. It has, after all, been seen because the user provides it. In fact it would make sense if the last_seen would be updated to the latest "now" if called twice for the same tx.

The last_seen value is meant to represent when a tx is last-seen in the mempool. Because we potentially haven't broadcasted the tx inserted with insert_tx, I'm wondering whether this will cause issues... 💭

[tnull]

What is the current purpose of insert_tx and how does it compare to apply_unconfirmed_txs? I don't see insert_tx used anywhere internally.

Also brought this up on Discord, but I'm also confused by this. What is the intended purpose of insert_tx?
If there is no way to ever remove the inserted tx again, and we can't be sure if the inserted tx would ever make it to chain, wouldn't inserting transactions permanently skew/corrupt the wallet state with potentially invalid data? Probably I'm missing something though?

[LLFourn]

What is the current purpose of insert_tx and how does it compare to apply_unconfirmed_txs? I don't see insert_tx used anywhere internally.

Also brought this up on Discord, but I'm also confused by this. What is the intended purpose of insert_tx? If there is no way to ever remove the inserted tx again, and we can't be sure if the inserted tx would ever make it to chain, wouldn't inserting transactions permanently skew/corrupt the wallet state with potentially invalid data? Probably I'm missing something though?

Basically if the transaction is only inserted it's in the "unbroadcasted" state. Concretely the wallet could show in its list of transactions "failed to broadcast" with a button to retry the broadcast. Unconfirmed txs have been broadcast and are therefore unconfirmed. There is no such thing as "invalid data" in this context (there is certainly useless data).

Like @evanlinjin suggest having the optional last_seen value would at least give you a place to document this and have the user explicitly opt in to the "unbroadcasted" state.

The more broad solution is to implement UTXO reservation/locking feature. Here are prev discussions:

1. Implement utxo reservation #913 (comment)
2. Let TxBuilder avoid previously used UTXOs (UTXO locking) #849

The main question is whether to persist these "locks" which enlarges the scope of the feature. But by the sounds of it @tnull and @DanGould want this which is enough to say it should exist.

The main challenge with persisting reservations is that naively they are not monotone (the locks can be turned off and turned on again). A way around this is to add a lock like (utxo, txid_locking) in a BTreeSet and then unlocks would just be HashSet<Txid>. So if you want to check if a utxo is locked you look for all the txids locking it and check if they are all in the unlock list.

[evanlinjin]

The more broad solution is to implement UTXO reservation/locking feature.

I can see how this can be useful, and I like your proposed implementation of how to make UTXO-lock changesets monotone.

However, I do NOT think UTXO-locking is a fit-for-all solution as it is locking up available funds. I.e. some users would like to use funds from unbroadcasted transactions as well.

My Proposed Solution (in addition to UTXO locking)

I want to modify how calculating the canonical history & utxo set works.

• I think by default, calculating the canonical history should INCLUDE unbroadcasted items.
• We should include a set of parameters that allow us to filter items out of the canonical history. I.e. exclude various unconfirmed txs and their children (this is useful when we want to replace them). I.e. exclude unconfirmed txs that have not been seen in the mempool (useful for displaying tx history to users - can separate out unbroadcasted stuff).

This allows the wallet to pick UTXOs from unbroadcasted txs. This also implements part of the logic required to replace a tx with children.

[DanGould]

I'm not sure exactly how locking / persisting locks / spending unbroadcasted utxos should work, but I can reiterate the constraints that caused me to raise this issue. In summary, Payjoin receivers must track 2 transactions spending the same foreign input: the original psbt containing only sender inputs, which the sender or receiver may broadcast at any time, and a payjoin psbt containing both sender and receiver inputs which a receiver must wait for the sender to sign and broadcast.

My understanding of the BDK wallet design is limited, but If I were deep in this problem, I might contact those from the Bitcoin Core wallet / mempool teams (Murch, Ava, and Gloria come to mind) since they have recorded opinions in PR decisions with historical context to the constraints of that project. Perhaps BDK has different constraints, and at the same time, could be informed by the decision making process of Core's constraints.

I believe that Core landed on in-memory UTXO locking long ago. Figuring out why that decision was made, and the problems that come up now with that presumably old design would inform my decision for a design in the relatively greenfield design space that is BDK.

edit: optional lock persistence was merged in 2021

[notmandatory]

This issue should be resolved by recently merged b0dc3dd and 00c568d. All tx will have a seen_at and the insert_tx function was removed since it's only meant for testing.

[LLFourn]

So those commits fix the issue described in the title however there is a more fundamental issue mentioned by @stevenroose:

Yeah nothing missing. Few short points
I expect my wallet to never double spend any txs that I explicitly made with it, unless explicitly indicated to double spend (RBF or otherwise).

In the current state of affairs this still happens by default. I think that having a concept of "unbroadcasted transactions" stored in the wallet which will reserve the inputs for use by that transaction before it is broadcast would solve this. This would involve inserting it into the tx graph as soon as its created and marking it us unbroadcasted. We could persist the designation too as a separate thing as part of the wallet changeset.

This doesn't solve @DanGould's problem at the wallet level but I think that the improvements to bdk_chain needed to make it happen would make his application easier to implement.

[notmandatory]

The headline issue has been fixed. Can we continue the discussion on related issue of locking unbroadcast spent utxos in new or exist issues? ie #1669

[LLFourn]

Sure I made an new issue: #1748