switchrpc: update TrackOnion rpc for new error handling

Author: calvinrzachman

itest/lnd_sendonion_test.go

@@ -93,7 +93,7 @@ func testSendOnion(ht *lntest.HarnessTest) {
 		HopPubkeys:  onionResp.HopPubkeys,
 	}
 	trackResp := alice.RPC.TrackOnion(trackReq)
-	require.Equal(ht, invoices[0].RPreimage, trackResp.Preimage)
+	require.Equal(ht, invoices[0].RPreimage, trackResp.GetPreimage())
 
 	// The invoice should show as settled for Dave.
 	ht.AssertInvoiceSettled(dave, invoices[0].PaymentAddr)
@@ -200,7 +200,7 @@ func testSendOnionTwice(ht *lntest.HarnessTest) {
 		HopPubkeys:  onionResp.HopPubkeys,
 	}
 	trackResp := alice.RPC.TrackOnion(trackReq)
-	require.Equal(ht, preimage[:], trackResp.Preimage)
+	require.Equal(ht, preimage[:], trackResp.GetPreimage())
 
 	// Now that the original HTLC attempt has settled, we'll send the same
 	// onion again with the same attempt ID.
@@ -275,9 +275,6 @@ func testTrackOnion(ht *lntest.HarnessTest) {
 	require.True(ht, resp.Success, "expected successful onion send")
 	require.Empty(ht, resp.ErrorMessage, "unexpected failure to send onion")
 
-	serverErrorStr := ""
-	clientErrorStr := ""
-
 	// Track the payment providing all necessary information to delegate
 	// error decryption to the server. We expect this to fail as Dave is not
 	// expecting payment.
@@ -288,10 +285,11 @@ func testTrackOnion(ht *lntest.HarnessTest) {
 		HopPubkeys:  onionResp.HopPubkeys,
 	}
 	trackResp := alice.RPC.TrackOnion(trackReq)
-	require.NotEmpty(ht, trackResp.ErrorMessage,
-		"expected onion tracking error")
+	serverFailure := trackResp.GetFailureDetails()
+	require.NotNil(ht, serverFailure, "expected onion tracking error")
 
-	serverErrorStr = trackResp.ErrorMessage
+	serverFwdFailure := serverFailure.GetForwardingFailure()
+	require.NotNil(ht, serverFwdFailure, "expected forwarding failure")
 
 	// Now we'll track the same payment attempt, but we'll specify that
 	// we want to handle the error decryption ourselves client side.
@@ -300,16 +298,18 @@ func testTrackOnion(ht *lntest.HarnessTest) {
 		PaymentHash: paymentHash,
 	}
 	trackResp = alice.RPC.TrackOnion(trackReq)
-	require.NotNil(ht, trackResp.EncryptedError, "expected encrypted error")
+	clientFailure := trackResp.GetFailureDetails()
+	require.NotNil(ht, clientFailure, "expected client tracking error")
+
+	encryptedErrorBytes := clientFailure.GetEncryptedErrorData()
+	require.NotNil(ht, encryptedErrorBytes, "expected encrypted error")
 
 	// Decrypt and inspect the error from the TrackOnion RPC response.
 	sessionKey, _ := btcec.PrivKeyFromBytes(onionResp.SessionKey)
 	var pubKeys []*btcec.PublicKey
 	for _, keyBytes := range onionResp.HopPubkeys {
 		pubKey, err := btcec.ParsePubKey(keyBytes)
-		if err != nil {
-			ht.Fatalf("Failed to parse public key: %v", err)
-		}
+		require.NoError(ht, err, "Failed to parse public key")
 		pubKeys = append(pubKeys, pubKey)
 	}
 
@@ -323,14 +323,19 @@ func testTrackOnion(ht *lntest.HarnessTest) {
 	}
 
 	// Simulate an RPC client decrypting the onion error.
-	encryptedError := lnwire.OpaqueReason(trackResp.EncryptedError)
-	forwardingError, err := errorDecryptor.DecryptError(encryptedError)
-	require.Nil(ht, err, "unable to decrypt error")
-
-	clientErrorStr = forwardingError.Error()
+	encryptedError := lnwire.OpaqueReason(encryptedErrorBytes)
+	clientFwdErr, err := errorDecryptor.DecryptError(encryptedError)
+	require.NoError(ht, err, "unable to decrypt error")
+
+	// Finally, assert that the structured forwarding failure is the same
+	// whether it was decrypted on the server or on the client.
+	serverFwdErr, err := switchrpc.UnmarshallForwardingError(
+		serverFwdFailure,
+	)
+	require.NoError(ht, err, "unable to decode server forwarding failure")
 
-	serverFwdErr, err := switchrpc.ParseForwardingError(serverErrorStr)
-	require.Nil(ht, err, "expected to parse forwarding error from server")
-	require.Equal(ht, serverFwdErr.Error(), clientErrorStr, "expect error "+
-		"message to match whether handled by client or server")
+	require.Equal(ht, serverFwdFailure.FailureSourceIndex,
+		uint32(clientFwdErr.FailureSourceIdx), "source index mismatch")
+	require.Equal(ht, serverFwdErr.WireMessage(),
+		clientFwdErr.WireMessage(), "wire message mismatch")
 }

lnrpc/switchrpc/switch_server.go

@@ -12,8 +12,6 @@ import (
 	"math/big"
 	"os"
 	"path/filepath"
-	"strconv"
-	"strings"
 
 	"github.com/btcsuite/btcd/btcec/v2"
 	"github.com/grpc-ecosystem/grpc-gateway/v2/runtime"
@@ -435,15 +433,19 @@ func (s *Server) TrackOnion(ctx context.Context,
 		req.AttemptId, hash, errorDecryptor,
 	)
 	if err != nil {
-		message, code := translateErrorForRPC(err)
-
 		log.Errorf("GetAttemptResult failed for attempt_id=%d of "+
-			" payment=%x: %v", req.AttemptId, hash, message)
+			" payment=%x: %v", req.AttemptId, hash, err)
 
-		return &TrackOnionResponse{
-			ErrorCode:    code,
-			ErrorMessage: message,
-		}, nil
+		// If the payment ID is not found, we return a NotFound error.
+		if errors.Is(err, htlcswitch.ErrPaymentIDNotFound) {
+			return nil, status.Errorf(codes.NotFound,
+				"payment with attempt ID %d not found",
+				req.AttemptId)
+		}
+
+		// For other errors, we return an internal error.
+		return nil, status.Errorf(codes.Internal,
+			"GetAttemptResult failed: %v", err)
 	}
 
 	// The switch knows about this payment, we'll wait for a result to be
@@ -456,12 +458,10 @@ func (s *Server) TrackOnion(ctx context.Context,
 	select {
 	case result, ok = <-resultChan:
 		if !ok {
-			// This channel is closed when the Switch shuts down.
-			return &TrackOnionResponse{
-				ErrorCode: ErrorCode_SWITCH_EXITING,
-				ErrorMessage: htlcswitch.ErrSwitchExiting.
-					Error(),
-			}, nil
+			// This channel is closed when the Switch shuts down. We
+			// return a gRPC error to the client.
+			return nil, status.Error(codes.Unavailable,
+				htlcswitch.ErrSwitchExiting.Error())
 		}
 
 	case <-ctx.Done():
@@ -470,25 +470,30 @@ func (s *Server) TrackOnion(ctx context.Context,
 		return nil, status.FromContextError(ctx.Err()).Err()
 	}
 
-	// The attempt result arrived so the HTLC is no longer in-flight.
+	// The attempt result arrived so the HTLC is no longer in-flight. If
+	// the payment failed, we build a structured response for the client.
 	if result.Error != nil {
-		message, code := translateErrorForRPC(result.Error)
-
 		log.Errorf("Payment via onion failed for payment=%v: %v",
-			hash, message)
+			hash, result.Error)
 
-		return &TrackOnionResponse{
-			ErrorCode:    code,
-			ErrorMessage: message,
-		}, nil
+		details := marshallFailureDetails(result.Error)
+
+		return newTrackOnionFailureResponse(details), nil
 	}
 
+	// If the server was unable to decrypt the error, it will be returned
+	// as an encrypted byte slice. We populate the response accordingly.
 	if len(result.EncryptedError) > 0 {
-		log.Errorf("Payment via onion failed for payment=%v", hash)
+		log.Errorf("Payment via onion failed for payment=%v with "+
+			"encrypted error", hash)
 
-		return &TrackOnionResponse{
-			EncryptedError: result.EncryptedError,
-		}, nil
+		details := &FailureDetails{
+			Failure: &FailureDetails_EncryptedErrorData{
+				EncryptedErrorData: result.EncryptedError,
+			},
+		}
+
+		return newTrackOnionFailureResponse(details), nil
 	}
 
 	// If we have reached this point, we expect a valid preimage for a
@@ -497,17 +502,21 @@ func (s *Server) TrackOnion(ctx context.Context,
 		log.Errorf("Payment %v completed without a valid preimage or "+
 			"error", hash)
 
-		return &TrackOnionResponse{
+		details := &FailureDetails{
 			ErrorCode:    ErrorCode_INTERNAL,
 			ErrorMessage: ErrAmbiguousPaymentState.Error(),
-		}, nil
+		}
+
+		return newTrackOnionFailureResponse(details), nil
 	}
 
 	log.Debugf("Received preimage via onion attempt_id=%d for payment=%v",
 		req.AttemptId, hash)
 
 	return &TrackOnionResponse{
-		Preimage: result.Preimage[:],
+		Result: &TrackOnionResponse_Preimage{
+			Preimage: result.Preimage[:],
+		},
 	}, nil
 }
 
@@ -662,7 +671,6 @@ func (s *Server) BuildOnion(_ context.Context,
 func translateErrorForRPC(err error) (string, ErrorCode) {
 	var (
 		clearTextErr htlcswitch.ClearTextError
-		fwdErr       *htlcswitch.ForwardingError
 	)
 
 	switch {
@@ -680,20 +688,6 @@ func translateErrorForRPC(err error) (string, ErrorCode) {
 		return err.Error(), ErrorCode_SWITCH_EXITING
 
 	case errors.As(err, &clearTextErr):
-		// If this is a forwarding error, we'll handle it specially.
-		if errors.As(err, &fwdErr) {
-			encodedError, encodeErr := encodeForwardingError(fwdErr)
-			if encodeErr != nil {
-				return fmt.Sprintf("failed to encode wire "+
-						"message: %v", encodeErr),
-					ErrorCode_INTERNAL
-			}
-
-			return encodedError,
-				ErrorCode_FORWARDING_ERROR
-		}
-
-		// Otherwise, we'll just encode the clear text error.
 		var buf bytes.Buffer
 		encodeErr := lnwire.EncodeFailure(
 			&buf, clearTextErr.WireMessage(), 0,
@@ -712,48 +706,116 @@ func translateErrorForRPC(err error) (string, ErrorCode) {
 	}
 }
 
-// encodeForwardingError converts a forwarding error from the switch to the
-// format we can package for delivery to SendOnion rpc clients. We preserve the
-// failure message from the wire as well as the index along the route where the
-// failure occurred.
-func encodeForwardingError(e *htlcswitch.ForwardingError) (string, error) {
-	var buf bytes.Buffer
-	err := lnwire.EncodeFailure(&buf, e.WireMessage(), 0)
-	if err != nil {
-		return "", fmt.Errorf("failed to encode wire message: %w", err)
+// newTrackOnionFailureResponse is a helper function that wraps a
+// PaymentFailureDetails message in a TrackOnionResponse.
+func newTrackOnionFailureResponse(
+	details *FailureDetails) *TrackOnionResponse {
+
+	return &TrackOnionResponse{
+		Result: &TrackOnionResponse_FailureDetails{
+			FailureDetails: details,
+		},
+	}
+}
+
+// marshallFailureDetails creates the FailureDetails message for the
+// TrackOnion response body.
+func marshallFailureDetails(err error) *FailureDetails {
+	var (
+		clearTextErr htlcswitch.ClearTextError
+		fwdErr       *htlcswitch.ForwardingError
+	)
+
+	details := &FailureDetails{
+		ErrorMessage: err.Error(),
 	}
 
-	return fmt.Sprintf("%d@%s", e.FailureSourceIdx,
-		hex.EncodeToString(buf.Bytes())), nil
+	switch {
+	case errors.Is(err, htlcswitch.ErrPaymentIDNotFound):
+		details.ErrorCode = ErrorCode_PAYMENT_ID_NOT_FOUND
+
+	case errors.Is(err, htlcswitch.ErrUnreadableFailureMessage):
+		details.ErrorCode = ErrorCode_UNREADABLE_FAILURE_MESSAGE
+
+	case errors.Is(err, htlcswitch.ErrSwitchExiting):
+		details.ErrorCode = ErrorCode_SWITCH_EXITING
+
+	case errors.As(err, &clearTextErr):
+		var buf bytes.Buffer
+
+		encodeErr := lnwire.EncodeFailure(
+			&buf, clearTextErr.WireMessage(), 0,
+		)
+		if encodeErr != nil {
+			log.Errorf("failed to encode wire message: %v",
+				encodeErr)
+			details.ErrorCode = ErrorCode_INTERNAL
+
+			return details
+		}
+
+		if errors.As(err, &fwdErr) {
+			details.Failure = &FailureDetails_ForwardingFailure{
+				ForwardingFailure: &ForwardingFailure{
+					FailureSourceIndex: uint32(
+						fwdErr.FailureSourceIdx,
+					),
+					WireMessage: buf.Bytes(),
+				},
+			}
+		} else {
+			details.Failure = &FailureDetails_ClearTextFailure{
+				ClearTextFailure: &ClearTextFailure{
+					WireMessage: buf.Bytes(),
+				},
+			}
+		}
+
+	default:
+		details.ErrorCode = ErrorCode_INTERNAL
+	}
+
+	return details
 }
 
-// ParseForwardingError converts an error from the format in SendOnion rpc
-// protos to a forwarding error type.
-func ParseForwardingError(errStr string) (*htlcswitch.ForwardingError, error) {
-	parts := strings.SplitN(errStr, "@", 2)
-	if len(parts) != 2 {
-		return nil, fmt.Errorf("invalid forwarding error format: %s",
-			errStr)
+// UnmarshallForwardingError converts a protobuf ForwardingFailure message into
+// an htlcswitch.ForwardingError.
+func UnmarshallForwardingError(f *ForwardingFailure) (
+	*htlcswitch.ForwardingError, error) {
+
+	if f == nil {
+		return nil, fmt.Errorf("cannot parse nil ForwardingFailure")
 	}
 
-	idx, err := strconv.Atoi(parts[0])
+	wireMsg, err := UnmarshallFailureMessage(f.WireMessage)
 	if err != nil {
-		return nil, fmt.Errorf("invalid forwarding error index: %s",
-			errStr)
+		return nil, fmt.Errorf("failed to decode wire message: %w", err)
 	}
 
-	wireMsgBytes, err := hex.DecodeString(parts[1])
-	if err != nil {
-		return nil, fmt.Errorf("invalid forwarding error wire "+
-			"message: %s", errStr)
+	return htlcswitch.NewForwardingError(
+		wireMsg, int(f.FailureSourceIndex),
+	), nil
+}
+
+// UnmarshallLinkError converts a protobuf ClearTextFailure message into the an
+// htlcswitch.LinkError.
+func UnmarshallLinkError(f *ClearTextFailure) (*htlcswitch.LinkError, error) {
+	if f == nil {
+		return nil, fmt.Errorf("cannot parse nil ClearTextFailure")
 	}
 
-	r := bytes.NewReader(wireMsgBytes)
-	wireMsg, err := lnwire.DecodeFailure(r, 0)
+	wireMsg, err := UnmarshallFailureMessage(f.WireMessage)
 	if err != nil {
-		return nil, fmt.Errorf("failed to decode wire message: %w",
-			err)
+		return nil, fmt.Errorf("failed to decode wire message: %w", err)
 	}
 
-	return htlcswitch.NewForwardingError(wireMsg, idx), nil
+	return htlcswitch.NewLinkError(wireMsg), nil
+}
+
+// UnmarshallFailureMessage decodes a raw wire message byte slice into a rich
+// lnwire.FailureMessage object.
+func UnmarshallFailureMessage(wireMsg []byte) (lnwire.FailureMessage, error) {
+	r := bytes.NewReader(wireMsg)
+
+	return lnwire.DecodeFailure(r, 0)
 }