From 26cdf9d3dca6df65286c8c1440d18e5e9126052d Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Wed, 12 Jun 2019 10:08:54 +0930 Subject: [PATCH] plugins/pay: don't retry routehint if it contains already-eliminated channel. Signed-off-by: Rusty Russell --- plugins/pay.c | 36 ++++++++++++++++++++++++++++-------- tests/test_misc.py | 2 +- tests/test_pay.py | 15 +++++---------- 3 files changed, 34 insertions(+), 19 deletions(-) diff --git a/plugins/pay.c b/plugins/pay.c index cea4e2052..1591560ec 100644 --- a/plugins/pay.c +++ b/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"); diff --git a/tests/test_misc.py b/tests/test_misc.py index 8a00be637..6c75a80bf 100644 --- a/tests/test_misc.py +++ b/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 diff --git a/tests/test_pay.py b/tests/test_pay.py index 2e599c757..47b01bf9d 100644 --- a/tests/test_pay.py +++ b/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)