Merge pull request #6766 from andreihod/fix-infinite-loop-error

routing: Fix possible infinite loop on first hop unknown next peer

docs/release-notes/release-notes-0.16.0.md

@@ -128,6 +128,9 @@ certain large transactions](https://github.com/lightningnetwork/lnd/pull/7100).
 * [Update cert module](https://github.com/lightningnetwork/lnd/pull/6573) to
   allow a way to update the tls certificate without restarting lnd.
 
+* [Fixed a bug where paying an invoice with a malformed route hint triggers a
+  never-ending retry loop](https://github.com/lightningnetwork/lnd/pull/6766)
+
 ## `lncli`
 * [Add an `insecure` flag to skip tls auth as well as a `metadata` string slice
   flag](https://github.com/lightningnetwork/lnd/pull/6818) that allows the
@@ -221,6 +224,7 @@ to refactor the itest for code health and maintenance.
 
 # Contributors (Alphabetical Order)
 
+* andreihod
 * Carla Kirk-Cohen
 * cutiful
 * Daniel McNally
@@ -232,6 +236,7 @@ to refactor the itest for code health and maintenance.
 * Jesse de Wit
 * Joost Jager
 * Jordi Montes
+* lsunsi
 * Matt Morehouse
 * Michael Street
 * Jordi Montes

routing/integrated_routing_context_test.go

@@ -10,6 +10,7 @@ import (
 	"github.com/lightningnetwork/lnd/kvdb"
 	"github.com/lightningnetwork/lnd/lnwire"
 	"github.com/lightningnetwork/lnd/routing/route"
+	"github.com/lightningnetwork/lnd/zpay32"
 )
 
 const (
@@ -47,6 +48,7 @@ type integratedRoutingContext struct {
 
 	mcCfg          MissionControlConfig
 	pathFindingCfg PathFindingConfig
+	routeHints     [][]zpay32.HopHint
 }
 
 // newIntegratedRoutingContext instantiates a new integrated routing test
@@ -177,6 +179,7 @@ func (c *integratedRoutingContext) testPayment(maxParts uint32,
 		Amount:         c.amt,
 		CltvLimit:      math.MaxUint32,
 		MaxParts:       maxParts,
+		RouteHints:     c.routeHints,
 	}
 
 	var paymentHash [32]byte

routing/integrated_routing_test.go

@@ -4,9 +4,11 @@ import (
 	"fmt"
 	"testing"
 
+	"github.com/btcsuite/btcd/btcec/v2"
 	"github.com/btcsuite/btcd/btcutil"
 	"github.com/davecgh/go-spew/spew"
 	"github.com/lightningnetwork/lnd/lnwire"
+	"github.com/lightningnetwork/lnd/zpay32"
 	"github.com/stretchr/testify/require"
 )
 
@@ -234,6 +236,34 @@ var mppTestCases = []mppSendTestCase{
 	},
 }
 
+// TestBadFirstHopHint tests that a payment with a first hop hint with an
+// invalid channel id still works since the node already knows its channel and
+// doesn't need hints.
+func TestBadFirstHopHint(t *testing.T) {
+	t.Parallel()
+
+	ctx := newIntegratedRoutingContext(t)
+
+	onePathGraph(ctx.graph)
+
+	sourcePubKey, _ := btcec.ParsePubKey(ctx.source.pubkey[:])
+
+	hopHint := zpay32.HopHint{
+		NodeID:                    sourcePubKey,
+		ChannelID:                 66,
+		FeeBaseMSat:               0,
+		FeeProportionalMillionths: 0,
+		CLTVExpiryDelta:           100,
+	}
+
+	ctx.routeHints = [][]zpay32.HopHint{{hopHint}}
+
+	ctx.amt = lnwire.NewMSatFromSatoshis(100)
+	_, err := ctx.testPayment(1)
+
+	require.NoError(t, err, "payment failed")
+}
+
 // TestMppSend tests that a payment can be completed using multiple shards.
 func TestMppSend(t *testing.T) {
 	for _, testCase := range mppTestCases {

routing/mock_graph_test.go

@@ -5,6 +5,7 @@ import (
 	"fmt"
 	"testing"
 
+	"github.com/btcsuite/btcd/btcec/v2"
 	"github.com/btcsuite/btcd/btcutil"
 	"github.com/lightningnetwork/lnd/channeldb"
 	"github.com/lightningnetwork/lnd/lnwire"
@@ -13,7 +14,12 @@ import (
 
 // createPubkey return a new test pubkey.
 func createPubkey(id byte) route.Vertex {
-	pubkey := route.Vertex{id}
+	_, secpPub := btcec.PrivKeyFromBytes([]byte{id})
+
+	var bytes [33]byte
+	copy(bytes[:], secpPub.SerializeCompressed()[:33])
+
+	pubkey := route.Vertex(bytes)
 	return pubkey
 }
 

routing/pathfind.go

@@ -538,10 +538,18 @@ func findPath(g *graphParams, r *RestrictParams, cfg *PathFindingConfig,
 
 	additionalEdgesWithSrc := make(map[route.Vertex][]*edgePolicyWithSource)
 	for vertex, outgoingEdgePolicies := range g.additionalEdges {
+		// Edges connected to self are always included in the graph,
+		// therefore can be skipped. This prevents us from trying
+		// routes to malformed hop hints.
+		if vertex == self {
+			continue
+		}
+
 		// Build reverse lookup to find incoming edges. Needed because
 		// search is taken place from target to source.
 		for _, outgoingEdgePolicy := range outgoingEdgePolicies {
 			toVertex := outgoingEdgePolicy.ToNodePubKey()
+
 			incomingEdgePolicy := &edgePolicyWithSource{
 				sourceNode: vertex,
 				edge:       outgoingEdgePolicy,

routing/pathfind_test.go

@@ -773,6 +773,9 @@ func TestPathFinding(t *testing.T) {
 	}, {
 		name: "path finding with additional edges",
 		fn:   runPathFindingWithAdditionalEdges,
+	}, {
+		name: "path finding with redundant additional edges",
+		fn:   runPathFindingWithRedundantAdditionalEdges,
 	}, {
 		name: "new route path too long",
 		fn:   runNewRoutePathTooLong,
@@ -1290,6 +1293,58 @@ func runPathFindingWithAdditionalEdges(t *testing.T, useCache bool) {
 	assertExpectedPath(t, graph.aliasMap, path, "songoku", "doge")
 }
 
+// runPathFindingWithRedundantAdditionalEdges asserts that we are able to find
+// paths to nodes ignoring additional edges that are already known by self node.
+func runPathFindingWithRedundantAdditionalEdges(t *testing.T, useCache bool) {
+	t.Helper()
+
+	var realChannelID uint64 = 3145
+	var hintChannelID uint64 = 1618
+
+	testChannels := []*testChannel{
+		symmetricTestChannel("alice", "bob", 100000, &testChannelPolicy{
+			Expiry:  144,
+			FeeRate: 400,
+			MinHTLC: 1,
+			MaxHTLC: 100000000,
+		}, realChannelID),
+	}
+
+	ctx := newPathFindingTestContext(t, useCache, testChannels, "alice")
+
+	target := ctx.keyFromAlias("bob")
+	paymentAmt := lnwire.NewMSatFromSatoshis(100)
+
+	// Create the channel edge going from alice to bob and include it in
+	// our map of additional edges.
+	aliceToBob := &channeldb.CachedEdgePolicy{
+		ToNodePubKey: func() route.Vertex {
+			return target
+		},
+		ToNodeFeatures:            lnwire.EmptyFeatureVector(),
+		ChannelID:                 hintChannelID,
+		FeeBaseMSat:               1,
+		FeeProportionalMillionths: 1000,
+		TimeLockDelta:             9,
+	}
+
+	additionalEdges := map[route.Vertex][]*channeldb.CachedEdgePolicy{
+		ctx.source: {aliceToBob},
+	}
+
+	path, err := dbFindPath(
+		ctx.graph, additionalEdges, ctx.bandwidthHints,
+		&ctx.restrictParams, &ctx.pathFindingConfig, ctx.source, target,
+		paymentAmt, ctx.timePref, 0,
+	)
+
+	require.NoError(t, err, "unable to find path to bob")
+	require.Len(t, path, 1)
+
+	require.Equal(t, realChannelID, path[0].ChannelID,
+		"additional edge for known edge wasn't ignored")
+}
+
 // TestNewRoute tests whether the construction of hop payloads by newRoute
 // is executed correctly.
 func TestNewRoute(t *testing.T) {

routing/router_test.go

@@ -361,6 +361,79 @@ func TestSendPaymentRouteFailureFallback(t *testing.T) {
 	)
 }
 
+// TestSendPaymentRouteInfiniteLoopWithBadHopHint tests that when sending
+// a payment with a malformed hop hint in the first hop, the hint is ignored
+// and the payment succeeds without an infinite loop of retries.
+func TestSendPaymentRouteInfiniteLoopWithBadHopHint(t *testing.T) {
+	t.Parallel()
+
+	const startingBlockHeight = 101
+	ctx := createTestCtxFromFile(t, startingBlockHeight, basicGraphFilePath)
+
+	source := ctx.aliases["roasbeef"]
+	sourceNodeID, err := btcec.ParsePubKey(source[:])
+	require.NoError(t, err)
+
+	actualChannelID := ctx.getChannelIDFromAlias(t, "roasbeef", "songoku")
+	badChannelID := uint64(66666)
+
+	// Craft a LightningPayment struct that'll send a payment from roasbeef
+	// to songoku for 1000 satoshis.
+	var payHash lntypes.Hash
+	paymentAmt := lnwire.NewMSatFromSatoshis(1000)
+	payment := LightningPayment{
+		Target:      ctx.aliases["songoku"],
+		Amount:      paymentAmt,
+		FeeLimit:    noFeeLimit,
+		paymentHash: &payHash,
+		RouteHints: [][]zpay32.HopHint{{
+			zpay32.HopHint{
+				NodeID:          sourceNodeID,
+				ChannelID:       badChannelID,
+				FeeBaseMSat:     uint32(50),
+				CLTVExpiryDelta: uint16(200),
+			},
+		}},
+	}
+
+	var preImage [32]byte
+	copy(preImage[:], bytes.Repeat([]byte{9}, 32))
+
+	// Mock a payment result that always fails with FailUnknownNextPeer when
+	// the bad channel is the first hop.
+	badShortChanID := lnwire.NewShortChanIDFromInt(badChannelID)
+	newFwdError := htlcswitch.NewForwardingError(
+		&lnwire.FailUnknownNextPeer{}, 0,
+	)
+
+	payer, ok := ctx.router.cfg.Payer.(*mockPaymentAttemptDispatcherOld)
+	require.Equal(t, ok, true, "failed Payer cast")
+
+	payer.setPaymentResult(
+		func(firstHop lnwire.ShortChannelID) ([32]byte, error) {
+			// Returns a FailUnknownNextPeer if it's trying
+			// to pay an invalid channel.
+			if firstHop == badShortChanID {
+				return [32]byte{}, newFwdError
+			}
+
+			return preImage, nil
+		})
+
+	// Send off the payment request to the router, should succeed
+	// ignoring the bad channel id hint.
+	paymentPreImage, route, paymentErr := ctx.router.SendPayment(&payment)
+	require.NoError(t, paymentErr, "payment returned an error")
+
+	// The preimage should match up with the one created above.
+	require.Equal(t, preImage[:], paymentPreImage[:], "incorrect preimage")
+
+	// The route should have songoku as the first hop.
+	require.Equal(t, actualChannelID, route.Hops[0].ChannelID,
+		"route should go through the correct channel id",
+	)
+}
+
 // TestChannelUpdateValidation tests that a failed payment with an associated
 // channel update will only be applied to the graph when the update contains a
 // valid signature.
@@ -903,7 +976,6 @@ func TestSendPaymentErrorNonFinalTimeLockErrors(t *testing.T) {
 	// only channel to Sophon.
 	ctx.router.cfg.Payer.(*mockPaymentAttemptDispatcherOld).setPaymentResult(
 		func(firstHop lnwire.ShortChannelID) ([32]byte, error) {
-
 			if firstHop == roasbeefSongoku {
 				return [32]byte{}, htlcswitch.NewForwardingError(
 					&lnwire.FailExpiryTooSoon{
@@ -951,7 +1023,6 @@ func TestSendPaymentErrorNonFinalTimeLockErrors(t *testing.T) {
 	// around the faulty Son Goku node.
 	ctx.router.cfg.Payer.(*mockPaymentAttemptDispatcherOld).setPaymentResult(
 		func(firstHop lnwire.ShortChannelID) ([32]byte, error) {
-
 			if firstHop == roasbeefSongoku {
 				return [32]byte{}, htlcswitch.NewForwardingError(
 					&lnwire.FailIncorrectCltvExpiry{