From 11dc1b341cedcf52c0de83b0cec96e05eb8d514b Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Tue, 17 Dec 2019 16:41:08 +1030 Subject: [PATCH] gossipd: hand all candidates up to lightningd to select routeboost. This lets us do more flexible filtering in the next patch. But it also keeps some weird logic out of gossipd. Signed-off-by: Rusty Russell --- gossipd/gossip_wire.csv | 9 +- gossipd/gossipd.c | 45 +++------- lightningd/invoice.c | 95 +++++++++++++++++++-- lightningd/test/run-invoice-select-inchan.c | 29 ++++--- tests/test_invoices.py | 14 ++- 5 files changed, 135 insertions(+), 57 deletions(-) diff --git a/gossipd/gossip_wire.csv b/gossipd/gossip_wire.csv index a1674d434..c7190e37c 100644 --- a/gossipd/gossip_wire.csv +++ b/gossipd/gossip_wire.csv @@ -128,12 +128,15 @@ msgdata,gossip_dev_compact_store_reply,success,bool, # master -> gossipd: get route_info for our incoming channels msgtype,gossip_get_incoming_channels,3025 -msgdata,gossip_get_incoming_channels,private_too,?bool, # gossipd -> master: here they are. msgtype,gossip_get_incoming_channels_reply,3125 -msgdata,gossip_get_incoming_channels_reply,num,u16, -msgdata,gossip_get_incoming_channels_reply,route_info,route_info,num +msgdata,gossip_get_incoming_channels_reply,num_public,u16, +msgdata,gossip_get_incoming_channels_reply,public_route_info,route_info,num_public +msgdata,gossip_get_incoming_channels_reply,public_deadends,bool,num_public +msgdata,gossip_get_incoming_channels_reply,num_private,u16, +msgdata,gossip_get_incoming_channels_reply,private_route_info,route_info,num_private +msgdata,gossip_get_incoming_channels_reply,private_deadends,bool,num_private # master -> gossipd: blockheight increased. msgtype,gossip_new_blockheight,3026 diff --git a/gossipd/gossipd.c b/gossipd/gossipd.c index 3c8d8303a..1abbecf9a 100644 --- a/gossipd/gossipd.c +++ b/gossipd/gossipd.c @@ -1188,18 +1188,6 @@ static bool node_has_public_channels(const struct node *peer, return false; } -/*~ The `exposeprivate` flag is a trinary: NULL == dynamic, otherwise - * value decides. Thus, we provide two wrappers for clarity: */ -static bool never_expose(bool *exposeprivate) -{ - return exposeprivate && !*exposeprivate; -} - -static bool always_expose(bool *exposeprivate) -{ - return exposeprivate && *exposeprivate; -} - /*~ For routeboost, we offer payers a hint of what incoming channels might * have capacity for their payment. To do this, lightningd asks for the * information about all channels to this node; but gossipd doesn't know about @@ -1211,14 +1199,12 @@ static struct io_plan *get_incoming_channels(struct io_conn *conn, struct node *node; struct route_info *public = tal_arr(tmpctx, struct route_info, 0); struct route_info *private = tal_arr(tmpctx, struct route_info, 0); - bool has_public; - bool *exposeprivate; + bool *priv_deadends = tal_arr(tmpctx, bool, 0); + bool *pub_deadends = tal_arr(tmpctx, bool, 0); - if (!fromwire_gossip_get_incoming_channels(tmpctx, msg, &exposeprivate)) + if (!fromwire_gossip_get_incoming_channels(msg)) master_badmsg(WIRE_GOSSIP_GET_INCOMING_CHANNELS, msg); - has_public = always_expose(exposeprivate); - node = get_node(daemon->rstate, &daemon->rstate->local_id); if (node) { struct chan_map_iter i; @@ -1227,6 +1213,7 @@ static struct io_plan *get_incoming_channels(struct io_conn *conn, for (c = first_chan(node, &i); c; c = next_chan(node, &i)) { const struct half_chan *hc; struct route_info ri; + bool deadend; hc = &c->half[half_chan_to(node, c)]; @@ -1239,25 +1226,21 @@ static struct io_plan *get_incoming_channels(struct io_conn *conn, ri.fee_proportional_millionths = hc->proportional_fee; ri.cltv_expiry_delta = hc->delay; - has_public |= is_chan_public(c); - - /* If peer doesn't have other public channels, - * no point giving route */ - if (!node_has_public_channels(other_node(node, c), c)) - continue; - - if (always_expose(exposeprivate) || is_chan_public(c)) + deadend = !node_has_public_channels(other_node(node, c), + c); + if (is_chan_public(c)) { tal_arr_expand(&public, ri); - else + tal_arr_expand(&pub_deadends, deadend); + } else { tal_arr_expand(&private, ri); + tal_arr_expand(&priv_deadends, deadend); + } } } - /* If no public channels (even deadend ones!), share private ones. */ - if (!has_public && !never_expose(exposeprivate)) - msg = towire_gossip_get_incoming_channels_reply(NULL, private); - else - msg = towire_gossip_get_incoming_channels_reply(NULL, public); + msg = towire_gossip_get_incoming_channels_reply(NULL, + public, pub_deadends, + private, priv_deadends); daemon_conn_send(daemon->master, take(msg)); return daemon_conn_read_next(conn, daemon->master); diff --git a/lightningd/invoice.c b/lightningd/invoice.c index 114900379..c8e9ef4ad 100644 --- a/lightningd/invoice.c +++ b/lightningd/invoice.c @@ -379,6 +379,7 @@ static struct route_info **select_inchan(const tal_t *ctx, struct lightningd *ld, struct amount_msat amount_needed, const struct route_info *inchans, + const bool *deadends, bool *any_offline) { /* BOLT11 struct wants an array of arrays (can provide multiple routes) */ @@ -413,6 +414,10 @@ static struct route_info **select_inchan(const tal_t *ctx, if (!c) continue; + /* Is it a dead-end? */ + if (deadends[i]) + continue; + /* Channel balance as seen by our node: |<----------------- capacity ----------------->| @@ -494,30 +499,88 @@ static struct route_info **select_inchan(const tal_t *ctx, } /* Encapsulating struct while we wait for gossipd to give us incoming channels */ +struct chanhints { + bool expose_all_private; + struct short_channel_id *hints; +}; + struct invoice_info { struct command *cmd; struct preimage payment_preimage; struct bolt11 *b11; struct json_escape *label; + struct chanhints *chanhints; }; +static void append_routes(struct route_info **dst, const struct route_info *src) +{ + size_t n = tal_count(*dst); + + tal_resize(dst, n + tal_count(src)); + memcpy(*dst + n, src, tal_count(src) * sizeof(*src)); +} + +static void append_bools(bool **dst, const bool *src) +{ + size_t n = tal_count(*dst); + + tal_resize(dst, n + tal_count(src)); + memcpy(*dst + n, src, tal_count(src) * sizeof(*src)); +} + +static bool all_true(const bool *barr, size_t n) +{ + for (size_t i = 0; i < n; i++) { + if (!barr[i]) + return false; + } + return true; +} + static void gossipd_incoming_channels_reply(struct subd *gossipd, const u8 *msg, const int *fs, struct invoice_info *info) { struct json_stream *response; - struct route_info *inchans; + struct route_info *inchans, *private; + bool *inchan_deadends, *private_deadends; bool any_offline; struct invoice invoice; char *b11enc; const struct invoice_details *details; struct wallet *wallet = info->cmd->ld->wallet; + const struct chanhints *chanhints = info->chanhints; - if (!fromwire_gossip_get_incoming_channels_reply(tmpctx, msg, &inchans)) + if (!fromwire_gossip_get_incoming_channels_reply(tmpctx, msg, + &inchans, + &inchan_deadends, + &private, + &private_deadends)) fatal("Gossip gave bad GOSSIP_GET_INCOMING_CHANNELS_REPLY %s", tal_hex(msg, msg)); + /* fromwire explicitly makes empty arrays into NULL */ + if (!inchans) { + inchans = tal_arr(tmpctx, struct route_info, 0); + inchan_deadends = tal_arr(tmpctx, bool, 0); + } + + if (chanhints->expose_all_private) { + append_routes(&inchans, private); + append_bools(&inchan_deadends, private_deadends); + } else if (chanhints->hints) { + /* FIXME: Implement hint support! */ + assert(!tal_count(chanhints->hints)); + } else { + /* By default, only consider private channels if there are + * no public channels *at all* */ + if (tal_count(inchans) == 0) { + append_routes(&inchans, private); + append_bools(&inchan_deadends, private_deadends); + } + } + #if DEVELOPER /* dev-routes overrides this. */ any_offline = false; @@ -528,6 +591,7 @@ static void gossipd_incoming_channels_reply(struct subd *gossipd, info->cmd->ld, info->b11->msat ? *info->b11->msat : AMOUNT_MSAT(1), inchans, + inchan_deadends, &any_offline); /* FIXME: add private routes if necessary! */ @@ -578,14 +642,19 @@ static void gossipd_incoming_channels_reply(struct subd *gossipd, any_offline ? " (among currently connected peers)" : ""); - if (any_offline) + if (tal_count(inchans) == 0) + json_add_string(response, "warning_capacity", + "No channels"); + else if (all_true(inchan_deadends, tal_count(inchans))) + json_add_string(response, "warning_deadends", + "No channel with a peer that is not a dead end"); + else if (any_offline) json_add_string(response, "warning_offline", "No channel with a peer that is currently connected" " has sufficient incoming capacity"); else json_add_string(response, "warning_capacity", - "No channel with a peer that is not a dead end," - " has sufficient incoming capacity"); + "No channel with a peer that has sufficient incoming capacity"); } was_pending(command_success(info->cmd, response)); @@ -739,6 +808,7 @@ static struct command_result *json_invoice(struct command *cmd, info = tal(cmd, struct invoice_info); info->cmd = cmd; + info->chanhints = tal(info, struct chanhints); if (!param(cmd, buffer, params, p_req("msatoshi", param_msat_or_any, &msatoshi_val), @@ -769,6 +839,17 @@ static struct command_result *json_invoice(struct command *cmd, strlen(desc_val)); } + /* Default is expose iff no public channels. */ + if (exposeprivate == NULL) { + info->chanhints->expose_all_private = false; + info->chanhints->hints = NULL; + } else { + info->chanhints->expose_all_private = *exposeprivate; + /* FIXME: Support hints! */ + info->chanhints->hints = tal_arr(info->chanhints, + struct short_channel_id, 0); + } + if (msatoshi_val && amount_msat_greater(*msatoshi_val, chainparams->max_payment)) { return command_fail(cmd, JSONRPC2_INVALID_PARAMS, @@ -829,10 +910,8 @@ static struct command_result *json_invoice(struct command *cmd, if (fallback_scripts) info->b11->fallbacks = tal_steal(info->b11, fallback_scripts); - log_debug(cmd->ld->log, "exposeprivate = %s", - exposeprivate ? (*exposeprivate ? "TRUE" : "FALSE") : "NULL"); subd_req(cmd, cmd->ld->gossip, - take(towire_gossip_get_incoming_channels(NULL, exposeprivate)), + take(towire_gossip_get_incoming_channels(NULL)), -1, 0, gossipd_incoming_channels_reply, info); return command_still_pending(cmd); diff --git a/lightningd/test/run-invoice-select-inchan.c b/lightningd/test/run-invoice-select-inchan.c index d4bd272bc..f49a19f30 100644 --- a/lightningd/test/run-invoice-select-inchan.c +++ b/lightningd/test/run-invoice-select-inchan.c @@ -107,7 +107,7 @@ bool fromwire_channel_dev_memleak_reply(const void *p UNNEEDED, bool *leak UNNEE bool fromwire_connect_peer_connected(const tal_t *ctx UNNEEDED, const void *p UNNEEDED, struct node_id *id UNNEEDED, struct wireaddr_internal *addr UNNEEDED, struct per_peer_state **pps UNNEEDED, u8 **features UNNEEDED) { fprintf(stderr, "fromwire_connect_peer_connected called!\n"); abort(); } /* Generated stub for fromwire_gossip_get_incoming_channels_reply */ -bool fromwire_gossip_get_incoming_channels_reply(const tal_t *ctx UNNEEDED, const void *p UNNEEDED, struct route_info **route_info UNNEEDED) +bool fromwire_gossip_get_incoming_channels_reply(const tal_t *ctx UNNEEDED, const void *p UNNEEDED, struct route_info **public_route_info UNNEEDED, bool **public_deadends UNNEEDED, struct route_info **private_route_info UNNEEDED, bool **private_deadends UNNEEDED) { fprintf(stderr, "fromwire_gossip_get_incoming_channels_reply called!\n"); abort(); } /* Generated stub for fromwire_hsm_get_channel_basepoints_reply */ bool fromwire_hsm_get_channel_basepoints_reply(const void *p UNNEEDED, struct basepoints *basepoints UNNEEDED, struct pubkey *funding_pubkey UNNEEDED) @@ -473,7 +473,7 @@ u8 *towire_errorfmt(const tal_t *ctx UNNEEDED, const char *fmt UNNEEDED, ...) { fprintf(stderr, "towire_errorfmt called!\n"); abort(); } /* Generated stub for towire_gossip_get_incoming_channels */ -u8 *towire_gossip_get_incoming_channels(const tal_t *ctx UNNEEDED, bool *private_too UNNEEDED) +u8 *towire_gossip_get_incoming_channels(const tal_t *ctx UNNEEDED) { fprintf(stderr, "towire_gossip_get_incoming_channels called!\n"); abort(); } /* Generated stub for towire_hsm_get_channel_basepoints */ u8 *towire_hsm_get_channel_basepoints(const tal_t *ctx UNNEEDED, const struct node_id *peerid UNNEEDED, u64 dbid UNNEEDED) @@ -686,6 +686,7 @@ int main(void) struct lightningd *ld; bool any_offline; struct route_info *inchans; + bool *deadends; struct route_info **ret; size_t n; @@ -698,51 +699,53 @@ int main(void) list_head_init(&ld->peers); inchans = tal_arr(tmpctx, struct route_info, 0); + deadends = tal_arrz(tmpctx, bool, 100); + /* 1. Nothing to choose from -> NULL result. */ - assert(select_inchan(tmpctx, ld, AMOUNT_MSAT(0), inchans, &any_offline) == NULL); + assert(select_inchan(tmpctx, ld, AMOUNT_MSAT(0), inchans, deadends, &any_offline) == NULL); assert(any_offline == false); /* 2. inchan but no corresponding peer -> NULL result. */ add_inchan(&inchans, 0); - assert(select_inchan(tmpctx, ld, AMOUNT_MSAT(0), inchans, &any_offline) == NULL); + assert(select_inchan(tmpctx, ld, AMOUNT_MSAT(0), inchans, deadends, &any_offline) == NULL); assert(any_offline == false); /* 3. inchan but its peer in awaiting lockin -> NULL result. */ add_peer(ld, 0, CHANNELD_AWAITING_LOCKIN, true); - assert(select_inchan(tmpctx, ld, AMOUNT_MSAT(0), inchans, &any_offline) == NULL); + assert(select_inchan(tmpctx, ld, AMOUNT_MSAT(0), inchans, deadends, &any_offline) == NULL); assert(any_offline == false); /* 4. connected peer but no corresponding inchan -> NULL result. */ add_peer(ld, 1, CHANNELD_NORMAL, true); - assert(select_inchan(tmpctx, ld, AMOUNT_MSAT(0), inchans, &any_offline) == NULL); + assert(select_inchan(tmpctx, ld, AMOUNT_MSAT(0), inchans, deadends, &any_offline) == NULL); assert(any_offline == false); /* 5. inchan but its peer (replaced with one) offline -> NULL result. */ list_del_from(&ld->peers, &list_tail(&ld->peers, struct peer, list)->list); add_peer(ld, 1, CHANNELD_NORMAL, false); add_inchan(&inchans, 1); - assert(select_inchan(tmpctx, ld, AMOUNT_MSAT(0), inchans, &any_offline) == NULL); + assert(select_inchan(tmpctx, ld, AMOUNT_MSAT(0), inchans, deadends, &any_offline) == NULL); assert(any_offline == true); /* 6. Finally, a correct peer! */ add_inchan(&inchans, 2); add_peer(ld, 2, CHANNELD_NORMAL, true); - ret = select_inchan(tmpctx, ld, AMOUNT_MSAT(0), inchans, &any_offline); + ret = select_inchan(tmpctx, ld, AMOUNT_MSAT(0), inchans, deadends, &any_offline); assert(tal_count(ret) == 1); assert(tal_count(ret[0]) == 1); assert(any_offline == true); /* Peer 1 is offline */ assert(route_info_eq(ret[0], &inchans[2])); /* 7. Correct peer with just enough capacity_to_pay_us */ - ret = select_inchan(tmpctx, ld, AMOUNT_MSAT(1999), inchans, &any_offline); + ret = select_inchan(tmpctx, ld, AMOUNT_MSAT(1999), inchans, deadends, &any_offline); assert(tal_count(ret) == 1); assert(tal_count(ret[0]) == 1); assert(any_offline == false); /* Other candidate insufficient funds. */ assert(route_info_eq(ret[0], &inchans[2])); /* 8. Not if we ask for too much! Our balance is 1msat. */ - ret = select_inchan(tmpctx, ld, AMOUNT_MSAT(2000), inchans, &any_offline); + ret = select_inchan(tmpctx, ld, AMOUNT_MSAT(2000), inchans, deadends, &any_offline); assert(ret == NULL); assert(any_offline == false); /* Other candidate insufficient funds. */ @@ -752,7 +755,7 @@ int main(void) /* Simulate selection ratios between excesses 25% and 50% of capacity*/ for (size_t i = n = 0; i < 1000; i++) { - ret = select_inchan(tmpctx, ld, AMOUNT_MSAT(1499), inchans, &any_offline); + ret = select_inchan(tmpctx, ld, AMOUNT_MSAT(1499), inchans, deadends, &any_offline); assert(tal_count(ret) == 1); assert(tal_count(ret[0]) == 1); assert(any_offline == false); /* Other candidate insufficient funds. */ @@ -770,11 +773,11 @@ int main(void) /* 10. Last peer's capacity goes from 3 to 2 sat*/ list_tail(&list_tail(&ld->peers, struct peer, list)->channels, struct channel, list)-> channel_info.their_config.channel_reserve = AMOUNT_SAT(1); - ret = select_inchan(tmpctx, ld, AMOUNT_MSAT(1499), inchans, &any_offline); + ret = select_inchan(tmpctx, ld, AMOUNT_MSAT(1499), inchans, deadends, &any_offline); /* Simulate selection ratios between excesses 25% and 75% of capacity*/ for (size_t i = n = 0; i < 1000; i++) { - ret = select_inchan(tmpctx, ld, AMOUNT_MSAT(1499), inchans, &any_offline); + ret = select_inchan(tmpctx, ld, AMOUNT_MSAT(1499), inchans, deadends, &any_offline); assert(tal_count(ret) == 1); assert(tal_count(ret[0]) == 1); assert(any_offline == false); /* Other candidate insufficient funds. */ diff --git a/tests/test_invoices.py b/tests/test_invoices.py index f64c40deb..4393eb81e 100644 --- a/tests/test_invoices.py +++ b/tests/test_invoices.py @@ -136,6 +136,7 @@ def test_invoice_routeboost(node_factory, bitcoind): # Check routeboost. assert 'warning_capacity' not in inv assert 'warning_offline' not in inv + assert 'warning_deadends' not in inv # Route array has single route with single element. r = only_one(only_one(l1.rpc.decodepay(inv['bolt11'])['routes'])) assert r['pubkey'] == l1.info['id'] @@ -153,6 +154,7 @@ def test_invoice_routeboost(node_factory, bitcoind): # Check warning assert 'warning_capacity' in inv assert 'warning_offline' not in inv + assert 'warning_deadends' not in inv l1.rpc.disconnect(l2.info['id'], True) wait_for(lambda: not only_one(l2.rpc.listpeers(l1.info['id'])['peers'])['connected']) @@ -160,6 +162,7 @@ def test_invoice_routeboost(node_factory, bitcoind): inv = l2.rpc.invoice(123456, label="inv3", description="?") # Check warning. assert 'warning_capacity' not in inv + assert 'warning_deadends' not in inv assert 'warning_offline' in inv # Close l0, l2 will not use l1 at all. @@ -171,7 +174,8 @@ def test_invoice_routeboost(node_factory, bitcoind): wait_for(lambda: len(l2.rpc.listchannels()['channels']) == 2) inv = l2.rpc.invoice(123456, label="inv4", description="?") # Check warning. - assert 'warning_capacity' in inv + assert 'warning_deadends' in inv + assert 'warning_capacity' not in inv assert 'warning_offline' not in inv @@ -195,6 +199,7 @@ def test_invoice_routeboost_private(node_factory, bitcoind): inv = l2.rpc.invoice(msatoshi=123456, label="inv0", description="?") assert 'warning_capacity' not in inv assert 'warning_offline' not in inv + assert 'warning_deadends' not in inv # Route array has single route with single element. r = only_one(only_one(l1.rpc.decodepay(inv['bolt11'])['routes'])) assert r['pubkey'] == l1.info['id'] @@ -206,6 +211,8 @@ def test_invoice_routeboost_private(node_factory, bitcoind): # If we explicitly say not to, it won't expose. inv = l2.rpc.invoice(msatoshi=123456, label="inv1", description="?", exposeprivatechannels=False) assert 'warning_capacity' in inv + assert 'warning_offline' not in inv + assert 'warning_deadends' not in inv assert 'routes' not in l1.rpc.decodepay(inv['bolt11']) # The existence of a public channel, even without capacity, will suppress @@ -219,12 +226,15 @@ def test_invoice_routeboost_private(node_factory, bitcoind): wait_for(lambda: [c['public'] for c in l3.rpc.listchannels(scid)['channels']] == [True, True]) inv = l2.rpc.invoice(msatoshi=10**7, label="inv2", description="?") - assert 'warning_capacity' in inv + assert 'warning_deadends' in inv + assert 'warning_capacity' not in inv + assert 'warning_offline' not in inv # Unless we tell it to include it. inv = l2.rpc.invoice(msatoshi=10**7, label="inv3", description="?", exposeprivatechannels=True) assert 'warning_capacity' not in inv assert 'warning_offline' not in inv + assert 'warning_deadends' not in inv # Route array has single route with single element. r = only_one(only_one(l1.rpc.decodepay(inv['bolt11'])['routes'])) assert r['pubkey'] == l1.info['id']