Merge pull request #1322 from Vandit1604/accounts-errored-payment-passthrough

accounts: don't mask payment errors when request values are absent

accounts/checkers.go

@@ -668,10 +668,18 @@ func erroredPaymentHandler(service Service) mid.ErrorHandler {
 			return nil, err
 		}
 
+		// On streaming sends a call can return a response and then a
+		// terminal error, so the same request's error may be processed
+		// more than once. If the request values are already gone there
+		// is nothing left to clean up and we let the error pass
+		// through.
 		reqVals, ok := service.GetValues(reqID)
 		if !ok {
-			return nil, fmt.Errorf("no request values found for "+
-				"request: %d", reqID)
+			log.Tracef("No request values found for request: %d, "+
+				"passing the response error through unchanged",
+				reqID)
+
+			return nil, nil
 		}
 
 		log.Tracef("Handling payment request error for payment with "+

accounts/checkers_test.go

@@ -677,6 +677,42 @@ func TestSendPaymentV2(t *testing.T) {
 	_, err = service.checkers.handleErrorResponse(ctx, uri, nil)
 	require.NoError(t, err)
 	assertBalance(acct.ID, 2000)
+
+	// Finally, replicate the streaming scenario. Since SendPaymentV2 is a
+	// streaming endpoint, the request values are deleted as soon as a
+	// terminal Payment response is handled. If lnd then sends a terminal
+	// error response for the same request, the error handler finds no
+	// request values. It must pass the original error through unchanged
+	// instead of masking it with a confusing "no request values found"
+	// error.
+	reqID5 := nextRequestID()
+	ctx = AddRequestIDToContext(ctxWithAcct, reqID5)
+
+	// Send a valid request so the request values are registered.
+	err = service.checkers.checkIncomingRequest(
+		ctx, uri, &routerrpc.SendPaymentRequest{
+			AmtMsat:     1000,
+			PaymentHash: testHash2[:],
+		},
+	)
+	require.NoError(t, err)
+
+	// A failed Payment response deletes the stored request values.
+	_, err = service.checkers.replaceOutgoingResponse(
+		ctx, uri, &lnrpc.Payment{
+			PaymentHash: hex.EncodeToString(testHash2[:]),
+			Status:      lnrpc.Payment_FAILED,
+		},
+	)
+	require.NoError(t, err)
+
+	// A subsequent error response for the same request must not be masked.
+	originalErr := fmt.Errorf("account balance insufficient")
+	returnedErr, err := service.checkers.handleErrorResponse(
+		ctx, uri, originalErr,
+	)
+	require.NoError(t, err)
+	require.ErrorContains(t, returnedErr, "account balance insufficient")
 }
 
 // TestSendToRouteV2 performs test coverage on the SendToRouteV2 checker.

docs/release-notes/release-notes-0.17.1.md

@@ -23,6 +23,14 @@
   `WAITING_TO_START` state, so the very next call could still fail with
   `rpc error: ... waiting to start`.
 
+* [Don't mask account payment errors when request values are
+  absent](https://github.com/lightninglabs/lightning-terminal/pull/1322):
+  When a streaming account payment (`SendPaymentV2`/`SendToRouteV2`) fails and
+  lnd returns a terminal error after the request values have already been
+  cleaned up, lnd's underlying error is now passed through to the caller instead
+  of being masked by a confusing `no request values found for request: <id>`
+  error.
+
 ### Functional Changes/Additions
 
 ### Technical and Architectural Updates
@@ -49,3 +57,4 @@
 
 * 0xfandom
 * bitromortac
+* Vandit Singh

itest/litd_accounts_test.go

@@ -170,6 +170,12 @@ func runAccountSystemTest(t *harnessTest, node *HarnessNode, hostPort,
 		ctxm, t, rawConn, newAcctBalance, acctResp.Account.Id,
 	)
 
+	// Make sure a payment that the account checker rejects surfaces the
+	// real error instead of the masked "no request values found" error.
+	testAccountPaymentErrorPassthrough(
+		ctxa, t, rawConn, charlie, newAcctBalance,
+	)
+
 	// Clean up our channel and payments, so we can start the next test
 	// iteration with a clean slate.
 	closeChannelAndAssert(t, net, node, channelOp, false)
@@ -313,6 +319,56 @@ func testAccountRestrictions(ctxa context.Context, t *harnessTest,
 	return initialAccountBalance + inboundPaymentAmt - outboundPaymentAmt
 }
 
+// testAccountPaymentErrorPassthrough verifies that a payment which the account
+// checker rejects because the account balance is insufficient surfaces a clear
+// account-balance error to the caller, and never the masked "no request values
+// found for request: <id>" error.
+//
+// SendPaymentV2 is a streaming RPC, so lnd surfaces the account checker's
+// rejection through the stream's terminal error path. That reaches
+// erroredPaymentHandler with no stored request values, which is exactly the
+// case where the error must pass through unmasked instead of being replaced by
+// a confusing "no request values found" error.
+func testAccountPaymentErrorPassthrough(ctxa context.Context, t *harnessTest,
+	rawConn grpc.ClientConnInterface, charlie *HarnessNode,
+	accountBalance uint64) {
+
+	ctxb := context.Background()
+	ctxt, cancel := context.WithTimeout(ctxb, defaultTimeout)
+	defer cancel()
+
+	routerClient := routerrpc.NewRouterClient(rawConn)
+
+	// Create a routable invoice on Charlie whose amount exceeds the account
+	// balance, so the only possible reason the payment fails is the
+	// insufficient account balance.
+	excessiveAmt := int64(accountBalance) + 10_000
+	invoice, err := charlie.AddInvoice(ctxt, &lnrpc.Invoice{
+		Value: excessiveAmt,
+		Memo:  "exceeds account balance",
+	})
+	require.NoError(t.t, err)
+
+	sendReq := &routerrpc.SendPaymentRequest{
+		PaymentRequest: invoice.PaymentRequest,
+		TimeoutSeconds: 60,
+		FeeLimitMsat:   1000,
+	}
+	stream, err := routerClient.SendPaymentV2(ctxa, sendReq)
+	require.NoError(t.t, err)
+
+	// The payment must fail with the underlying account-balance error and
+	// not be masked by the confusing "no request values found" error.
+	_, err = getPaymentResult(stream, false)
+	require.Error(t.t, err)
+	require.NotContains(t.t, err.Error(), "no request values found")
+	require.Contains(t.t, err.Error(), "account balance insufficient")
+
+	// The account balance must be untouched by the rejected payment.
+	lightningClient := lnrpc.NewLightningClient(rawConn)
+	assertChannelBalance(ctxa, t.t, lightningClient, accountBalance, 0)
+}
+
 func assertChannelBalance(ctx context.Context, t *testing.T,
 	client lnrpc.LightningClient, localBalance, remoteBalance uint64) {