From c053dc9a6ac456a04d8dbb688fe8703411d268e4 Mon Sep 17 00:00:00 2001 From: Simon Vrouwe Date: Tue, 9 Apr 2019 18:35:28 +0300 Subject: [PATCH] lightningd: fix/refactor select_inchan for invoice route-hint, use fractional excess as weight Refactored the weighted-reservoir-sampling algo to make it more straightforward. It now uses the excess as fraction of capacity as weight. This favors channels that are more _relatively_ unbalanced to be used for incoming payment. Now passes test_invoice_routeboost_private() when using max fundamount=16777215. --- lightningd/invoice.c | 108 +++++++++++++++----- lightningd/test/run-invoice-select-inchan.c | 76 ++++++++------ tests/test_invoices.py | 2 +- 3 files changed, 128 insertions(+), 58 deletions(-) diff --git a/lightningd/invoice.c b/lightningd/invoice.c index 98f166e0e..d5a9406bb 100644 --- a/lightningd/invoice.c +++ b/lightningd/invoice.c @@ -313,27 +313,45 @@ static struct command_result *parse_fallback(struct command *cmd, return NULL; } -/* BOLT11 struct wants an array of arrays (can provide multiple routes) */ +/* + * From array of incoming channels [inchan], find suitable ones for + * a payment-to-us of [amount_needed], using criteria: + * 1. Channel's peer is known, in state CHANNELD_NORMAL and is online. + * 2. Channel's peer capacity to pay us is sufficient. + * + * Then use weighted reservoir sampling, which makes probing channel balances + * harder, to choose one channel from the set of suitable channels. It favors + * channels that have less balance on our side as fraction of their capacity. + * + * [any_offline] is set if the peer of any suitable channel appears offline. + */ static struct route_info **select_inchan(const tal_t *ctx, struct lightningd *ld, - struct amount_msat capacity_needed, + struct amount_msat amount_needed, const struct route_info *inchans, bool *any_offline) { - const struct route_info *r = NULL; - struct route_info **ret; + /* BOLT11 struct wants an array of arrays (can provide multiple routes) */ + struct route_info **R; + double wsum, p; + + struct sample { + const struct route_info *route; + double weight; + }; + + struct sample *S = tal_arr(tmpctx, struct sample, 0); *any_offline = false; - /* Weighted reservoir sampling. - * Based on https://en.wikipedia.org/wiki/Reservoir_sampling - * Algorithm A-Chao - */ - u64 wsum = 0; + /* Collect suitable channels and assign each a weight. */ for (size_t i = 0; i < tal_count(inchans); i++) { struct peer *peer; struct channel *c; - struct amount_msat avail, excess; + struct sample sample; + struct amount_msat their_msat, capacity_to_pay_us, excess, capacity; + struct amount_sat cumulative_reserve; + double excess_frac; /* Do we know about this peer? */ peer = peer_by_id(ld, &inchans[i].pubkey); @@ -345,8 +363,23 @@ static struct route_info **select_inchan(const tal_t *ctx, if (!c) continue; - /* Does it have sufficient capacity. */ - if (!amount_sat_sub_msat(&avail, c->funding, c->our_msat)) { + /* Channel balance as seen by our node: + + |<----------------- capacity ----------------->| + . . + . |<------------------ their_msat -------------------->| + . | . | + . |<----- capacity_to_pay_us ----->|<- their_reserve ->| + . | | | + . |<- amount_needed --><- excess ->| | + . | | | + |-------|-------------|--------------------------------|-------------------| + 0 ^ ^ ^ funding + our_reserve our_msat */ + + /* Does the peer have sufficient balance to pay us. */ + if (!amount_sat_sub_msat(&their_msat, c->funding, c->our_msat)) { + log_broken(ld->log, "underflow: funding %s - our_msat %s", type_to_string(tmpctx, struct amount_sat, @@ -356,12 +389,12 @@ static struct route_info **select_inchan(const tal_t *ctx, continue; } - /* Even after reserve taken into account */ - if (!amount_msat_sub_sat(&avail, - avail, c->our_config.channel_reserve)) + /* Even after taken into account their reserve */ + if (!amount_msat_sub_sat(&capacity_to_pay_us, their_msat, + c->our_config.channel_reserve)) continue; - if (!amount_msat_sub(&excess, avail, capacity_needed)) + if (!amount_msat_sub(&excess, capacity_to_pay_us, amount_needed)) continue; /* Is it offline? */ @@ -370,19 +403,44 @@ static struct route_info **select_inchan(const tal_t *ctx, continue; } - /* Avoid divide-by-zero corner case. */ - wsum += excess.millisatoshis + 1; /* Raw: rand select */ - if (pseudorand(1ULL << 32) - <= ((excess.millisatoshis + 1) << 32) / wsum) /* Raw: rand select */ - r = &inchans[i]; + /* Find capacity and calculate its excess fraction */ + if (!amount_sat_add(&cumulative_reserve, + c->our_config.channel_reserve, + c->channel_info.their_config.channel_reserve) + || !amount_sat_to_msat(&capacity, c->funding) + || !amount_msat_sub_sat(&capacity, capacity, cumulative_reserve)) { + log_broken(ld->log, "Channel %s capacity overflow!", + type_to_string(tmpctx, struct short_channel_id, c->scid)); + continue; + } + + excess_frac = (double)excess.millisatoshis / capacity.millisatoshis; /* Raw: double fraction */ + + sample.route = &inchans[i]; + sample.weight = excess_frac; + tal_arr_expand(&S, sample); } - if (!r) + if (!tal_count(S)) return NULL; - ret = tal_arr(ctx, struct route_info *, 1); - ret[0] = tal_dup(ret, struct route_info, r); - return ret; + /* Use weighted reservoir sampling, see: + * https://en.wikipedia.org/wiki/Reservoir_sampling#Algorithm_A-Chao + * But (currently) the result will consist of only one sample (k=1) */ + R = tal_arr(ctx, struct route_info *, 1); + R[0] = tal_dup(R, struct route_info, S[0].route); + wsum = S[0].weight; + + for (size_t i = 1; i < tal_count(S); i++) { + wsum += S[i].weight; + p = S[i].weight / wsum; + double random_1 = pseudorand_double(); /* range [0,1) */ + + if (random_1 <= p) + R[0] = tal_dup(R, struct route_info, S[i].route); + } + + return R; } /* Encapsulating struct while we wait for gossipd to give us incoming channels */ diff --git a/lightningd/test/run-invoice-select-inchan.c b/lightningd/test/run-invoice-select-inchan.c index 034fec7e3..0befd3343 100644 --- a/lightningd/test/run-invoice-select-inchan.c +++ b/lightningd/test/run-invoice-select-inchan.c @@ -618,6 +618,7 @@ static void add_peer(struct lightningd *ld, int n, enum channel_state state, c->funding.satoshis = n+1; c->our_msat = AMOUNT_MSAT(1); c->our_config.channel_reserve = AMOUNT_SAT(1); + c->channel_info.their_config.channel_reserve = AMOUNT_SAT(0); list_add_tail(&peer->channels, &c->list); } @@ -647,71 +648,59 @@ int main(void) list_head_init(&ld->peers); inchans = tal_arr(tmpctx, struct route_info, 0); - /* Nothing to choose from -> NULL result. */ + /* 1. Nothing to choose from -> NULL result. */ assert(select_inchan(tmpctx, ld, AMOUNT_MSAT(0), inchans, &any_offline) == NULL); assert(any_offline == false); - /* inchan but no peer -> NULL result. */ + /* 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(any_offline == false); - /* connected peer but no inchan -> NULL result. */ - add_peer(ld, 1, CHANNELD_NORMAL, false); - assert(select_inchan(tmpctx, ld, AMOUNT_MSAT(0), inchans, &any_offline) == NULL); - assert(any_offline == false); - - /* inchan but peer awaiting lockin -> NULL result. */ + /* 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(any_offline == false); - /* inchan but peer not connected -> NULL result. */ + /* 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(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(any_offline == true); - /* Finally, a correct peer! */ + /* 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); assert(tal_count(ret) == 1); assert(tal_count(ret[0]) == 1); - assert(any_offline == true); + assert(any_offline == true); /* Peer 1 is offline */ assert(route_info_eq(ret[0], &inchans[2])); - /* Not if we ask for too much! Reserve is 1 satoshi */ + /* 7. Correct peer with just enough capacity_to_pay_us */ ret = select_inchan(tmpctx, ld, AMOUNT_MSAT(1999), inchans, &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); assert(ret == NULL); assert(any_offline == false); /* Other candidate insufficient funds. */ - /* Add another candidate, with twice as much excess. */ + /* 9. Add another peer */ add_inchan(&inchans, 3); add_peer(ld, 3, CHANNELD_NORMAL, true); - for (size_t i = n = 0; i < 1000; i++) { - ret = select_inchan(tmpctx, ld, AMOUNT_MSAT(1000), inchans, &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]) - || route_info_eq(ret[0], &inchans[3])); - n += route_info_eq(ret[0], &inchans[2]); - } - - /* Handwave over probability of this happening! Within 20% */ - assert(n > 333 - 66 && n < 333 + 66); - printf("Number of selections with 999 excess: %zu\n" - "Number of selections with 1999 excess: %zu\n", - n, 1000 - n); - + /* 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); assert(tal_count(ret) == 1); @@ -722,10 +711,33 @@ int main(void) n += route_info_eq(ret[0], &inchans[2]); } - assert(n > 250 - 50 && n < 250 + 50); - printf("Number of selections with 500 excess: %zu\n" - "Number of selections with 1500 excess: %zu\n", + /* Handwave over probability of this happening! Within 20% */ + printf("Number of selections with excess 25 percent of capacity: %zu\n" + "Number of selections with excess 50 percent of capacity: %zu\n", n, 1000 - n); + assert(n > 333 - 66 && n < 333 + 66); + + /* 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); + + /* 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); + 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]) + || route_info_eq(ret[0], &inchans[3])); + n += route_info_eq(ret[0], &inchans[2]); + } + + /* Handwave over probability of this happening! Within 20% */ + printf("Number of selections with excess 25 percent of capacity: %zu\n" + "Number of selections with excess 75 percent of capacity: %zu\n", + n, 1000 - n); + assert(n > 250 - 50 && n < 250 + 50); /* No memory leaks please */ secp256k1_context_destroy(secp256k1_ctx); diff --git a/tests/test_invoices.py b/tests/test_invoices.py index 666c11334..7ba08e61b 100644 --- a/tests/test_invoices.py +++ b/tests/test_invoices.py @@ -176,7 +176,7 @@ def test_invoice_routeboost(node_factory, bitcoind): def test_invoice_routeboost_private(node_factory, bitcoind): """Test routeboost 'r' hint in bolt11 invoice for private channels """ - l1, l2 = node_factory.line_graph(2, fundamount=10**6, announce_channels=False) + l1, l2 = node_factory.line_graph(2, fundamount=16777215, announce_channels=False) # Attach public channel to l1 so it doesn't look like a dead-end. l0 = node_factory.get_node()