From e8d571ba370c35d843f9ff6b362c85de21585433 Mon Sep 17 00:00:00 2001 From: ZmnSCPxj Date: Fri, 1 Feb 2019 01:01:56 +1030 Subject: [PATCH 1/8] test_pay.py: Add test that we prefer direct route. --- tests/test_pay.py | 38 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 38 insertions(+) diff --git a/tests/test_pay.py b/tests/test_pay.py index 3c62627856af..42651b95e252 100644 --- a/tests/test_pay.py +++ b/tests/test_pay.py @@ -1357,3 +1357,41 @@ def test_pay_routeboost(node_factory, bitcoind): assert [h['channel'] for h in attempts[0]['routehint']] == [r['short_channel_id'] for r in routel3l4l5] assert [h['channel'] for h in attempts[1]['routehint']] == [r['short_channel_id'] for r in routel3l5] + + +@pytest.mark.xfail(strict=True) +def test_pay_direct(node_factory, bitcoind): + """Check that we prefer the direct route. + """ + l1, l2, l3 = node_factory.get_nodes(3) + + # Direct channel + l1.rpc.connect(l3.info['id'], 'localhost', l3.port) + l1.fund_channel(l3, 10**7) + # Indirect route + l1.rpc.connect(l2.info['id'], 'localhost', l2.port) + l1.fund_channel(l2, 10**7) + l2.rpc.connect(l3.info['id'], 'localhost', l3.port) + c3 = l2.fund_channel(l3, 10**7) + + # Let channels lock in. + bitcoind.generate_block(5) + + # Make sure l1 knows the l2->l3 channel. + l1.wait_channel_active(c3) + + # Find out how much msatoshi l1 owns on l1->l2 channel. + l1l2msatreference = only_one(l1.rpc.getpeer(l2.info['id'])['channels'])['msatoshi_to_us'] + + # Try multiple times to ensure that route randomization + # will not override our preference for direct route. + for i in range(8): + inv = l3.rpc.invoice(10000, 'pay{}'.format(i), 'desc')['bolt11'] + + l1.rpc.pay(inv) + + # We should have gone the direct route, so + # l1->l2 channel msatoshi_to_us should not + # have changed. + l1l2msat = only_one(l1.rpc.getpeer(l2.info['id'])['channels'])['msatoshi_to_us'] + assert l1l2msat == l1l2msatreference From b49fa5fd5090a9871087db247fffa5ab41adf5c8 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Fri, 1 Feb 2019 13:06:18 +1030 Subject: [PATCH 2/8] gossipd: increase randomness in route selection. We have a seed, which is for (future!) unit testing consistency. This makes it change every time, so our pay_direct_test is more useful. I tried restarting the noed around the loop, but it tended to fail rebinding to the same port for some reason? Signed-off-by: Rusty Russell --- common/pseudorand.c | 7 +++++++ common/pseudorand.h | 5 +++++ gossipd/gossipd.c | 2 +- gossipd/routing.c | 7 +++++-- gossipd/routing.h | 2 +- 5 files changed, 19 insertions(+), 4 deletions(-) diff --git a/common/pseudorand.c b/common/pseudorand.c index dc8f6b17b567..817a9e5bfa9a 100644 --- a/common/pseudorand.c +++ b/common/pseudorand.c @@ -37,6 +37,13 @@ uint64_t pseudorand(uint64_t max) return isaac64_next_uint(&isaac64, max); } +uint64_t pseudorand_u64(void) +{ + init_if_needed(); + + return isaac64_next_uint64(&isaac64); +} + const struct siphash_seed *siphash_seed(void) { init_if_needed(); diff --git a/common/pseudorand.h b/common/pseudorand.h index d66435ed44d5..0a46223bffb2 100644 --- a/common/pseudorand.h +++ b/common/pseudorand.h @@ -8,6 +8,11 @@ */ uint64_t pseudorand(uint64_t max); +/** + * pseudorand - pseudo (guessable!) random number between 0 and UINT64_MAX. + */ +uint64_t pseudorand_u64(void); + /** * Get the siphash seed for hash tables. */ diff --git a/gossipd/gossipd.c b/gossipd/gossipd.c index 9289f6a579bf..73502022885b 100644 --- a/gossipd/gossipd.c +++ b/gossipd/gossipd.c @@ -1922,7 +1922,7 @@ static struct io_plan *getroute_req(struct io_conn *conn, struct daemon *daemon, /* routing.c does all the hard work; can return NULL. */ hops = get_route(tmpctx, daemon->rstate, &source, &destination, msatoshi, riskfactor, final_cltv, - fuzz, siphash_seed(), excluded, max_hops); + fuzz, pseudorand_u64(), excluded, max_hops); out = towire_gossip_getroute_reply(NULL, hops); daemon_conn_send(daemon->master, take(out)); diff --git a/gossipd/routing.c b/gossipd/routing.c index c9ce2ad99f96..1a23dd8c8131 100644 --- a/gossipd/routing.c +++ b/gossipd/routing.c @@ -1496,7 +1496,7 @@ struct route_hop *get_route(const tal_t *ctx, struct routing_state *rstate, const struct pubkey *destination, const u64 msatoshi, double riskfactor, u32 final_cltv, - double fuzz, const struct siphash_seed *base_seed, + double fuzz, u64 seed, const struct short_channel_id_dir *excluded, size_t max_hops) { @@ -1507,9 +1507,12 @@ struct route_hop *get_route(const tal_t *ctx, struct routing_state *rstate, struct route_hop *hops; struct node *n; u64 *saved_capacity; + struct siphash_seed base_seed; saved_capacity = tal_arr(tmpctx, u64, tal_count(excluded)); + base_seed.u.u64[0] = base_seed.u.u64[1] = seed; + /* Temporarily set excluded channels' capacity to zero. */ for (size_t i = 0; i < tal_count(excluded); i++) { struct chan *chan = get_channel(rstate, &excluded[i].scid); @@ -1522,7 +1525,7 @@ struct route_hop *get_route(const tal_t *ctx, struct routing_state *rstate, route = find_route(ctx, rstate, source, destination, msatoshi, riskfactor / BLOCKS_PER_YEAR / 10000, - fuzz, base_seed, max_hops, &fee); + fuzz, &base_seed, max_hops, &fee); /* Now restore the capacity. */ for (size_t i = 0; i < tal_count(excluded); i++) { diff --git a/gossipd/routing.h b/gossipd/routing.h index 80c762243607..038a9ea5a0de 100644 --- a/gossipd/routing.h +++ b/gossipd/routing.h @@ -264,7 +264,7 @@ struct route_hop *get_route(const tal_t *ctx, struct routing_state *rstate, const u64 msatoshi, double riskfactor, u32 final_cltv, double fuzz, - const struct siphash_seed *base_seed, + u64 seed, const struct short_channel_id_dir *excluded, size_t max_hops); /* Disable channel(s) based on the given routing failure. */ From f10dc44f3026eb1524b2154d06de80929c9f5bae Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Fri, 1 Feb 2019 13:33:27 +1030 Subject: [PATCH 3/8] plugins/pay: try without routehints first. This is the direct cause of the failure of the original test_pay_direct test and it makes sense: invoice routehints may not be necessary, so try without them *first* rather than last. We didn't mention the use of routehints in CHANGELOG at all yet, so do that now. Signed-off-by: Rusty Russell --- CHANGELOG.md | 1 + plugins/pay.c | 83 ++++++++++++++++++++++++++--------------------- tests/test_pay.py | 78 +++++++++++++++++--------------------------- 3 files changed, 77 insertions(+), 85 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d51ca91bd68d..ecbe90f78b2e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0. - plugins: fully enabled, and ready for you to write some! - plugins: `pay` is now a plugin. +- protocol: `pay` will now use routehints in invoices if it needs to. - lightning-cli: `help ` finds man pages even if `make install` not run. - JSON API: `waitsendpay` now has an `erring_direction` field. - JSON API: `listpeers` now has a `direction` field in `channels`. diff --git a/plugins/pay.c b/plugins/pay.c index 19905d9216cf..5910e2f99078 100644 --- a/plugins/pay.c +++ b/plugins/pay.c @@ -92,7 +92,10 @@ struct pay_command { /* Channels which have failed us. */ const char **excludes; - /* Any routehints to use. */ + /* Current routehint, if any. */ + struct route_info *current_routehint; + + /* Any remaining routehints to try. */ struct route_info **routehints; /* Current node during shadow route calculation. */ @@ -179,12 +182,23 @@ static struct command_result *waitsendpay_expired(struct command *cmd, return command_done_err(cmd, PAY_STOPPED_RETRYING, errmsg, data); } -/* Try again with the next routehint (or none if that was the last) */ static struct command_result *next_routehint(struct command *cmd, struct pay_command *pc) { - tal_arr_remove(&pc->routehints, 0); - return start_pay_attempt(cmd, pc, "Removed route hint"); + if (tal_count(pc->routehints) > 0) { + pc->current_routehint = 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. */ + /* If we eliminated one because it was too pricy, return that. */ + if (pc->expensive_route) + return command_fail(cmd, PAY_ROUTE_TOO_EXPENSIVE, + "%s", pc->expensive_route); + + return command_fail(cmd, PAY_ROUTE_NOT_FOUND, + "Could not find a route"); } static struct command_result *waitsendpay_error(struct command *cmd, @@ -222,9 +236,8 @@ static struct command_result *waitsendpay_error(struct command *cmd, return waitsendpay_expired(cmd, pc); } - /* If failure is in routehint part, eliminate that */ - if (tal_count(pc->routehints) != 0 - && channel_in_routehint(pc->routehints[0], buf, scidtok)) + /* If failure is in routehint part, try next one */ + if (channel_in_routehint(pc->current_routehint, buf, scidtok)) return next_routehint(cmd, pc); /* Otherwise, add erring channel to exclusion list. */ @@ -370,8 +383,7 @@ static bool maybe_exclude(struct pay_command *pc, scid = json_get_member(buf, route, "channel"); - if (tal_count(pc->routehints) != 0 - && channel_in_routehint(pc->routehints[0], buf, scid)) + if (channel_in_routehint(pc->current_routehint, buf, scid)) return false; dir = json_get_member(buf, route, "direction"); @@ -399,9 +411,9 @@ static struct command_result *getroute_done(struct command *cmd, plugin_err("getroute gave no 'route'? '%.*s'", result->end - result->start, buf); - if (tal_count(pc->routehints)) + if (pc->current_routehint) attempt->route = join_routehint(pc->ps->attempts, buf, t, - pc, pc->routehints[0]); + pc, pc->current_routehint); else attempt->route = json_strdup(pc->ps->attempts, buf, t); @@ -441,11 +453,7 @@ static struct command_result *getroute_done(struct command *cmd, pc->excludes[tal_count(pc->excludes)-1]); } - if (tal_count(pc->routehints) != 0) - return next_routehint(cmd, pc); - - return command_fail(cmd, PAY_ROUTE_TOO_EXPENSIVE, - "%s", pc->expensive_route); + return next_routehint(cmd, pc); } if (delay > pc->maxdelay) { @@ -472,11 +480,7 @@ static struct command_result *getroute_done(struct command *cmd, pc->excludes[tal_count(pc->excludes)-1]); } - if (tal_count(pc->routehints) != 0) - return next_routehint(cmd, pc); - - return command_fail(cmd, PAY_ROUTE_TOO_EXPENSIVE, - "%s", pc->expensive_route); + return next_routehint(cmd, pc); } if (pc->desc) @@ -497,18 +501,21 @@ static struct command_result *getroute_error(struct command *cmd, const jsmntok_t *error, struct pay_command *pc) { + int code; + const jsmntok_t *codetok; + attempt_failed_tok(pc, "getroute", buf, error); - /* If we were trying to use a routehint, remove and try again. */ - if (tal_count(pc->routehints) != 0) - return next_routehint(cmd, pc); + codetok = json_get_member(buf, error, "code"); + if (!json_to_int(buf, codetok, &code)) + plugin_err("getroute error gave no 'code'? '%.*s'", + error->end - error->start, buf + error->start); - /* If we've run out of routes, there might be a good reason. */ - if (pc->expensive_route) - return command_fail(cmd, PAY_ROUTE_TOO_EXPENSIVE, - "%s", pc->expensive_route); + /* Strange errors from getroute should be forwarded. */ + if (code != PAY_ROUTE_NOT_FOUND) + return forward_error(cmd, buf, error, pc); - return forward_error(cmd, buf, error, pc); + return next_routehint(cmd, pc); } /* Deep copy of excludes array. */ @@ -559,17 +566,17 @@ static struct command_result *start_pay_attempt(struct command *cmd, /* If we have a routehint, try that first; we need to do extra * checks that it meets our criteria though. */ - if (tal_count(pc->routehints)) { + if (pc->current_routehint) { amount = route_msatoshi(pc->msatoshi, - pc->routehints[0], - tal_count(pc->routehints[0])); + pc->current_routehint, + tal_count(pc->current_routehint)); dest = type_to_string(tmpctx, struct pubkey, - &pc->routehints[0][0].pubkey); - max_hops -= tal_count(pc->routehints[0]); + &pc->current_routehint[0].pubkey); + max_hops -= tal_count(pc->current_routehint); cltv = route_cltv(pc->final_cltv, - pc->routehints[0], - tal_count(pc->routehints[0])); - attempt.routehint = tal_steal(pc->ps, pc->routehints[0]); + pc->current_routehint, + tal_count(pc->current_routehint)); + attempt.routehint = tal_steal(pc->ps, pc->current_routehint); } else { amount = pc->msatoshi; dest = pc->dest; @@ -871,6 +878,8 @@ static struct command_result *handle_pay(struct command *cmd, pc->stoptime = timeabs_add(time_now(), time_from_sec(*retryfor)); pc->excludes = tal_arr(cmd, const char *, 0); pc->ps = add_pay_status(pc, b11str); + /* We try first without using routehint */ + pc->current_routehint = NULL; pc->routehints = filter_routehints(pc, b11->routes); pc->expensive_route = NULL; diff --git a/tests/test_pay.py b/tests/test_pay.py index 42651b95e252..fa0f7a2fe1a6 100644 --- a/tests/test_pay.py +++ b/tests/test_pay.py @@ -76,10 +76,14 @@ 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'] - assert len(status) == 3 + + # Hits weird corner case: it excludes channel, then uses routehint + # which reintroduces it, so then it excludes other channel. + assert len(status) == 4 assert status[0]['strategy'] == "Initial attempt" assert status[1]['strategy'].startswith("Excluded expensive channel ") - assert status[2]['strategy'] == "Removed route hint" + 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: @@ -88,10 +92,11 @@ 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) == 3 + assert len(status) == 4 assert status[0]['strategy'] == "Initial attempt" assert status[1]['strategy'].startswith("Excluded delaying channel ") - assert status[2]['strategy'] == "Removed route hint" + 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}) @@ -1259,13 +1264,21 @@ def test_pay_routeboost(node_factory, bitcoind): assert 'description' not in only_one(status['pay']) assert 'routehint_modifications' not in only_one(status['pay']) assert 'local_exclusions' not in only_one(status['pay']) - attempt = only_one(only_one(status['pay'])['attempts']) - assert attempt['age_in_seconds'] <= time.time() - start - assert attempt['duration_in_seconds'] <= end - start - assert only_one(attempt['routehint']) - assert only_one(attempt['routehint'])['id'] == l3.info['id'] - assert only_one(attempt['routehint'])['msatoshi'] == 10**5 + 1 + 10**5 // 100000 - assert only_one(attempt['routehint'])['delay'] == 5 + 6 + # First attempt will fail, then it will try route hint + attempts = only_one(status['pay'])['attempts'] + assert len(attempts) == 2 + assert attempts[0]['strategy'] == "Initial attempt" + # FIXME! + PAY_ROUTE_NOT_FOUND = 205 + assert attempts[0]['failure']['code'] == PAY_ROUTE_NOT_FOUND + assert attempts[1]['strategy'] == "Trying route hint" + assert 'success' in attempts[1] + assert attempts[1]['age_in_seconds'] <= time.time() - start + assert attempts[1]['duration_in_seconds'] <= end - start + assert only_one(attempts[1]['routehint']) + assert only_one(attempts[1]['routehint'])['id'] == l3.info['id'] + assert only_one(attempts[1]['routehint'])['msatoshi'] == 10**5 + 1 + 10**5 // 100000 + assert only_one(attempts[1]['routehint'])['delay'] == 5 + 6 # With dev-route option we can test longer routehints. if DEVELOPER: @@ -1287,43 +1300,11 @@ def test_pay_routeboost(node_factory, bitcoind): 'dev-routes': [routel3l4l5]}) l1.rpc.pay(inv['bolt11']) status = l1.rpc.call('paystatus', [inv['bolt11']]) - assert len(only_one(status['pay'])['attempts']) == 1 - assert 'failure' not in only_one(status['pay'])['attempts'][0] - assert 'success' in only_one(status['pay'])['attempts'][0] - - # Now test that it falls back correctly to not using routeboost - # if it can't route to the node mentioned - routel4l3 = [{'id': l4.info['id'], - 'short_channel_id': scid34, - 'fee_base_msat': 1000, - 'fee_proportional_millionths': 10, - 'cltv_expiry_delta': 6}] - inv = l3.rpc.call('invoice', {'msatoshi': 10**5, - 'label': 'test_pay_routeboost3', - 'description': 'test_pay_routeboost3', - 'dev-routes': [routel4l3]}) - l1.rpc.pay(inv['bolt11']) - status = l1.rpc.call('paystatus', [inv['bolt11']]) assert len(only_one(status['pay'])['attempts']) == 2 assert 'failure' in only_one(status['pay'])['attempts'][0] assert 'success' not in only_one(status['pay'])['attempts'][0] - routehint = only_one(status['pay'])['attempts'][0]['routehint'] - assert [h['channel'] for h in routehint] == [r['short_channel_id'] for r in routel4l3] assert 'failure' not in only_one(status['pay'])['attempts'][1] assert 'success' in only_one(status['pay'])['attempts'][1] - assert 'routehint' not in only_one(status['pay'])['attempts'][1] - - # Similarly if it can route, but payment fails. - routel2bad = [{'id': l2.info['id'], - 'short_channel_id': scid34, # Invalid scid - 'fee_base_msat': 1000, - 'fee_proportional_millionths': 10, - 'cltv_expiry_delta': 6}] - inv = l3.rpc.call('invoice', {'msatoshi': 10**5, - 'label': 'test_pay_routeboost4', - 'description': 'test_pay_routeboost4', - 'dev-routes': [routel2bad]}) - l1.rpc.pay(inv['bolt11']) # Finally, it should fall back to second routehint if first fails. # (Note, this is not public because it's not 6 deep) @@ -1350,13 +1331,14 @@ def test_pay_routeboost(node_factory, bitcoind): assert 'local_exclusions' not in only_one(status['pay']) attempts = only_one(status['pay'])['attempts'] - # First failed, second succeeded. - assert len(attempts) == 2 + # First two failed (w/o routehint and w bad hint), third succeeded. + assert len(attempts) == 3 assert 'success' not in attempts[0] - assert 'success' in attempts[1] + assert 'success' not in attempts[1] + assert 'success' in attempts[2] - assert [h['channel'] for h in attempts[0]['routehint']] == [r['short_channel_id'] for r in routel3l4l5] - assert [h['channel'] for h in attempts[1]['routehint']] == [r['short_channel_id'] for r in routel3l5] + assert [h['channel'] for h in attempts[1]['routehint']] == [r['short_channel_id'] for r in routel3l4l5] + assert [h['channel'] for h in attempts[2]['routehint']] == [r['short_channel_id'] for r in routel3l5] @pytest.mark.xfail(strict=True) From b8389d29cada363972b45f1fb9623e101abc2b23 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Fri, 1 Feb 2019 16:23:35 +1030 Subject: [PATCH 4/8] pytest: make test_pay_direct more effective. The test sometimes passes: our routing logic always chooses between the shorter of two equal-cost routes (because we compare best with < not <=). By adding another hop, we add more noise, and by making the alternate route fee 0 we provide the worst case. But to be fair, we make the amount of the payment ~50c (15,000,000 msat), and increase our cltv-delay to 14 and fee-base 1000 to match mainnet. The final patch shows the effect of this choice. Otherwise our risk penalty is completely in the noise on mainnet which has the vast majority of fees set at 1000msat + 1ppm. Signed-off-by: Rusty Russell --- tests/test_pay.py | 26 ++++++++++++++++++-------- 1 file changed, 18 insertions(+), 8 deletions(-) diff --git a/tests/test_pay.py b/tests/test_pay.py index fa0f7a2fe1a6..c2bbffd7d29f 100644 --- a/tests/test_pay.py +++ b/tests/test_pay.py @@ -1345,12 +1345,22 @@ def test_pay_routeboost(node_factory, bitcoind): def test_pay_direct(node_factory, bitcoind): """Check that we prefer the direct route. """ - l1, l2, l3 = node_factory.get_nodes(3) - - # Direct channel + # l2->l3 is really cheap by comparison. + l0, l1, l2, l3 = node_factory.get_nodes(4, opts=[{'fee-base': 1000, + 'cltv-delta': 14}, + {'fee-base': 1000, + 'cltv-delta': 14}, + {'fee-base': 0, + 'cltv-delta': 14}, + {'fee-base': 1000, + 'cltv-delta': 14}]) + + # Direct channel l0->l1->l3 + l0.rpc.connect(l1.info['id'], 'localhost', l1.port) + l0.fund_channel(l1, 10**7) l1.rpc.connect(l3.info['id'], 'localhost', l3.port) l1.fund_channel(l3, 10**7) - # Indirect route + # Indirect route l0->l1->l2->l3 l1.rpc.connect(l2.info['id'], 'localhost', l2.port) l1.fund_channel(l2, 10**7) l2.rpc.connect(l3.info['id'], 'localhost', l3.port) @@ -1359,8 +1369,8 @@ def test_pay_direct(node_factory, bitcoind): # Let channels lock in. bitcoind.generate_block(5) - # Make sure l1 knows the l2->l3 channel. - l1.wait_channel_active(c3) + # Make sure l0 knows the l2->l3 channel. + l0.wait_channel_active(c3) # Find out how much msatoshi l1 owns on l1->l2 channel. l1l2msatreference = only_one(l1.rpc.getpeer(l2.info['id'])['channels'])['msatoshi_to_us'] @@ -1368,9 +1378,9 @@ def test_pay_direct(node_factory, bitcoind): # Try multiple times to ensure that route randomization # will not override our preference for direct route. for i in range(8): - inv = l3.rpc.invoice(10000, 'pay{}'.format(i), 'desc')['bolt11'] + inv = l3.rpc.invoice(15000000, 'pay{}'.format(i), 'desc')['bolt11'] - l1.rpc.pay(inv) + l0.rpc.pay(inv) # We should have gone the direct route, so # l1->l2 channel msatoshi_to_us should not From a5434e312412e108a5f033ff64d78ea1d53f60be Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Fri, 1 Feb 2019 16:23:38 +1030 Subject: [PATCH 5/8] gossipd: fix riskfactor passing. We used a u16, and a 1000 multiplier, which meant we wrapped at riskfactor 66. We also never undid the multiplier, so we ended up applying 1000x the riskfactor they specified. This changes us to pass the riskfactor with a 1M multiplier. The next patch changes the definition of riskfactor to be more useful. Signed-off-by: Rusty Russell --- gossipd/gossip_wire.csv | 3 ++- gossipd/gossipd.c | 6 +++--- lightningd/gossip_control.c | 3 ++- 3 files changed, 7 insertions(+), 5 deletions(-) diff --git a/gossipd/gossip_wire.csv b/gossipd/gossip_wire.csv index 658f54681390..e86f1260ec83 100644 --- a/gossipd/gossip_wire.csv +++ b/gossipd/gossip_wire.csv @@ -29,7 +29,8 @@ gossip_getroute_request,3006 gossip_getroute_request,,source,struct pubkey gossip_getroute_request,,destination,struct pubkey gossip_getroute_request,,msatoshi,u64 -gossip_getroute_request,,riskfactor,u16 +# We don't pass doubles, so pass riskfactor * 1000000. +gossip_getroute_request,,riskfactor_by_million,u64 gossip_getroute_request,,final_cltv,u32 gossip_getroute_request,,fuzz,double gossip_getroute_request,,num_excluded,u16 diff --git a/gossipd/gossipd.c b/gossipd/gossipd.c index 73502022885b..556e12aedd3f 100644 --- a/gossipd/gossipd.c +++ b/gossipd/gossipd.c @@ -1894,7 +1894,7 @@ static struct io_plan *getroute_req(struct io_conn *conn, struct daemon *daemon, struct pubkey source, destination; u64 msatoshi; u32 final_cltv; - u16 riskfactor; + u64 riskfactor_by_million; u32 max_hops; u8 *out; struct route_hop *hops; @@ -1909,7 +1909,7 @@ static struct io_plan *getroute_req(struct io_conn *conn, struct daemon *daemon, * avoid being too predictable. */ if (!fromwire_gossip_getroute_request(msg, msg, &source, &destination, - &msatoshi, &riskfactor, + &msatoshi, &riskfactor_by_million, &final_cltv, &fuzz, &excluded, &max_hops)) @@ -1921,7 +1921,7 @@ static struct io_plan *getroute_req(struct io_conn *conn, struct daemon *daemon, /* routing.c does all the hard work; can return NULL. */ hops = get_route(tmpctx, daemon->rstate, &source, &destination, - msatoshi, riskfactor, final_cltv, + msatoshi, riskfactor_by_million / 1000000.0, final_cltv, fuzz, pseudorand_u64(), excluded, max_hops); out = towire_gossip_getroute_reply(NULL, hops); diff --git a/lightningd/gossip_control.c b/lightningd/gossip_control.c index 3fc0a18ffaee..1b2a9b30fb54 100644 --- a/lightningd/gossip_control.c +++ b/lightningd/gossip_control.c @@ -352,7 +352,8 @@ static struct command_result *json_getroute(struct command *cmd, } u8 *req = towire_gossip_getroute_request(cmd, source, destination, - *msatoshi, *riskfactor * 1000, + *msatoshi, + *riskfactor * 1000000.0, *cltv, fuzz, excluded, *max_hops); From 7b0973b22458e97413b771ef5dcb46fa4642f551 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Fri, 1 Feb 2019 16:23:38 +1030 Subject: [PATCH 6/8] gossipd: take into account risk in final route comparison. We were only comparing by total msatoshis. Note, this *still* isn't sufficient to fix our indirect problem, as our risk values are all 1 (the minimum): lightning_gossipd(25480): 2 hop solution: 1501990 + 2 lightning_gossipd(25480): 3 hop solution: 1501971 + 3 ... lightning_gossipd(25480): => chose 3 hop solution Signed-off-by: Rusty Russell --- CHANGELOG.md | 1 + gossipd/routing.c | 6 +++++- 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ecbe90f78b2e..47e7a8efb060 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -39,6 +39,7 @@ changes. (eg. 4 billion) was slow due to a bug. - Fixed occasional deadlock with peers when exchanging huge amounts of gossip. - You can no longer make giant unpayable "wumbo" invoices. +- CLTV of total route now correctly evaluated when finding best route. ### Security diff --git a/gossipd/routing.c b/gossipd/routing.c index 1a23dd8c8131..5fcf2a2b8ad0 100644 --- a/gossipd/routing.c +++ b/gossipd/routing.c @@ -529,9 +529,13 @@ find_route(const tal_t *ctx, struct routing_state *rstate, best = 0; for (i = 1; i <= max_hops; i++) { - if (dst->bfg[i].total < dst->bfg[best].total) + status_trace("%i hop solution: %"PRIu64" + %"PRIu64, + i, dst->bfg[i].total, dst->bfg[i].risk); + if (dst->bfg[i].total + dst->bfg[i].risk + < dst->bfg[best].total + dst->bfg[best].risk) best = i; } + status_trace("=> chose %i hop solution", best); /* No route? */ if (dst->bfg[best].total >= INFINITE) { From 8e210e90591868feed60206157af51273f405579 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Fri, 1 Feb 2019 16:23:39 +1030 Subject: [PATCH 7/8] getroute: change definition (and pay default) for riskfactor. Up until now, riskfactor was useless due to implementation bugs, and also the default setting is wrong (too low to have an effect on reasonable payment scenarios). Let's simplify the definition (by assuming that P(failure) of a node is 1), to make it a simple percentage. I examined the current network fees to see what would work, and under this definition, a default of 10 seems reasonable (equivalent to 1000 under the old definition). It is *this* change which finally fixes our test case! The riskfactor is now 40msat (1500000 * 14 * 10 / 5259600 = 39.9), comparable with worst-case fuzz is 50msat (1001 * 0.05 = 50). Signed-off-by: Rusty Russell --- CHANGELOG.md | 2 + doc/lightning-getroute.7 | 357 ++++++++++++++++++++++++++++++----- doc/lightning-getroute.7.txt | 74 +++++--- doc/lightning-pay.7 | 6 +- doc/lightning-pay.7.txt | 2 +- gossipd/routing.c | 2 +- plugins/pay.c | 2 +- tests/test_pay.py | 1 - 8 files changed, 359 insertions(+), 87 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 47e7a8efb060..cf44d1acedd8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,6 +17,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0. - JSON API: `listpeers` now has a `direction` field in `channels`. - JSON API: `listchannels` now takes a `source` option to filter by node id. - JSON API: New command `paystatus` gives detailed information on `pay` commands. +- JSON API: `getroute` `riskfactor` argument is simplified; `pay` now defaults to setting it to 10. ### Changed @@ -40,6 +41,7 @@ changes. - Fixed occasional deadlock with peers when exchanging huge amounts of gossip. - You can no longer make giant unpayable "wumbo" invoices. - CLTV of total route now correctly evaluated when finding best route. +- `riskfactor` arguments to `pay` and `getroute` now have an effect. ### Security diff --git a/doc/lightning-getroute.7 b/doc/lightning-getroute.7 index af94a0aa3e88..3d9de867557f 100644 --- a/doc/lightning-getroute.7 +++ b/doc/lightning-getroute.7 @@ -2,12 +2,12 @@ .\" Title: lightning-getroute .\" Author: [see the "AUTHOR" section] .\" Generator: DocBook XSL Stylesheets v1.79.1 -.\" Date: 01/23/2019 +.\" Date: 02/01/2019 .\" Manual: \ \& .\" Source: \ \& .\" Language: English .\" -.TH "LIGHTNING\-GETROUTE" "7" "01/23/2019" "\ \&" "\ \&" +.TH "LIGHTNING\-GETROUTE" "7" "02/01/2019" "\ \&" "\ \&" .\" ----------------------------------------------------------------- .\" * Define some portability stuff .\" ----------------------------------------------------------------- @@ -36,9 +36,9 @@ lightning-getroute \- Command for routing a payment (low\-level)\&. .sp The \fBgetroute\fR RPC command attempts to find the best route for the payment of \fImsatoshi\fR to lightning node \fIid\fR, such that the payment will arrive at \fIid\fR with \fIcltv\fR\-blocks to spare (default 9)\&. .sp -There are two considerations for how good a route is: how low the fees are, and how long your payment will get stuck if a node goes down during the process\&. The \fIriskfactor\fR floating\-point field controls this tradeoff; it is the annual cost of your funds being stuck (as a percentage), multiplied by the percentage chance of each node failing\&. +There are two considerations for how good a route is: how low the fees are, and how long your payment will get stuck if a node goes down during the process\&. The \fIriskfactor\fR floating\-point field controls this tradeoff; it is the annual cost of your funds being stuck (as a percentage)\&. .sp -For example, if you thought there was a 1% chance that a node would fail, and it would cost you 20% per annum if that happened, \fIriskfactor\fR would be 20\&. +For example, if you thought the inconvenience of having funds stuck was worth 20% per annum interest, \fIriskfactor\fR would be 20\&. .sp If you didn\(cqt care about risk, \fIriskfactor\fR would be zero\&. .sp @@ -55,15 +55,13 @@ The formula used is the following approximation: .RS 4 .\} .nf -hop\-risk = num\-hops x per\-hop\-risk -timeout\-cost = blocks\-timeout x per\-block\-cost -risk\-fee = amount x hop\-risk x timeout\-cost +risk\-fee = amount x blocks\-timeout x per\-block\-cost .fi .if n \{\ .RE .\} .sp -We are given a \fIriskfactor\fR; expressed as two multiplied percentages is the same as fractions multiplied by 10000\&. There are 52596 blocks per year, thus \fIper\-block\-cost\fR x \fIper\-hop\-risk\fR is riskfactor\*(Aq divided by 5,259,600,000\&. +We are given a \fIriskfactor\fR expressed as a percentage\&. There are 52596 blocks per year, thus \fIper\-block\-cost\fR is \fIriskfactor\fR divided by 5,259,600\&. .sp The final result is: .sp @@ -71,26 +69,26 @@ The final result is: .RS 4 .\} .nf -risk\-fee = amount x num\-hops x blocks\-timeout x riskfactor / 5259600000 +risk\-fee = amount x blocks\-timeout x riskfactor / 5259600 .fi .if n \{\ .RE .\} .sp -Here are the risk fees as a percentage of the amount sent, using various parameters\&. For comparison with actual fees, we assume nodes charge 0\&.05%: +Here are the risk fees in millisatoshis, using various parameters\&. I assume a channel charges the default of 1000 millisatoshis plus 1 part\-per\-million\&. Common delay values on the network at 14 and 144\&. .TS allbox tab(:); ltB ltB ltB ltB ltB. T{ -Riskfactor +Amount (msat) T}:T{ -Nodes +Riskfactor T}:T{ -Delay per node +Delay T}:T{ -Risk Fee % +Risk Fee T}:T{ -Route fee % +Route fee T} .T& lt lt lt lt lt @@ -101,160 +99,417 @@ lt lt lt lt lt lt lt lt lt lt lt lt lt lt lt lt lt lt lt lt +lt lt lt lt lt +lt lt lt lt lt +lt lt lt lt lt +lt lt lt lt lt +lt lt lt lt lt +lt lt lt lt lt +lt lt lt lt lt +lt lt lt lt lt +lt lt lt lt lt +lt lt lt lt lt +lt lt lt lt lt +lt lt lt lt lt +lt lt lt lt lt +lt lt lt lt lt +lt lt lt lt lt lt lt lt lt lt. T{ .sp -0\&.001 +10,000 T}:T{ .sp -5 +1 T}:T{ .sp -6 +14 T}:T{ .sp 0 T}:T{ .sp -0\&.25 +1001 T} T{ .sp -1 +10,000 T}:T{ .sp -5 +10 T}:T{ .sp -6 +14 T}:T{ .sp 0 T}:T{ .sp -0\&.25 +1001 +T} +T{ +.sp +10,000 +T}:T{ +.sp +100 +T}:T{ +.sp +14 +T}:T{ +.sp +2 +T}:T{ +.sp +1001 T} T{ .sp +10,000 +T}:T{ +.sp 1000 T}:T{ .sp -5 +14 T}:T{ .sp -6 +26 T}:T{ .sp -0\&.0029 +1001 +T} +T{ +.sp +1,000,000 T}:T{ .sp -0\&.25 +1 +T}:T{ +.sp +14 +T}:T{ +.sp +2 +T}:T{ +.sp +1001 T} T{ .sp -0\&.001 +1,000,000 T}:T{ .sp 10 T}:T{ .sp -72 +14 T}:T{ .sp -0 +26 T}:T{ .sp -0\&.5 +1001 T} T{ .sp +1,000,000 +T}:T{ +.sp +100 +T}:T{ +.sp +14 +T}:T{ +.sp +266 +T}:T{ +.sp +1001 +T} +T{ +.sp +1,000,000 +T}:T{ +.sp +1000 +T}:T{ +.sp +14 +T}:T{ +.sp +2661 +T}:T{ +.sp +1001 +T} +T{ +.sp +100,000,000 +T}:T{ +.sp 1 T}:T{ .sp +14 +T}:T{ +.sp +266 +T}:T{ +.sp +1100 +T} +T{ +.sp +100,000,000 +T}:T{ +.sp 10 T}:T{ .sp -72 +14 +T}:T{ +.sp +2661 +T}:T{ +.sp +1100 +T} +T{ +.sp +100,000,000 +T}:T{ +.sp +100 T}:T{ .sp -0\&.0001 +14 T}:T{ .sp -0\&.5 +26617 +T}:T{ +.sp +1100 T} T{ .sp +100,000,000 +T}:T{ +.sp 1000 T}:T{ .sp +14 +T}:T{ +.sp +266179 +T}:T{ +.sp +1100 +T} +T{ +.sp +10,000 +T}:T{ +.sp +1 +T}:T{ +.sp +144 +T}:T{ +.sp +0 +T}:T{ +.sp +1001 +T} +T{ +.sp +10,000 +T}:T{ +.sp 10 T}:T{ .sp -72 +144 T}:T{ .sp -0\&.1369 +2 T}:T{ .sp -0\&.5 +1001 T} T{ .sp -0\&.001 +10,000 T}:T{ .sp -20 +100 T}:T{ .sp -1008 +144 T}:T{ .sp -0 +27 T}:T{ .sp -1\&.0 +1001 T} T{ .sp +10,000 +T}:T{ +.sp +1000 +T}:T{ +.sp +144 +T}:T{ +.sp +273 +T}:T{ +.sp +1001 +T} +T{ +.sp +1,000,000 +T}:T{ +.sp 1 T}:T{ .sp -20 +144 +T}:T{ +.sp +27 +T}:T{ +.sp +1001 +T} +T{ +.sp +1,000,000 +T}:T{ +.sp +10 +T}:T{ +.sp +144 +T}:T{ +.sp +273 +T}:T{ +.sp +1001 +T} +T{ +.sp +1,000,000 +T}:T{ +.sp +100 T}:T{ .sp -1008 +144 T}:T{ .sp -0\&.0077 +2737 T}:T{ .sp -1\&.0 +1001 T} T{ .sp +1,000,000 +T}:T{ +.sp 1000 T}:T{ .sp -20 +144 +T}:T{ +.sp +27378 +T}:T{ +.sp +1001 +T} +T{ +.sp +100,000,000 +T}:T{ +.sp +1 +T}:T{ +.sp +144 +T}:T{ +.sp +2737 +T}:T{ +.sp +1100 +T} +T{ +.sp +100,000,000 +T}:T{ +.sp +10 T}:T{ .sp -1008 +144 T}:T{ .sp -7\&.6660 +27378 T}:T{ .sp -1\&.0 +1100 +T} +T{ +.sp +100,000,000 +T}:T{ +.sp +100 +T}:T{ +.sp +144 +T}:T{ +.sp +273785 +T}:T{ +.sp +1100 +T} +T{ +.sp +100,000,000 +T}:T{ +.sp +1000 +T}:T{ +.sp +144 +T}:T{ +.sp +2737850 +T}:T{ +.sp +1100 T} .TE .sp 1 .SH "RECOMMENDED RISKFACTOR VALUES" .sp -0\&.001 is a value for tie\-breaking in favor of shorter routes, but not really costing in any risk\&. +The default \fIfuzz\fR factor is 5%, so as you can see from the table above, that tends to overwhelm the effect of \fIriskfactor\fR less than about 5\&. .sp 1 is a conservative value for a stable lightning network with very few failures\&. .sp 1000 is an aggressive value for trying to minimize timeouts at all costs\&. +.sp +The default for lightning\-pay(7) is 10, which starts to become a major factor for larger amounts, and is basically ignored for tiny ones\&. .SH "RETURN VALUE" .sp On success, a "route" array is returned\&. Each array element contains \fIid\fR (the node being routed through), \fImsatoshi\fR (the millisatoshis sent), and \fIdelay\fR (the number of blocks to timeout at this node)\&. diff --git a/doc/lightning-getroute.7.txt b/doc/lightning-getroute.7.txt index 388072f1488b..771b95ca5ba7 100644 --- a/doc/lightning-getroute.7.txt +++ b/doc/lightning-getroute.7.txt @@ -21,11 +21,10 @@ There are two considerations for how good a route is: how low the fees are, and how long your payment will get stuck if a node goes down during the process. The 'riskfactor' floating-point field controls this tradeoff; it is the annual cost of your funds being stuck (as a -percentage), multiplied by the percentage chance of each node failing. +percentage). -For example, if you thought there was a 1% chance that a node would -fail, and it would cost you 20% per annum if that happened, -'riskfactor' would be 20. +For example, if you thought the inconvenience of having funds stuck was +worth 20% per annum interest, 'riskfactor' would be 20. If you didn't care about risk, 'riskfactor' would be zero. @@ -46,45 +45,59 @@ for the purposes of comparing routes. The formula used is the following approximation: ---- -hop-risk = num-hops x per-hop-risk -timeout-cost = blocks-timeout x per-block-cost -risk-fee = amount x hop-risk x timeout-cost +risk-fee = amount x blocks-timeout x per-block-cost ---- -We are given a 'riskfactor'; expressed as two multiplied percentages -is the same as fractions multiplied by 10000. There are 52596 blocks -per year, thus 'per-block-cost' x 'per-hop-risk' is riskfactor' -divided by 5,259,600,000. +We are given a 'riskfactor' expressed as a percentage. There are 52596 blocks +per year, thus 'per-block-cost' is 'riskfactor' divided by 5,259,600. The final result is: ---- -risk-fee = amount x num-hops x blocks-timeout x riskfactor / 5259600000 +risk-fee = amount x blocks-timeout x riskfactor / 5259600 ---- -Here are the risk fees as a percentage of the amount sent, using -various parameters. For comparison with actual fees, we assume nodes -charge 0.05%: +Here are the risk fees in millisatoshis, using various parameters. I +assume a channel charges the default of 1000 millisatoshis plus 1 +part-per-million. Common delay values on the network at 14 and 144. [options="header"] |======================= -|Riskfactor |Nodes | Delay per node |Risk Fee % |Route fee % -|0.001 |5 | 6 |0 |0.25 -|1 |5 | 6 |0 |0.25 -|1000 |5 | 6 |0.0029 |0.25 - -|0.001 |10 | 72 |0 |0.5 -|1 |10 | 72 |0.0001 |0.5 -|1000 |10 | 72 |0.1369 |0.5 - -|0.001 |20 | 1008 |0 |1.0 -|1 |20 | 1008 |0.0077 |1.0 -|1000 |20 | 1008 |7.6660 |1.0 +|Amount (msat) |Riskfactor | Delay |Risk Fee |Route fee +|10,000 |1 | 14 |0 |1001 +|10,000 |10 | 14 |0 |1001 +|10,000 |100 | 14 |2 |1001 +|10,000 |1000 | 14 |26 |1001 + +|1,000,000 |1 | 14 |2 |1001 +|1,000,000 |10 | 14 |26 |1001 +|1,000,000 |100 | 14 |266 |1001 +|1,000,000 |1000 | 14 |2661 |1001 + +|100,000,000 |1 | 14 |266 |1100 +|100,000,000 |10 | 14 |2661 |1100 +|100,000,000 |100 | 14 |26617 |1100 +|100,000,000 |1000 | 14 |266179 |1100 + +|10,000 |1 | 144 |0 |1001 +|10,000 |10 | 144 |2 |1001 +|10,000 |100 | 144 |27 |1001 +|10,000 |1000 | 144 |273 |1001 + +|1,000,000 |1 | 144 |27 |1001 +|1,000,000 |10 | 144 |273 |1001 +|1,000,000 |100 | 144 |2737 |1001 +|1,000,000 |1000 | 144 |27378 |1001 + +|100,000,000 |1 | 144 |2737 |1100 +|100,000,000 |10 | 144 |27378 |1100 +|100,000,000 |100 | 144 |273785 |1100 +|100,000,000 |1000 | 144 |2737850 |1100 |======================= RECOMMENDED RISKFACTOR VALUES ----------------------------- -0.001 is a value for tie-breaking in favor of shorter routes, but not really -costing in any risk. +The default 'fuzz' factor is 5%, so as you can see from the table above, +that tends to overwhelm the effect of 'riskfactor' less than about 5. 1 is a conservative value for a stable lightning network with very few failures. @@ -92,6 +105,9 @@ failures. 1000 is an aggressive value for trying to minimize timeouts at all costs. +The default for lightning-pay(7) is 10, which starts to become a major +factor for larger amounts, and is basically ignored for tiny ones. + RETURN VALUE ------------ diff --git a/doc/lightning-pay.7 b/doc/lightning-pay.7 index 1983dbab3a88..b4193205bf45 100644 --- a/doc/lightning-pay.7 +++ b/doc/lightning-pay.7 @@ -2,12 +2,12 @@ .\" Title: lightning-pay .\" Author: [see the "AUTHOR" section] .\" Generator: DocBook XSL Stylesheets v1.79.1 -.\" Date: 11/26/2018 +.\" Date: 02/01/2019 .\" Manual: \ \& .\" Source: \ \& .\" Language: English .\" -.TH "LIGHTNING\-PAY" "7" "11/26/2018" "\ \&" "\ \&" +.TH "LIGHTNING\-PAY" "7" "02/01/2019" "\ \&" "\ \&" .\" ----------------------------------------------------------------- .\" * Define some portability stuff .\" ----------------------------------------------------------------- @@ -34,7 +34,7 @@ lightning-pay \- Command for sending a payment to a BOLT11 invoice \fBpay\fR \fIbolt11\fR [\fImsatoshi\fR] [\fIdescription\fR] [\fIriskfactor\fR] [\fImaxfeepercent\fR] [\fIretry_for\fR] [\fImaxdelay\fR] [\fIexemptfee\fR] .SH "DESCRIPTION" .sp -The \fBpay\fR RPC command attempts to find a route to the given destination, and send the funds it asks for\&. If the \fIbolt11\fR does not contain an amount, \fImsatoshi\fR is required, otherwise if it is specified it must be \fInull\fR\&. If \fIbolt11\fR contains a description hash (\fIh\fR field) \fIdescription\fR is required, otherwise it is unused\&. The \fIriskfactor\fR is described in detail in lightning\-getroute(7), and defaults to 1\&.0\&. The \fImaxfeepercent\fR limits the money paid in fees, and defaults to 0\&.5\&. The maxfeepercent\*(Aq is a percentage of the amount that is to be paid\&. The `exemptfee option can be used for tiny payments which would be dominated by the fee leveraged by forwarding nodes\&. Setting exemptfee allows the maxfeepercent check to be skipped on fees that are smaller than exemptfee (default: 5000 millisatoshi)\&. +The \fBpay\fR RPC command attempts to find a route to the given destination, and send the funds it asks for\&. If the \fIbolt11\fR does not contain an amount, \fImsatoshi\fR is required, otherwise if it is specified it must be \fInull\fR\&. If \fIbolt11\fR contains a description hash (\fIh\fR field) \fIdescription\fR is required, otherwise it is unused\&. The \fIriskfactor\fR is described in detail in lightning\-getroute(7), and defaults to 10\&. The \fImaxfeepercent\fR limits the money paid in fees, and defaults to 0\&.5\&. The maxfeepercent\*(Aq is a percentage of the amount that is to be paid\&. The `exemptfee option can be used for tiny payments which would be dominated by the fee leveraged by forwarding nodes\&. Setting exemptfee allows the maxfeepercent check to be skipped on fees that are smaller than exemptfee (default: 5000 millisatoshi)\&. .sp The response will occur when the payment fails or succeeds\&. Once a payment has succeeded, calls to \fBpay\fR with the same \fIbolt11\fR will succeed immediately\&. .sp diff --git a/doc/lightning-pay.7.txt b/doc/lightning-pay.7.txt index 04e830f6517d..fab59a867d87 100644 --- a/doc/lightning-pay.7.txt +++ b/doc/lightning-pay.7.txt @@ -18,7 +18,7 @@ and send the funds it asks for. If the 'bolt11' does not contain an amount, 'msatoshi' is required, otherwise if it is specified it must be 'null'. If 'bolt11' contains a description hash ('h' field) 'description' is required, otherwise it is unused. The 'riskfactor' is described in detail -in lightning-getroute(7), and defaults to 1.0. +in lightning-getroute(7), and defaults to 10. The 'maxfeepercent' limits the money paid in fees, and defaults to 0.5. The `maxfeepercent' is a percentage of the amount that is to be paid. diff --git a/gossipd/routing.c b/gossipd/routing.c index 5fcf2a2b8ad0..e77c2e606359 100644 --- a/gossipd/routing.c +++ b/gossipd/routing.c @@ -1528,7 +1528,7 @@ struct route_hop *get_route(const tal_t *ctx, struct routing_state *rstate, } route = find_route(ctx, rstate, source, destination, msatoshi, - riskfactor / BLOCKS_PER_YEAR / 10000, + riskfactor / BLOCKS_PER_YEAR / 100, fuzz, &base_seed, max_hops, &fee); /* Now restore the capacity. */ diff --git a/plugins/pay.c b/plugins/pay.c index 5910e2f99078..352c25f7cdb7 100644 --- a/plugins/pay.c +++ b/plugins/pay.c @@ -833,7 +833,7 @@ static struct command_result *handle_pay(struct command *cmd, p_req("bolt11", param_string, &b11str), p_opt("msatoshi", param_u64, &msatoshi), p_opt("description", param_string, &pc->desc), - p_opt_def("riskfactor", param_double, &riskfactor, 1.0), + p_opt_def("riskfactor", param_double, &riskfactor, 10), p_opt_def("maxfeepercent", param_percent, &maxfeepercent, 0.5), p_opt_def("retry_for", param_number, &retryfor, 60), p_opt_def("maxdelay", param_number, &maxdelay, diff --git a/tests/test_pay.py b/tests/test_pay.py index c2bbffd7d29f..4bad1387dd1f 100644 --- a/tests/test_pay.py +++ b/tests/test_pay.py @@ -1341,7 +1341,6 @@ def test_pay_routeboost(node_factory, bitcoind): assert [h['channel'] for h in attempts[2]['routehint']] == [r['short_channel_id'] for r in routel3l5] -@pytest.mark.xfail(strict=True) def test_pay_direct(node_factory, bitcoind): """Check that we prefer the direct route. """ From bc1c0af16a15ba9a872e0a4051fea3c66aae1a6d Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Sun, 3 Feb 2019 10:54:11 +1030 Subject: [PATCH 8/8] pytest: don't time out on test_pay_direct !DEVELOPER Travis timed out. Waiting for three fundchannel commands depends on the bitcoind polling interval (30 seconds), and then waiting for gossip propagation requires two propagation intervals (120 seconds). Signed-off-by: Rusty Russell --- tests/test_pay.py | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/tests/test_pay.py b/tests/test_pay.py index 4bad1387dd1f..1dea06e04bcc 100644 --- a/tests/test_pay.py +++ b/tests/test_pay.py @@ -1356,20 +1356,23 @@ def test_pay_direct(node_factory, bitcoind): # Direct channel l0->l1->l3 l0.rpc.connect(l1.info['id'], 'localhost', l1.port) - l0.fund_channel(l1, 10**7) + # Waiting takes a *long* time if !DEVELOPER. + l0.fund_channel(l1, 10**7, wait_for_active=False) l1.rpc.connect(l3.info['id'], 'localhost', l3.port) - l1.fund_channel(l3, 10**7) + l1.fund_channel(l3, 10**7, wait_for_active=False) # Indirect route l0->l1->l2->l3 l1.rpc.connect(l2.info['id'], 'localhost', l2.port) - l1.fund_channel(l2, 10**7) + l1.fund_channel(l2, 10**7, wait_for_active=False) l2.rpc.connect(l3.info['id'], 'localhost', l3.port) - c3 = l2.fund_channel(l3, 10**7) + c3 = l2.fund_channel(l3, 10**7, wait_for_active=False) # Let channels lock in. bitcoind.generate_block(5) # Make sure l0 knows the l2->l3 channel. - l0.wait_channel_active(c3) + # Without DEVELOPER, channel lockin can take 30 seconds to detect, + # and gossip 2 minutes to propagate + wait_for(lambda: l0.is_channel_active(c3), timeout=180) # Find out how much msatoshi l1 owns on l1->l2 channel. l1l2msatreference = only_one(l1.rpc.getpeer(l2.info['id'])['channels'])['msatoshi_to_us']