Merge pull request #3818 from joostjager/check-max-onion

routing: send to route max hops check

go.mod

@@ -34,7 +34,7 @@ require (
 	github.com/juju/version v0.0.0-20180108022336-b64dbd566305 // indirect
 	github.com/kkdai/bstream v0.0.0-20181106074824-b3251f7901ec
 	github.com/lightninglabs/neutrino v0.11.0
-	github.com/lightningnetwork/lightning-onion v0.0.0-20190909101754-850081b08b6a
+	github.com/lightningnetwork/lightning-onion v0.0.0-20191214001659-f34e9dc1651d
 	github.com/lightningnetwork/lnd/cert v1.0.0
 	github.com/lightningnetwork/lnd/queue v1.0.2
 	github.com/lightningnetwork/lnd/ticker v1.0.0

go.sum

@@ -132,8 +132,8 @@ github.com/lightninglabs/gozmq v0.0.0-20191113021534-d20a764486bf h1:HZKvJUHlcXI
 github.com/lightninglabs/gozmq v0.0.0-20191113021534-d20a764486bf/go.mod h1:vxmQPeIQxPf6Jf9rM8R+B4rKBqLA2AjttNxkFBL2Plk=
 github.com/lightninglabs/neutrino v0.11.0 h1:lPpYFCtsfJX2W5zI4pWycPmbbBdr7zU+BafYdLoD6k0=
 github.com/lightninglabs/neutrino v0.11.0/go.mod h1:CuhF0iuzg9Sp2HO6ZgXgayviFTn1QHdSTJlMncK80wg=
-github.com/lightningnetwork/lightning-onion v0.0.0-20190909101754-850081b08b6a h1:GoWPN4i4jTKRxhVNh9a2vvBBO1Y2seiJB+SopUYoKyo=
-github.com/lightningnetwork/lightning-onion v0.0.0-20190909101754-850081b08b6a/go.mod h1:rigfi6Af/KqsF7Za0hOgcyq2PNH4AN70AaMRxcJkff4=
+github.com/lightningnetwork/lightning-onion v0.0.0-20191214001659-f34e9dc1651d h1:U50MHOOeL6gR3Ee/l0eMvZMpmRo+ydzmlQuIruCyCsA=
+github.com/lightningnetwork/lightning-onion v0.0.0-20191214001659-f34e9dc1651d/go.mod h1:rigfi6Af/KqsF7Za0hOgcyq2PNH4AN70AaMRxcJkff4=
 github.com/ltcsuite/ltcd v0.0.0-20190101042124-f37f8bf35796 h1:sjOGyegMIhvgfq5oaue6Td+hxZuf3tDC8lAPrFldqFw=
 github.com/ltcsuite/ltcd v0.0.0-20190101042124-f37f8bf35796/go.mod h1:3p7ZTf9V1sNPI5H8P3NkTFF4LuwMdPl2DodF60qAKqY=
 github.com/ltcsuite/ltcutil v0.0.0-20181217130922-17f3b04680b6/go.mod h1:8Vg/LTOO0KYa/vlHWJ6XZAevPQThGH5sufO0Hrou/lA=

routing/payment_lifecycle.go

@@ -74,7 +74,9 @@ func (p *paymentLifecycle) resumePayment() ([32]byte, *route.Route, error) {
 			}
 		} else {
 			// If this was a resumed attempt, we must regenerate the
-			// circuit.
+			// circuit. We don't need to check for errors resulting
+			// from an invalid route, because the sphinx packet has
+			// been successfully generated before.
 			_, c, err := generateSphinxPacket(
 				&p.attempt.Route, p.payment.PaymentHash[:],
 				p.attempt.SessionKey,
@@ -239,7 +241,7 @@ func (p *paymentLifecycle) createNewPaymentAttempt() (lnwire.ShortChannelID,
 	}
 
 	// Create a new payment attempt from the given payment session.
-	route, err := p.paySession.RequestRoute(
+	rt, err := p.paySession.RequestRoute(
 		p.payment, uint32(p.currentHeight), p.finalCLTVDelta,
 	)
 	if err != nil {
@@ -279,8 +281,26 @@ func (p *paymentLifecycle) createNewPaymentAttempt() (lnwire.ShortChannelID,
 	// with the htlcAdd message that we send directly to the
 	// switch.
 	onionBlob, c, err := generateSphinxPacket(
-		route, p.payment.PaymentHash[:], sessionKey,
+		rt, p.payment.PaymentHash[:], sessionKey,
 	)
+
+	// With SendToRoute, it can happen that the route exceeds protocol
+	// constraints. Mark the payment as failed with an internal error.
+	if err == route.ErrMaxRouteHopsExceeded ||
+		err == sphinx.ErrMaxRoutingInfoSizeExceeded {
+
+		log.Debugf("Invalid route provided for payment %x: %v",
+			p.payment.PaymentHash, err)
+
+		controlErr := p.router.cfg.Control.Fail(
+			p.payment.PaymentHash, channeldb.FailureReasonError,
+		)
+		if controlErr != nil {
+			return lnwire.ShortChannelID{}, nil, controlErr
+		}
+	}
+
+	// In any case, don't continue if there is an error.
 	if err != nil {
 		return lnwire.ShortChannelID{}, nil, err
 	}
@@ -293,8 +313,8 @@ func (p *paymentLifecycle) createNewPaymentAttempt() (lnwire.ShortChannelID,
 	// metadata within this packet will be used to route the
 	// payment through the network, starting with the first-hop.
 	htlcAdd := &lnwire.UpdateAddHTLC{
-		Amount:      route.TotalAmount,
-		Expiry:      route.TotalTimeLock,
+		Amount:      rt.TotalAmount,
+		Expiry:      rt.TotalTimeLock,
 		PaymentHash: p.payment.PaymentHash,
 	}
 	copy(htlcAdd.OnionBlob[:], onionBlob)
@@ -303,7 +323,7 @@ func (p *paymentLifecycle) createNewPaymentAttempt() (lnwire.ShortChannelID,
 	// the payment. If this attempt fails, then we'll continue on
 	// to the next available route.
 	firstHop := lnwire.NewShortChanIDFromInt(
-		route.Hops[0].ChannelID,
+		rt.Hops[0].ChannelID,
 	)
 
 	// We generate a new, unique payment ID that we will use for
@@ -318,7 +338,7 @@ func (p *paymentLifecycle) createNewPaymentAttempt() (lnwire.ShortChannelID,
 	p.attempt = &channeldb.PaymentAttemptInfo{
 		PaymentID:  paymentID,
 		SessionKey: sessionKey,
-		Route:      *route,
+		Route:      *rt,
 	}
 
 	// Before sending this HTLC to the switch, we checkpoint the

routing/route/route.go

@@ -26,6 +26,10 @@ var (
 	// each route.
 	ErrNoRouteHopsProvided = fmt.Errorf("empty route hops provided")
 
+	// ErrMaxRouteHopsExceeded is returned when a caller attempts to
+	// construct a new sphinx packet, but provides too many hops.
+	ErrMaxRouteHopsExceeded = fmt.Errorf("route has too many hops")
+
 	// ErrIntermediateMPPHop is returned when a hop tries to deliver an MPP
 	// record to an intermediate hop, only final hops can receive MPP
 	// records.
@@ -267,6 +271,16 @@ func NewRouteFromHops(amtToSend lnwire.MilliSatoshi, timeLock uint32,
 func (r *Route) ToSphinxPath() (*sphinx.PaymentPath, error) {
 	var path sphinx.PaymentPath
 
+	// We can only construct a route if there are hops provided.
+	if len(r.Hops) == 0 {
+		return nil, ErrNoRouteHopsProvided
+	}
+
+	// Check maximum route length.
+	if len(r.Hops) > sphinx.NumMaxHops {
+		return nil, ErrMaxRouteHopsExceeded
+	}
+
 	// For each hop encoded within the route, we'll convert the hop struct
 	// to an OnionHop with matching per-hop payload within the path as used
 	// by the sphinx package.

routing/router.go

@@ -1487,13 +1487,6 @@ func generateNewSessionKey() (*btcec.PrivateKey, error) {
 func generateSphinxPacket(rt *route.Route, paymentHash []byte,
 	sessionKey *btcec.PrivateKey) ([]byte, *sphinx.Circuit, error) {
 
-	// As a sanity check, we'll ensure that the set of hops has been
-	// properly filled in, otherwise, we won't actually be able to
-	// construct a route.
-	if len(rt.Hops) == 0 {
-		return nil, nil, route.ErrNoRouteHopsProvided
-	}
-
 	// Now that we know we have an actual route, we'll map the route into a
 	// sphinx payument path which includes per-hop paylods for each hop
 	// that give each node within the route the necessary information

routing/router_test.go

@@ -3339,6 +3339,74 @@ func TestSendToRouteStructuredError(t *testing.T) {
 	}
 }
 
+// TestSendToRouteMaxHops asserts that SendToRoute fails when using a route that
+// exceeds the maximum number of hops.
+func TestSendToRouteMaxHops(t *testing.T) {
+	t.Parallel()
+
+	// Setup a two node network.
+	chanCapSat := btcutil.Amount(100000)
+	testChannels := []*testChannel{
+		symmetricTestChannel("a", "b", chanCapSat, &testChannelPolicy{
+			Expiry:  144,
+			FeeRate: 400,
+			MinHTLC: 1,
+			MaxHTLC: lnwire.NewMSatFromSatoshis(chanCapSat),
+		}, 1),
+	}
+
+	testGraph, err := createTestGraphFromChannels(testChannels, "a")
+	if err != nil {
+		t.Fatalf("unable to create graph: %v", err)
+	}
+	defer testGraph.cleanUp()
+
+	const startingBlockHeight = 101
+
+	ctx, cleanUp, err := createTestCtxFromGraphInstance(
+		startingBlockHeight, testGraph,
+	)
+	if err != nil {
+		t.Fatalf("unable to create router: %v", err)
+	}
+	defer cleanUp()
+
+	// Create a 30 hop route that exceeds the maximum hop limit.
+	const payAmt = lnwire.MilliSatoshi(10000)
+	hopA := ctx.aliases["a"]
+	hopB := ctx.aliases["b"]
+
+	var hops []*route.Hop
+	for i := 0; i < 15; i++ {
+		hops = append(hops, &route.Hop{
+			ChannelID:     1,
+			PubKeyBytes:   hopB,
+			AmtToForward:  payAmt,
+			LegacyPayload: true,
+		})
+
+		hops = append(hops, &route.Hop{
+			ChannelID:     1,
+			PubKeyBytes:   hopA,
+			AmtToForward:  payAmt,
+			LegacyPayload: true,
+		})
+	}
+
+	rt, err := route.NewRouteFromHops(payAmt, 100, ctx.aliases["a"], hops)
+	if err != nil {
+		t.Fatalf("unable to create route: %v", err)
+	}
+
+	// Send off the payment request to the router. We expect an error back
+	// indicating that the route is too long.
+	var payment lntypes.Hash
+	_, err = ctx.router.SendToRoute(payment, rt)
+	if err != route.ErrMaxRouteHopsExceeded {
+		t.Fatalf("expected ErrMaxRouteHopsExceeded, but got %v", err)
+	}
+}
+
 // TestBuildRoute tests whether correct routes are built.
 func TestBuildRoute(t *testing.T) {
 	// Setup a three node network.