Merge pull request #1177 from chandachewe10/master

fix dead error variable and silent invoice cancel in setStateAbandoned

loopin.go

@@ -27,6 +27,8 @@ import (
 	"github.com/lightningnetwork/lnd/lnwallet/chainfee"
 	"github.com/lightningnetwork/lnd/lnwire"
 	"github.com/lightningnetwork/lnd/routing/route"
+	"google.golang.org/grpc/codes"
+	"google.golang.org/grpc/status"
 )
 
 var (
@@ -58,6 +60,25 @@ var (
 	ErrSwapFinalized = errors.New("swap is in a final state")
 )
 
+// isInvoiceAlreadySettledError reports whether err indicates that an invoice
+// cancellation failed because the invoice was already settled. If lnd returns
+// the sentinel from an RPC handler, gRPC transports it as an Unknown status
+// with the sentinel's error text.
+func isInvoiceAlreadySettledError(err error) bool {
+	if err == nil {
+		return false
+	}
+
+	if errors.Is(err, invpkg.ErrInvoiceAlreadySettled) {
+		return true
+	}
+
+	rpcStatus, ok := status.FromError(err)
+	return ok &&
+		rpcStatus.Code() == codes.Unknown &&
+		rpcStatus.Message() == invpkg.ErrInvoiceAlreadySettled.Error()
+}
+
 // loopInSwap contains all the in-memory state related to a pending loop in
 // swap.
 type loopInSwap struct {
@@ -1087,7 +1108,7 @@ func (s *loopInSwap) processHtlcSpend(ctx context.Context,
 		// already settled. This means that the server didn't succeed in
 		// sweeping the htlc after paying the invoice.
 		err := s.lnd.Invoices.CancelInvoice(ctx, s.hash)
-		if err != nil && err != invpkg.ErrInvoiceAlreadySettled {
+		if err != nil && !isInvoiceAlreadySettledError(err) {
 			return err
 		}
 	}
@@ -1174,13 +1195,17 @@ func (s *loopInSwap) setStateAbandoned(ctx context.Context) error {
 		return err
 	}
 
-	// If the invoice is already settled or canceled, this is a nop.
-	_ = s.lnd.Invoices.CancelInvoice(ctx, s.hash)
+	// Cancel the invoice so the server can no longer settle it. If the
+	// invoice is already settled we ignore the error, matching the
+	// behaviour of the timeout path. Any other unexpected error is logged
+	// but does not prevent the abandon from completing.
+	err = s.lnd.Invoices.CancelInvoice(ctx, s.hash)
+	if err != nil && !isInvoiceAlreadySettledError(err) {
+		s.log.Warnf("Failed to cancel invoice for abandoned swap: %v",
+			err)
+	}
 
-	return fmt.Errorf("swap hash "+
-		"abandoned by client, "+
-		"swap ID: %v, %v",
-		s.hash, err)
+	return fmt.Errorf("swap hash abandoned by client, swap ID: %v", s.hash)
 }
 
 // persistAndAnnounceState updates the swap state on disk and sends out an

loopin_test.go

@@ -20,6 +20,8 @@ import (
 	"github.com/lightningnetwork/lnd/routing/route"
 	"github.com/lightningnetwork/lnd/zpay32"
 	"github.com/stretchr/testify/require"
+	"google.golang.org/grpc/codes"
+	"google.golang.org/grpc/status"
 )
 
 var (
@@ -44,6 +46,142 @@ type probeInvoicesMock struct {
 	cancelCtxErr chan error
 }
 
+// cancelErrorInvoicesMock is an InvoicesClient that returns a configured
+// cancellation error.
+type cancelErrorInvoicesMock struct {
+	lndclient.InvoicesClient
+
+	err error
+}
+
+// CancelInvoice returns the cancellation error configured on the mock.
+func (c *cancelErrorInvoicesMock) CancelInvoice(context.Context,
+	lntypes.Hash) error {
+
+	return c.err
+}
+
+// TestIsInvoiceAlreadySettledError verifies the local and gRPC error forms
+// recognized by the already-settled classifier.
+func TestIsInvoiceAlreadySettledError(t *testing.T) {
+	t.Parallel()
+
+	testCases := []struct {
+		name     string
+		err      error
+		expected bool
+	}{
+		{
+			name:     "sentinel",
+			err:      invpkg.ErrInvoiceAlreadySettled,
+			expected: true,
+		},
+		{
+			name: "wrapped sentinel",
+			err: fmt.Errorf(
+				"cancel invoice: %w",
+				invpkg.ErrInvoiceAlreadySettled,
+			),
+			expected: true,
+		},
+		{
+			name: "grpc representation",
+			err: status.Error(
+				codes.Unknown,
+				invpkg.ErrInvoiceAlreadySettled.Error(),
+			),
+			expected: true,
+		},
+		{
+			name: "different grpc status",
+			err: status.Error(
+				codes.FailedPrecondition,
+				invpkg.ErrInvoiceAlreadySettled.Error(),
+			),
+		},
+		{
+			name: "different error",
+			err:  fmt.Errorf("cancel invoice failed"),
+		},
+		{
+			name: "nil",
+		},
+	}
+
+	for _, testCase := range testCases {
+		t.Run(testCase.name, func(t *testing.T) {
+			require.Equal(
+				t, testCase.expected,
+				isInvoiceAlreadySettledError(testCase.err),
+			)
+		})
+	}
+}
+
+// TestProcessHtlcSpendIgnoresGRPCAlreadySettled verifies that the timeout path
+// completes normally when invoice cancellation reports an already-settled
+// invoice.
+func TestProcessHtlcSpendIgnoresGRPCAlreadySettled(t *testing.T) {
+	defer test.Guard(t)()
+
+	// Initialize a loop-in so the test uses the same contract and HTLC
+	// scripts as the production flow.
+	testCtx := newLoopInTestContext(t)
+	cfg := newSwapConfig(
+		&testCtx.lnd.LndServices, testCtx.store, testCtx.server, nil,
+		clock.NewTestClock(time.Unix(123, 0)),
+	)
+
+	initResult, err := newLoopInSwap(
+		context.Background(), cfg, 600, &testLoopInRequest,
+	)
+	require.NoError(t, err)
+	testCtx.store.AssertLoopInStored()
+
+	// Return the gRPC status observed when lnd transports its
+	// ErrInvoiceAlreadySettled sentinel across the RPC boundary.
+	cfg.lnd.Invoices = &cancelErrorInvoicesMock{
+		err: status.Error(
+			codes.Unknown,
+			invpkg.ErrInvoiceAlreadySettled.Error(),
+		),
+	}
+
+	// Confirmation processing normally selects the HTLC version that was
+	// found on chain. Select the initialized version explicitly because this
+	// test calls processHtlcSpend directly.
+	if initResult.swap.htlcP2TR != nil {
+		initResult.swap.htlc = initResult.swap.htlcP2TR
+	} else {
+		initResult.swap.htlc = initResult.swap.htlcP2WSH
+	}
+	require.NotNil(t, initResult.swap.htlc)
+
+	// Construct a timeout spend so processHtlcSpend takes the invoice
+	// cancellation branch.
+	timeoutWitness, err := initResult.swap.htlc.GenTimeoutWitness([]byte{1})
+	require.NoError(t, err)
+
+	timeoutTx := &wire.MsgTx{
+		TxIn: []*wire.TxIn{
+			{
+				Witness: timeoutWitness,
+			},
+		},
+	}
+
+	// The already-settled status must be suppressed while the swap still
+	// transitions to its terminal timeout state.
+	err = initResult.swap.processHtlcSpend(
+		context.Background(), &chainntnfs.SpendDetail{
+			SpendingTx:        timeoutTx,
+			SpenderInputIndex: 0,
+		}, 100,
+	)
+	require.NoError(t, err)
+	require.Equal(t, loopdb.StateFailTimeout, initResult.swap.state)
+}
+
 // SubscribeSingleInvoice returns the mock's preconfigured channels.
 func (p *probeInvoicesMock) SubscribeSingleInvoice(_ context.Context,
 	_ lntypes.Hash) (<-chan lndclient.InvoiceUpdate, <-chan error, error) {