Fix wallet UTXO reuse for funding transactions and onchain spends - #1037
Fix wallet UTXO reuse for funding transactions and onchain spends#1037tnull wants to merge 3 commits into
Conversation
|
👋 Thanks for assigning @jkczyz as a reviewer! |
5385ef8 to
2932580
Compare
|
Drafting this for now as it might make sense to wait for #962 to land first, and then rebase this. |
2932580 to
61b7adf
Compare
I'd say either here or in a dedicated PR. It can be done independent of #930, which is already pretty big. That touches |
Record wallet transactions before broadcast and reserve funding inputs until their transactions are durable. This prevents concurrent operations from selecting the same inputs. Dropped transactions remain recoverable through the existing rebroadcast path. Co-Authored-By: HAL 9000
Locally inserted transactions can be newer than Bitcoin Core's latest mempool timestamp. Reporting that stale timestamp for an eviction makes BDK ignore it and leaves the transaction's inputs unavailable. Use the later of the local observation time and Bitcoin Core's mempool time. This makes local transactions evictable without regressing nodes whose Bitcoin Core clock is ahead of the application clock. Co-Authored-By: HAL 9000
Reserve wallet inputs as soon as splice coin selection returns so concurrent wallet operations cannot reuse them before the funding transaction reaches the wallet. Release discarded contributions so failed or superseded splice rounds do not strand funds. Co-Authored-By: HAL 9000
61b7adf to
4e7409c
Compare
Now added a commit here, let me know what you think |
Jolah1
left a comment
Jolah1
left a comment
There was a problem hiding this comment.
The overall approach looks good, but I found one persistence-failure issue that should be addressed before merging. Both channel-funding creation and splice coin selection can leave their selected inputs durably locked when persisting the locks fails.
I reproduced this with a fault-injecting store: the operation returned an error, the inputs remained locked, and after persistence recovered the locks survived a wallet reload. The same issue occurs in select_confirmed_utxos at line 1384.
I left the details inline.
|
|
||
| (tx, locked_wallet.take_staged().unwrap_or_default()) | ||
| }; | ||
| locked_persister.persist_changeset(change_set).await.map_err(|e| { |
There was a problem hiding this comment.
If this persistence fails, the inputs locked above are leaked. persist_changeset retains the staged changeset on failure, but this function returns without the transaction, so the caller cannot release the locks and LDK cannot later emit DiscardFunding.
A subsequent successful wallet persistence makes the orphan locks durable. I reproduced this across a wallet reload with a fault-injecting store. The same issue occurs in select_confirmed_utxos below. Could we roll back both the in-memory and staged locks before returning the error, with regression tests for both paths?
4 participants
Fixes #41.
For the longest time BDK didn't offer any UTXO locking mechanisms and only considered transactions canonical once seen in the mempool during syncing. This always left a gap between the time of transaction signing/broadcast and the time of sync during which the wallet could double-spend itself. Since
bdk_walletv3.0 they finally offer UTXO locking APIs which we finally use here to close this gap for funding transactions and onchain spends.Note: We intentionally leave splicing transactions out-of-scope of this PR because with #962 and #930 there are related PRs inflight. Depending on the order these land, this PR or they need to be updated to marry the two approaches. (cc @jkczyz)