diff --git a/docs/release-notes/release-notes-next.md b/docs/release-notes/release-notes-next.md index ce6f7c64c..9a8fd6269 100644 --- a/docs/release-notes/release-notes-next.md +++ b/docs/release-notes/release-notes-next.md @@ -13,6 +13,10 @@ #### Bug Fixes +* Newly initiated Instant Out swaps are now indexed by their actual swap hash, + and normal payment-stream closure no longer aborts a swap before the server + reports that it accepted the payment. + * Instant Out now attempts to cancel server-side swaps when client initialization fails, allowing locked reservations to be released without waiting for the server timeout. diff --git a/instantout/actions.go b/instantout/actions.go index fa07348e2..31cd31104 100644 --- a/instantout/actions.go +++ b/instantout/actions.go @@ -299,9 +299,20 @@ func (f *FSM) PollPaymentAcceptedAction(ctx context.Context, // We want to poll quickly the first time. timer := time.NewTimer(time.Second) + defer timer.Stop() + for { select { - case payRes := <-payChan: + case payRes, ok := <-payChan: + if !ok { + // The router closes both payment channels after the + // payment stream reaches a terminal state. Stop selecting + // on this channel and let the server poll determine whether + // it accepted the payment. + payChan = nil + continue + } + f.Debugf("payment result: %v", payRes) if payRes.State == lnrpc.Payment_FAILED { return f.handleErrorAndUnlockReservations( @@ -309,12 +320,21 @@ func (f *FSM) PollPaymentAcceptedAction(ctx context.Context, payRes.FailureReason), ) } - case err := <-paymentErrChan: + case err, ok := <-paymentErrChan: + if !ok { + // Channel closure is the normal end-of-stream signal. + paymentErrChan = nil + continue + } + if err == nil { + err = errors.New("payment error channel returned nil") + } + f.Errorf("error sending payment: %v", err) return f.handleErrorAndUnlockReservations(ctx, err) case <-ctx.Done(): - return f.handleErrorAndUnlockReservations(ctx, nil) + return f.handleErrorAndUnlockReservations(ctx, ctx.Err()) case <-timer.C: res, err := f.cfg.InstantOutClient.PollPaymentAccepted( diff --git a/instantout/manager.go b/instantout/manager.go index 9a98020e4..dc47e4ae5 100644 --- a/instantout/manager.go +++ b/instantout/manager.go @@ -166,14 +166,12 @@ func (m *Manager) NewInstantOut(ctx context.Context, sweepAddress: sweepAddr, maxSwapFee: maxSwapFee, } + m.Unlock() instantOut, err := NewFSM(m.cfg, ProtocolVersionFullReservation) if err != nil { - m.Unlock() return nil, err } - m.activeInstantOuts[instantOut.InstantOut.SwapHash] = instantOut - m.Unlock() // Start the instantout FSM. go func() { @@ -193,6 +191,10 @@ func (m *Manager) NewInstantOut(ctx context.Context, return nil, err } + m.Lock() + m.activeInstantOuts[instantOut.InstantOut.SwapHash] = instantOut + m.Unlock() + return instantOut, nil } diff --git a/instantout/payment_test.go b/instantout/payment_test.go new file mode 100644 index 000000000..4b3fab133 --- /dev/null +++ b/instantout/payment_test.go @@ -0,0 +1,148 @@ +package instantout + +import ( + "context" + "testing" + + "github.com/lightninglabs/lndclient" + "github.com/lightninglabs/loop/fsm" + "github.com/lightninglabs/loop/instantout/reservation" + "github.com/lightninglabs/loop/swapserverrpc" + "github.com/lightningnetwork/lnd/lntypes" + "github.com/stretchr/testify/require" + "google.golang.org/grpc" +) + +type paymentTestRouter struct { + lndclient.RouterClient + + statusChan chan lndclient.PaymentStatus + errorChan chan error +} + +func (r *paymentTestRouter) SendPayment(context.Context, + lndclient.SendPaymentRequest) (chan lndclient.PaymentStatus, chan error, + error) { + + return r.statusChan, r.errorChan, nil +} + +type paymentTestReservationManager struct { + ReservationManager +} + +func (m *paymentTestReservationManager) LockReservation(context.Context, + reservation.ID) error { + + return nil +} + +func (m *paymentTestReservationManager) UnlockReservation(context.Context, + reservation.ID) error { + + return nil +} + +type paymentTestInstantOutClient struct { + swapserverrpc.InstantSwapServerClient + + accepted bool + polls int +} + +func (c *paymentTestInstantOutClient) PollPaymentAccepted(context.Context, + *swapserverrpc.PollPaymentAcceptedRequest, ...grpc.CallOption) ( + *swapserverrpc.PollPaymentAcceptedResponse, error) { + + c.polls++ + return &swapserverrpc.PollPaymentAcceptedResponse{ + Accepted: c.accepted, + }, nil +} + +func (c *paymentTestInstantOutClient) CancelInstantSwap(context.Context, + *swapserverrpc.CancelInstantSwapRequest, ...grpc.CallOption) ( + *swapserverrpc.CancelInstantSwapResponse, error) { + + return &swapserverrpc.CancelInstantSwapResponse{}, nil +} + +func newPaymentTestFSM(router lndclient.RouterClient, + client swapserverrpc.InstantSwapServerClient) *FSM { + + return &FSM{ + StateMachine: &fsm.StateMachine{}, + cfg: &Config{ + RouterClient: router, + InstantOutClient: client, + ReservationManager: &paymentTestReservationManager{}, + }, + InstantOut: &InstantOut{ + SwapHash: lntypes.Hash{1}, + Reservations: []*reservation.Reservation{ + {ID: reservation.ID{1}}, + }, + }, + } +} + +// TestPollPaymentAcceptedIgnoresClosedStreams verifies that the normal router +// end-of-stream signal doesn't race with the server's acceptance response. +func TestPollPaymentAcceptedIgnoresClosedStreams(t *testing.T) { + statusChan := make(chan lndclient.PaymentStatus) + errorChan := make(chan error) + close(statusChan) + close(errorChan) + + router := &paymentTestRouter{ + statusChan: statusChan, + errorChan: errorChan, + } + client := &paymentTestInstantOutClient{accepted: true} + instantOutFSM := newPaymentTestFSM(router, client) + + event := instantOutFSM.PollPaymentAcceptedAction(t.Context(), nil) + + require.Equal(t, OnPaymentAccepted, event) + require.Equal(t, 1, client.polls) +} + +// TestPollPaymentAcceptedRejectsNilError verifies that an invalid nil error +// value can't make the FSM report a successful failure path. +func TestPollPaymentAcceptedRejectsNilError(t *testing.T) { + errorChan := make(chan error, 1) + errorChan <- nil + + router := &paymentTestRouter{ + statusChan: make(chan lndclient.PaymentStatus), + errorChan: errorChan, + } + instantOutFSM := newPaymentTestFSM( + router, &paymentTestInstantOutClient{}, + ) + + event := instantOutFSM.PollPaymentAcceptedAction(t.Context(), nil) + + require.Equal(t, fsm.OnError, event) + require.ErrorContains( + t, instantOutFSM.LastActionError, + "payment error channel returned nil", + ) +} + +// TestPollPaymentAcceptedPreservesContextError verifies that cancellation is +// recorded as the action error rather than becoming a nil FSM error. +func TestPollPaymentAcceptedPreservesContextError(t *testing.T) { + ctx, cancel := context.WithCancel(t.Context()) + cancel() + + router := &paymentTestRouter{} + instantOutFSM := newPaymentTestFSM( + router, &paymentTestInstantOutClient{}, + ) + + event := instantOutFSM.PollPaymentAcceptedAction(ctx, nil) + + require.Equal(t, fsm.OnError, event) + require.ErrorIs(t, instantOutFSM.LastActionError, context.Canceled) +}