utils: fix multiple issues in SelectHopHints

utils.go

@@ -18,6 +18,23 @@ var (
 	DefaultMaxHopHints = 20
 )
 
+// chanCanBeHopHint checks whether the passed channel could be used as a private
+// hophint.
+func chanCanBeHopHint(chanInfo *lndclient.ChannelInfo) bool {
+	return chanInfo.Private && chanInfo.Active
+}
+
+// chanRemotePolicy selectes the correct remote routing policy.
+func chanRemotePolicy(remotePub route.Vertex,
+	edgeInfo *lndclient.ChannelEdge) *lndclient.RoutingPolicy {
+
+	if remotePub == edgeInfo.Node1 {
+		return edgeInfo.Node1Policy
+	}
+
+	return edgeInfo.Node2Policy
+}
+
 // SelectHopHints is a direct port of the SelectHopHints found in lnd. It was
 // reimplemented because the current implementation in LND relies on internals
 // not externalized through the API. Hopefully in the future SelectHopHints
@@ -44,50 +61,35 @@ func SelectHopHints(ctx context.Context, lnd *lndclient.LndServices,
 	// through GetChanInfo
 	chanInfoCache := make(map[uint64]*lndclient.ChannelEdge)
 
-	// skipCache is a simple cache which holds the indice of any
+	// skipCache is a simple cache which holds the indices of any channel
-	// channel we've added to final hopHints
+	// that we should skip when doing the second round of channel selection.
 	skipCache := make(map[int]struct{})
 
 	hopHints := make([][]zpay32.HopHint, 0, numMaxHophints)
 
 	for i, channel := range openChannels {
+		channel := channel
+
 		// In this first pass, we'll ignore all channels in
 		// isolation that can't satisfy this payment.
 
-		// Retrieve extra info for each channel not available in
+		// Skip public or inactive channels.
-		// listChannels
+		if !chanCanBeHopHint(&channel) {
-		chanInfo, err := lnd.Client.GetChanInfo(ctx, channel.ChannelID)
+			log.Debugf("SelectHopHints: skipping ChannelID: %v, " +
-		if err != nil {
+				"as is not eligible for a private hop hint")
-			return nil, err
+			skipCache[i] = struct{}{}
-		}
-
-		// Cache the GetChanInfo result since it might be useful
-		chanInfoCache[channel.ChannelID] = chanInfo
-
-		// Skip if channel can't forward payment
-		if channel.RemoteBalance < amtMSat {
-			log.Debugf(
-				"Skipping ChannelID: %v for hints as "+
-					"remote balance (%v sats) "+
-					"insufficient appears to be private",
-				channel.ChannelID, channel.RemoteBalance,
-			)
 			continue
 		}
+
 		// If includeNodes is set, we'll only add channels with peers in
 		// includeNodes. This is done to respect the last_hop parameter.
 		if len(includeNodes) > 0 {
 			if _, ok := includeNodes[channel.PubKeyBytes]; !ok {
+				skipCache[i] = struct{}{}
 				continue
 			}
 		}
 
-		// Mark the index to skip so we can skip it on the next
-		// iteration if needed. We'll skip all channels that make
-		// it past this point as they'll likely belong to private
-		// nodes or be selected.
-		skipCache[i] = struct{}{}
-
 		// We want to prevent leaking private nodes, which we define as
 		// nodes with only private channels.
 		//
@@ -102,8 +104,8 @@ func SelectHopHints(ctx context.Context, lnd *lndclient.LndServices,
 		// fail.
 		status, ok := status.FromError(err)
 		if ok && status.Code() == codes.NotFound {
-			log.Warnf("Skipping ChannelID: %v for hints as peer "+
+			log.Warnf("SelectHopHints: skipping ChannelID: %v, "+
-				"(NodeID: %v) is not found: %v",
+				"as peer (NodeID: %v) is not found: %v",
 				channel.ChannelID, channel.PubKeyBytes.String(),
 				err)
 			continue
@@ -113,34 +115,67 @@ func SelectHopHints(ctx context.Context, lnd *lndclient.LndServices,
 
 		if len(nodeInfo.Channels) == 0 {
 			log.Infof(
-				"Skipping ChannelID: %v for hints as peer "+
+				"SelectHopHints: skipping ChannelID: %v as "+
-					"(NodeID: %v) appears to be private",
+					"peer (NodeID: %v) appears to be private",
 				channel.ChannelID, channel.PubKeyBytes.String(),
 			)
+
+			// Skip this channel since the remote node is private.
+			skipCache[i] = struct{}{}
 			continue
 		}
 
-		nodeID, err := btcec.ParsePubKey(
+		// Retrieve extra info for each channel not available in
+		// listChannels.
+		chanInfo, err := lnd.Client.GetChanInfo(ctx, channel.ChannelID)
+		if err != nil {
+			return nil, err
+		}
+
+		// Cache the GetChanInfo result since it might be useful
+		chanInfoCache[channel.ChannelID] = chanInfo
+
+		// Skip if channel can't forward payment
+		if channel.RemoteBalance < amtMSat {
+			log.Debugf(
+				"SelectHopHints: skipping ChannelID: %v, as "+
+					"the remote balance (%v sats) is "+
+					"insufficient", channel.ChannelID,
+				channel.RemoteBalance,
+			)
+			continue
+		}
+
+		// Now, we'll need to determine which is the correct policy.
+		policy := chanRemotePolicy(channel.PubKeyBytes, chanInfo)
+		if policy == nil {
+			continue
+		}
+
+		nodePubKey, err := btcec.ParsePubKey(
 			channel.PubKeyBytes[:], btcec.S256(),
 		)
 		if err != nil {
 			return nil, err
 		}
 
-		// Now that we now this channel use usable, add it as a hop
+		// Now that we know this channel is usable, add it as a hop
-		// hint and the indexes we'll use later.
+		// hint and the indices we'll use later.
 		hopHints = append(hopHints, []zpay32.HopHint{{
-			NodeID:      nodeID,
+			NodeID:      nodePubKey,
 			ChannelID:   channel.ChannelID,
-			FeeBaseMSat: uint32(chanInfo.Node2Policy.FeeBaseMsat),
+			FeeBaseMSat: uint32(policy.FeeBaseMsat),
 			FeeProportionalMillionths: uint32(
-				chanInfo.Node2Policy.FeeRateMilliMsat,
+				policy.FeeRateMilliMsat,
 			),
-			CLTVExpiryDelta: uint16(
+			CLTVExpiryDelta: uint16(policy.TimeLockDelta),
-				chanInfo.Node2Policy.TimeLockDelta),
 		}})
 
 		totalHintBandwidth += channel.RemoteBalance
+
+		// Mark the index to skip so we can skip it on the next
+		// iteration.
+		skipCache[i] = struct{}{}
 	}
 
 	// If we have enough hop hints at this point, then we'll exit early.
@@ -165,7 +200,9 @@ func SelectHopHints(ctx context.Context, lnd *lndclient.LndServices,
 			break
 		}
 
-		// Skip the channel if we already selected it.
+		// Channels of private nodes, inactive, or public channels or
+		// those that have already been selected can be skipped in this
+		// iteration.
 		if _, ok := skipCache[i]; ok {
 			continue
 		}
@@ -173,6 +210,12 @@ func SelectHopHints(ctx context.Context, lnd *lndclient.LndServices,
 		channel := openChannels[i]
 		chanInfo := chanInfoCache[channel.ChannelID]
 
+		// Now, we'll need to determine which is the correct policy.
+		policy := chanRemotePolicy(channel.PubKeyBytes, chanInfo)
+		if policy == nil {
+			continue
+		}
+
 		nodeID, err := btcec.ParsePubKey(
 			channel.PubKeyBytes[:], btcec.S256())
 		if err != nil {
@@ -184,12 +227,11 @@ func SelectHopHints(ctx context.Context, lnd *lndclient.LndServices,
 		hopHints = append(hopHints, []zpay32.HopHint{{
 			NodeID:      nodeID,
 			ChannelID:   channel.ChannelID,
-			FeeBaseMSat: uint32(chanInfo.Node2Policy.FeeBaseMsat),
+			FeeBaseMSat: uint32(policy.FeeBaseMsat),
 			FeeProportionalMillionths: uint32(
-				chanInfo.Node2Policy.FeeRateMilliMsat,
+				policy.FeeRateMilliMsat,
 			),
-			CLTVExpiryDelta: uint16(
+			CLTVExpiryDelta: uint16(policy.TimeLockDelta),
-				chanInfo.Node2Policy.TimeLockDelta),
 		}})
 
 		// As we've just added a new hop hint, we'll accumulate it's

utils_test.go

@@ -17,10 +17,12 @@ import (
 )
 
 var (
-	chanID1 = lnwire.NewShortChanIDFromInt(1)
+	chanID0 = lnwire.NewShortChanIDFromInt(10)
-	chanID2 = lnwire.NewShortChanIDFromInt(2)
+	chanID1 = lnwire.NewShortChanIDFromInt(11)
-	chanID3 = lnwire.NewShortChanIDFromInt(3)
+	chanID2 = lnwire.NewShortChanIDFromInt(12)
-	chanID4 = lnwire.NewShortChanIDFromInt(4)
+	chanID3 = lnwire.NewShortChanIDFromInt(13)
+	chanID4 = lnwire.NewShortChanIDFromInt(14)
+	chanID5 = lnwire.NewShortChanIDFromInt(15)
 
 	// To generate a nodeID we'll have to perform a few steps.
 	//
@@ -79,28 +81,60 @@ var (
 	}
 	peer2, _ = route.NewVertexFromBytes(pubKeyPeer2.SerializeCompressed())
 
-	// Construct channel1 which will be returned my listChannels and
+	capacity = btcutil.Amount(10000)
-	// channelEdge1 which will be returned by getChanInfo
+
-	chan1Capacity = btcutil.Amount(10000)
+	// Channel0 is a public active channel with peer1 with 10k remote
-	channel1      = lndclient.ChannelInfo{
+	// capacity.
+	channel0 = lndclient.ChannelInfo{
 		Active:        true,
+		Private:       false,
+		ChannelID:     chanID0.ToUint64(),
+		PubKeyBytes:   peer1,
+		LocalBalance:  0,
+		RemoteBalance: capacity,
+		Capacity:      capacity,
+	}
+	channelEdge0 = lndclient.ChannelEdge{
+		ChannelID: chanID0.ToUint64(),
+		ChannelPoint: "b121f1d368b8f60648970bc36b37e7b9700d" +
+			"ed098c60b027e42e9c648e297501:0",
+		Capacity: capacity,
+		Node1:    peer1,
+		Node2:    origin,
+		Node1Policy: &lndclient.RoutingPolicy{
+			FeeBaseMsat:      0,
+			FeeRateMilliMsat: 0,
+			TimeLockDelta:    140,
+		},
+		Node2Policy: &lndclient.RoutingPolicy{
+			FeeBaseMsat:      0,
+			FeeRateMilliMsat: 0,
+			TimeLockDelta:    140,
+		},
+	}
+
+	// Channel1 is a private active channel with peer1 with 10k remote
+	// capacity.
+	channel1 = lndclient.ChannelInfo{
+		Active:        true,
+		Private:       true,
 		ChannelID:     chanID1.ToUint64(),
 		PubKeyBytes:   peer1,
-		LocalBalance:  10000,
+		LocalBalance:  0,
-		RemoteBalance: 0,
+		RemoteBalance: capacity,
-		Capacity:      chan1Capacity,
+		Capacity:      capacity,
 	}
 	channelEdge1 = lndclient.ChannelEdge{
 		ChannelID: chanID1.ToUint64(),
 		ChannelPoint: "b121f1d368b8f60648970bc36b37e7b9700d" +
 			"ed098c60b027e42e9c648e297502:0",
-		Capacity: chan1Capacity,
+		Capacity: capacity,
-		Node1:    origin,
+		Node1:    peer1,
-		Node2:    peer1,
+		Node2:    origin,
 		Node1Policy: &lndclient.RoutingPolicy{
-			FeeBaseMsat:      0,
+			FeeBaseMsat:      1,
-			FeeRateMilliMsat: 0,
+			FeeRateMilliMsat: 1,
-			TimeLockDelta:    144,
+			TimeLockDelta:    141,
 		},
 		Node2Policy: &lndclient.RoutingPolicy{
 			FeeBaseMsat:      0,
@@ -109,22 +143,22 @@ var (
 		},
 	}
 
-	// Construct channel2 which will be returned my listChannels and
+	// Channel2 is a private active channel with peer2 with 10k remote
-	// channelEdge2 which will be returned by getChanInfo
+	// capacity.
-	chan2Capacity = btcutil.Amount(10000)
+	channel2 = lndclient.ChannelInfo{
-	channel2      = lndclient.ChannelInfo{
 		Active:        true,
-		ChannelID:     chanID1.ToUint64(),
+		Private:       true,
+		ChannelID:     chanID2.ToUint64(),
 		PubKeyBytes:   peer2,
 		LocalBalance:  0,
-		RemoteBalance: 10000,
+		RemoteBalance: capacity,
-		Capacity:      chan1Capacity,
+		Capacity:      capacity,
 	}
 	channelEdge2 = lndclient.ChannelEdge{
 		ChannelID: chanID2.ToUint64(),
 		ChannelPoint: "b121f1d368b8f60648970bc36b37e7b9700d" +
 			"ed098c60b027e42e9c648e297502:0",
-		Capacity: chan2Capacity,
+		Capacity: capacity,
 		Node1:    origin,
 		Node2:    peer2,
 		Node1Policy: &lndclient.RoutingPolicy{
@@ -133,34 +167,34 @@ var (
 			TimeLockDelta:    144,
 		},
 		Node2Policy: &lndclient.RoutingPolicy{
-			FeeBaseMsat:      0,
+			FeeBaseMsat:      2,
-			FeeRateMilliMsat: 0,
+			FeeRateMilliMsat: 2,
-			TimeLockDelta:    144,
+			TimeLockDelta:    142,
 		},
 	}
 
-	// Construct channel3 which will be returned my listChannels and
+	// Channel3 is a private inactive channel with peer2 with 0 remote
-	// channelEdge3 which will be returned by getChanInfo
+	// capacity.
-	chan3Capacity = btcutil.Amount(10000)
+	channel3 = lndclient.ChannelInfo{
-	channel3      = lndclient.ChannelInfo{
+		Active:        false,
-		Active:        true,
+		Private:       true,
 		ChannelID:     chanID3.ToUint64(),
 		PubKeyBytes:   peer2,
-		LocalBalance:  10000,
+		LocalBalance:  capacity,
 		RemoteBalance: 0,
-		Capacity:      chan1Capacity,
+		Capacity:      capacity,
 	}
 	channelEdge3 = lndclient.ChannelEdge{
 		ChannelID: chanID3.ToUint64(),
 		ChannelPoint: "b121f1d368b8f60648970bc36b37e7b9700d" +
 			"ed098c60b027e42e9c648e297502:0",
-		Capacity: chan3Capacity,
+		Capacity: capacity,
-		Node1:    origin,
+		Node1:    peer2,
-		Node2:    peer2,
+		Node2:    origin,
 		Node1Policy: &lndclient.RoutingPolicy{
-			FeeBaseMsat:      0,
+			FeeBaseMsat:      3,
-			FeeRateMilliMsat: 0,
+			FeeRateMilliMsat: 3,
-			TimeLockDelta:    144,
+			TimeLockDelta:    143,
 		},
 		Node2Policy: &lndclient.RoutingPolicy{
 			FeeBaseMsat:      0,
@@ -169,22 +203,22 @@ var (
 		},
 	}
 
-	// Construct channel4 which will be returned my listChannels and
+	// Channel4 is a private active channel with peer2 with 5k remote
-	// channelEdge4 which will be returned by getChanInfo
+	// capacity.
-	chan4Capacity = btcutil.Amount(10000)
+	channel4 = lndclient.ChannelInfo{
-	channel4      = lndclient.ChannelInfo{
 		Active:        true,
+		Private:       true,
 		ChannelID:     chanID4.ToUint64(),
 		PubKeyBytes:   peer2,
-		LocalBalance:  10000,
+		LocalBalance:  capacity / 2,
-		RemoteBalance: 0,
+		RemoteBalance: capacity / 2,
-		Capacity:      chan4Capacity,
+		Capacity:      capacity,
 	}
 	channelEdge4 = lndclient.ChannelEdge{
 		ChannelID: chanID4.ToUint64(),
 		ChannelPoint: "6fe4408bba52c0a0ee15365e107105de" +
 			"fabfc70c497556af69351c4cfbc167b:0",
-		Capacity: chan1Capacity,
+		Capacity: capacity,
 		Node1:    origin,
 		Node2:    peer2,
 		Node1Policy: &lndclient.RoutingPolicy{
@@ -193,10 +227,40 @@ var (
 			TimeLockDelta:    144,
 		},
 		Node2Policy: &lndclient.RoutingPolicy{
+			FeeBaseMsat:      4,
+			FeeRateMilliMsat: 4,
+			TimeLockDelta:    144,
+		},
+	}
+
+	// Channel5 is a public active channel with peer2 with 5k remote
+	// capacity. It is useful to make peer2 public (give the testcase).
+	channel5 = lndclient.ChannelInfo{
+		Active:        true,
+		Private:       false,
+		ChannelID:     chanID5.ToUint64(),
+		PubKeyBytes:   peer2,
+		LocalBalance:  capacity / 2,
+		RemoteBalance: capacity / 2,
+		Capacity:      capacity,
+	}
+	channelEdge5 = lndclient.ChannelEdge{
+		ChannelID: chanID5.ToUint64(),
+		ChannelPoint: "abcde52c0a0ee15365e107105de" +
+			"fabfc70c497556af69351c4cfbc167b:0",
+		Capacity: capacity,
+		Node1:    origin,
+		Node2:    peer2,
+		Node1Policy: &lndclient.RoutingPolicy{
 			FeeBaseMsat:      0,
 			FeeRateMilliMsat: 0,
 			TimeLockDelta:    144,
 		},
+		Node2Policy: &lndclient.RoutingPolicy{
+			FeeBaseMsat:      5,
+			FeeRateMilliMsat: 5,
+			TimeLockDelta:    145,
+		},
 	}
 )
 
@@ -211,67 +275,120 @@ func TestSelectHopHints(t *testing.T) {
 		includeNodes     map[route.Vertex]struct{}
 		expectedError    error
 	}{
-		// 3 inputs set assumes the host node has 3 channels to chose
+		// Chan2: private, active, remote balance OK
-		// from. Only channel 2 with peer 2 is ideal, however we should
+		// Chan3: private, !active
-		// still include the other 2 after in the order they were
+		// Chan4: private, active, small remote balance (second round).
-		// provided just in case
+		// Chan5: public, active => makes peer2 public.
 		{
-			name: "3 inputs set",
+			name: "2 out of 4 selected",
 			channels: []lndclient.ChannelInfo{
 				channel2,
 				channel3,
 				channel4,
+				channel5,
 			},
 			channelEdges: map[uint64]*lndclient.ChannelEdge{
 				channel2.ChannelID: &channelEdge2,
 				channel3.ChannelID: &channelEdge3,
 				channel4.ChannelID: &channelEdge4,
+				channel5.ChannelID: &channelEdge5,
 			},
 			expectedHopHints: [][]zpay32.HopHint{
 				{{
 					NodeID:                    pubKeyPeer2,
 					ChannelID:                 channel2.ChannelID,
-					FeeBaseMSat:               0,
+					FeeBaseMSat:               2,
-					FeeProportionalMillionths: 0,
+					FeeProportionalMillionths: 2,
-					CLTVExpiryDelta:           144,
+					CLTVExpiryDelta:           142,
-				}},
-				{{
-					NodeID:                    pubKeyPeer2,
-					ChannelID:                 channel3.ChannelID,
-					FeeBaseMSat:               0,
-					FeeProportionalMillionths: 0,
-					CLTVExpiryDelta:           144,
 				}},
 				{{
 					NodeID:                    pubKeyPeer2,
 					ChannelID:                 channel4.ChannelID,
-					FeeBaseMSat:               0,
+					FeeBaseMSat:               4,
-					FeeProportionalMillionths: 0,
+					FeeProportionalMillionths: 4,
 					CLTVExpiryDelta:           144,
 				}},
 			},
-			amtMSat:        chan1Capacity,
+			amtMSat:        capacity,
 			numMaxHophints: 20,
 			includeNodes:   make(map[route.Vertex]struct{}),
 			expectedError:  nil,
 		},
+		// A variation of the above test case but nodes are filtered so
+		// we only add channel 1 (as channel 0 is public making peer1
+		// public).
 		{
-			name: "invalid set",
+			name: "1 out of 6 selected",
 			channels: []lndclient.ChannelInfo{
+				channel0,
 				channel1,
+				channel2,
+				channel3,
+				channel4,
+				channel5,
 			},
 			channelEdges: map[uint64]*lndclient.ChannelEdge{
+				channel0.ChannelID: &channelEdge0,
 				channel1.ChannelID: &channelEdge1,
+				channel2.ChannelID: &channelEdge2,
+				channel3.ChannelID: &channelEdge3,
+				channel4.ChannelID: &channelEdge4,
+				channel5.ChannelID: &channelEdge5,
 			},
 			expectedHopHints: [][]zpay32.HopHint{
 				{{
 					NodeID:                    pubKeyPeer1,
 					ChannelID:                 channel1.ChannelID,
-					FeeBaseMSat:               0,
+					FeeBaseMSat:               1,
-					FeeProportionalMillionths: 0,
+					FeeProportionalMillionths: 1,
+					CLTVExpiryDelta:           141,
+				}},
+			},
+			amtMSat:        capacity,
+			numMaxHophints: 20,
+			includeNodes: map[route.Vertex]struct{}{
+				peer1: {},
+			},
+			expectedError: nil,
+		},
+
+		// Chan1: private, active, remote balance OK => node not public
+		{
+			name: "1 private chan",
+			channels: []lndclient.ChannelInfo{
+				channel1,
+			},
+			channelEdges: map[uint64]*lndclient.ChannelEdge{
+				channel1.ChannelID: &channelEdge1,
+			},
+			expectedHopHints: [][]zpay32.HopHint{},
+			amtMSat:          capacity,
+			numMaxHophints:   20,
+			includeNodes:     make(map[route.Vertex]struct{}),
+			expectedError:    nil,
+		},
+		// Chan4: private, active, small remote balance (second round).
+		// Chan5: public, active => makes peer2 public.
+		{
+			name: "1 out of 2 selected",
+			channels: []lndclient.ChannelInfo{
+				channel4,
+				channel5,
+			},
+			channelEdges: map[uint64]*lndclient.ChannelEdge{
+				channel4.ChannelID: &channelEdge4,
+				channel5.ChannelID: &channelEdge5,
+			},
+			expectedHopHints: [][]zpay32.HopHint{
+				{{
+					NodeID:                    pubKeyPeer2,
+					ChannelID:                 channel4.ChannelID,
+					FeeBaseMSat:               4,
+					FeeProportionalMillionths: 4,
 					CLTVExpiryDelta:           144,
 				}},
-			}, amtMSat: chan1Capacity,
+			},
+			amtMSat:        capacity,
 			numMaxHophints: 20,
 			includeNodes:   make(map[route.Vertex]struct{}),
 			expectedError:  nil,