diff --git a/docs/release-notes/release-notes-0.16.0.md b/docs/release-notes/release-notes-0.16.0.md index 8a443f035..d6df7387b 100644 --- a/docs/release-notes/release-notes-0.16.0.md +++ b/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 diff --git a/routing/integrated_routing_context_test.go b/routing/integrated_routing_context_test.go index d3baa3b41..3c5b63c6f 100644 --- a/routing/integrated_routing_context_test.go +++ b/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 diff --git a/routing/integrated_routing_test.go b/routing/integrated_routing_test.go index 0962f45f5..2dd2cf632 100644 --- a/routing/integrated_routing_test.go +++ b/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 { diff --git a/routing/mock_graph_test.go b/routing/mock_graph_test.go index 33a9c11af..79d46d012 100644 --- a/routing/mock_graph_test.go +++ b/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 } diff --git a/routing/pathfind.go b/routing/pathfind.go index a92d0a7e5..0d0a1e920 100644 --- a/routing/pathfind.go +++ b/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, diff --git a/routing/pathfind_test.go b/routing/pathfind_test.go index c14317059..a3202d1e9 100644 --- a/routing/pathfind_test.go +++ b/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) { diff --git a/routing/router_test.go b/routing/router_test.go index 8c1edd253..a3f832b82 100644 --- a/routing/router_test.go +++ b/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{