Repository navigation
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
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.
4e7409c to
ff48307
Compare
|
Rebased to resolve some conflicts. |
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>
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>
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>
Reword what the comments say LDK returns for a synchronously rejected contribution and for a signed round the monitor never watched, so they hold at the pinned LDK and once it carries the fixes for rust-lightning issues 4986 and 4967: a refusal's `DiscardFunding` names the parts no pending splice attempt still uses, except that before 4986 is fixed a refusal for a channel or peer LDK no longer knows names the whole contribution, and once 4967 is fixed a round the monitor never watched is released by the `DiscardFunding` LDK reports at the force-close. Scope the `TODO(lightningdevkit#1037)` notes to those fixes. Developed with assistance from Claude Code. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
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>
Reword what the reconciliation comments say about rounds LDK wrote, so they hold once the pinned LDK carries the fix for rust-lightning issue 4967: LDK then reports a `DiscardFunding` at a force-close for a recorded round the monitor never watched, which nothing released before. Split the `TODO(lightningdevkit#1037)` note into the part that fix removes and the case that stays: a hand-off LDK never wrote, whose channel is force-closed as stale at startup, gets no event and must be released here. Developed with assistance from Claude Code. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Reword what the comments say LDK returns for a synchronously rejected contribution and for a signed round the monitor never watched, so they hold at the pinned LDK and once it carries the fixes for rust-lightning issues 4986 and 4967: a refusal's `DiscardFunding` names the parts no pending splice attempt still uses, except that before 4986 is fixed a refusal for a channel or peer LDK no longer knows names the whole contribution, and once 4967 is fixed a round the monitor never watched is released by the `DiscardFunding` LDK reports at the force-close. Scope the `TODO(lightningdevkit#1037)` notes to those fixes. Developed with assistance from Claude Code. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Reword what the reconciliation comments say about rounds LDK wrote, so they hold once the pinned LDK carries the fix for rust-lightning issue 4967: LDK then reports a `DiscardFunding` at a force-close for a recorded round the monitor never watched, which nothing released before. Split the `TODO(lightningdevkit#1037)` note into the part that fix removes and the case that stays: a hand-off LDK never wrote, whose channel is force-closed as stale at startup, gets no event and must be released here. Developed with assistance from Claude Code. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Reword what the comments say LDK returns for a synchronously rejected contribution and for a signed round the monitor never watched, so they hold at the pinned LDK and once it carries the fixes for rust-lightning issues 4986 and 4967: a refusal's `DiscardFunding` names the parts no pending splice attempt still uses, except that before 4986 is fixed a refusal for a channel or peer LDK no longer knows names the whole contribution, and once 4967 is fixed a round the monitor never watched is released by the `DiscardFunding` LDK reports at the force-close. Scope the `TODO(lightningdevkit#1037)` notes to those fixes. Developed with assistance from Claude Code. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Reword what the reconciliation comments say about rounds LDK wrote, so they hold once the pinned LDK carries the fix for rust-lightning issue 4967: LDK then reports a `DiscardFunding` at a force-close for a recorded round the monitor never watched, which nothing released before. Split the `TODO(lightningdevkit#1037)` note into the part that fix removes and the case that stays: a hand-off LDK never wrote, whose channel is force-closed as stale at startup, gets no event and must be released here. Developed with assistance from Claude Code. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Reword what the comments say LDK returns for a synchronously rejected contribution and for a signed round the monitor never watched, so they hold at the pinned LDK and once it carries the fixes for rust-lightning issues 4986 and 4967: a refusal's `DiscardFunding` names the parts no pending splice attempt still uses, except that before 4986 is fixed a refusal for a channel or peer LDK no longer knows names the whole contribution, and once 4967 is fixed a round the monitor never watched is released by the `DiscardFunding` LDK reports at the force-close. Scope the `TODO(lightningdevkit#1037)` notes to those fixes. Developed with assistance from Claude Code. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Reword what the reconciliation comments say about rounds LDK wrote, so they hold once the pinned LDK carries the fix for rust-lightning issue 4967: LDK then reports a `DiscardFunding` at a force-close for a recorded round the monitor never watched, which nothing released before. Split the `TODO(lightningdevkit#1037)` note into the part that fix removes and the case that stays: a hand-off LDK never wrote, whose channel is force-closed as stale at startup, gets no event and must be released here. Developed with assistance from Claude Code. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Reword what the comments say LDK returns for a synchronously rejected contribution and for a signed round the monitor never watched, so they hold at the pinned LDK and once it carries the fixes for rust-lightning issues 4986 and 4967: a refusal's `DiscardFunding` names the parts no pending splice attempt still uses, except that before 4986 is fixed a refusal for a channel or peer LDK no longer knows names the whole contribution, and once 4967 is fixed a round the monitor never watched is released by the `DiscardFunding` LDK reports at the force-close. Scope the `TODO(lightningdevkit#1037)` notes to those fixes. Developed with assistance from Claude Code. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Reword what the reconciliation comments say about rounds LDK wrote, so they hold once the pinned LDK carries the fix for rust-lightning issue 4967: LDK then reports a `DiscardFunding` at a force-close for a recorded round the monitor never watched, which nothing released before. Split the `TODO(lightningdevkit#1037)` note into the part that fix removes and the case that stays: a hand-off LDK never wrote, whose channel is force-closed as stale at startup, gets no event and must be released here. Developed with assistance from Claude Code. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Reword what the comments say LDK returns for a synchronously rejected contribution and for a signed round the monitor never watched, so they hold at the pinned LDK and once it carries the fixes for rust-lightning issues 4986 and 4967: a refusal's `DiscardFunding` names the parts no pending splice attempt still uses, except that before 4986 is fixed a refusal for a channel or peer LDK no longer knows names the whole contribution, and once 4967 is fixed a round the monitor never watched is released by the `DiscardFunding` LDK reports at the force-close. Scope the `TODO(lightningdevkit#1037)` notes to those fixes. Developed with assistance from Claude Code. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Reword what the reconciliation comments say about rounds LDK wrote, so they hold once the pinned LDK carries the fix for rust-lightning issue 4967: LDK then reports a `DiscardFunding` at a force-close for a recorded round the monitor never watched, which nothing released before. Split the `TODO(lightningdevkit#1037)` note into the part that fix removes and the case that stays: a hand-off LDK never wrote, whose channel is force-closed as stale at startup, gets no event and must be released here. Developed with assistance from Claude Code. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Reword what the comments say LDK returns for a synchronously rejected contribution and for a signed round the monitor never watched, so they hold at the pinned LDK and once it carries the fixes for rust-lightning issues 4986 and 4967: a refusal's `DiscardFunding` names the parts no pending splice attempt still uses, except that before 4986 is fixed a refusal for a channel or peer LDK no longer knows names the whole contribution, and once 4967 is fixed a round the monitor never watched is released by the `DiscardFunding` LDK reports at the force-close. Scope the `TODO(lightningdevkit#1037)` notes to those fixes. Developed with assistance from Claude Code. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Reword what the reconciliation comments say about rounds LDK wrote, so they hold once the pinned LDK carries the fix for rust-lightning issue 4967: LDK then reports a `DiscardFunding` at a force-close for a recorded round the monitor never watched, which nothing released before. Split the `TODO(lightningdevkit#1037)` note into the part that fix removes and the case that stays: a hand-off LDK never wrote, whose channel is force-closed as stale at startup, gets no event and must be released here. Developed with assistance from Claude Code. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
6605157 to
f7b63a8
Compare
|
Just ran into this with my LN node management bot, looking forward to this landing! |
`send_to_address` signed and broadcast a transaction without applying it to the BDK wallet, which only learned of it at the next sync. A second send before then could select the same inputs: the network accepted one and rejected the other, while both callers got a txid. The rejection was logged at TRACE only. This produced phantom reverse swap lockups on signet (kaleidoswap/kaleidoswap-maker-rs#853 and kaleidoswap/kaleidoswap-maker-rs#880). Modelled on lightningdevkit#1037 (still open upstream), not copied, and scoped for our mainnet branch: - Apply sends to the wallet before broadcasting them, and record the pending payment then. A failed wallet persist undoes the in-memory application and returns an error without broadcasting. Once the transaction is durable it is always broadcast, so an error never means "maybe sent". - Apply RBF replacements before broadcasting them too, carrying the replaced rounds forward in `conflicting_txids` (the next sync no longer reports `TxReplaced`). - Do not let a sync evict one of our own spends seen in the last 300 s: a chain source that has not indexed a fresh send yet would otherwise hand its inputs back to coin selection. Covers Esplora, Electrum and the bitcoind mempool poll. Transactions that only pay us are not shielded, so a sender's replacement is still noticed. - Rebroadcast evicted outbound sends, reapplying them first, unless a canonical transaction now spends their inputs or a channel funding reserved them. - Reserve channel funding inputs from creation until broadcast, failure or `DiscardFunding`. The locks are in memory only (our persister does not write BDK's `locked_outpoints`), so a restart releases them instead of stranding inputs. Locked value is not spendable and not offered for anchor spends. - Log Esplora broadcast rejections at ERROR unless bitcoind merely already knows the transaction. Splice inputs stay out of scope, as upstream. Tests: wallet unit tests for disjoint back-to-back sends, a failed persist releasing inputs, the eviction grace (own spends only), safe rebroadcast, double RBF before sync, and funding reservations; the `back_to_back_onchain_sends_before_sync` integration test against bitcoind/electrs, which fails without the fix the way signet did.
🤖 #1057, #1079 and #1080 have all landed, so the splicing parts can move again. A few notes from looking at f7b63a8 against current The splice intent is gone. #1080 merged without the intent tracker, and #1232 removes the Recovery of a dropped splice mostly comes for free from LDK. With the pinned LDK, What remains is a crash window. The wallet persists the locks at coin selection, before the contribution reaches LDK. A crash between that write and the background processor's next manager write leaves the outpoints locked until a later transaction spends them. Since this PR introduces the persisted locks, it should also close that window: at startup, unlock any persisted outpoint that no pending channel funding or in-flight splice contribution references. Rebase notes.
|
The wallet persister wrote every part of a BDK change set except the locked outpoints, so a lock placed on a wallet output was forgotten on restart. Store the locked outpoints under their own key and restore them when the wallet is loaded. Released locks are dropped from the stored set so it does not grow with every lock ever taken. Co-Authored-By: HAL 9000
The wallet learned of its own sends and fee bumps only from the next chain sync. Until then a second send selected the same inputs, and a second fee bump of the same payment failed as the wallet did not know the first replacement. Apply such a transaction to the wallet when it is created, and record its payment the way a sync would. A chain source that has not seen the transaction yet reports it as gone, so a sync returns a pending send to the wallet unless that would overrule what else the chain source reported. Fixes lightningdevkit#1117. Co-Authored-By: HAL 9000
Bumping the same on-chain payment twice left the first replacement unmapped. The payment id is derived from the original txid and each bump sets the payment's txid to the newest one, so an earlier replacement is found only through the pending-store entry's conflicting txids. The bump never added the replaced txid to that list, and since the bump applies the replacement to the wallet itself, no later sync reports the replacement to fill it in. The bump now records the replaced txid in the pending-store entry, so a replaced round that confirms or is reported by sync still resolves to the payment. Co-Authored-By: HAL 9000
The wallet learns of a channel funding transaction only from a chain sync, as LDK broadcasts it once the counterparty signed. Until then nothing kept a second channel open or an on-chain send from selecting the same inputs, leaving one of the two transactions unable to confirm. Lock the inputs of a funding transaction when it is built, and keep them out of coin selection and the spendable balance. The locks are returned when LDK discards the transaction or never takes it, and dropped once the transaction confirms. They are recorded with what the node knows about the transaction, so that a startup sweep can return the locks of a channel that is gone, as LDK does not report every transaction it lets go. Co-Authored-By: HAL 9000
The inputs the wallet selects for a splice are handed to LDK, which negotiates the transaction spending them with the counterparty. The wallet sees nothing of that negotiation, so a concurrent splice, channel open or on-chain send could select the same inputs. Lock the inputs of a splice contribution when they are selected, the way channel funding inputs are. They are returned when LDK discards the contribution, dropped once the splice transaction confirms, and kept across a restart for as long as LDK tracks the round they were contributed to. Inputs LDK would refuse to announce are left out of the selection, as it refuses them only after they are locked. Co-Authored-By: HAL 9000
f7b63a8 to
00136a4
Compare
|
Now rewrote the branch after #1080 landed. |
Fixes #41, #1117.
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)