plugins/pay: don't retry routehint if it contains already-eliminated channel.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

plugins/pay.c

@@ -148,13 +148,13 @@ static struct command_result *start_pay_attempt(struct command *cmd,
 
 /* Is this (erring) channel within the routehint itself? */
 static bool channel_in_routehint(const struct route_info *routehint,
-				 const char *buf, const jsmntok_t *scidtok)
+				 const char *scidstr, size_t scidlen)
 {
 	struct short_channel_id scid;
 
-	if (!json_to_short_channel_id(buf, scidtok, &scid, false))
+	if (!short_channel_id_from_str(scidstr, scidlen, &scid, false))
 		plugin_err("bad erring_channel '%.*s'",
-			   scidtok->end - scidtok->start, buf + scidtok->start);
+			   (int)scidlen, scidstr);
 
 	for (size_t i = 0; i < tal_count(routehint); i++)
 		if (short_channel_id_eq(&scid, &routehint[i].short_channel_id))
@@ -199,15 +199,32 @@ static struct command_result *waitsendpay_expired(struct command *cmd,
 	return command_done_err(cmd, PAY_STOPPED_RETRYING, errmsg, data);
 }
 
+static bool routehint_excluded(const struct route_info *routehint,
+			       const char **excludes)
+{
+	/* Note that we ignore direction here: in theory, we could have
+	 * found that one direction of a channel is unavailable, but they
+	 * are suggesting we use it the other way.  Very unlikely though! */
+	for (size_t i = 0; i < tal_count(excludes); i++)
+		if (channel_in_routehint(routehint,
+					 excludes[i], strlen(excludes[i])))
+			return true;
+	return false;
+}
+
 static struct command_result *next_routehint(struct command *cmd,
 					     struct pay_command *pc)
 {
 	size_t num_attempts = count_sendpays(pc->ps->attempts);
 
-	if (tal_count(pc->routehints) > 0) {
-		pc->current_routehint = pc->routehints[0];
+	while (tal_count(pc->routehints) > 0) {
+		if (!routehint_excluded(pc->routehints[0], pc->excludes)) {
+			pc->current_routehint = pc->routehints[0];
+			tal_arr_remove(&pc->routehints, 0);
+			return start_pay_attempt(cmd, pc, "Trying route hint");
+		}
+		tal_free(pc->routehints[0]);
 		tal_arr_remove(&pc->routehints, 0);
-		return start_pay_attempt(cmd, pc, "Trying route hint");
 	}
 
 	/* No (more) routehints; we're out of routes. */
@@ -262,7 +279,9 @@ static struct command_result *waitsendpay_error(struct command *cmd,
 	}
 
 	/* If failure is in routehint part, try next one */
-	if (channel_in_routehint(pc->current_routehint, buf, scidtok))
+	if (channel_in_routehint(pc->current_routehint,
+				 buf + scidtok->start,
+				 scidtok->end - scidtok->start))
 		return next_routehint(cmd, pc);
 
 	/* Otherwise, add erring channel to exclusion list. */
@@ -416,7 +435,8 @@ static bool maybe_exclude(struct pay_command *pc,
 
 	scid = json_get_member(buf, route, "channel");
 
-	if (channel_in_routehint(pc->current_routehint, buf, scid))
+	if (channel_in_routehint(pc->current_routehint,
+				 buf + scid->start, scid->end - scid->start))
 		return false;
 
 	dir = json_get_member(buf, route, "direction");

tests/test_misc.py

@@ -910,7 +910,7 @@ def test_htlc_send_timeout(node_factory, bitcoind):
             timedout = True
 
     inv = l3.rpc.invoice(123000, 'test_htlc_send_timeout', 'description')
-    with pytest.raises(RpcError, match=r'Ran out of routes to try after 2 attempts') as excinfo:
+    with pytest.raises(RpcError, match=r'Ran out of routes to try after 1 attempt: see paystatus') as excinfo:
         l1.rpc.pay(inv['bolt11'])
 
     err = excinfo.value

tests/test_pay.py

@@ -96,13 +96,11 @@ def test_pay_limits(node_factory):
     # It should have retried (once without routehint, too)
     status = l1.rpc.call('paystatus', {'bolt11': inv['bolt11']})['pay'][0]['attempts']
 
-    # Hits weird corner case: it excludes channel, then uses routehint
-    # which reintroduces it, so then it excludes other channel.
-    assert len(status) == 4
+    # Excludes channel, then ignores routehint which includes that, then
+    # it excludes other channel.
+    assert len(status) == 2
     assert status[0]['strategy'] == "Initial attempt"
     assert status[1]['strategy'].startswith("Excluded expensive channel ")
-    assert status[2]['strategy'] == "Trying route hint"
-    assert status[3]['strategy'].startswith("Excluded expensive channel ")
 
     # Delay too high.
     with pytest.raises(RpcError, match=r'Route wanted delay of .* blocks') as err:
@@ -111,11 +109,9 @@ def test_pay_limits(node_factory):
     assert err.value.error['code'] == PAY_ROUTE_TOO_EXPENSIVE
     # Should also have retried.
     status = l1.rpc.call('paystatus', {'bolt11': inv['bolt11']})['pay'][1]['attempts']
-    assert len(status) == 4
+    assert len(status) == 2
     assert status[0]['strategy'] == "Initial attempt"
     assert status[1]['strategy'].startswith("Excluded delaying channel ")
-    assert status[2]['strategy'] == "Trying route hint"
-    assert status[3]['strategy'].startswith("Excluded delaying channel ")
 
     # This works, because fee is less than exemptfee.
     l1.rpc.call('pay', {'bolt11': inv['bolt11'], 'msatoshi': 100000, 'maxfeepercent': 0.0001, 'exemptfee': 2000})
@@ -1493,9 +1489,8 @@ def test_pay_retry(node_factory, bitcoind):
     # It will try l1->l2->l5, which fails.
     # It will try l1->l2->l3->l5, which fails.
     # It will try l1->l2->l3->l4->l5, which fails.
-    # It then tries l1->l2->l3->l4->l5, because of routeboost, which fails.
     inv = l5.rpc.invoice(10**8, 'test_retry2', 'test_retry2')['bolt11']
-    with pytest.raises(RpcError, match=r'4 attempts'):
+    with pytest.raises(RpcError, match=r'3 attempts'):
         l1.rpc.pay(inv)