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.
Document every published Loop release and preserve authoritative notes.
Add the next-release workflow and rebuild chronological navigation.
Rename the reproducible-build guide for clarity.
