diff --git a/docs/release-notes/release-notes-next.md b/docs/release-notes/release-notes-next.md index ef705ccf7..92d78645c 100644 --- a/docs/release-notes/release-notes-next.md +++ b/docs/release-notes/release-notes-next.md @@ -48,6 +48,15 @@ `loopd` failed with `exec format error` on ARM hosts. [Issue #1211](https://github.com/lightninglabs/loop/issues/1211) +* A MuSig2 Loop In no longer reveals the internal key of its HTLC to the + server when the swap invoice is canceled. The key is only shared once the + invoice is paid. + +* A Loop In now cancels its swap invoice before it refunds an expired HTLC, + and refunds only once lnd confirmed that the invoice can no longer be + paid. Previously the invoice stayed payable until the refund confirmed, so + a late payment could settle while the HTLC was being refunded. + #### Maintenance * Align the standalone `looprpc` module's OpenTelemetry SDK and OTLP trace diff --git a/loopin.go b/loopin.go index c047aefc1..cb5ddf127 100644 --- a/loopin.go +++ b/loopin.go @@ -79,6 +79,23 @@ func isInvoiceAlreadySettledError(err error) bool { rpcStatus.Message() == invpkg.ErrInvoiceAlreadySettled.Error() } +// isInvoiceNotFoundError returns true if the error reports that lnd does not +// know the invoice, either as the sentinel itself or as its gRPC form. +func isInvoiceNotFoundError(err error) bool { + if err == nil { + return false + } + + if errors.Is(err, invpkg.ErrInvoiceNotFound) { + return true + } + + rpcStatus, ok := status.FromError(err) + return ok && + rpcStatus.Code() == codes.Unknown && + rpcStatus.Message() == invpkg.ErrInvoiceNotFound.Error() +} + // loopInSwap contains all the in-memory state related to a pending loop in // swap. type loopInSwap struct { @@ -99,6 +116,15 @@ type loopInSwap struct { timeoutAddr btcutil.Address + // invoiceSettled is set once the swap invoice is known to be settled. + // Settlement is final, so the flag is never cleared. + invoiceSettled bool + + // invoiceCanceled is set once lnd acknowledged the cancellation of the + // swap invoice, or no longer knows the invoice. Either way, the server + // can no longer pay it. + invoiceCanceled bool + abandonChan chan struct{} wg sync.WaitGroup @@ -904,24 +930,33 @@ func (s *loopInSwap) waitForSwapComplete(ctx context.Context, return fmt.Errorf("subscribe to swap invoice: %v", err) } + if s.state == loopdb.StateInvoiceSettled || + s.state == loopdb.StateSuccess { + + s.invoiceSettled = true + } + // publishTxOnTimeout publishes the timeout tx if the contract has - // expired and invoice has not been settled. + // expired and the invoice can no longer be settled. publishTxOnTimeout := func() (btcutil.Amount, error) { - // Don't publish the timeout tx if the invoice was settled. - if s.state == loopdb.StateInvoiceSettled { + // Don't publish the timeout tx if the invoice was settled or + // the swap succeeded. + if s.invoiceSettled || s.state == loopdb.StateInvoiceSettled || + s.state == loopdb.StateSuccess { + return 0, nil } - // Don't publish the timeout tx if the swap succeeded. - if s.state == loopdb.StateSuccess { + if s.height < s.LoopInContract.CltvExpiry { return 0, nil } - if s.height >= s.LoopInContract.CltvExpiry { - return s.publishTimeoutTx(ctx, htlcOutpoint, htlcValue) + refund, err := s.authorizeRefund(ctx) + if err != nil || !refund { + return 0, err } - return 0, nil + return s.publishTimeoutTx(ctx, htlcOutpoint, htlcValue) } // Check timeout at current height. After a restart we may want to @@ -936,8 +971,9 @@ func (s *loopInSwap) waitForSwapComplete(ctx context.Context, invoiceFinalized := false htlcKeyRevealed := false for { - // Check stop conditions. - if htlcSpend && invoiceFinalized { + // Check stop conditions. A canceled invoice is final even if + // lnd deleted it and never reports the cancellation. + if htlcSpend && (invoiceFinalized || s.invoiceCanceled) { break } if s.state == loopdb.StateInvoiceSettled { @@ -967,7 +1003,7 @@ func (s *loopInSwap) waitForSwapComplete(ctx context.Context, return err } - if invoiceFinalized && !htlcKeyRevealed { + if s.invoiceSettled && !htlcKeyRevealed { htlcKeyRevealed = s.tryPushHtlcKey(ctx) } @@ -1030,6 +1066,7 @@ func (s *loopInSwap) waitForSwapComplete(ctx context.Context, } invoiceFinalized = true + s.invoiceSettled = true htlcKeyRevealed = s.tryPushHtlcKey(ctx) s.cost.Server = s.AmountRequested - update.AmtPaid @@ -1048,14 +1085,79 @@ func (s *loopInSwap) waitForSwapComplete(ctx context.Context, return nil } +// authorizeRefund makes sure that the swap invoice can no longer be settled +// before the expired htlc is refunded, and reports whether the refund may +// proceed. +// +// Once the htlc expired, the server must not be able to pay the invoice +// anymore: a late payment would give the server the preimage while the client +// takes back the htlc. lnd's CancelInvoice resolves the race with settlement +// atomically. It succeeds for an open or already canceled invoice and fails +// for a settled one, which then blocks the refund. lnd only deletes canceled +// invoices, so an invoice that it no longer knows can't be settled either, +// for example one that its garbage collection removed after an earlier +// cancellation. Any other error leaves the outcome unresolved, so the refund +// waits for a later attempt. +func (s *loopInSwap) authorizeRefund(ctx context.Context) (bool, error) { + if s.invoiceCanceled { + return true, nil + } + + err := s.lnd.Invoices.CancelInvoice(ctx, s.hash) + switch { + case err == nil, isInvoiceNotFoundError(err): + + case isInvoiceAlreadySettledError(err): + s.log.Infof("Swap invoice settled before the refund, not " + + "refunding the htlc") + + // The swap can complete before the invoice update that + // reports the paid amount arrives, so take the amount from the + // invoice itself. + invoice, lookupErr := s.lnd.Client.LookupInvoice(ctx, s.hash) + if lookupErr != nil { + s.log.Warnf("Unable to look up the paid amount of the "+ + "settled swap invoice: %v", lookupErr) + } else { + s.cost.Server = s.AmountRequested - + invoice.AmountPaid.ToSatoshis() + } + + s.invoiceSettled = true + if s.state == loopdb.StateHtlcPublished { + s.setState(loopdb.StateInvoiceSettled) + return false, s.persistAndAnnounceState(ctx) + } + + return false, nil + + default: + s.log.Warnf("Unable to cancel the swap invoice before the "+ + "refund, retrying at the next block: %v", err) + + return false, nil + } + + s.invoiceCanceled = true + + return true, nil +} + // tryPushHtlcKey attempts to push the htlc key to the server. If the server // returns an error of any kind we'll log it as a warning but won't act as the // swap execution can just go on without the server gaining knowledge of our // internal key. +// +// The internal key lets the server spend the htlc through its key path, so it +// is only revealed after the swap invoice was settled. For a canceled invoice +// the key must stay secret while the htlc can still be spent. func (s *loopInSwap) tryPushHtlcKey(ctx context.Context) bool { if s.ProtocolVersion < loopdb.ProtocolVersionMuSig2 { return false } + if !s.invoiceSettled { + return false + } log.Infof("Attempting to reveal internal HTLC key to the server") @@ -1106,9 +1208,14 @@ func (s *loopInSwap) processHtlcSpend(ctx context.Context, // swap invoice. We still need to query the final invoice state. // This is not a hodl invoice, so it may be that the invoice was // already settled. This means that the server didn't succeed in - // sweeping the htlc after paying the invoice. + // sweeping the htlc after paying the invoice. An invoice that + // lnd no longer knows was canceled before the refund. err := s.lnd.Invoices.CancelInvoice(ctx, s.hash) - if err != nil && !isInvoiceAlreadySettledError(err) { + switch { + case err == nil, isInvoiceNotFoundError(err): + s.invoiceCanceled = true + + case !isInvoiceAlreadySettledError(err): return err } } diff --git a/loopin_test.go b/loopin_test.go index 93d019da8..107046e66 100644 --- a/loopin_test.go +++ b/loopin_test.go @@ -597,6 +597,10 @@ func handleHtlcExpiry(t *testing.T, ctx *loopInTestContext, inSwap *loopInSwap, // Let htlc expire. ctx.blockEpochChan <- inSwap.LoopInContract.CltvExpiry + // Before refunding, the client cancels the swap invoice so that the + // server can no longer pay it. + require.Equal(t, ctx.server.swapHash, <-ctx.lnd.FailInvoiceChannel) + // Expect a signing request for the htlc tx output value. signReq := <-ctx.lnd.SignOutputRawChannel require.Equal( @@ -1086,3 +1090,238 @@ func startNewLoopIn(t *testing.T, ctx *loopInTestContext, height int32) ( return cfg, inSwap, err } + +// refundGateInvoices returns scripted errors for invoice cancellations and +// forwards every other call to the mock invoices client. +type refundGateInvoices struct { + lndclient.InvoicesClient + + cancelErrs chan error +} + +// CancelInvoice returns the next scripted error, or forwards the call. +func (r *refundGateInvoices) CancelInvoice(ctx context.Context, + hash lntypes.Hash) error { + + select { + case err := <-r.cancelErrs: + if err != nil { + return err + } + + default: + } + + return r.InvoicesClient.CancelInvoice(ctx, hash) +} + +// expiringLoopIn is a loop in whose confirmed htlc is about to expire. +type expiringLoopIn struct { + ctx *loopInTestContext + swap *loopInSwap + htlcTx wire.MsgTx + invoices *refundGateInvoices + errChan chan error +} + +// startExpiringLoopIn runs a loop in until its htlc confirmed and the client +// watches the htlc and the swap invoice. +func startExpiringLoopIn(t *testing.T) *expiringLoopIn { + t.Helper() + + ctx := newLoopInTestContext(t) + cfg := newSwapConfig( + &ctx.lnd.LndServices, ctx.store, ctx.server, nil, + clock.NewTestClock(time.Unix(123, 0)), + ) + req := testLoopInRequest + initResult, err := newLoopInSwap( + context.Background(), cfg, 600, &req, + ) + require.NoError(t, err) + ctx.store.AssertLoopInStored() + + invoices := &refundGateInvoices{ + InvoicesClient: ctx.lnd.LndServices.Invoices, + cancelErrs: make(chan error, 4), + } + ctx.lnd.LndServices.Invoices = invoices + + errChan := make(chan error, 1) + go func() { + errChan <- initResult.swap.execute( + context.Background(), ctx.cfg, 600, + ) + }() + + ctx.assertState(loopdb.StateInitiated) + ctx.assertState(loopdb.StateHtlcPublished) + ctx.store.AssertLoopInState(loopdb.StateHtlcPublished) + htlcTx := <-ctx.lnd.SendOutputsChannel + ctx.store.AssertLoopInState(loopdb.StateHtlcPublished) + + <-ctx.lnd.RegisterConfChannel + ctx.lnd.ConfChannel <- &chainntnfs.TxConfirmation{Tx: &htlcTx} + <-ctx.lnd.RegisterSpendChannel + ctx.assertSubscribeInvoice(ctx.server.swapHash) + + return &expiringLoopIn{ + ctx: ctx, + swap: initResult.swap, + htlcTx: htlcTx, + invoices: invoices, + errChan: errChan, + } +} + +// requireNoRefund asserts that no refund is signed. +func (e *expiringLoopIn) requireNoRefund(t *testing.T) { + t.Helper() + + select { + case <-e.ctx.lnd.SignOutputRawChannel: + t.Fatal("htlc refund was signed") + + case <-time.After(100 * time.Millisecond): + } +} + +// TestLoopInRefundGateSettledInvoice asserts that a refund is never published +// when the swap invoice turns out to be settled at the htlc expiry, even if +// the client has not seen the settlement yet. +func TestLoopInRefundGateSettledInvoice(t *testing.T) { + defer test.Guard(t)() + + e := startExpiringLoopIn(t) + e.invoices.cancelErrs <- status.Error( + codes.Unknown, invpkg.ErrInvoiceAlreadySettled.Error(), + ) + + // The server paid the invoice, but its update has not arrived. + invoice, err := e.ctx.lnd.Client.LookupInvoice( + context.Background(), e.swap.hash, + ) + require.NoError(t, err) + invoice.State = invpkg.ContractSettled + invoice.AmountPaid = invoice.Amount + e.ctx.lnd.SetInvoice(invoice) + + e.ctx.blockEpochChan <- e.swap.LoopInContract.CltvExpiry + e.ctx.store.AssertLoopInState(loopdb.StateInvoiceSettled) + e.ctx.assertState(loopdb.StateInvoiceSettled) + e.requireNoRefund(t) + + // The server cost is recorded without the invoice update. + state := e.ctx.store.AssertLoopInState(loopdb.StateSuccess) + e.ctx.assertState(loopdb.StateSuccess) + require.Equal(t, + e.swap.AmountRequested-invoice.Amount.ToSatoshis(), + state.Cost.Server) + require.Positive(t, state.Cost.Server) + require.NoError(t, <-e.errChan) + require.Positive(t, e.ctx.server.pushKeyCalls.Load()) +} + +// TestLoopInRefundGateUnresolvedCancellation asserts that an invoice +// cancellation with an uncertain outcome defers the refund to a later block. +func TestLoopInRefundGateUnresolvedCancellation(t *testing.T) { + defer test.Guard(t)() + + e := startExpiringLoopIn(t) + e.invoices.cancelErrs <- status.Error( + codes.Unavailable, "connection lost", + ) + + expiry := e.swap.LoopInContract.CltvExpiry + e.ctx.blockEpochChan <- expiry + e.requireNoRefund(t) + + // The next block cancels the invoice and refunds the htlc. + e.ctx.blockEpochChan <- expiry + 1 + require.Equal(t, e.ctx.server.swapHash, <-e.ctx.lnd.FailInvoiceChannel) + + signReq := <-e.ctx.lnd.SignOutputRawChannel + require.Equal(t, e.htlcTx.TxOut[0].Value, + signReq.SignDescriptors[0].Output.Value) + timeoutTx := <-e.ctx.lnd.TxPublishChannel + + e.ctx.lnd.SpendChannel <- &chainntnfs.SpendDetail{ + SpendingTx: timeoutTx, + SpenderInputIndex: 0, + } + <-e.ctx.lnd.FailInvoiceChannel + e.ctx.updateInvoiceState(0, invpkg.ContractCanceled) + e.ctx.assertState(loopdb.StateFailTimeout) + e.ctx.store.AssertLoopInState(loopdb.StateFailTimeout) + require.NoError(t, <-e.errChan) + + // A canceled invoice never reveals the htlc key. + require.Zero(t, e.ctx.server.pushKeyCalls.Load()) +} + +// TestLoopInCanceledInvoiceKeepsHtlcKey asserts that the htlc key is not +// revealed after the swap invoice was canceled, not even on later blocks. +func TestLoopInCanceledInvoiceKeepsHtlcKey(t *testing.T) { + defer test.Guard(t)() + + e := startExpiringLoopIn(t) + e.ctx.updateInvoiceState(0, invpkg.ContractCanceled) + + expiry := e.swap.LoopInContract.CltvExpiry + e.ctx.blockEpochChan <- expiry - 2 + e.ctx.blockEpochChan <- expiry - 1 + time.Sleep(100 * time.Millisecond) + require.Zero(t, e.ctx.server.pushKeyCalls.Load()) + + e.ctx.blockEpochChan <- expiry + <-e.ctx.lnd.FailInvoiceChannel + <-e.ctx.lnd.SignOutputRawChannel + timeoutTx := <-e.ctx.lnd.TxPublishChannel + e.ctx.lnd.SpendChannel <- &chainntnfs.SpendDetail{ + SpendingTx: timeoutTx, + SpenderInputIndex: 0, + } + <-e.ctx.lnd.FailInvoiceChannel + e.ctx.assertState(loopdb.StateFailTimeout) + e.ctx.store.AssertLoopInState(loopdb.StateFailTimeout) + require.NoError(t, <-e.errChan) + require.Zero(t, e.ctx.server.pushKeyCalls.Load()) +} + +// TestLoopInRefundGateDeletedInvoice asserts that a swap invoice that lnd no +// longer knows, for example one that its garbage collection deleted after an +// earlier cancellation, counts as canceled: the expired htlc is refunded, and +// the swap completes without an update of the deleted invoice. +func TestLoopInRefundGateDeletedInvoice(t *testing.T) { + defer test.Guard(t)() + + e := startExpiringLoopIn(t) + notFound := status.Error( + codes.Unknown, invpkg.ErrInvoiceNotFound.Error(), + ) + + // Both the cancellation before the refund and the one after it find + // no invoice. + e.invoices.cancelErrs <- notFound + e.invoices.cancelErrs <- notFound + + e.ctx.blockEpochChan <- e.swap.LoopInContract.CltvExpiry + select { + case signReq := <-e.ctx.lnd.SignOutputRawChannel: + require.Equal(t, e.htlcTx.TxOut[0].Value, + signReq.SignDescriptors[0].Output.Value) + + case <-time.After(test.Timeout): + t.Fatal("htlc refund was not signed") + } + timeoutTx := <-e.ctx.lnd.TxPublishChannel + + e.ctx.lnd.SpendChannel <- &chainntnfs.SpendDetail{ + SpendingTx: timeoutTx, + SpenderInputIndex: 0, + } + e.ctx.assertState(loopdb.StateFailTimeout) + e.ctx.store.AssertLoopInState(loopdb.StateFailTimeout) + require.NoError(t, <-e.errChan) + require.Zero(t, e.ctx.server.pushKeyCalls.Load()) +} diff --git a/server_mock_test.go b/server_mock_test.go index e8bc47277..bf4e72f11 100644 --- a/server_mock_test.go +++ b/server_mock_test.go @@ -3,6 +3,7 @@ package loop import ( "context" "errors" + "sync/atomic" "testing" "time" @@ -60,6 +61,9 @@ type serverMock struct { // cancelSwap is a channel that swap cancellations are sent into. cancelSwap chan *outCancelDetails + // pushKeyCalls counts the htlc key reveals received. + pushKeyCalls atomic.Int32 + lnd *test.LndMockServices } @@ -300,6 +304,8 @@ func (s *serverMock) MultiMuSig2SignSweep(ctx context.Context, func (s *serverMock) PushKey(_ context.Context, _ loopdb.ProtocolVersion, _ lntypes.Hash, _ [32]byte) error { + s.pushKeyCalls.Add(1) + return nil }