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 |
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?
4e7409c to
ff48307
Compare
|
Rebased to resolve some conflicts. |
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
ff48307 to
6605157
Compare
LDK only persists a splice once its negotiation reaches AwaitingSignatures, so a splice in flight when the node stops can leave no trace in LDK's channel state, and no event of LDK's ever returns what the wallet reserved for it — today the addresses its outputs pay; once lightningdevkit#1037 locks a contribution's inputs in the wallet, those too, forever. At startup, reconcile each persisted splice intent against live channel state: release the reservations of a splice LDK no longer holds and drop its record, re-anchor a queued splice whose predecessor locked while the node was down, and keep — minus any inputs no surviving round still claims — those LDK resumes on its own. A splice whose channel closed meanwhile is released only if no round of it reached signing: a signed round is one the channel's monitor watches until the close matures, and what it reserved is spent by it or returned through DiscardFunding then. Reconciliation holds the lock that serializes splice submissions, as the event handlers settling intents do. Recovery fabricates no failure event for a splice lost this way: the initiating call already returned, and the channel simply no longer shows a pending splice. LDK itself reports the loss of a contribution it was still queueing or negotiating when it was last persisted — it fails the contribution as it is written and replays the failure at startup. The replay runs after reconciliation, so that report carries the splice's parameters only where reconciliation kept the intent: for a splice queued behind a pending one of ours, or a fee bump of one, but not for a channel's only splice, whose intent reconciliation settled. Reconciliation runs before background syncing and broadcasting start, so nothing can act on the stale reservations first. Events LDK replays from its last persisted state (e.g. a DiscardFunding for a splice that died before the node stopped) are likewise consumed before the node is running, so they cannot act on state a new user operation set up since. Developed with assistance from Claude Code. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
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)