instantout: improve reservation and swap handling - #1194
Conversation
44c544a to
4f07354
Compare
|
/gateway review |
2 similar comments
|
/gateway review |
|
/gateway review |
![lightninglabs-gateway[bot]](https://avatars.githubusercontent.com/in/3635431?s=60&v=4){kind=small}
There was a problem hiding this comment.
Gateway review — 10 findings
🔴 1 Blocker · 🟠 7 Major · 🟡 2 Minor · 🔵 0 Nit
Summary
This PR hardens the instant-out and reservation paths against a misbehaving server: length/nil validation on server-controlled MuSig2 vectors, local script-engine verification of the combined signature, MuSig2 session cleanup on every exit path, exact-value matching of the reservation output, duplicate and resource bounds on reservation notifications, a client-side swap-fee cap, and consistent macaroon scoping. The direction is right and the new test coverage is genuinely targeted at the new logic.
Three things need attention before merge. First, registering finalStateObserver turns a pre-existing unlocked map write in RecoverReservations into a live data race that aborts the process at startup. Second, the new resource bound is enforced fail-closed during recovery, so exceeding it permanently prevents the reservation subsystem from starting — worse than the exhaustion it defends against, and inconsistent with the same change making the notification loop resilient. Third, the fee cap is weaker than it reads: its only value comes from the server's own quote, and its wire default of 0 rejects every fee-bearing swap for any client that isn't rebuilt.
The recovery gate in PushPreimageAction also deserves a second look. It is keyed on the dynamic type of the event context and compares against reservation expiry only, so on the paths and heights where revealing the preimage is most dangerous it either isn't reached or passes anyway.
Bot commands
/gateway re-review— re-run after pushing changes (maintainers)/gateway dismiss <id>— silence a finding (maintainers)/gateway explain <id>— elaborate on a finding (anyone)
| @@ -174,6 +241,11 @@ func (m *Manager) RecoverReservations(ctx context.Context) error { | |||
| reservationFSM := NewFSMFromReservation(m.cfg, reservation) | |||
|
|
|||
| m.activeReservations[reservation.ID] = reservationFSM | |||
There was a problem hiding this comment.
🔴 F1 (Blocker) — Reservation recovery races the new observer into a fatal map write · instantout/reservation/manager.go:243
RecoverReservations writes m.activeReservations[reservation.ID] = reservationFSM without holding m.Mutex, and this PR now registers a finalStateObserver on each recovered FSM whose Notify deletes from that same map under the lock, from the FSM's own goroutine. With two or more non-final persisted reservations where the first reaches a terminal state while the loop is still inserting later entries, the Go runtime kills the process with an unrecoverable fatal error: concurrent map writes. RecoverReservations is called from Manager.Run, so this is a crash on the loopd startup path.
Why this matters
The unlocked write predates this PR and was benign, because nothing removed entries from activeReservations during recovery; the observer is what introduces the concurrent mutator. A node that has been offline past a reservation expiry hits exactly this shape — the first recovered reservation finalizes almost immediately on OnRecover while the loop continues — which makes it a restart crash loop rather than a narrow timing window.
Take m.Lock() around the insert in the recovery loop, mirroring the guarded insert newReservation already performs; registering the observer and spawning the goroutine after the unlock is still safe, since the goroutine cannot run before it is created. Note that the identity check o.manager.activeReservations[o.id] == o.fsm in Notify does not help here — the race is on the map itself, not on the value. Blame on the surrounding lines is 4430627 (PR 839) and 61a5f9d (PR 632); the map's locking discipline has been inconsistent since it was introduced, and this is the change that makes it load-bearing.
There was a problem hiding this comment.
Fixed in 42748896. RecoverReservations now holds the manager mutex while inserting each recovered FSM, matching the locking used by observer-driven removal and all other activeReservations access. TestManagerRecoversAllPersistedReservations exercises concurrent terminal observer removals, and the reservation suite passes under -race.
| ReservationIds: selectedReservations, | ||
| OutgoingChanSet: outgoingChanSet, | ||
| DestAddr: cmd.String("addr"), | ||
| MaxSwapFeeSat: quote.ServiceFeeSat, |
There was a problem hiding this comment.
🟠 F2 (Major) — The client fee cap is chosen by the server it is meant to bound · cmd/loop/instantout.go:193
MaxSwapFee is documented as "the maximum off-chain swap fee accepted by the client", but the only caller that sets it is the CLI, which echoes back quote.ServiceFeeSat from the server's own InstantOutQuote response with no flag to override it. A server that wants to overcharge simply quotes a large ServiceFeeSat; the CLI adopts it as the ceiling and validateInstantOutInvoiceAmount accepts the matching invoice. The check therefore detects only a server that contradicts its own quote, not one that quotes an extortionate fee — the fund-loss case the cap exists for is not bounded.
Why this matters
Compare loop out, where the client supplies max_swap_fee / max_miner_fee limits independently of what the server quotes. The user's only protection here remains the printed Service fee: line and the CONTINUE SWAP? (y/n) prompt, which is what already existed before this PR.
Second, the cap has zero tolerance: validateInstantOutInvoiceAmount rounds the observed fee up to the next satoshi and rejects at one millisatoshi over, so any drift between the quote call and the swap call fails the swap with instant out swap fee N exceeds maximum M and no operator remedy. A --max_swap_fee flag that defaults to the quoted fee (optionally with a small documented tolerance, or a percentage-of-amount ceiling the quote must fit inside) would make the cap a real client-side limit and give the user a way out of a repriced quote.
There was a problem hiding this comment.
No code change for this finding. The in-tree CLI displays the quote, requires explicit confirmation, and then supplies that exact accepted service fee as the cap. Keeping the cap exact avoids silently accepting a different fee; if the quote changes, the intended flow is to quote and confirm again. A separate tolerance flag would be a product/API enhancement rather than a correctness change for this PR.
| // A recovered swap may have been offline long enough that the server's | ||
| // reservation timeout is now close. Fall back to the already finalized | ||
| // HTLC instead of revealing the preimage without enough time to publish | ||
| // that safety transaction. |
There was a problem hiding this comment.
🟠 F3 (Major) — Preimage-reveal gate is keyed on the event context's dynamic type · instantout/actions.go:427
The new safety gate in PushPreimageAction runs only when eventCtx type-asserts to *RecoverInstantOutCtx, and that type is constructed in exactly one place — recoverInstantOuts — for a single SendEvent(ctx, OnRecover, recoverCtx). A swap recovered in an earlier state that walks forward through BuildHTLCAction (which returns OnHtlcSigReceived with no event context of its own) therefore reaches PushPreimageAction without that type and reveals the preimage with no expiry check at all — precisely the long-offline case the gate was added for.
Why this matters
I cannot confirm from the diff how fsm propagates event contexts across transitions triggered by an action's returned event type, and the FSM's state/transition table is not in the reviewed context; if internal transitions forward the originating event context, the gate holds for those paths. TestPushPreimageRejectsExpiringReservation calls PushPreimageAction directly with a *RecoverInstantOutCtx, so it cannot distinguish the two cases either.
Deriving the safety condition from durable state — the manager's current height plus fields already on InstantOut — and checking it unconditionally in PushPreimageAction would remove the dependence on how the state was entered. Revealing the preimage is irreversible in one direction only, so this check should fail closed regardless of the event path.
There was a problem hiding this comment.
No code change for this finding. SendEvent retains the same event context through action-driven transitions, so a recovered PushPreimageAction receives RecoverInstantOutCtx both when recovery starts in PushPreimage and when BuildHtlc advances into it. Recovery from Init or SendPaymentAndPollAccepted transitions directly to Failed, so those paths do not reach this action.
| // reservation timeout is now close. Fall back to the already finalized | ||
| // HTLC instead of revealing the preimage without enough time to publish | ||
| // that safety transaction. | ||
| if recoverCtx, ok := eventCtx.(*RecoverInstantOutCtx); ok { |
There was a problem hiding this comment.
🟠 F4 (Major) — Recovery gate ignores the swap's own CLTV expiry · instantout/actions.go:428
The gate requires res.Expiry >= currentHeight + htlcExpiryDelta but says nothing about InstantOut.CltvExpiry, so it permits revealing the preimage at heights well past the HTLC's own timeout — where the OnErrorPublishHtlc fallback is worthless. Once the tip is at or beyond cltvExpiry, publishing the finalized HTLC gives the client no blocks to sweep it with the preimage while the server's timeout branch is already (or imminently) spendable, and the server has the preimage from PushPreimage.
Why this matters
Concretely: InitInstantOutAction enforces res.Expiry >= cltvExpiry + htlcExpiryDelta, so with cltvExpiry = 200 and res.Expiry = 400 the gate keeps passing until height 360 — 160 blocks after the HTLC expired. The two deltas are not interchangeable: htlcExpiryDelta is documented as the margin "between the htlc expiry and reservation expiry", not between the current tip and reservation expiry.
The check should also require the tip to sit some margin below f.InstantOut.CltvExpiry before revealing, and fall back to the HTLC (or a terminal error) otherwise. I have not traced the HTLC-sweep deadline in PublishHtlcSweepAction / generateHtlcSweepTx in detail, so the exact margin needed is for the author to set; the missing condition itself is visible in the diff.
There was a problem hiding this comment.
Fixed in 6077a269. Recovery now fetches a fresh chain height inside PushPreimageAction, verifies that CltvExpiry leaves two urgent confirmation targets (one for the HTLC and one for its preimage sweep), and selects the HTLC path before revealing the preimage when that window is too short. A focused test covers the HTLC deadline.
| fsm.WithWaitForStateOption(time.Second), | ||
| ) | ||
| if err != nil { | ||
| m.Lock() |
There was a problem hiding this comment.
🟠 F5 (Major) — Timed-out reservation is dropped from the map while its FSM keeps running · instantout/reservation/manager.go:197
When WaitForState(ctx, 5*time.Second, WaitForConfirmation, ...) fails, newReservation now deletes the entry from activeReservations, but the goroutine started just above keeps driving OnServerRequest and may still persist and advance that reservation. The reservation becomes invisible to LockReservation / UnlockReservation / GetActiveInstantOut until the next restart even though it can confirm and hold funds, and it no longer counts toward the new len(m.activeReservations) >= maxActiveReservations bound.
Why this matters
That second effect defeats the bound this PR is adding: a server that keeps each notification just slow enough to miss the 5s deadline drives an unbounded number of live FSMs and persisted non-final rows past the cap, and those same rows are what RecoverReservations counts to decide whether to refuse startup (see the recovery-abort finding).
Either cancel/abort the FSM when the wait fails, or keep the entry until the FSM actually reaches a final state and let the observer evict it — the mechanism this PR already added for exactly that purpose.
There was a problem hiding this comment.
Fixed in 42748896. A WaitForState timeout no longer deletes the active map entry while initialization can continue on the manager context. Terminal-state observation is now the authoritative eviction path. A delayed-initialization test verifies that the FSM remains tracked after the caller's wait times out and can still advance.
| activeCount++ | ||
| } | ||
| } | ||
| if activeCount > maxActiveReservations { |
There was a problem hiding this comment.
🟠 F6 (Major) — Exceeding the reservation cap permanently prevents startup · instantout/reservation/manager.go:228
RecoverReservations returns ErrTooManyActiveReservations when more than 1000 stored reservations are non-final, and that error propagates out of Manager.Run — the function that starts the reservation subsystem. Once a node's store crosses that line, the manager never starts again: no reservation is recovered, no notification is served, and funds committed to in-flight reservations go unmanaged until they expire. Since reservations only reach a final state by the manager running, there is no path back short of editing the database.
Why this matters
This is the one place the new bound fails closed. The in-memory bound is already enforced where it matters, in newReservation, and the same change makes the Run loop log-and-continue instead of returning — the right shape. Recovering up to the cap (nearest-expiry first) and logging the remainder with the observed count would degrade service instead of bricking the subsystem. Making the limit an operator-visible config value with a documented default of 1000, rather than an unexported constant, would also give an affected node a way out without a rebuild.
Two smaller points in the same block: the boundary disagrees with newReservation, which rejects at >= maxActiveReservations while this path aborts at > maxActiveReservations; and this branch has no test — TestManagerLimitsActiveReservations covers the newReservation path only.
There was a problem hiding this comment.
Fixed in 42748896. All persisted non-terminal reservations are now resumed, even when their count exceeds the live-notification cap; the cap applies only to newly accepted notifications. The >/>= comparison already allowed exactly 1000 in both paths, so the substantive fix was removing the startup rejection for persisted obligations. A test recovers maxActiveReservations + 1 entries.
| /* | ||
| The maximum off-chain swap fee that may be charged for the swap. | ||
| */ | ||
| int64 max_swap_fee_sat = 4; |
There was a problem hiding this comment.
🟠 F7 (Major) — Unset max_swap_fee_sat is enforced as a zero cap · looprpc/client.proto:1692
max_swap_fee_sat is a plain proto3 int64 with no presence tracking, so every client that does not set it sends 0, loopd forwards btcutil.Amount(0) into NewInstantOut, and validateInstantOutInvoiceAmount then fails the swap as soon as the invoice charges a single millisatoshi of fee. Instant out breaks outright for any caller not rebuilt against this PR — lightning-terminal, RTL, scripts against the gRPC/REST surface — and the failure arrives as the opaque instant out swap fee N exceeds maximum 0 from inside the FSM rather than as an argument error naming the field.
Why this matters
Nothing distinguishes "the client approved a zero fee" from "the client expressed no opinion": NewInstantOut rejects only negative values, and the proto comment says nothing about the zero value — that comment is also the entire documentation REST callers see via client.swagger.json. The in-tree CLI is updated, so this break is invisible from inside the repo, and InstantOut (*swapClientServer) is the highest-reach symbol touched here.
Either give the field presence (optional int64) and treat unset as "no client-side cap", or reject unset/zero at the RPC boundary with status.Error(codes.InvalidArgument, ...) before calling NewInstantOut, and document the chosen semantics in the proto comment. Two related items worth handling in the same pass: NewInstantOut's signature change is breaking for in-process consumers and isn't called out in the release notes, and now that MaxSwapFee is read back from the store, rows written before this PR report 0 indistinguishably from a genuine zero cap.
There was a problem hiding this comment.
Fixed in 6f3aa345. The RPC field is now a presence-aware oneof that retains the original scalar wire representation: omitted means legacy behavior with no cap, while an explicitly encoded zero remains a strict zero-fee cap. The RPC layer adds WithMaxSwapFee only when present, and NewInstantOut uses a variadic option so existing three-argument Go callers still compile. A wire-level test covers omitted versus explicit zero.
| Entity: "swap", | ||
| Action: "read", | ||
| }, { | ||
| Entity: "loop", |
There was a problem hiding this comment.
🟠 F8 (Major) — New macaroon op breaks existing scoped macaroons · looprpc/perms.go:181
Appending {Entity: "loop", Action: "out"} makes it a second required op on ListReservations, InstantOut, InstantOutQuote and ListInstantOuts, since all ops listed for a method must be satisfied together. Any already-issued custom-baked macaroon scoped to only swap:read / swap:execute stops working against these four RPCs the moment the operator upgrades, and the failure surfaces as a generic permission-denied with nothing pointing at the newly added entity.
Why this matters
The end state is defensible — it matches the LoopOut / LoopOutQuote / SweepHtlc siblings and closes a real under-scoping gap — so this is about how the change lands, not whether it should. But loopd documents custom baked macaroons as a supported workflow, and the release-notes entry files this under #### Bug Fixes while #### Breaking Changes is left empty, so an operator has no way to learn they must re-bake.
Add an entry under #### Breaking Changes naming the four methods and the required loop:out op, so the remediation is mechanical rather than a bisect against permission-denied errors. That entry is also missing the #1194 link that the neighbouring PR #1189 entry carries.
There was a problem hiding this comment.
Fixed in 920e2384. The Breaking Changes section now lists the four affected Instant Out/reservation RPCs, the added loop:out permission, and the need for operators using custom scoped macaroons to rebake them.
| // As SendEvent can block, we'll start a goroutine to process | ||
| // the event. | ||
| recoverCtx := &RecoverInstantOutCtx{ | ||
| currentHeight: m.currentHeight, |
There was a problem hiding this comment.
🟡 F9 (Minor) — Recovery height is a stale, unsynchronized snapshot · instantout/manager.go:123
recoverInstantOuts snapshots m.currentHeight without holding m.Lock() (the block-epoch loop writes it under the lock) and this value now feeds the preimage-reveal gate; because the loop only starts after recovery, the height is whatever was passed to NewInstantOutManager and is never refreshed for the recovered swap. A height lower than the real tip yields a lower minReservationExpiry and makes the gate less conservative, which is the wrong direction for a safety check — read the height under the mutex, and prefer evaluating it when the action runs rather than when the event is queued.
There was a problem hiding this comment.
Addressed the valid portion in 6077a269. Recovery runs before the manager enters its block-update loop, so the former read was not concurrent with that loop's writes; however, the startup snapshot could still be stale. The snapshot has been removed, and the action now fetches the chain height immediately before both recovery deadline checks.
| defer o.manager.Unlock() | ||
|
|
||
| if o.manager.activeReservations[o.id] == o.fsm { | ||
| delete(o.manager.activeReservations, o.id) |
There was a problem hiding this comment.
🟡 F10 (Minor) — Eviction makes activeReservations lookups newly fallible · instantout/reservation/manager.go:47
Entries now disappear once a reservation reaches a terminal state, so LockReservation / UnlockReservation, which return fmt.Errorf("reservation not found") on a map miss, can now fail where they previously always found the entry — notably in handleErrorAndUnlockReservations, which turns that miss into a further FSM error for a swap whose reservations have already gone final. Worth confirming those call sites treat a missing entry as benign rather than as a failure.
There was a problem hiding this comment.
Fixed in 42748896. If an active entry has already been evicted, UnlockReservation checks persisted state and treats a terminal reservation as an idempotent success while still returning an error for unknown or non-terminal missing entries. Cleanup now attempts every unlock, preserves the original action error, and always dispatches cancellation with its own bounded uncancelled context. Focused tests cover terminal eviction and cleanup-error behavior.
|
🤖 gateway audit metadata for this PR — auto-generated, please don't edit. |
4f07354 to
920e238
Compare
|
/gateway re-review Handover for the next pass:
Validation completed: |
There was a problem hiding this comment.
Gateway review — 3 findings
🔴 0 Blocker · 🟠 0 Major · 🟡 3 Minor · 🔵 0 Nit
Summary
Eight of the ten prior findings are fixed, each with a focused test. The reservation-lifecycle rework is the substantive one: the recovery insert now holds the manager mutex (removing the concurrent map writes startup crash), terminal-state observation is the sole eviction path so a caller-side wait timeout no longer orphans a live FSM, all persisted non-terminal reservations are resumed regardless of the new cap, and terminal unlocks are idempotent while cleanup preserves the original action error. The recovery gate now reads a fresh chain height and checks both the HTLC and reservation deadlines before releasing the preimage, and the fee cap is presence-aware at the wire level with the macaroon change documented under Breaking Changes.
Two priors stay open at reduced severity rather than as blockers. The fee cap is still exactly the server's own quote — I accept the rationale that the CLI's printed fee plus y/n confirmation makes it a user-approved ceiling, leaving only the non-interactive path as a follow-up. The recovery gate still keys off the event-context type; that one depends on fsm transition semantics I cannot see in the reviewed context, so it is recorded as a defense-in-depth note, not a defect.
One genuinely-new minor item falls out of the fix I asked for: retaining the FSM after a wait timeout means nothing evicts an entry whose server call never returns. Separately, I checked the new per-input script verification in finalizeMusig2Transaction — verifying input idx before later inputs' witnesses are set is safe, since taproot sighashes commit to prevouts, amounts, scripts, sequences and outputs but not to other inputs' witness data.
Status of prior findings
- F1 addressed: Fixed in
instantout/reservation/manager.go:232— the recovery insert is now wrapped inm.Lock()/m.Unlock(), matching the guarded insert innewReservation, andTestManagerRecoversAllPersistedReservationsexercises concurrent observer-driven removal. - F4 addressed: Fixed in
instantout/actions.go:452— recovery now requiresCltvExpiry >= currentHeight + htlcRecoverySafetyDelta(one urgent conf target for the HTLC, one for its preimage sweep) before revealing, covered byTestPushPreimageRejectsExpiringHtlc. - F5 addressed: Fixed in
instantout/reservation/manager.go:200— the wait-timeout path no longer deletes the map entry, andTestManagerKeepsReservationAfterWaitTimeoutconfirms the FSM stays tracked and can still reachWaitForConfirmation. - F6 addressed: Fixed in
instantout/reservation/manager.go:220—RecoverReservationsno longer consultsmaxActiveReservations, so the cap applies only to new notifications and the subsystem can always start. - F7 addressed: Fixed in
looprpc/client.proto:1688—max_swap_fee_satis now aoneofmember, so omitted means no cap while an explicit zero is preserved;NewInstantOuttook a variadic option to keep existing Go callers compiling, andlooprpc/client_test.goasserts the wire-level distinction. - F8 addressed: Fixed in
docs/release-notes/release-notes-next.md:7— the Breaking Changes section names the four RPCs, the addedloop:outop, and the need to rebake custom scoped macaroons, with the#1194link. - F9 addressed: Fixed in
instantout/actions.go:437— the stalem.currentHeightsnapshot is gone;RecoverInstantOutCtxis now an empty marker and the action fetches the height viaGetInfoimmediately before both deadline checks. - F10 addressed: Fixed in
instantout/reservation/manager.go:297—UnlockReservationconsults persisted state on a map miss and treats a terminal reservation as success, while cleanup attempts every unlock and returns the action error in preference to the unlock error.
Bot commands
/gateway re-review— re-run after pushing changes (maintainers)/gateway dismiss <id>— silence a finding (maintainers)/gateway explain <id>— elaborate on a finding (anyone)
Apply the loop:out permission to Instant Out and reservation RPCs so their authorization requirements match the rest of the Loop Out API.
Log individual reservation initialization failures and continue consuming later notifications instead of stopping the manager.
Use a goroutine-local result for event dispatch so observer errors remain independent and initialization outcomes stay deterministic.
Check active and persisted reservations before creating a new state machine, preserving the existing reservation when a duplicate arrives.
920e238 to
84f3bdf
Compare
Limit newly accepted reservation state machines, resume all persisted reservations, remove terminal entries from memory, and make cleanup resilient to observer-driven eviction.
Compare each confirmed transaction output with the expected reservation amount before advancing the state machine.
Check nonce, signature, session, and transaction input counts before indexing signing vectors, returning clear errors for incomplete data.
Run script validation for every combined signature before accepting a finalized transaction, surfacing invalid witnesses immediately.
Clean up abandoned signing sessions on error paths while leaving completed sessions to lnd.
Refresh the chain height when a swap resumes, verify both the reservation and HTLC windows, and select the HTLC path when either remaining window is too short.
Carry the accepted quote into each request, persist it, and reject invoices above that limit. Preserve compatibility for requests that omit the cap while distinguishing an explicit zero.
Record the Instant Out and reservation validation, recovery, fee-limit, lifecycle, and custom macaroon updates in the next release notes.
84f3bdf to
f826260
Compare
3 participants
Summary
Motivation
Instant Out and reservation flows currently assume several inputs and lifecycle transitions are well formed. That can leave state machines stuck, produce inconsistent errors, or make recovery less predictable. These changes make the flows fail cleanly and keep persisted state aligned with chain and RPC data.
User impact
Instant Out requests now enforce the quoted service-fee ceiling, recovered swaps use the on-chain fallback when reservation timing is too tight, and reservation notifications no longer interrupt the manager. The CLI automatically supplies the accepted quote as the fee limit.
Testing