From 3f22fc9dc9db985c1281d5c5fe2c9e3ad54a17d3 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Mon, 2 Oct 2023 11:42:19 +1030 Subject: [PATCH 1/6] plugins/topology: remove unused fuzz factor. This breaks Dijkstra, which is presumably why it was actually disabled. Remove the code altoghether, instead. Changelog-Fixed: JSON-RPC: `getroute` now documents that it ignores `fuzzpercent`. Signed-off-by: Rusty Russell --- doc/lightning-getroute.7.md | 8 ++------ plugins/topology.c | 36 ++---------------------------------- 2 files changed, 4 insertions(+), 40 deletions(-) diff --git a/doc/lightning-getroute.7.md b/doc/lightning-getroute.7.md index ff3777f31d0b..e87f4dd972f4 100644 --- a/doc/lightning-getroute.7.md +++ b/doc/lightning-getroute.7.md @@ -34,12 +34,8 @@ If you didn't care about risk, *riskfactor* would be zero. *fromid* is the node to start the route from: default is this node. -The *fuzzpercent* is a non-negative floating-point number, representing a -percentage of the actual fee. The *fuzzpercent* is used to distort -computed fees along each channel, to provide some randomization to the -route generated. 0.0 means the exact fee of that channel is used, while -100.0 means the fee used might be from 0 to twice the actual fee. The -default is 5.0, or up to 5% fee distortion. +*fuzzpercent* was used to distort fees to provide some randomization to the +route generated, but it was not properly implemented and is ignored. *exclude* is a JSON array of short-channel-id/direction (e.g. [ "564334x877x1/0", "564195x1292x0/1" ]) or node-id which should be excluded diff --git a/plugins/topology.c b/plugins/topology.c index e3df729a1ae9..1ec27d0fb70a 100644 --- a/plugins/topology.c +++ b/plugins/topology.c @@ -28,30 +28,6 @@ static struct gossmap *get_gossmap(void) return global_gossmap; } -/* Convenience global since route_score_fuzz doesn't take args. 0 to 1. */ -static double fuzz; - -/* Prioritize costs over distance, but with fuzz. Cost must be - * the same when the same channel queried, so we base it on that. */ -static u64 route_score_fuzz(u32 distance, - struct amount_msat cost, - struct amount_msat risk, - int dir UNUSED, - const struct gossmap_chan *c) -{ - u64 costs = cost.millisatoshis + risk.millisatoshis; /* Raw: score */ - /* Use the literal pointer, since it's stable. */ - u64 h = siphash24(siphash_seed(), &c, sizeof(c)); - - /* Use distance as the tiebreaker */ - costs += distance; - - /* h / (UINT64_MAX / 2.0) is between 0 and 2. */ - costs *= (h / (double)(UINT64_MAX / 2) - 1) * fuzz; - - return costs; -} - static bool can_carry(const struct gossmap *map, const struct gossmap_chan *c, int dir, @@ -141,13 +117,6 @@ static struct command_result *json_getroute(struct command *cmd, NULL)) return command_param_failed(); - /* Convert from percentage */ - fuzz = *fuzz_millionths / 100.0 / 1000000.0; - if (fuzz > 1.0) - return command_fail_badparam(cmd, "fuzzpercent", - buffer, params, - "should be <= 100"); - gossmap = get_gossmap(); src = gossmap_find_node(gossmap, source); if (!src) @@ -161,10 +130,9 @@ static struct command_result *json_getroute(struct command *cmd, "%s: unknown destination node_id (no public channels?)", type_to_string(tmpctx, struct node_id, destination)); - fuzz = 0; dij = dijkstra(tmpctx, gossmap, dst, *msat, *riskfactor_millionths / 1000000.0, - can_carry, route_score_fuzz, excluded); + can_carry, route_score_cheaper, excluded); route = route_from_dijkstra(dij, gossmap, dij, src, *msat, *cltv); if (!route) return command_fail(cmd, PAY_ROUTE_NOT_FOUND, "Could not find a route"); @@ -637,7 +605,7 @@ static const struct plugin_command commands[] = { "Primitive route command", "Show route to {id} for {msatoshi}, using {riskfactor} and optional {cltv} (default 9). " "If specified search from {fromid} otherwise use this node as source. " - "Randomize the route with up to {fuzzpercent} (default 5.0). " + "Randomize the route with up to {fuzzpercent} (ignored)). " "{exclude} an array of short-channel-id/direction (e.g. [ '564334x877x1/0', '564195x1292x0/1' ]) " "or node-id from consideration. " "Set the {maxhops} the route can take (default 20).", From 77c97674298b841c5567704935f5b6c9d9e64bc0 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Mon, 2 Oct 2023 11:43:19 +1030 Subject: [PATCH 2/6] plugins/topology: split getroute logic. Mechanical change to bundle into struct getroute_info for next patch which uses callback. Signed-off-by: Rusty Russell --- plugins/topology.c | 84 ++++++++++++++++++++++++++-------------------- 1 file changed, 48 insertions(+), 36 deletions(-) diff --git a/plugins/topology.c b/plugins/topology.c index 1ec27d0fb70a..506e51d65470 100644 --- a/plugins/topology.c +++ b/plugins/topology.c @@ -86,66 +86,54 @@ static void json_add_route_hop(struct json_stream *js, json_object_end(js); } -static struct command_result *json_getroute(struct command *cmd, - const char *buffer, - const jsmntok_t *params) -{ +struct getroute_info { struct node_id *destination; struct node_id *source; struct amount_msat *msat; u32 *cltv; /* risk factor 12.345% -> riskfactor_millionths = 12345000 */ - u64 *riskfactor_millionths, *fuzz_millionths; + u64 *riskfactor_millionths; struct route_exclusion **excluded; u32 *max_hops; +}; + +static struct command_result *try_route(struct command *cmd, + struct gossmap *gossmap, + struct getroute_info *info) +{ const struct dijkstra *dij; struct route_hop *route; struct gossmap_node *src, *dst; struct json_stream *js; - struct gossmap *gossmap; - - if (!param(cmd, buffer, params, - p_req("id", param_node_id, &destination), - p_req("amount_msat|msatoshi", param_msat, &msat), - p_req("riskfactor", param_millionths, &riskfactor_millionths), - p_opt_def("cltv", param_number, &cltv, 9), - p_opt_def("fromid", param_node_id, &source, local_id), - p_opt_def("fuzzpercent", param_millionths, &fuzz_millionths, - 5000000), - p_opt("exclude", param_route_exclusion_array, &excluded), - p_opt_def("maxhops", param_number, &max_hops, ROUTING_MAX_HOPS), - NULL)) - return command_param_failed(); - - gossmap = get_gossmap(); - src = gossmap_find_node(gossmap, source); + src = gossmap_find_node(gossmap, info->source); if (!src) return command_fail(cmd, JSONRPC2_INVALID_PARAMS, "%s: unknown source node_id (no public channels?)", - type_to_string(tmpctx, struct node_id, source)); + type_to_string(tmpctx, struct node_id, info->source)); - dst = gossmap_find_node(gossmap, destination); + dst = gossmap_find_node(gossmap, info->destination); if (!dst) return command_fail(cmd, JSONRPC2_INVALID_PARAMS, "%s: unknown destination node_id (no public channels?)", - type_to_string(tmpctx, struct node_id, destination)); + type_to_string(tmpctx, struct node_id, info->destination)); - dij = dijkstra(tmpctx, gossmap, dst, *msat, - *riskfactor_millionths / 1000000.0, - can_carry, route_score_cheaper, excluded); - route = route_from_dijkstra(dij, gossmap, dij, src, *msat, *cltv); + dij = dijkstra(tmpctx, gossmap, dst, *info->msat, + *info->riskfactor_millionths / 1000000.0, + can_carry, route_score_cheaper, info->excluded); + route = route_from_dijkstra(dij, gossmap, dij, src, + *info->msat, *info->cltv); if (!route) return command_fail(cmd, PAY_ROUTE_NOT_FOUND, "Could not find a route"); /* If it's too far, fall back to using shortest path. */ - if (tal_count(route) > *max_hops) { - plugin_notify_message(cmd, LOG_INFORM, "Cheapest route %zu hops: seeking shorter (no fuzz)", + if (tal_count(route) > *info->max_hops) { + plugin_notify_message(cmd, LOG_INFORM, "Cheapest route %zu hops: seeking shorter", tal_count(route)); - dij = dijkstra(tmpctx, gossmap, dst, *msat, - *riskfactor_millionths / 1000000.0, - can_carry, route_score_shorter, excluded); - route = route_from_dijkstra(dij, gossmap, dij, src, *msat, *cltv); - if (tal_count(route) > *max_hops) + dij = dijkstra(tmpctx, gossmap, dst, *info->msat, + *info->riskfactor_millionths / 1000000.0, + can_carry, route_score_shorter, info->excluded); + route = route_from_dijkstra(dij, gossmap, dij, src, *info->msat, *info->cltv); + if (tal_count(route) > *info->max_hops) return command_fail(cmd, PAY_ROUTE_NOT_FOUND, "Shortest route was %zu", tal_count(route)); } @@ -160,6 +148,30 @@ static struct command_result *json_getroute(struct command *cmd, return command_finished(cmd, js); } +static struct command_result *json_getroute(struct command *cmd, + const char *buffer, + const jsmntok_t *params) +{ + struct getroute_info *info = tal(cmd, struct getroute_info); + u64 *fuzz_ignored; + struct gossmap *gossmap; + + if (!param(cmd, buffer, params, + p_req("id", param_node_id, &info->destination), + p_req("amount_msat|msatoshi", param_msat, &info->msat), + p_req("riskfactor", param_millionths, &info->riskfactor_millionths), + p_opt_def("cltv", param_number, &info->cltv, 9), + p_opt_def("fromid", param_node_id, &info->source, local_id), + p_opt("fuzzpercent", param_millionths, &fuzz_ignored), + p_opt("exclude", param_route_exclusion_array, &info->excluded), + p_opt_def("maxhops", param_number, &info->max_hops, ROUTING_MAX_HOPS), + NULL)) + return command_param_failed(); + + gossmap = get_gossmap(); + return try_route(cmd, gossmap, info); +} + HTABLE_DEFINE_TYPE(struct node_id, node_id_keyof, node_id_hash, node_id_eq, node_map); From 77821a026b58072148ae1dc8c659e606095c040b Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Mon, 2 Oct 2023 11:44:19 +1030 Subject: [PATCH 3/6] plugins/topology: convert listpeerchannels into local overlay. This prepares us for when the gossmap doesn't contain private channel info. Signed-off-by: Rusty Russell --- plugins/topology.c | 123 +++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 120 insertions(+), 3 deletions(-) diff --git a/plugins/topology.c b/plugins/topology.c index 506e51d65470..296c615745ce 100644 --- a/plugins/topology.c +++ b/plugins/topology.c @@ -148,13 +148,127 @@ static struct command_result *try_route(struct command *cmd, return command_finished(cmd, js); } +static struct gossmap_localmods * +gossmods_from_listpeerchannels(const tal_t *ctx, + struct plugin *plugin, + struct gossmap *gossmap, + const char *buf, + const jsmntok_t *toks) +{ + struct gossmap_localmods *mods = gossmap_localmods_new(ctx); + const jsmntok_t *channels, *channel; + size_t i; + + channels = json_get_member(buf, toks, "channels"); + json_for_each_arr(i, channel, channels) { + struct short_channel_id scid; + int dir; + bool connected; + struct node_id dst; + struct amount_msat capacity; + const char *state, *err; + + /* scid/direction may not exist. */ + scid.u64 = 0; + capacity = AMOUNT_MSAT(0); + err = json_scan(tmpctx, buf, channel, + "{short_channel_id?:%," + "direction?:%," + "spendable_msat?:%," + "peer_connected:%," + "state:%," + "peer_id:%}", + JSON_SCAN(json_to_short_channel_id, &scid), + JSON_SCAN(json_to_int, &dir), + JSON_SCAN(json_to_msat, &capacity), + JSON_SCAN(json_to_bool, &connected), + JSON_SCAN_TAL(tmpctx, json_strdup, &state), + JSON_SCAN(json_to_node_id, &dst)); + if (err) { + plugin_err(plugin, + "Bad listpeerchannels.channels %zu: %s", + i, err); + } + + /* Unusable if no scid (yet) */ + if (scid.u64 == 0) + continue; + + /* Disable if in bad state, or disconnected */ + if (!streq(state, "CHANNELD_NORMAL") + && !streq(state, "CHANNELD_AWAITING_SPLICE")) { + goto disable; + } + + if (!connected) { + goto disable; + } + + /* FIXME: features? */ + gossmap_local_addchan(mods, &local_id, &dst, &scid, NULL); + gossmap_local_updatechan(mods, &scid, + AMOUNT_MSAT(0), capacity, + /* We don't charge ourselves fees */ + 0, 0, 0, + true, + dir); + continue; + + disable: + /* Only apply fake "disabled" if channel exists */ + if (gossmap_find_chan(gossmap, &scid)) { + gossmap_local_updatechan(mods, &scid, + AMOUNT_MSAT(0), AMOUNT_MSAT(0), + 0, 0, 0, + false, + dir); + } + } + + return mods; +} + +static struct command_result * +listpeerchannels_getroute_done(struct command *cmd, + const char *buf, + const jsmntok_t *result, + struct getroute_info *info) +{ + struct gossmap *gossmap; + struct gossmap_localmods *mods; + struct command_result *res; + + /* Get local knowledge */ + gossmap = get_gossmap(); + mods = gossmods_from_listpeerchannels(tmpctx, cmd->plugin, + gossmap, buf, result); + + /* Overlay local knowledge for dijkstra */ + gossmap_apply_localmods(gossmap, mods); + res = try_route(cmd, gossmap, info); + gossmap_remove_localmods(gossmap, mods); + + return res; +} + +static struct command_result *listpeerchannels_err(struct command *cmd, + const char *buf, + const jsmntok_t *result, + struct getroute_info *info) +{ + plugin_err(cmd->plugin, + "Bad listpeerchannels: %.*s", + json_tok_full_len(result), + json_tok_full(buf, result)); +} + static struct command_result *json_getroute(struct command *cmd, const char *buffer, const jsmntok_t *params) { struct getroute_info *info = tal(cmd, struct getroute_info); + struct out_req *req; u64 *fuzz_ignored; - struct gossmap *gossmap; if (!param(cmd, buffer, params, p_req("id", param_node_id, &info->destination), @@ -168,8 +282,11 @@ static struct command_result *json_getroute(struct command *cmd, NULL)) return command_param_failed(); - gossmap = get_gossmap(); - return try_route(cmd, gossmap, info); + /* Add local info */ + req = jsonrpc_request_start(cmd->plugin, cmd, "listpeerchannels", + listpeerchannels_getroute_done, + listpeerchannels_err, info); + return send_outreq(cmd->plugin, req); } HTABLE_DEFINE_TYPE(struct node_id, node_id_keyof, node_id_hash, node_id_eq, From 782f2c6fd7a74a34fd0ce491045dc87cafdeed92 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Mon, 2 Oct 2023 11:45:19 +1030 Subject: [PATCH 4/6] renepay: work around change in fundchannel in tests. The tests will wait until it's locally enabled, but it might not have the update in the gossip store. So have renepay enhance its local view even if it already knows about the channel (this is correct anyway, it just isn't very important usually). Signed-off-by: Rusty Russell --- plugins/renepay/uncertainty_network.c | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/plugins/renepay/uncertainty_network.c b/plugins/renepay/uncertainty_network.c index 8c4eedffa02c..e335daaa752e 100644 --- a/plugins/renepay/uncertainty_network.c +++ b/plugins/renepay/uncertainty_network.c @@ -305,22 +305,22 @@ bool uncertainty_network_update_from_listpeerchannels( /* FIXME: features? */ gossmap_local_addchan(p->local_gossmods, &src, &dst, &scidd.scid, NULL); - gossmap_local_updatechan(p->local_gossmods, - &scidd.scid, + } + gossmap_local_updatechan(p->local_gossmods, + &scidd.scid, - /* TODO(eduardo): does it - * matter to consider HTLC - * limits in our own channel? */ - AMOUNT_MSAT(0),capacity, + /* TODO(eduardo): does it + * matter to consider HTLC + * limits in our own channel? */ + AMOUNT_MSAT(0),capacity, - /* fees = */0,0, + /* fees = */0,0, - /* TODO(eduardo): does it - * matter to set this delay? */ - /*delay=*/0, - true, - scidd.dir); - } + /* TODO(eduardo): does it + * matter to set this delay? */ + /*delay=*/0, + true, + scidd.dir); /* FIXME: There is a bug with us trying to send more down a local * channel (after fees) than it has capacity. For now, we reduce From a471672b5be30150f235d692439dc222c6df6111 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Mon, 2 Oct 2023 14:22:57 +1030 Subject: [PATCH 5/6] pytest: remove old test_tlv_or_legacy We no longer support legacy at all, so this test doesn't really test anything. Signed-off-by: Rusty Russell --- tests/test_pay.py | 42 ------------------------------------------ 1 file changed, 42 deletions(-) diff --git a/tests/test_pay.py b/tests/test_pay.py index 59641087ae1b..d73a8b618bf3 100644 --- a/tests/test_pay.py +++ b/tests/test_pay.py @@ -2745,48 +2745,6 @@ def test_error_returns_blockheight(node_factory, bitcoind): == '400f{:016x}{:08x}'.format(100, bitcoind.rpc.getblockcount())) -def test_tlv_or_legacy(node_factory, bitcoind): - # Ideally we'd test with l2 NOT var-onion, but then it can no longer connect - # to us! - l1, l2, l3 = node_factory.line_graph(3, - opts={'plugin': os.path.join(os.getcwd(), 'tests/plugins/print_htlc_onion.py')}) - - scid12 = l1.get_channel_scid(l2) - scid23 = l2.get_channel_scid(l3) - - # We need to force l3 to provide route hint from l2 (it won't normally, - # since it sees l2 as a dead end). - inv = l3.dev_invoice(amount_msat=10000, - label="test_tlv1", - description="test_tlv1", - dev_routes=[[{'id': l2.info['id'], - 'short_channel_id': scid23, - 'fee_base_msat': 1, - 'fee_proportional_millionths': 10, - 'cltv_expiry_delta': 6}]])['bolt11'] - l1.rpc.pay(inv) - - # Since L1 hasn't seen broadcast, it doesn't know L2 isn't TLV, but invoice tells it about L3 - l2.daemon.wait_for_log("Got onion.*'type': 'tlv'") - l3.daemon.wait_for_log("Got onion.*'type': 'tlv'") - - # We need 5 more blocks to announce l1->l2 channel. - mine_funding_to_announce(bitcoind, [l1, l2, l3]) - - # Make sure l1 knows about l2 - wait_for(lambda: 'alias' in l1.rpc.listnodes(l2.info['id'])['nodes'][0]) - - # Make sure l3 knows about l1->l2, so it will add route hint now. - wait_for(lambda: len(l3.rpc.listchannels(scid12)['channels']) > 0) - - # Now it should send TLV to l2. - inv = l3.rpc.invoice(10000, "test_tlv2", "test_tlv2")['bolt11'] - - l1.rpc.pay(inv) - l2.daemon.wait_for_log("Got onion.*'type': 'tlv'") - l3.daemon.wait_for_log("Got onion.*'type': 'tlv'") - - @unittest.skipIf(TEST_NETWORK != 'regtest', "Invoice is network specific") def test_pay_no_secret(node_factory, bitcoind): l1, l2 = node_factory.line_graph(2, wait_for_announce=True) From 384106e073c06a57ff32283e867cb5c133ff8764 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Mon, 2 Oct 2023 14:24:58 +1030 Subject: [PATCH 6/6] pytest: wean many tests off the assumption that listchannels shows private channels. We will be changing this, or at least deprecating it, so get our tests ready. Signed-off-by: Rusty Russell --- contrib/pyln-testing/pyln/testing/utils.py | 24 ++++++++++++------- tests/test_bookkeeper.py | 9 +++---- tests/test_cln_rs.py | 4 ++++ tests/test_closing.py | 27 +++++++++++---------- tests/test_connection.py | 5 ---- tests/test_gossip.py | 11 +++++---- tests/test_misc.py | 28 +++++++--------------- tests/test_pay.py | 19 +++++++++++---- tests/test_plugin.py | 2 ++ 9 files changed, 70 insertions(+), 59 deletions(-) diff --git a/contrib/pyln-testing/pyln/testing/utils.py b/contrib/pyln-testing/pyln/testing/utils.py index 5e9672217bb4..7b51fc3a8a1d 100644 --- a/contrib/pyln-testing/pyln/testing/utils.py +++ b/contrib/pyln-testing/pyln/testing/utils.py @@ -956,9 +956,6 @@ def fundbalancedchannel(self, remote_node, total_capacity=FUNDAMOUNT, announce=T return '{}x{}x{}'.format(self.bitcoin.rpc.getblockcount(), txnum, res['outnum']) - def getactivechannels(self): - return [c for c in self.rpc.listchannels()['channels'] if c['active']] - def db_query(self, query): return self.db.query(query) @@ -1064,8 +1061,8 @@ def has_funds_on_addr(addr): txnum, res['outnum']) if wait_for_active: - self.wait_channel_active(scid) - l2.wait_channel_active(scid) + self.wait_local_channel_active(scid) + l2.wait_local_channel_active(scid) return scid, res @@ -1113,7 +1110,16 @@ def get_channel_id(self, other): return None return channels[0]['channel_id'] + def is_local_channel_active(self, scid): + """Is the local channel @scid usable?""" + channels = self.rpc.listpeerchannels()['channels'] + return [c['state'] in ('CHANNELD_NORMAL', 'CHANNELD_AWAITING_SPLICE') for c in channels if c.get('short_channel_id') == scid] == [True] + + def wait_local_channel_active(self, scid): + wait_for(lambda: self.is_local_channel_active(scid)) + def is_channel_active(self, chanid): + """Does gossip show this channel as enabled both ways?""" channels = self.rpc.listchannels(chanid)['channels'] active = [(c['short_channel_id'], c['channel_flags']) for c in channels if c['active']] return (chanid, 0) in active and (chanid, 1) in active @@ -1122,8 +1128,8 @@ def wait_for_channel_onchain(self, peerid): txid = only_one(self.rpc.listpeerchannels(peerid)['channels'])['scratch_txid'] wait_for(lambda: txid in self.bitcoin.rpc.getrawmempool()) - def wait_channel_active(self, chanid): - wait_for(lambda: self.is_channel_active(chanid)) + def wait_channel_active(self, scid): + wait_for(lambda: self.is_channel_active(scid)) # This waits until gossipd sees channel_update in both directions # (or for local channels, at least a local announcement) @@ -1605,8 +1611,8 @@ def join_nodes(self, nodes, fundchannel=True, fundamount=FUNDAMOUNT, wait_for_an # Wait for all channels to be active (locally) for i, n in enumerate(scids): - nodes[i].wait_channel_active(scids[i]) - nodes[i + 1].wait_channel_active(scids[i]) + nodes[i].wait_local_channel_active(scids[i]) + nodes[i + 1].wait_local_channel_active(scids[i]) if not wait_for_announce: return diff --git a/tests/test_bookkeeper.py b/tests/test_bookkeeper.py index f2acb9afc3b7..9614942e863d 100644 --- a/tests/test_bookkeeper.py +++ b/tests/test_bookkeeper.py @@ -366,7 +366,7 @@ def test_bookkeeping_missed_chans_leases(node_factory, bitcoind): bitcoind.generate_block(1, wait_for_mempool=[txid]) wait_for(lambda: l1.channel_state(l2) == 'CHANNELD_NORMAL') scid = l1.get_channel_scid(l2) - l1.wait_channel_active(scid) + l1.wait_local_channel_active(scid) channel_id = first_channel_id(l1, l2) l1.pay(l2, invoice_msat) @@ -431,7 +431,7 @@ def test_bookkeeping_missed_chans_pushed(node_factory, bitcoind): bitcoind.generate_block(1, wait_for_mempool=[txid]) wait_for(lambda: l1.channel_state(l2) == 'CHANNELD_NORMAL') scid = l1.get_channel_scid(l2) - l1.wait_channel_active(scid) + l1.wait_local_channel_active(scid) channel_id = first_channel_id(l1, l2) # Send l2 funds via the channel @@ -498,7 +498,7 @@ def test_bookkeeping_missed_chans_pay_after(node_factory, bitcoind): bitcoind.generate_block(1, wait_for_mempool=[txid]) wait_for(lambda: l1.channel_state(l2) == 'CHANNELD_NORMAL') scid = l1.get_channel_scid(l2) - l1.wait_channel_active(scid) + l1.wait_local_channel_active(scid) channel_id = first_channel_id(l1, l2) # Now turn the bookkeeper on and restart @@ -519,7 +519,8 @@ def test_bookkeeping_missed_chans_pay_after(node_factory, bitcoind): assert channel_id in accts # Send a payment, should be ok. - l1.wait_channel_active(scid) + l1.rpc.connect(l2.info['id'], 'localhost', l2.port) + l1.wait_local_channel_active(scid) l1.pay(l2, invoice_msat) l1.daemon.wait_for_log(r'coin movement:.*\'invoice\'') diff --git a/tests/test_cln_rs.py b/tests/test_cln_rs.py index 96cd413cdc98..52a36f63129b 100644 --- a/tests/test_cln_rs.py +++ b/tests/test_cln_rs.py @@ -3,6 +3,7 @@ from pathlib import Path from pyln import grpc as clnpb from pyln.testing.utils import env, TEST_NETWORK, wait_for, sync_blockheight, TIMEOUT +from utils import first_scid import grpc import pytest import subprocess @@ -306,6 +307,9 @@ def test_grpc_keysend_routehint(bitcoind, node_factory): ]) ]) + # FIXME: keysend needs (unannounced) channel in gossip_store + l1.wait_channel_active(first_scid(l1, l2)) + # And now we send a keysend with that routehint list call = clnpb.KeysendRequest( destination=bytes.fromhex(l3.info['id']), diff --git a/tests/test_closing.py b/tests/test_closing.py index 67f2412f0cd4..382233e65533 100644 --- a/tests/test_closing.py +++ b/tests/test_closing.py @@ -38,9 +38,9 @@ def test_closing_simple(node_factory, bitcoind, chainparams): assert billboard == ['CHANNELD_NORMAL:Channel ready for use.'] bitcoind.generate_block(5) + l1.wait_channel_active(chan) + l2.wait_channel_active(chan) - wait_for(lambda: len(l1.getactivechannels()) == 2) - wait_for(lambda: len(l2.getactivechannels()) == 2) billboard = only_one(l1.rpc.listpeerchannels(l2.info['id'])['channels'])['status'] # This may either be from a local_update or an announce, so just # check for the substring @@ -60,9 +60,9 @@ def test_closing_simple(node_factory, bitcoind, chainparams): l1.daemon.wait_for_log('sendrawtx exit 0') l2.daemon.wait_for_log('sendrawtx exit 0') - # Both nodes should have disabled the channel in their view - wait_for(lambda: len(l1.getactivechannels()) == 0) - wait_for(lambda: len(l2.getactivechannels()) == 0) + # Both nodes should have disabled the channel in gossip + wait_for(lambda: not any([c['active'] for c in l1.rpc.listchannels()['channels']])) + wait_for(lambda: not any([c['active'] for c in l2.rpc.listchannels()['channels']])) assert bitcoind.rpc.getmempoolinfo()['size'] == 1 @@ -304,8 +304,9 @@ def test_closing_specified_destination(node_factory, bitcoind, chainparams): l1.daemon.wait_for_logs([' to CLOSINGD_SIGEXCHANGE'] * 3) - # Both nodes should have disabled the channel in their view - wait_for(lambda: len(l1.getactivechannels()) == 0) + # Both nodes should have disabled the channel in gossip + wait_for(lambda: not any([c['active'] for c in l1.rpc.listchannels()['channels']])) + wait_for(lambda: not any([c['active'] for c in l2.rpc.listchannels()['channels']])) wait_for(lambda: bitcoind.rpc.getmempoolinfo()['size'] == 3) @@ -504,12 +505,13 @@ def test_penalty_inhtlc(node_factory, bitcoind, executor, chainparams, anchors): opts]) channel_id = first_channel_id(l1, l2) + scid = first_scid(l1, l2) # Now, this will get stuck due to l1 commit being disabled.. t = executor.submit(l1.pay, l2, 100000000) - assert len(l1.getactivechannels()) == 2 - assert len(l2.getactivechannels()) == 2 + assert l1.is_local_channel_active(scid) + assert l2.is_local_channel_active(scid) # They should both have commitments blocked now. l1.daemon.wait_for_log('dev-disable-commit-after: disabling') @@ -545,7 +547,7 @@ def test_penalty_inhtlc(node_factory, bitcoind, executor, chainparams, anchors): l2.daemon.wait_for_log(' to ONCHAIN') # FIXME: l1 should try to stumble along! - wait_for(lambda: len(l2.getactivechannels()) == 0) + wait_for(lambda: l2.is_local_channel_active(scid) is False) # l2 should spend all of the outputs (except to-us). # Could happen in any order, depending on commitment tx. @@ -1593,13 +1595,14 @@ def test_penalty_rbf_normal(node_factory, bitcoind, executor, chainparams, ancho l1.rpc.connect(l2.info['id'], 'localhost', l2.port) l1.fundchannel(l2, 10**7) channel_id = first_channel_id(l1, l2) + scid = first_scid(l1, l2) # Trigger an HTLC being added. t = executor.submit(l1.pay, l2, 1000000 * 1000) # Make sure the channel is still alive. - assert len(l1.getactivechannels()) == 2 - assert len(l2.getactivechannels()) == 2 + assert l1.is_local_channel_active(scid) + assert l2.is_local_channel_active(scid) # Wait for the disconnection. l1.daemon.wait_for_log('dev-disable-commit-after: disabling') diff --git a/tests/test_connection.py b/tests/test_connection.py index 001b844b88a1..93802c289157 100644 --- a/tests/test_connection.py +++ b/tests/test_connection.py @@ -2429,11 +2429,6 @@ def test_update_fee(node_factory, bitcoind): # Make l1 send out feechange. l1.set_feerates((14000, 11000, 7500, 3750)) - # Now make sure an HTLC works. - # (First wait for route propagation.) - l1.wait_channel_active(chanid) - sync_blockheight(bitcoind, [l1, l2]) - # Make payments. l1.pay(l2, 200000000) # First payment causes fee update. diff --git a/tests/test_gossip.py b/tests/test_gossip.py index 36cf2efff9be..7f9d201820d7 100644 --- a/tests/test_gossip.py +++ b/tests/test_gossip.py @@ -449,8 +449,8 @@ def test_gossip_jsonrpc(node_factory): needle = "Received node_announcement for node" l1.daemon.wait_for_log(needle) l2.daemon.wait_for_log(needle) - wait_for(lambda: len(l1.getactivechannels()) == 2) - wait_for(lambda: len(l2.getactivechannels()) == 2) + l1.wait_channel_active(only_one(channels1)['short_channel_id']) + l2.wait_channel_active(only_one(channels1)['short_channel_id']) nodes = l1.rpc.listnodes()['nodes'] assert set([n['nodeid'] for n in nodes]) == set([l1.info['id'], l2.info['id']]) @@ -1621,6 +1621,9 @@ def setup_gossip_store_test(node_factory, bitcoind): # Create another channel, which will stay private. scid12, _ = l1.fundchannel(l2, 10**6) + # FIXME: We assume that private announcements are in gossip_store! + l2.wait_channel_active(scid12) + # Now insert channel_update for previous channel; now they're both past the # node announcements. l3.rpc.setchannel(l2.info['id'], feebase=20, feeppm=1000) @@ -2031,8 +2034,8 @@ def test_tor_port_onions(node_factory): def test_routetool(node_factory): - """Test that route tool can see unpublished channels""" - l1, l2 = node_factory.line_graph(2) + """Test that route tool can see published channels""" + l1, l2 = node_factory.line_graph(2, wait_for_announce=True) subprocess.run(['devtools/route', os.path.join(l1.daemon.lightning_dir, diff --git a/tests/test_misc.py b/tests/test_misc.py index aa758372c07c..147c5681f252 100644 --- a/tests/test_misc.py +++ b/tests/test_misc.py @@ -320,9 +320,6 @@ def test_htlc_out_timeout(node_factory, bitcoind, executor): l1.rpc.connect(l2.info['id'], 'localhost', l2.port) chanid, _ = l1.fundchannel(l2, 10**6) - # Wait for route propagation. - l1.wait_channel_active(chanid) - amt = 200000000 inv = l2.rpc.invoice(amt, 'test_htlc_out_timeout', 'desc')['bolt11'] assert only_one(l2.rpc.listinvoices('test_htlc_out_timeout')['invoices'])['status'] == 'unpaid' @@ -392,7 +389,6 @@ def test_htlc_in_timeout(node_factory, bitcoind, executor): l1.rpc.connect(l2.info['id'], 'localhost', l2.port) chanid, _ = l1.fundchannel(l2, 10**6) - l1.wait_channel_active(chanid) sync_blockheight(bitcoind, [l1, l2]) amt = 200000000 @@ -1235,12 +1231,6 @@ def test_blockchaintrack(node_factory, bitcoind): assert [o for o in l1.rpc.listfunds()['outputs'] if o['status'] != "unconfirmed"] == [] -def chan_active(node, scid, is_active): - chans = node.rpc.listchannels(scid)['channels'] - print(chans) - return [c['active'] for c in chans] == [is_active, is_active] - - @pytest.mark.openchannel('v1') def test_funding_reorg_private(node_factory, bitcoind): """Change funding tx height after lockin, between node restart. @@ -1268,8 +1258,8 @@ def test_funding_reorg_private(node_factory, bitcoind): wait_for(lambda: only_one(l1.rpc.listpeerchannels()['channels'])['status'] == ["{}_AWAITING_LOCKIN:They've confirmed channel ready, we haven't yet.".format(daemon)]) bitcoind.generate_block(1) # height 107 - l1.wait_channel_active('106x1x0') - l2.wait_channel_active('106x1x0') + l1.wait_local_channel_active('106x1x0') + l2.wait_local_channel_active('106x1x0') l1.stop() # Create a fork that changes short_channel_id from 106x1x0 to 108x1x0 @@ -1282,13 +1272,11 @@ def test_funding_reorg_private(node_factory, bitcoind): r'Got depth change .->{} for .* REORG'.format(0)]) # New one should replace old. - wait_for(lambda: chan_active(l2, '108x1x0', True)) - assert l2.rpc.listchannels('106x1x0')['channels'] == [] + wait_for(lambda: l2.is_local_channel_active('108x1x0')) + assert [c for c in l2.rpc.listpeerchannels()['channels'] if c['short_channel_id'] == '106x1x0'] == [] l1.rpc.close(l2.info['id']) bitcoind.generate_block(1, True) - l1.daemon.wait_for_log(r'Deleting channel') - l2.daemon.wait_for_log(r'Deleting channel') @pytest.mark.openchannel('v1') @@ -1307,8 +1295,8 @@ def test_funding_reorg_remote_lags(node_factory, bitcoind): l1.rpc.fundchannel(l2.info['id'], "all") bitcoind.generate_block(5) # heights 103 - 107 - l1.wait_channel_active('103x1x0') - l2.wait_channel_active('103x1x0') + l1.wait_local_channel_active('103x1x0') + l2.wait_local_channel_active('103x1x0') # Make l2 temporary blind for blocks > 107 def no_more_blocks(req): @@ -1328,8 +1316,8 @@ def no_more_blocks(req): # Unblinding l2 brings it back in sync, restarts channeld and sends its announce sig l2.daemon.rpcproxy.mock_rpc('getblockhash', None) - wait_for(lambda: chan_active(l2, '104x1x0', True)) - assert l2.rpc.listchannels('103x1x0')['channels'] == [] + wait_for(lambda: l2.is_local_channel_active('104x1x0')) + assert [c for c in l2.rpc.listpeerchannels()['channels'] if c['short_channel_id'] == '103x1x0'] == [] wait_for(lambda: only_one(l2.rpc.listpeerchannels()['channels'])['status'] == [ 'CHANNELD_NORMAL:Reconnected, and reestablished.', diff --git a/tests/test_pay.py b/tests/test_pay.py index d73a8b618bf3..2163db5d6fc7 100644 --- a/tests/test_pay.py +++ b/tests/test_pay.py @@ -430,7 +430,7 @@ def test_payment_success_persistence(node_factory, bitcoind, executor): assert len(payments) == 1 and payments[0]['status'] == 'complete' assert len(invoices) == 1 and invoices[0]['status'] == 'paid' - l1.wait_channel_active(chanid) + l1.wait_local_channel_active(chanid) # A duplicate should succeed immediately (nop) and return correct preimage. preimage = l1.dev_pay( @@ -3961,13 +3961,13 @@ def test_mpp_waitblockheight_routehint_conflict(node_factory, bitcoind, executor l1.rpc.connect(l2.info['id'], 'localhost', l2.port) l1l2, _ = l1.fundchannel(l2, 10**7, announce_channel=True) l2.rpc.connect(l3.info['id'], 'localhost', l3.port) - l2.fundchannel(l3, 10**7, announce_channel=False) + l2l3, _ = l2.fundchannel(l3, 10**7, announce_channel=False) mine_funding_to_announce(bitcoind, [l1, l2, l3]) # Wait for l3 to learn about l1->l2, otherwise it will think # l2 is a deadend and not add it to the routehint. - wait_for(lambda: len(l3.rpc.listchannels(l1l2)['channels']) >= 2) + l3.wait_channel_active(l1l2) # Now make the l1 payer stop receiving blocks. def no_more_blocks(req): @@ -3979,7 +3979,11 @@ def no_more_blocks(req): bitcoind.generate_block(2) sync_blockheight(bitcoind, [l3]) + # FIXME: routehint currently requires channels in gossip store + l3.wait_channel_active(l2l3) + inv = l3.rpc.invoice(Millisatoshi(2 * 10000 * 1000), 'i', 'i', exposeprivatechannels=True)['bolt11'] + assert 'routes' in l3.rpc.decodepay(inv) # Have l1 pay l3 def pay(l1, inv): @@ -4824,12 +4828,15 @@ def test_routehint_tous(node_factory, bitcoind): # Existence of l1 makes l3 use l2 for routehint (otherwise it sees deadend) l1, l2 = node_factory.line_graph(2, wait_for_announce=True) + scid12 = first_scid(l1, l2) l3 = node_factory.get_node() l3.rpc.connect(l2.info['id'], 'localhost', l2.port) scid23, _ = l2.fundchannel(l3, 1000000, announce_channel=False) # Make sure l3 sees l1->l2 channel. - wait_for(lambda: l3.rpc.listnodes(l1.info['id'])['nodes'] != []) + l3.wait_channel_active(scid12) + # FIXME: Routehint code currently relies on private gossip in store! + l3.wait_channel_active(scid23) inv = l3.rpc.invoice(10, "test", "test")['bolt11'] decoded = l3.rpc.decodepay(inv) assert(only_one(only_one(decoded['routes']))['short_channel_id'] @@ -5303,7 +5310,7 @@ def test_pay_routehint_minhtlc(node_factory, bitcoind): l1, l2, l3 = node_factory.line_graph(3, wait_for_announce=True) l4 = node_factory.get_node() - l3.fundchannel(l4, announce_channel=False) + scid34, _ = l3.fundchannel(l4, announce_channel=False) # l2->l3 required htlc of at least 1sat scid = only_one(l2.rpc.setchannel(l3.info['id'], htlcmin=1000)['channels'])['short_channel_id'] @@ -5314,6 +5321,8 @@ def test_pay_routehint_minhtlc(node_factory, bitcoind): # And make sure l1 knows that l2->l3 has htlcmin 1000 wait_for(lambda: l1.rpc.listchannels(scid)['channels'][0]['htlc_minimum_msat'] == Millisatoshi(1000)) + # FIXME: Routehint code currently relies on private gossip in store! + l4.wait_channel_active(scid34) inv = l4.rpc.invoice(100000, "inv", "inv") assert only_one(l1.rpc.decodepay(inv['bolt11'])['routes']) diff --git a/tests/test_plugin.py b/tests/test_plugin.py index b20836c77992..353ad6e3f3d0 100644 --- a/tests/test_plugin.py +++ b/tests/test_plugin.py @@ -2102,6 +2102,8 @@ def test_coin_movement_notices(node_factory, bitcoind, chainparams): # send a payment (originator) inv = l1.rpc.invoice(amount // 2, "second", "desc") payment_hash21 = inv['payment_hash'] + # Make sure previous completely settled + wait_for(lambda: only_one(l2.rpc.listpeerchannels(l1.info['id'])['channels'])['htlcs'] == []) route = l2.rpc.getroute(l1.info['id'], amount // 2, 1)['route'] l2.rpc.sendpay(route, payment_hash21, payment_secret=inv['payment_secret']) l2.rpc.waitsendpay(payment_hash21)