Add ability to buy reservations for instant outs from server - #1195
Draft
hieblmi wants to merge 35 commits into
Draft
Add ability to buy reservations for instant outs from server#1195hieblmi wants to merge 35 commits into
hieblmi wants to merge 35 commits into
Conversation
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.
Limit active reservation state machines, remove terminal entries from memory, and count recovered entries toward the same bound.
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.
Compare reservation expiry with the current height during recovery and use the HTLC path when the remaining window is too short.
Carry the accepted quote into each request, persist it, and reject invoices above that limit while retaining millisatoshi precision.
Record the reservation and Instant Out validation, recovery, fee-limit, and lifecycle updates in the next release notes.
When loopd is started without --experimental the swap client server's reservationManager and instantOutManager are nil. ListReservations already returns codes.Unimplemented in that case; the rest of the instant-out / reservation RPC family didn't, and would dereference a nil pointer. Affected handlers (all of which now return the same Unimplemented status): - ReservationRequest (new in PR lightninglabs#883) - ReservationQuote (new in PR lightninglabs#883) - InstantOut - InstantOutQuote - ListInstantOuts Without this fix an authenticated caller can crash the daemon by invoking any of these RPCs against a non-experimental loopd. With default localhost binding the attack surface is small, but loop is also commonly fronted by lit / LSP wrappers that expose RPCs to other internal services, so a single packet is enough for a remote DoS.
The InstantOut RPC accepts a caller-controlled dest_addr that becomes
the output of the cooperative sweepless sweep (and of the htlc success
sweep on the fallback path), so it is a fund-moving operation equivalent
to LoopOut. Until now it required only swap:execute, while LoopOut
requires both swap:execute and loop:out. A macaroon scoped to
swap:execute -- intended for, say, an autoloop scheduler or a quote
poller -- could therefore drain reservation balances to an attacker
address. ReservationRequest is analogous on the inbound side: it
triggers an outgoing LN prepayment, so it also belongs behind loop:out.
We also harden the address handling in instantout.Manager.NewInstantOut
to match validateLoopOutRequest:
- sweepAddr.IsForNet(m.cfg.Network) is now enforced. btcutil
.DecodeAddress is more permissive than IsForNet for some formats
(notably anything that happens to share a network prefix); without
the explicit network check cross-chain copy-paste mistakes parse
silently and then sign over an unspendable output.
- The address must be one of the formats Loop normally accepts: P2TR /
P2WSH / P2WPKH / P2SH / P2PKH. Anything else (e.g. a future address
type that the user's wallet would otherwise interpret differently)
is rejected up front rather than failing later in the signing path.
InstantOutQuote and ReservationQuote stay on swap:read since they are
read-only.
InitFromClientRequestAction validates that the server-returned absolute expiry is within +/- expiryDelta of expectedExpiry = relativeExpiry + heightHint. Both sides were uint32, so when expectedExpiry < expiryDelta (low regtest heights, fresh deployments, anything with heightHint = 0 like the existing test fixtures) expectedExpiry - expiryDelta wrapped to ~2^32. The lower bound check then trivially admitted any reasonable response, and the client would accept e.g. Expiry = 0 from the server, immediately past the reservation's own deadline -- meaning the server can sweep via the expiry script path while the client still believes it owns the reservation slot. Promote the comparison to int64 so the arithmetic is sign-honest. This is the smallest patch that closes the underflow; a follow-up should also add an absolute floor (e.g. Expiry >= heightHint + minSafeExpiry) so the server cannot return a near-deadline reservation even within the delta.
RequestReservationFromServer dispatched the OnClientInitialized event to the manager's Run loop via a bare 'm.reqChan <- ...' send. reqChan is an unbuffered channel; if Run had already returned (e.g. because the block epoch subscription errored, or the manager is shutting down) the send would block forever, holding the gRPC handler goroutine and the caller's connection open until something external killed it. Wrap the send in a select that also watches the caller's context. A cancelled caller context now returns ctx.Err() instead of hanging. Note: this still does not detect "Run exited cleanly while reqChan was empty" -- doing that requires exposing Run's runCtx (or a quit channel) on the Manager struct. That refactor is left for a follow-up; the caller-side cancel path above is enough to keep RPC handlers from leaking when their grpc deadline fires.
The reservation new command printed the prepay cost and asked the user
to confirm with 'y/n'. The implementation read the answer with
fmt.Scanln(&answer) and treated only the literal 'n' as a 'no'. The
return value was discarded, so:
- On EOF / closed stdin (CI pipelines, automated wrappers, terminal
disconnect) Scanln returned an error and answer remained the empty
string, which is not 'n', so the command proceeded and paid the
LN prepayment with no user confirmation.
- The case-sensitive 'n' check also accepted 'N', 'no', 'yes', 'Y',
or any other string as a 'yes'.
Match the convention used by the rest of the loop CLI: only continue
when the user typed exactly 'y' (or 'Y'), and treat any read error as
'no'.
InitFromClientRequestAction wrote the new reservation row via Store.CreateReservation while reservation.State was still the zero value (fsm.EmptyState) returned by NewReservation. The GetClientInitiatedReservationStates() state map has no OnRecover transition on EmptyState. If the daemon crashed (or the context was cancelled) any time after CreateReservation returned, the row was permanently stuck: on restart RecoverReservations rebuilt the FSM at state "", SendEvent(OnRecover) returned "event not allowed", and the goroutine just logged and gave up. The HD key index was wasted; the server-side reservation was left orphan. Set reservation.State = Init before persisting. The Init state already has OnRecover: Failed, so a crashed-mid-Init reservation now recovers cleanly into Failed on the next start. updateReservation's existing skip-list keeps the immediately-following SendPrepaymentPayment transition working as before (it skips writes while in Init). A follow-up should also notify the server to cancel orphaned reservations from Failed.OnRecover; that requires plumbing a cancel-RPC into the client-initiated state map.
Migration 000014_reservation_protocol_version used 'protocol_Version' (capital V) in the ADD COLUMN / DROP COLUMN statements. Postgres folds unquoted identifiers to lowercase and SQLite is case-insensitive on identifier comparison, so the running schema column is 'protocol_version' either way -- but the sqlc-generated Go (loopdb/sqlc/reservations.sql.go) also uses the lowercase form, so the file as written was both unusual and inconsistent with its own generated SQL. Use 'protocol_version' everywhere. No data migration is required; the column on disk is unchanged. Pure cosmetic / portability fix.
RequestReservationFromServer blocked for defaultWaitForStateTime (15s)
waiting for the FSM to reach SendPrepaymentPayment. Reaching that state
requires, in order:
- Wallet.DeriveNextKey (local lnd round-trip)
- server's RequestReservation gRPC (network + server's own lnd invoice
creation, including hold-invoice persistence)
- LightningClient.DecodePaymentRequest
- Store.CreateReservation
15 seconds was achievable on a fast LAN with idle servers, but under
even modest load (server-side hold-invoice creation can routinely take
several seconds in the wild) the timer expired and the RPC returned an
error to the caller. The FSM kept running in the background and the
wallet would still pay the prepay LN invoice -- so the user got an
error, but their funds still moved. The next call to the same
reservation_id would then fail mysteriously because the server-side
state was already advanced.
Bump to 60s. The right longer-term fix is to plumb the caller's gRPC
context into the FSM SendEvent so cancellation actually aborts the
in-flight server call instead of orphaning it; that's a larger refactor
and is left as a follow-up.
SendPaymentAndPollAccepted and BuildHtlc both run after PollPaymentAcceptedAction has called LockReservation on every reservation backing the swap. Their OnRecover transitions pointed directly to Failed, whose action is fsm.NoOpAction -- so on daemon restart while in either state, the FSM moved to Failed without ever unlocking the reservations. The local store kept them in the Locked state until on-chain expiry (typically tens of hours later), making them unusable for any subsequent swap. For users who pay for reservations (PR lightninglabs#883's invoice-requested flow) that is a direct material loss. Add an intermediate UnlockReservationsOnRecover state whose action calls handleErrorAndUnlockReservations and then routes to Failed via the normal OnError edge. SendPaymentAndPollAccepted.OnRecover and BuildHtlc.OnRecover now point at this state instead of Failed directly. Init.OnRecover -> Failed is left alone because at that point the InstantOut row has not yet been persisted and no reservation locks have been taken; there is nothing to clean up. Post-PushPreimage states (PushPreimage.OnRecover -> PushPreimage, etc.) are also left alone since they self-loop on recovery rather than terminate. The cleanup helper itself still derives its context from the caller's context (see existing handleErrorAndUnlockReservations); fixing that context-cancel hazard is a separate change.
handleErrorAndUnlockReservations is called specifically from error paths
and from the new OnRecover cleanup. In practice the caller's ctx is
almost always already canceled by the time we get here (caller timeout,
daemon shutdown, ctx.Done() arm in PollPaymentAcceptedAction, etc.). The
existing implementation derived its 30s timeout context from that
canceled parent, so:
- The for-loop calling UnlockReservation immediately hit ctx.Err() ==
context.Canceled on every reservation. Locks were never released on
disk.
- The goroutine sending CancelInstantSwap to the server captured the
same already-canceled ctx, then further wrapped it in WithTimeout
(still canceled). The server never heard about the cancel.
Both code paths were no-ops in exactly the scenario they were written
for. Switch to context.Background() with a fresh 30s timeout so the
cleanup actually runs. The goroutine also gets its own background
context (the previous code captured the parent's already-done ctx via
closure, then re-wrapped it).
Run wrote m.currentHeight = height without holding the lock, while later writes (newBlockChan case) and reads in RequestReservationFromServer take m.Lock. Daemon startup serializes 'wait for initChan' before serving RPC, so in practice the race window is short, but the race detector flags it -- and on the nautilus side a similar pattern is the most plausible cause of the unit-race CI failure on the buy-reservations head commit. Symmetric fix here keeps the synchronisation rule uniform.
Current master already uses migration 15 for static address withdrawals. Move the reservation prepay migration to the next available version so database initialization does not reject the duplicate version.
The security tests use the pre-feature helper names. Update them for the client-requested reservation manager API after combining both PR stacks.
hieblmi
force-pushed
the
codex/buy-reservations-rebased
branch
from
August 11, 2026 13:03
45eec40 to
fb6e610
Compare
Regenerate the command reference after adding reservation quote and purchase commands.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
loop reservations newCLImasterMotivation
Instant Out currently depends on reservation UTXOs proposed by the server. This
change also lets a client request and pay for a reservation with a chosen value
and relative expiry.
Rebase
This PR supersedes #883. It is rebased onto the complete agentic-security stack
from #1194, which is now an ancestor of this branch. Where the older reservation
stack contained an equivalent notification-handling fix, that duplicate was
dropped in favor of the #1194 implementation.
The combined branch also regenerates the RPC and CLI documentation artifacts
and adapts the security tests to the protocol-versioned reservation manager.
Companion server change
Validation
go test ./instantout/... ./loopd ./cmd/loopgo test ./...fromlooprpcgo test ./...fromswapserverrpcmake rpcmake commitmsg-lint range=origin/master..HEADgit diff --check origin/master...HEAD