diff --git a/accounts/checkers.go b/accounts/checkers.go index 03a50fc9..53eda5a1 100644 --- a/accounts/checkers.go +++ b/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 "+ diff --git a/accounts/checkers_test.go b/accounts/checkers_test.go index c6203c6f..8c4e294b 100644 --- a/accounts/checkers_test.go +++ b/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. diff --git a/docs/release-notes/release-notes-0.17.1.md b/docs/release-notes/release-notes-0.17.1.md index 6627288e..888c9cfc 100644 --- a/docs/release-notes/release-notes-0.17.1.md +++ b/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: ` + error. + ### Functional Changes/Additions ### Technical and Architectural Updates @@ -49,3 +57,4 @@ * 0xfandom * bitromortac +* Vandit Singh diff --git a/itest/litd_accounts_test.go b/itest/litd_accounts_test.go index 3f097465..9fa8d55a 100644 --- a/itest/litd_accounts_test.go +++ b/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: " 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) {