Repository navigation
Conversation
|
👋 Thanks for assigning @tnull as a reviewer! |
aba7686 to
80a1b27
Compare
80a1b27 to
ca4e5fd
Compare
d457713 to
74e0acd
Compare
d520e2e to
8f2f58d
Compare
0767802 to
67315bb
Compare
67315bb to
f597be8
Compare
ce2f247 to
3194e0c
Compare
f5e5753 to
9f474a0
Compare
|
Also needs a rebase again. |
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. Persist each user-initiated splice as an intent record before its contribution is handed to LDK, so such a splice can be recognized at the next startup -- releasing whatever the wallet still reserves for it, which a later commit adds -- and so events about the splice can be described in terms of the original request. Each splice gets a record of its own, so that its failure is described from its own intent and a restart recognizes it whatever became of the channel's other splices: a splice queued behind a pending one negotiates as a splice of its own once the pending one locks, and its rounds must not be filed under the pending splice's payment. Only a fee bump joins an existing record, that of the round it replaces; when wallet sync has removed that record in between, as it does once the round's payment graduates, the bump gets a record of its own rather than going out with none. A splice is refused while the channel carries an intent anchored at another funding -- one the lock that superseded it failed to settle or to re-anchor -- rather than recorded beside it. A submission reads the channel's funding under the lock that serializes splice submissions and anchors its intent there, not at the funding the caller read before building the contribution: a splice locking in between moves the funding, and an intent anchored at the old one would never be settled by the lock that superseded it. A funding that moved refuses a fee bump, whose round has locked, and a splice-in, whose inputs the locked round may have spent; a splice-out carries no wallet inputs and proceeds. A splice submitted after the previous one locked with zero confirmations settles that splice's intent first, as the lock's event would have: LDK promotes the funding as soon as splice_locked is exchanged but only queues the event. The lock and close event handlers settle intents under the same lock, so a lock handled mid-submission cannot settle the new intent before its contribution reaches LDK. The record is undone when LDK rejects the hand-off synchronously and settled once the splice locks, its failure is surfaced, or its channel closes. A failure event settles the intent only after the event is durably queued -- a crash in between leaves the intent for the replayed event to settle, erring toward a duplicate report over a lost one -- and only when the event's contribution identifies the recorded splice: a mismatch means the failure concerns an older, superseded attempt with no record of its own. Dropping the signed round a failure abandoned from its record leaves the intent the record carried behind as a bare intent, so the report can still describe the splice. A splice queued behind another pending splice survives the pending splice's lock, so its intent is re-anchored to the new funding rather than settled. Failing a funding payment -- when a round other than its own locks, when its channel closes, or when wallet sync finds its round lost to a confirmed conflicting spend -- likewise keeps the intent its entry carried, as a bare intent under an id of its own. LDK carries a fee bump queued behind a round it does not overlap across that round's lock and begins a fresh splice from it, so the intent is still needed: to re-anchor it at the new funding, to file the fresh round under it when signed, and to describe the failure LDK reports if the fresh negotiation fails instead. Under the failed record's id, the fresh round would take that record and go untracked. The lock and close handlers settle the kept intent right after it is kept, unless LDK still tracks its splice and the lock re-anchors it instead; one kept from wallet sync stays anchored at the channel's unchanged funding, where a later fee bump joins it, and the submission check does not refuse a splice on its account. An entry without a payment record yet, failed on those occasions under a record written for it then, keeps its intent the same way. Wallet state staged on a splice's behalf is flushed only after the intent record persists, so what the wallet reserves for a splice can never outlive the record through which a later startup would release it. A splice that fails before the hand-off immediately releases what the wallet reserves for it, short of what another round uses as well -- a fee bump built by adjusting the fee of the round it replaces shares that round's inputs and change address, which stay reserved while the round can confirm; one LDK rejects has it returned through the DiscardFunding event instead. A lock settles an intent without releasing anything: what the locked round did not spend, LDK returns through the DiscardFunding events it queues at the promotion. The round recorded when this node signs a splice round is filed under the record of the intent carrying the round's contribution, written while holding the lock that serializes splice submissions, so neither a fee bump replacing the intent nor a failure settling it can interleave with the write. Wallet sync later files the funding payment under that id and indexes it in the pending store in the same write, so a payment record found under a bare intent is the first half of a write that never completed, which the pending store would never drive; settling the intent removes it along with the entry rather than leaving a record that could never be driven. Developed with assistance from Claude Code. Co-Authored-By: Elias Rohrer <dev@tnull.de> Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Written with the help of an AI tool. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
A record written when this node signed a round, before wallet sync observed any transaction of the funding, has the funding's channels but no payment record yet. concerns_channel only looked at the payment record's classification, so it missed such a record once a failed fee bump's settlement had cleared its intent: the next bump's intent went into a fresh record while its round was filed under the original one. At the next startup, a channel closed while the node was down then had that bump's contribution released from the fresh record, though the rounds recorded under the original could still confirm. Written with the help of an AI tool. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The record is already gone when this branch runs: remove_payment_if removed it, or found it removed by an earlier attempt. The second removal is left over from the read-then-remove it replaced, and the comment above it already says the record went first. Written with the help of an AI tool. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
219c46f to
8e5fe8b
Compare
|
Rebased and had Claude address those: Thanks — went through all eight. Four led to fixups, the rest I've answered inline. 1. 2. 3. The signing event when the cancel fails. 4. 5. 6. The second removal. Fixed in d8d6fca: the removal is gone. It was left over from the read-then-remove it replaced. 7. Force-close vs cooperative close. Documented on the 8. |
decide_on_lock and LockDecision::{Keep, Refresh, Settle} were named
after the caller and its reactions. What the function determines is
what the funding moving on did to a recorded intent, so name it and
its outcomes after that: effect_of_funding_move and
FundingMoveEffect::{Unaffected, StillQueued, Superseded}.
Written with the help of an AI tool.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
8e5fe8b to
946f2d3
Compare
"Funding move" was this module's own phrase; LDK calls the event a splice lock (splice_locked, SpliceLocked), and every caller of the check is reacting to one. Rename FundingMoveEffect and effect_of_funding_move to SpliceLockEffect and effect_of_splice_lock, and say "a splice locked" wherever the docs, comments, refusal messages and a test said the funding moved. Written with the help of an AI tool. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The signing handler previously logged and dropped both failure paths (with TODOs to abort once LDK supported it), leaving the negotiation dangling until a peer disconnect abandons it. Cancel the contributed funding instead. LDK then emits DiscardFunding, releasing whatever the wallet reserves for the contribution, and SpliceNegotiationFailed, which surfaces the failure and settles the persisted intent. Cancel errors are only logged: every error case means the splice is already beyond canceling. When LDK refuses the already-signed transaction, the failure report that cancelling produces also drops the round recorded at signing time from its record: the round is gone from the channel's history and can never be broadcast, so left in place the record would wait forever on a payment that can never confirm. Developed with assistance from Claude Code. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
An application handling SpliceNegotiationFailed had nothing to act on: the event did not say why the splice failed, nor what the failed call had attempted. Both matter for deciding what to do next — a fee bump lost to a disconnect can simply be re-issued, while the splice it meant to bump may still confirm at the prior feerate. Attach a reason, mapped from LDK's NegotiationFailureReason onto an ldk-node-owned enum so the event's serialization and bindings do not change with LDK's, and the parameters of the originating API call, taken from the persisted splice intent when the failure identifies it. Both fields are optional and serialized as odd TLVs: events written by LDK Node v0.7 read back as None, and v0.7 readers ignore the new fields. Developed with assistance from Claude Code. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
A force-close reports a splice failure after the channel's ChannelClosed event, which settles the channel's splices, so the failure arrives without the splice's parameters; a cooperative close reports it before and with them. The field's doc only covered the fee-bump case. Written with the help of an AI tool. 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 LDK never returns through an event what the wallet reserved for it -- today the addresses its outputs pay; once inputs are locked at coin selection, those inputs as well. At startup, reconcile each persisted splice intent against live channel state: release the reservations of a splice LDK no longer tracks 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 its rounds never 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 the stale reservations are gone before anything can act on them. 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>
Written with the help of an AI tool. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
reconcile lists the intent records once and runs the settle pass per record whose funding moved, so a later record in the list may already have been re-anchored by an earlier pass and never reach the startup decision. That is safe, but the code did not say why: such an intent is one LDK still tracks with the same inputs, which the decision would keep and find nothing to unlock for. Written with the help of an AI tool. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
decide_reconcile and ReconcileDecision::{Keep, Lost} were named after
the caller and its reactions. The function answers whether LDK still
tracks the splice an intent recorded, so it is now
splice_survived_restart and returns a bool.
Written with the help of an AI tool.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The comment used words that are not in the code ("the pass", "the
list above", "the decision below") and never said what it was
explaining. Name the call that handles the channel's other intents,
the snapshot their stale copies sit in, and the check they skip.
Written with the help of an AI tool.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Same wording change as for the lock-time check, applied to what this commit added: the reconcile comment, the settle doc's reconcile clause, and a test doc. Written with the help of an AI tool. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
A disconnect during the interactive negotiation fails the splice with PeerDisconnected. The first test asserts that exactly one SpliceNegotiationFailed reaches the user -- carrying the reason and the originating request's parameters -- and that a new splice initiated afterwards completes with a single funding payment. The window only exists mid-negotiation: a contribution still queued at disconnect is resumed by LDK itself on reconnect, and one awaiting signatures survives re-establishment. The test therefore synchronizes on the counterparty's splice_ack -- logged by LDK's peer handler -- and stretches the negotiation by funding the splice from many small UTXOs, each of which adds an interactive-tx round trip. A splice dropped by a restart is recovered silently: startup reconciliation releases what the wallet reserved and drops the record without fabricating a failure event. What does reach the user is the failure LDK persisted at shutdown and replays at startup -- once, with parameters only when it still matches a kept record. The restart tests cover both cases: a dropped splice-out surfaces without parameters and a further restart stays silent, while a dropped fee bump -- whose record reconciliation keeps, since LDK still tracks the negotiated splice -- surfaces with the bump's parameters. In both, the application re-initiates and the splice completes. A splice confirmed while its node was offline keeps exactly one payment record under its splice-time id across the restart. Three more cases: a second splice submitted right after a zero-conf lock gets a record of its own rather than being folded into the record of the splice that just locked; a queued splice the node stopped on, which LDK fails as it shuts down, is reported at startup with its parameters -- its record, an intent that never became a payment, outlives the pending splice's graduation, and reconciliation keeps it while LDK still tracks that splice; and a funding record left half-written by a stop between the two stores of the write that files it is dropped at the next startup instead of lingering as a payment that is never indexed. The existing test of a signed round the channel's monitor never watched asserts the reason the node surfaces at the force-close, ChannelClosing, and that no parameters come with it, the intent having been cleared at the close. Developed with assistance from Claude Code. Co-Authored-By: Elias Rohrer <dev@tnull.de> Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Recording what a signed interactive funding round is, and what this node's share of it comes to, can be refused for want of room, and the signing went ahead regardless. The transaction was then released without this node's contribution on record, so whoever first observed it recorded the wallet's view of a funding output both parties own: the whole of it read as this node's spend, a figure that is never corrected. The round's facts are now admitted however many records the store contains. The cap bounds what a counterparty drives by opening and closing channels or replacing a negotiated funding; a round this node chose to sign is not that, and its record describes the round and this node's share of it while carrying no outputs. What can still refuse the round is a record already as large as one record may be, which never shrinks, so the round is reported unmeasurable and the handler cancels the splice, as it already does where LDK refuses the signed transaction. The failure is then the splice's rather than the node's, and the round is never released with the wallet's view of a funding output both parties own standing in for this node's share. Failing the signing for the handler to replay would not do: LDK stops handling events at the first failure and leaves the failing one at the head of its queue, so everything behind it waits, claims on inbound HTLCs among them, while the same pass flags the channel manager for persistence and notifies a waiter that re-enters at once. A full store is not a condition a replay clears: room comes only from the dropping pass, which wants a record a year old whose channels have all resolved. The commit introducing the refusal said its cost was a transaction reported without a classification. That holds for a producer reporting on something it has already released, which still only logs, and the places restating it now distinguish the two. Co-Authored-By: HAL 9000 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
A splice is persisted with nothing but its intent before anything about it has been observed. Wallet sync promotes that entry once it sees the transaction -- unless the payment has already advanced past pending, in which case the store should not have an entry for it and the write is declined. Declining left the bare intent where it was, so a splice that was over went on looking like one still in flight, and the next restart acted on it. The intent is now taken back instead, and only while the entry still is the bare one: a round signed or a fee bump submitted since then is live state of its own, which the splice lifecycle has to be the one to resolve. Co-Authored-By: HAL 9000
A channel becoming pending reported both the output that funds it and that the transaction is a funding of that channel. The second only repeats the first, since a funding output is what a funding transaction is recognised by. It also cost something. One transaction can open several channels, and each reports only its own output; a report stating outright what the transaction is lists one channel, so the next channel's contradicts it and is refused whole, taking its funding output with it. The payment then listed one of the channels opened instead of all of them. A channel becoming ready already reports only its funding output, so the two backstops now agree. Co-Authored-By: HAL 9000 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
It only forwarded to mutate_with, so that method is now pub(crate) and the callers that await fallible reads use it directly. Written with the help of an AI tool. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
946f2d3 to
957275d
Compare
The comment that Claude wrote for this wasn't really understandable. I had it rename a couple methods and re-write it. 🤖 Five more fixups on top. The comment I added for 5 (72ad320) leaned on words that aren't in the code, so 95523e0 rewrites it to name the call, the snapshot, and the check it talks about. The two helpers it refers to are renamed after what they answer rather than after their caller: |
tnull
left a comment
tnull
left a comment
There was a problem hiding this comment.
Another round of Claude feedback:
- Medium: cancel_splice can cancel the wrong splice and leave the failed round stuck (src/event.rs:949, called from lines 2832, 2859 and 2871)
The handler assumes that cancelling aborts the round it just declined to sign. The pinned LDK's Channel::cancel_funding_contributed (channel.rs:13681) checks
for a queued contribution first, and if there is one it abandons that and returns.LDK explicitly allows a queued contribution alongside a counterparty-initiated negotiation (channel.rs:13570). The PR's own docs also describe joining a
counterparty's round.Failure scenario:
- This node has a contribution in a counterparty-initiated round A, which is awaiting signatures.
- The user submits splice B. LDK queues it, because already_contributing only looks at negotiations this node initiated.
- Handling FundingTransactionReadyForSigning for A fails, through a signing failure, Unmeasurable, or LDK refusing the witnesses.
- cancel_splice cancels B, so the user sees LocallyCanceled for a healthy splice.
- A stays unsigned and the handler returns Ok(()), so nothing replays it in-session. A keeps the channel's splice state stuck until a restart regenerates
the event.In the "LDK refused" path, A's record also isn't taken back, contrary to the comment at event.rs:2857.
Possible fix: call cancel again if the first call only removed a queued contribution, or have LDK expose which one it cancelled. A regression test should
queue B behind a counterparty round and force a signing failure.
- Low: restart recovery releases a lost fee bump's inputs but not its outputs (src/channel/mod.rs:193)
When a splice survives a restart, reconcile only unlocks unclaimed_inputs, whereas the other release paths use release_contribution, which also frees
outputs.If a fee bump that re-selected its inputs is lost, its new change address stays marked as used. Nothing else recovers it at that point: the kept intent
settles without a release, and LDK's DiscardFunding may not cover a bump it never persisted. The cost is a skipped address rather than lost funds.Possible fix: use unclaimed_parts here and release both parts.
- Low: two comments describe an event LDK doesn't emit (src/channel/mod.rs:72, :561)
Both justify the submit lock partly by the failure event of a synchronously rejected hand-off. LDK's funding_contributed docs and code say a rejection emits
no SpliceNegotiationFailed, only DiscardFunding. The lock is still justified by the other races listed there, but these comments send readers after a race
that can't happen.
|
I also asked Claude whether it sees a way to keep this PR less invasive, and it seems to think so:
Probably the items on the 'What you give up' section are the debatable parts here? |
Persist every application-initiated splice until LDK is guaranteed to remember it, so that a startup pass can release wallet inputs held for splices lost to a crash, and failure events can say which operation failed and why.
LDK persists a splice only once negotiation reaches
AwaitingSignatures; rounds short of that are failed on reload through theSpliceNegotiationFailed/DiscardFundingevents aChannelManagerwrite records alongside itself. But a splice initiated after the last manager write leaves no trace at all — no event ever comes. Once #1037 lands and splice contributions lock wallet inputs, a splice lost in that window would leave its inputs reserved forever, with nothing left running that knows to release them.What changes
Intent record. The splice is written into its pending payment record (from #1079) before the contribution is handed to LDK, and settled once the splice locks, its failure surfaces, or its channel closes. Each splice gets a record of its own — a channel may carry several, a pending splice and the splices queued behind it. Only a fee bump joins an existing record, replacing the intent of the round it replaces; a submission is refused while the channel carries an intent anchored at another funding.
Signing-time record, tracked. #1057 records each negotiated round into the funding payment while handling
Event::FundingTransactionReadyForSigning, beforefunding_transaction_signedreleases ourtx_signatures. Here that write goes through the tracker, under the lock that serializes splice submissions, and the round is filed under the intent whose contribution it carries, so the payment moves from intent to funding under one id.Abort on signing failure. If signing fails or LDK refuses the signed transaction, the splice is aborted through
cancel_funding_contributed. LDK'sDiscardFundingandSpliceNegotiationFailedthen release the contribution, settle the intent, and (through #1057's handling) take back the signing-time record of the refused round. Previously the handler logged and left the negotiation dangling.Failure events.
Event::SpliceNegotiationFailedgains the failure'sreason(mirroring LDK's negotiation-failure reasons) and the initiating request'sparameters(In/Out/FeeBump).reasonis always set;parametersisNonewhen the failure matches no recorded intent. Both are new odd TLVs, so events written by v0.7.0 still read and v0.7.0 readers skip them.Startup reconciliation. Runs before syncing and event processing, checking each record against the reloaded channel:
AwaitingSignatures, resumed on reconnect, orNegotiatedwith our contribution — keeps its record. Only inputs no surviving candidate spends are released; a fee bump lost with the restart may have reserved extras.Recovery is silent — the initiating call already returned and the channel shows no pending splice, so no failure event is fabricated. The failures LDK replays on reload are consumed before the node is marked running.
Notes for reviewers
DiscardFundinghandler that releases a discarded contribution's inputs, and a companion commit proposed there makes coin selection stage its locks so they reach disk only with the intent record.funding_contributedLDK refuses as a failed splice also queues aSpliceNegotiationFailed, so the call both returns an error and surfaces a failure event. For a splice-in or splice-out the event matches no intent by then; for a fee bump built from the round's own inputs it matches the intent of the round being bumped and settles it early, while the round itself is unaffected.Last in the PR stack replacing #930 for this release, per the discussion there; stacked on #1057 → #1079 (funding payment model).
Developed with assistance from Claude Code.