Repository: lightningnetwork/lnd
Pull request #9433 (2025-01-22 15:07:06)

routerrpc: fix estimateroutefee for public route hints
This PR attempts to fix https://github.com/lightningnetwork/lnd/issues/9431, an edge case in `EstimateRouteFee`.

We want to send a probe in case a route hint contains a public edge.

lightninglabs-deploy: @hieblmi, remember to re-request review from reviewers when ready
lightninglabs-deploy: @gijswijs: review reminder
@bitromortac: review reminder

Patch:
diff --git a/docs/release-notes/release-notes-0.19.0.md b/docs/release-notes/release-notes-0.19.0.md
index 781c8a8d1..3c8e53e76 100644
--- a/docs/release-notes/release-notes-0.19.0.md
+++ b/docs/release-notes/release-notes-0.19.0.md
@@ -62,7 +62,11 @@
   https://github.com/lightningnetwork/lnd/pull/9253)
 
 * [Fixed a bug](https://github.com/lightningnetwork/lnd/pull/9322) that caused
-    estimateroutefee to ignore the default payment timeout.
+  estimateroutefee to ignore the default payment timeout.
+
+* [Fixed a bug](https://github.com/lightningnetwork/lnd/pull/9433) that caused 
+  `estimateroutefee` to assume probing an LSP when given an invoice with a route 
+  hint containing a public channel to the destination.
 
 * [Fix a bug](https://github.com/lightningnetwork/lnd/pull/9474) where LND would
   fail to persist (and hence, propagate) node announcements containing address 
diff --git a/graph/builder.go b/graph/builder.go
index 3e1115553..c379fe93f 100644
--- a/graph/builder.go
+++ b/graph/builder.go
@@ -123,10 +123,10 @@ type Builder struct {
 	// of our currently known best chain are sent over.
 	staleBlocks <-chan *chainview.FilteredBlock
 
-	// topologyUpdates is a channel that carries new topology updates
+	// networkUpdates is a channel that carries new topology updates
 	// messages from outside the Builder to be processed by the
 	// networkHandler.
-	topologyUpdates chan any
+	networkUpdates chan *routingMsg
 
 	// topologyClients maps a client's unique notification ID to a
 	// topologyClient client that contains its notification dispatch
@@ -164,7 +164,7 @@ var _ ChannelGraphSource = (*Builder)(nil)
 func NewBuilder(cfg *Config) (*Builder, error) {
 	return &Builder{
 		cfg:               cfg,
-		topologyUpdates:   make(chan any),
+		networkUpdates:    make(chan *routingMsg),
 		topologyClients:   &lnutils.SyncMap[uint64, *topologyClient]{},
 		ntfnClientUpdates: make(chan *topologyClientUpdate),
 		channelEdgeMtx:    multimutex.NewMutex[uint64](),
@@ -656,26 +656,59 @@ func (b *Builder) pruneZombieChans() error {
 	return nil
 }
 
-// handleTopologyUpdate is responsible for sending any topology changes
-// notifications to registered clients.
+// handleNetworkUpdate is responsible for processing the update message and
+// notifies topology changes, if any.
 //
 // NOTE: must be run inside goroutine.
-func (b *Builder) handleTopologyUpdate(update any) {
+func (b *Builder) handleNetworkUpdate(update *routingMsg) {
 	defer b.wg.Done()
 
+	// Process the routing update to determine if this is either a new
+	// update from our PoV or an update to a prior vertex/edge we
+	// previously accepted.
+	var err error
+	switch msg := update.msg.(type) {
+	case *models.LightningNode:
+		err = b.addNode(msg, update.op...)
+
+	case *models.ChannelEdgeInfo:
+		err = b.addEdge(msg, update.op...)
+
+	case *models.ChannelEdgePolicy:
+		err = b.updateEdge(msg, update.op...)
+
+	default:
+		err = errors.Errorf("wrong routing update message type")
+	}
+	update.err <- err
+
+	// If the error is not nil here, there's no need to send topology
+	// change.
+	if err != nil {
+		// Log as a debug message if this is not an error we need to be
+		// concerned about.
+		if IsError(err, ErrIgnored, ErrOutdated) {
+			log.Debugf("process network updates got: %v", err)
+		} else {
+			log.Errorf("process network updates got: %v", err)
+		}
+
+		return
+	}
+
+	// Otherwise, we'll send off a new notification for the newly accepted
+	// update, if any.
 	topChange := &TopologyChange{}
-	err := addToTopologyChange(b.cfg.Graph, topChange, update)
+	err = addToTopologyChange(b.cfg.Graph, topChange, update.msg)
 	if err != nil {
 		log.Errorf("unable to update topology change notification: %v",
 			err)
 		return
 	}
 
-	if topChange.isEmpty() {
-		return
+	if !topChange.isEmpty() {
+		b.notifyTopologyChange(topChange)
 	}
-
-	b.notifyTopologyChange(topChange)
 }
 
 // networkHandler is the primary goroutine for the Builder. The roles of
@@ -701,11 +734,12 @@ func (b *Builder) networkHandler() {
 		}
 
 		select {
-		// A new fully validated topology update has just arrived.
-		// We'll notify any registered clients.
-		case update := <-b.topologyUpdates:
+		// A new fully validated network update has just arrived. As a
+		// result we'll modify the channel graph accordingly depending
+		// on the exact type of the message.
+		case update := <-b.networkUpdates:
 			b.wg.Add(1)
-			go b.handleTopologyUpdate(update)
+			go b.handleNetworkUpdate(update)
 
 			// TODO(roasbeef): remove all unconnected vertexes
 			// after N blocks pass with no corresponding
@@ -999,6 +1033,14 @@ func (b *Builder) MarkZombieEdge(chanID uint64) error {
 	return nil
 }
 
+// routingMsg couples a routing related routing topology update to the
+// error channel.
+type routingMsg struct {
+	msg interface{}
+	op  []batch.SchedulerOption
+	err chan error
+}
+
 // ApplyChannelUpdate validates a channel update and if valid, applies it to the
 // database. It returns a bool indicating whether the updates were successful.
 func (b *Builder) ApplyChannelUpdate(msg *lnwire.ChannelUpdate1) bool {
@@ -1060,20 +1102,23 @@ func (b *Builder) ApplyChannelUpdate(msg *lnwire.ChannelUpdate1) bool {
 func (b *Builder) AddNode(node *models.LightningNode,
 	op ...batch.SchedulerOption) error {
 
-	err := b.addNode(node, op...)
-	if err != nil {
-		logNetworkMsgProcessError(err)
-
-		return err
+	rMsg := &routingMsg{
+		msg: node,
+		op:  op,
+		err: make(chan error, 1),
 	}
 
 	select {
-	case b.topologyUpdates <- node:
+	case b.networkUpdates <- rMsg:
+		select {
+		case err := <-rMsg.err:
+			return err
+		case <-b.quit:
+			return ErrGraphBuilderShuttingDown
+		}
 	case <-b.quit:
 		return ErrGraphBuilderShuttingDown
 	}
-
-	return nil
 }
 
 // addNode does some basic checks on the given LightningNode against what we
@@ -1110,20 +1155,23 @@ func (b *Builder) addNode(node *models.LightningNode,
 func (b *Builder) AddEdge(edge *models.ChannelEdgeInfo,
 	op ...batch.SchedulerOption) error {
 
-	err := b.addEdge(edge, op...)
-	if err != nil {
-		logNetworkMsgProcessError(err)
-
-		return err
+	rMsg := &routingMsg{
+		msg: edge,
+		op:  op,
+		err: make(chan error, 1),
 	}
 
 	select {
-	case b.topologyUpdates <- edge:
+	case b.networkUpdates <- rMsg:
+		select {
+		case err := <-rMsg.err:
+			return err
+		case <-b.quit:
+			return ErrGraphBuilderShuttingDown
+		}
 	case <-b.quit:
 		return ErrGraphBuilderShuttingDown
 	}
-
-	return nil
 }
 
 // addEdge does some validation on the new channel edge against what we
@@ -1217,20 +1265,23 @@ func (b *Builder) addEdge(edge *models.ChannelEdgeInfo,
 func (b *Builder) UpdateEdge(update *models.ChannelEdgePolicy,
 	op ...batch.SchedulerOption) error {
 
-	err := b.updateEdge(update, op...)
-	if err != nil {
-		logNetworkMsgProcessError(err)
-
-		return err
+	rMsg := &routingMsg{
+		msg: update,
+		op:  op,
+		err: make(chan error, 1),
 	}
 
 	select {
-	case b.topologyUpdates <- update:
+	case b.networkUpdates <- rMsg:
+		select {
+		case err := <-rMsg.err:
+			return err
+		case <-b.quit:
+			return ErrGraphBuilderShuttingDown
+		}
 	case <-b.quit:
 		return ErrGraphBuilderShuttingDown
 	}
-
-	return nil
 }
 
 // updateEdge validates the new edge policy against what we currently have
@@ -1324,18 +1375,6 @@ func (b *Builder) updateEdge(policy *models.ChannelEdgePolicy,
 	return nil
 }
 
-// logNetworkMsgProcessError logs the error received from processing a network
-// message. It logs as a debug message if the error is not critical.
-func logNetworkMsgProcessError(err error) {
-	if IsError(err, ErrIgnored, ErrOutdated) {
-		log.Debugf("process network updates got: %v", err)
-
-		return
-	}
-
-	log.Errorf("process network updates got: %v", err)
-}
-
 // CurrentBlockHeight returns the block height from POV of the router subsystem.
 //
 // NOTE: This method is part of the ChannelGraphSource interface.
diff --git a/lnrpc/routerrpc/router_server.go b/lnrpc/routerrpc/router_server.go
index c4e4d51a6..818464be7 100644
--- a/lnrpc/routerrpc/router_server.go
+++ b/lnrpc/routerrpc/router_server.go
@@ -171,6 +171,11 @@ var (
 	DefaultRouterMacFilename = "router.macaroon"
 )
 
+// FetchChannelEndpoints returns the pubkeys of both endpoints of the
+// given channel id if it exists in the graph.
+type FetchChannelEndpoints func(chanID uint64) (route.Vertex, route.Vertex,
+	error)
+
 // ServerShell is a shell struct holding a reference to the actual sub-server.
 // It is used to register the gRPC sub-server with the root server before we
 // have the necessary dependencies to populate the actual sub-server.
@@ -552,7 +557,7 @@ func (s *Server) probePaymentRequest(ctx context.Context, paymentRequest string,
 	// If the hints don't indicate an LSP then chances are that our probe
 	// payment won't be blocked along the route to the destination. We send
 	// a probe payment with unmodified route hints.
-	if !isLSP(hints) {
+	if !isLSP(hints, s.cfg.RouterBackend.FetchChannelEndpoints) {
 		probeRequest.RouteHints = invoicesrpc.CreateRPCRouteHints(hints)
 		return s.sendProbePayment(ctx, probeRequest)
 	}
@@ -625,14 +630,27 @@ func (s *Server) probePaymentRequest(ctx context.Context, paymentRequest string,
 }
 
 // isLSP checks if the route hints indicate an LSP. An LSP is indicated with
-// true if the last node in each route hint has the same node id, false
-// otherwise.
-func isLSP(routeHints [][]zpay32.HopHint) bool {
+// true if the destination hop hint in each route hint has the same node id,
+// false otherwise. If the destination hop hint of any route hint contains a
+// public channel, the function returns false because we can directly send a
+// probe to the final destination.
+func isLSP(routeHints [][]zpay32.HopHint,
+	fetchChannelEndpoints FetchChannelEndpoints) bool {
+
 	if len(routeHints) == 0 || len(routeHints[0]) == 0 {
 		return false
 	}
 
-	refNodeID := routeHints[0][len(routeHints[0])-1].NodeID
+	destHopHint := routeHints[0][len(routeHints[0])-1]
+
+	// If the destination hop hint of the first route hint contains a public
+	// channel we can send a probe to it directly, hence we don't signal an
+	// LSP.
+	_, _, err := fetchChannelEndpoints(destHopHint.ChannelID)
+	if err == nil {
+		return false
+	}
+
 	for i := 1; i < len(routeHints); i++ {
 		// Skip empty route hints.
 		if len(routeHints[i]) == 0 {
@@ -640,15 +658,27 @@ func isLSP(routeHints [][]zpay32.HopHint) bool {
 		}
 
 		lastHop := routeHints[i][len(routeHints[i])-1]
+
+		// If the last hop hint of any route hint contains a public
+		// channel we can send a probe to it directly, hence we don't
+		// signal an LSP.
+		_, _, err = fetchChannelEndpoints(lastHop.ChannelID)
+		if err == nil {
+			return false
+		}
+
 		idMatchesRefNode := bytes.Equal(
 			lastHop.NodeID.SerializeCompressed(),
-			refNodeID.SerializeCompressed(),
+			destHopHint.NodeID.SerializeCompressed(),
 		)
 		if !idMatchesRefNode {
 			return false
 		}
 	}
 
+	// We ensured that the destination hop hint doesn't contain a public
+	// channel, and that all destination hop hints of all route hints match,
+	// so we signal an LSP.
 	return true
 }
 


Test patch:
diff --git a/lnrpc/routerrpc/router_server_test.go b/lnrpc/routerrpc/router_server_test.go
index a4047977e..bc5a7f16d 100644
--- a/lnrpc/routerrpc/router_server_test.go
+++ b/lnrpc/routerrpc/router_server_test.go
@@ -7,10 +7,12 @@ import (
 
 	"github.com/btcsuite/btcd/btcec/v2"
 	"github.com/lightningnetwork/lnd/channeldb"
+	graphdb "github.com/lightningnetwork/lnd/graph/db"
 	"github.com/lightningnetwork/lnd/lnrpc"
 	"github.com/lightningnetwork/lnd/lnwire"
 	"github.com/lightningnetwork/lnd/queue"
 	"github.com/lightningnetwork/lnd/routing"
+	"github.com/lightningnetwork/lnd/routing/route"
 	"github.com/lightningnetwork/lnd/zpay32"
 	"github.com/stretchr/testify/require"
 	"google.golang.org/grpc"
@@ -269,6 +271,14 @@ func TestIsLsp(t *testing.T) {
 			FeeProportionalMillionths: 2_000,
 			ChannelID:                 815,
 		}
+
+		publicChannelID       = uint64(42)
+		daveHopHintPublicChan = zpay32.HopHint{
+			NodeID:                    davePubKey,
+			FeeBaseMSat:               2_000,
+			FeeProportionalMillionths: 2_000,
+			ChannelID:                 publicChannelID,
+		}
 	)
 
 	bobExpensiveCopy := bobHopHint.Copy()
@@ -383,11 +393,56 @@ func TestIsLsp(t *testing.T) {
 			expectedHints:  [][]zpay32.HopHint{},
 			expectedLspHop: nil,
 		},
+		{
+			name: "multiple routes, same public hops",
+			routeHints: [][]zpay32.HopHint{
+				{
+					aliceHopHint, daveHopHintPublicChan,
+				},
+				{
+					carolHopHint, daveHopHintPublicChan,
+				},
+			},
+			probeAmtMsat:   probeAmtMsat,
+			isLsp:          false,
+			expectedHints:  [][]zpay32.HopHint{},
+			expectedLspHop: nil,
+		},
+		{
+			name: "multiple routes, same public hops",
+			routeHints: [][]zpay32.HopHint{
+				{
+					aliceHopHint, daveHopHint,
+				},
+				{
+					carolHopHint, daveHopHintPublicChan,
+				},
+				{
+					aliceHopHint, daveHopHintPublicChan,
+				},
+			},
+			probeAmtMsat:   probeAmtMsat,
+			isLsp:          false,
+			expectedHints:  [][]zpay32.HopHint{},
+			expectedLspHop: nil,
+		},
+	}
+
+	// Returns ErrEdgeNotFound for private channels.
+	fetchChannelEndpoints := func(chanID uint64) (route.Vertex,
+		route.Vertex, error) {
+
+		if chanID == publicChannelID {
+			return route.Vertex{}, route.Vertex{}, nil
+		}
+
+		return route.Vertex{}, route.Vertex{}, graphdb.ErrEdgeNotFound
 	}
 
 	for _, tc := range lspTestCases {
 		t.Run(tc.name, func(t *testing.T) {
-			require.Equal(t, tc.isLsp, isLSP(tc.routeHints))
+			isLsp := isLSP(tc.routeHints, fetchChannelEndpoints)
+			require.Equal(t, tc.isLsp, isLsp)
 			if !tc.isLsp {
 				return
 			}