From 1e1d7b387cda1f0b25dba66f517b53f2a98c6d3d Mon Sep 17 00:00:00 2001
From: Christian Decker <decker.christian@gmail.com>
Date: Thu, 25 Feb 2021 16:25:55 +0100
Subject: [PATCH] pay: Filter out routehints whose entrypoint is unknown

We would happily spin on attempts that are doomed to fail because we
don't know the entrypoint. Next up: remove routehints whose
entrypoints are known but unreachable.
---
 plugins/libplugin-pay.c | 35 +++++++++++++++++++++++++-----
 tests/test_pay.py       | 48 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 78 insertions(+), 5 deletions(-)

diff --git a/plugins/libplugin-pay.c b/plugins/libplugin-pay.c
index c77d75d83..1e063122c 100644
--- a/plugins/libplugin-pay.c
+++ b/plugins/libplugin-pay.c
@@ -2313,14 +2313,17 @@ static void trim_route(struct route_info **route, size_t n)
 
 /* Make sure routehints are reasonable length, and (since we assume we
  * can append), not directly to us.  Note: untrusted data! */
-static struct route_info **filter_routehints(struct routehints_data *d,
+static struct route_info **filter_routehints(struct gossmap *map,
+					     struct payment *p,
+					     struct routehints_data *d,
 					     struct node_id *myid,
 					     struct route_info **hints)
 {
+	const size_t max_hops = ROUTING_MAX_HOPS / 2;
 	char *mods = tal_strdup(tmpctx, "");
 	for (size_t i = 0; i < tal_count(hints); i++) {
+		struct gossmap_node *entrynode;
 		/* Trim any routehint > 10 hops */
-		size_t max_hops = ROUTING_MAX_HOPS / 2;
 		if (tal_count(hints[i]) > max_hops) {
 			tal_append_fmt(&mods,
 				       "Trimmed routehint %zu (%zu hops) to %zu. ",
@@ -2344,6 +2347,27 @@ static struct route_info **filter_routehints(struct routehints_data *d,
 			tal_arr_remove(&hints, i);
 			i--;
 		}
+
+		/* If routehint entrypoint is unreachable there's no
+		 * point in keeping it. */
+		entrynode = gossmap_find_node(map, &hints[i][0].pubkey);
+		if (entrynode == NULL) {
+			tal_append_fmt(&mods,
+				       "Removed routehint %zu because "
+				       "entrypoint %s is unknown. ",
+				       i,
+				       type_to_string(tmpctx, struct node_id,
+						      &hints[i][0].pubkey));
+			plugin_log(p->plugin, LOG_DBG,
+				   "Removed routehint %zu because "
+				   "entrypoint %s is unknown. ",
+				   i,
+				   type_to_string(tmpctx, struct node_id,
+						  &hints[i][0].pubkey));
+			tal_arr_remove(&hints, i);
+			i--;
+			continue;
+		}
 	}
 
 	if (!streq(mods, ""))
@@ -2616,7 +2640,7 @@ static void routehint_step_cb(struct routehints_data *d, struct payment *p)
 {
 	struct route_hop hop;
 	const struct payment *root = payment_root(p);
-
+	struct gossmap *map;
 	if (p->step == PAYMENT_STEP_INITIALIZED) {
 		if (root->routes == NULL)
 			return payment_continue(p);
@@ -2625,8 +2649,9 @@ static void routehint_step_cb(struct routehints_data *d, struct payment *p)
 		 * beginning, and every other payment will filter out the
 		 * exluded ones on the fly. */
 		if (p->parent == NULL) {
-			d->routehints = filter_routehints(d, p->local_id,
-							  p->routes);
+			map = get_gossmap(p->plugin);
+			d->routehints = filter_routehints(
+			    map, p, d, p->local_id, p->routes);
 			/* filter_routehints modifies the array, but
 			 * this could trigger a resize and the resize
 			 * could trigger a realloc.
diff --git a/tests/test_pay.py b/tests/test_pay.py
index d21b7e80d..9535fbb9a 100644
--- a/tests/test_pay.py
+++ b/tests/test_pay.py
@@ -4172,3 +4172,51 @@ def test_self_pay(node_factory):
 
     with pytest.raises(RpcError):
         l1.rpc.pay(inv)
+
+
+@unittest.skipIf(TEST_NETWORK != 'regtest', "Canned invoice is network specific")
+def test_unreachable_routehint(node_factory, bitcoind):
+    """Test that we discard routehints that we can't reach.
+
+    Reachability is tested by checking whether we can reach the
+    entrypoint of the routehint, i.e., the first node in the
+    routehint. The network we create is partitioned on purpose for
+    this: first we attempt with an unknown destination and an unknown
+    routehint entrypoint, later we make them known, but still
+    unreachable, by connecting them without a channel.
+
+    """
+
+    # Create a partitioned network, first without connecting it, then
+    # connecting it without a channel so they can sync gossip. Notice
+    # that l4 is there only to trick the deadend heuristic.
+    l1, l2 = node_factory.line_graph(2, wait_for_announce=True)
+    l3, l4, l5 = node_factory.line_graph(3, wait_for_announce=True)
+    entrypoint = '0382ce59ebf18be7d84677c2e35f23294b9992ceca95491fcf8a56c6cb2d9de199'
+
+    # Generate an invoice with exactly one routehint.
+    for i in range(100):
+        invoice = l5.rpc.invoice(10, 'attempt{}'.format(i), 'description')['bolt11']
+        decoded = l1.rpc.decodepay(invoice)
+        if 'routes' in decoded and len(decoded['routes']) == 1:
+            break
+
+    assert('routes' in decoded and len(decoded['routes']) == 1)
+
+    with pytest.raises(RpcError, match=r'Destination [a-f0-9]{66} is not reachable'):
+        l1.rpc.pay(invoice)
+
+    assert(l1.daemon.is_in_log(
+        r"Removed routehint 0 because entrypoint {entrypoint} is unknown.".format(
+            entrypoint=entrypoint
+        )
+    ))
+
+    # Now connect l2 to l3 to create a bridge, but without a
+    # channel. The entrypoint will become known, but still
+    # unreachable, resulting in a slightly different error message,
+    # but the routehint will still be removed.
+    l2.connect(l3)
+    wait_for(lambda: len(l1.rpc.listnodes(entrypoint)['nodes']) == 1)
+
+    # TODO(cdecker) investigate why dijkstra_distance tells us the entrypoint is reachable...