From acc6ad64aad4498ef19845bbc1c67fe60ba20651 Mon Sep 17 00:00:00 2001 From: Christian Decker Date: Sun, 19 Jul 2020 19:52:18 +0200 Subject: [PATCH 01/24] paymod: Count all attempts, not just the ones with a result With the presplitter in particular we would have n attempts but the array contains n+1 entries, which is kinda weird. --- plugins/libplugin-pay.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/plugins/libplugin-pay.c b/plugins/libplugin-pay.c index aa414b2f07b8..a13dcacac79a 100644 --- a/plugins/libplugin-pay.c +++ b/plugins/libplugin-pay.c @@ -91,8 +91,7 @@ struct payment_tree_result payment_collect_result(struct payment *p) struct payment_tree_result res; size_t numchildren = tal_count(p->children); res.sent = AMOUNT_MSAT(0); - /* If we didn't have a route, we didn't attempt. */ - res.attempts = p->route == NULL ? 0 : 1; + res.attempts = 1; res.treestates = p->step; res.leafstates = 0; res.preimage = NULL; From 33d60c1750174b71cbd34c2684b1fd3fd1a5ef25 Mon Sep 17 00:00:00 2001 From: Christian Decker Date: Sun, 19 Jul 2020 21:31:41 +0200 Subject: [PATCH 02/24] paymod: Add a comment about how we derive errors from erring_index Mainly to help my future self remember --- plugins/libplugin-pay.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/plugins/libplugin-pay.c b/plugins/libplugin-pay.c index a13dcacac79a..ddc83891511b 100644 --- a/plugins/libplugin-pay.c +++ b/plugins/libplugin-pay.c @@ -684,6 +684,22 @@ payment_waitsendpay_finished(struct command *cmd, const char *buffer, root = payment_root(p); payment_chanhints_apply_route(p, true); + /* Either we have no erring_index, or it must be a correct index into + * p->route. From the docs: + * + * - *erring_index*: The index of the node along the route that + * reported the error. 0 for the local node, 1 for the first hop, + * and so on. + * + * The only difficulty is mapping the erring_index to the correct hop, + * since the hop consists of the processing node, but the payload for + * the next hop. In addition there is a class of onion-related errors + * that are reported by the previous hop to the one erring, since the + * erring node couldn't read the onion in the first place. + */ + assert(p->result->erring_index == NULL || + *p->result->erring_index - 1 < tal_count(p->route)); + switch (p->result->failcode) { case WIRE_PERMANENT_CHANNEL_FAILURE: case WIRE_CHANNEL_DISABLED: From 24e9a164f92e400423c3e59f9b64e16a7e3406fd Mon Sep 17 00:00:00 2001 From: Christian Decker Date: Sun, 19 Jul 2020 21:32:52 +0200 Subject: [PATCH 03/24] paymod: Add invariant verification for constraints on shadowroute This was highlighted in #3851, so I added an assertion. After the rewrite in the next commit we would simply skip if any of the constraints were not maintained, but this serves as the canary in the coalmine, so we don't paper over. --- plugins/libplugin-pay.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/plugins/libplugin-pay.c b/plugins/libplugin-pay.c index ddc83891511b..55769bda59b8 100644 --- a/plugins/libplugin-pay.c +++ b/plugins/libplugin-pay.c @@ -1784,6 +1784,11 @@ static struct command_result *shadow_route_listchannels(struct command *cmd, const jsmntok_t *sattok, *delaytok, *basefeetok, *propfeetok, *desttok, *channelstok, *chan; + /* Check the invariants on the constraints between payment and modifier. */ + assert(d->constraints.cltv_budget <= p->constraints.cltv_budget / 4); + assert(amount_msat_greater_eq(p->constraints.fee_budget, + d->constraints.fee_budget)); + channelstok = json_get_member(buf, result, "channels"); json_for_each_arr(i, chan, channelstok) { u64 v = pseudorand(UINT64_MAX); From 68e23f4232c53390f13fec69871776787ce71826 Mon Sep 17 00:00:00 2001 From: Christian Decker Date: Sun, 19 Jul 2020 21:34:50 +0200 Subject: [PATCH 04/24] paymod: Rewrite the shadow-route constraint enforcement We now check against both constraints on the modifier and the payment before applying either. This "fixes" the assert that was causing the crash in #3851, but we are still looking for the source of the inconsistency where the modifier constraints, initialized to 1/4th of the payment, suddenly get more permissive than the payment itself. --- plugins/libplugin-pay.c | 80 +++++++++++++++++++++++++++++------------ 1 file changed, 57 insertions(+), 23 deletions(-) diff --git a/plugins/libplugin-pay.c b/plugins/libplugin-pay.c index 55769bda59b8..602d56c983c4 100644 --- a/plugins/libplugin-pay.c +++ b/plugins/libplugin-pay.c @@ -1782,7 +1782,7 @@ static struct command_result *shadow_route_listchannels(struct command *cmd, u64 sample = 0; struct amount_msat best_fee; const jsmntok_t *sattok, *delaytok, *basefeetok, *propfeetok, *desttok, - *channelstok, *chan; + *channelstok, *chan, *scidtok; /* Check the invariants on the constraints between payment and modifier. */ assert(d->constraints.cltv_budget <= p->constraints.cltv_budget / 4); @@ -1800,18 +1800,21 @@ static struct command_result *shadow_route_listchannels(struct command *cmd, delaytok = json_get_member(buf, chan, "delay"); basefeetok = json_get_member(buf, chan, "base_fee_millisatoshi"); propfeetok = json_get_member(buf, chan, "fee_per_millionth"); + scidtok = json_get_member(buf, chan, "short_channel_id"); desttok = json_get_member(buf, chan, "destination"); if (sattok == NULL || delaytok == NULL || delaytok->type != JSMN_PRIMITIVE || basefeetok == NULL || basefeetok->type != JSMN_PRIMITIVE || propfeetok == NULL || - propfeetok->type != JSMN_PRIMITIVE || desttok == NULL) + propfeetok->type != JSMN_PRIMITIVE || desttok == NULL || + scidtok == NULL) continue; json_to_u16(buf, delaytok, &curr.cltv_expiry_delta); json_to_number(buf, basefeetok, &curr.fee_base_msat); json_to_number(buf, propfeetok, &curr.fee_proportional_millionths); + json_to_short_channel_id(buf, scidtok, &curr.short_channel_id); json_to_sat(buf, sattok, &capacity); json_to_node_id(buf, desttok, &curr.pubkey); @@ -1841,33 +1844,64 @@ static struct command_result *shadow_route_listchannels(struct command *cmd, } if (best != NULL) { - bool ok; - /* Ok, we found an extension, let's add it. */ - d->destination = best->pubkey; - - /* Apply deltas to the constraints in the shadow route so we - * don't overshoot our 1/4th target. */ - if (!payment_constraints_update(&d->constraints, best_fee, - best->cltv_expiry_delta)) { + /* Check that we could apply the shadow route extension. Check + * against both the shadow route budget as well as the + * original payment's budget. */ + if (best->cltv_expiry_delta > d->constraints.cltv_budget || + best->cltv_expiry_delta > p->constraints.cltv_budget) { best = NULL; goto next; } - /* Now do the same to the payment constraints so other - * modifiers don't do it either. */ - ok = payment_constraints_update(&p->constraints, best_fee, - best->cltv_expiry_delta); - - /* And now the thing that caused all of this: adjust the call - * to getroute. */ - if (d->fuzz_amount) { - /* Only fuzz the amount to route to the destination if - * we didn't opt-out earlier. */ - ok &= amount_msat_add(&p->getroute->amount, - p->getroute->amount, best_fee); + /* Check the fee budget only if we didn't opt out, since + * testing against a virtual budget is not useful if we do not + * actually use it (it could give false positives and fail + * attempts that might have gone through, */ + if (d->fuzz_amount && + (amount_msat_greater(best_fee, d->constraints.fee_budget) || + (amount_msat_greater(best_fee, + p->constraints.fee_budget)))) { + best = NULL; + goto next; } + + /* Now we can be sure that adding the shadow route will succeed */ + plugin_log( + p->plugin, LOG_DBG, + "Adding shadow_route hop over channel %s: adding %s " + "in fees and %d CLTV delta", + type_to_string(tmpctx, struct short_channel_id, + &best->short_channel_id), + type_to_string(tmpctx, struct amount_msat, &best_fee), + best->cltv_expiry_delta); + + d->destination = best->pubkey; + d->constraints.cltv_budget -= best->cltv_expiry_delta; p->getroute->cltv += best->cltv_expiry_delta; - assert(ok); + + if (!d->fuzz_amount) + goto next; + + /* Only try to apply the fee budget changes if we want to fuzz + * the amount. Virtual fees that we then don't deliver to the + * destination could otherwise cause the route to be too + * expensive, while really being ok. If any of these fail then + * the above checks are insufficient. */ + if (!amount_msat_sub(&d->constraints.fee_budget, + d->constraints.fee_budget, best_fee) || + !amount_msat_sub(&p->constraints.fee_budget, + p->constraints.fee_budget, best_fee)) + plugin_err(p->plugin, + "Could not update fee constraints " + "for shadow route extension. " + "payment fee budget %s, modifier " + "fee budget %s, shadow fee to add %s", + type_to_string(tmpctx, struct amount_msat, + &p->constraints.fee_budget), + type_to_string(tmpctx, struct amount_msat, + &d->constraints.fee_budget), + type_to_string(tmpctx, struct amount_msat, + &best_fee)); } next: From 546c3a84a6f05e8bc2a5c0d6afc402396cd9cb9c Mon Sep 17 00:00:00 2001 From: Christian Decker Date: Sun, 19 Jul 2020 21:37:00 +0200 Subject: [PATCH 05/24] paymod: Add a log entry whenever we add a channel hint Mainly used for testing so we make sure we exclude or constrain the correct channels. Test to follow. --- plugins/libplugin-pay.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/plugins/libplugin-pay.c b/plugins/libplugin-pay.c index 602d56c983c4..ebb013198ebc 100644 --- a/plugins/libplugin-pay.c +++ b/plugins/libplugin-pay.c @@ -619,6 +619,14 @@ static void channel_hints_update(struct payment *root, hint.scid.dir = direction; hint.estimated_capacity = estimated_capacity; tal_arr_expand(&root->channel_hints, hint); + + plugin_log( + root->plugin, LOG_DBG, + "Added a channel hint for %s: enabled %s, estimated capacity %s", + type_to_string(tmpctx, struct short_channel_id_dir, &hint.scid), + hint.enabled ? "true" : "false", + type_to_string(tmpctx, struct amount_msat, + &hint.estimated_capacity)); } /* Try to infer the erring_node, erring_channel and erring_direction from what From a94ad3fc05b141bb0fd8704740cfa9c4fd011c56 Mon Sep 17 00:00:00 2001 From: Christian Decker Date: Sun, 19 Jul 2020 21:39:53 +0200 Subject: [PATCH 06/24] paymod: Consolidate step selection and changes in presplit modifier We skip most payment steps and all sub-payments, so consolidate the skip conditions in one if-statement. We also not use `payment_set_step` to skip any modifiers after us after the step change. --- plugins/libplugin-pay.c | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/plugins/libplugin-pay.c b/plugins/libplugin-pay.c index ebb013198ebc..b97ad6d9ed10 100644 --- a/plugins/libplugin-pay.c +++ b/plugins/libplugin-pay.c @@ -2192,10 +2192,7 @@ static void presplit_cb(struct presplit_mod_data *d, struct payment *p) struct payment *root = payment_root(p); struct amount_msat amt = root->amount; - if (d->disable) - return payment_continue(p); - - if (!payment_supports_mpp(p)) + if (d->disable || p->parent != NULL || !payment_supports_mpp(p)) return payment_continue(p); if (p->step == PAYMENT_STEP_ONION_PAYLOAD) { @@ -2210,7 +2207,7 @@ static void presplit_cb(struct presplit_mod_data *d, struct payment *p) fields, root->payment_secret, root->amount.millisatoshis); /* Raw: onion payload */ } - } else if (p == root && p->step == PAYMENT_STEP_INITIALIZED) { + } else if (p->step == PAYMENT_STEP_INITIALIZED) { /* The presplitter only acts on the root and only in the first * step. */ size_t count = 0; @@ -2261,8 +2258,9 @@ static void presplit_cb(struct presplit_mod_data *d, struct payment *p) payment_start(c); count++; } - p->step = PAYMENT_STEP_SPLIT; - p->end_time = time_now(); + + p->result = NULL; + p->route = NULL; p->why = tal_fmt( p, "Split into %zu sub-payments due to initial size (%s > " @@ -2270,9 +2268,8 @@ static void presplit_cb(struct presplit_mod_data *d, struct payment *p) count, type_to_string(tmpctx, struct amount_msat, &root->amount), MPP_TARGET_SIZE); + payment_set_step(p, PAYMENT_STEP_SPLIT); plugin_log(p->plugin, LOG_INFORM, "%s", p->why); - p->result = NULL; - p->route = NULL; } payment_continue(p); } From b99db9e979812bea0628e0828caca3b7b15c9cd9 Mon Sep 17 00:00:00 2001 From: Christian Decker Date: Sun, 19 Jul 2020 21:41:39 +0200 Subject: [PATCH 07/24] paymod: Do not duplicate partids When using mpp we need to always have partids>0, since we bumped the partid for the root, but not the next_id we'd end up with partid=1 being duplicated. Not a big problem since we never ended up sending the root to lightningd, instead skipping it, but it was confusing me while trying to trace sub-payment's ancestry. --- plugins/libplugin-pay.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/plugins/libplugin-pay.c b/plugins/libplugin-pay.c index b97ad6d9ed10..ef682f2d3d65 100644 --- a/plugins/libplugin-pay.c +++ b/plugins/libplugin-pay.c @@ -2217,6 +2217,12 @@ static void presplit_cb(struct presplit_mod_data *d, struct payment *p) * value. */ root->partid++; + /* Bump the next_partid as well so we don't have duplicate + * partids. Not really necessary since the root payment whose + * id could be reused will never reach the `sendonion` step, + * but makes debugging a bit easier. */ + root->next_partid++; + /* If we are already below the target size don't split it * either. */ if (amount_msat_greater(MPP_TARGET_MSAT, p->amount)) From 4af40007075ffb22e76dd1f1a1ca571dcc28ed38 Mon Sep 17 00:00:00 2001 From: Christian Decker Date: Sun, 19 Jul 2020 21:44:03 +0200 Subject: [PATCH 08/24] paymod: Fix the adaptive splitter partitioning We were using the current constraints, including any shadow route and other modifications, when computing the remainder that the second child should use. Instead we should use the `start_constraints` on the parent payment, which is a copy of `constraints` created in `payment_start` exactly for this purpose. Also added an assert for the invariant on the multiplier. --- plugins/libplugin-pay.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/plugins/libplugin-pay.c b/plugins/libplugin-pay.c index ef682f2d3d65..5d18f7223fde 100644 --- a/plugins/libplugin-pay.c +++ b/plugins/libplugin-pay.c @@ -2335,6 +2335,8 @@ static void adaptive_splitter_cb(struct presplit_mod_data *d, struct payment *p) double rand = pseudorand_double() * 0.2 + 0.9; u64 mid = p->amount.millisatoshis / 2 * rand; /* Raw: multiplication */ bool ok; + /* Use the start constraints, not the ones updated by routes and shadow-routes. */ + struct payment_constraints *pconstraints = p->start_constraints; a = payment_new(p, NULL, p, p->modifiers); b = payment_new(p, NULL, p, p->modifiers); @@ -2342,11 +2344,15 @@ static void adaptive_splitter_cb(struct presplit_mod_data *d, struct payment *p) a->amount.millisatoshis = mid; /* Raw: split. */ b->amount.millisatoshis -= mid; /* Raw: split. */ + double multiplier = (double)a->amount.millisatoshis / (double)p->amount.millisatoshis; /* Raw: msat division */ + assert(multiplier >= 0.4 && multiplier < 0.6); + /* Adjust constraints since we don't want to double our * fee allowance when we split. */ - a->constraints.fee_budget.millisatoshis *= (double)a->amount.millisatoshis / (double)p->amount.millisatoshis; /* Raw: msat division. */ + a->constraints.fee_budget.millisatoshis = pconstraints->fee_budget.millisatoshis * multiplier; /* Raw: msat multiplication. */ + ok = amount_msat_sub(&b->constraints.fee_budget, - p->constraints.fee_budget, + pconstraints->fee_budget, a->constraints.fee_budget); /* Should not fail, mid is less than 55% of original From e3918698fac2e40b5973b26f7d9074cec648fc4c Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Mon, 20 Jul 2020 14:18:56 +0930 Subject: [PATCH 09/24] pay: fix problematic sharing of shadow route data. Because listchannels responses are async, with mpp we can end up fighting over use of the parent's data giving results from incorrect final CLTVs to assertion failures like below: ``` pay: plugins/libplugin-pay.c:1964: shadow_route_listchannels: Assertion `amount_msat_greater_eq(p->constraints.fee_budget, d->constraints.fee_budget)' failed. pay: FATAL SIGNAL 6 (version v0.9.0rc2-11-g29d32e0-modded) 0x563129eb6a15 send_backtrace common/daemon.c:38 0x563129eb6abf crashdump common/daemon.c:51 0x7fe37c8b920f ??? ???:0 0x7fe37c8b918b ??? ???:0 0x7fe37c898858 ??? ???:0 0x7fe37c898728 ??? ???:0 0x7fe37c8a9f35 ??? ???:0 0x563129ea5c63 shadow_route_listchannels plugins/libplugin-pay.c:1964 0x563129e9c56a handle_rpc_reply plugins/libplugin.c:547 0x563129e9cbfa rpc_read_response_one plugins/libplugin.c:662 0x563129e9ccf0 rpc_conn_read_response plugins/libplugin.c:681 0x563129ece660 next_plan ccan/ccan/io/io.c:59 0x563129ecf245 do_plan ccan/ccan/io/io.c:407 0x563129ecf287 io_ready ccan/ccan/io/io.c:417 0x563129ed151f io_loop ccan/ccan/io/poll.c:445 0x563129e9ef03 plugin_main plugins/libplugin.c:1284 0x563129e9b099 main plugins/pay.c:2010 0x7fe37c89a0b2 ??? ???:0 0x563129e9452d ??? ???:0 0xffffffffffffffff ??? ???:0 ``` --- plugins/libplugin-pay.c | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/plugins/libplugin-pay.c b/plugins/libplugin-pay.c index 5d18f7223fde..5186632e96e8 100644 --- a/plugins/libplugin-pay.c +++ b/plugins/libplugin-pay.c @@ -1750,13 +1750,21 @@ REGISTER_PAYMENT_MODIFIER(exemptfee, struct exemptfee_data *, static struct shadow_route_data *shadow_route_init(struct payment *p) { + struct shadow_route_data *d = tal(p, struct shadow_route_data), *pd; + + /* If we're not the root we need to inherit the flags set only on the + * root payment. Since we inherit them at each step it's sufficient to + * do so from our direct parent. */ if (p->parent != NULL) { - return payment_mod_shadowroute_get_data(p->parent); + pd = payment_mod_shadowroute_get_data(p->parent); + d->fuzz_amount = pd->fuzz_amount; +#if DEVELOPER + d->use_shadow = pd->use_shadow; +#endif } else { - struct shadow_route_data *d = tal(p, struct shadow_route_data); d->fuzz_amount = true; - return d; } + return d; } /* Mutual recursion */ From fa7b2b3c236322aaee918d8bc31ae73575c063e7 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Mon, 20 Jul 2020 15:16:55 +0930 Subject: [PATCH 10/24] pytest: fix erroneous test_pay_retry result. Seems like we get *4* failures, since failing to find a route counts now? Signed-off-by: Rusty Russell --- tests/test_pay.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/test_pay.py b/tests/test_pay.py index 48c38dab6c64..ff658e9fbd8a 100644 --- a/tests/test_pay.py +++ b/tests/test_pay.py @@ -1672,8 +1672,9 @@ def listpays_nofail(b11): # It will try l1->l2->l5, which fails. # It will try l1->l2->l3->l5, which fails. # It will try l1->l2->l3->l4->l5, which fails. + # Finally, fails to find a route. inv = l5.rpc.invoice(10**8, 'test_retry2', 'test_retry2')['bolt11'] - with pytest.raises(RpcError, match=r'3 attempts'): + with pytest.raises(RpcError, match=r'4 attempts'): l1.rpc.dev_pay(inv, use_shadow=False) From 92a4f39d414cd3dee7f020bcf878f230d6ebd912 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Mon, 20 Jul 2020 15:17:55 +0930 Subject: [PATCH 11/24] pay: handle returned errors more gracefully. The code had incorrect assertions, partially because it didn't clearly distinguish errors from the final node (which, barring blockheight issues, mean a complete failre) and intermediate nodes. In particular, we can't trust the *values*, so we need to distinguish these by the *sender*. If a route is of length 2 (A, B): - erring_index == 0 means us complaining about channel A. - erring_index == 1 means A.node complaining about channel B. - erring_index == 2 means the final destination node B.node. This is particularly of note because Travis does NOT run test_pay_routeboost! Signed-off-by: Rusty Russell --- plugins/libplugin-pay.c | 341 ++++++++++++++++++++++++++++++---------- 1 file changed, 256 insertions(+), 85 deletions(-) diff --git a/plugins/libplugin-pay.c b/plugins/libplugin-pay.c index 5186632e96e8..1e88e873b191 100644 --- a/plugins/libplugin-pay.c +++ b/plugins/libplugin-pay.c @@ -592,7 +592,8 @@ static struct payment_result *tal_sendpay_result_from_json(const tal_t *ctx, } static void channel_hints_update(struct payment *root, - struct short_channel_id *scid, int direction, + const struct short_channel_id *scid, + int direction, bool enabled, struct amount_msat estimated_capacity) { @@ -660,129 +661,299 @@ static void payment_result_infer(struct route_hop *route, r->erring_direction = &route[i].direction; } -static struct command_result * -payment_waitsendpay_finished(struct command *cmd, const char *buffer, - const jsmntok_t *toks, struct payment *p) +/* If a node takes too much fee or cltv, the next one reports it. We don't + * know who to believe, but log it */ +static void report_tampering(struct payment *p, + size_t report_pos, + const char *style) { - struct payment *root; - struct route_hop *hop; - assert(p->route != NULL); - - p->end_time = time_now(); - p->result = tal_sendpay_result_from_json(p, buffer, toks); + const struct node_id *id = &p->route[report_pos].nodeid; - if (p->result == NULL) { + if (report_pos == 0) { plugin_log(p->plugin, LOG_UNUSUAL, - "Unable to parse `waitsendpay` result: %.*s", - json_tok_full_len(toks), - json_tok_full(buffer, toks)); - payment_set_step(p, PAYMENT_STEP_FAILED); - payment_continue(p); - return command_still_pending(cmd); + "Node #%zu (%s) claimed we sent them invalid %s", + report_pos + 1, + type_to_string(tmpctx, struct node_id, id), + style); + } else { + plugin_log(p->plugin, LOG_UNUSUAL, + "Node #%zu (%s) claimed #%zu (%s) sent them invalid %s", + report_pos + 1, + type_to_string(tmpctx, struct node_id, id), + report_pos, + type_to_string(tmpctx, struct node_id, + &p->route[report_pos-1].nodeid), + style); } +} - payment_result_infer(p->route, p->result); +static struct command_result * +handle_final_failure(struct command *cmd, + struct payment *p, + const struct node_id *final_id, + enum onion_type failcode) +{ + /* We use an exhaustive switch statement here so you get a compile + * warning when new ones are added, and can think about where they go */ + switch (failcode) { + case WIRE_FINAL_INCORRECT_CLTV_EXPIRY: + report_tampering(p, tal_count(p->route)-1, "cltv"); + goto error; + case WIRE_FINAL_INCORRECT_HTLC_AMOUNT: + report_tampering(p, tal_count(p->route)-1, "amount"); + goto error; - if (p->result->state == PAYMENT_COMPLETE) { - payment_set_step(p, PAYMENT_STEP_SUCCESS); - payment_continue(p); - return command_still_pending(cmd); + /* BOLT #4: + * + * A _forwarding node_ MAY, but a _final node_ MUST NOT: + *... + * - return an `invalid_onion_version` error. + *... + * - return an `invalid_onion_hmac` error. + *... + * - return an `invalid_onion_key` error. + *... + * - return a `temporary_channel_failure` error. + *... + * - return a `permanent_channel_failure` error. + *... + * - return a `required_channel_feature_missing` error. + *... + * - return an `unknown_next_peer` error. + *... + * - return an `amount_below_minimum` error. + *... + * - return a `fee_insufficient` error. + *... + * - return an `incorrect_cltv_expiry` error. + *... + * - return an `expiry_too_soon` error. + *... + * - return an `expiry_too_far` error. + *... + * - return a `channel_disabled` error. + */ + case WIRE_INVALID_ONION_VERSION: + case WIRE_INVALID_ONION_HMAC: + case WIRE_INVALID_ONION_KEY: + case WIRE_TEMPORARY_CHANNEL_FAILURE: + case WIRE_PERMANENT_CHANNEL_FAILURE: + case WIRE_REQUIRED_CHANNEL_FEATURE_MISSING: + case WIRE_UNKNOWN_NEXT_PEER: + case WIRE_AMOUNT_BELOW_MINIMUM: + case WIRE_FEE_INSUFFICIENT: + case WIRE_INCORRECT_CLTV_EXPIRY: + case WIRE_EXPIRY_TOO_FAR: + case WIRE_EXPIRY_TOO_SOON: + case WIRE_CHANNEL_DISABLED: + goto strange_error; + + case WIRE_INVALID_ONION_PAYLOAD: + case WIRE_INVALID_REALM: + case WIRE_PERMANENT_NODE_FAILURE: + case WIRE_TEMPORARY_NODE_FAILURE: + case WIRE_REQUIRED_NODE_FEATURE_MISSING: +#if EXPERIMENTAL_FEATURES + case WIRE_INVALID_ONION_BLINDING: +#endif + case WIRE_INCORRECT_OR_UNKNOWN_PAYMENT_DETAILS: + case WIRE_MPP_TIMEOUT: + goto error; } - root = payment_root(p); - payment_chanhints_apply_route(p, true); +strange_error: + plugin_log(p->plugin, LOG_UNUSUAL, + "Final node %s reported strange error code %u", + type_to_string(tmpctx, struct node_id, final_id), + failcode); + +error: + p->result->code = PAY_DESTINATION_PERM_FAIL; + payment_root(p)->abort = true; + + payment_fail(p, "%s", p->result->message); + return command_still_pending(cmd); +} + - /* Either we have no erring_index, or it must be a correct index into - * p->route. From the docs: +static struct command_result * +handle_intermediate_failure(struct command *cmd, + struct payment *p, + const struct node_id *errnode, + const struct route_hop *errchan, + enum onion_type failcode) +{ + struct payment *root = payment_root(p); + + /* We use an exhaustive switch statement here so you get a compile + * warning when new ones are added, and can think about where they go */ + switch (failcode) { + /* BOLT #4: * - * - *erring_index*: The index of the node along the route that - * reported the error. 0 for the local node, 1 for the first hop, - * and so on. - * - * The only difficulty is mapping the erring_index to the correct hop, - * since the hop consists of the processing node, but the payload for - * the next hop. In addition there is a class of onion-related errors - * that are reported by the previous hop to the one erring, since the - * erring node couldn't read the onion in the first place. + * An _intermediate hop_ MUST NOT, but the _final node_: + *... + * - MUST return an `incorrect_or_unknown_payment_details` error. + *... + * - MUST return `final_incorrect_cltv_expiry` error. + *... + * - MUST return a `final_incorrect_htlc_amount` error. */ - assert(p->result->erring_index == NULL || - *p->result->erring_index - 1 < tal_count(p->route)); + case WIRE_INCORRECT_OR_UNKNOWN_PAYMENT_DETAILS: + case WIRE_FINAL_INCORRECT_CLTV_EXPIRY: + case WIRE_FINAL_INCORRECT_HTLC_AMOUNT: + /* FIXME: Document in BOLT that intermediates must not return this! */ + case WIRE_MPP_TIMEOUT: + goto strange_error; - switch (p->result->failcode) { case WIRE_PERMANENT_CHANNEL_FAILURE: case WIRE_CHANNEL_DISABLED: case WIRE_UNKNOWN_NEXT_PEER: case WIRE_REQUIRED_CHANNEL_FEATURE_MISSING: /* All of these result in the channel being marked as disabled. */ - assert(*p->result->erring_index < tal_count(p->route)); - hop = &p->route[*p->result->erring_index]; - channel_hints_update(root, &hop->channel_id, hop->direction, + channel_hints_update(root, + &errchan->channel_id, errchan->direction, false, AMOUNT_MSAT(0)); break; - case WIRE_TEMPORARY_CHANNEL_FAILURE: + + case WIRE_TEMPORARY_CHANNEL_FAILURE: { /* These are an indication that the capacity was insufficient, * remember the amount we tried as an estimate. */ - assert(*p->result->erring_index < tal_count(p->route)); - hop = &p->route[*p->result->erring_index]; - struct amount_msat est = { - .millisatoshis = hop->amount.millisatoshis * 0.75}; /* Raw: Multiplication */ - channel_hints_update(root, &hop->channel_id, hop->direction, + struct amount_msat est = errchan->amount; + est.millisatoshis *= 0.75; /* Raw: Multiplication */ + channel_hints_update(root, + &errchan->channel_id, errchan->direction, true, est); - break; + goto error; + } + + case WIRE_INCORRECT_CLTV_EXPIRY: + report_tampering(p, errchan - p->route, "cltv"); + goto error; - case WIRE_INVALID_ONION_PAYLOAD: - case WIRE_INVALID_REALM: - case WIRE_PERMANENT_NODE_FAILURE: - case WIRE_TEMPORARY_NODE_FAILURE: - case WIRE_REQUIRED_NODE_FEATURE_MISSING: case WIRE_INVALID_ONION_VERSION: case WIRE_INVALID_ONION_HMAC: case WIRE_INVALID_ONION_KEY: + case WIRE_PERMANENT_NODE_FAILURE: + case WIRE_TEMPORARY_NODE_FAILURE: + case WIRE_REQUIRED_NODE_FEATURE_MISSING: + case WIRE_INVALID_ONION_PAYLOAD: + case WIRE_INVALID_REALM: #if EXPERIMENTAL_FEATURES case WIRE_INVALID_ONION_BLINDING: #endif - /* These are reported by the last hop, i.e., the destination of hop i-1. */ - assert(*p->result->erring_index - 1 < tal_count(p->route)); - hop = &p->route[*p->result->erring_index - 1]; - tal_arr_expand(&root->excluded_nodes, hop->nodeid); - break; - - case WIRE_INCORRECT_OR_UNKNOWN_PAYMENT_DETAILS: - p->result->code = PAY_DESTINATION_PERM_FAIL; - root->abort = true; - case WIRE_MPP_TIMEOUT: - /* These are permanent failures that should abort all of our - * attempts right away. We'll still track pending partial - * payments correctly, just not start new ones. */ - root->abort = true; - break; + tal_arr_expand(&root->excluded_nodes, *errnode); + goto error; case WIRE_AMOUNT_BELOW_MINIMUM: + case WIRE_FEE_INSUFFICIENT: case WIRE_EXPIRY_TOO_FAR: case WIRE_EXPIRY_TOO_SOON: - case WIRE_FEE_INSUFFICIENT: - case WIRE_INCORRECT_CLTV_EXPIRY: - case WIRE_FINAL_INCORRECT_CLTV_EXPIRY: - /* These are issues that are due to gossipd being out of date, - * we ignore them here, and wait for gossipd to adjust - * instead. */ - break; - case WIRE_FINAL_INCORRECT_HTLC_AMOUNT: - /* These are symptoms of intermediate hops tampering with the - * payment. */ - hop = &p->route[*p->result->erring_index]; - plugin_log( - p->plugin, LOG_UNUSUAL, - "Node %s reported an incorrect HTLC amount, this could be " - "a prior hop messing with the amounts.", - type_to_string(tmpctx, struct node_id, &hop->nodeid)); - break; + goto error; } +strange_error: + plugin_log(p->plugin, LOG_UNUSUAL, + "Intermediate node %s reported strange error code %u", + type_to_string(tmpctx, struct node_id, errnode), + failcode); + +error: payment_fail(p, "%s", p->result->message); return command_still_pending(cmd); } +/* From the docs: + * + * - *erring_index*: The index of the node along the route that + * reported the error. 0 for the local node, 1 for the first hop, + * and so on. + * + * The only difficulty is mapping the erring_index to the correct hop. + * We split into the erring node, and the error channel, since they're + * used in different contexts. NULL error_channel means it's the final + * node, whose errors are treated differently. + */ +static bool assign_blame(const struct payment *p, + const struct node_id **errnode, + const struct route_hop **errchan) +{ + int index; + + if (p->result->erring_index == NULL) + return false; + + index = *p->result->erring_index; + + /* BADONION errors are reported on behalf of the next node. */ + if (p->result->failcode & BADONION) + index++; + + /* Final node *shouldn't* report BADONION, but don't assume. */ + if (index >= tal_count(p->route)) { + *errchan = NULL; + *errnode = &p->route[tal_count(p->route) - 1].nodeid; + return true; + } + + *errchan = &p->route[index]; + if (index == 0) + *errnode = p->local_id; + else + *errnode = &p->route[index - 1].nodeid; + return true; +} + +static struct command_result * +payment_waitsendpay_finished(struct command *cmd, const char *buffer, + const jsmntok_t *toks, struct payment *p) +{ + const struct node_id *errnode; + const struct route_hop *errchan; + + assert(p->route != NULL); + + p->end_time = time_now(); + p->result = tal_sendpay_result_from_json(p, buffer, toks); + + if (p->result == NULL) { + plugin_log(p->plugin, LOG_UNUSUAL, + "Unable to parse `waitsendpay` result: %.*s", + json_tok_full_len(toks), + json_tok_full(buffer, toks)); + payment_set_step(p, PAYMENT_STEP_FAILED); + payment_continue(p); + return command_still_pending(cmd); + } + + payment_result_infer(p->route, p->result); + + if (p->result->state == PAYMENT_COMPLETE) { + payment_set_step(p, PAYMENT_STEP_SUCCESS); + payment_continue(p); + return command_still_pending(cmd); + } + + payment_chanhints_apply_route(p, true); + + if (!assign_blame(p, &errnode, &errchan)) { + plugin_log(p->plugin, LOG_UNUSUAL, + "No erring_index set in `waitsendpay` result: %.*s", + json_tok_full_len(toks), + json_tok_full(buffer, toks)); + /* FIXME: Pick a random channel to fail? */ + payment_set_step(p, PAYMENT_STEP_FAILED); + payment_continue(p); + return command_still_pending(cmd); + } + + if (!errchan) + return handle_final_failure(cmd, p, errnode, + p->result->failcode); + + return handle_intermediate_failure(cmd, p, errnode, errchan, + p->result->failcode); +} + static struct command_result *payment_sendonion_success(struct command *cmd, const char *buffer, const jsmntok_t *toks, From 09a8bfaec29d9957648e50f188aad55724859754 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Mon, 20 Jul 2020 15:19:52 +0930 Subject: [PATCH 12/24] test_penalty_htlc_tx_timeout: debugging Somehow, we occasionally set the wrong amount field? Doesn't happen all the time, but when it does: b'2020-07-20T05:40:15.510Z DEBUG lightningd: Attept to pay 528691fdf7baf0043ef2305e504988a5ae510dcb6127b463b3103b40c2b82a87 with amount 10839569msat < 500000000msat' b'2020-07-20T05:40:15.510Z DEBUG lightningd: WIRE_INCORRECT_OR_UNKNOWN_PAYMENT_DETAILS: lightningd/htlc_set.c:113' b'2020-07-20T05:40:15.510Z DEBUG 035d2b1192dfba134e10e540875d366ebc8bc353d5aa766b80c090b39c3a5d885d-chan#1: HTLC in 5 RCVD_ADD_ACK_REVOCATION->SENT_REMOVE_HTLC' b'2020-07-20T05:40:15.513Z DEBUG 035d2b1192dfba134e10e540875d366ebc8bc353d5aa766b80c090b39c3a5d885d-channeld-chan#1: peer_in WIRE_UPDATE_ADD_HTLC' b'2020-07-20T05:40:15.514Z DEBUG lightningd: Attept to pay 528691fdf7baf0043ef2305e504988a5ae510dcb6127b463b3103b40c2b82a87 with amount 9710103msat < 500000000msat' b'2020-07-20T05:40:15.514Z DEBUG lightningd: WIRE_INCORRECT_OR_UNKNOWN_PAYMENT_DETAILS: lightningd/htlc_set.c:113' b'2020-07-20T05:40:15.514Z DEBUG 035d2b1192dfba134e10e540875d366ebc8bc353d5aa766b80c090b39c3a5d885d-chan#1: HTLC in 10 RCVD_ADD_ACK_REVOCATION->SENT_REMOVE_HTLC' b'2020-07-20T05:40:15.518Z DEBUG lightningd: Attept to pay 528691fdf7baf0043ef2305e504988a5ae510dcb6127b463b3103b40c2b82a87 with amount 10915092msat < 500000000msat' b'2020-07-20T05:40:15.518Z DEBUG lightningd: WIRE_INCORRECT_OR_UNKNOWN_PAYMENT_DETAILS: lightningd/htlc_set.c:113' b'2020-07-20T05:40:15.518Z DEBUG 035d2b1192dfba134e10e540875d366ebc8bc353d5aa766b80c090b39c3a5d885d-chan#1: HTLC in 0 RCVD_ADD_ACK_REVOCATION->SENT_REMOVE_HTLC' b'2020-07-20T05:40:15.521Z DEBUG lightningd: Attept to pay 528691fdf7baf0043ef2305e504988a5ae510dcb6127b463b3103b40c2b82a87 with amount 9143652msat < 500000000msat' b'2020-07-20T05:40:15.521Z DEBUG lightningd: WIRE_INCORRECT_OR_UNKNOWN_PAYMENT_DETAILS: lightningd/htlc_set.c:113' b'2020-07-20T05:40:15.521Z DEBUG 035d2b1192dfba134e10e540875d366ebc8bc353d5aa766b80c090b39c3a5d885d-chan#1: HTLC in 3 RCVD_ADD_ACK_REVOCATION->SENT_REMOVE_HTLC' b'2020-07-20T05:40:15.524Z DEBUG lightningd: Attept to pay 528691fdf7baf0043ef2305e504988a5ae510dcb6127b463b3103b40c2b82a87 with amount 9840417msat < 500000000msat' b'2020-07-20T05:40:15.524Z DEBUG lightningd: WIRE_INCORRECT_OR_UNKNOWN_PAYMENT_DETAILS: lightningd/htlc_set.c:113' b'2020-07-20T05:40:15.524Z DEBUG 035d2b1192dfba134e10e540875d366ebc8bc353d5aa766b80c090b39c3a5d885d-chan#1: HTLC in 6 RCVD_ADD_ACK_REVOCATION->SENT_REMOVE_HTLC' b'2020-07-20T05:40:15.527Z DEBUG lightningd: Attept to pay 528691fdf7baf0043ef2305e504988a5ae510dcb6127b463b3103b40c2b82a87 with amount 10524535msat < 500000000msat' b'2020-07-20T05:40:15.527Z DEBUG lightningd: WIRE_INCORRECT_OR_UNKNOWN_PAYMENT_DETAILS: lightningd/htlc_set.c:113' b'2020-07-20T05:40:15.527Z DEBUG 035d2b1192dfba134e10e540875d366ebc8bc353d5aa766b80c090b39c3a5d885d-chan#1: HTLC in 8 RCVD_ADD_ACK_REVOCATION->SENT_REMOVE_HTLC' b'2020-07-20T05:40:15.536Z DEBUG lightningd: Attept to pay 528691fdf7baf0043ef2305e504988a5ae510dcb6127b463b3103b40c2b82a87 with amount 9579583msat < 500000000msat' b'2020-07-20T05:40:15.536Z DEBUG lightningd: WIRE_INCORRECT_OR_UNKNOWN_PAYMENT_DETAILS: lightningd/htlc_set.c:113' b'2020-07-20T05:40:15.536Z DEBUG 035d2b1192dfba134e10e540875d366ebc8bc353d5aa766b80c090b39c3a5d885d-chan#1: HTLC in 1 RCVD_ADD_ACK_REVOCATION->SENT_REMOVE_HTLC' b'2020-07-20T05:40:15.541Z DEBUG lightningd: Attept to pay 528691fdf7baf0043ef2305e504988a5ae510dcb6127b463b3103b40c2b82a87 with amount 9048144msat < 500000000msat' b'2020-07-20T05:40:15.541Z DEBUG lightningd: WIRE_INCORRECT_OR_UNKNOWN_PAYMENT_DETAILS: lightningd/htlc_set.c:113' b'2020-07-20T05:40:15.541Z DEBUG 035d2b1192dfba134e10e540875d366ebc8bc353d5aa766b80c090b39c3a5d885d-chan#1: HTLC in 7 RCVD_ADD_ACK_REVOCATION->SENT_REMOVE_HTLC' b'2020-07-20T05:40:15.544Z DEBUG lightningd: Attept to pay 528691fdf7baf0043ef2305e504988a5ae510dcb6127b463b3103b40c2b82a87 with amount 10858167msat < 500000000msat' b'2020-07-20T05:40:15.544Z DEBUG lightningd: WIRE_INCORRECT_OR_UNKNOWN_PAYMENT_DETAILS: lightningd/htlc_set.c:113' b'2020-07-20T05:40:15.544Z DEBUG 035d2b1192dfba134e10e540875d366ebc8bc353d5aa766b80c090b39c3a5d885d-chan#1: HTLC in 9 RCVD_ADD_ACK_REVOCATION->SENT_REMOVE_HTLC' b'2020-07-20T05:40:15.548Z DEBUG lightningd: Attept to pay 528691fdf7baf0043ef2305e504988a5ae510dcb6127b463b3103b40c2b82a87 with amount 10137155msat < 500000000msat' b'2020-07-20T05:40:15.548Z DEBUG lightningd: WIRE_INCORRECT_OR_UNKNOWN_PAYMENT_DETAILS: lightningd/htlc_set.c:113' b'2020-07-20T05:40:15.548Z DEBUG 035d2b1192dfba134e10e540875d366ebc8bc353d5aa766b80c090b39c3a5d885d-chan#1: HTLC in 2 RCVD_ADD_ACK_REVOCATION->SENT_REMOVE_HTLC' b'2020-07-20T05:40:15.551Z DEBUG lightningd: Attept to pay 528691fdf7baf0043ef2305e504988a5ae510dcb6127b463b3103b40c2b82a87 with amount 10002298msat < 500000000msat' b'2020-07-20T05:40:15.551Z DEBUG lightningd: WIRE_INCORRECT_OR_UNKNOWN_PAYMENT_DETAILS: lightningd/htlc_set.c:113' b'2020-07-20T05:40:15.551Z DEBUG 035d2b1192dfba134e10e540875d366ebc8bc353d5aa766b80c090b39c3a5d885d-chan#1: HTLC in 4 RCVD_ADD_ACK_REVOCATION->SENT_REMOVE_HTLC' b'2020-07-20T05:40:15.554Z DEBUG 035d2b1192dfba134e10e540875d366ebc8bc353d5aa766b80c090b39c3a5d885d-channeld-chan#1: NEW:: HTLC REMOTE 14 = RCVD_ADD_HTLC/SENT_ADD_HTLC' b'2020-07-20T05:40:15.554Z DEBUG 035d2b1192dfba134e10e540875d366ebc8bc353d5aa766b80c090b39c3a5d885d-channeld-chan#1: peer_in WIRE_UPDATE_ADD_HTLC' b'2020-07-20T05:40:15.554Z DEBUG 035d2b1192dfba134e10e540875d366ebc8bc353d5aa766b80c090b39c3a5d885d-channeld-chan#1: NEW:: HTLC REMOTE 15 = RCVD_ADD_HTLC/SENT_ADD_HTLC' b'2020-07-20T05:40:15.554Z DEBUG 035d2b1192dfba134e10e540875d366ebc8bc353d5aa766b80c090b39c3a5d885d-channeld-chan#1: peer_in WIRE_UPDATE_ADD_HTLC' b'2020-07-20T05:40:15.554Z DEBUG 035d2b1192dfba134e10e540875d366ebc8bc353d5aa766b80c090b39c3a5d885d-channeld-chan#1: NEW:: HTLC REMOTE 16 = RCVD_ADD_HTLC/SENT_ADD_HTLC' --- lightningd/invoice.c | 22 +++++++++++++++++++-- lightningd/peer_htlcs.c | 9 ++++++--- lightningd/peer_htlcs.h | 10 +++++++--- lightningd/test/run-invoice-select-inchan.c | 11 ++++++----- 4 files changed, 39 insertions(+), 13 deletions(-) diff --git a/lightningd/invoice.c b/lightningd/invoice.c index 866f054f6fac..87ef0e31d196 100644 --- a/lightningd/invoice.c +++ b/lightningd/invoice.c @@ -302,8 +302,15 @@ invoice_check_payment(const tal_t *ctx, * - MUST fail the HTLC. * - MUST return an `incorrect_or_unknown_payment_details` error. */ - if (!wallet_invoice_find_unpaid(ld->wallet, &invoice, payment_hash)) + if (!wallet_invoice_find_unpaid(ld->wallet, &invoice, payment_hash)) { + log_debug(ld->log, "Unknown paid invoice %s", + type_to_string(tmpctx, struct sha256, payment_hash)); + if (wallet_invoice_find_by_rhash(ld->wallet, &invoice, payment_hash)) { + log_debug(ld->log, "ALREADY paid invoice %s", + type_to_string(tmpctx, struct sha256, payment_hash)); + } return NULL; + } details = wallet_invoice_details(ctx, ld->wallet, invoice); @@ -342,11 +349,22 @@ invoice_check_payment(const tal_t *ctx, if (details->msat != NULL) { struct amount_msat twice; - if (amount_msat_less(msat, *details->msat)) + if (amount_msat_less(msat, *details->msat)) { + log_debug(ld->log, "Attept to pay %s with amount %s < %s", + type_to_string(tmpctx, struct sha256, + &details->rhash), + type_to_string(tmpctx, struct amount_msat, &msat), + type_to_string(tmpctx, struct amount_msat, details->msat)); return tal_free(details); + } if (amount_msat_add(&twice, *details->msat, *details->msat) && amount_msat_greater(msat, twice)) { + log_debug(ld->log, "Attept to pay %s with amount %s > %s", + type_to_string(tmpctx, struct sha256, + &details->rhash), + type_to_string(tmpctx, struct amount_msat, details->msat), + type_to_string(tmpctx, struct amount_msat, &twice)); /* BOLT #4: * * - if the amount paid is more than twice the amount diff --git a/lightningd/peer_htlcs.c b/lightningd/peer_htlcs.c index 641a325b2c8e..3d6a0eead17c 100644 --- a/lightningd/peer_htlcs.c +++ b/lightningd/peer_htlcs.c @@ -283,10 +283,13 @@ void local_fail_in_htlc_needs_update(struct htlc_in *hin, } /* Helper to create (common) WIRE_INCORRECT_OR_UNKNOWN_PAYMENT_DETAILS */ -const u8 *failmsg_incorrect_or_unknown(const tal_t *ctx, - struct lightningd *ld, - const struct htlc_in *hin) +const u8 *failmsg_incorrect_or_unknown_(const tal_t *ctx, + struct lightningd *ld, + const struct htlc_in *hin, + const char *file, int line) { + log_debug(ld->log, "WIRE_INCORRECT_OR_UNKNOWN_PAYMENT_DETAILS: %s:%u", + file, line); return towire_incorrect_or_unknown_payment_details( ctx, hin->msat, get_block_height(ld->topology)); diff --git a/lightningd/peer_htlcs.h b/lightningd/peer_htlcs.h index c24f76555643..6d786c510a74 100644 --- a/lightningd/peer_htlcs.h +++ b/lightningd/peer_htlcs.h @@ -81,7 +81,11 @@ void json_format_forwarding_object(struct json_stream *response, const char *fie const struct forwarding *cur); /* Helper to create (common) WIRE_INCORRECT_OR_UNKNOWN_PAYMENT_DETAILS */ -const u8 *failmsg_incorrect_or_unknown(const tal_t *ctx, - struct lightningd *ld, - const struct htlc_in *hin); +#define failmsg_incorrect_or_unknown(ctx, ld, hin) \ + failmsg_incorrect_or_unknown_((ctx), (ld), (hin), __FILE__, __LINE__) + +const u8 *failmsg_incorrect_or_unknown_(const tal_t *ctx, + struct lightningd *ld, + const struct htlc_in *hin, + const char *file, int line); #endif /* LIGHTNING_LIGHTNINGD_PEER_HTLCS_H */ diff --git a/lightningd/test/run-invoice-select-inchan.c b/lightningd/test/run-invoice-select-inchan.c index e7d426b0c0d5..1d8e4ba9d395 100644 --- a/lightningd/test/run-invoice-select-inchan.c +++ b/lightningd/test/run-invoice-select-inchan.c @@ -88,11 +88,12 @@ char *encode_scriptpubkey_to_addr(const tal_t *ctx UNNEEDED, const struct chainparams *chainparams UNNEEDED, const u8 *scriptPubkey UNNEEDED) { fprintf(stderr, "encode_scriptpubkey_to_addr called!\n"); abort(); } -/* Generated stub for failmsg_incorrect_or_unknown */ -const u8 *failmsg_incorrect_or_unknown(const tal_t *ctx UNNEEDED, - struct lightningd *ld UNNEEDED, - const struct htlc_in *hin UNNEEDED) -{ fprintf(stderr, "failmsg_incorrect_or_unknown called!\n"); abort(); } +/* Generated stub for failmsg_incorrect_or_unknown_ */ +const u8 *failmsg_incorrect_or_unknown_(const tal_t *ctx UNNEEDED, + struct lightningd *ld UNNEEDED, + const struct htlc_in *hin UNNEEDED, + const char *file UNNEEDED, int line UNNEEDED) +{ fprintf(stderr, "failmsg_incorrect_or_unknown_ called!\n"); abort(); } /* Generated stub for fatal */ void fatal(const char *fmt UNNEEDED, ...) { fprintf(stderr, "fatal called!\n"); abort(); } From 243b5dbc7b591714adc6129406893e787e90fc98 Mon Sep 17 00:00:00 2001 From: ZmnSCPxj jxPCSnmZ Date: Sun, 19 Jul 2020 23:06:52 +0800 Subject: [PATCH 13/24] tests/test_pay.py: Replicate #3855. --- tests/test_pay.py | 38 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 38 insertions(+) diff --git a/tests/test_pay.py b/tests/test_pay.py index ff658e9fbd8a..5fe7762b97ee 100644 --- a/tests/test_pay.py +++ b/tests/test_pay.py @@ -3155,3 +3155,41 @@ def test_mpp_adaptive(node_factory, bitcoind): from pprint import pprint pprint(p) pprint(l1.rpc.paystatus(inv)) + + +@pytest.mark.xfail(strict=True) +def test_pay_fail_unconfirmed_channel(node_factory, bitcoind): + ''' + Replicate #3855. + `pay` crash when any direct channel is still + unconfirmed. + ''' + l1, l2 = node_factory.get_nodes(2) + + amount_sat = 10 ** 6 + + # create l2->l1 channel. + l2.fundwallet(amount_sat * 5) + l1.rpc.connect(l2.info['id'], 'localhost', l2.port) + l2.rpc.fundchannel(l1.info['id'], amount_sat * 3) + # channel is still unconfirmed. + + # Attempt to pay from l1 to l2. + # This should fail since the channel capacities are wrong. + invl2 = l2.rpc.invoice(Millisatoshi(amount_sat * 1000), 'i', 'i')['bolt11'] + with pytest.raises(RpcError): + l1.rpc.pay(invl2) + + # Let the channel confirm. + bitcoind.generate_block(6) + sync_blockheight(bitcoind, [l1, l2]) + + # Now give enough capacity so l1 can pay. + invl1 = l1.rpc.invoice(Millisatoshi(amount_sat * 2 * 1000), 'j', 'j')['bolt11'] + l2.rpc.pay(invl1) + + # Wait for us to recognize that the channel is available + wait_for(lambda: l1.rpc.listpeers()['peers'][0]['channels'][0]['spendable_msat'].millisatoshis > amount_sat * 1000) + + # Now l1 can pay to l2. + l1.rpc.pay(invl2) From 60c61c6c85e54626f071e78afab2bcd3248ec8ad Mon Sep 17 00:00:00 2001 From: Vincent Date: Sun, 19 Jul 2020 17:09:24 +0200 Subject: [PATCH 14/24] Fixed assertion in pay plugin This PR includes the fix discussed on PR #3855. This fix was tested with the use case described inside the issue and worked. Fixes: #3855 Changelog-None --- plugins/libplugin-pay.c | 3 ++- tests/test_pay.py | 1 - 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/plugins/libplugin-pay.c b/plugins/libplugin-pay.c index 1e88e873b191..3b38378a60eb 100644 --- a/plugins/libplugin-pay.c +++ b/plugins/libplugin-pay.c @@ -1622,7 +1622,8 @@ local_channel_hints_listpeers(struct command *cmd, const char *buffer, spendsats = json_get_member(buffer, channel, "spendable_msat"); scid = json_get_member(buffer, channel, "short_channel_id"); dir = json_get_member(buffer, channel, "direction"); - assert(spendsats != NULL && scid != NULL && dir != NULL); + if(spendsats == NULL || scid == NULL || dir == NULL) + continue; json_to_bool(buffer, connected, &h.enabled); json_to_short_channel_id(buffer, scid, &h.scid.scid); diff --git a/tests/test_pay.py b/tests/test_pay.py index 5fe7762b97ee..e41afdfff96b 100644 --- a/tests/test_pay.py +++ b/tests/test_pay.py @@ -3157,7 +3157,6 @@ def test_mpp_adaptive(node_factory, bitcoind): pprint(l1.rpc.paystatus(inv)) -@pytest.mark.xfail(strict=True) def test_pay_fail_unconfirmed_channel(node_factory, bitcoind): ''' Replicate #3855. From 25aff8166d493ab1e3bc06423418e9879c5ce85d Mon Sep 17 00:00:00 2001 From: Christian Decker Date: Mon, 20 Jul 2020 14:39:32 +0200 Subject: [PATCH 15/24] plugin: Do not automatically initialize the RPC connection in bcli Changelog-Fixed: plugin: `bcli` no longer logs a harmless warning about being unable to connect to the JSON-RPC interface. Changelog-Added: plugin: Plugins can opt out of having an RPC connection automatically initialized on startup. --- plugins/autoclean.c | 2 +- plugins/bcli.c | 3 +- plugins/fundchannel.c | 2 +- plugins/keysend.c | 2 +- plugins/libplugin.c | 71 +++++++++++++++++++++------------- plugins/libplugin.h | 1 + plugins/pay.c | 2 +- tests/plugins/test_libplugin.c | 3 +- 8 files changed, 54 insertions(+), 32 deletions(-) diff --git a/plugins/autoclean.c b/plugins/autoclean.c index acdab496ea0a..eecdd52f719d 100644 --- a/plugins/autoclean.c +++ b/plugins/autoclean.c @@ -88,7 +88,7 @@ static const struct plugin_command commands[] = { { int main(int argc, char *argv[]) { setup_locale(); - plugin_main(argv, init, PLUGIN_STATIC, NULL, commands, ARRAY_SIZE(commands), + plugin_main(argv, init, PLUGIN_STATIC, true, NULL, commands, ARRAY_SIZE(commands), NULL, 0, NULL, 0, plugin_option("autocleaninvoice-cycle", "string", diff --git a/plugins/bcli.c b/plugins/bcli.c index e3936a359558..2aae8acc2d0f 100644 --- a/plugins/bcli.c +++ b/plugins/bcli.c @@ -961,7 +961,8 @@ int main(int argc, char *argv[]) /* Initialize our global context object here to handle startup options. */ bitcoind = new_bitcoind(NULL); - plugin_main(argv, init, PLUGIN_STATIC, NULL, commands, ARRAY_SIZE(commands), + plugin_main(argv, init, PLUGIN_STATIC, false /* Do not init RPC on startup*/, + NULL, commands, ARRAY_SIZE(commands), NULL, 0, NULL, 0, plugin_option("bitcoin-datadir", "string", diff --git a/plugins/fundchannel.c b/plugins/fundchannel.c index 7caf703880a6..06b73308859d 100644 --- a/plugins/fundchannel.c +++ b/plugins/fundchannel.c @@ -446,6 +446,6 @@ static const struct plugin_command commands[] = { { int main(int argc, char *argv[]) { setup_locale(); - plugin_main(argv, init, PLUGIN_RESTARTABLE, NULL, commands, + plugin_main(argv, init, PLUGIN_RESTARTABLE, true, NULL, commands, ARRAY_SIZE(commands), NULL, 0, NULL, 0, NULL); } diff --git a/plugins/keysend.c b/plugins/keysend.c index 8d664caab080..8dc5a419b9b2 100644 --- a/plugins/keysend.c +++ b/plugins/keysend.c @@ -358,7 +358,7 @@ int main(int argc, char *argv[]) features.bits[i] = tal_arr(NULL, u8, 0); set_feature_bit(&features.bits[NODE_ANNOUNCE_FEATURE], KEYSEND_FEATUREBIT); - plugin_main(argv, init, PLUGIN_STATIC, &features, commands, + plugin_main(argv, init, PLUGIN_STATIC, true, &features, commands, ARRAY_SIZE(commands), NULL, 0, hooks, ARRAY_SIZE(hooks), NULL); } diff --git a/plugins/libplugin.c b/plugins/libplugin.c index c5c5500a66dd..e9c9cd12448a 100644 --- a/plugins/libplugin.c +++ b/plugins/libplugin.c @@ -83,6 +83,10 @@ struct plugin { /* Feature set for lightningd */ struct feature_set *our_features; + + /* Location of the RPC filename in case we need to defer RPC + * initialization or need to recover from a disconnect. */ + const char *rpc_location; }; /* command_result is mainly used as a compile-time check to encourage you @@ -763,7 +767,7 @@ static struct command_result *handle_init(struct command *cmd, char *dir, *network; struct json_out *param_obj; struct plugin *p = cmd->plugin; - bool with_rpc = true; + bool with_rpc = p->rpc_conn != NULL; configtok = json_delve(buf, params, ".configuration"); @@ -780,27 +784,39 @@ static struct command_result *handle_init(struct command *cmd, fsettok = json_delve(buf, configtok, ".feature_set"); p->our_features = json_to_feature_set(p, buf, fsettok); + /* Only attempt to connect if the plugin has configured the rpc_conn + * already, if that's not the case we were told to run without an RPC + * connection, so don't even log an error. */ rpctok = json_delve(buf, configtok, ".rpc-file"); - p->rpc_conn->fd = socket(AF_UNIX, SOCK_STREAM, 0); - if (rpctok->end - rpctok->start + 1 > sizeof(addr.sun_path)) - plugin_err(p, "rpc filename '%.*s' too long", - rpctok->end - rpctok->start, - buf + rpctok->start); - memcpy(addr.sun_path, buf + rpctok->start, rpctok->end - rpctok->start); - addr.sun_path[rpctok->end - rpctok->start] = '\0'; - addr.sun_family = AF_UNIX; - - if (connect(p->rpc_conn->fd, (struct sockaddr *)&addr, sizeof(addr)) != 0) { - with_rpc = false; - plugin_log(p, LOG_UNUSUAL, "Could not connect to '%.*s': %s", - rpctok->end - rpctok->start, buf + rpctok->start, - strerror(errno)); - } else { + p->rpc_location = json_strdup(p, buf, rpctok); + /* FIXME: Move this to its own function so we can initialize at a + * later point in time. */ + if (p->rpc_conn != NULL) { + p->rpc_conn->fd = socket(AF_UNIX, SOCK_STREAM, 0); + if (strlen(p->rpc_location) + 1 > sizeof(addr.sun_path)) + plugin_err(p, "rpc filename '%s' too long", + p->rpc_location); + memcpy(addr.sun_path, buf + rpctok->start, + rpctok->end - rpctok->start); + addr.sun_path[rpctok->end - rpctok->start] = '\0'; + addr.sun_family = AF_UNIX; + + if (connect(p->rpc_conn->fd, (struct sockaddr *)&addr, + sizeof(addr)) != 0) { + with_rpc = false; + plugin_log(p, LOG_UNUSUAL, + "Could not connect to '%s': %s", + p->rpc_location, strerror(errno)); + } + + membuf_init(&p->rpc_conn->mb, tal_arr(p, char, READ_CHUNKSIZE), + READ_CHUNKSIZE, membuf_tal_realloc); + param_obj = json_out_obj(NULL, "config", "allow-deprecated-apis"); - deprecated_apis = streq(rpc_delve(tmpctx, p, "listconfigs", - take(param_obj), - ".allow-deprecated-apis"), - "true"); + deprecated_apis = + streq(rpc_delve(tmpctx, p, "listconfigs", take(param_obj), + ".allow-deprecated-apis"), + "true"); } opttok = json_get_member(buf, params, "options"); @@ -1182,6 +1198,7 @@ static struct plugin *new_plugin(const tal_t *ctx, void (*init)(struct plugin *p, const char *buf, const jsmntok_t *), const enum plugin_restartability restartability, + bool init_rpc, struct feature_set *features, const struct plugin_command *commands, size_t num_commands, @@ -1207,11 +1224,12 @@ static struct plugin *new_plugin(const tal_t *ctx, uintmap_init(&p->out_reqs); p->our_features = features; - /* Sync RPC FIXME: maybe go full async ? */ - p->rpc_conn = tal(p, struct rpc_conn); - membuf_init(&p->rpc_conn->mb, - tal_arr(p, char, READ_CHUNKSIZE), READ_CHUNKSIZE, - membuf_tal_realloc); + if (init_rpc) { + /* Sync RPC FIXME: maybe go full async ? */ + p->rpc_conn = tal(p, struct rpc_conn); + } else { + p->rpc_conn = NULL; + } p->init = init; p->manifested = p->initialized = false; @@ -1244,6 +1262,7 @@ void plugin_main(char *argv[], void (*init)(struct plugin *p, const char *buf, const jsmntok_t *), const enum plugin_restartability restartability, + bool init_rpc, struct feature_set *features, const struct plugin_command *commands, size_t num_commands, @@ -1264,7 +1283,7 @@ void plugin_main(char *argv[], daemon_setup(argv[0], NULL, NULL); va_start(ap, num_hook_subs); - plugin = new_plugin(NULL, init, restartability, features, commands, + plugin = new_plugin(NULL, init, restartability, init_rpc, features, commands, num_commands, notif_subs, num_notif_subs, hook_subs, num_hook_subs, ap); va_end(ap); diff --git a/plugins/libplugin.h b/plugins/libplugin.h index bc1e2ac21b4b..87ec13e1c970 100644 --- a/plugins/libplugin.h +++ b/plugins/libplugin.h @@ -248,6 +248,7 @@ void NORETURN LAST_ARG_NULL plugin_main(char *argv[], void (*init)(struct plugin *p, const char *buf, const jsmntok_t *), const enum plugin_restartability restartability, + bool init_rpc, struct feature_set *features, const struct plugin_command *commands, size_t num_commands, diff --git a/plugins/pay.c b/plugins/pay.c index 423816c4c214..77e81703778d 100644 --- a/plugins/pay.c +++ b/plugins/pay.c @@ -2007,7 +2007,7 @@ static const struct plugin_command commands[] = { int main(int argc, char *argv[]) { setup_locale(); - plugin_main(argv, init, PLUGIN_RESTARTABLE, NULL, commands, + plugin_main(argv, init, PLUGIN_RESTARTABLE, true, NULL, commands, ARRAY_SIZE(commands), NULL, 0, NULL, 0, plugin_option("disable-mpp", "flag", "Disable multi-part payments.", diff --git a/tests/plugins/test_libplugin.c b/tests/plugins/test_libplugin.c index bff60e89b6ef..66dcb7c182ec 100644 --- a/tests/plugins/test_libplugin.c +++ b/tests/plugins/test_libplugin.c @@ -124,7 +124,8 @@ static const struct plugin_notification notifs[] = { { int main(int argc, char *argv[]) { setup_locale(); - plugin_main(argv, init, PLUGIN_RESTARTABLE, NULL, commands, ARRAY_SIZE(commands), + plugin_main(argv, init, PLUGIN_RESTARTABLE, true, NULL, + commands, ARRAY_SIZE(commands), notifs, ARRAY_SIZE(notifs), hooks, ARRAY_SIZE(hooks), plugin_option("name", "string", From a6db3a1a6d054e4de07dc1ad46f05fc2e4640108 Mon Sep 17 00:00:00 2001 From: Christian Decker Date: Mon, 20 Jul 2020 15:34:23 +0200 Subject: [PATCH 16/24] pytest: We now have multiple attempts in test_htlc_send_timeout With MPP we end up with more than 1 attempt almost always. --- tests/test_misc.py | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/tests/test_misc.py b/tests/test_misc.py index e9ac8a9455c3..d235837cb235 100644 --- a/tests/test_misc.py +++ b/tests/test_misc.py @@ -1368,8 +1368,11 @@ def test_reserve_enforcement(node_factory, executor): def test_htlc_send_timeout(node_factory, bitcoind, compat): """Test that we don't commit an HTLC to an unreachable node.""" # Feerates identical so we don't get gratuitous commit to update them - l1 = node_factory.get_node(options={'log-level': 'io'}, - feerates=(7500, 7500, 7500, 7500)) + l1 = node_factory.get_node( + options={'log-level': 'io'}, + feerates=(7500, 7500, 7500, 7500) + ) + # Blackhole it after it sends HTLC_ADD to l3. l2 = node_factory.get_node(disconnect=['0WIRE_UPDATE_ADD_HTLC'], options={'log-level': 'io'}, @@ -1395,7 +1398,7 @@ def test_htlc_send_timeout(node_factory, bitcoind, compat): timedout = True inv = l3.rpc.invoice(123000, 'test_htlc_send_timeout', 'description') - with pytest.raises(RpcError, match=r'Ran out of routes to try after 1 attempt') as excinfo: + with pytest.raises(RpcError, match=r'Ran out of routes to try after [0-9]+ attempt[s]?') as excinfo: l1.rpc.pay(inv['bolt11']) err = excinfo.value From fa4c2ff1cff2313ab4a3cdd10b46fb3760046bb8 Mon Sep 17 00:00:00 2001 From: Christian Decker Date: Mon, 20 Jul 2020 20:28:31 +0200 Subject: [PATCH 17/24] paymod: Track how many HTLCs each channel can still add It turns out that by aggressively splitting payments we may end up exhausting the number of HTLCs we can add to a channel quickly. By tracking the number of HTLCs we can still add, and excluding the channels to which we cannot add any more we increase the route diversity, and avoid quickly exhausting the HTLC budget. In the next commit we'll also implement an early abort if we've exhausted all channels, so we don't end up splitting indefinitely and we can also optimize the initial split to not run afoul of that limit. --- plugins/libplugin-pay.c | 35 +++++++++++++++++++++++++++++++++-- plugins/libplugin-pay.h | 8 ++++++++ 2 files changed, 41 insertions(+), 2 deletions(-) diff --git a/plugins/libplugin-pay.c b/plugins/libplugin-pay.c index 3b38378a60eb..bab99f621e2b 100644 --- a/plugins/libplugin-pay.c +++ b/plugins/libplugin-pay.c @@ -198,6 +198,7 @@ static void payment_exclude_most_expensive(struct payment *p) hint.scid.scid = e->channel_id; hint.scid.dir = e->direction; hint.enabled = false; + hint.local = false; tal_arr_expand(&root->channel_hints, hint); } @@ -218,6 +219,7 @@ static void payment_exclude_longest_delay(struct payment *p) hint.scid.scid = e->channel_id; hint.scid.dir = e->direction; hint.enabled = false; + hint.local = false; tal_arr_expand(&root->channel_hints, hint); } @@ -278,6 +280,14 @@ static void payment_chanhints_apply_route(struct payment *p, bool remove) if (short_channel_id_eq(&curhint->scid.scid, &curhop->channel_id) && curhint->scid.dir == curhop->direction) { + + /* Update the number of htlcs for any local + * channel in the route */ + if (curhint->local && remove) + curhint->htlc_budget++; + else if (curhint->local) + curhint->htlc_budget--; + if (remove && !amount_msat_add( &curhint->estimated_capacity, curhint->estimated_capacity, @@ -392,6 +402,10 @@ payment_get_excluded_channels(const tal_t *ctx, struct payment *p) else if (amount_msat_greater_eq(p->amount, hint->estimated_capacity)) tal_arr_expand(&res, hint->scid); + else if (hint->local && hint->htlc_budget == 0) + /* If we cannot add any HTLCs to the channel we + * shouldn't look for a route through that channel */ + tal_arr_expand(&res, hint->scid); } return res; } @@ -619,6 +633,7 @@ static void channel_hints_update(struct payment *root, hint.scid.scid = *scid; hint.scid.dir = direction; hint.estimated_capacity = estimated_capacity; + hint.local = false; tal_arr_expand(&root->channel_hints, hint); plugin_log( @@ -1603,7 +1618,8 @@ static struct command_result * local_channel_hints_listpeers(struct command *cmd, const char *buffer, const jsmntok_t *toks, struct payment *p) { - const jsmntok_t *peers, *peer, *channels, *channel, *spendsats, *scid, *dir, *connected; + const jsmntok_t *peers, *peer, *channels, *channel, *spendsats, *scid, + *dir, *connected, *max_htlc, *htlcs; size_t i, j; peers = json_get_member(buffer, toks, "peers"); @@ -1622,7 +1638,12 @@ local_channel_hints_listpeers(struct command *cmd, const char *buffer, spendsats = json_get_member(buffer, channel, "spendable_msat"); scid = json_get_member(buffer, channel, "short_channel_id"); dir = json_get_member(buffer, channel, "direction"); - if(spendsats == NULL || scid == NULL || dir == NULL) + max_htlc = json_get_member(buffer, channel, "max_accepted_htlcs"); + htlcs = json_get_member(buffer, channel, "htlcs"); + if (spendsats == NULL || scid == NULL || dir == NULL || + max_htlc == NULL || + max_htlc->type != JSMN_PRIMITIVE || htlcs == NULL || + htlcs->type != JSMN_ARRAY) continue; json_to_bool(buffer, connected, &h.enabled); @@ -1630,6 +1651,16 @@ local_channel_hints_listpeers(struct command *cmd, const char *buffer, json_to_int(buffer, dir, &h.scid.dir); json_to_msat(buffer, spendsats, &h.estimated_capacity); + + /* Take the configured number of max_htlcs and + * subtract any HTLCs that might already be added to + * the channel. This is a best effort estimate and + * mostly considers stuck htlcs, concurrent payments + * may throw us off a bit. */ + json_to_u16(buffer, max_htlc, &h.htlc_budget); + h.htlc_budget -= htlcs->size; + h.local = true; + tal_arr_expand(&p->channel_hints, h); } } diff --git a/plugins/libplugin-pay.h b/plugins/libplugin-pay.h index 7c52173d69cb..a1d272984b16 100644 --- a/plugins/libplugin-pay.h +++ b/plugins/libplugin-pay.h @@ -69,6 +69,14 @@ struct channel_hint { /* Is the channel enabled? */ bool enabled; + + /* True if we are one endpoint of this channel */ + bool local; + + /* How many more htlcs can we send over this channel? Only set if this + * is a local channel, because those are the channels we have exact + * numbers on, and they are the bottleneck onto the network. */ + u16 htlc_budget; }; /* Each payment goes through a number of steps that are always processed in From 4102815b9ff2b6075534043d6f93ab3da0707150 Mon Sep 17 00:00:00 2001 From: Christian Decker Date: Mon, 20 Jul 2020 21:56:05 +0200 Subject: [PATCH 18/24] paymod: Teach the presplit modifier to respect the chan HTLC limit The presplit modifier could end up exceeding the maximum number of HTLCs we can add to a channel right out the gate, so we switch to a dynamic presplit if that is the case. The presplit will now at most use 1/3rd of the available HTLCs on the channels if the normal split would exceed the number of availabe HTLCs. And we also abort early if we don't have a sufficient HTLCs available. --- plugins/libplugin-pay.c | 78 ++++++++++++++++++++++++++++++----------- 1 file changed, 57 insertions(+), 21 deletions(-) diff --git a/plugins/libplugin-pay.c b/plugins/libplugin-pay.c index bab99f621e2b..31cc86f2cb72 100644 --- a/plugins/libplugin-pay.c +++ b/plugins/libplugin-pay.c @@ -2373,10 +2373,16 @@ REGISTER_PAYMENT_MODIFIER(waitblockheight, void *, NULL, waitblockheight_cb); * really the case, so this is likely a lower bound on the success rate. * * As the network evolves these numbers are also likely to change. + * + * Finally, if applied trivially this splitter may end up creating more splits + * than the sum of all channels can support, i.e., each split results in an + * HTLC, and each channel has an upper limit on the number of HTLCs it'll + * allow us to add. If the initial split would result in more than 1/3rd of + * the total available HTLCs we clamp the number of splits to 1/3rd. We don't + * use 3/3rds in order to retain flexibility in the adaptive splitter. */ #define MPP_TARGET_SIZE (10 * 1000 * 1000) -#define MPP_TARGET_MSAT AMOUNT_MSAT(MPP_TARGET_SIZE) -#define MPP_TARGET_FUZZ ( 1 * 1000 * 1000) +#define PRESPLIT_MAX_HTLC_SHARE 3 static struct presplit_mod_data *presplit_mod_data_init(struct payment *p) { @@ -2390,6 +2396,18 @@ static struct presplit_mod_data *presplit_mod_data_init(struct payment *p) } } +static u32 payment_max_htlcs(const struct payment *p) +{ + struct channel_hint *h; + u32 res = 0; + for (size_t i = 0; i < tal_count(p->channel_hints); i++) { + h = &p->channel_hints[i]; + if (h->local && h->enabled) + res += h->htlc_budget; + } + return res; +} + static bool payment_supports_mpp(struct payment *p) { if (p->invoice == NULL || p->invoice->features == NULL) @@ -2398,10 +2416,26 @@ static bool payment_supports_mpp(struct payment *p) return feature_offered(p->invoice->features, OPT_BASIC_MPP); } +/* Return fuzzed amount ~= target, but never exceeding max */ +static struct amount_msat fuzzed_near(struct amount_msat target, + struct amount_msat max) +{ + s64 fuzz; + struct amount_msat res = target; + + /* Somewhere within 25% of target please. */ + fuzz = pseudorand(target.millisatoshis / 2) /* Raw: fuzz */ + - target.millisatoshis / 4; /* Raw: fuzz */ + res.millisatoshis = target.millisatoshis + fuzz; /* Raw: fuzz < msat */ + + if (amount_msat_greater(res, max)) + res = max; + return res; +} + static void presplit_cb(struct presplit_mod_data *d, struct payment *p) { struct payment *root = payment_root(p); - struct amount_msat amt = root->amount; if (d->disable || p->parent != NULL || !payment_supports_mpp(p)) return payment_continue(p); @@ -2422,6 +2456,8 @@ static void presplit_cb(struct presplit_mod_data *d, struct payment *p) /* The presplitter only acts on the root and only in the first * step. */ size_t count = 0; + u32 htlcs = payment_max_htlcs(p) / PRESPLIT_MAX_HTLC_SHARE; + struct amount_msat target, amt = p->amount; /* We need to opt-in to the MPP sending facility no matter * what we do. That means setting all partids to a non-zero @@ -2434,31 +2470,31 @@ static void presplit_cb(struct presplit_mod_data *d, struct payment *p) * but makes debugging a bit easier. */ root->next_partid++; + if (htlcs == 0) { + p->abort = true; + return payment_fail( + p, "Cannot attempt payment, we have no channel to " + "which we can add an HTLC"); + } else if (p->amount.millisatoshis / MPP_TARGET_SIZE > htlcs) /* Raw: division */ + target.millisatoshis = p->amount.millisatoshis / htlcs; /* Raw: division */ + else + target = AMOUNT_MSAT(MPP_TARGET_SIZE); + /* If we are already below the target size don't split it * either. */ - if (amount_msat_greater(MPP_TARGET_MSAT, p->amount)) + if (amount_msat_greater(target, p->amount)) return payment_continue(p); /* Ok, we know we should split, so split here and then skip this * payment and start the children instead. */ - while (!amount_msat_eq(amt, AMOUNT_MSAT(0))) { - struct payment *c = - payment_new(p, NULL, p, p->modifiers); - - /* Pseudorandom number in the range [-1, 1]. */ - double rand = pseudorand_double() * 2 - 1; double multiplier; - c->amount.millisatoshis = rand * MPP_TARGET_FUZZ + MPP_TARGET_SIZE; /* Raw: Multiplication */ - - /* Clamp the value to the total amount, so the fuzzing - * doesn't go above the total. */ - if (amount_msat_greater(c->amount, amt)) - c->amount = amt; + struct payment *c = + payment_new(p, NULL, p, p->modifiers); - multiplier = - (double)c->amount.millisatoshis / (double)p->amount.millisatoshis; /* Raw: msat division. */ + /* Get ~ target, but don't exceed amt */ + c->amount = fuzzed_near(target, amt); if (!amount_msat_sub(&amt, amt, c->amount)) plugin_err( @@ -2471,6 +2507,7 @@ static void presplit_cb(struct presplit_mod_data *d, struct payment *p) /* Now adjust the constraints so we don't multiply them * when splitting. */ + multiplier = (double)c->amount.millisatoshis / (double)p->amount.millisatoshis; /* Raw: msat division. */ c->constraints.fee_budget.millisatoshis *= multiplier; /* Raw: Multiplication */ payment_start(c); count++; @@ -2480,11 +2517,10 @@ static void presplit_cb(struct presplit_mod_data *d, struct payment *p) p->route = NULL; p->why = tal_fmt( p, - "Split into %zu sub-payments due to initial size (%s > " - "%dmsat)", + "Split into %zu sub-payments due to initial size (%s > %s)", count, type_to_string(tmpctx, struct amount_msat, &root->amount), - MPP_TARGET_SIZE); + type_to_string(tmpctx, struct amount_msat, &target)); payment_set_step(p, PAYMENT_STEP_SPLIT); plugin_log(p->plugin, LOG_INFORM, "%s", p->why); } From c5f32e24187c52a789ed9bfd73b02b2a55aa9a57 Mon Sep 17 00:00:00 2001 From: Christian Decker Date: Mon, 20 Jul 2020 22:00:43 +0200 Subject: [PATCH 19/24] paymod: Teach the adaptive splitter to respect the HTLC limit There is little point in trying to split if the resulting HTLCs exceed the maximum number of HTLCs we can add to our channels. So abort if a split would result in more HTLCs than our channels can support. --- plugins/libplugin-pay.c | 52 +++++++++++++++++++++++++++++++++++------ plugins/libplugin-pay.h | 10 ++++---- 2 files changed, 51 insertions(+), 11 deletions(-) diff --git a/plugins/libplugin-pay.c b/plugins/libplugin-pay.c index 31cc86f2cb72..1736a122dfc3 100644 --- a/plugins/libplugin-pay.c +++ b/plugins/libplugin-pay.c @@ -2537,32 +2537,56 @@ REGISTER_PAYMENT_MODIFIER(presplit, struct presplit_mod_data *, * +/- 10% randomness, and then starts two attempts, one for either side of * the split. The goal is to find two smaller routes, that still adhere to our * constraints, but that can complete the payment. + * + * This modifier also checks whether we can split and still have enough HTLCs + * available on the channels and aborts if that's no longer the case. */ #define MPP_ADAPTIVE_LOWER_LIMIT AMOUNT_MSAT(100 * 1000) -static struct presplit_mod_data *adaptive_splitter_data_init(struct payment *p) +static struct adaptive_split_mod_data *adaptive_splitter_data_init(struct payment *p) { - struct presplit_mod_data *d; + struct adaptive_split_mod_data *d; if (p->parent == NULL) { - d = tal(p, struct presplit_mod_data); + d = tal(p, struct adaptive_split_mod_data); d->disable = false; + d->htlc_budget = 0; return d; } else { - return payment_mod_presplit_get_data(p->parent); + return payment_mod_adaptive_splitter_get_data(p->parent); } } -static void adaptive_splitter_cb(struct presplit_mod_data *d, struct payment *p) +static void adaptive_splitter_cb(struct adaptive_split_mod_data *d, struct payment *p) { struct payment *root = payment_root(p); - + struct adaptive_split_mod_data *root_data = + payment_mod_adaptive_splitter_get_data(root); if (d->disable) return payment_continue(p); if (!payment_supports_mpp(p) || root->abort) return payment_continue(p); + if (p->parent == NULL && d->htlc_budget == 0) { + /* Now that we potentially had an early splitter run, let's + * update our htlc_budget that we own exclusively from now + * on. We do this by subtracting the number of payment + * attempts an eventual presplitter has already performed. */ + struct payment_tree_result res; + res = payment_collect_result(p); + d->htlc_budget = payment_max_htlcs(p); + if (res.attempts > d->htlc_budget) { + p->abort = true; + return payment_fail( + p, + "Cannot add %d HTLCs to our channels, we " + "only have %d HTLCs available.", + res.attempts, d->htlc_budget); + } + d->htlc_budget -= res.attempts; + } + if (p->step == PAYMENT_STEP_ONION_PAYLOAD) { /* We need to tell the last hop the total we're going to * send. Presplit disables amount fuzzing, so we should always @@ -2585,6 +2609,16 @@ static void adaptive_splitter_cb(struct presplit_mod_data *d, struct payment *p) /* Use the start constraints, not the ones updated by routes and shadow-routes. */ struct payment_constraints *pconstraints = p->start_constraints; + /* First check that splitting doesn't exceed our HTLC budget */ + if (root_data->htlc_budget == 0) { + root->abort = true; + return payment_fail( + p, + "Cannot split payment any further without " + "exceeding the maximum number of HTLCs " + "allowed by our channels"); + } + a = payment_new(p, NULL, p, p->modifiers); b = payment_new(p, NULL, p, p->modifiers); @@ -2610,6 +2644,10 @@ static void adaptive_splitter_cb(struct presplit_mod_data *d, struct payment *p) payment_start(a); payment_start(b); p->step = PAYMENT_STEP_SPLIT; + + /* Take note that we now have an additional split that + * may end up using an HTLC. */ + root_data->htlc_budget--; } else { plugin_log(p->plugin, LOG_INFORM, "Lower limit of adaptive splitter reached " @@ -2623,5 +2661,5 @@ static void adaptive_splitter_cb(struct presplit_mod_data *d, struct payment *p) payment_continue(p); } -REGISTER_PAYMENT_MODIFIER(adaptive_splitter, struct presplit_mod_data *, +REGISTER_PAYMENT_MODIFIER(adaptive_splitter, struct adaptive_split_mod_data *, adaptive_splitter_data_init, adaptive_splitter_cb); diff --git a/plugins/libplugin-pay.h b/plugins/libplugin-pay.h index a1d272984b16..209c2edce650 100644 --- a/plugins/libplugin-pay.h +++ b/plugins/libplugin-pay.h @@ -328,13 +328,15 @@ struct direct_pay_data { struct short_channel_id_dir *chan; }; -/* Since presplit and adaptive mpp modifiers share the same information we - * just use the same backing struct. Should they deviate we can create an - * adaptive_splitter_mod_data struct and populate that. */ struct presplit_mod_data { bool disable; }; +struct adaptive_split_mod_data { + bool disable; + u32 htlc_budget; +}; + /* List of globally available payment modifiers. */ REGISTER_PAYMENT_MODIFIER_HEADER(retry, struct retry_mod_data); REGISTER_PAYMENT_MODIFIER_HEADER(routehints, struct routehints_data); @@ -343,7 +345,7 @@ REGISTER_PAYMENT_MODIFIER_HEADER(shadowroute, struct shadow_route_data); REGISTER_PAYMENT_MODIFIER_HEADER(directpay, struct direct_pay_data); extern struct payment_modifier waitblockheight_pay_mod; REGISTER_PAYMENT_MODIFIER_HEADER(presplit, struct presplit_mod_data); -REGISTER_PAYMENT_MODIFIER_HEADER(adaptive_splitter, struct presplit_mod_data); +REGISTER_PAYMENT_MODIFIER_HEADER(adaptive_splitter, struct adaptive_split_mod_data); /* For the root payment we can seed the channel_hints with the result from * `listpeers`, hence avoid channels that we know have insufficient capacity From 2b4ad9b31f695f7b0ccd462a8671cadd2f09bf13 Mon Sep 17 00:00:00 2001 From: Christian Decker Date: Tue, 21 Jul 2020 20:45:38 +0200 Subject: [PATCH 20/24] paymod: Add the courtesy +1 to the CLTVs to allow for a new block This may be related to the issue #3862, however the water was muddied by it being the wrong error to return, and the node should not expect this courtesy feature to be present at all... --- plugins/libplugin-pay.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/plugins/libplugin-pay.c b/plugins/libplugin-pay.c index 1736a122dfc3..adc97c8178ba 100644 --- a/plugins/libplugin-pay.c +++ b/plugins/libplugin-pay.c @@ -1049,7 +1049,7 @@ static void payment_add_hop_onion_payload(struct payment *p, struct secret *payment_secret) { struct createonion_request *cr = p->createonion_request; - u32 cltv = p->start_block + next->delay; + u32 cltv = p->start_block + next->delay + 1; u64 msat = next->amount.millisatoshis; /* Raw: TLV payload generation*/ struct tlv_field **fields; static struct short_channel_id all_zero_scid = {.u64 = 0}; From 2e80e1c336ec3f1469a4ae1d4afdfd05f95b74bc Mon Sep 17 00:00:00 2001 From: Christian Decker Date: Tue, 21 Jul 2020 22:09:54 +0200 Subject: [PATCH 21/24] paymod: Fix the routehints being lost when retrying We were removing the current hint from the list and not inheriting the current routehint, so we'd be forgetting a hint at each retry. Now we keep the array unchanged in the root, and simply skip the ones that are not usable given the current information we have about the channels (in the form of channel_hints). Fixes #3861 --- plugins/libplugin-pay.c | 33 +++++++++++++++++++-------------- 1 file changed, 19 insertions(+), 14 deletions(-) diff --git a/plugins/libplugin-pay.c b/plugins/libplugin-pay.c index adc97c8178ba..9ec527b8e09e 100644 --- a/plugins/libplugin-pay.c +++ b/plugins/libplugin-pay.c @@ -1765,14 +1765,12 @@ static bool routehint_excluded(struct payment *p, static struct route_info *next_routehint(struct routehints_data *d, struct payment *p) { - while (tal_count(d->routehints) > 0) { - if (!routehint_excluded(p, d->routehints[0])) { - d->current_routehint = d->routehints[0]; - tal_arr_remove(&d->routehints, 0); - return d->current_routehint; + /* FIXME: Add a pseudorandom offset to rotate between the routehints + * we use, similar to what we'd do by randomizing the routes. */ + for (size_t i=0; iroutehints); i++) { + if (!routehint_excluded(p, d->routehints[i])) { + return d->routehints[i]; } - tal_free(d->routehints[0]); - tal_arr_remove(&d->routehints, 0); } return NULL; } @@ -1821,17 +1819,19 @@ static void routehint_step_cb(struct routehints_data *d, struct payment *p) if (root->invoice == NULL || root->invoice->routes == NULL) return payment_continue(p); - /* The root payment gets the unmodified routehints, children may - * start dropping some as they learn that they were not - * functional. */ + /* We filter out non-functional routehints once at the + * beginning, and every other payment will filter out the + * exluded ones on the fly. */ if (p->parent == NULL) { d->routehints = filter_routehints(d, p->local_id, p->invoice->routes); } else { pd = payment_mod_get_data(p->parent, &routehints_pay_mod); - d->routehints = tal_dup_talarr(d, struct route_info *, - pd->routehints); + /* Since we don't modify the list of routehints after + * the root has filtered them we can just shared a + * pointer here. */ + d->routehints = pd->routehints; } d->current_routehint = next_routehint(d, p); @@ -1847,6 +1847,11 @@ static void routehint_step_cb(struct routehints_data *d, struct payment *p) p->getroute->cltv = route_cltv(p->getroute->cltv, d->current_routehint, tal_count(d->current_routehint)); + plugin_log(p->plugin, LOG_DBG, "Using routehint %s (%s) cltv_delta=%d", + type_to_string(tmpctx, struct node_id, &d->current_routehint->pubkey), + type_to_string(tmpctx, struct short_channel_id, &d->current_routehint->short_channel_id), + d->current_routehint->cltv_expiry_delta + ); } } else if (p->step == PAYMENT_STEP_GOT_ROUTE) { /* Now it's time to stitch the two partial routes together. */ @@ -1886,8 +1891,8 @@ static void routehint_step_cb(struct routehints_data *d, struct payment *p) static struct routehints_data *routehint_data_init(struct payment *p) { - /* We defer the actual initialization to the step callback when we have - * the invoice attached. */ + /* We defer the actual initialization to the step callback when + * we have the invoice attached. */ return talz(p, struct routehints_data); } From 89cb5a6aeab4d5d0988a5b6f291ef4585a20cb74 Mon Sep 17 00:00:00 2001 From: Christian Decker Date: Wed, 22 Jul 2020 11:36:55 +0200 Subject: [PATCH 22/24] paymod: Consolidate channel_hint creation in channel_hints_update As the hints get new fields added it is easy to forget to amend one of the places we create them, since we already have an update method let's use that to handle all additions to the array of known channel hints. --- plugins/libplugin-pay.c | 126 +++++++++++++++++++++------------------- 1 file changed, 65 insertions(+), 61 deletions(-) diff --git a/plugins/libplugin-pay.c b/plugins/libplugin-pay.c index 9ec527b8e09e..4c896d4e1491 100644 --- a/plugins/libplugin-pay.c +++ b/plugins/libplugin-pay.c @@ -179,12 +179,63 @@ void payment_start(struct payment *p) payment_rpc_failure, p)); } -static void payment_exclude_most_expensive(struct payment *p) +static void channel_hints_update(struct payment *p, + const struct short_channel_id scid, + int direction, bool enabled, bool local, + struct amount_msat *estimated_capacity, + u16 *htlc_budget) { struct payment *root = payment_root(p); + struct channel_hint hint; + + /* If the channel is marked as enabled it must have an estimate. */ + assert(!enabled || estimated_capacity != NULL); + + /* Try and look for an existing hint: */ + for (size_t i=0; ichannel_hints); i++) { + struct channel_hint *hint = &root->channel_hints[i]; + if (short_channel_id_eq(&hint->scid.scid, &scid) && + hint->scid.dir == direction) { + /* Prefer to disable a channel. */ + hint->enabled = hint->enabled & enabled; + + /* Prefer the more conservative estimate. */ + if (estimated_capacity != NULL && + amount_msat_greater(hint->estimated_capacity, + *estimated_capacity)) + hint->estimated_capacity = *estimated_capacity; + if (htlc_budget != NULL && *htlc_budget < hint->htlc_budget) + hint->htlc_budget = *htlc_budget; + return; + } + } + + /* No hint found, create one. */ + hint.enabled = enabled; + hint.scid.scid = scid; + hint.scid.dir = direction; + hint.local = local; + if (estimated_capacity != NULL) + hint.estimated_capacity = *estimated_capacity; + + if (htlc_budget != NULL) + hint.htlc_budget = *htlc_budget; + + tal_arr_expand(&root->channel_hints, hint); + + plugin_log( + root->plugin, LOG_DBG, + "Added a channel hint for %s: enabled %s, estimated capacity %s", + type_to_string(tmpctx, struct short_channel_id_dir, &hint.scid), + hint.enabled ? "true" : "false", + type_to_string(tmpctx, struct amount_msat, + &hint.estimated_capacity)); +} + +static void payment_exclude_most_expensive(struct payment *p) +{ struct route_hop *e = &p->route[0]; struct amount_msat fee, worst = AMOUNT_MSAT(0); - struct channel_hint hint; for (size_t i = 0; i < tal_count(p->route)-1; i++) { if (!amount_msat_sub(&fee, p->route[i].amount, p->route[i+1].amount)) @@ -195,19 +246,14 @@ static void payment_exclude_most_expensive(struct payment *p) worst = fee; } } - hint.scid.scid = e->channel_id; - hint.scid.dir = e->direction; - hint.enabled = false; - hint.local = false; - tal_arr_expand(&root->channel_hints, hint); + channel_hints_update(p, e->channel_id, e->direction, false, false, + NULL, NULL); } static void payment_exclude_longest_delay(struct payment *p) { - struct payment *root = payment_root(p); struct route_hop *e = &p->route[0]; u32 delay, worst = 0; - struct channel_hint hint; for (size_t i = 0; i < tal_count(p->route)-1; i++) { delay = p->route[i].delay - p->route[i+1].delay; @@ -216,11 +262,8 @@ static void payment_exclude_longest_delay(struct payment *p) worst = delay; } } - hint.scid.scid = e->channel_id; - hint.scid.dir = e->direction; - hint.enabled = false; - hint.local = false; - tal_arr_expand(&root->channel_hints, hint); + channel_hints_update(p, e->channel_id, e->direction, false, false, + NULL, NULL); } static struct amount_msat payment_route_fee(struct payment *p) @@ -605,46 +648,6 @@ static struct payment_result *tal_sendpay_result_from_json(const tal_t *ctx, return tal_free(result); } -static void channel_hints_update(struct payment *root, - const struct short_channel_id *scid, - int direction, - bool enabled, - struct amount_msat estimated_capacity) -{ - struct channel_hint hint; - /* Try and look for an existing hint: */ - for (size_t i=0; ichannel_hints); i++) { - struct channel_hint *hint = &root->channel_hints[i]; - if (short_channel_id_eq(&hint->scid.scid, scid) && - hint->scid.dir == direction) { - /* Prefer to disable a channel. */ - hint->enabled = hint->enabled & enabled; - - /* Prefer the more conservative estimate. */ - if (amount_msat_greater(hint->estimated_capacity, - estimated_capacity)) - hint->estimated_capacity = estimated_capacity; - return; - } - } - - /* No hint found, create one. */ - hint.enabled = enabled; - hint.scid.scid = *scid; - hint.scid.dir = direction; - hint.estimated_capacity = estimated_capacity; - hint.local = false; - tal_arr_expand(&root->channel_hints, hint); - - plugin_log( - root->plugin, LOG_DBG, - "Added a channel hint for %s: enabled %s, estimated capacity %s", - type_to_string(tmpctx, struct short_channel_id_dir, &hint.scid), - hint.enabled ? "true" : "false", - type_to_string(tmpctx, struct amount_msat, - &hint.estimated_capacity)); -} - /* Try to infer the erring_node, erring_channel and erring_direction from what * we know, but don't override the values that are returned by `waitsendpay`. */ static void payment_result_infer(struct route_hop *route, @@ -825,9 +828,9 @@ handle_intermediate_failure(struct command *cmd, case WIRE_UNKNOWN_NEXT_PEER: case WIRE_REQUIRED_CHANNEL_FEATURE_MISSING: /* All of these result in the channel being marked as disabled. */ - channel_hints_update(root, - &errchan->channel_id, errchan->direction, - false, AMOUNT_MSAT(0)); + channel_hints_update(root, errchan->channel_id, + errchan->direction, false, false, NULL, + NULL); break; case WIRE_TEMPORARY_CHANNEL_FAILURE: { @@ -835,9 +838,9 @@ handle_intermediate_failure(struct command *cmd, * remember the amount we tried as an estimate. */ struct amount_msat est = errchan->amount; est.millisatoshis *= 0.75; /* Raw: Multiplication */ - channel_hints_update(root, - &errchan->channel_id, errchan->direction, - true, est); + channel_hints_update(root, errchan->channel_id, + errchan->direction, true, false, &est, + NULL); goto error; } @@ -1661,7 +1664,8 @@ local_channel_hints_listpeers(struct command *cmd, const char *buffer, h.htlc_budget -= htlcs->size; h.local = true; - tal_arr_expand(&p->channel_hints, h); + channel_hints_update(p, h.scid.scid, h.scid.dir, + h.enabled, true, &h.estimated_capacity, &h.htlc_budget); } } From 648af0292b95074bf3ca7eee3e85dfbc546af79d Mon Sep 17 00:00:00 2001 From: Christian Decker Date: Wed, 22 Jul 2020 16:06:39 +0200 Subject: [PATCH 23/24] paymod: Randomly select a routehint, or none at random The adaptive MPP test was showing an issue with always using a routehint, even when it wasn't necessary: we would insist on routhing to the entrypoint of the routehint, even through the actual destination. If a channel on that loop would result being over capacity we'd slam below 0, and then increase again by unapplying the route. The solution really is not to insist on routing through a routehint, so we implement random skipping of routehints, and we rotate them if we have multiples. --- plugins/libplugin-pay.c | 15 +++++++++++---- tests/test_pay.py | 8 ++++---- 2 files changed, 15 insertions(+), 8 deletions(-) diff --git a/plugins/libplugin-pay.c b/plugins/libplugin-pay.c index 4c896d4e1491..d2a058f1c69c 100644 --- a/plugins/libplugin-pay.c +++ b/plugins/libplugin-pay.c @@ -1769,12 +1769,19 @@ static bool routehint_excluded(struct payment *p, static struct route_info *next_routehint(struct routehints_data *d, struct payment *p) { - /* FIXME: Add a pseudorandom offset to rotate between the routehints - * we use, similar to what we'd do by randomizing the routes. */ + /* Implements a random selection of a routehint, or none in 1/numhints + * cases, by starting the iteration of the routehints in a random + * order, and adding a virtual NULL result at the end. */ + size_t numhints = tal_count(d->routehints); + size_t offset = pseudorand(numhints + 1); + for (size_t i=0; iroutehints); i++) { - if (!routehint_excluded(p, d->routehints[i])) { + size_t curr = (offset + i) % (numhints + 1); + if (curr == numhints) + return NULL; + + if (!routehint_excluded(p, d->routehints[curr])) return d->routehints[i]; - } } return NULL; } diff --git a/tests/test_pay.py b/tests/test_pay.py index e41afdfff96b..e4b7c2df6751 100644 --- a/tests/test_pay.py +++ b/tests/test_pay.py @@ -3103,10 +3103,10 @@ def test_mpp_adaptive(node_factory, bitcoind): ```dot digraph { - l1 -> l2; - l2 -> l4; - l1 -> l3; - l3 -> l4; + l1 -> l2 [label="scid=103x1x1, cap=amt-1"]; + l2 -> l4 [label="scid=105x1x1, cap=max"]; + l1 -> l3 [label="scid=107x1x1, cap=max"]; + l3 -> l4 [label="scid=109x1x1, cap=amt-1"]; } """ amt = 10**7 - 1 From aad62fce6e1ae478bc6319ac58a6c08eabcfd0f4 Mon Sep 17 00:00:00 2001 From: Christian Decker Date: Wed, 22 Jul 2020 17:47:59 +0200 Subject: [PATCH 24/24] paymod: Always initialize p->route We're using it in a couple of places to see if we even performed the attempt, so we need to make sure it's initialized. --- plugins/libplugin-pay.c | 1 + 1 file changed, 1 insertion(+) diff --git a/plugins/libplugin-pay.c b/plugins/libplugin-pay.c index d2a058f1c69c..b4f612cd1109 100644 --- a/plugins/libplugin-pay.c +++ b/plugins/libplugin-pay.c @@ -25,6 +25,7 @@ struct payment *payment_new(tal_t *ctx, struct command *cmd, p->failreason = NULL; p->getroute->riskfactorppm = 10000000; p->abort = false; + p->route = NULL; /* Copy over the relevant pieces of information. */ if (parent != NULL) {