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.
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.
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).
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 #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.
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.
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.
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.
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'.
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.
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.
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.
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 #883)
- ReservationQuote (new in PR #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.
Validate the bid rate before returning an accepted asset sell quote.
This prevents malformed rates from reaching downstream quote arithmetic,
where nil or non-positive values can panic. Cover valid and malformed
responses with table-driven tests.
Validate the rate pointer and decimal coefficient before converting
asset units. Return errors for nil, malformed, non-positive, and
oversized-scale rates instead of allowing nil dereferences or
division-by-zero panics. Add regression tests for each case.
Restrict the asset-name cache mutex to map access so a slow
QueryAssetStats call cannot block cached readers. Use an RWMutex for
independent cache reads and add a concurrent regression test.
Convert the configured duration once during client creation. Round
positive fractional durations up to the whole seconds accepted by tapd.
Reject zero, negative, and overflowing values, and cover the conversion
boundaries with unit tests.
Close the TapdClient when daemon initialization fails, during normal
shutdown, and after the view command completes. This prevents gRPC
transport resources from leaking across embedded daemon lifecycles and
error paths.
Bumps both pins together: the gateway-action SHA and the runtime_ref it
resolves. runtime_ref is pinned explicitly rather than left to the
action's default, so bumping only the action would leave the job on the
v0.5.0 runtime.
v0.6.0 adds no trigger and no input, so the rest of the shim is
unchanged.
Also corrects a copy-paste artifact in the adjacent comment, which said
runtime upgrades go through an lnd PR.
