From 2e1d6c3c29a7c002c1e42174dcd02df62051e29d Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Wed, 9 Aug 2023 11:44:58 +0930 Subject: [PATCH 01/22] renepay: remove unused all_flows field. Signed-off-by: Rusty Russell --- plugins/renepay/payment.c | 1 - plugins/renepay/payment.h | 3 --- 2 files changed, 4 deletions(-) diff --git a/plugins/renepay/payment.c b/plugins/renepay/payment.c index deff954957d0..452a4caf2036 100644 --- a/plugins/renepay/payment.c +++ b/plugins/renepay/payment.c @@ -55,7 +55,6 @@ struct renepay * renepay_new(struct command *cmd) renepay->rexmit_timer = NULL; renepay->next_attempt=1; renepay->next_partid=1; - renepay->all_flows = tal(renepay,tal_t); return renepay; } diff --git a/plugins/renepay/payment.h b/plugins/renepay/payment.h index 96da7a9ce2f5..fda3ad29df57 100644 --- a/plugins/renepay/payment.h +++ b/plugins/renepay/payment.h @@ -124,9 +124,6 @@ struct renepay int next_attempt; /* Used in get_payflows to set ids to each pay_flow. */ u64 next_partid; - - /* Root to destroy pending flows */ - tal_t *all_flows; }; struct payment * payment_new(struct renepay *renepay); From 8c0e4fe50dd97b31a2902d37534f9dfaeadbd151 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Wed, 9 Aug 2023 11:45:58 +0930 Subject: [PATCH 02/22] renepay: remove attempt limit. Time is what users care about, so remove this. Signed-off-by: Rusty Russell --- plugins/renepay/pay.c | 12 +----------- plugins/renepay/pay_flow.c | 1 - plugins/renepay/pay_flow.h | 3 --- plugins/renepay/payment.c | 15 --------------- plugins/renepay/payment.h | 7 ------- 5 files changed, 1 insertion(+), 37 deletions(-) diff --git a/plugins/renepay/pay.c b/plugins/renepay/pay.c index ff11721447df..2c634b33130f 100644 --- a/plugins/renepay/pay.c +++ b/plugins/renepay/pay.c @@ -490,19 +490,9 @@ static struct command_result *try_paying(struct command *cmd, struct payment * const p = renepay->payment; plugin_log(pay_plugin->plugin,LOG_DBG,"calling %s",__PRETTY_FUNCTION__); - // TODO(eduardo): does it make sense to have this limit on attempts? - /* I am classifying the flows in attempt cycles. */ - renepay_new_attempt(renepay); - /* We try only MAX_NUM_ATTEMPTS, then we give up. */ - if ( renepay_attempt_count(renepay) > MAX_NUM_ATTEMPTS) - { - return renepay_fail(renepay, PAY_STOPPED_RETRYING, - "Reached maximum number of attempts (%d)", - MAX_NUM_ATTEMPTS); - } - struct amount_msat feebudget, fees_spent, remaining; + renepay->payment->status=PAYMENT_PENDING; if (time_after(time_now(), p->stop_time)) return renepay_fail(renepay, PAY_STOPPED_RETRYING, "Timed out"); diff --git a/plugins/renepay/pay_flow.c b/plugins/renepay/pay_flow.c index a1fed1076ca7..91c691ab4fe3 100644 --- a/plugins/renepay/pay_flow.c +++ b/plugins/renepay/pay_flow.c @@ -244,7 +244,6 @@ static struct pay_flow **flows_to_pay_flows(struct payment *payment, pf->amounts = tal_steal(pf, f->amounts); pf->path_dirs = tal_steal(pf, f->dirs); pf->success_prob = f->success_prob; - pf->attempt = renepay_current_attempt(payment->renepay); } tal_free(flows); diff --git a/plugins/renepay/pay_flow.h b/plugins/renepay/pay_flow.h index e519e9f7f098..1a6e9b73c4f5 100644 --- a/plugins/renepay/pay_flow.h +++ b/plugins/renepay/pay_flow.h @@ -14,9 +14,6 @@ struct pay_flow { /* So we can be an independent object for callbacks. */ struct payment * payment; - // TODO(eduardo): remove this, unnecessary - int attempt; - /* Information to link this flow to a unique sendpay. */ struct payflow_key { diff --git a/plugins/renepay/payment.c b/plugins/renepay/payment.c index 452a4caf2036..de3a48af28ac 100644 --- a/plugins/renepay/payment.c +++ b/plugins/renepay/payment.c @@ -53,7 +53,6 @@ struct renepay * renepay_new(struct command *cmd) renepay->local_gossmods = gossmap_localmods_new(renepay); renepay->disabled = tal_arr(renepay,struct short_channel_id,0); renepay->rexmit_timer = NULL; - renepay->next_attempt=1; renepay->next_partid=1; return renepay; @@ -131,20 +130,6 @@ void payment_assert_delivering_all(const struct payment *p) } } - -int renepay_current_attempt(const struct renepay * renepay) -{ - return renepay->next_attempt-1; -} -int renepay_attempt_count(const struct renepay * renepay) -{ - return renepay->next_attempt-1; -} -void renepay_new_attempt(struct renepay * renepay) -{ - renepay->payment->status=PAYMENT_PENDING; - renepay->next_attempt++; -} struct command_result *renepay_success(struct renepay * renepay) { debug_info("calling %s",__PRETTY_FUNCTION__); diff --git a/plugins/renepay/payment.h b/plugins/renepay/payment.h index fda3ad29df57..74ccabe3a380 100644 --- a/plugins/renepay/payment.h +++ b/plugins/renepay/payment.h @@ -120,8 +120,6 @@ struct renepay /* Timers. */ struct plugin_timer *rexmit_timer; - /* Keep track of the number of attempts. */ - int next_attempt; /* Used in get_payflows to set ids to each pay_flow. */ u64 next_partid; }; @@ -143,11 +141,6 @@ void payment_note(struct payment *p, const char *fmt, ...); void payment_assert_delivering_incomplete(const struct payment *p); void payment_assert_delivering_all(const struct payment *p); - -int renepay_current_attempt(const struct renepay *renepay); -int renepay_attempt_count(const struct renepay *renepay); -void renepay_new_attempt(struct renepay *renepay); - struct command_result *renepay_success(struct renepay *renepay); struct command_result *renepay_fail( From 9fe203e0cacacd9d92534107e07cc85944ecd3e4 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Wed, 9 Aug 2023 11:46:58 +0930 Subject: [PATCH 03/22] renepay: fix localmods. You cannot refresh the gossmap with localmods applied, nor apply localmods when others have applied localmods in the same process. There are optimizations we could do, but for now always apply/unapply before querying gossmap. Signed-off-by: Rusty Russell --- plugins/renepay/pay.c | 6 ++---- plugins/renepay/payment.c | 12 ------------ plugins/renepay/payment.h | 1 - 3 files changed, 2 insertions(+), 17 deletions(-) diff --git a/plugins/renepay/pay.c b/plugins/renepay/pay.c index 2c634b33130f..9af3e6c6fb7a 100644 --- a/plugins/renepay/pay.c +++ b/plugins/renepay/pay.c @@ -546,6 +546,7 @@ static struct command_result *try_paying(struct command *cmd, /* We let this return an unlikely path, as it's better to try once * than simply refuse. Plus, models are not truth! */ + gossmap_apply_localmods(pay_plugin->gossmap, renepay->local_gossmods); struct pay_flow **pay_flows = get_payflows( renepay, remaining, feebudget, @@ -558,6 +559,7 @@ static struct command_result *try_paying(struct command *cmd, amount_msat_eq(p->total_delivering, AMOUNT_MSAT(0)), &err_msg); + gossmap_remove_localmods(pay_plugin->gossmap, renepay->local_gossmods); // plugin_log(pay_plugin->plugin,LOG_DBG,"get_payflows produced %s",fmt_payflows(tmpctx,pay_flows)); @@ -590,13 +592,9 @@ static struct command_result *listpeerchannels_done( "listpeerchannels malformed: %.*s", json_tok_full_len(result), json_tok_full(buf, result)); - // So we have all localmods data, now we apply it. Only once per - // payment. // TODO(eduardo): check that there won't be a prob. cost associated with // any gossmap local chan. The same way there aren't fees to pay for my // local channels. - gossmap_apply_localmods(pay_plugin->gossmap,renepay->local_gossmods); - renepay->localmods_applied=true; return try_paying(cmd, renepay, true); } diff --git a/plugins/renepay/payment.c b/plugins/renepay/payment.c index de3a48af28ac..8eb070b805a7 100644 --- a/plugins/renepay/payment.c +++ b/plugins/renepay/payment.c @@ -49,7 +49,6 @@ struct renepay * renepay_new(struct command *cmd) renepay->cmd = cmd; renepay->payment = payment_new(renepay); - renepay->localmods_applied=false; renepay->local_gossmods = gossmap_localmods_new(renepay); renepay->disabled = tal_arr(renepay,struct short_channel_id,0); renepay->rexmit_timer = NULL; @@ -190,21 +189,10 @@ void renepay_cleanup( struct gossmap * gossmap) { debug_info("calling %s",__PRETTY_FUNCTION__); - /* Always remove our local mods (routehints) so others can use - * gossmap. We do this only after the payment completes. */ // TODO(eduardo): it can happen that local_gossmods removed below // contained a set of channels for which there is information in the // uncertainty network (chan_extra_map) and that are part of some pending // payflow (payflow_map). Handle this situation. - if(renepay->localmods_applied) - gossmap_remove_localmods(gossmap, - renepay->local_gossmods); - // TODO(eduardo): I wonder if it is possible to have two instances of - // renepay at the same time. - // 1st problem: dijkstra datastructure is global, this can be fixed, - // 2nd problem: we don't know if gossmap_apply_localmods and gossmap_remove_localmods, - // can handle different local_gossmods applied to the same gossmap. - renepay->localmods_applied=false; tal_free(renepay->local_gossmods); renepay->rexmit_timer = tal_free(renepay->rexmit_timer); diff --git a/plugins/renepay/payment.h b/plugins/renepay/payment.h index 74ccabe3a380..471fda906a0b 100644 --- a/plugins/renepay/payment.h +++ b/plugins/renepay/payment.h @@ -111,7 +111,6 @@ struct renepay struct payment * payment; /* Localmods to apply to gossip_map for our own use. */ - bool localmods_applied; struct gossmap_localmods *local_gossmods; /* Channels we decided to disable for various reasons. */ From 70042fea422f158bf211ad725c76fda5ebe5e131 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Thu, 10 Aug 2023 09:30:43 +0930 Subject: [PATCH 04/22] renepay: use more formal allocator pattern. The general pattern for xxx_new is that it should populate all fields, for encapsulation and so you never can have a half-formed object. This means a fair bit of work for now, but it pays off in the next patch. Signed-off-by: Rusty Russell --- plugins/renepay/pay.c | 215 ++++++++++++++++++-------------------- plugins/renepay/payment.c | 95 +++++++++++++---- plugins/renepay/payment.h | 23 +++- 3 files changed, 195 insertions(+), 138 deletions(-) diff --git a/plugins/renepay/pay.c b/plugins/renepay/pay.c index 9af3e6c6fb7a..a8d1219a4243 100644 --- a/plugins/renepay/pay.c +++ b/plugins/renepay/pay.c @@ -916,16 +916,18 @@ static struct command_result *json_pay(struct command *cmd, u64 invexpiry; struct amount_msat *msat, *invmsat; struct amount_msat *maxfee; + struct sha256 payment_hash; + struct secret *payment_secret; + const u8 *payment_metadata; + struct node_id destination; u32 *maxdelay; u32 *retryfor; - -#if DEVELOPER u64 *base_fee_penalty; u64 *prob_cost_factor; u64 *riskfactor_millionths; u64 *min_prob_success_millionths; bool *use_shadow; -#endif + u16 final_cltv; if (!param(cmd, buf, params, p_req("invstring", param_invstring, &invstr), @@ -954,95 +956,34 @@ static struct command_result *json_pay(struct command *cmd, NULL)) return command_param_failed(); - /* renepay is bound to the command, if the command finishes renepay is - * freed. */ - struct renepay * renepay = renepay_new(cmd); - tal_add_destructor2(renepay, - renepay_cleanup, - pay_plugin->gossmap); - struct payment * p = renepay->payment; - - p->invstr = tal_steal(p,invstr); - p->description = tal_steal(p,description); - p->label = tal_steal(p,label); - p->local_offer_id = tal_steal(p,local_offer_id); - - - - /* Please renepay try to give me a reliable payment 90% chances of - * success, once you do, then minimize as much as possible those fees. */ - p->min_prob_success = 0.9; - - /* Default delay_feefactor: how much shall we penalize for delay. */ - p->delay_feefactor = 1e-6; - - /* Default prob_cost_factor: how to convert prob. cost to sats. */ - p->prob_cost_factor = 10; - - /* Default base_fee_penalty: how to convert a base fee into a - * proportional fee. */ - p->base_fee_penalty = 10; - -#if DEVELOPER - p->base_fee_penalty = *base_fee_penalty; - base_fee_penalty = tal_free(base_fee_penalty); - - p->prob_cost_factor = *prob_cost_factor; - prob_cost_factor = tal_free(prob_cost_factor); - - p->min_prob_success = *min_prob_success_millionths/1e6; - min_prob_success_millionths = tal_free(min_prob_success_millionths); - - p->delay_feefactor = *riskfactor_millionths/1e6; - riskfactor_millionths = tal_free(riskfactor_millionths); -#endif - - p->maxdelay = *maxdelay; - maxdelay = tal_free(maxdelay); - - /* We inmediately add this payment to the payment list. */ - tal_steal(pay_plugin->ctx,p); - list_add_tail(&pay_plugin->payments, &p->list); - tal_add_destructor(p, destroy_payment); - - plugin_log(pay_plugin->plugin,LOG_DBG,"Starting renepay"); - bool gossmap_changed = gossmap_refresh(pay_plugin->gossmap, NULL); - - if (pay_plugin->gossmap == NULL) - plugin_err(pay_plugin->plugin, "Failed to refresh gossmap: %s", - strerror(errno)); - - p->start_time = time_now(); - p->stop_time = timeabs_add(p->start_time, time_from_sec(*retryfor)); - tal_free(retryfor); - + /* We might need to parse invstring to get amount */ bool invstr_is_b11=false; - if (!bolt12_has_prefix(p->invstr)) { + if (!bolt12_has_prefix(invstr)) { struct bolt11 *b11; char *fail; b11 = - bolt11_decode(tmpctx, p->invstr, plugin_feature_set(cmd->plugin), - p->description, chainparams, &fail); + bolt11_decode(tmpctx, invstr, plugin_feature_set(cmd->plugin), + description, chainparams, &fail); if (b11 == NULL) - return renepay_fail(renepay, JSONRPC2_INVALID_PARAMS, + return command_fail(cmd, JSONRPC2_INVALID_PARAMS, "Invalid bolt11: %s", fail); invstr_is_b11=true; invmsat = b11->msat; invexpiry = b11->timestamp + b11->expiry; - p->destination = b11->receiver_id; - p->payment_hash = b11->payment_hash; - p->payment_secret = - tal_dup_or_null(p, struct secret, b11->payment_secret); + destination = b11->receiver_id; + payment_hash = b11->payment_hash; + payment_secret = + tal_dup_or_null(cmd, struct secret, b11->payment_secret); if (b11->metadata) - p->payment_metadata = tal_dup_talarr(p, u8, b11->metadata); + payment_metadata = tal_dup_talarr(cmd, u8, b11->metadata); else - p->payment_metadata = NULL; + payment_metadata = NULL; - p->final_cltv = b11->min_final_cltv_expiry; + final_cltv = b11->min_final_cltv_expiry; /* Sanity check */ if (feature_offered(b11->features, OPT_VAR_ONION) && !b11->payment_secret) @@ -1058,12 +999,12 @@ static struct command_result *json_pay(struct command *cmd, */ if (!b11->description) { if (!b11->description_hash) { - return renepay_fail(renepay, + return command_fail(cmd, JSONRPC2_INVALID_PARAMS, "Invalid bolt11: missing description"); } - if (!p->description) - return renepay_fail(renepay, + if (!description) + return command_fail(cmd, JSONRPC2_INVALID_PARAMS, "bolt11 uses description_hash, but you did not provide description parameter"); } @@ -1071,24 +1012,24 @@ static struct command_result *json_pay(struct command *cmd, // TODO(eduardo): check this, compare with `pay` const struct tlv_invoice *b12; char *fail; - b12 = invoice_decode(tmpctx, p->invstr, strlen(p->invstr), + b12 = invoice_decode(tmpctx, invstr, strlen(invstr), plugin_feature_set(cmd->plugin), chainparams, &fail); if (b12 == NULL) - return renepay_fail(renepay, JSONRPC2_INVALID_PARAMS, + return command_fail(cmd, JSONRPC2_INVALID_PARAMS, "Invalid bolt12: %s", fail); if (!pay_plugin->exp_offers) - return renepay_fail(renepay, JSONRPC2_INVALID_PARAMS, + return command_fail(cmd, JSONRPC2_INVALID_PARAMS, "experimental-offers disabled"); if (!b12->offer_node_id) - return renepay_fail(renepay, JSONRPC2_INVALID_PARAMS, + return command_fail(cmd, JSONRPC2_INVALID_PARAMS, "invoice missing offer_node_id"); if (!b12->invoice_payment_hash) - return renepay_fail(renepay, JSONRPC2_INVALID_PARAMS, + return command_fail(cmd, JSONRPC2_INVALID_PARAMS, "invoice missing payment_hash"); if (!b12->invoice_created_at) - return renepay_fail(renepay, JSONRPC2_INVALID_PARAMS, + return command_fail(cmd, JSONRPC2_INVALID_PARAMS, "invoice missing created_at"); if (b12->invoice_amount) { invmsat = tal(cmd, struct amount_msat); @@ -1096,25 +1037,24 @@ static struct command_result *json_pay(struct command *cmd, } else invmsat = NULL; - node_id_from_pubkey(&p->destination, b12->offer_node_id); - p->payment_hash = *b12->invoice_payment_hash; - if (b12->invreq_recurrence_counter && !p->label) - return renepay_fail( - renepay, JSONRPC2_INVALID_PARAMS, + node_id_from_pubkey(&destination, b12->offer_node_id); + payment_hash = *b12->invoice_payment_hash; + if (b12->invreq_recurrence_counter && !label) + return command_fail(cmd, JSONRPC2_INVALID_PARAMS, "recurring invoice requires a label"); /* FIXME payment_secret should be signature! */ { struct sha256 merkle; - p->payment_secret = tal(p, struct secret); + payment_secret = tal(cmd, struct secret); merkle_tlv(b12->fields, &merkle); - memcpy(p->payment_secret, &merkle, sizeof(merkle)); - BUILD_ASSERT(sizeof(*p->payment_secret) == + memcpy(payment_secret, &merkle, sizeof(merkle)); + BUILD_ASSERT(sizeof(*payment_secret) == sizeof(merkle)); } - p->payment_metadata = NULL; + payment_metadata = NULL; /* FIXME: blinded paths! */ - p->final_cltv = 18; + final_cltv = 18; /* BOLT-offers #12: * - if `relative_expiry` is present: * - MUST reject the invoice if the current time since @@ -1131,49 +1071,96 @@ static struct command_result *json_pay(struct command *cmd, invexpiry = *b12->invoice_created_at + BOLT12_DEFAULT_REL_EXPIRY; } - if (node_id_eq(&pay_plugin->my_id, &p->destination)) - return renepay_fail(renepay, JSONRPC2_INVALID_PARAMS, + if (node_id_eq(&pay_plugin->my_id, &destination)) + return command_fail(cmd, JSONRPC2_INVALID_PARAMS, "This payment is destined for ourselves. " "Self-payments are not supported"); - // set the payment amount if (invmsat) { // amount is written in the invoice if (msat) { - return renepay_fail(renepay, JSONRPC2_INVALID_PARAMS, + return command_fail(cmd, JSONRPC2_INVALID_PARAMS, "amount_msat parameter unnecessary"); } - p->amount = *invmsat; - tal_free(invmsat); + msat = invmsat; } else { // amount is not written in the invoice if (!msat) { - return renepay_fail(renepay, JSONRPC2_INVALID_PARAMS, + return command_fail(cmd, JSONRPC2_INVALID_PARAMS, "amount_msat parameter required"); } - p->amount = *msat; - tal_free(msat); } /* Default max fee is 5 sats, or 0.5%, whichever is *higher* */ if (!maxfee) { - struct amount_msat fee = amount_msat_div(p->amount, 200); + struct amount_msat fee = amount_msat_div(*msat, 200); if (amount_msat_less(fee, AMOUNT_MSAT(5000))) fee = AMOUNT_MSAT(5000); maxfee = tal_dup(tmpctx, struct amount_msat, &fee); } - if (!amount_msat_add(&p->maxspend, p->amount, *maxfee)) { - return renepay_fail( - renepay, JSONRPC2_INVALID_PARAMS, - "Overflow when computing fee budget, fee far too high."); - } - tal_free(maxfee); - const u64 now_sec = time_now().ts.tv_sec; if (now_sec > invexpiry) - return renepay_fail(renepay, PAY_INVOICE_EXPIRED, "Invoice expired"); + return command_fail(cmd, PAY_INVOICE_EXPIRED, "Invoice expired"); + +#if !DEVELOPER + /* Please renepay try to give me a reliable payment 90% chances of + * success, once you do, then minimize as much as possible those fees. */ + base_fee_penalty = tal(tmpctx, u64); + *base_fee_penalty = 10; + prob_cost_factor = tal(tmpctx, u64); + *prob_cost_factor = 10; + riskfactor_millionths = tal(tmpctx, u64); + *riskfactor_millionths = 1; + min_prob_success_millionths = tal(tmpctx, u64); + *min_prob_success_millionths = 90; + use_shadow = tal(tmpctx, bool); + *use_shadow = 1; +#endif + + /* renepay is bound to the command, if the command finishes renepay is + * freed. */ + struct renepay * renepay = renepay_new(cmd, + take(invstr), + take(label), + take(description), + take(local_offer_id), + take(payment_secret), + take(payment_metadata), + &destination, + &payment_hash, + *msat, + *maxfee, + *maxdelay, + *retryfor, + final_cltv, + *base_fee_penalty, + *prob_cost_factor, + *riskfactor_millionths, + *min_prob_success_millionths, + use_shadow); + tal_add_destructor2(renepay, + renepay_cleanup, + pay_plugin->gossmap); + + /* We inmediately add this payment to the payment list. */ + tal_steal(pay_plugin->ctx, renepay->payment); + list_add_tail(&pay_plugin->payments, &renepay->payment->list); + tal_add_destructor(renepay->payment, destroy_payment); + + plugin_log(pay_plugin->plugin,LOG_DBG,"Starting renepay"); + bool gossmap_changed = gossmap_refresh(pay_plugin->gossmap, NULL); + + if (pay_plugin->gossmap == NULL) + plugin_err(pay_plugin->plugin, "Failed to refresh gossmap: %s", + strerror(errno)); + + /* Free parameters which would be considered "leaks" by our fussy memleak code */ + tal_free(msat); + tal_free(maxfee); + tal_free(maxdelay); + tal_free(retryfor); /* To construct the uncertainty network we need to perform the following * steps: @@ -1220,7 +1207,7 @@ static struct command_result *json_pay(struct command *cmd, payment_listsendpays_previous, payment_listsendpays_previous, renepay); - json_add_sha256(req->js, "payment_hash", &p->payment_hash); + json_add_sha256(req->js, "payment_hash", &renepay->payment->payment_hash); return send_outreq(cmd->plugin, req); } diff --git a/plugins/renepay/payment.c b/plugins/renepay/payment.c index 8eb070b805a7..b9baf5792ad5 100644 --- a/plugins/renepay/payment.c +++ b/plugins/renepay/payment.c @@ -3,7 +3,26 @@ #include #include -struct payment * payment_new(struct renepay * renepay) +static struct payment * payment_new(struct renepay * renepay, + const char *invstr TAKES, + const char *label TAKES, + const char *description TAKES, + const struct sha256 *local_offer_id TAKES, + const struct secret *payment_secret TAKES, + const u8 *payment_metadata TAKES, + const struct node_id *destination, + const struct sha256 *payment_hash, + struct amount_msat amount, + struct amount_msat maxfee, + unsigned int maxdelay, + u64 retryfor, + u16 final_cltv, + /* Tweakable in DEVELOPER mode */ + u64 base_fee_penalty, + u64 prob_cost_factor, + u64 riskfactor_millionths, + u64 min_prob_success_millionths, + bool use_shadow) { struct payment *p = tal(renepay,struct payment); p->renepay = renepay; @@ -12,43 +31,75 @@ struct payment * payment_new(struct renepay * renepay) p->total_sent = AMOUNT_MSAT(0); p->total_delivering = AMOUNT_MSAT(0); - p->invstr=NULL; + p->invstr = tal_strdup(p, invstr); - p->amount = AMOUNT_MSAT(0); - // p->destination= - // p->payment_hash - p->maxspend = AMOUNT_MSAT(0); - p->maxdelay=0; - // p->start_time= - // p->stop_time= + p->amount = amount; + p->destination = *destination; + p->payment_hash = *payment_hash; + if (!amount_msat_add(&p->maxspend, amount, maxfee)) + p->maxspend = AMOUNT_MSAT(UINT64_MAX); + + p->maxdelay = maxdelay; + p->start_time = time_now(); + p->stop_time = timeabs_add(p->start_time, time_from_sec(retryfor)); p->preimage = NULL; - p->payment_secret=NULL; - p->payment_metadata=NULL; + p->payment_secret = tal_dup_or_null(p, struct secret, payment_secret); + p->payment_metadata = tal_dup_talarr(p, u8, payment_metadata); p->status=PAYMENT_PENDING; - p->final_cltv=0; + p->final_cltv=final_cltv; // p->list= - p->description=NULL; - p->label=NULL; + p->description = tal_strdup_or_null(p, description); + p->label = tal_strdup_or_null(p, label); - p->delay_feefactor=0; - p->base_fee_penalty=0; - p->prob_cost_factor=0; - p->min_prob_success=0; + p->delay_feefactor = riskfactor_millionths / 1e6; + p->base_fee_penalty = base_fee_penalty; + p->prob_cost_factor = prob_cost_factor; + p->min_prob_success = min_prob_success_millionths / 1e6; - p->local_offer_id=NULL; - p->use_shadow=true; + p->local_offer_id = tal_dup_or_null(p, struct sha256, local_offer_id); + p->use_shadow = use_shadow; p->groupid=1; p->result = NULL; return p; } -struct renepay * renepay_new(struct command *cmd) +struct renepay *renepay_new(struct command *cmd, + const char *invstr TAKES, + const char *label TAKES, + const char *description TAKES, + const struct sha256 *local_offer_id TAKES, + const struct secret *payment_secret TAKES, + const u8 *payment_metadata TAKES, + const struct node_id *destination, + const struct sha256 *payment_hash, + struct amount_msat amount, + struct amount_msat maxfee, + unsigned int maxdelay, + u64 retryfor, + u16 final_cltv, + /* Tweakable in DEVELOPER mode */ + u64 base_fee_penalty, + u64 prob_cost_factor, + u64 riskfactor_millionths, + u64 min_prob_success_millionths, + bool use_shadow) { struct renepay *renepay = tal(cmd,struct renepay); renepay->cmd = cmd; - renepay->payment = payment_new(renepay); + renepay->payment = payment_new(renepay, + invstr, label, description, + local_offer_id, payment_secret, payment_metadata, + destination, payment_hash, + amount, maxfee, maxdelay, + retryfor, final_cltv, + base_fee_penalty, + prob_cost_factor, + riskfactor_millionths, + min_prob_success_millionths, + use_shadow); + renepay->local_gossmods = gossmap_localmods_new(renepay); renepay->disabled = tal_arr(renepay,struct short_channel_id,0); renepay->rexmit_timer = NULL; diff --git a/plugins/renepay/payment.h b/plugins/renepay/payment.h index 471fda906a0b..b59eaad95f9c 100644 --- a/plugins/renepay/payment.h +++ b/plugins/renepay/payment.h @@ -123,8 +123,27 @@ struct renepay u64 next_partid; }; -struct payment * payment_new(struct renepay *renepay); -struct renepay * renepay_new(struct command *cmd); +struct renepay *renepay_new(struct command *cmd, + const char *invstr TAKES, + const char *label TAKES, + const char *description TAKES, + const struct sha256 *local_offer_id TAKES, + const struct secret *payment_secret TAKES, + const u8 *payment_metadata TAKES, + const struct node_id *destination, + const struct sha256 *payment_hash, + struct amount_msat amount, + struct amount_msat maxfee, + unsigned int maxdelay, + u64 retryfor, + u16 final_cltv, + /* Tweakable in DEVELOPER mode */ + u64 base_fee_penalty, + u64 prob_cost_factor, + u64 riskfactor_millionths, + u64 min_prob_success_millionths, + bool use_shadow); + void renepay_cleanup( struct renepay * renepay, struct gossmap * gossmap); From 52260c6ec6b38ed060efe20806e1c7d5325c239d Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Thu, 10 Aug 2023 09:30:45 +0930 Subject: [PATCH 05/22] renepay: move list_node to first member of struct payment. Results in payments having a pointer to the start of the object, which helps our memleak code. Signed-off-by: Rusty Russell --- plugins/renepay/payment.h | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/plugins/renepay/payment.h b/plugins/renepay/payment.h index b59eaad95f9c..159028369627 100644 --- a/plugins/renepay/payment.h +++ b/plugins/renepay/payment.h @@ -10,6 +10,9 @@ enum payment_status { struct payment { + /* Inside pay_plugin->payments list */ + struct list_node list; + struct renepay * renepay; /* Chatty description of attempts. */ @@ -56,9 +59,6 @@ struct payment { u32 final_cltv; - /* Inside pay_plugin->payments list */ - struct list_node list; - /* Description and labels, if any. */ const char *description, *label; From dccf4874b6a673df14d03aba3bc6850dc7eacfbe Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Thu, 10 Aug 2023 09:30:45 +0930 Subject: [PATCH 06/22] renepay: make memleak simpler. Simply tell it to scan the entire object. Signed-off-by: Rusty Russell --- plugins/renepay/pay.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/plugins/renepay/pay.c b/plugins/renepay/pay.c index a8d1219a4243..a13e7c19d6b8 100644 --- a/plugins/renepay/pay.c +++ b/plugins/renepay/pay.c @@ -65,9 +65,7 @@ void amount_msat_reduce_(struct amount_msat *dst, #if DEVELOPER static void memleak_mark(struct plugin *p, struct htable *memtable) { - memleak_scan_obj(memtable, pay_plugin->ctx); - memleak_scan_obj(memtable, pay_plugin->gossmap); - memleak_scan_obj(memtable, pay_plugin->chan_extra_map); + memleak_scan_region(memtable, pay_plugin, sizeof(*pay_plugin)); memleak_scan_htable(memtable, &pay_plugin->chan_extra_map->raw); } #endif From 698f415fc364fe16e6bc22024bc72d70b1225492 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Thu, 10 Aug 2023 09:31:08 +0930 Subject: [PATCH 07/22] renepay: merge `struct renepay` and `struct payment` into one. There are a few fields in `struct renepay` which are genuinely transient, but it makes the code much harder to follow than simply having a single structure. More cleanups will follow, but this is the minimal set. Signed-off-by: Rusty Russell --- plugins/renepay/pay.c | 231 +++++++++++++------------- plugins/renepay/pay_flow.c | 28 ++-- plugins/renepay/pay_flow.h | 2 +- plugins/renepay/payment.c | 120 ++++--------- plugins/renepay/payment.h | 56 +++---- plugins/renepay/uncertainty_network.c | 24 ++- plugins/renepay/uncertainty_network.h | 4 +- 7 files changed, 194 insertions(+), 271 deletions(-) diff --git a/plugins/renepay/pay.c b/plugins/renepay/pay.c index a13e7c19d6b8..c91c96327975 100644 --- a/plugins/renepay/pay.c +++ b/plugins/renepay/pay.c @@ -32,9 +32,9 @@ static struct pay_plugin the_pay_plugin; struct pay_plugin * const pay_plugin = &the_pay_plugin; -static void timer_kick(struct renepay * renepay); +static void timer_kick(struct payment *payment); static struct command_result *try_paying(struct command *cmd, - struct renepay * renepay, + struct payment *payment, bool first_time); void amount_msat_accumulate_(struct amount_msat *dst, @@ -126,56 +126,54 @@ static const char *init(struct plugin *p, } -static void renepay_settimer(struct renepay * renepay) +static void payment_settimer(struct payment *payment) { - renepay->rexmit_timer = tal_free(renepay->rexmit_timer); - renepay->rexmit_timer = plugin_timer( + payment->rexmit_timer = tal_free(payment->rexmit_timer); + payment->rexmit_timer = plugin_timer( pay_plugin->plugin, time_from_msec(TIMER_COLLECT_FAILURES_MSEC), - timer_kick, renepay); + timer_kick, payment); } /* Happens when timer goes off, but also works to arm timer if nothing to do */ -static void timer_kick(struct renepay * renepay) +static void timer_kick(struct payment *payment) { - struct payment * const p = renepay->payment; plugin_log(pay_plugin->plugin,LOG_DBG,"calling %s",__PRETTY_FUNCTION__); - switch(p->status) + switch (payment->status) { /* Some flows succeeded, we finish the payment. */ case PAYMENT_SUCCESS: plugin_log(pay_plugin->plugin,LOG_DBG,"status is PAYMENT_SUCCESS"); - renepay_success(renepay); + payment_success(payment); break; /* Some flows failed, we retry. */ case PAYMENT_FAIL: plugin_log(pay_plugin->plugin,LOG_DBG,"status is PAYMENT_FAIL"); - payment_assert_delivering_incomplete(p); - try_paying(renepay->cmd,renepay,/* always try even if prob is low */ true); + payment_assert_delivering_incomplete(payment); + try_paying(payment->cmd,payment,/* always try even if prob is low */ true); break; /* Nothing has returned yet, we have to wait. */ case PAYMENT_PENDING: plugin_log(pay_plugin->plugin,LOG_DBG,"status is PAYMENT_PENDING"); - payment_assert_delivering_all(p); - renepay_settimer(renepay); + payment_assert_delivering_all(payment); + payment_settimer(payment); break; } } /* Sometimes we don't know exactly who to blame... */ -static struct command_result *handle_unhandleable_error(struct renepay * renepay, +static struct command_result *handle_unhandleable_error(struct payment *payment, struct pay_flow *flow, const char *what) { - struct payment * const p = renepay->payment; plugin_log(pay_plugin->plugin,LOG_DBG,"calling %s",__PRETTY_FUNCTION__); size_t n = tal_count(flow); /* We got a mangled reply. We don't know who to penalize! */ - debug_paynote(p, "%s on route %s", what, flow_path_to_str(tmpctx, flow)); + debug_paynote(payment, "%s on route %s", what, flow_path_to_str(tmpctx, flow)); // TODO(eduardo): does LOG_BROKEN finish the plugin execution? plugin_log(pay_plugin->plugin, LOG_BROKEN, @@ -185,7 +183,7 @@ static struct command_result *handle_unhandleable_error(struct renepay * renepay if (n == 1) { payflow_fail(flow); - return renepay_fail(renepay, PAY_UNPARSEABLE_ONION, + return payment_fail(payment, PAY_UNPARSEABLE_ONION, "Got %s from the destination", what); } /* FIXME: check chan_extra_map, since we might have succeeded though @@ -199,8 +197,8 @@ static struct command_result *handle_unhandleable_error(struct renepay * renepay /* Assume it's not the destination */ n = pseudorand(n-1); - tal_arr_expand(&renepay->disabled, flow->path_scids[n]); - debug_paynote(p, "... eliminated %s", + tal_arr_expand(&payment->disabled, flow->path_scids[n]); + debug_paynote(payment, "... eliminated %s", type_to_string(tmpctx, struct short_channel_id, &flow->path_scids[n])); return NULL; @@ -219,13 +217,13 @@ static struct command_result *addgossip_done(struct command *cmd, struct addgossip *adg) { plugin_log(pay_plugin->plugin,LOG_DBG,"calling %s",__PRETTY_FUNCTION__); - struct renepay * renepay = adg->flow->payment->renepay; + struct payment * payment = adg->flow->payment; /* Release this: if it's the last flow we'll retry immediately */ payflow_fail(adg->flow); tal_free(adg); - renepay_settimer(renepay); + payment_settimer(payment); return command_still_pending(cmd); } @@ -236,14 +234,13 @@ static struct command_result *addgossip_failure(struct command *cmd, struct addgossip *adg) { + struct payment * payment = adg->flow->payment; plugin_log(pay_plugin->plugin,LOG_DBG,"calling %s",__PRETTY_FUNCTION__); - struct payment * p = adg->flow->payment; - struct renepay * renepay = p->renepay; - debug_paynote(p, "addgossip failed, removing channel %s (%.*s)", + debug_paynote(payment, "addgossip failed, removing channel %s (%.*s)", type_to_string(tmpctx, struct short_channel_id, &adg->scid), err->end - err->start, buf + err->start); - tal_arr_expand(&renepay->disabled, adg->scid); + tal_arr_expand(&payment->disabled, adg->scid); return addgossip_done(cmd, buf, err, adg); } @@ -254,8 +251,7 @@ static struct command_result *submit_update(struct command *cmd, struct short_channel_id errscid) { plugin_log(pay_plugin->plugin,LOG_DBG,"calling %s",__PRETTY_FUNCTION__); - struct payment * p = flow->payment; - struct renepay * renepay = p->renepay; + struct payment *payment = flow->payment; struct out_req *req; struct addgossip *adg = tal(cmd, struct addgossip); @@ -264,10 +260,9 @@ static struct command_result *submit_update(struct command *cmd, adg->scid = errscid; adg->flow = flow; /* Disable re-xmit until this returns */ - renepay->rexmit_timer - = tal_free(renepay->rexmit_timer); + payment->rexmit_timer = tal_free(payment->rexmit_timer); - debug_paynote(p, "... extracted channel_update, telling gossipd"); + debug_paynote(payment, "... extracted channel_update, telling gossipd"); plugin_log(pay_plugin->plugin, LOG_DBG, "(update = %s)", tal_hex(tmpctx, update)); req = jsonrpc_request_start(pay_plugin->plugin, NULL, "addgossip", @@ -357,13 +352,12 @@ static struct command_result *flow_sendpay_failed(struct command *cmd, { plugin_log(pay_plugin->plugin,LOG_DBG,"calling %s",__PRETTY_FUNCTION__); - struct payment *p = flow->payment; - debug_assert(p); - struct renepay * renepay = p->renepay; - debug_assert(renepay); + struct payment *payment = flow->payment; + debug_assert(payment); /* This is a fail. */ - payment_fail(p); + if (payment->status != PAYMENT_SUCCESS) + payment->status=PAYMENT_FAIL; u64 errcode; const jsmntok_t *msg = json_get_member(buf, err, "message"); @@ -376,13 +370,13 @@ static struct command_result *flow_sendpay_failed(struct command *cmd, plugin_err(cmd->plugin, "Strange error from sendpay: %.*s", json_tok_full_len(err), json_tok_full(buf, err)); - debug_paynote(p, + debug_paynote(payment, "sendpay didn't like first hop, eliminated: %.*s", msg->end - msg->start, buf + msg->start); /* There is no new knowledge from this kind of failure. * We just disable this scid. */ - tal_arr_expand(&renepay->disabled, flow->path_scids[0]); + tal_arr_expand(&payment->disabled, flow->path_scids[0]); payflow_fail(flow); return command_still_pending(cmd); @@ -391,11 +385,9 @@ static struct command_result *flow_sendpay_failed(struct command *cmd, static struct command_result * sendpay_flows(struct command *cmd, - struct renepay * renepay, + struct payment *p, struct pay_flow **flows STEALS) { - struct payment * const p = renepay->payment; - plugin_log(pay_plugin->plugin,LOG_DBG,"calling %s",__PRETTY_FUNCTION__); debug_paynote(p, "Sending out batch of %zu payments", tal_count(flows)); @@ -476,44 +468,43 @@ sendpay_flows(struct command *cmd, tal_free(flows); /* Get ready to process replies */ - renepay_settimer(renepay); + payment_settimer(p); return command_still_pending(cmd); } static struct command_result *try_paying(struct command *cmd, - struct renepay *renepay, + struct payment *payment, bool first_time) { - struct payment * const p = renepay->payment; plugin_log(pay_plugin->plugin,LOG_DBG,"calling %s",__PRETTY_FUNCTION__); struct amount_msat feebudget, fees_spent, remaining; - renepay->payment->status=PAYMENT_PENDING; - if (time_after(time_now(), p->stop_time)) - return renepay_fail(renepay, PAY_STOPPED_RETRYING, "Timed out"); + payment->status = PAYMENT_PENDING; + if (time_after(time_now(), payment->stop_time)) + return payment_fail(payment, PAY_STOPPED_RETRYING, "Timed out"); /* Total feebudget */ - if (!amount_msat_sub(&feebudget, p->maxspend, p->amount)) + if (!amount_msat_sub(&feebudget, payment->maxspend, payment->amount)) { plugin_err(pay_plugin->plugin, "%s (line %d) could not substract maxspend=%s and amount=%s.", __PRETTY_FUNCTION__, __LINE__, - type_to_string(tmpctx, struct amount_msat, &p->maxspend), - type_to_string(tmpctx, struct amount_msat, &p->amount)); + type_to_string(tmpctx, struct amount_msat, &payment->maxspend), + type_to_string(tmpctx, struct amount_msat, &payment->amount)); } /* Fees spent so far */ - if (!amount_msat_sub(&fees_spent, p->total_sent, p->total_delivering)) + if (!amount_msat_sub(&fees_spent, payment->total_sent, payment->total_delivering)) { plugin_err(pay_plugin->plugin, "%s (line %d) could not substract total_sent=%s and total_delivering=%s.", __PRETTY_FUNCTION__, __LINE__, - type_to_string(tmpctx, struct amount_msat, &p->total_sent), - type_to_string(tmpctx, struct amount_msat, &p->total_delivering)); + type_to_string(tmpctx, struct amount_msat, &payment->total_sent), + type_to_string(tmpctx, struct amount_msat, &payment->total_delivering)); } /* Remaining fee budget. */ @@ -528,14 +519,14 @@ static struct command_result *try_paying(struct command *cmd, } /* How much are we still trying to send? */ - if (!amount_msat_sub(&remaining, p->amount, p->total_delivering)) + if (!amount_msat_sub(&remaining, payment->amount, payment->total_delivering)) { plugin_err(pay_plugin->plugin, "%s (line %d) could not substract amount=%s and total_delivering=%s.", __PRETTY_FUNCTION__, __LINE__, - type_to_string(tmpctx, struct amount_msat, &p->amount), - type_to_string(tmpctx, struct amount_msat, &p->total_delivering)); + type_to_string(tmpctx, struct amount_msat, &payment->amount), + type_to_string(tmpctx, struct amount_msat, &payment->total_delivering)); } // plugin_log(pay_plugin->plugin,LOG_DBG,fmt_chan_extra_map(tmpctx,pay_plugin->chan_extra_map)); @@ -544,9 +535,9 @@ static struct command_result *try_paying(struct command *cmd, /* We let this return an unlikely path, as it's better to try once * than simply refuse. Plus, models are not truth! */ - gossmap_apply_localmods(pay_plugin->gossmap, renepay->local_gossmods); + gossmap_apply_localmods(pay_plugin->gossmap, payment->local_gossmods); struct pay_flow **pay_flows = get_payflows( - renepay, + payment, remaining, feebudget, /* would you accept unlikely @@ -554,46 +545,59 @@ static struct command_result *try_paying(struct command *cmd, true, /* is entire payment? */ - amount_msat_eq(p->total_delivering, AMOUNT_MSAT(0)), + amount_msat_eq(payment->total_delivering, AMOUNT_MSAT(0)), &err_msg); - gossmap_remove_localmods(pay_plugin->gossmap, renepay->local_gossmods); + gossmap_remove_localmods(pay_plugin->gossmap, payment->local_gossmods); // plugin_log(pay_plugin->plugin,LOG_DBG,"get_payflows produced %s",fmt_payflows(tmpctx,pay_flows)); /* MCF cannot find a feasible route, we stop. */ if (!pay_flows) { - return renepay_fail(renepay, PAY_ROUTE_NOT_FOUND, + return payment_fail(payment, PAY_ROUTE_NOT_FOUND, "Failed to find a route, %s", err_msg); } /* Now begin making payments */ - return sendpay_flows(cmd, renepay, pay_flows); + return sendpay_flows(cmd, payment, pay_flows); +} + +static void destroy_cmd_payment_ptr(struct command *cmd, + struct payment *payment) +{ + assert(payment->cmd == cmd); + payment->cmd = NULL; } static struct command_result *listpeerchannels_done( struct command *cmd, const char *buf, const jsmntok_t *result, - struct renepay *renepay) + struct payment *payment) { plugin_log(pay_plugin->plugin,LOG_DBG,"calling %s",__PRETTY_FUNCTION__); if (!uncertainty_network_update_from_listpeerchannels( pay_plugin->chan_extra_map, pay_plugin->my_id, - renepay, + payment, buf, result)) - return renepay_fail(renepay,LIGHTNINGD, + return command_fail(cmd, LIGHTNINGD, "listpeerchannels malformed: %.*s", json_tok_full_len(result), json_tok_full(buf, result)); // TODO(eduardo): check that there won't be a prob. cost associated with // any gossmap local chan. The same way there aren't fees to pay for my // local channels. - return try_paying(cmd, renepay, true); + + /* From now on, we keep a record of the payment, so persist it beyond this cmd. */ + tal_steal(pay_plugin->plugin, payment); + /* When we terminate cmd for any reason, clear it from payment so we don't do it again. */ + tal_add_destructor2(cmd, destroy_cmd_payment_ptr, payment); + + return try_paying(cmd, payment, true); } @@ -685,10 +689,9 @@ payment_listsendpays_previous( struct command *cmd, const char *buf, const jsmntok_t *result, - struct renepay * renepay) + struct payment * payment) { debug_info("calling %s",__PRETTY_FUNCTION__); - struct payment * p = renepay->payment; size_t i; const jsmntok_t *t, *arr, *err; @@ -841,25 +844,23 @@ payment_listsendpays_previous( json_add_string(ret, "status", "complete"); json_add_amount_msat(ret, "amount_msat", complete_msat); json_add_amount_msat(ret, "amount_sent_msat",complete_sent); - json_add_node_id(ret, "destination", &p->destination); - json_add_sha256(ret, "payment_hash", &p->payment_hash); + json_add_node_id(ret, "destination", &payment->destination); + json_add_sha256(ret, "payment_hash", &payment->payment_hash); json_add_u32(ret, "created_at", complete_created_at); json_add_num(ret, "parts", complete_parts); /* This payment was already completed, we don't keep record of - * it twice. */ - renepay->payment = tal_free(renepay->payment); - + * it twice: payment will be freed with cmd */ return command_finished(cmd, ret); } else if (pending) { assert(last_pending_group_id!=INVALID_ID); assert(first_pending_group_id!=INVALID_ID); - p->groupid = last_pending_group_id; - renepay->next_partid = last_pending_partid+1; + payment->groupid = last_pending_group_id; + payment->next_partid = last_pending_partid+1; - p->total_sent = pending_sent; - p->total_delivering = pending_msat; + payment->total_sent = pending_sent; + payment->total_delivering = pending_msat; plugin_log(pay_plugin->plugin,LOG_DBG, "There are pending sendpays to this invoice. " @@ -867,31 +868,29 @@ payment_listsendpays_previous( "delivering = %s, " "last_partid = %"PRIu64, last_pending_group_id, - type_to_string(tmpctx,struct amount_msat,&p->total_delivering), + type_to_string(tmpctx,struct amount_msat,&payment->total_delivering), last_pending_partid); if( first_pending_group_id != last_pending_group_id) { /* At least two pending groups for the same invoice, * this is weird, we better stop. */ - renepay->payment = tal_free(renepay->payment); - return renepay_fail(renepay, PAY_IN_PROGRESS, + return command_fail(cmd, PAY_IN_PROGRESS, "Payment is pending by some other request."); } - if(amount_msat_greater_eq(p->total_delivering,p->amount)) + if(amount_msat_greater_eq(payment->total_delivering,payment->amount)) { /* Pending payment already pays the full amount, we * better stop. */ - renepay->payment = tal_free(renepay->payment); - return renepay_fail(renepay, PAY_IN_PROGRESS, + return command_fail(cmd, PAY_IN_PROGRESS, "Payment is pending with full amount already commited"); } }else { /* There are no pending nor completed sendpays, get me the last * sendpay group. */ - p->groupid = (last_group==INVALID_ID ? 1 : (last_group+1)) ; - renepay->next_partid=1; + payment->groupid = (last_group==INVALID_ID ? 1 : (last_group+1)) ; + payment->next_partid=1; } @@ -899,7 +898,7 @@ payment_listsendpays_previous( /* Get local capacities... */ req = jsonrpc_request_start(cmd->plugin, cmd, "listpeerchannels", listpeerchannels_done, - listpeerchannels_done, renepay); + listpeerchannels_done, payment); return send_outreq(cmd->plugin, req); } @@ -1117,9 +1116,12 @@ static struct command_result *json_pay(struct command *cmd, *use_shadow = 1; #endif - /* renepay is bound to the command, if the command finishes renepay is - * freed. */ - struct renepay * renepay = renepay_new(cmd, + /* Payment is allocated off cmd to start, in case we fail cmd + * (e.g. already in progress, already succeeded). Once it's + * actually started, it persists beyond the command, so we + * tal_steal. */ + struct payment *payment = payment_new(cmd, + cmd, take(invstr), take(label), take(description), @@ -1138,14 +1140,10 @@ static struct command_result *json_pay(struct command *cmd, *riskfactor_millionths, *min_prob_success_millionths, use_shadow); - tal_add_destructor2(renepay, - renepay_cleanup, - pay_plugin->gossmap); - /* We inmediately add this payment to the payment list. */ - tal_steal(pay_plugin->ctx, renepay->payment); - list_add_tail(&pay_plugin->payments, &renepay->payment->list); - tal_add_destructor(renepay->payment, destroy_payment); + /* We immediately add this payment to the payment list. */ + list_add_tail(&pay_plugin->payments, &payment->list); + tal_add_destructor(payment, destroy_payment); plugin_log(pay_plugin->plugin,LOG_DBG,"Starting renepay"); bool gossmap_changed = gossmap_refresh(pay_plugin->gossmap, NULL); @@ -1191,7 +1189,7 @@ static struct command_result *json_pay(struct command *cmd, // TODO(eduardo): are there route hints for B12? // Add any extra hidden channel revealed by the routehints to the uncertainty network. if(invstr_is_b11) - uncertainty_network_add_routehints(pay_plugin->chan_extra_map,renepay); + uncertainty_network_add_routehints(pay_plugin->chan_extra_map, payment); if(!uncertainty_network_check_invariants(pay_plugin->chan_extra_map)) plugin_log(pay_plugin->plugin, @@ -1203,9 +1201,9 @@ static struct command_result *json_pay(struct command *cmd, struct out_req *req = jsonrpc_request_start(cmd->plugin, cmd, "listsendpays", payment_listsendpays_previous, - payment_listsendpays_previous, renepay); + payment_listsendpays_previous, payment); - json_add_sha256(req->js, "payment_hash", &renepay->payment->payment_hash); + json_add_sha256(req->js, "payment_hash", &payment->payment_hash); return send_outreq(cmd->plugin, req); } @@ -1213,12 +1211,10 @@ static void handle_sendpay_failure_renepay( struct command *cmd, const char *buf, const jsmntok_t *result, - struct renepay *renepay, + struct payment *p, struct pay_flow *flow) { - debug_assert(renepay); debug_assert(flow); - struct payment *p = renepay->payment; debug_assert(p); u64 errcode; @@ -1249,11 +1245,11 @@ static void handle_sendpay_failure_renepay( case PAY_TRY_OTHER_ROUTE: break; case PAY_DESTINATION_PERM_FAIL: - renepay_fail(renepay,errcode, + payment_fail(p, errcode, "Got a final failure from destination"); return; default: - renepay_fail(renepay,errcode, + payment_fail(p, errcode, "Unexpected errocode from sendpay_failure: %.*s", json_tok_full_len(result), json_tok_full(buf,result)); @@ -1315,7 +1311,7 @@ static void handle_sendpay_failure_renepay( { // TODO(eduardo): I wonder which error code should I show the // user in this case? - renepay_fail(renepay,LIGHTNINGD, + payment_fail(p,LIGHTNINGD, "Failed to get failcode from sendpay_failure notification" ", received json: %.*s", json_tok_full_len(result), @@ -1355,7 +1351,7 @@ static void handle_sendpay_failure_renepay( case WIRE_EXPIRY_TOO_FAR: debug_paynote(p, "we're removing scid %s", type_to_string(tmpctx,struct short_channel_id,&errscid)); - tal_arr_expand(&renepay->disabled, errscid); + tal_arr_expand(&p->disabled, errscid); return; /* These can be fixed (maybe) by applying the included channel_update */ @@ -1375,7 +1371,7 @@ static void handle_sendpay_failure_renepay( debug_paynote(p, "missing an update, so we're removing scid %s", type_to_string(tmpctx,struct short_channel_id,&errscid)); - tal_arr_expand(&renepay->disabled, errscid); + tal_arr_expand(&p->disabled, errscid); return; case WIRE_TEMPORARY_CHANNEL_FAILURE: @@ -1387,7 +1383,7 @@ static void handle_sendpay_failure_renepay( case WIRE_FINAL_INCORRECT_CLTV_EXPIRY: case WIRE_FINAL_INCORRECT_HTLC_AMOUNT: debug_paynote(p,"final destination failure"); - renepay_fail(renepay,errcode, + payment_fail(p,errcode, "Destination said %s: %s", onion_wire_name(onionerr), message); @@ -1400,7 +1396,7 @@ static void handle_sendpay_failure_renepay( { debug_paynote(p,"unkown onion error code %u, fatal", onionerr); - renepay_fail(renepay,errcode, + payment_fail(p,errcode, "Destination gave unknown error code %u: %s", onionerr,message); return; @@ -1409,12 +1405,12 @@ static void handle_sendpay_failure_renepay( debug_paynote(p,"unkown onion error code %u, removing scid %s", onionerr, type_to_string(tmpctx,struct short_channel_id,&errscid)); - tal_arr_expand(&renepay->disabled, errscid); + tal_arr_expand(&p->disabled, errscid); return; } unhandleable: // TODO(eduardo): check - handle_unhandleable_error(renepay, flow, ""); + handle_unhandleable_error(p, flow, ""); } static void handle_sendpay_failure_flow( @@ -1428,7 +1424,8 @@ static void handle_sendpay_failure_flow( debug_assert(flow); struct payment * const p = flow->payment; - payment_fail(p); + if (p->status != PAYMENT_SUCCESS) + p->status=PAYMENT_FAIL; u64 errcode; if (!json_to_u64(buf, json_get_member(buf, result, "code"), &errcode)) @@ -1620,8 +1617,7 @@ static struct command_result *notification_sendpay_success( // 3. mark as success struct payment * const p = flow->payment; debug_assert(p); - - payment_success(p); + p->status = PAYMENT_SUCCESS; const jsmntok_t *preimagetok = json_get_member(buf, resulttok, "payment_preimage"); @@ -1633,6 +1629,8 @@ static struct command_result *notification_sendpay_success( json_tok_full_len(params), json_tok_full(buf,params)); + /* We could have preimage from previous part */ + tal_free(p->preimage); p->preimage = tal_dup_or_null(p,struct preimage,&preimage); // 4. update information and release pending HTLCs @@ -1706,10 +1704,7 @@ static struct command_result *notification_sendpay_failure( handle_sendpay_failure_flow(cmd,buf,resulttok,flow); // there is possibly a pending renepay command for this flow - struct renepay * const renepay = flow->payment->renepay; - - if(renepay) - handle_sendpay_failure_renepay(cmd,buf,resulttok,renepay,flow); + handle_sendpay_failure_renepay(cmd,buf,resulttok,flow->payment,flow); done: if(flow) payflow_fail(flow); diff --git a/plugins/renepay/pay_flow.c b/plugins/renepay/pay_flow.c index 91c691ab4fe3..0776dc3266b4 100644 --- a/plugins/renepay/pay_flow.c +++ b/plugins/renepay/pay_flow.c @@ -137,11 +137,10 @@ static u64 flow_delay(const struct flow *flow) /* This enhances f->amounts, and returns per-flow cltvs */ static u32 *shadow_additions(const tal_t *ctx, const struct gossmap *gossmap, - struct renepay *renepay, + struct payment *p, struct flow **flows, bool is_entire_payment) { - struct payment * p = renepay->payment; u32 *final_cltvs; /* Set these up now in case we decide to do nothing */ @@ -279,12 +278,11 @@ static u64 flows_worst_delay(struct flow **flows) } /* FIXME: If only path has channels marked disabled, we should try... */ -static bool disable_htlc_violations_oneflow(struct renepay * renepay, +static bool disable_htlc_violations_oneflow(struct payment *p, const struct flow *flow, const struct gossmap *gossmap, bitmap *disabled) { - struct payment * p = renepay->payment; bool disabled_some = false; for (size_t i = 0; i < tal_count(flow->path); i++) { @@ -307,7 +305,7 @@ static bool disable_htlc_violations_oneflow(struct renepay * renepay, reason); /* Add this for future searches for this payment. */ - tal_arr_expand(&renepay->disabled, scid); + tal_arr_expand(&p->disabled, scid); /* Add to existing bitmap */ bitmap_set_bit(disabled, gossmap_chan_idx(gossmap, flow->path[i])); @@ -318,7 +316,7 @@ static bool disable_htlc_violations_oneflow(struct renepay * renepay, /* If we can't use one of these flows because we hit limits, we disable that * channel for future searches and return false */ -static bool disable_htlc_violations(struct renepay *renepay, +static bool disable_htlc_violations(struct payment *payment, struct flow **flows, const struct gossmap *gossmap, bitmap *disabled) @@ -327,7 +325,7 @@ static bool disable_htlc_violations(struct renepay *renepay, /* We continue through all of them, to disable many at once. */ for (size_t i = 0; i < tal_count(flows); i++) { - disabled_some |= disable_htlc_violations_oneflow(renepay, flows[i], + disabled_some |= disable_htlc_violations_oneflow(payment, flows[i], gossmap, disabled); } @@ -335,7 +333,7 @@ static bool disable_htlc_violations(struct renepay *renepay, } /* Get some payment flows to get this amount to destination, or NULL. */ -struct pay_flow **get_payflows(struct renepay * renepay, +struct pay_flow **get_payflows(struct payment *p, struct amount_msat amount, struct amount_msat feebudget, bool unlikely_ok, @@ -344,12 +342,11 @@ struct pay_flow **get_payflows(struct renepay * renepay, { *err_msg = tal_fmt(tmpctx,"[no error]"); - struct payment * p = renepay->payment; bitmap *disabled; struct pay_flow **pay_flows; const struct gossmap_node *src, *dst; - disabled = make_disabled_bitmap(tmpctx, pay_plugin->gossmap, renepay->disabled); + disabled = make_disabled_bitmap(tmpctx, pay_plugin->gossmap, p->disabled); src = gossmap_find_node(pay_plugin->gossmap, &pay_plugin->my_id); if (!src) { debug_paynote(p, "We don't have any channels?"); @@ -453,7 +450,7 @@ struct pay_flow **get_payflows(struct renepay * renepay, * to do this inside minflow(), but the diagnostics here * are far better, since we can report min/max which * *actually* made us reconsider. */ - if (disable_htlc_violations(renepay, flows, pay_plugin->gossmap, + if (disable_htlc_violations(p, flows, pay_plugin->gossmap, disabled)) { continue; // retry @@ -462,12 +459,12 @@ struct pay_flow **get_payflows(struct renepay * renepay, /* This can adjust amounts and final cltv for each flow, * to make it look like it's going elsewhere */ final_cltvs = shadow_additions(tmpctx, pay_plugin->gossmap, - renepay, flows, is_entire_payment); + p, flows, is_entire_payment); /* OK, we are happy with these flows: convert to * pay_flows to outlive the current gossmap. */ - pay_flows = flows_to_pay_flows(renepay->payment, pay_plugin->gossmap, + pay_flows = flows_to_pay_flows(p, pay_plugin->gossmap, flows, final_cltvs, - &renepay->next_partid); + &p->next_partid); break; } @@ -628,7 +625,8 @@ struct pay_flow* payflow_fail(struct pay_flow *flow) struct payment * p = flow->payment; debug_assert(p); - payment_fail(p); + if (p->status != PAYMENT_SUCCESS) + p->status = PAYMENT_FAIL; amount_msat_reduce(&p->total_delivering, payflow_delivered(flow)); amount_msat_reduce(&p->total_sent, flow->amounts[0]); diff --git a/plugins/renepay/pay_flow.h b/plugins/renepay/pay_flow.h index 1a6e9b73c4f5..bda8564e0391 100644 --- a/plugins/renepay/pay_flow.h +++ b/plugins/renepay/pay_flow.h @@ -78,7 +78,7 @@ HTABLE_DEFINE_TYPE(struct pay_flow, payflow_map); -struct pay_flow **get_payflows(struct renepay * renepay, +struct pay_flow **get_payflows(struct payment *payment, struct amount_msat amount, struct amount_msat feebudget, bool unlikely_ok, diff --git a/plugins/renepay/payment.c b/plugins/renepay/payment.c index b9baf5792ad5..5216b808cbc5 100644 --- a/plugins/renepay/payment.c +++ b/plugins/renepay/payment.c @@ -3,7 +3,8 @@ #include #include -static struct payment * payment_new(struct renepay * renepay, +struct payment *payment_new(const tal_t *ctx, + struct command *cmd, const char *invstr TAKES, const char *label TAKES, const char *description TAKES, @@ -24,8 +25,8 @@ static struct payment * payment_new(struct renepay * renepay, u64 min_prob_success_millionths, bool use_shadow) { - struct payment *p = tal(renepay,struct payment); - p->renepay = renepay; + struct payment *p = tal(ctx,struct payment); + p->cmd = cmd; p->paynotes = tal_arr(p, const char *, 0); p->total_sent = AMOUNT_MSAT(0); @@ -61,64 +62,12 @@ static struct payment * payment_new(struct renepay * renepay, p->groupid=1; p->result = NULL; - return p; -} - -struct renepay *renepay_new(struct command *cmd, - const char *invstr TAKES, - const char *label TAKES, - const char *description TAKES, - const struct sha256 *local_offer_id TAKES, - const struct secret *payment_secret TAKES, - const u8 *payment_metadata TAKES, - const struct node_id *destination, - const struct sha256 *payment_hash, - struct amount_msat amount, - struct amount_msat maxfee, - unsigned int maxdelay, - u64 retryfor, - u16 final_cltv, - /* Tweakable in DEVELOPER mode */ - u64 base_fee_penalty, - u64 prob_cost_factor, - u64 riskfactor_millionths, - u64 min_prob_success_millionths, - bool use_shadow) -{ - struct renepay *renepay = tal(cmd,struct renepay); - - renepay->cmd = cmd; - renepay->payment = payment_new(renepay, - invstr, label, description, - local_offer_id, payment_secret, payment_metadata, - destination, payment_hash, - amount, maxfee, maxdelay, - retryfor, final_cltv, - base_fee_penalty, - prob_cost_factor, - riskfactor_millionths, - min_prob_success_millionths, - use_shadow); - - renepay->local_gossmods = gossmap_localmods_new(renepay); - renepay->disabled = tal_arr(renepay,struct short_channel_id,0); - renepay->rexmit_timer = NULL; - renepay->next_partid=1; - - return renepay; -} - + p->local_gossmods = gossmap_localmods_new(p); + p->disabled = tal_arr(p,struct short_channel_id,0); + p->rexmit_timer = NULL; + p->next_partid=1; -void payment_fail(struct payment * p) -{ - /* If the payment already succeeded this function call must correspond - * to an old sendpay. */ - if(p->status == PAYMENT_SUCCESS)return; - p->status=PAYMENT_FAIL; -} -void payment_success(struct payment * p) -{ - p->status=PAYMENT_SUCCESS; + return p; } struct amount_msat payment_sent(const struct payment *p) @@ -180,22 +129,25 @@ void payment_assert_delivering_all(const struct payment *p) } } -struct command_result *renepay_success(struct renepay * renepay) +struct command_result *payment_success(struct payment *p) { debug_info("calling %s",__PRETTY_FUNCTION__); - struct payment *p = renepay->payment; - payment_success(p); + p->status = PAYMENT_SUCCESS; payment_assert_delivering_all(p); + /* We only finish command once: its destructor clears this. */ + if (!p->cmd) + return NULL; + struct json_stream *response - = jsonrpc_stream_success(renepay->cmd); + = jsonrpc_stream_success(p->cmd); /* Any one succeeding is success. */ json_add_preimage(response, "payment_preimage", p->preimage); json_add_sha256(response, "payment_hash", &p->payment_hash); json_add_timeabs(response, "created_at", p->start_time); - json_add_u32(response, "parts", renepay_parts(renepay)); + json_add_u32(response, "parts", payment_parts(p)); json_add_amount_msat(response, "amount_msat", p->amount); json_add_amount_msat(response, "amount_sent_msat", @@ -203,51 +155,45 @@ struct command_result *renepay_success(struct renepay * renepay) json_add_string(response, "status", "complete"); json_add_node_id(response, "destination", &p->destination); - return command_finished(renepay->cmd, response); + return command_finished(p->cmd, response); } -struct command_result *renepay_fail( - struct renepay * renepay, +struct command_result *payment_fail( + struct payment *payment, enum jsonrpc_errcode code, const char *fmt, ...) { - /* renepay_fail is called after command finished. */ - if(renepay==NULL) - { - return command_still_pending(NULL); - } - payment_fail(renepay->payment); + /* We only finish command once: its destructor clears this. */ + if (!payment->cmd) + return NULL; + + if (payment->status != PAYMENT_SUCCESS) + payment->status = PAYMENT_FAIL; va_list args; va_start(args, fmt); char *message = tal_vfmt(tmpctx,fmt,args); va_end(args); - debug_paynote(renepay->payment,"%s",message); + debug_paynote(payment,"%s",message); - return command_fail(renepay->cmd,code,"%s",message); + return command_fail(payment->cmd,code,"%s",message); } -u64 renepay_parts(const struct renepay *renepay) +u64 payment_parts(const struct payment *payment) { - return renepay->next_partid-1; + return payment->next_partid-1; } /* Either the payment succeeded or failed, we need to cleanup/set the plugin * into a valid state before the next payment. */ -void renepay_cleanup( - struct renepay * renepay, - struct gossmap * gossmap) +void payment_cleanup(struct payment *payment) { debug_info("calling %s",__PRETTY_FUNCTION__); // TODO(eduardo): it can happen that local_gossmods removed below // contained a set of channels for which there is information in the // uncertainty network (chan_extra_map) and that are part of some pending // payflow (payflow_map). Handle this situation. - tal_free(renepay->local_gossmods); - - renepay->rexmit_timer = tal_free(renepay->rexmit_timer); - - if(renepay->payment) - renepay->payment->renepay = NULL; + payment->local_gossmods = tal_free(payment->local_gossmods); + payment->rexmit_timer = tal_free(payment->rexmit_timer); } diff --git a/plugins/renepay/payment.h b/plugins/renepay/payment.h index 159028369627..80c52d907a8e 100644 --- a/plugins/renepay/payment.h +++ b/plugins/renepay/payment.h @@ -13,7 +13,20 @@ struct payment { /* Inside pay_plugin->payments list */ struct list_node list; - struct renepay * renepay; + /* The command if still running (needed for timer func) */ + struct command *cmd; + + /* Localmods to apply to gossip_map for our own use. */ + struct gossmap_localmods *local_gossmods; + + /* Channels we decided to disable for various reasons. */ + struct short_channel_id *disabled; + + /* Timers. */ + struct plugin_timer *rexmit_timer; + + /* Used in get_payflows to set ids to each pay_flow. */ + u64 next_partid; /* Chatty description of attempts. */ const char **paynotes; @@ -32,7 +45,6 @@ struct payment { struct node_id destination; struct sha256 payment_hash; - /* Limits on what routes we'll accept. */ struct amount_msat maxspend; @@ -100,30 +112,9 @@ struct payment { struct command_result * result; }; -/* Data only kept while the payment is being processed. */ -struct renepay -{ - /* The command, and our owner (needed for timer func) */ - struct command *cmd; - - /* Payment information that will eventually outlive renepay and be - * registered. */ - struct payment * payment; - - /* Localmods to apply to gossip_map for our own use. */ - struct gossmap_localmods *local_gossmods; - - /* Channels we decided to disable for various reasons. */ - struct short_channel_id *disabled; - /* Timers. */ - struct plugin_timer *rexmit_timer; - - /* Used in get_payflows to set ids to each pay_flow. */ - u64 next_partid; -}; - -struct renepay *renepay_new(struct command *cmd, +struct payment *payment_new(const tal_t *ctx, + struct command *cmd, const char *invstr TAKES, const char *label TAKES, const char *description TAKES, @@ -144,12 +135,6 @@ struct renepay *renepay_new(struct command *cmd, u64 min_prob_success_millionths, bool use_shadow); -void renepay_cleanup( - struct renepay * renepay, - struct gossmap * gossmap); - -void payment_fail(struct payment * p); -void payment_success(struct payment * p); struct amount_msat payment_sent(const struct payment *p); struct amount_msat payment_delivered(const struct payment *p); struct amount_msat payment_amount(const struct payment *p); @@ -159,13 +144,14 @@ void payment_note(struct payment *p, const char *fmt, ...); void payment_assert_delivering_incomplete(const struct payment *p); void payment_assert_delivering_all(const struct payment *p); -struct command_result *renepay_success(struct renepay *renepay); +struct command_result *payment_success(struct payment *payment); -struct command_result *renepay_fail( - struct renepay * renepay, +struct command_result *payment_fail( + struct payment *payment, enum jsonrpc_errcode code, const char *fmt, ...); -u64 renepay_parts(const struct renepay *renepay); +u64 payment_parts(const struct payment *payment); +void payment_cleanup(struct payment *payment); #endif /* LIGHTNING_PLUGINS_RENEPAY_PAYMENT_H */ diff --git a/plugins/renepay/uncertainty_network.c b/plugins/renepay/uncertainty_network.c index 410e8561d8fd..0d5db6e7ad80 100644 --- a/plugins/renepay/uncertainty_network.c +++ b/plugins/renepay/uncertainty_network.c @@ -41,7 +41,7 @@ bool uncertainty_network_check_invariants(struct chan_extra_map *chan_extra_map) static void add_hintchan( struct chan_extra_map *chan_extra_map, - struct renepay * renepay, + struct payment *payment, const struct node_id *src, const struct node_id *dst, u16 cltv_expiry_delta, @@ -63,9 +63,9 @@ static void add_hintchan( scid, MAX_CAP); /* FIXME: features? */ - gossmap_local_addchan(renepay->local_gossmods, + gossmap_local_addchan(payment->local_gossmods, src, dst, &scid, NULL); - gossmap_local_updatechan(renepay->local_gossmods, + gossmap_local_updatechan(payment->local_gossmods, &scid, /* We assume any HTLC is allowed */ AMOUNT_MSAT(0), MAX_CAP, @@ -84,15 +84,14 @@ static void add_hintchan( /* Add routehints provided by bolt11 */ void uncertainty_network_add_routehints( struct chan_extra_map *chan_extra_map, - struct renepay *renepay) + struct payment *p) { - const struct payment *const p = renepay->payment; struct bolt11 *b11; char *fail; b11 = bolt11_decode(tmpctx, p->invstr, - plugin_feature_set(renepay->cmd->plugin), + plugin_feature_set(p->cmd->plugin), p->description, chainparams, &fail); if (b11 == NULL) debug_err("add_routehints: Invalid bolt11: %s", fail); @@ -104,7 +103,7 @@ void uncertainty_network_add_routehints( for (int j = tal_count(r)-1; j >= 0; j--) { add_hintchan( chan_extra_map, - renepay, &r[j].pubkey, end, + p, &r[j].pubkey, end, r[j].cltv_expiry_delta, r[j].short_channel_id, r[j].fee_base_msat, @@ -232,11 +231,10 @@ void uncertainty_network_channel_can_send( bool uncertainty_network_update_from_listpeerchannels( struct chan_extra_map * chan_extra_map, struct node_id my_id, - struct renepay * renepay, + struct payment *p, const char *buf, const jsmntok_t *toks) { - struct payment * const p = renepay->payment; const jsmntok_t *channels, *channel; size_t i; @@ -270,7 +268,7 @@ bool uncertainty_network_update_from_listpeerchannels( type_to_string(tmpctx, struct short_channel_id, &scid)); - tal_arr_expand(&renepay->disabled, scid); + tal_arr_expand(&p->disabled, scid); continue; } @@ -302,7 +300,7 @@ bool uncertainty_network_update_from_listpeerchannels( /* Don't report opening/closing channels */ if (!json_tok_streq(buf, statetok, "CHANNELD_NORMAL")) { - tal_arr_expand(&renepay->disabled, scid); + tal_arr_expand(&p->disabled, scid); continue; } @@ -316,9 +314,9 @@ bool uncertainty_network_update_from_listpeerchannels( scid, capacity); /* FIXME: features? */ - gossmap_local_addchan(renepay->local_gossmods, + gossmap_local_addchan(p->local_gossmods, &src, &dst, &scid, NULL); - gossmap_local_updatechan(renepay->local_gossmods, + gossmap_local_updatechan(p->local_gossmods, &scid, /* TODO(eduardo): does it diff --git a/plugins/renepay/uncertainty_network.h b/plugins/renepay/uncertainty_network.h index 6a8a0bffc9f0..d07c627a371b 100644 --- a/plugins/renepay/uncertainty_network.h +++ b/plugins/renepay/uncertainty_network.h @@ -14,7 +14,7 @@ bool uncertainty_network_check_invariants(struct chan_extra_map *chan_extra_map) /* Add routehints provided by bolt11 */ void uncertainty_network_add_routehints( struct chan_extra_map *chan_extra_map, - struct renepay *renepay); + struct payment *payment); /* Mirror the gossmap in the public uncertainty network. * result: Every channel in gossmap must have associated data in chan_extra_map, @@ -40,7 +40,7 @@ void uncertainty_network_channel_can_send( bool uncertainty_network_update_from_listpeerchannels( struct chan_extra_map * chan_extra_map, struct node_id my_id, - struct renepay * renepay, + struct payment *payment, const char *buf, const jsmntok_t *toks); From fa90e583ef750a618ad065edd97ee3a67fad341f Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Thu, 10 Aug 2023 09:31:14 +0930 Subject: [PATCH 08/22] renepay: get max group_id in single iteration. Simplifies the logic since we bail out if there are two different group ids in progress. Signed-off-by: Rusty Russell --- plugins/renepay/pay.c | 139 +++++++++++------------------------------- 1 file changed, 37 insertions(+), 102 deletions(-) diff --git a/plugins/renepay/pay.c b/plugins/renepay/pay.c index c91c96327975..b2f138185963 100644 --- a/plugins/renepay/pay.c +++ b/plugins/renepay/pay.c @@ -696,18 +696,14 @@ payment_listsendpays_previous( size_t i; const jsmntok_t *t, *arr, *err; - /* Do we have pending sendpays for the previous attempt? */ - bool pending = false; - /* Group ID of the first pending payment, this will be the one + /* Group ID of the pending payment, this will be the one * who's result gets replayed if we end up suspending. */ - u64 first_pending_group_id = INVALID_ID; - u64 last_pending_group_id = INVALID_ID; - u64 last_pending_partid=0; + u64 pending_group_id = INVALID_ID; + u64 max_pending_partid=0; + u64 max_group_id = 0; struct amount_msat pending_sent = AMOUNT_MSAT(0), pending_msat = AMOUNT_MSAT(0); - /* Did a prior attempt succeed? */ - bool completed = false; /* Metadata for a complete payment, if one exists. */ u32 complete_parts = 0; struct preimage complete_preimage; @@ -715,8 +711,6 @@ payment_listsendpays_previous( complete_msat = AMOUNT_MSAT(0); u32 complete_created_at; - u64 last_group=INVALID_ID; - err = json_get_member(buf, result, "error"); if (err) return command_fail( @@ -730,45 +724,32 @@ payment_listsendpays_previous( cmd, LIGHTNINGD, "Unexpected non-array result from listsendpays"); - /* We need two scans of the payments, the first to identify the groupid - * that have pending sendpays and the second to get the maximum partid - * from that group. */ - - /* We iterate through all prior sendpays, looking for the - * latest group and remembering what its state is. */ json_for_each_arr(i, t, arr) { u64 partid, groupid; struct amount_msat this_msat, this_sent; - - const jsmntok_t *status; + const char *status; // TODO(eduardo): assuming amount_msat is always known. json_scan(tmpctx,buf,t, - "{partid:%" + "{status:%" + ",partid:%" ",groupid:%" ",amount_msat:%" ",amount_sent_msat:%}", + JSON_SCAN_TAL(tmpctx, json_strdup, &status), JSON_SCAN(json_to_u64,&partid), JSON_SCAN(json_to_u64,&groupid), JSON_SCAN(json_to_msat,&this_msat), JSON_SCAN(json_to_msat,&this_sent)); - if(last_group==INVALID_ID) - last_group = groupid; - - last_group = MAX(last_group,groupid); + /* If we decide to create a new group, we base it on max_group_id */ + if (groupid > max_group_id) + max_group_id = 1; /* status could be completed, pending or failed */ - status = json_get_member(buf, t, "status"); - - if(json_tok_streq(buf,status,"failed")) - continue; - - if(json_tok_streq(buf,status,"complete")) - { + if (streq(status, "complete")) { /* Now we know the payment completed. */ - completed = true; if(!amount_msat_add(&complete_msat,complete_msat,this_msat)) debug_err("%s (line %d) msat overflow.", __PRETTY_FUNCTION__,__LINE__); @@ -780,63 +761,21 @@ payment_listsendpays_previous( ",payment_preimage:%}", JSON_SCAN(json_to_u32, &complete_created_at), JSON_SCAN(json_to_preimage, &complete_preimage)); - complete_parts ++; - } - - if(json_tok_streq(buf,status,"pending")) - { - pending = true; // there are parts pending - - if(first_pending_group_id==INVALID_ID || - last_pending_group_id==INVALID_ID) - first_pending_group_id = last_pending_group_id = groupid; - - last_pending_group_id = MAX(last_pending_group_id,groupid); - first_pending_group_id = MIN(first_pending_group_id,groupid); - } - } - - /* We iterate through all prior sendpays, looking for the - * latest pending group. */ - json_for_each_arr(i, t, arr) - { - u64 partid, groupid; - struct amount_msat this_msat, this_sent; - - const jsmntok_t *status; - - // TODO(eduardo): assuming amount_msat is always known. - json_scan(tmpctx,buf,t, - "{partid:%" - ",groupid:%" - ",amount_msat:%" - ",amount_sent_msat:%}", - JSON_SCAN(json_to_u64,&partid), - JSON_SCAN(json_to_u64,&groupid), - JSON_SCAN(json_to_msat,&this_msat), - JSON_SCAN(json_to_msat,&this_sent)); - - /* status could be completed, pending or failed */ - status = json_get_member(buf, t, "status"); - - /* It seems I cannot reuse failed partids for the same groupid, - * therefore let's count them all whatever the status. */ - if(groupid==last_pending_group_id) - last_pending_partid = MAX(last_pending_partid,partid); - - if(groupid == last_pending_group_id && json_tok_streq(buf,status,"pending")) - { - amount_msat_accumulate(&pending_sent,this_sent); - amount_msat_accumulate(&pending_msat,this_msat); - - plugin_log(pay_plugin->plugin,LOG_DBG, - "pending deliver increased by %s", - type_to_string(tmpctx,struct amount_msat,&this_msat)); - } + complete_parts++; + } else if (streq(status, "pending")) { + /* If we have more than one pending group, something went wrong! */ + if (pending_group_id != INVALID_ID + && groupid != pending_group_id) + return command_fail(cmd, PAY_STATUS_UNEXPECTED, + "Multiple pending groups for this payment?"); + pending_group_id = groupid; + if (partid > max_pending_partid) + max_pending_partid = partid; + } else + assert(streq(status, "failed")); } - - if (completed) { + if (complete_parts != 0) { /* There are completed sendpays, we don't need to do anything * but summarize the result. */ struct json_stream *ret = jsonrpc_stream_success(cmd); @@ -852,12 +791,10 @@ payment_listsendpays_previous( /* This payment was already completed, we don't keep record of * it twice: payment will be freed with cmd */ return command_finished(cmd, ret); - } else if (pending) { - assert(last_pending_group_id!=INVALID_ID); - assert(first_pending_group_id!=INVALID_ID); - - payment->groupid = last_pending_group_id; - payment->next_partid = last_pending_partid+1; + } else if (pending_group_id != INVALID_ID) { + /* Continue where we left off? */ + payment->groupid = pending_group_id; + payment->next_partid = max_pending_partid+1; payment->total_sent = pending_sent; payment->total_delivering = pending_msat; @@ -867,17 +804,10 @@ payment_listsendpays_previous( "groupid = %"PRIu64" " "delivering = %s, " "last_partid = %"PRIu64, - last_pending_group_id, + pending_group_id, type_to_string(tmpctx,struct amount_msat,&payment->total_delivering), - last_pending_partid); + max_pending_partid); - if( first_pending_group_id != last_pending_group_id) - { - /* At least two pending groups for the same invoice, - * this is weird, we better stop. */ - return command_fail(cmd, PAY_IN_PROGRESS, - "Payment is pending by some other request."); - } if(amount_msat_greater_eq(payment->total_delivering,payment->amount)) { /* Pending payment already pays the full amount, we @@ -889,7 +819,12 @@ payment_listsendpays_previous( { /* There are no pending nor completed sendpays, get me the last * sendpay group. */ - payment->groupid = (last_group==INVALID_ID ? 1 : (last_group+1)) ; + /* FIXME: use groupid 0 to have sendpay assign an unused groupid, + * as this is theoretically racy against other plugins paying the + * same thing! + * *BUT* that means we have to create one flow first, so we + * can match the others. */ + payment->groupid = max_group_id + 1; payment->next_partid=1; } From 2b813ac72b41dd4a8e75d927f13149f775413c52 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Thu, 10 Aug 2023 09:31:15 +0930 Subject: [PATCH 09/22] renepay: simplify JSON handling. We can use json_scan(), and share a routine to map the notification to the pay_flow. Signed-off-by: Rusty Russell --- plugins/renepay/pay.c | 486 ++++++++++++++---------------------------- 1 file changed, 163 insertions(+), 323 deletions(-) diff --git a/plugins/renepay/pay.c b/plugins/renepay/pay.c index b2f138185963..5b416458007c 100644 --- a/plugins/renepay/pay.c +++ b/plugins/renepay/pay.c @@ -165,15 +165,14 @@ static void timer_kick(struct payment *payment) } /* Sometimes we don't know exactly who to blame... */ -static struct command_result *handle_unhandleable_error(struct payment *payment, - struct pay_flow *flow, - const char *what) +static void handle_unhandleable_error(struct pay_flow *flow, + const char *what) { plugin_log(pay_plugin->plugin,LOG_DBG,"calling %s",__PRETTY_FUNCTION__); size_t n = tal_count(flow); /* We got a mangled reply. We don't know who to penalize! */ - debug_paynote(payment, "%s on route %s", what, flow_path_to_str(tmpctx, flow)); + debug_paynote(flow->payment, "%s on route %s", what, flow_path_to_str(tmpctx, flow)); // TODO(eduardo): does LOG_BROKEN finish the plugin execution? plugin_log(pay_plugin->plugin, LOG_BROKEN, @@ -183,8 +182,9 @@ static struct command_result *handle_unhandleable_error(struct payment *payment, if (n == 1) { payflow_fail(flow); - return payment_fail(payment, PAY_UNPARSEABLE_ONION, - "Got %s from the destination", what); + payment_fail(flow->payment, PAY_UNPARSEABLE_ONION, + "Got %s from the destination", what); + return; } /* FIXME: check chan_extra_map, since we might have succeeded though * this node before? */ @@ -197,11 +197,10 @@ static struct command_result *handle_unhandleable_error(struct payment *payment, /* Assume it's not the destination */ n = pseudorand(n-1); - tal_arr_expand(&payment->disabled, flow->path_scids[n]); - debug_paynote(payment, "... eliminated %s", + tal_arr_expand(&flow->payment->disabled, flow->path_scids[n]); + debug_paynote(flow->payment, "... eliminated %s", type_to_string(tmpctx, struct short_channel_id, &flow->path_scids[n])); - return NULL; } /* We hold onto the flow (and delete the timer) while we're waiting for @@ -245,15 +244,14 @@ static struct command_result *addgossip_failure(struct command *cmd, return addgossip_done(cmd, buf, err, adg); } -static struct command_result *submit_update(struct command *cmd, - struct pay_flow *flow, +static struct command_result *submit_update(struct pay_flow *flow, const u8 *update, struct short_channel_id errscid) { plugin_log(pay_plugin->plugin,LOG_DBG,"calling %s",__PRETTY_FUNCTION__); struct payment *payment = flow->payment; struct out_req *req; - struct addgossip *adg = tal(cmd, struct addgossip); + struct addgossip *adg = tal(flow, struct addgossip); /* We need to stash scid in case this fails, and we need to hold flow so * we don't get a rexmit before this is complete. */ @@ -298,13 +296,11 @@ static u8 *patch_channel_update(const tal_t *ctx, u8 *channel_update TAKES) /* Return NULL if the wrapped onion error message has no channel_update field, * or return the embedded channel_update message otherwise. */ static u8 *channel_update_from_onion_error(const tal_t *ctx, - const char *buf, - const jsmntok_t *onionmsgtok) + const u8 *onion_message) { u8 *channel_update = NULL; struct amount_msat unused_msat; u32 unused32; - u8 *onion_message = json_tok_bin_from_hex(tmpctx, buf, onionmsgtok); /* Identify failcodes that have some channel_update. * @@ -350,29 +346,31 @@ static struct command_result *flow_sendpay_failed(struct command *cmd, const jsmntok_t *err, struct pay_flow *flow) { + struct payment *payment = flow->payment; + enum jsonrpc_errcode errcode; + const char *msg; + plugin_log(pay_plugin->plugin,LOG_DBG,"calling %s",__PRETTY_FUNCTION__); - struct payment *payment = flow->payment; debug_assert(payment); /* This is a fail. */ if (payment->status != PAYMENT_SUCCESS) payment->status=PAYMENT_FAIL; - u64 errcode; - const jsmntok_t *msg = json_get_member(buf, err, "message"); - - if (!json_to_u64(buf, json_get_member(buf, err, "code"), &errcode)) - plugin_err(cmd->plugin, "Bad errcode from sendpay: %.*s", + if (json_scan(tmpctx, buf, err, + "{code:%,message:%}", + JSON_SCAN(json_to_jsonrpc_errcode, &errcode), + JSON_SCAN_TAL(tmpctx, json_strdup, &msg))) { + plugin_err(cmd->plugin, "Bad fail from sendpay: %.*s", json_tok_full_len(err), json_tok_full(buf, err)); - + } if (errcode != PAY_TRY_OTHER_ROUTE) plugin_err(cmd->plugin, "Strange error from sendpay: %.*s", json_tok_full_len(err), json_tok_full(buf, err)); debug_paynote(payment, - "sendpay didn't like first hop, eliminated: %.*s", - msg->end - msg->start, buf + msg->start); + "sendpay didn't like first hop, eliminated: %s", msg) /* There is no new knowledge from this kind of failure. * We just disable this scid. */ @@ -694,7 +692,7 @@ payment_listsendpays_previous( debug_info("calling %s",__PRETTY_FUNCTION__); size_t i; - const jsmntok_t *t, *arr, *err; + const jsmntok_t *t, *arr; /* Group ID of the pending payment, this will be the one * who's result gets replayed if we end up suspending. */ @@ -711,18 +709,13 @@ payment_listsendpays_previous( complete_msat = AMOUNT_MSAT(0); u32 complete_created_at; - err = json_get_member(buf, result, "error"); - if (err) - return command_fail( - cmd, LIGHTNINGD, - "Error retrieving previous pay attempts: %s", - json_strdup(tmpctx, buf, err)); - arr = json_get_member(buf, result, "payments"); if (!arr || arr->type != JSMN_ARRAY) return command_fail( cmd, LIGHTNINGD, - "Unexpected non-array result from listsendpays"); + "Unexpected non-array result from listsendpays: %.*s", + json_tok_full_len(result), + json_tok_full(buf, result)); json_for_each_arr(i, t, arr) { @@ -1142,118 +1135,39 @@ static struct command_result *json_pay(struct command *cmd, return send_outreq(cmd->plugin, req); } -static void handle_sendpay_failure_renepay( - struct command *cmd, - const char *buf, - const jsmntok_t *result, - struct payment *p, - struct pay_flow *flow) +static void handle_sendpay_failure_payment(struct pay_flow *flow, + const char *message, + u32 erridx, + enum onion_wire onionerr, + const u8 *raw) { + struct short_channel_id errscid; + struct payment *p = flow->payment; + debug_assert(flow); debug_assert(p); - u64 errcode; - if (!json_to_u64(buf, json_get_member(buf, result, "code"), &errcode)) - { - plugin_log(pay_plugin->plugin,LOG_BROKEN, - "Failed to get code from sendpay_failure notification" - ", received json: %.*s", - json_tok_full_len(result), - json_tok_full(buf,result)); - return; - } - const jsmntok_t *msgtok = json_get_member(buf, result, "message"); - const char *message; - if(msgtok) - message = tal_fmt(tmpctx,"%.*s", - msgtok->end - msgtok->start, - buf + msgtok->start); - else - message = "[message missing from sendpay_failure notification]"; - - switch(errcode) - { - case PAY_UNPARSEABLE_ONION: - debug_paynote(p, "Unparsable onion reply on route %s", - flow_path_to_str(tmpctx, flow)); - goto unhandleable; - case PAY_TRY_OTHER_ROUTE: - break; - case PAY_DESTINATION_PERM_FAIL: - payment_fail(p, errcode, - "Got a final failure from destination"); - return; - default: - payment_fail(p, errcode, - "Unexpected errocode from sendpay_failure: %.*s", - json_tok_full_len(result), - json_tok_full(buf,result)); - return; - } - - const jsmntok_t* datatok = json_get_member(buf, result, "data"); - - if(!datatok) - { - plugin_err(pay_plugin->plugin, - "Failed to get data from sendpay_failure notification" - ", received json: %.*s", - json_tok_full_len(result), - json_tok_full(buf,result)); - } - - - /* OK, we expect an onion error reply. */ - u32 erridx; - const jsmntok_t * erridxtok = json_get_member(buf, datatok, "erring_index"); - if (!erridxtok || !json_to_u32(buf, erridxtok, &erridx)) - { - debug_paynote(p, "Missing erring_index reply on route %s", - flow_path_to_str(tmpctx, flow)); - plugin_log(pay_plugin->plugin,LOG_DBG, - "%s (line %d) missing erring_index " - "on request %.*s", - __PRETTY_FUNCTION__,__LINE__, - json_tok_full_len(result), - json_tok_full(buf,result)); - goto unhandleable; - } - - struct short_channel_id errscid; - const jsmntok_t *errchantok = json_get_member(buf, datatok, "erring_channel"); - if(!errchantok || !json_to_short_channel_id(buf, errchantok, &errscid)) - { - debug_paynote(p, "Missing erring_channel reply on route %s", - flow_path_to_str(tmpctx, flow)); - goto unhandleable; - } - - if (erridxpath_scids) - && !short_channel_id_eq(&errscid, &flow->path_scids[erridx])) - { + /* Final node is usually a hard failure, but lightningd said + * TRY_OTHER_ROUTE? */ + if (erridx == tal_count(flow->path_scids)) { debug_paynote(p, - "erring_index (%d) does not correspond" - "to erring_channel (%s) on route %s", + "onion error %s from final node #%u: %s", + onion_wire_name(onionerr), erridx, - type_to_string(tmpctx,struct short_channel_id,&errscid), - flow_path_to_str(tmpctx,flow)); - goto unhandleable; - } + message); - u32 onionerr; - const jsmntok_t *failcodetok = json_get_member(buf, datatok, "failcode"); - if(!failcodetok || !json_to_u32(buf, failcodetok, &onionerr)) - { - // TODO(eduardo): I wonder which error code should I show the - // user in this case? - payment_fail(p,LIGHTNINGD, - "Failed to get failcode from sendpay_failure notification" - ", received json: %.*s", - json_tok_full_len(result), - json_tok_full(buf,result)); + if (onionerr == WIRE_MPP_TIMEOUT) + return; + + debug_paynote(p,"final destination failure"); + payment_fail(p, PAY_STATUS_UNEXPECTED, + "Destination said %s: %s", + onion_wire_name(onionerr), + message); return; } + errscid = flow->path_scids[erridx]; debug_paynote(p, "onion error %s from node #%u %s: %s", onion_wire_name(onionerr), @@ -1261,11 +1175,7 @@ static void handle_sendpay_failure_renepay( type_to_string(tmpctx, struct short_channel_id, &errscid), message); - const jsmntok_t *rawoniontok = json_get_member(buf, datatok, "raw_message"); - if(!rawoniontok) - goto unhandleable; - - switch ((enum onion_wire)onionerr) { + switch (onionerr) { /* These definitely mean eliminate channel */ case WIRE_PERMANENT_CHANNEL_FAILURE: case WIRE_REQUIRED_CHANNEL_FEATURE_MISSING: @@ -1297,10 +1207,10 @@ static void handle_sendpay_failure_renepay( plugin_log(pay_plugin->plugin,LOG_DBG,"sendpay_failure, apply channel_update"); /* FIXME: Check scid! */ // TODO(eduardo): check - const u8 *update = channel_update_from_onion_error(tmpctx, buf, rawoniontok); + const u8 *update = channel_update_from_onion_error(tmpctx, raw); if (update) { - submit_update(cmd, flow, update, errscid); + submit_update(flow, update, errscid); return; } @@ -1310,152 +1220,43 @@ static void handle_sendpay_failure_renepay( return; case WIRE_TEMPORARY_CHANNEL_FAILURE: - case WIRE_MPP_TIMEOUT: return; - /* These are from the final distination: fail */ + /* These should only come from the final distination. */ + case WIRE_MPP_TIMEOUT: case WIRE_INCORRECT_OR_UNKNOWN_PAYMENT_DETAILS: case WIRE_FINAL_INCORRECT_CLTV_EXPIRY: case WIRE_FINAL_INCORRECT_HTLC_AMOUNT: - debug_paynote(p,"final destination failure"); - payment_fail(p,errcode, - "Destination said %s: %s", - onion_wire_name(onionerr), - message); - return; + break; } - debug_assert(erridx<=tal_count(flow->path_nodes)); - - if(erridx == tal_count(flow->path_nodes)) - { - debug_paynote(p,"unkown onion error code %u, fatal", - onionerr); - payment_fail(p,errcode, - "Destination gave unknown error code %u: %s", - onionerr,message); - return; - }else - { - debug_paynote(p,"unkown onion error code %u, removing scid %s", - onionerr, - type_to_string(tmpctx,struct short_channel_id,&errscid)); - tal_arr_expand(&p->disabled, errscid); - return; - } - unhandleable: - // TODO(eduardo): check - handle_unhandleable_error(p, flow, ""); + debug_paynote(p,"unkown onion error code %u, removing scid %s", + onionerr, + type_to_string(tmpctx,struct short_channel_id,&errscid)); + tal_arr_expand(&p->disabled, errscid); + return; } -static void handle_sendpay_failure_flow( - struct command *cmd, - const char *buf, - const jsmntok_t *result, - struct pay_flow *flow) +static void handle_sendpay_failure_flow(struct pay_flow *flow, + const char *msg, + u32 erridx, + u32 onionerr) { - // TODO(eduardo): review with Rusty the level of severity of the - // different cases of error below. debug_assert(flow); struct payment * const p = flow->payment; if (p->status != PAYMENT_SUCCESS) p->status=PAYMENT_FAIL; - u64 errcode; - if (!json_to_u64(buf, json_get_member(buf, result, "code"), &errcode)) - { - plugin_err(pay_plugin->plugin, - "Failed to get code from sendpay_failure notification" - ", received json: %.*s", - json_tok_full_len(result), - json_tok_full(buf,result)); - return; - } - const jsmntok_t *msgtok = json_get_member(buf, result, "message"); - const char *message; - if(msgtok) - message = tal_fmt(tmpctx,"%.*s", - msgtok->end - msgtok->start, - buf + msgtok->start); - else - message = "[message missing from sendpay_failure notification]"; - - if(errcode!=PAY_TRY_OTHER_ROUTE) - return; - - const jsmntok_t* datatok = json_get_member(buf, result, "data"); - if(!datatok) - { - plugin_err(pay_plugin->plugin, - "Failed to get data from sendpay_failure notification" - ", received json: %.*s", - json_tok_full_len(result), - json_tok_full(buf,result)); - } - - /* OK, we expect an onion error reply. */ - u32 erridx; - const jsmntok_t * erridxtok = json_get_member(buf, datatok, "erring_index"); - if (!erridxtok || !json_to_u32(buf, erridxtok, &erridx)) - { - plugin_log(pay_plugin->plugin,LOG_BROKEN, - "Failed to get erring_index from sendpay_failure notification" - ", received json: %.*s", - json_tok_full_len(result), - json_tok_full(buf,result)); - return; - } - - struct short_channel_id errscid; - const jsmntok_t *errchantok = json_get_member(buf, datatok, "erring_channel"); - if(!errchantok || !json_to_short_channel_id(buf, errchantok, &errscid)) - { - plugin_log(pay_plugin->plugin,LOG_BROKEN, - "Failed to get erring_channel from sendpay_failure notification" - ", received json: %.*s", - json_tok_full_len(result), - json_tok_full(buf,result)); - return; - } - - if (erridxpath_scids) - && !short_channel_id_eq(&errscid, &flow->path_scids[erridx])) - { - plugin_err(pay_plugin->plugin, - "Erring channel %u/%zu was %s not %s (path %s)", - erridx, tal_count(flow->path_scids), - type_to_string(tmpctx, - struct short_channel_id, - &errscid), - type_to_string(tmpctx, - struct short_channel_id, - &flow->path_scids[erridx]), - flow_path_to_str(tmpctx, flow)); - return; - } - - - u32 onionerr; - const jsmntok_t *failcodetok = json_get_member(buf, datatok, "failcode"); - if(!failcodetok || !json_to_u32(buf, failcodetok, &onionerr)) - { - plugin_log(pay_plugin->plugin,LOG_BROKEN, - "Failed to get failcode from sendpay_failure notification" - ", received json: %.*s", - json_tok_full_len(result), - json_tok_full(buf,result)); - return; - - } - - plugin_log(pay_plugin->plugin,LOG_UNUSUAL, + plugin_log(pay_plugin->plugin, LOG_UNUSUAL, "onion error %s from node #%u %s: " "%s", onion_wire_name(onionerr), erridx, - type_to_string(tmpctx, struct short_channel_id, &errscid), - message); + erridx == tal_count(flow->path_scids) + ? "final" + : type_to_string(tmpctx, struct short_channel_id, &flow->path_scids[erridx]), + msg); /* we know that all channels before erridx where able to commit to this payment */ uncertainty_network_channel_can_send( @@ -1463,8 +1264,9 @@ static void handle_sendpay_failure_flow( flow, erridx); - /* Insufficient funds! */ - if((enum onion_wire)onionerr == WIRE_TEMPORARY_CHANNEL_FAILURE) + /* Insufficient funds (not from final, that's weird!) */ + if((enum onion_wire)onionerr == WIRE_TEMPORARY_CHANNEL_FAILURE + && erridx < tal_count(flow->path_scids)) { plugin_log(pay_plugin->plugin,LOG_DBG, "sendpay_failure says insufficient funds!"); @@ -1480,6 +1282,31 @@ static void handle_sendpay_failure_flow( } } +/* See if this notification is about one of our flows. */ +static struct pay_flow *pay_flow_from_notification(const char *buf, + const jsmntok_t *obj) +{ + struct payflow_key key; + const char *err; + + /* Single part payment? No partid */ + key.partid = 0; + key.payment_hash = tal(tmpctx, struct sha256); + err = json_scan(tmpctx, buf, obj, "{partid?:%,groupid:%,payment_hash:%}", + JSON_SCAN(json_to_u64, &key.partid), + JSON_SCAN(json_to_u64, &key.groupid), + JSON_SCAN(json_to_sha256, key.payment_hash)); + if (err) { + plugin_err(pay_plugin->plugin, + "Missing fields (%s) in notification: %.*s", + err, + json_tok_full_len(obj), + json_tok_full(buf, obj)); + } + + return payflow_map_get(pay_plugin->payflow_map, key); +} + // TODO(eduardo): if I subscribe to a shutdown notification, the plugin takes // forever to close and eventually it gets killed by force. // static struct command_result *notification_shutdown(struct command *cmd, @@ -1575,75 +1402,88 @@ static struct command_result *notification_sendpay_success( tal_free(flow); return notification_handled(cmd); } + static struct command_result *notification_sendpay_failure( struct command *cmd, const char *buf, const jsmntok_t *params) { - struct pay_flow *flow = NULL; - - const jsmntok_t *resulttok = json_get_member(buf,params,"sendpay_failure"); - if(!resulttok) - debug_err("Failed to get result from sendpay_failure notification" - ", received json: %.*s", - json_tok_full_len(params), - json_tok_full(buf,params)); - - const jsmntok_t *datatok = json_get_member(buf,resulttok,"data"); - if(!datatok) - debug_err("Failed to get data from sendpay_failure notification" - ", received json: %.*s", - json_tok_full_len(params), - json_tok_full(buf,params)); + struct pay_flow *flow; + const char *err; + enum jsonrpc_errcode errcode; + const jsmntok_t *sub = json_get_member(buf, params, "sendpay_failure"); + flow = pay_flow_from_notification(buf, json_get_member(buf, sub, "data")); + if (!flow) + return notification_handled(cmd); - // 1. generate the key of this payflow - struct payflow_key key; - key.payment_hash = tal(tmpctx,struct sha256); - - const jsmntok_t *parttok = json_get_member(buf,datatok,"partid"); - if(!parttok || !json_to_u64(buf,parttok,&key.partid)) - { - // No partid, is this a single-path payment? - key.partid = 0; + err = json_scan(tmpctx, buf, sub, "{code:%}", + JSON_SCAN(json_to_jsonrpc_errcode, &errcode)); + if (err) { + plugin_err(pay_plugin->plugin, + "Bad code (%s) in sendpay_failure: %.*s", + err, + json_tok_full_len(params), + json_tok_full(buf, params)); } - const jsmntok_t *grouptok = json_get_member(buf,datatok,"groupid"); - if(!grouptok || !json_to_u64(buf,grouptok,&key.groupid)) - debug_err("Failed to get groupid from sendpay_failure notification" - ", received json: %.*s", - json_tok_full_len(params), - json_tok_full(buf,params)); - - const jsmntok_t *hashtok = json_get_member(buf,datatok,"payment_hash"); - if(!hashtok || !json_to_sha256(buf,hashtok,key.payment_hash)) - debug_err("Failed to get payment_hash from sendpay_failure notification" - ", received json: %.*s", - json_tok_full_len(params), - json_tok_full(buf,params)); - plugin_log(pay_plugin->plugin,LOG_DBG, - "I received a sendpay_failure with key %s", - fmt_payflow_key(tmpctx,&key)); - - // 2. is this payflow recorded in renepay? - flow = payflow_map_get(pay_plugin->payflow_map,key); - if(!flow) - { - plugin_log(pay_plugin->plugin,LOG_DBG, - "sendpay_failure does not correspond to a renepay attempt, %s", - fmt_payflow_key(tmpctx,&key)); + /* Only one code is really actionable */ + switch (errcode) { + case PAY_UNPARSEABLE_ONION: + debug_paynote(flow->payment, "Unparsable onion reply on route %s", + flow_path_to_str(tmpctx, flow)); + err = "Unparsable onion reply"; + goto unhandleable; + case PAY_TRY_OTHER_ROUTE: + break; + case PAY_DESTINATION_PERM_FAIL: + /* FIXME: This is a *success* from the routing POV! */ + payment_fail(flow->payment, errcode, + "Got a final failure from destination"); + goto done; + default: + payment_fail(flow->payment, errcode, + "Unexpected errocode from sendpay_failure: %.*s", + json_tok_full_len(params), + json_tok_full(buf, params)); goto done; } - // 3. process failure - handle_sendpay_failure_flow(cmd,buf,resulttok,flow); + /* Extract remaining fields forto feedback */ + const char *msg; + u32 erridx, onionerr; + const u8 *raw; + + err = json_scan(tmpctx, buf, sub, + "{message:%" + ",data:{erring_index:%" + ",failcode:%" + ",raw_message:%}}", + JSON_SCAN_TAL(tmpctx, json_strdup, &msg), + JSON_SCAN(json_to_u32, &erridx), + JSON_SCAN(json_to_u32, &onionerr), + JSON_SCAN_TAL(tmpctx, json_tok_bin_from_hex, &raw)); + if (err) + goto unhandleable; - // there is possibly a pending renepay command for this flow - handle_sendpay_failure_renepay(cmd,buf,resulttok,flow->payment,flow); + /* Answer must be sane: but note, erridx can be final node! */ + if (erridx > tal_count(flow->path_scids)) { + plugin_err(pay_plugin->plugin, + "Erring channel %u/%zu in path %s", + erridx, tal_count(flow->path_scids), + flow_path_to_str(tmpctx, flow)); + } - done: + handle_sendpay_failure_flow(flow, msg, erridx, onionerr); + handle_sendpay_failure_payment(flow, msg, erridx, onionerr, raw); + +done: if(flow) payflow_fail(flow); return notification_handled(cmd); + +unhandleable: + handle_unhandleable_error(flow, err); + goto done; } static const struct plugin_command commands[] = { From 775cebc5421ba2231ccfe96f4d631ebffe2e168d Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Thu, 10 Aug 2023 09:31:16 +0930 Subject: [PATCH 10/22] renepay: simplify JSON handling in notification_sendpay_success. Use json_scan(), and use the new pay_flow_from_notification() routine. Also, the tal_dup_or_null can be tal_dup, since &preimage is never NULL. Signed-off-by: Rusty Russell --- plugins/renepay/pay.c | 78 ++++++++++--------------------------------- 1 file changed, 18 insertions(+), 60 deletions(-) diff --git a/plugins/renepay/pay.c b/plugins/renepay/pay.c index 5b416458007c..9eaaa3117fbc 100644 --- a/plugins/renepay/pay.c +++ b/plugins/renepay/pay.c @@ -1326,54 +1326,23 @@ static struct command_result *notification_sendpay_success( const char *buf, const jsmntok_t *params) { - struct pay_flow *flow = NULL; - const jsmntok_t *resulttok = json_get_member(buf,params,"sendpay_success"); - if(!resulttok) - debug_err("Failed to get result from sendpay_success notification" - ", received json: %.*s", - json_tok_full_len(params), - json_tok_full(buf,params)); - - // 1. generate the key of this payflow - struct payflow_key key; - key.payment_hash = tal(tmpctx,struct sha256); + struct pay_flow *flow; + struct preimage preimage; + const char *err; + const jsmntok_t *sub = json_get_member(buf, params, "sendpay_success"); - const jsmntok_t *parttok = json_get_member(buf,resulttok,"partid"); - if(!parttok || !json_to_u64(buf,parttok,&key.partid)) - { - // No partid, is this a single-path payment? - key.partid = 0; - // debug_err("Failed to get partid from sendpay_success notification" - // ", received json: %.*s", - // json_tok_full_len(params), - // json_tok_full(buf,params)); - } - const jsmntok_t *grouptok = json_get_member(buf,resulttok,"groupid"); - if(!grouptok || !json_to_u64(buf,grouptok,&key.groupid)) - debug_err("Failed to get groupid from sendpay_success notification" - ", received json: %.*s", - json_tok_full_len(params), - json_tok_full(buf,params)); - - const jsmntok_t *hashtok = json_get_member(buf,resulttok,"payment_hash"); - if(!hashtok || !json_to_sha256(buf,hashtok,key.payment_hash)) - debug_err("Failed to get payment_hash from sendpay_success notification" - ", received json: %.*s", - json_tok_full_len(params), - json_tok_full(buf,params)); - - plugin_log(pay_plugin->plugin,LOG_DBG, - "I received a sendpay_success with key %s", - fmt_payflow_key(tmpctx,&key)); - - // 2. is this payflow recorded in renepay? - flow = payflow_map_get(pay_plugin->payflow_map,key); - if(!flow) - { - plugin_log(pay_plugin->plugin,LOG_DBG, - "sendpay_success does not correspond to a renepay attempt, %s", - fmt_payflow_key(tmpctx,&key)); - goto done; + flow = pay_flow_from_notification(buf, sub); + if (!flow) + return notification_handled(cmd); + + err = json_scan(tmpctx, buf, sub, "{payment_preimage:%}", + JSON_SCAN(json_to_preimage, &preimage)); + if (err) { + plugin_err(pay_plugin->plugin, + "Bad payment_preimage (%s) in sendpay_success: %.*s", + err, + json_tok_full_len(params), + json_tok_full(buf, params)); } // 3. mark as success @@ -1381,25 +1350,14 @@ static struct command_result *notification_sendpay_success( debug_assert(p); p->status = PAYMENT_SUCCESS; - const jsmntok_t *preimagetok - = json_get_member(buf, resulttok, "payment_preimage"); - struct preimage preimage; - - if (!preimagetok || !json_to_preimage(buf, preimagetok,&preimage)) - debug_err("Failed to get payment_preimage from sendpay_success notification" - ", received json: %.*s", - json_tok_full_len(params), - json_tok_full(buf,params)); - /* We could have preimage from previous part */ tal_free(p->preimage); - p->preimage = tal_dup_or_null(p,struct preimage,&preimage); + p->preimage = tal_dup(p,struct preimage, &preimage); // 4. update information and release pending HTLCs uncertainty_network_flow_success(pay_plugin->chan_extra_map,flow); - - done: tal_free(flow); + return notification_handled(cmd); } From 0fb46e1da5ce5cf0fcbaaa6a7df2186c2174e441 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Thu, 10 Aug 2023 09:31:17 +0930 Subject: [PATCH 11/22] renepay: don't re-parse bolt11 to get routehints. Signed-off-by: Rusty Russell --- plugins/renepay/pay.c | 9 +++++---- plugins/renepay/uncertainty_network.c | 24 ++++++++---------------- plugins/renepay/uncertainty_network.h | 3 ++- 3 files changed, 15 insertions(+), 21 deletions(-) diff --git a/plugins/renepay/pay.c b/plugins/renepay/pay.c index 9eaaa3117fbc..46c5d9aa5f80 100644 --- a/plugins/renepay/pay.c +++ b/plugins/renepay/pay.c @@ -853,6 +853,7 @@ static struct command_result *json_pay(struct command *cmd, u64 *min_prob_success_millionths; bool *use_shadow; u16 final_cltv; + const struct route_info **routes = NULL; if (!param(cmd, buf, params, p_req("invstring", param_invstring, &invstr), @@ -882,7 +883,6 @@ static struct command_result *json_pay(struct command *cmd, return command_param_failed(); /* We might need to parse invstring to get amount */ - bool invstr_is_b11=false; if (!bolt12_has_prefix(invstr)) { struct bolt11 *b11; char *fail; @@ -893,7 +893,6 @@ static struct command_result *json_pay(struct command *cmd, if (b11 == NULL) return command_fail(cmd, JSONRPC2_INVALID_PARAMS, "Invalid bolt11: %s", fail); - invstr_is_b11=true; invmsat = b11->msat; invexpiry = b11->timestamp + b11->expiry; @@ -933,6 +932,9 @@ static struct command_result *json_pay(struct command *cmd, JSONRPC2_INVALID_PARAMS, "bolt11 uses description_hash, but you did not provide description parameter"); } + + routes = cast_const2(const struct route_info **, + b11->routes); } else { // TODO(eduardo): check this, compare with `pay` const struct tlv_invoice *b12; @@ -1116,8 +1118,7 @@ static struct command_result *json_pay(struct command *cmd, // TODO(eduardo): are there route hints for B12? // Add any extra hidden channel revealed by the routehints to the uncertainty network. - if(invstr_is_b11) - uncertainty_network_add_routehints(pay_plugin->chan_extra_map, payment); + uncertainty_network_add_routehints(pay_plugin->chan_extra_map, routes, payment); if(!uncertainty_network_check_invariants(pay_plugin->chan_extra_map)) plugin_log(pay_plugin->plugin, diff --git a/plugins/renepay/uncertainty_network.c b/plugins/renepay/uncertainty_network.c index 0d5db6e7ad80..832d7fb7f9a0 100644 --- a/plugins/renepay/uncertainty_network.c +++ b/plugins/renepay/uncertainty_network.c @@ -41,7 +41,7 @@ bool uncertainty_network_check_invariants(struct chan_extra_map *chan_extra_map) static void add_hintchan( struct chan_extra_map *chan_extra_map, - struct payment *payment, + struct gossmap_localmods *local_gossmods, const struct node_id *src, const struct node_id *dst, u16 cltv_expiry_delta, @@ -63,9 +63,9 @@ static void add_hintchan( scid, MAX_CAP); /* FIXME: features? */ - gossmap_local_addchan(payment->local_gossmods, + gossmap_local_addchan(local_gossmods, src, dst, &scid, NULL); - gossmap_local_updatechan(payment->local_gossmods, + gossmap_local_updatechan(local_gossmods, &scid, /* We assume any HTLC is allowed */ AMOUNT_MSAT(0), MAX_CAP, @@ -84,26 +84,18 @@ static void add_hintchan( /* Add routehints provided by bolt11 */ void uncertainty_network_add_routehints( struct chan_extra_map *chan_extra_map, + const struct route_info **routes, struct payment *p) { - struct bolt11 *b11; - char *fail; - - b11 = - bolt11_decode(tmpctx, p->invstr, - plugin_feature_set(p->cmd->plugin), - p->description, chainparams, &fail); - if (b11 == NULL) - debug_err("add_routehints: Invalid bolt11: %s", fail); - - for (size_t i = 0; i < tal_count(b11->routes); i++) { + for (size_t i = 0; i < tal_count(routes); i++) { /* Each one, presumably, leads to the destination */ - const struct route_info *r = b11->routes[i]; + const struct route_info *r = routes[i]; const struct node_id *end = & p->destination; for (int j = tal_count(r)-1; j >= 0; j--) { add_hintchan( chan_extra_map, - p, &r[j].pubkey, end, + p->local_gossmods, + &r[j].pubkey, end, r[j].cltv_expiry_delta, r[j].short_channel_id, r[j].fee_base_msat, diff --git a/plugins/renepay/uncertainty_network.h b/plugins/renepay/uncertainty_network.h index d07c627a371b..fda20e0249b1 100644 --- a/plugins/renepay/uncertainty_network.h +++ b/plugins/renepay/uncertainty_network.h @@ -14,7 +14,8 @@ bool uncertainty_network_check_invariants(struct chan_extra_map *chan_extra_map) /* Add routehints provided by bolt11 */ void uncertainty_network_add_routehints( struct chan_extra_map *chan_extra_map, - struct payment *payment); + const struct route_info **routes, + struct payment *p); /* Mirror the gossmap in the public uncertainty network. * result: Every channel in gossmap must have associated data in chan_extra_map, From e1da480e2b24e9c27b8e434d3ad82b16e68c53ca Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Thu, 10 Aug 2023 09:31:18 +0930 Subject: [PATCH 12/22] renepay: put the entire hash in the key struct. As recommended by your TODO, a bit simpler: we also make the hash function return a ptr rather than the (now rather large) struct. Signed-off-by: Rusty Russell --- plugins/renepay/pay.c | 5 ++--- plugins/renepay/pay_flow.c | 2 +- plugins/renepay/pay_flow.h | 23 +++++++++++------------ plugins/renepay/test/run-payflow_map.c | 19 +++++++++++-------- 4 files changed, 25 insertions(+), 24 deletions(-) diff --git a/plugins/renepay/pay.c b/plugins/renepay/pay.c index 46c5d9aa5f80..9a6e010d76af 100644 --- a/plugins/renepay/pay.c +++ b/plugins/renepay/pay.c @@ -1292,11 +1292,10 @@ static struct pay_flow *pay_flow_from_notification(const char *buf, /* Single part payment? No partid */ key.partid = 0; - key.payment_hash = tal(tmpctx, struct sha256); err = json_scan(tmpctx, buf, obj, "{partid?:%,groupid:%,payment_hash:%}", JSON_SCAN(json_to_u64, &key.partid), JSON_SCAN(json_to_u64, &key.groupid), - JSON_SCAN(json_to_sha256, key.payment_hash)); + JSON_SCAN(json_to_sha256, &key.payment_hash)); if (err) { plugin_err(pay_plugin->plugin, "Missing fields (%s) in notification: %.*s", @@ -1305,7 +1304,7 @@ static struct pay_flow *pay_flow_from_notification(const char *buf, json_tok_full(buf, obj)); } - return payflow_map_get(pay_plugin->payflow_map, key); + return payflow_map_get(pay_plugin->payflow_map, &key); } // TODO(eduardo): if I subscribe to a shutdown notification, the plugin takes diff --git a/plugins/renepay/pay_flow.c b/plugins/renepay/pay_flow.c index 0776dc3266b4..903eb8aaebfd 100644 --- a/plugins/renepay/pay_flow.c +++ b/plugins/renepay/pay_flow.c @@ -220,7 +220,7 @@ static struct pay_flow **flows_to_pay_flows(struct payment *payment, pf->payment = payment; pf->key.partid = (*next_partid)++; pf->key.groupid = payment->groupid; - pf->key.payment_hash = &payment->payment_hash; + pf->key.payment_hash = payment->payment_hash; /* Convert gossmap_chan into scids and nodes */ pf->path_scids = tal_arr(pf, struct short_channel_id, plen); diff --git a/plugins/renepay/pay_flow.h b/plugins/renepay/pay_flow.h index bda8564e0391..542891405b55 100644 --- a/plugins/renepay/pay_flow.h +++ b/plugins/renepay/pay_flow.h @@ -17,8 +17,7 @@ struct pay_flow { /* Information to link this flow to a unique sendpay. */ struct payflow_key { - // TODO(eduardo): pointer or value? - struct sha256 *payment_hash; + struct sha256 payment_hash; u64 groupid; u64 partid; } key; @@ -36,9 +35,9 @@ struct pay_flow { }; static inline struct payflow_key -payflow_key(struct sha256 *hash, u64 groupid, u64 partid) +payflow_key(const struct sha256 *hash, u64 groupid, u64 partid) { - struct payflow_key k= {hash,groupid,partid}; + struct payflow_key k= {*hash,groupid,partid}; return k; } @@ -50,27 +49,27 @@ static inline const char* fmt_payflow_key( ctx, "key: groupid=%"PRIu64", partid=%"PRIu64", payment_hash=%s", k->groupid,k->partid, - type_to_string(ctx,struct sha256,k->payment_hash)); + type_to_string(ctx,struct sha256,&k->payment_hash)); return str; } -static inline const struct payflow_key +static inline const struct payflow_key * payflow_get_key(const struct pay_flow * pf) { - return pf->key; + return &pf->key; } -static inline size_t payflow_key_hash(const struct payflow_key k) +static inline size_t payflow_key_hash(const struct payflow_key *k) { - return k.payment_hash->u.u32[0] ^ (k.groupid << 32) ^ k.partid; + return k->payment_hash.u.u32[0] ^ (k->groupid << 32) ^ k->partid; } static inline bool payflow_key_equal(const struct pay_flow *pf, - const struct payflow_key k) + const struct payflow_key *k) { - return pf->key.partid==k.partid && pf->key.groupid==k.groupid - && sha256_eq(pf->key.payment_hash,k.payment_hash); + return pf->key.partid==k->partid && pf->key.groupid==k->groupid + && sha256_eq(&pf->key.payment_hash, &k->payment_hash); } HTABLE_DEFINE_TYPE(struct pay_flow, diff --git a/plugins/renepay/test/run-payflow_map.c b/plugins/renepay/test/run-payflow_map.c index 9627ef810ecc..1d3c65261133 100644 --- a/plugins/renepay/test/run-payflow_map.c +++ b/plugins/renepay/test/run-payflow_map.c @@ -31,14 +31,14 @@ static void destroy_payflow( } static struct pay_flow* new_payflow( const tal_t *ctx, - struct sha256 * payment_hash, + const struct sha256 * payment_hash, u64 gid, u64 pid) { struct pay_flow *p = tal(ctx,struct pay_flow); p->payment=NULL; - p->key.payment_hash=payment_hash; + p->key.payment_hash = *payment_hash; p->key.groupid = gid; p->key.partid = pid; @@ -61,6 +61,7 @@ static void valgrind_ok1(void) { tal_t *local_ctx = tal(this_ctx,tal_t); + struct payflow_key key; struct pay_flow *p1 = new_payflow(local_ctx, &hash,1,1); @@ -69,17 +70,19 @@ static void valgrind_ok1(void) printf("key1 = %s\n",fmt_payflow_key(local_ctx,&p1->key)); printf("key1 = %s\n",fmt_payflow_key(local_ctx,&p2->key)); - printf("key hash 1 = %zu\n",payflow_key_hash(p1->key)); - printf("key hash 2 = %zu\n",payflow_key_hash(p2->key)); + printf("key hash 1 = %zu\n",payflow_key_hash(&p1->key)); + printf("key hash 2 = %zu\n",payflow_key_hash(&p2->key)); payflow_map_add(map,p1); tal_add_destructor2(p1,destroy_payflow,map); payflow_map_add(map,p2); tal_add_destructor2(p2,destroy_payflow,map); - struct pay_flow *q1 = payflow_map_get(map,payflow_key(&hash,1,1)); - struct pay_flow *q2 = payflow_map_get(map,payflow_key(&hash,2,3)); + key = payflow_key(&hash,1,1); + struct pay_flow *q1 = payflow_map_get(map, &key); + key = payflow_key(&hash,2,3); + struct pay_flow *q2 = payflow_map_get(map, &key); - assert(payflow_key_hash(q1->key)==payflow_key_hash(p1->key)); - assert(payflow_key_hash(q2->key)==payflow_key_hash(p2->key)); + assert(payflow_key_hash(&q1->key)==payflow_key_hash(&p1->key)); + assert(payflow_key_hash(&q2->key)==payflow_key_hash(&p2->key)); tal_free(local_ctx); } From 149057f2ce7cb68af0e7cfe9fbde6d7f251f6281 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Thu, 10 Aug 2023 09:31:19 +0930 Subject: [PATCH 13/22] renepay: remove unused result member. Signed-off-by: Rusty Russell --- plugins/renepay/payment.c | 1 - plugins/renepay/payment.h | 2 -- 2 files changed, 3 deletions(-) diff --git a/plugins/renepay/payment.c b/plugins/renepay/payment.c index 5216b808cbc5..7bb5b257fef8 100644 --- a/plugins/renepay/payment.c +++ b/plugins/renepay/payment.c @@ -61,7 +61,6 @@ struct payment *payment_new(const tal_t *ctx, p->use_shadow = use_shadow; p->groupid=1; - p->result = NULL; p->local_gossmods = gossmap_localmods_new(p); p->disabled = tal_arr(p,struct short_channel_id,0); p->rexmit_timer = NULL; diff --git a/plugins/renepay/payment.h b/plugins/renepay/payment.h index 80c52d907a8e..fff94dea5076 100644 --- a/plugins/renepay/payment.h +++ b/plugins/renepay/payment.h @@ -108,8 +108,6 @@ struct payment { /* Groupid, so listpays() can group them back together */ u64 groupid; - - struct command_result * result; }; From 99f331e0fbf76f2967853a63e737118fab3ea279 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Thu, 10 Aug 2023 09:31:20 +0930 Subject: [PATCH 14/22] renepay: remove always-true "first_time" and "unlikely_ok" flags. Signed-off-by: Rusty Russell --- plugins/renepay/pay.c | 15 ++++----------- plugins/renepay/pay_flow.c | 14 +------------- plugins/renepay/pay_flow.h | 1 - 3 files changed, 5 insertions(+), 25 deletions(-) diff --git a/plugins/renepay/pay.c b/plugins/renepay/pay.c index 9a6e010d76af..56129489d84e 100644 --- a/plugins/renepay/pay.c +++ b/plugins/renepay/pay.c @@ -34,8 +34,7 @@ struct pay_plugin * const pay_plugin = &the_pay_plugin; static void timer_kick(struct payment *payment); static struct command_result *try_paying(struct command *cmd, - struct payment *payment, - bool first_time); + struct payment *payment); void amount_msat_accumulate_(struct amount_msat *dst, struct amount_msat src, @@ -152,7 +151,7 @@ static void timer_kick(struct payment *payment) case PAYMENT_FAIL: plugin_log(pay_plugin->plugin,LOG_DBG,"status is PAYMENT_FAIL"); payment_assert_delivering_incomplete(payment); - try_paying(payment->cmd,payment,/* always try even if prob is low */ true); + try_paying(payment->cmd, payment); break; /* Nothing has returned yet, we have to wait. */ @@ -472,8 +471,7 @@ sendpay_flows(struct command *cmd, } static struct command_result *try_paying(struct command *cmd, - struct payment *payment, - bool first_time) + struct payment *payment) { plugin_log(pay_plugin->plugin,LOG_DBG,"calling %s",__PRETTY_FUNCTION__); @@ -537,11 +535,6 @@ static struct command_result *try_paying(struct command *cmd, struct pay_flow **pay_flows = get_payflows( payment, remaining, feebudget, - - /* would you accept unlikely - * payments? */ - true, - /* is entire payment? */ amount_msat_eq(payment->total_delivering, AMOUNT_MSAT(0)), @@ -595,7 +588,7 @@ static struct command_result *listpeerchannels_done( /* When we terminate cmd for any reason, clear it from payment so we don't do it again. */ tal_add_destructor2(cmd, destroy_cmd_payment_ptr, payment); - return try_paying(cmd, payment, true); + return try_paying(cmd, payment); } diff --git a/plugins/renepay/pay_flow.c b/plugins/renepay/pay_flow.c index 903eb8aaebfd..6af73a4317de 100644 --- a/plugins/renepay/pay_flow.c +++ b/plugins/renepay/pay_flow.c @@ -336,7 +336,6 @@ static bool disable_htlc_violations(struct payment *payment, struct pay_flow **get_payflows(struct payment *p, struct amount_msat amount, struct amount_msat feebudget, - bool unlikely_ok, bool is_entire_payment, const char **err_msg) { @@ -365,7 +364,7 @@ struct pay_flow **get_payflows(struct payment *p, double prob; struct amount_msat fee; u64 delay; - bool too_unlikely, too_expensive, too_delayed; + bool too_expensive, too_delayed; const u32 *final_cltvs; flows = minflow(tmpctx, pay_plugin->gossmap, src, dst, @@ -399,17 +398,6 @@ struct pay_flow **get_payflows(struct payment *p, type_to_string(tmpctx,struct amount_msat,&fee), delay); - too_unlikely = (prob < p->min_prob_success); - if (too_unlikely && !unlikely_ok) - { - debug_paynote(p, "Flows too unlikely, P() = %f%%", prob * 100); - *err_msg = tal_fmt(tmpctx, - "Probability is too small, " - "Prob = %f%% (min = %f%%)", - prob*100, - p->min_prob_success*100); - goto fail; - } too_expensive = amount_msat_greater(fee, feebudget); if (too_expensive) { diff --git a/plugins/renepay/pay_flow.h b/plugins/renepay/pay_flow.h index 542891405b55..a91a9df6b249 100644 --- a/plugins/renepay/pay_flow.h +++ b/plugins/renepay/pay_flow.h @@ -80,7 +80,6 @@ HTABLE_DEFINE_TYPE(struct pay_flow, struct pay_flow **get_payflows(struct payment *payment, struct amount_msat amount, struct amount_msat feebudget, - bool unlikely_ok, bool is_entire_payment, const char **err_msg); From c358b56fea3bf04799bf2ddd1a5384b64ee06c8a Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Thu, 10 Aug 2023 09:31:21 +0930 Subject: [PATCH 15/22] renepay: fix up handling of errors from final node. Treat it just like "PAY_TRY_OTHER_ROUTE", except it is from the final node: this means we correctly process that it "succeeded". Add a test: this crashes sometimes, but it's cleaned up soon... Signed-off-by: Rusty Russell --- plugins/renepay/pay.c | 7 ++----- tests/test_renepay.py | 10 ++++++++++ 2 files changed, 12 insertions(+), 5 deletions(-) diff --git a/plugins/renepay/pay.c b/plugins/renepay/pay.c index 56129489d84e..a0cfc7dca820 100644 --- a/plugins/renepay/pay.c +++ b/plugins/renepay/pay.c @@ -1154,7 +1154,7 @@ static void handle_sendpay_failure_payment(struct pay_flow *flow, return; debug_paynote(p,"final destination failure"); - payment_fail(p, PAY_STATUS_UNEXPECTED, + payment_fail(p, PAY_DESTINATION_PERM_FAIL, "Destination said %s: %s", onion_wire_name(onionerr), message); @@ -1388,10 +1388,7 @@ static struct command_result *notification_sendpay_failure( case PAY_TRY_OTHER_ROUTE: break; case PAY_DESTINATION_PERM_FAIL: - /* FIXME: This is a *success* from the routing POV! */ - payment_fail(flow->payment, errcode, - "Got a final failure from destination"); - goto done; + break; default: payment_fail(flow->payment, errcode, "Unexpected errocode from sendpay_failure: %.*s", diff --git a/tests/test_renepay.py b/tests/test_renepay.py index 53c5afbe0f14..c2989552cc5f 100644 --- a/tests/test_renepay.py +++ b/tests/test_renepay.py @@ -48,6 +48,8 @@ def test_errors(node_factory, bitcoind): l1, l2, l3, l4, l5, l6 = node_factory.get_nodes(6, opts=opts * 6) send_amount = Millisatoshi('21sat') inv = l6.rpc.invoice(send_amount, 'test_renepay', 'description')['bolt11'] + inv_deleted = l6.rpc.invoice(send_amount, 'test_renepay2', 'description2')['bolt11'] + l6.rpc.delinvoice('test_renepay2', 'unpaid') failmsg = r'We don\'t have any channels' with pytest.raises(RpcError, match=failmsg): @@ -81,6 +83,14 @@ def test_errors(node_factory, bitcoind): assert details['amount_msat'] == send_amount assert details['destination'] == l6.info['id'] + # Test error from final node. + with pytest.raises(RpcError) as err: + l1.rpc.call('renepay', {'invstring': inv_deleted}) + + PAY_DESTINATION_PERM_FAIL = 203 + assert err.value.error['code'] == PAY_DESTINATION_PERM_FAIL + assert 'WIRE_INCORRECT_OR_UNKNOWN_PAYMENT_DETAILS' in err.value.error['message'] + @pytest.mark.developer("needs to deactivate shadow routing") @pytest.mark.openchannel('v1') From e4b386fc656e305f82be2a2dc1313c435c32ed2c Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Thu, 10 Aug 2023 11:29:42 +0930 Subject: [PATCH 16/22] renepay: make pay_plugin a tal object. Avoids a gratuitous "ctx" field, and the simplified declaration is now understood by `make update-mocks`. Signed-off-by: Rusty Russell --- common/test/run-bolt12_merkle-json.c | 6 +++--- common/test/run-json_remove.c | 6 ------ lightningd/test/run-invoice-select-inchan.c | 14 ++++--------- plugins/renepay/pay.c | 22 +++++++++++---------- plugins/renepay/pay.h | 5 +---- plugins/renepay/test/run-arc.c | 5 ++--- plugins/test/run-route-overlong.c | 3 --- wallet/test/run-wallet.c | 8 ++++---- 8 files changed, 26 insertions(+), 43 deletions(-) diff --git a/common/test/run-bolt12_merkle-json.c b/common/test/run-bolt12_merkle-json.c index b7519fb7f4f7..bcfc9ee6b0bc 100644 --- a/common/test/run-bolt12_merkle-json.c +++ b/common/test/run-bolt12_merkle-json.c @@ -45,6 +45,9 @@ void towire_channel_id(u8 **pptr UNNEEDED, const struct channel_id *channel_id U /* Generated stub for towire_node_id */ void towire_node_id(u8 **pptr UNNEEDED, const struct node_id *id UNNEEDED) { fprintf(stderr, "towire_node_id called!\n"); abort(); } +/* Generated stub for towire_s64 */ +void towire_s64(u8 **pptr UNNEEDED, s64 v UNNEEDED) +{ fprintf(stderr, "towire_s64 called!\n"); abort(); } /* Generated stub for towire_secp256k1_ecdsa_signature */ void towire_secp256k1_ecdsa_signature(u8 **pptr UNNEEDED, const secp256k1_ecdsa_signature *signature UNNEEDED) @@ -67,9 +70,6 @@ void towire_u32(u8 **pptr UNNEEDED, u32 v UNNEEDED) /* Generated stub for towire_u64 */ void towire_u64(u8 **pptr UNNEEDED, u64 v UNNEEDED) { fprintf(stderr, "towire_u64 called!\n"); abort(); } -/* Generated stub for towire_s64 */ -void towire_s64(u8 **pptr UNNEEDED, s64 v UNNEEDED) -{ fprintf(stderr, "towire_s64 called!\n"); abort(); } /* Generated stub for towire_u8 */ void towire_u8(u8 **pptr UNNEEDED, u8 v UNNEEDED) { fprintf(stderr, "towire_u8 called!\n"); abort(); } diff --git a/common/test/run-json_remove.c b/common/test/run-json_remove.c index 526611768a79..64e246e00085 100644 --- a/common/test/run-json_remove.c +++ b/common/test/run-json_remove.c @@ -87,9 +87,6 @@ u32 fromwire_u32(const u8 **cursor UNNEEDED, size_t *max UNNEEDED) /* Generated stub for fromwire_u64 */ u64 fromwire_u64(const u8 **cursor UNNEEDED, size_t *max UNNEEDED) { fprintf(stderr, "fromwire_u64 called!\n"); abort(); } -/* Generated stub for fromwire_s64 */ -s64 fromwire_s64(const u8 **cursor UNNEEDED, size_t *max UNNEEDED) -{ fprintf(stderr, "fromwire_s64 called!\n"); abort(); } /* Generated stub for fromwire_u8 */ u8 fromwire_u8(const u8 **cursor UNNEEDED, size_t *max UNNEEDED) { fprintf(stderr, "fromwire_u8 called!\n"); abort(); } @@ -187,9 +184,6 @@ void towire_u32(u8 **pptr UNNEEDED, u32 v UNNEEDED) /* Generated stub for towire_u64 */ void towire_u64(u8 **pptr UNNEEDED, u64 v UNNEEDED) { fprintf(stderr, "towire_u64 called!\n"); abort(); } -/* Generated stub for towire_s64 */ -void towire_s64(u8 **pptr UNNEEDED, s64 v UNNEEDED) -{ fprintf(stderr, "towire_s64 called!\n"); abort(); } /* Generated stub for towire_u8 */ void towire_u8(u8 **pptr UNNEEDED, u8 v UNNEEDED) { fprintf(stderr, "towire_u8 called!\n"); abort(); } diff --git a/lightningd/test/run-invoice-select-inchan.c b/lightningd/test/run-invoice-select-inchan.c index 2b1c0c0aea38..e94054688ff6 100644 --- a/lightningd/test/run-invoice-select-inchan.c +++ b/lightningd/test/run-invoice-select-inchan.c @@ -499,6 +499,10 @@ void json_add_num(struct json_stream *result UNNEEDED, const char *fieldname UNN void json_add_preimage(struct json_stream *result UNNEEDED, const char *fieldname UNNEEDED, const struct preimage *preimage UNNEEDED) { fprintf(stderr, "json_add_preimage called!\n"); abort(); } +/* Generated stub for json_add_s64 */ +void json_add_s64(struct json_stream *result UNNEEDED, const char *fieldname UNNEEDED, + int64_t value UNNEEDED) +{ fprintf(stderr, "json_add_s64 called!\n"); abort(); } /* Generated stub for json_add_secret */ void json_add_secret(struct json_stream *response UNNEEDED, const char *fieldname UNNEEDED, @@ -544,10 +548,6 @@ void json_add_u32(struct json_stream *result UNNEEDED, const char *fieldname UNN void json_add_u64(struct json_stream *result UNNEEDED, const char *fieldname UNNEEDED, uint64_t value UNNEEDED) { fprintf(stderr, "json_add_u64 called!\n"); abort(); } -/* Generated stub for json_add_s64 */ -void json_add_s64(struct json_stream *result UNNEEDED, const char *fieldname UNNEEDED, - int64_t value UNNEEDED) -{ fprintf(stderr, "json_add_s64 called!\n"); abort(); } /* Generated stub for json_add_uncommitted_channel */ void json_add_uncommitted_channel(struct json_stream *response UNNEEDED, const struct uncommitted_channel *uc UNNEEDED, @@ -798,12 +798,6 @@ struct command_result *param_u64(struct command *cmd UNNEEDED, const char *name const char *buffer UNNEEDED, const jsmntok_t *tok UNNEEDED, uint64_t **num UNNEEDED) { fprintf(stderr, "param_u64 called!\n"); abort(); } -/* Generated stub for channel_state_normalish */ -bool channel_state_normalish(const struct channel *channel UNNEEDED) -{ fprintf(stderr, "channel_state_normalish called!\n"); abort(); } -/* Generated stub for channel_state_awaitish */ -bool channel_state_awaitish(const struct channel *channel UNNEEDED) -{ fprintf(stderr, "channel_state_awaitish called!\n"); abort(); } /* Generated stub for peer_any_active_channel */ struct channel *peer_any_active_channel(struct peer *peer UNNEEDED, bool *others UNNEEDED) { fprintf(stderr, "peer_any_active_channel called!\n"); abort(); } diff --git a/plugins/renepay/pay.c b/plugins/renepay/pay.c index a0cfc7dca820..e6b77e01f34d 100644 --- a/plugins/renepay/pay.c +++ b/plugins/renepay/pay.c @@ -29,8 +29,7 @@ #define MAX(a,b) ((a)>(b)? (a) : (b)) #define MIN(a,b) ((a)<(b)? (a) : (b)) -static struct pay_plugin the_pay_plugin; -struct pay_plugin * const pay_plugin = &the_pay_plugin; +struct pay_plugin *pay_plugin; static void timer_kick(struct payment *payment); static struct command_result *try_paying(struct command *cmd, @@ -64,7 +63,7 @@ void amount_msat_reduce_(struct amount_msat *dst, #if DEVELOPER static void memleak_mark(struct plugin *p, struct htable *memtable) { - memleak_scan_region(memtable, pay_plugin, sizeof(*pay_plugin)); + memleak_scan_obj(memtable, pay_plugin); memleak_scan_htable(memtable, &pay_plugin->chan_extra_map->raw); } #endif @@ -80,7 +79,7 @@ static const char *init(struct plugin *p, { size_t num_channel_updates_rejected; - pay_plugin->ctx = notleak_with_children(tal(p,tal_t)); + tal_steal(p, pay_plugin); pay_plugin->plugin = p; pay_plugin->last_time = 0; @@ -98,13 +97,13 @@ static const char *init(struct plugin *p, list_head_init(&pay_plugin->payments); - pay_plugin->chan_extra_map = tal(pay_plugin->ctx,struct chan_extra_map); + pay_plugin->chan_extra_map = tal(pay_plugin,struct chan_extra_map); chan_extra_map_init(pay_plugin->chan_extra_map); - pay_plugin->payflow_map = tal(pay_plugin->ctx,struct payflow_map); + pay_plugin->payflow_map = tal(pay_plugin,struct payflow_map); payflow_map_init(pay_plugin->payflow_map); - pay_plugin->gossmap = gossmap_load(pay_plugin->ctx, + pay_plugin->gossmap = gossmap_load(pay_plugin, GOSSIP_STORE_FILENAME, &num_channel_updates_rejected); @@ -445,7 +444,7 @@ sendpay_flows(struct command *cmd, /* Flow now owned by all_flows instead of req., in this way we * can control the destruction occurs before we remove temporary * channels from chan_extra_map. */ - tal_steal(pay_plugin->ctx,flows[i]); + tal_steal(pay_plugin,flows[i]); /* Let's keep record of this flow. */ payflow_map_add(pay_plugin->payflow_map,flows[i]); @@ -1469,6 +1468,11 @@ static const struct plugin_notification notifications[] = { int main(int argc, char *argv[]) { setup_locale(); + + /* Most gets initialized in init(), but set debug options here. */ + pay_plugin = tal(NULL, struct pay_plugin); + pay_plugin->debug_mcf = pay_plugin->debug_payflow = false; + plugin_main( argv, init, @@ -1487,7 +1491,5 @@ int main(int argc, char *argv[]) flag_option, &pay_plugin->debug_payflow), NULL); - // TODO(eduardo): I think this is actually never executed - tal_free(pay_plugin->ctx); return 0; } diff --git a/plugins/renepay/pay.h b/plugins/renepay/pay.h index 0c7159568f50..e50bd90758d2 100644 --- a/plugins/renepay/pay.h +++ b/plugins/renepay/pay.h @@ -75,9 +75,6 @@ struct pay_plugin { bool debug_mcf; bool debug_payflow; - /* I'll allocate all global (controlled by pay_plugin) variables tied to - * this tal_t. */ - tal_t *ctx; /* Pending flows have HTLCs (in-flight) liquidity * attached that is reflected in the uncertainty network. * When sendpay_fail or sendpay_success notifications arrive @@ -96,7 +93,7 @@ struct pay_plugin { }; /* Set in init */ -extern struct pay_plugin * const pay_plugin; +extern struct pay_plugin *pay_plugin; /* Accumulate or panic on overflow */ #define amount_msat_accumulate(dst, src) \ diff --git a/plugins/renepay/test/run-arc.c b/plugins/renepay/test/run-arc.c index 1c1304171935..293520bcdbdc 100644 --- a/plugins/renepay/test/run-arc.c +++ b/plugins/renepay/test/run-arc.c @@ -11,9 +11,6 @@ #include "../mcf.c" -/* update-mocks isn't quiet smart enough for this, so place here */ -struct pay_plugin *const pay_plugin; - /* AUTOGENERATED MOCKS START */ /* Generated stub for flow_complete */ void flow_complete(struct flow *flow UNNEEDED, @@ -43,6 +40,8 @@ s64 linear_fee_cost( double base_fee_penalty UNNEEDED, double delay_feefactor UNNEEDED) { fprintf(stderr, "linear_fee_cost called!\n"); abort(); } +/* Generated stub for pay_plugin */ +struct pay_plugin *pay_plugin; /* AUTOGENERATED MOCKS END */ int main(int argc, char *argv[]) diff --git a/plugins/test/run-route-overlong.c b/plugins/test/run-route-overlong.c index 94d33085273e..f78f0fa561ad 100644 --- a/plugins/test/run-route-overlong.c +++ b/plugins/test/run-route-overlong.c @@ -160,9 +160,6 @@ bool json_to_u32(const char *buffer UNNEEDED, const jsmntok_t *tok UNNEEDED, u32 /* Generated stub for json_to_u64 */ bool json_to_u64(const char *buffer UNNEEDED, const jsmntok_t *tok UNNEEDED, u64 *num UNNEEDED) { fprintf(stderr, "json_to_u64 called!\n"); abort(); } -/* Generated stub for json_to_s64 */ -bool json_to_s64(const char *buffer UNNEEDED, const jsmntok_t *tok UNNEEDED, s64 *num UNNEEDED) -{ fprintf(stderr, "json_to_s64 called!\n"); abort(); } /* Generated stub for json_tok_bin_from_hex */ u8 *json_tok_bin_from_hex(const tal_t *ctx UNNEEDED, const char *buffer UNNEEDED, const jsmntok_t *tok UNNEEDED) { fprintf(stderr, "json_tok_bin_from_hex called!\n"); abort(); } diff --git a/wallet/test/run-wallet.c b/wallet/test/run-wallet.c index 99c2cc057877..74f4c0b93d9e 100644 --- a/wallet/test/run-wallet.c +++ b/wallet/test/run-wallet.c @@ -309,6 +309,10 @@ void json_add_pubkey(struct json_stream *response UNNEEDED, void json_add_s32(struct json_stream *result UNNEEDED, const char *fieldname UNNEEDED, int32_t value UNNEEDED) { fprintf(stderr, "json_add_s32 called!\n"); abort(); } +/* Generated stub for json_add_s64 */ +void json_add_s64(struct json_stream *result UNNEEDED, const char *fieldname UNNEEDED, + int64_t value UNNEEDED) +{ fprintf(stderr, "json_add_s64 called!\n"); abort(); } /* Generated stub for json_add_secret */ void json_add_secret(struct json_stream *response UNNEEDED, const char *fieldname UNNEEDED, @@ -354,10 +358,6 @@ void json_add_u32(struct json_stream *result UNNEEDED, const char *fieldname UNN void json_add_u64(struct json_stream *result UNNEEDED, const char *fieldname UNNEEDED, uint64_t value UNNEEDED) { fprintf(stderr, "json_add_u64 called!\n"); abort(); } -/* Generated stub for json_add_s64 */ -void json_add_s64(struct json_stream *result UNNEEDED, const char *fieldname UNNEEDED, - int64_t value UNNEEDED) -{ fprintf(stderr, "json_add_s64 called!\n"); abort(); } /* Generated stub for json_add_uncommitted_channel */ void json_add_uncommitted_channel(struct json_stream *response UNNEEDED, const struct uncommitted_channel *uc UNNEEDED, From 8cdef9a47f1c843b3d897f7c65457efc568aeeb5 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Thu, 10 Aug 2023 11:29:44 +0930 Subject: [PATCH 17/22] renepay: grab update from WIRE_TEMPORARY_CHANNEL_FAILURE if present. It's not required, but it should be there so we might as well use it (though we sometimes don't put one in, esp if it's a private channel). Signed-off-by: Rusty Russell --- plugins/renepay/pay.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/plugins/renepay/pay.c b/plugins/renepay/pay.c index e6b77e01f34d..d5053bf43737 100644 --- a/plugins/renepay/pay.c +++ b/plugins/renepay/pay.c @@ -1213,6 +1213,12 @@ static void handle_sendpay_failure_payment(struct pay_flow *flow, return; case WIRE_TEMPORARY_CHANNEL_FAILURE: + /* These also contain a channel_update, but in this case it's simply + * advisory, not necessary. */ + const u8 *update = channel_update_from_onion_error(tmpctx, raw); + if (update) + submit_update(flow, update, errscid); + return; /* These should only come from the final distination. */ From a578732d77c6326b60d48ad0f19a9c03a38d74c6 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Thu, 10 Aug 2023 11:29:44 +0930 Subject: [PATCH 18/22] renepay: drive *all* progress from termination of struct pay_flow. The main function here is payment_reconsider: * Each payment has a list of pay_flow. * This is populated in try_paying(), calling add_payflows & sendpay_new_flows. * When we get a notification, we resolve a pay_flow using one of the pay_flow_failedxxx or pay_flow_succeeded functions. * They call payment_reconsider() which cleans up finished flows decides what to do: often calling try_paying again. Signed-off-by: Rusty Russell --- plugins/renepay/pay.c | 248 ++++++++----------------- plugins/renepay/pay.h | 6 +- plugins/renepay/pay_flow.c | 188 ++++++++++++------- plugins/renepay/pay_flow.h | 73 ++++++-- plugins/renepay/payment.c | 229 ++++++++++++++++++++--- plugins/renepay/payment.h | 26 +-- plugins/renepay/test/run-mcf-diamond.c | 10 + plugins/renepay/test/run-mcf.c | 10 + plugins/renepay/test/run-testflow.c | 10 + tests/test_renepay.py | 8 +- 10 files changed, 517 insertions(+), 291 deletions(-) diff --git a/plugins/renepay/pay.c b/plugins/renepay/pay.c index d5053bf43737..4be441f6f9ec 100644 --- a/plugins/renepay/pay.c +++ b/plugins/renepay/pay.c @@ -31,10 +31,6 @@ struct pay_plugin *pay_plugin; -static void timer_kick(struct payment *payment); -static struct command_result *try_paying(struct command *cmd, - struct payment *payment); - void amount_msat_accumulate_(struct amount_msat *dst, struct amount_msat src, const char *dstname, @@ -68,12 +64,6 @@ static void memleak_mark(struct plugin *p, struct htable *memtable) } #endif -static void destroy_payflow(struct pay_flow *flow) -{ - remove_htlc_payflow(pay_plugin->chan_extra_map,flow); - payflow_map_del(pay_plugin->payflow_map, flow); -} - static const char *init(struct plugin *p, const char *buf UNUSED, const jsmntok_t *config UNUSED) { @@ -123,45 +113,6 @@ static const char *init(struct plugin *p, return NULL; } - -static void payment_settimer(struct payment *payment) -{ - payment->rexmit_timer = tal_free(payment->rexmit_timer); - payment->rexmit_timer = plugin_timer( - pay_plugin->plugin, - time_from_msec(TIMER_COLLECT_FAILURES_MSEC), - timer_kick, payment); -} - -/* Happens when timer goes off, but also works to arm timer if nothing to do */ -static void timer_kick(struct payment *payment) -{ - plugin_log(pay_plugin->plugin,LOG_DBG,"calling %s",__PRETTY_FUNCTION__); - - switch (payment->status) - { - /* Some flows succeeded, we finish the payment. */ - case PAYMENT_SUCCESS: - plugin_log(pay_plugin->plugin,LOG_DBG,"status is PAYMENT_SUCCESS"); - payment_success(payment); - break; - - /* Some flows failed, we retry. */ - case PAYMENT_FAIL: - plugin_log(pay_plugin->plugin,LOG_DBG,"status is PAYMENT_FAIL"); - payment_assert_delivering_incomplete(payment); - try_paying(payment->cmd, payment); - break; - - /* Nothing has returned yet, we have to wait. */ - case PAYMENT_PENDING: - plugin_log(pay_plugin->plugin,LOG_DBG,"status is PAYMENT_PENDING"); - payment_assert_delivering_all(payment); - payment_settimer(payment); - break; - } -} - /* Sometimes we don't know exactly who to blame... */ static void handle_unhandleable_error(struct pay_flow *flow, const char *what) @@ -179,9 +130,8 @@ static void handle_unhandleable_error(struct pay_flow *flow, if (n == 1) { - payflow_fail(flow); - payment_fail(flow->payment, PAY_UNPARSEABLE_ONION, - "Got %s from the destination", what); + /* This is a terminal error. */ + pay_flow_failed_final(flow, PAY_UNPARSEABLE_ONION, what); return; } /* FIXME: check chan_extra_map, since we might have succeeded though @@ -199,6 +149,8 @@ static void handle_unhandleable_error(struct pay_flow *flow, debug_paynote(flow->payment, "... eliminated %s", type_to_string(tmpctx, struct short_channel_id, &flow->path_scids[n])); + + pay_flow_failed(flow); } /* We hold onto the flow (and delete the timer) while we're waiting for @@ -214,13 +166,9 @@ static struct command_result *addgossip_done(struct command *cmd, struct addgossip *adg) { plugin_log(pay_plugin->plugin,LOG_DBG,"calling %s",__PRETTY_FUNCTION__); - struct payment * payment = adg->flow->payment; - /* Release this: if it's the last flow we'll retry immediately */ - - payflow_fail(adg->flow); + pay_flow_finished_adding_gossip(adg->flow); tal_free(adg); - payment_settimer(payment); return command_still_pending(cmd); } @@ -255,8 +203,6 @@ static struct command_result *submit_update(struct pay_flow *flow, * we don't get a rexmit before this is complete. */ adg->scid = errscid; adg->flow = flow; - /* Disable re-xmit until this returns */ - payment->rexmit_timer = tal_free(payment->rexmit_timer); debug_paynote(payment, "... extracted channel_update, telling gossipd"); plugin_log(pay_plugin->plugin, LOG_DBG, "(update = %s)", tal_hex(tmpctx, update)); @@ -352,10 +298,6 @@ static struct command_result *flow_sendpay_failed(struct command *cmd, debug_assert(payment); - /* This is a fail. */ - if (payment->status != PAYMENT_SUCCESS) - payment->status=PAYMENT_FAIL; - if (json_scan(tmpctx, buf, err, "{code:%,message:%}", JSON_SCAN(json_to_jsonrpc_errcode, &errcode), @@ -374,45 +316,44 @@ static struct command_result *flow_sendpay_failed(struct command *cmd, * We just disable this scid. */ tal_arr_expand(&payment->disabled, flow->path_scids[0]); - payflow_fail(flow); + pay_flow_failed(flow); return command_still_pending(cmd); } - -static struct command_result * -sendpay_flows(struct command *cmd, - struct payment *p, - struct pay_flow **flows STEALS) +/* Kick off all pay_flows which are in state PAY_FLOW_NOT_STARTED */ +static void sendpay_new_flows(struct payment *p) { - plugin_log(pay_plugin->plugin,LOG_DBG,"calling %s",__PRETTY_FUNCTION__); - debug_paynote(p, "Sending out batch of %zu payments", tal_count(flows)); - - for (size_t i = 0; i < tal_count(flows); i++) { - const size_t path_lengh = tal_count(flows[i]->amounts); - debug_paynote(p, "sendpay flow groupid=%ld, partid=%ld, delivering=%s, probability=%.3lf", - flows[i]->key.groupid, - flows[i]->key.partid, - type_to_string(tmpctx,struct amount_msat, - &flows[i]->amounts[path_lengh-1]), - flows[i]->success_prob); + struct pay_flow *pf; + + list_for_each(&p->flows, pf, list) { + if (pf->state != PAY_FLOW_NOT_STARTED) + continue; + + debug_paynote(p, "sendpay flow groupid=%"PRIu64", partid=%"PRIu64", delivering=%s, probability=%.3lf", + pf->key.groupid, + pf->key.partid, + fmt_amount_msat(tmpctx, payflow_delivered(pf)), + pf->success_prob); struct out_req *req; - req = jsonrpc_request_start(cmd->plugin, cmd, "sendpay", + /* FIXME: We don't actually want cmd to own this sendpay, so we use NULL here, + * but we should use a variant which allows us to set json id! */ + req = jsonrpc_request_start(pay_plugin->plugin, NULL, "sendpay", flow_sent, flow_sendpay_failed, - flows[i]); + pf); json_array_start(req->js, "route"); - for (size_t j = 0; j < tal_count(flows[i]->path_nodes); j++) { + for (size_t j = 0; j < tal_count(pf->path_nodes); j++) { json_object_start(req->js, NULL); json_add_node_id(req->js, "id", - &flows[i]->path_nodes[j]); + &pf->path_nodes[j]); json_add_short_channel_id(req->js, "channel", - &flows[i]->path_scids[j]); + &pf->path_scids[j]); json_add_amount_msat(req->js, "amount_msat", - flows[i]->amounts[j]); + pf->amounts[j]); json_add_num(req->js, "direction", - flows[i]->path_dirs[j]); + pf->path_dirs[j]); json_add_u32(req->js, "delay", - flows[i]->cltv_delays[j]); + pf->cltv_delays[j]); json_add_string(req->js,"style","tlv"); json_object_end(req->js); } @@ -423,7 +364,7 @@ sendpay_flows(struct command *cmd, json_add_amount_msat(req->js, "amount_msat", p->amount); - json_add_u64(req->js, "partid", flows[i]->key.partid); + json_add_u64(req->js, "partid", pf->key.partid); json_add_u64(req->js, "groupid", p->groupid); if (p->payment_metadata) @@ -437,48 +378,25 @@ sendpay_flows(struct command *cmd, if (p->description) json_add_string(req->js, "description", p->description); - amount_msat_accumulate(&p->total_sent, flows[i]->amounts[0]); - amount_msat_accumulate(&p->total_delivering, - payflow_delivered(flows[i])); - - /* Flow now owned by all_flows instead of req., in this way we - * can control the destruction occurs before we remove temporary - * channels from chan_extra_map. */ - tal_steal(pay_plugin,flows[i]); - - /* Let's keep record of this flow. */ - payflow_map_add(pay_plugin->payflow_map,flows[i]); + send_outreq(pay_plugin->plugin, req); - /* record these HTLC along the flow path */ - commit_htlc_payflow(pay_plugin->chan_extra_map,flows[i]); - - /* Remove the HTLC from the chan_extra_map after finish. */ - tal_add_destructor(flows[i], destroy_payflow); - - send_outreq(cmd->plugin, req); + /* Now you're started! */ + pf->state = PAY_FLOW_IN_PROGRESS; } /* Safety check. */ payment_assert_delivering_all(p); - - tal_free(flows); - - /* Get ready to process replies */ - payment_settimer(p); - - return command_still_pending(cmd); } -static struct command_result *try_paying(struct command *cmd, - struct payment *payment) +const char *try_paying(const tal_t *ctx, + struct payment *payment, + enum jsonrpc_errcode *ecode) { plugin_log(pay_plugin->plugin,LOG_DBG,"calling %s",__PRETTY_FUNCTION__); struct amount_msat feebudget, fees_spent, remaining; - payment->status = PAYMENT_PENDING; - if (time_after(time_now(), payment->stop_time)) - return payment_fail(payment, PAY_STOPPED_RETRYING, "Timed out"); + assert(payment->status == PAYMENT_PENDING); /* Total feebudget */ if (!amount_msat_sub(&feebudget, payment->maxspend, payment->amount)) @@ -531,27 +449,22 @@ static struct command_result *try_paying(struct command *cmd, /* We let this return an unlikely path, as it's better to try once * than simply refuse. Plus, models are not truth! */ gossmap_apply_localmods(pay_plugin->gossmap, payment->local_gossmods); - struct pay_flow **pay_flows = get_payflows( - payment, - remaining, feebudget, - /* is entire payment? */ - amount_msat_eq(payment->total_delivering, AMOUNT_MSAT(0)), - - &err_msg); + err_msg = add_payflows(tmpctx, + payment, + remaining, feebudget, + /* is entire payment? */ + amount_msat_eq(payment->total_delivering, AMOUNT_MSAT(0)), + ecode); gossmap_remove_localmods(pay_plugin->gossmap, payment->local_gossmods); - // plugin_log(pay_plugin->plugin,LOG_DBG,"get_payflows produced %s",fmt_payflows(tmpctx,pay_flows)); - /* MCF cannot find a feasible route, we stop. */ - if (!pay_flows) - { - return payment_fail(payment, PAY_ROUTE_NOT_FOUND, - "Failed to find a route, %s", - err_msg); - } + if (err_msg) + return err_msg; + /* Now begin making payments */ + sendpay_new_flows(payment); - return sendpay_flows(cmd, payment, pay_flows); + return NULL; } static void destroy_cmd_payment_ptr(struct command *cmd, @@ -568,6 +481,9 @@ static struct command_result *listpeerchannels_done( struct payment *payment) { plugin_log(pay_plugin->plugin,LOG_DBG,"calling %s",__PRETTY_FUNCTION__); + const char *errmsg; + enum jsonrpc_errcode ecode; + if (!uncertainty_network_update_from_listpeerchannels( pay_plugin->chan_extra_map, pay_plugin->my_id, @@ -585,9 +501,15 @@ static struct command_result *listpeerchannels_done( /* From now on, we keep a record of the payment, so persist it beyond this cmd. */ tal_steal(pay_plugin->plugin, payment); /* When we terminate cmd for any reason, clear it from payment so we don't do it again. */ + assert(cmd == payment->cmd); tal_add_destructor2(cmd, destroy_cmd_payment_ptr, payment); - return try_paying(cmd, payment); + /* This looks for a route, and if OK, fires off the sendpay commands */ + errmsg = try_paying(tmpctx, payment, &ecode); + if (errmsg) + return payment_fail(payment, ecode, "%s", errmsg); + + return command_still_pending(cmd); } @@ -1128,7 +1050,8 @@ static struct command_result *json_pay(struct command *cmd, return send_outreq(cmd->plugin, req); } -static void handle_sendpay_failure_payment(struct pay_flow *flow, +/* Terminates flow */ +static void handle_sendpay_failure_payment(struct pay_flow *flow STEALS, const char *message, u32 erridx, enum onion_wire onionerr, @@ -1136,12 +1059,12 @@ static void handle_sendpay_failure_payment(struct pay_flow *flow, { struct short_channel_id errscid; struct payment *p = flow->payment; + const u8 *update; debug_assert(flow); debug_assert(p); - /* Final node is usually a hard failure, but lightningd said - * TRY_OTHER_ROUTE? */ + /* Final node is usually a hard failure */ if (erridx == tal_count(flow->path_scids)) { debug_paynote(p, "onion error %s from final node #%u: %s", @@ -1149,14 +1072,13 @@ static void handle_sendpay_failure_payment(struct pay_flow *flow, erridx, message); - if (onionerr == WIRE_MPP_TIMEOUT) + if (onionerr == WIRE_MPP_TIMEOUT) { + pay_flow_failed(flow); return; + } debug_paynote(p,"final destination failure"); - payment_fail(p, PAY_DESTINATION_PERM_FAIL, - "Destination said %s: %s", - onion_wire_name(onionerr), - message); + pay_flow_failed_final(flow, PAY_DESTINATION_PERM_FAIL, message); return; } @@ -1190,6 +1112,7 @@ static void handle_sendpay_failure_payment(struct pay_flow *flow, debug_paynote(p, "we're removing scid %s", type_to_string(tmpctx,struct short_channel_id,&errscid)); tal_arr_expand(&p->disabled, errscid); + pay_flow_failed(flow); return; /* These can be fixed (maybe) by applying the included channel_update */ @@ -1200,10 +1123,12 @@ static void handle_sendpay_failure_payment(struct pay_flow *flow, plugin_log(pay_plugin->plugin,LOG_DBG,"sendpay_failure, apply channel_update"); /* FIXME: Check scid! */ // TODO(eduardo): check - const u8 *update = channel_update_from_onion_error(tmpctx, raw); + update = channel_update_from_onion_error(tmpctx, raw); if (update) { submit_update(flow, update, errscid); + /* Don't retry until we call pay_flow_finished_adding_gossip! */ + pay_flow_failed_adding_gossip(flow); return; } @@ -1215,9 +1140,12 @@ static void handle_sendpay_failure_payment(struct pay_flow *flow, case WIRE_TEMPORARY_CHANNEL_FAILURE: /* These also contain a channel_update, but in this case it's simply * advisory, not necessary. */ - const u8 *update = channel_update_from_onion_error(tmpctx, raw); - if (update) + update = channel_update_from_onion_error(tmpctx, raw); + if (update) { submit_update(flow, update, errscid); + /* Don't retry until we call pay_flow_finished_adding_gossip! */ + pay_flow_failed_adding_gossip(flow); + } return; @@ -1233,7 +1161,7 @@ static void handle_sendpay_failure_payment(struct pay_flow *flow, onionerr, type_to_string(tmpctx,struct short_channel_id,&errscid)); tal_arr_expand(&p->disabled, errscid); - return; + pay_flow_failed(flow); } static void handle_sendpay_failure_flow(struct pay_flow *flow, @@ -1244,8 +1172,6 @@ static void handle_sendpay_failure_flow(struct pay_flow *flow, debug_assert(flow); struct payment * const p = flow->payment; - if (p->status != PAYMENT_SUCCESS) - p->status=PAYMENT_FAIL; plugin_log(pay_plugin->plugin, LOG_UNUSUAL, "onion error %s from node #%u %s: " @@ -1305,20 +1231,6 @@ static struct pay_flow *pay_flow_from_notification(const char *buf, return payflow_map_get(pay_plugin->payflow_map, &key); } -// TODO(eduardo): if I subscribe to a shutdown notification, the plugin takes -// forever to close and eventually it gets killed by force. -// static struct command_result *notification_shutdown(struct command *cmd, -// const char *buf, -// const jsmntok_t *params) -// { -// /* TODO(eduardo): -// * 1. at shutdown the `struct plugin *p` is not freed, -// * 2. `memleak_check` is called before we have the chance to get this -// * notification. */ -// // plugin_log(pay_plugin->plugin,LOG_DBG,"received shutdown notification, freeing data."); -// pay_plugin->ctx = tal_free(pay_plugin->ctx); -// return notification_handled(cmd); -// } static struct command_result *notification_sendpay_success( struct command *cmd, const char *buf, @@ -1395,10 +1307,9 @@ static struct command_result *notification_sendpay_failure( case PAY_DESTINATION_PERM_FAIL: break; default: - payment_fail(flow->payment, errcode, - "Unexpected errocode from sendpay_failure: %.*s", - json_tok_full_len(params), - json_tok_full(buf, params)); + pay_flow_failed_final(flow, + errcode, + "Unexpected errorcode from sendpay_failure"); goto done; } @@ -1431,7 +1342,6 @@ static struct command_result *notification_sendpay_failure( handle_sendpay_failure_payment(flow, msg, erridx, onionerr, raw); done: - if(flow) payflow_fail(flow); return notification_handled(cmd); unhandleable: diff --git a/plugins/renepay/pay.h b/plugins/renepay/pay.h index e50bd90758d2..6a2c90e89d6c 100644 --- a/plugins/renepay/pay.h +++ b/plugins/renepay/pay.h @@ -69,7 +69,7 @@ struct pay_plugin { /* Per-channel metadata: some persists between payments */ struct chan_extra_map *chan_extra_map; - /* Pending senpays. */ + /* Pending sendpays (to match notifications to). */ struct payflow_map * payflow_map; bool debug_mcf; @@ -110,4 +110,8 @@ void amount_msat_reduce_(struct amount_msat *dst, const char *dstname, const char *srcname); +/* Returns NULL if OK, otherwise an error msg and sets *ecode */ +const char *try_paying(const tal_t *ctx, + struct payment *payment, + enum jsonrpc_errcode *ecode); #endif /* LIGHTNING_PLUGINS_RENEPAY_PAY_H */ diff --git a/plugins/renepay/pay_flow.c b/plugins/renepay/pay_flow.c index 6af73a4317de..27dfb4ca2c79 100644 --- a/plugins/renepay/pay_flow.c +++ b/plugins/renepay/pay_flow.c @@ -35,6 +35,12 @@ #define MAX_SHADOW_LEN 3 +/* FIXME: rearrange to avoid predecls. */ +static void remove_htlc_payflow(struct chan_extra_map *chan_extra_map, + struct pay_flow *flow); +static void commit_htlc_payflow(struct chan_extra_map *chan_extra_map, + const struct pay_flow *flow); + /* Returns CLTV, and fills in *shadow_fee, based on extending the path */ static u32 shadow_one_flow(const struct gossmap *gossmap, const struct flow *f, @@ -198,26 +204,30 @@ static u32 *shadow_additions(const tal_t *ctx, return final_cltvs; } -/* Calculates delays and converts to scids. Frees flows. Caller is responsible - * for removing resultings flows from the chan_extra_map. */ -static struct pay_flow **flows_to_pay_flows(struct payment *payment, - struct gossmap *gossmap, - struct flow **flows STEALS, - const u32 *final_cltvs, - u64 *next_partid) +static void destroy_payment_flow(struct pay_flow *pf) { - struct pay_flow **pay_flows - = tal_arr(payment, struct pay_flow *, tal_count(flows)); + list_del_from(&pf->payment->flows, &pf->list); + remove_htlc_payflow(pay_plugin->chan_extra_map, pf); + payflow_map_del(pay_plugin->payflow_map, pf); +} +/* Calculates delays and converts to scids, and links to the payment. + * Frees flows. */ +static void convert_and_attach_flows(struct payment *payment, + struct gossmap *gossmap, + struct flow **flows STEALS, + const u32 *final_cltvs, + u64 *next_partid) +{ for (size_t i = 0; i < tal_count(flows); i++) { struct flow *f = flows[i]; - struct pay_flow *pf = tal(pay_flows, struct pay_flow); + struct pay_flow *pf = tal(payment, struct pay_flow); size_t plen; plen = tal_count(f->path); - pay_flows[i] = pf; pf->payment = payment; + pf->state = PAY_FLOW_NOT_STARTED; pf->key.partid = (*next_partid)++; pf->key.groupid = payment->groupid; pf->key.payment_hash = payment->payment_hash; @@ -243,10 +253,25 @@ static struct pay_flow **flows_to_pay_flows(struct payment *payment, pf->amounts = tal_steal(pf, f->amounts); pf->path_dirs = tal_steal(pf, f->dirs); pf->success_prob = f->success_prob; + + /* Payment keeps a list of its flows. */ + list_add(&payment->flows, &pf->list); + + /* Increase totals for payment */ + amount_msat_accumulate(&payment->total_sent, pf->amounts[0]); + amount_msat_accumulate(&payment->total_delivering, + payflow_delivered(pf)); + + /* We keep a global map to identify notifications + * about this flow. */ + payflow_map_add(pay_plugin->payflow_map, pf); + + /* record these HTLC along the flow path */ + commit_htlc_payflow(pay_plugin->chan_extra_map, pf); + + tal_add_destructor(pf, destroy_payment_flow); } tal_free(flows); - - return pay_flows; } static bitmap *make_disabled_bitmap(const tal_t *ctx, @@ -332,31 +357,28 @@ static bool disable_htlc_violations(struct payment *payment, return disabled_some; } -/* Get some payment flows to get this amount to destination, or NULL. */ -struct pay_flow **get_payflows(struct payment *p, - struct amount_msat amount, - struct amount_msat feebudget, - bool is_entire_payment, - const char **err_msg) +const char *add_payflows(const tal_t *ctx, + struct payment *p, + struct amount_msat amount, + struct amount_msat feebudget, + bool is_entire_payment, + enum jsonrpc_errcode *ecode) { - *err_msg = tal_fmt(tmpctx,"[no error]"); - bitmap *disabled; - struct pay_flow **pay_flows; const struct gossmap_node *src, *dst; disabled = make_disabled_bitmap(tmpctx, pay_plugin->gossmap, p->disabled); src = gossmap_find_node(pay_plugin->gossmap, &pay_plugin->my_id); if (!src) { debug_paynote(p, "We don't have any channels?"); - *err_msg = tal_fmt(tmpctx,"We don't have any channels."); - goto fail; + *ecode = PAY_ROUTE_NOT_FOUND; + return tal_fmt(ctx, "We don't have any channels."); } dst = gossmap_find_node(pay_plugin->gossmap, &p->destination); if (!dst) { debug_paynote(p, "No trace of destination in network gossip"); - *err_msg = tal_fmt(tmpctx,"Destination is unreacheable in the network gossip."); - goto fail; + *ecode = PAY_ROUTE_NOT_FOUND; + return tal_fmt(ctx, "Destination is unknown in the network gossip."); } for (;;) { @@ -380,10 +402,10 @@ struct pay_flow **get_payflows(struct payment *p, "minflow couldn't find a feasible flow for %s", type_to_string(tmpctx,struct amount_msat,&amount)); - *err_msg = tal_fmt(tmpctx, - "minflow couldn't find a feasible flow for %s", - type_to_string(tmpctx,struct amount_msat,&amount)); - goto fail; + *ecode = PAY_ROUTE_NOT_FOUND; + return tal_fmt(ctx, + "minflow couldn't find a feasible flow for %s", + type_to_string(tmpctx,struct amount_msat,&amount)); } /* Are we unhappy? */ @@ -404,12 +426,12 @@ struct pay_flow **get_payflows(struct payment *p, debug_paynote(p, "Flows too expensive, fee = %s (max %s)", type_to_string(tmpctx, struct amount_msat, &fee), type_to_string(tmpctx, struct amount_msat, &feebudget)); - *err_msg = tal_fmt(tmpctx, - "Fee exceeds our fee budget, " - "fee = %s (maxfee = %s)", - type_to_string(tmpctx, struct amount_msat, &fee), - type_to_string(tmpctx, struct amount_msat, &feebudget)); - goto fail; + *ecode = PAY_ROUTE_TOO_EXPENSIVE; + return tal_fmt(ctx, + "Fee exceeds our fee budget, " + "fee = %s (maxfee = %s)", + type_to_string(tmpctx, struct amount_msat, &fee), + type_to_string(tmpctx, struct amount_msat, &feebudget)); } too_delayed = (delay > p->maxdelay); if (too_delayed) { @@ -419,11 +441,11 @@ struct pay_flow **get_payflows(struct payment *p, /* FIXME: What is a sane limit? */ if (p->delay_feefactor > 1000) { debug_paynote(p, "Giving up!"); - *err_msg = tal_fmt(tmpctx, - "CLTV delay exceeds our CLTV budget, " - "delay = %"PRIu64" (maxdelay = %u)", - delay,p->maxdelay); - goto fail; + *ecode = PAY_ROUTE_TOO_EXPENSIVE; + return tal_fmt(ctx, + "CLTV delay exceeds our CLTV budget, " + "delay = %"PRIu64" (maxdelay = %u)", + delay, p->maxdelay); } p->delay_feefactor *= 2; @@ -448,18 +470,15 @@ struct pay_flow **get_payflows(struct payment *p, * to make it look like it's going elsewhere */ final_cltvs = shadow_additions(tmpctx, pay_plugin->gossmap, p, flows, is_entire_payment); + /* OK, we are happy with these flows: convert to - * pay_flows to outlive the current gossmap. */ - pay_flows = flows_to_pay_flows(p, pay_plugin->gossmap, - flows, final_cltvs, - &p->next_partid); - break; + * pay_flows in the current payment, to outlive the + * current gossmap. */ + convert_and_attach_flows(p, pay_plugin->gossmap, + flows, final_cltvs, + &p->next_partid); + return NULL; } - - return pay_flows; - -fail: - return NULL; } const char *flow_path_to_str(const tal_t *ctx, const struct pay_flow *flow) @@ -538,7 +557,7 @@ const char* fmt_payflows(const tal_t *ctx, return json_out_contents(jout,&len); } -void remove_htlc_payflow( +static void remove_htlc_payflow( struct chan_extra_map *chan_extra_map, struct pay_flow *flow) { @@ -573,7 +592,7 @@ void remove_htlc_payflow( h->num_htlcs--; } } -void commit_htlc_payflow( +static void commit_htlc_payflow( struct chan_extra_map *chan_extra_map, const struct pay_flow *flow) { @@ -607,19 +626,62 @@ struct amount_msat payflow_delivered(const struct pay_flow *flow) return flow->amounts[tal_count(flow->amounts)-1]; } -struct pay_flow* payflow_fail(struct pay_flow *flow) +/* Remove (failed) payment from amounts. */ +static void payment_remove_flowamount(const struct pay_flow *pf) +{ + amount_msat_reduce(&pf->payment->total_delivering, + payflow_delivered(pf)); + amount_msat_reduce(&pf->payment->total_sent, pf->amounts[0]); +} + +/* We've been notified that a pay_flow has failed */ +void pay_flow_failed(struct pay_flow *pf) +{ + assert(pf->state == PAY_FLOW_IN_PROGRESS); + pf->state = PAY_FLOW_FAILED; + payment_remove_flowamount(pf); + + payment_reconsider(pf->payment); +} + +/* We've been notified that a pay_flow has failed, payment is done. */ +void pay_flow_failed_final(struct pay_flow *pf, + enum jsonrpc_errcode final_error, + const char *final_msg TAKES) +{ + assert(pf->state == PAY_FLOW_IN_PROGRESS); + pf->state = PAY_FLOW_FAILED_FINAL; + pf->final_error = final_error; + pf->final_msg = tal_strdup(pf, final_msg); + payment_remove_flowamount(pf); + + payment_reconsider(pf->payment); +} + +/* We've been notified that a pay_flow has failed, adding gossip. */ +void pay_flow_failed_adding_gossip(struct pay_flow *pf) { - debug_assert(flow); - struct payment * p = flow->payment; - debug_assert(p); + assert(pf->state == PAY_FLOW_IN_PROGRESS); + pf->state = PAY_FLOW_FAILED_GOSSIP_PENDING; + payment_remove_flowamount(pf); +} - if (p->status != PAYMENT_SUCCESS) - p->status = PAYMENT_FAIL; - amount_msat_reduce(&p->total_delivering, payflow_delivered(flow)); - amount_msat_reduce(&p->total_sent, flow->amounts[0]); +/* We've finished adding gossip. */ +void pay_flow_finished_adding_gossip(struct pay_flow *pf) +{ + assert(pf->state == PAY_FLOW_FAILED_GOSSIP_PENDING); + pf->state = PAY_FLOW_FAILED; - /* Release the HTLCs in the uncertainty_network. */ - return tal_free(flow); + payment_reconsider(pf->payment); } +/* We've been notified that a pay_flow has succeeded. */ +void pay_flow_succeeded(struct pay_flow *pf, + const struct preimage *preimage) +{ + assert(pf->state == PAY_FLOW_IN_PROGRESS); + pf->state = PAY_FLOW_SUCCESS; + pf->payment_preimage = tal_dup(pf, struct preimage, preimage); + payment_reconsider(pf->payment); +} diff --git a/plugins/renepay/pay_flow.h b/plugins/renepay/pay_flow.h index a91a9df6b249..8ef03a59fd20 100644 --- a/plugins/renepay/pay_flow.h +++ b/plugins/renepay/pay_flow.h @@ -8,9 +8,36 @@ #include #include +/* There are several states a payment can be in */ +enum pay_flow_state { + /* Created, but not sent to sendpay */ + PAY_FLOW_NOT_STARTED, + /* Normally, here */ + PAY_FLOW_IN_PROGRESS, + /* Failed: we've fed the data back to the uncertainly network. */ + PAY_FLOW_FAILED, + /* Failed from the final node, so give up: see ->final_error. */ + PAY_FLOW_FAILED_FINAL, + /* Failed, but still updating gossip. */ + PAY_FLOW_FAILED_GOSSIP_PENDING, + /* Succeeded: see ->payment_preimage. */ + PAY_FLOW_SUCCESS, +}; +#define NUM_PAY_FLOW (PAY_FLOW_SUCCESS + 1) + /* This is like a struct flow, but independent of gossmap, and contains * all we need to actually send the part payment. */ struct pay_flow { + /* Linked from payment->flows */ + struct list_node list; + + enum pay_flow_state state; + /* Iff state == PAY_FLOW_SUCCESS */ + const struct preimage *payment_preimage; + /* Iff state == PAY_FAILED_FINAL */ + enum jsonrpc_errcode final_error; + const char *final_msg; + /* So we can be an independent object for callbacks. */ struct payment * payment; @@ -76,21 +103,32 @@ HTABLE_DEFINE_TYPE(struct pay_flow, payflow_get_key, payflow_key_hash, payflow_key_equal, payflow_map); - -struct pay_flow **get_payflows(struct payment *payment, - struct amount_msat amount, - struct amount_msat feebudget, - bool is_entire_payment, - const char **err_msg); - -void commit_htlc_payflow( - struct chan_extra_map *chan_extra_map, - const struct pay_flow *flow); - -void remove_htlc_payflow( - struct chan_extra_map *chan_extra_map, - struct pay_flow *flow); - +/* Add one or more IN_PROGRESS pay_flow to payment. Return NULL if we did, + * otherwise an error message (and sets *ecode). */ +const char *add_payflows(const tal_t *ctx, + struct payment *payment, + struct amount_msat amount, + struct amount_msat feebudget, + bool is_entire_payment, + enum jsonrpc_errcode *ecode); + +/* Each payflow is eventually terminated by one of these: */ + +/* We've been notified that a pay_flow has failed */ +void pay_flow_failed(struct pay_flow *pf STEALS); +/* We've been notified that a pay_flow has failed, payment is done. */ +void pay_flow_failed_final(struct pay_flow *pf STEALS, + enum jsonrpc_errcode final_error, + const char *final_msg TAKES); +/* We've been notified that a pay_flow has failed, adding gossip. */ +void pay_flow_failed_adding_gossip(struct pay_flow *pf STEALS); +/* We've finished adding gossip. */ +void pay_flow_finished_adding_gossip(struct pay_flow *pf STEALS); +/* We've been notified that a pay_flow has succeeded. */ +void pay_flow_succeeded(struct pay_flow *pf STEALS, + const struct preimage *preimage); + +/* Formatting helpers */ const char *flow_path_to_str(const tal_t *ctx, const struct pay_flow *flow); const char* fmt_payflows(const tal_t *ctx, @@ -99,9 +137,4 @@ const char* fmt_payflows(const tal_t *ctx, /* How much does this flow deliver to destination? */ struct amount_msat payflow_delivered(const struct pay_flow *flow); -/* Removes amounts from payment and frees flow pointer. - * A possible destructor for flow would remove HTLCs from the - * uncertainty_network and remove the flow from any data structure. */ -struct pay_flow* payflow_fail(struct pay_flow *flow); - #endif /* LIGHTNING_PLUGINS_RENEPAY_PAY_FLOW_H */ diff --git a/plugins/renepay/payment.c b/plugins/renepay/payment.c index 7bb5b257fef8..9012acee0642 100644 --- a/plugins/renepay/payment.c +++ b/plugins/renepay/payment.c @@ -1,6 +1,8 @@ #include "config.h" #include +#include #include +#include #include struct payment *payment_new(const tal_t *ctx, @@ -47,6 +49,7 @@ struct payment *payment_new(const tal_t *ctx, p->payment_secret = tal_dup_or_null(p, struct secret, payment_secret); p->payment_metadata = tal_dup_talarr(p, u8, payment_metadata); p->status=PAYMENT_PENDING; + list_head_init(&p->flows); p->final_cltv=final_cltv; // p->list= p->description = tal_strdup_or_null(p, description); @@ -63,8 +66,8 @@ struct payment *payment_new(const tal_t *ctx, p->local_gossmods = gossmap_localmods_new(p); p->disabled = tal_arr(p,struct short_channel_id,0); - p->rexmit_timer = NULL; p->next_partid=1; + p->progress_deadline = NULL; return p; } @@ -128,13 +131,8 @@ void payment_assert_delivering_all(const struct payment *p) } } -struct command_result *payment_success(struct payment *p) +static struct command_result *payment_success(struct payment *p) { - debug_info("calling %s",__PRETTY_FUNCTION__); - - p->status = PAYMENT_SUCCESS; - payment_assert_delivering_all(p); - /* We only finish command once: its destructor clears this. */ if (!p->cmd) return NULL; @@ -162,12 +160,14 @@ struct command_result *payment_fail( enum jsonrpc_errcode code, const char *fmt, ...) { - /* We only finish command once: its destructor clears this. */ - if (!payment->cmd) - return NULL; + /* We usually get called because a flow failed, but we + * can also get called because we couldn't route any more + * or some strange error. */ + payment->status = PAYMENT_FAIL; - if (payment->status != PAYMENT_SUCCESS) - payment->status = PAYMENT_FAIL; + /* We only finish command once: its destructor clears this. */ + if (!payment->cmd) + return NULL; va_list args; va_start(args, fmt); @@ -184,15 +184,200 @@ u64 payment_parts(const struct payment *payment) return payment->next_partid-1; } -/* Either the payment succeeded or failed, we need to cleanup/set the plugin - * into a valid state before the next payment. */ -void payment_cleanup(struct payment *payment) +void payment_reconsider(struct payment *payment) { - debug_info("calling %s",__PRETTY_FUNCTION__); - // TODO(eduardo): it can happen that local_gossmods removed below - // contained a set of channels for which there is information in the - // uncertainty network (chan_extra_map) and that are part of some pending - // payflow (payflow_map). Handle this situation. - payment->local_gossmods = tal_free(payment->local_gossmods); - payment->rexmit_timer = tal_free(payment->rexmit_timer); + struct pay_flow *i, *next; + bool have_state[NUM_PAY_FLOW] = {false}; + enum jsonrpc_errcode final_error, ecode; + const char *final_msg; + const char *errmsg; + + plugin_log(pay_plugin->plugin, LOG_DBG, "payment_reconsider"); + + /* Harvest results and free up finished flows */ + list_for_each_safe(&payment->flows, i, next, list) { + plugin_log(pay_plugin->plugin, LOG_DBG, "Flow in state %u", i->state); + have_state[i->state] = true; + + switch (i->state) { + case PAY_FLOW_NOT_STARTED: + /* Can't happen: we start just after we add. */ + plugin_err(pay_plugin->plugin, "flow not started?"); + case PAY_FLOW_IN_PROGRESS: + /* Don't free, it's still going! */ + continue; + case PAY_FLOW_FAILED: + break; + case PAY_FLOW_FAILED_FINAL: + final_error = i->final_error; + final_msg = tal_steal(tmpctx, i->final_msg); + break; + case PAY_FLOW_FAILED_GOSSIP_PENDING: + break; + case PAY_FLOW_SUCCESS: + if (payment->preimage) { + /* This should be impossible without breaking SHA256 */ + if (!preimage_eq(payment->preimage, + i->payment_preimage)) { + plugin_err(pay_plugin->plugin, + "Impossible preimage clash for %s: %s and %s?", + type_to_string(tmpctx, + struct sha256, + &payment->payment_hash), + type_to_string(tmpctx, + struct preimage, + payment->preimage), + type_to_string(tmpctx, + struct preimage, + i->payment_preimage)); + } + } else { + payment->preimage = tal_dup(payment, struct preimage, + i->payment_preimage); + } + break; + } + tal_free(i); + } + + /* First, did one of these succeed? */ + if (have_state[PAY_FLOW_SUCCESS]) { + plugin_log(pay_plugin->plugin, LOG_DBG, "one succeeded!"); + + switch (payment->status) { + case PAYMENT_PENDING: + /* The normal case: one part succeeded, we can succeed immediately */ + payment_success(payment); + payment->status = PAYMENT_SUCCESS; + /* fall thru */ + case PAYMENT_SUCCESS: + /* Since we already succeeded, cmd must be NULL */ + assert(payment->cmd == NULL); + break; + case PAYMENT_FAIL: + /* OK, they told us it failed, but also + * succeeded? It's theoretically possible, + * but someone screwed up. */ + plugin_log(pay_plugin->plugin, LOG_BROKEN, + "Destination %s succeeded payment %s" + " (preimage %s) after previous final failure?", + type_to_string(tmpctx, struct node_id, + &payment->destination), + type_to_string(tmpctx, struct sha256, + &payment->payment_hash), + type_to_string(tmpctx, + struct preimage, + payment->preimage)); + break; + } + + /* We don't need to do anything else. */ + return; + } + + /* One of these returned an error from the destination? */ + if (have_state[PAY_FLOW_FAILED_FINAL]) { + plugin_log(pay_plugin->plugin, LOG_DBG, "one failed final!"); + switch (payment->status) { + case PAYMENT_PENDING: + /* The normal case: we can fail immediately */ + payment_fail(payment, final_error, "%s", final_msg); + /* fall thru */ + case PAYMENT_FAIL: + /* Since we already failed, cmd must be NULL */ + assert(payment->cmd == NULL); + break; + case PAYMENT_SUCCESS: + /* OK, they told us it failed, but also + * succeeded? It's theoretically possible, + * but someone screwed up. */ + plugin_log(pay_plugin->plugin, LOG_BROKEN, + "Destination %s failed payment %s with %u/%s" + " after previous success?", + type_to_string(tmpctx, struct node_id, + &payment->destination), + type_to_string(tmpctx, struct sha256, + &payment->payment_hash), + final_error, final_msg); + break; + } + + /* We don't need to do anything else. */ + return; + } + + /* Now, do we still care about retrying the payment? It could + * have terminated a while ago, and we're just collecting + * outstanding results. */ + switch (payment->status) { + case PAYMENT_PENDING: + break; + case PAYMENT_FAIL: + case PAYMENT_SUCCESS: + assert(!payment->cmd); + plugin_log(pay_plugin->plugin, LOG_DBG, "payment already status %u!", + payment->status); + return; + } + + /* Are we waiting on addgossip? We'll come back later when + * they call pay_flow_finished_adding_gossip. */ + if (have_state[PAY_FLOW_FAILED_GOSSIP_PENDING]) { + plugin_log(pay_plugin->plugin, LOG_DBG, + "%s waiting on addgossip return", + type_to_string(tmpctx, struct sha256, + &payment->payment_hash)); + return; + } + + /* Do we still have pending payment parts? First time, we set + * up a deadline so we don't respond immediately to every + * return: it's better to gather a few failed flows before + * retrying. */ + if (have_state[PAY_FLOW_IN_PROGRESS]) { + struct timemono now = time_mono(); + + /* If we don't have a deadline yet, set it now. */ + if (!payment->progress_deadline) { + payment->progress_deadline = tal(payment, struct timemono); + *payment->progress_deadline = timemono_add(now, + time_from_msec(TIMER_COLLECT_FAILURES_MSEC)); + plugin_log(pay_plugin->plugin, LOG_DBG, "Set deadline"); + } + + /* FIXME: add timemono_before to ccan/time */ + if (time_less_(now.ts, payment->progress_deadline->ts)) { + /* Come back later. */ + /* We don't care that this temporily looks like a leak; we don't even + * care if we end up with multiple outstanding. They just check + * the progress_deadline. */ + plugin_log(pay_plugin->plugin, LOG_DBG, "Setting timer to kick us"); + notleak(plugin_timer(pay_plugin->plugin, + timemono_between(*payment->progress_deadline, now), + payment_reconsider, payment)); + return; + } + } + + /* At this point, we may have some funds to deliver (or we + * could still be waiting). */ + if (amount_msat_greater_eq(payment->total_delivering, payment->amount)) { + plugin_log(pay_plugin->plugin, LOG_DBG, "No more to deliver right now"); + assert(have_state[PAY_FLOW_IN_PROGRESS]); + return; + } + + /* If we had a deadline, reset it */ + payment->progress_deadline = tal_free(payment->progress_deadline); + + /* Before we do that, make sure we're not going over time. */ + if (time_after(time_now(), payment->stop_time)) { + payment_fail(payment, PAY_STOPPED_RETRYING, "Timed out"); + return; + } + + plugin_log(pay_plugin->plugin, LOG_DBG, "Retrying payment"); + errmsg = try_paying(tmpctx, payment, &ecode); + if (errmsg) + payment_fail(payment, ecode, "%s", errmsg); } diff --git a/plugins/renepay/payment.h b/plugins/renepay/payment.h index fff94dea5076..219b4a2f534a 100644 --- a/plugins/renepay/payment.h +++ b/plugins/renepay/payment.h @@ -8,12 +8,20 @@ enum payment_status { PAYMENT_PENDING, PAYMENT_SUCCESS, PAYMENT_FAIL }; - struct payment { /* Inside pay_plugin->payments list */ struct list_node list; - /* The command if still running (needed for timer func) */ + /* Overall, how are we going? */ + enum payment_status status; + + /* The flows we are managing. */ + struct list_head flows; + + /* Deadline for flow status collection. */ + struct timemono *progress_deadline; + + /* The command if still running */ struct command *cmd; /* Localmods to apply to gossip_map for our own use. */ @@ -22,9 +30,6 @@ struct payment { /* Channels we decided to disable for various reasons. */ struct short_channel_id *disabled; - /* Timers. */ - struct plugin_timer *rexmit_timer; - /* Used in get_payflows to set ids to each pay_flow. */ u64 next_partid; @@ -66,9 +71,6 @@ struct payment { /* Payment metadata, if specified by invoice. */ const u8 *payment_metadata; - /* To know if the last attempt failed, succeeded or is it pending. */ - enum payment_status status; - u32 final_cltv; /* Description and labels, if any. */ @@ -142,14 +144,14 @@ void payment_note(struct payment *p, const char *fmt, ...); void payment_assert_delivering_incomplete(const struct payment *p); void payment_assert_delivering_all(const struct payment *p); -struct command_result *payment_success(struct payment *payment); +/* A flow has changed state, or we've hit a timeout: do something! */ +void payment_reconsider(struct payment *p); + +u64 payment_parts(const struct payment *payment); struct command_result *payment_fail( struct payment *payment, enum jsonrpc_errcode code, const char *fmt, ...); -u64 payment_parts(const struct payment *payment); -void payment_cleanup(struct payment *payment); - #endif /* LIGHTNING_PLUGINS_RENEPAY_PAYMENT_H */ diff --git a/plugins/renepay/test/run-mcf-diamond.c b/plugins/renepay/test/run-mcf-diamond.c index b764d5b0eb91..6436e01c6281 100644 --- a/plugins/renepay/test/run-mcf-diamond.c +++ b/plugins/renepay/test/run-mcf-diamond.c @@ -17,6 +17,16 @@ #include #include +/* AUTOGENERATED MOCKS START */ +/* Generated stub for pay_plugin */ +struct pay_plugin *pay_plugin; +/* Generated stub for try_paying */ +const char *try_paying(const tal_t *ctx UNNEEDED, + struct payment *payment UNNEEDED, + enum jsonrpc_errcode *ecode UNNEEDED) +{ fprintf(stderr, "try_paying called!\n"); abort(); } +/* AUTOGENERATED MOCKS END */ + static u8 empty_map[] = { 0 }; diff --git a/plugins/renepay/test/run-mcf.c b/plugins/renepay/test/run-mcf.c index 723d53d60547..9d862f203716 100644 --- a/plugins/renepay/test/run-mcf.c +++ b/plugins/renepay/test/run-mcf.c @@ -17,6 +17,16 @@ #include #include +/* AUTOGENERATED MOCKS START */ +/* Generated stub for pay_plugin */ +struct pay_plugin *pay_plugin; +/* Generated stub for try_paying */ +const char *try_paying(const tal_t *ctx UNNEEDED, + struct payment *payment UNNEEDED, + enum jsonrpc_errcode *ecode UNNEEDED) +{ fprintf(stderr, "try_paying called!\n"); abort(); } +/* AUTOGENERATED MOCKS END */ + static void swap(int *a, int *b) { int temp = *a; diff --git a/plugins/renepay/test/run-testflow.c b/plugins/renepay/test/run-testflow.c index 711d5db78bbd..8b5111b255dc 100644 --- a/plugins/renepay/test/run-testflow.c +++ b/plugins/renepay/test/run-testflow.c @@ -16,6 +16,16 @@ #include "../uncertainty_network.c" #include "../mcf.c" +/* AUTOGENERATED MOCKS START */ +/* Generated stub for pay_plugin */ +struct pay_plugin *pay_plugin; +/* Generated stub for try_paying */ +const char *try_paying(const tal_t *ctx UNNEEDED, + struct payment *payment UNNEEDED, + enum jsonrpc_errcode *ecode UNNEEDED) +{ fprintf(stderr, "try_paying called!\n"); abort(); } +/* AUTOGENERATED MOCKS END */ + static const u8 canned_map[] = { 0x0c, 0x80, 0x00, 0x01, 0xbc, 0x86, 0xe4, 0xbf, 0x95, 0x00, 0x00, 0x00, 0x00, 0x10, 0x08, 0x00, 0x00, 0x00, 0x00, 0x00, 0x0f, 0x42, 0x40, 0x01, 0xb0, 0x01, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, diff --git a/tests/test_renepay.py b/tests/test_renepay.py index c2989552cc5f..5f24889f4622 100644 --- a/tests/test_renepay.py +++ b/tests/test_renepay.py @@ -59,7 +59,7 @@ def test_errors(node_factory, bitcoind): node_factory.join_nodes([l1, l3, l5], wait_for_announce=True, fundamount=1000000) - failmsg = r'Destination is unreacheable in the network gossip.' + failmsg = r'Destination is unknown in the network gossip.' with pytest.raises(RpcError, match=failmsg): l1.rpc.call('renepay', {'invstring': inv}) @@ -208,7 +208,7 @@ def test_limits(node_factory): # FIXME: pylightning should define these! # PAY_STOPPED_RETRYING = 210 - PAY_ROUTE_NOT_FOUND = 205 + PAY_ROUTE_TOO_EXPENSIVE = 206 inv = l6.rpc.invoice("any", "any", 'description') @@ -217,7 +217,7 @@ def test_limits(node_factory): with pytest.raises(RpcError, match=failmsg) as err: l1.rpc.call( 'renepay', {'invstring': inv['bolt11'], 'amount_msat': 1000000, 'maxfee': 1}) - assert err.value.error['code'] == PAY_ROUTE_NOT_FOUND + assert err.value.error['code'] == PAY_ROUTE_TOO_EXPENSIVE # TODO(eduardo): which error code shall we use here? # TODO(eduardo): shall we list attempts in renepay? @@ -228,7 +228,7 @@ def test_limits(node_factory): with pytest.raises(RpcError, match=failmsg) as err: l1.rpc.call( 'renepay', {'invstring': inv['bolt11'], 'amount_msat': 1000000, 'maxdelay': 0}) - assert err.value.error['code'] == PAY_ROUTE_NOT_FOUND + assert err.value.error['code'] == PAY_ROUTE_TOO_EXPENSIVE inv2 = l6.rpc.invoice("800000sat", "inv2", 'description') l1.rpc.call( From d4ea3dad8ad6db89572a23db7d4642c3a42301c4 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Thu, 10 Aug 2023 11:29:44 +0930 Subject: [PATCH 19/22] renepay: add dummy pf_resuly type to ensure we deal with the flow. We want to make sure that on every path, we terminate the flow. The simplest way to do this is encourage the pattern "return pay_flow_xxx(flow)". Indeed, this caught a few places I missed! Signed-off-by: Rusty Russell --- plugins/renepay/pay.c | 159 +++++++++++++++++-------------------- plugins/renepay/pay_flow.c | 21 +++-- plugins/renepay/pay_flow.h | 21 ++--- 3 files changed, 99 insertions(+), 102 deletions(-) diff --git a/plugins/renepay/pay.c b/plugins/renepay/pay.c index 4be441f6f9ec..e92057426434 100644 --- a/plugins/renepay/pay.c +++ b/plugins/renepay/pay.c @@ -114,8 +114,8 @@ static const char *init(struct plugin *p, } /* Sometimes we don't know exactly who to blame... */ -static void handle_unhandleable_error(struct pay_flow *flow, - const char *what) +static struct pf_result *handle_unhandleable_error(struct pay_flow *flow, + const char *what) { plugin_log(pay_plugin->plugin,LOG_DBG,"calling %s",__PRETTY_FUNCTION__); size_t n = tal_count(flow); @@ -131,8 +131,7 @@ static void handle_unhandleable_error(struct pay_flow *flow, if (n == 1) { /* This is a terminal error. */ - pay_flow_failed_final(flow, PAY_UNPARSEABLE_ONION, what); - return; + return pay_flow_failed_final(flow, PAY_UNPARSEABLE_ONION, what); } /* FIXME: check chan_extra_map, since we might have succeeded though * this node before? */ @@ -150,7 +149,7 @@ static void handle_unhandleable_error(struct pay_flow *flow, type_to_string(tmpctx, struct short_channel_id, &flow->path_scids[n])); - pay_flow_failed(flow); + return pay_flow_failed(flow); } /* We hold onto the flow (and delete the timer) while we're waiting for @@ -190,9 +189,9 @@ static struct command_result *addgossip_failure(struct command *cmd, return addgossip_done(cmd, buf, err, adg); } -static struct command_result *submit_update(struct pay_flow *flow, - const u8 *update, - struct short_channel_id errscid) +static struct pf_result *submit_update(struct pay_flow *flow, + const u8 *update, + struct short_channel_id errscid) { plugin_log(pay_plugin->plugin,LOG_DBG,"calling %s",__PRETTY_FUNCTION__); struct payment *payment = flow->payment; @@ -212,7 +211,10 @@ static struct command_result *submit_update(struct pay_flow *flow, addgossip_failure, adg); json_add_hex_talarr(req->js, "message", update); - return send_outreq(pay_plugin->plugin, req); + send_outreq(pay_plugin->plugin, req); + + /* Don't retry until we call pay_flow_finished_adding_gossip! */ + return pay_flow_failed_adding_gossip(flow); } /* Fix up the channel_update to include the type if it doesn't currently have @@ -1051,11 +1053,11 @@ static struct command_result *json_pay(struct command *cmd, } /* Terminates flow */ -static void handle_sendpay_failure_payment(struct pay_flow *flow STEALS, - const char *message, - u32 erridx, - enum onion_wire onionerr, - const u8 *raw) +static struct pf_result *handle_sendpay_failure_payment(struct pay_flow *flow STEALS, + const char *message, + u32 erridx, + enum onion_wire onionerr, + const u8 *raw) { struct short_channel_id errscid; struct payment *p = flow->payment; @@ -1073,13 +1075,11 @@ static void handle_sendpay_failure_payment(struct pay_flow *flow STEALS, message); if (onionerr == WIRE_MPP_TIMEOUT) { - pay_flow_failed(flow); - return; + return pay_flow_failed(flow); } debug_paynote(p,"final destination failure"); - pay_flow_failed_final(flow, PAY_DESTINATION_PERM_FAIL, message); - return; + return pay_flow_failed_final(flow, PAY_DESTINATION_PERM_FAIL, message); } errscid = flow->path_scids[erridx]; @@ -1112,8 +1112,7 @@ static void handle_sendpay_failure_payment(struct pay_flow *flow STEALS, debug_paynote(p, "we're removing scid %s", type_to_string(tmpctx,struct short_channel_id,&errscid)); tal_arr_expand(&p->disabled, errscid); - pay_flow_failed(flow); - return; + return pay_flow_failed(flow); /* These can be fixed (maybe) by applying the included channel_update */ case WIRE_AMOUNT_BELOW_MINIMUM: @@ -1125,29 +1124,21 @@ static void handle_sendpay_failure_payment(struct pay_flow *flow STEALS, // TODO(eduardo): check update = channel_update_from_onion_error(tmpctx, raw); if (update) - { - submit_update(flow, update, errscid); - /* Don't retry until we call pay_flow_finished_adding_gossip! */ - pay_flow_failed_adding_gossip(flow); - return; - } + return submit_update(flow, update, errscid); debug_paynote(p, "missing an update, so we're removing scid %s", type_to_string(tmpctx,struct short_channel_id,&errscid)); tal_arr_expand(&p->disabled, errscid); - return; + return pay_flow_failed(flow); case WIRE_TEMPORARY_CHANNEL_FAILURE: /* These also contain a channel_update, but in this case it's simply * advisory, not necessary. */ update = channel_update_from_onion_error(tmpctx, raw); - if (update) { - submit_update(flow, update, errscid); - /* Don't retry until we call pay_flow_finished_adding_gossip! */ - pay_flow_failed_adding_gossip(flow); - } + if (update) + return submit_update(flow, update, errscid); - return; + return pay_flow_failed(flow); /* These should only come from the final distination. */ case WIRE_MPP_TIMEOUT: @@ -1161,7 +1152,7 @@ static void handle_sendpay_failure_payment(struct pay_flow *flow STEALS, onionerr, type_to_string(tmpctx,struct short_channel_id,&errscid)); tal_arr_expand(&p->disabled, errscid); - pay_flow_failed(flow); + return pay_flow_failed(flow); } static void handle_sendpay_failure_flow(struct pay_flow *flow, @@ -1231,6 +1222,8 @@ static struct pay_flow *pay_flow_from_notification(const char *buf, return payflow_map_get(pay_plugin->payflow_map, &key); } + + static struct command_result *notification_sendpay_success( struct command *cmd, const char *buf, @@ -1255,69 +1248,43 @@ static struct command_result *notification_sendpay_success( json_tok_full(buf, params)); } - // 3. mark as success - struct payment * const p = flow->payment; - debug_assert(p); - p->status = PAYMENT_SUCCESS; - - /* We could have preimage from previous part */ - tal_free(p->preimage); - p->preimage = tal_dup(p,struct preimage, &preimage); + // 2. update information + uncertainty_network_flow_success(pay_plugin->chan_extra_map, flow); - // 4. update information and release pending HTLCs - uncertainty_network_flow_success(pay_plugin->chan_extra_map,flow); - tal_free(flow); + // 3. mark as success (frees flow) + pay_flow_succeeded(flow, &preimage); return notification_handled(cmd); } -static struct command_result *notification_sendpay_failure( - struct command *cmd, - const char *buf, - const jsmntok_t *params) +/* Dummy return ensures all paths call pay_flow_* to close flow! */ +static struct pf_result *sendpay_failure(struct pay_flow *flow, + enum jsonrpc_errcode errcode, + const char *buf, + const jsmntok_t *sub) { - struct pay_flow *flow; - const char *err; - enum jsonrpc_errcode errcode; - const jsmntok_t *sub = json_get_member(buf, params, "sendpay_failure"); - - flow = pay_flow_from_notification(buf, json_get_member(buf, sub, "data")); - if (!flow) - return notification_handled(cmd); - - err = json_scan(tmpctx, buf, sub, "{code:%}", - JSON_SCAN(json_to_jsonrpc_errcode, &errcode)); - if (err) { - plugin_err(pay_plugin->plugin, - "Bad code (%s) in sendpay_failure: %.*s", - err, - json_tok_full_len(params), - json_tok_full(buf, params)); - } + const char *msg, *err; + u32 erridx, onionerr; + const u8 *raw; /* Only one code is really actionable */ switch (errcode) { case PAY_UNPARSEABLE_ONION: debug_paynote(flow->payment, "Unparsable onion reply on route %s", flow_path_to_str(tmpctx, flow)); - err = "Unparsable onion reply"; - goto unhandleable; + return handle_unhandleable_error(flow, "Unparsable onion reply"); + case PAY_TRY_OTHER_ROUTE: break; case PAY_DESTINATION_PERM_FAIL: break; default: - pay_flow_failed_final(flow, - errcode, - "Unexpected errorcode from sendpay_failure"); - goto done; + return pay_flow_failed_final(flow, + errcode, + "Unexpected errorcode from sendpay_failure"); } - /* Extract remaining fields forto feedback */ - const char *msg; - u32 erridx, onionerr; - const u8 *raw; - + /* Extract remaining fields for feedback */ err = json_scan(tmpctx, buf, sub, "{message:%" ",data:{erring_index:%" @@ -1328,7 +1295,7 @@ static struct command_result *notification_sendpay_failure( JSON_SCAN(json_to_u32, &onionerr), JSON_SCAN_TAL(tmpctx, json_tok_bin_from_hex, &raw)); if (err) - goto unhandleable; + return handle_unhandleable_error(flow, err); /* Answer must be sane: but note, erridx can be final node! */ if (erridx > tal_count(flow->path_scids)) { @@ -1339,14 +1306,36 @@ static struct command_result *notification_sendpay_failure( } handle_sendpay_failure_flow(flow, msg, erridx, onionerr); - handle_sendpay_failure_payment(flow, msg, erridx, onionerr, raw); -done: - return notification_handled(cmd); + return handle_sendpay_failure_payment(flow, msg, erridx, onionerr, raw); +} + +static struct command_result *notification_sendpay_failure( + struct command *cmd, + const char *buf, + const jsmntok_t *params) +{ + struct pay_flow *flow; + const char *err; + enum jsonrpc_errcode errcode; + const jsmntok_t *sub = json_get_member(buf, params, "sendpay_failure"); + + flow = pay_flow_from_notification(buf, json_get_member(buf, sub, "data")); + if (!flow) + return notification_handled(cmd); + + err = json_scan(tmpctx, buf, sub, "{code:%}", + JSON_SCAN(json_to_jsonrpc_errcode, &errcode)); + if (err) { + plugin_err(pay_plugin->plugin, + "Bad code (%s) in sendpay_failure: %.*s", + err, + json_tok_full_len(params), + json_tok_full(buf, params)); + } -unhandleable: - handle_unhandleable_error(flow, err); - goto done; + sendpay_failure(flow, errcode, buf, sub); + return notification_handled(cmd); } static const struct plugin_command commands[] = { diff --git a/plugins/renepay/pay_flow.c b/plugins/renepay/pay_flow.c index 27dfb4ca2c79..26b1cdd2f8ce 100644 --- a/plugins/renepay/pay_flow.c +++ b/plugins/renepay/pay_flow.c @@ -635,19 +635,20 @@ static void payment_remove_flowamount(const struct pay_flow *pf) } /* We've been notified that a pay_flow has failed */ -void pay_flow_failed(struct pay_flow *pf) +struct pf_result *pay_flow_failed(struct pay_flow *pf) { assert(pf->state == PAY_FLOW_IN_PROGRESS); pf->state = PAY_FLOW_FAILED; payment_remove_flowamount(pf); payment_reconsider(pf->payment); + return NULL; } /* We've been notified that a pay_flow has failed, payment is done. */ -void pay_flow_failed_final(struct pay_flow *pf, - enum jsonrpc_errcode final_error, - const char *final_msg TAKES) +struct pf_result *pay_flow_failed_final(struct pay_flow *pf, + enum jsonrpc_errcode final_error, + const char *final_msg TAKES) { assert(pf->state == PAY_FLOW_IN_PROGRESS); pf->state = PAY_FLOW_FAILED_FINAL; @@ -656,32 +657,36 @@ void pay_flow_failed_final(struct pay_flow *pf, payment_remove_flowamount(pf); payment_reconsider(pf->payment); + return NULL; } /* We've been notified that a pay_flow has failed, adding gossip. */ -void pay_flow_failed_adding_gossip(struct pay_flow *pf) +struct pf_result *pay_flow_failed_adding_gossip(struct pay_flow *pf) { assert(pf->state == PAY_FLOW_IN_PROGRESS); pf->state = PAY_FLOW_FAILED_GOSSIP_PENDING; payment_remove_flowamount(pf); + return NULL; } /* We've finished adding gossip. */ -void pay_flow_finished_adding_gossip(struct pay_flow *pf) +struct pf_result *pay_flow_finished_adding_gossip(struct pay_flow *pf) { assert(pf->state == PAY_FLOW_FAILED_GOSSIP_PENDING); pf->state = PAY_FLOW_FAILED; payment_reconsider(pf->payment); + return NULL; } /* We've been notified that a pay_flow has succeeded. */ -void pay_flow_succeeded(struct pay_flow *pf, - const struct preimage *preimage) +struct pf_result *pay_flow_succeeded(struct pay_flow *pf, + const struct preimage *preimage) { assert(pf->state == PAY_FLOW_IN_PROGRESS); pf->state = PAY_FLOW_SUCCESS; pf->payment_preimage = tal_dup(pf, struct preimage, preimage); payment_reconsider(pf->payment); + return NULL; } diff --git a/plugins/renepay/pay_flow.h b/plugins/renepay/pay_flow.h index 8ef03a59fd20..4756ad7a2a4e 100644 --- a/plugins/renepay/pay_flow.h +++ b/plugins/renepay/pay_flow.h @@ -112,21 +112,24 @@ const char *add_payflows(const tal_t *ctx, bool is_entire_payment, enum jsonrpc_errcode *ecode); -/* Each payflow is eventually terminated by one of these: */ +/* Each payflow is eventually terminated by one of these. + * + * To make sure you deal with flows, they return a special type. + */ /* We've been notified that a pay_flow has failed */ -void pay_flow_failed(struct pay_flow *pf STEALS); +struct pf_result *pay_flow_failed(struct pay_flow *pf STEALS); /* We've been notified that a pay_flow has failed, payment is done. */ -void pay_flow_failed_final(struct pay_flow *pf STEALS, - enum jsonrpc_errcode final_error, - const char *final_msg TAKES); +struct pf_result *pay_flow_failed_final(struct pay_flow *pf STEALS, + enum jsonrpc_errcode final_error, + const char *final_msg TAKES); /* We've been notified that a pay_flow has failed, adding gossip. */ -void pay_flow_failed_adding_gossip(struct pay_flow *pf STEALS); +struct pf_result *pay_flow_failed_adding_gossip(struct pay_flow *pf STEALS); /* We've finished adding gossip. */ -void pay_flow_finished_adding_gossip(struct pay_flow *pf STEALS); +struct pf_result *pay_flow_finished_adding_gossip(struct pay_flow *pf STEALS); /* We've been notified that a pay_flow has succeeded. */ -void pay_flow_succeeded(struct pay_flow *pf STEALS, - const struct preimage *preimage); +struct pf_result *pay_flow_succeeded(struct pay_flow *pf STEALS, + const struct preimage *preimage); /* Formatting helpers */ const char *flow_path_to_str(const tal_t *ctx, const struct pay_flow *flow); From 07d78c9bbc6004448047d6b3e79011a3691d5913 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Thu, 10 Aug 2023 11:29:44 +0930 Subject: [PATCH 20/22] renepay: do less work in destroy_pay_flow, and reorder pay_flow.c Unifies the pay_flow resolve functions, and moves remove_htlc_payflow and commit_htlc_payflow to the top. Signed-off-by: Rusty Russell --- plugins/renepay/pay_flow.c | 186 ++++++++++++++++++------------------- 1 file changed, 91 insertions(+), 95 deletions(-) diff --git a/plugins/renepay/pay_flow.c b/plugins/renepay/pay_flow.c index 26b1cdd2f8ce..06c340e1c272 100644 --- a/plugins/renepay/pay_flow.c +++ b/plugins/renepay/pay_flow.c @@ -35,11 +35,69 @@ #define MAX_SHADOW_LEN 3 -/* FIXME: rearrange to avoid predecls. */ -static void remove_htlc_payflow(struct chan_extra_map *chan_extra_map, - struct pay_flow *flow); -static void commit_htlc_payflow(struct chan_extra_map *chan_extra_map, - const struct pay_flow *flow); +static void remove_htlc_payflow( + struct chan_extra_map *chan_extra_map, + struct pay_flow *flow) +{ + for (size_t i = 0; i < tal_count(flow->path_scids); i++) { + struct chan_extra_half *h = get_chan_extra_half_by_scid( + chan_extra_map, + flow->path_scids[i], + flow->path_dirs[i]); + if(!h) + { + plugin_err(pay_plugin->plugin, + "%s could not resolve chan_extra_half", + __PRETTY_FUNCTION__); + } + if (!amount_msat_sub(&h->htlc_total, h->htlc_total, flow->amounts[i])) + { + plugin_err(pay_plugin->plugin, + "%s could not substract HTLC amounts, " + "half total htlc amount = %s, " + "flow->amounts[%lld] = %s.", + __PRETTY_FUNCTION__, + type_to_string(tmpctx, struct amount_msat, &h->htlc_total), + i, + type_to_string(tmpctx, struct amount_msat, &flow->amounts[i])); + } + if (h->num_htlcs == 0) + { + plugin_err(pay_plugin->plugin, + "%s could not decrease HTLC count.", + __PRETTY_FUNCTION__); + } + h->num_htlcs--; + } +} + +static void commit_htlc_payflow( + struct chan_extra_map *chan_extra_map, + const struct pay_flow *flow) +{ + for (size_t i = 0; i < tal_count(flow->path_scids); i++) { + struct chan_extra_half *h = get_chan_extra_half_by_scid( + chan_extra_map, + flow->path_scids[i], + flow->path_dirs[i]); + if(!h) + { + plugin_err(pay_plugin->plugin, + "%s could not resolve chan_extra_half", + __PRETTY_FUNCTION__); + } + if (!amount_msat_add(&h->htlc_total, h->htlc_total, flow->amounts[i])) + { + plugin_err(pay_plugin->plugin, + "%s could not add HTLC amounts, " + "flow->amounts[%lld] = %s.", + __PRETTY_FUNCTION__, + i, + type_to_string(tmpctx, struct amount_msat, &flow->amounts[i])); + } + h->num_htlcs++; + } +} /* Returns CLTV, and fills in *shadow_fee, based on extending the path */ static u32 shadow_one_flow(const struct gossmap *gossmap, @@ -207,8 +265,6 @@ static u32 *shadow_additions(const tal_t *ctx, static void destroy_payment_flow(struct pay_flow *pf) { list_del_from(&pf->payment->flows, &pf->list); - remove_htlc_payflow(pay_plugin->chan_extra_map, pf); - payflow_map_del(pay_plugin->payflow_map, pf); } /* Calculates delays and converts to scids, and links to the payment. @@ -557,92 +613,41 @@ const char* fmt_payflows(const tal_t *ctx, return json_out_contents(jout,&len); } -static void remove_htlc_payflow( - struct chan_extra_map *chan_extra_map, - struct pay_flow *flow) -{ - for (size_t i = 0; i < tal_count(flow->path_scids); i++) { - struct chan_extra_half *h = get_chan_extra_half_by_scid( - chan_extra_map, - flow->path_scids[i], - flow->path_dirs[i]); - if(!h) - { - plugin_err(pay_plugin->plugin, - "%s could not resolve chan_extra_half", - __PRETTY_FUNCTION__); - } - if (!amount_msat_sub(&h->htlc_total, h->htlc_total, flow->amounts[i])) - { - plugin_err(pay_plugin->plugin, - "%s could not substract HTLC amounts, " - "half total htlc amount = %s, " - "flow->amounts[%lld] = %s.", - __PRETTY_FUNCTION__, - type_to_string(tmpctx, struct amount_msat, &h->htlc_total), - i, - type_to_string(tmpctx, struct amount_msat, &flow->amounts[i])); - } - if (h->num_htlcs == 0) - { - plugin_err(pay_plugin->plugin, - "%s could not decrease HTLC count.", - __PRETTY_FUNCTION__); - } - h->num_htlcs--; - } -} -static void commit_htlc_payflow( - struct chan_extra_map *chan_extra_map, - const struct pay_flow *flow) -{ - for (size_t i = 0; i < tal_count(flow->path_scids); i++) { - struct chan_extra_half *h = get_chan_extra_half_by_scid( - chan_extra_map, - flow->path_scids[i], - flow->path_dirs[i]); - if(!h) - { - plugin_err(pay_plugin->plugin, - "%s could not resolve chan_extra_half", - __PRETTY_FUNCTION__); - } - if (!amount_msat_add(&h->htlc_total, h->htlc_total, flow->amounts[i])) - { - plugin_err(pay_plugin->plugin, - "%s could not add HTLC amounts, " - "flow->amounts[%lld] = %s.", - __PRETTY_FUNCTION__, - i, - type_to_string(tmpctx, struct amount_msat, &flow->amounts[i])); - } - h->num_htlcs++; - } -} - /* How much does this flow deliver to destination? */ struct amount_msat payflow_delivered(const struct pay_flow *flow) { return flow->amounts[tal_count(flow->amounts)-1]; } -/* Remove (failed) payment from amounts. */ -static void payment_remove_flowamount(const struct pay_flow *pf) +static struct pf_result *pf_resolve(struct pay_flow *pf, + enum pay_flow_state oldstate, + enum pay_flow_state newstate, + bool reconsider) { - amount_msat_reduce(&pf->payment->total_delivering, - payflow_delivered(pf)); - amount_msat_reduce(&pf->payment->total_sent, pf->amounts[0]); + assert(pf->state == oldstate); + pf->state = newstate; + + /* If it didn't deliver, remove from totals */ + if (pf->state != PAY_FLOW_SUCCESS) { + amount_msat_reduce(&pf->payment->total_delivering, + payflow_delivered(pf)); + amount_msat_reduce(&pf->payment->total_sent, pf->amounts[0]); + } + + /* Subtract HTLC counters from the path */ + remove_htlc_payflow(pay_plugin->chan_extra_map, pf); + /* And remove from the global map: no more notifications about this! */ + payflow_map_del(pay_plugin->payflow_map, pf); + + if (reconsider) + payment_reconsider(pf->payment); + return NULL; } /* We've been notified that a pay_flow has failed */ struct pf_result *pay_flow_failed(struct pay_flow *pf) { - assert(pf->state == PAY_FLOW_IN_PROGRESS); - pf->state = PAY_FLOW_FAILED; - payment_remove_flowamount(pf); - - payment_reconsider(pf->payment); - return NULL; + return pf_resolve(pf, PAY_FLOW_IN_PROGRESS, PAY_FLOW_FAILED, true); } /* We've been notified that a pay_flow has failed, payment is done. */ @@ -650,23 +655,18 @@ struct pf_result *pay_flow_failed_final(struct pay_flow *pf, enum jsonrpc_errcode final_error, const char *final_msg TAKES) { - assert(pf->state == PAY_FLOW_IN_PROGRESS); - pf->state = PAY_FLOW_FAILED_FINAL; pf->final_error = final_error; pf->final_msg = tal_strdup(pf, final_msg); - payment_remove_flowamount(pf); - payment_reconsider(pf->payment); - return NULL; + return pf_resolve(pf, PAY_FLOW_IN_PROGRESS, PAY_FLOW_FAILED_FINAL, true); } /* We've been notified that a pay_flow has failed, adding gossip. */ struct pf_result *pay_flow_failed_adding_gossip(struct pay_flow *pf) { - assert(pf->state == PAY_FLOW_IN_PROGRESS); - pf->state = PAY_FLOW_FAILED_GOSSIP_PENDING; - payment_remove_flowamount(pf); - return NULL; + /* Don't bother reconsidering until addgossip done */ + return pf_resolve(pf, PAY_FLOW_IN_PROGRESS, PAY_FLOW_FAILED_GOSSIP_PENDING, + false); } /* We've finished adding gossip. */ @@ -683,10 +683,6 @@ struct pf_result *pay_flow_finished_adding_gossip(struct pay_flow *pf) struct pf_result *pay_flow_succeeded(struct pay_flow *pf, const struct preimage *preimage) { - assert(pf->state == PAY_FLOW_IN_PROGRESS); - pf->state = PAY_FLOW_SUCCESS; pf->payment_preimage = tal_dup(pf, struct preimage, preimage); - - payment_reconsider(pf->payment); - return NULL; + return pf_resolve(pf, PAY_FLOW_IN_PROGRESS, PAY_FLOW_SUCCESS, true); } From a64a6baa9c9eaf14d80e3fed0dd3f23bc6b55013 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Thu, 10 Aug 2023 11:29:44 +0930 Subject: [PATCH 21/22] renepay: trivial cleanup to rename `flow` to `pf` everywhere. Consistency FTW. Signed-off-by: Rusty Russell --- plugins/renepay/pay.c | 132 +++++++++++++------------- plugins/renepay/pay_flow.c | 28 +++--- plugins/renepay/uncertainty_network.c | 18 ++-- 3 files changed, 89 insertions(+), 89 deletions(-) diff --git a/plugins/renepay/pay.c b/plugins/renepay/pay.c index e92057426434..aae578aa65ea 100644 --- a/plugins/renepay/pay.c +++ b/plugins/renepay/pay.c @@ -114,24 +114,24 @@ static const char *init(struct plugin *p, } /* Sometimes we don't know exactly who to blame... */ -static struct pf_result *handle_unhandleable_error(struct pay_flow *flow, +static struct pf_result *handle_unhandleable_error(struct pay_flow *pf, const char *what) { plugin_log(pay_plugin->plugin,LOG_DBG,"calling %s",__PRETTY_FUNCTION__); - size_t n = tal_count(flow); + size_t n = tal_count(pf); /* We got a mangled reply. We don't know who to penalize! */ - debug_paynote(flow->payment, "%s on route %s", what, flow_path_to_str(tmpctx, flow)); + debug_paynote(pf->payment, "%s on route %s", what, flow_path_to_str(tmpctx, pf)); // TODO(eduardo): does LOG_BROKEN finish the plugin execution? plugin_log(pay_plugin->plugin, LOG_BROKEN, "%s on route %s", - what, flow_path_to_str(tmpctx, flow)); + what, flow_path_to_str(tmpctx, pf)); if (n == 1) { /* This is a terminal error. */ - return pay_flow_failed_final(flow, PAY_UNPARSEABLE_ONION, what); + return pay_flow_failed_final(pf, PAY_UNPARSEABLE_ONION, what); } /* FIXME: check chan_extra_map, since we might have succeeded though * this node before? */ @@ -144,19 +144,19 @@ static struct pf_result *handle_unhandleable_error(struct pay_flow *flow, /* Assume it's not the destination */ n = pseudorand(n-1); - tal_arr_expand(&flow->payment->disabled, flow->path_scids[n]); - debug_paynote(flow->payment, "... eliminated %s", + tal_arr_expand(&pf->payment->disabled, pf->path_scids[n]); + debug_paynote(pf->payment, "... eliminated %s", type_to_string(tmpctx, struct short_channel_id, - &flow->path_scids[n])); + &pf->path_scids[n])); - return pay_flow_failed(flow); + return pay_flow_failed(pf); } /* We hold onto the flow (and delete the timer) while we're waiting for * gossipd to receive the channel_update we got from the error. */ struct addgossip { struct short_channel_id scid; - struct pay_flow *flow; + struct pay_flow *pf; }; static struct command_result *addgossip_done(struct command *cmd, @@ -166,7 +166,7 @@ static struct command_result *addgossip_done(struct command *cmd, { plugin_log(pay_plugin->plugin,LOG_DBG,"calling %s",__PRETTY_FUNCTION__); - pay_flow_finished_adding_gossip(adg->flow); + pay_flow_finished_adding_gossip(adg->pf); tal_free(adg); return command_still_pending(cmd); @@ -178,7 +178,7 @@ static struct command_result *addgossip_failure(struct command *cmd, struct addgossip *adg) { - struct payment * payment = adg->flow->payment; + struct payment * payment = adg->pf->payment; plugin_log(pay_plugin->plugin,LOG_DBG,"calling %s",__PRETTY_FUNCTION__); debug_paynote(payment, "addgossip failed, removing channel %s (%.*s)", @@ -189,19 +189,19 @@ static struct command_result *addgossip_failure(struct command *cmd, return addgossip_done(cmd, buf, err, adg); } -static struct pf_result *submit_update(struct pay_flow *flow, +static struct pf_result *submit_update(struct pay_flow *pf, const u8 *update, struct short_channel_id errscid) { plugin_log(pay_plugin->plugin,LOG_DBG,"calling %s",__PRETTY_FUNCTION__); - struct payment *payment = flow->payment; + struct payment *payment = pf->payment; struct out_req *req; - struct addgossip *adg = tal(flow, struct addgossip); + struct addgossip *adg = tal(pf, struct addgossip); /* We need to stash scid in case this fails, and we need to hold flow so * we don't get a rexmit before this is complete. */ adg->scid = errscid; - adg->flow = flow; + adg->pf = pf; debug_paynote(payment, "... extracted channel_update, telling gossipd"); plugin_log(pay_plugin->plugin, LOG_DBG, "(update = %s)", tal_hex(tmpctx, update)); @@ -214,7 +214,7 @@ static struct pf_result *submit_update(struct pay_flow *flow, send_outreq(pay_plugin->plugin, req); /* Don't retry until we call pay_flow_finished_adding_gossip! */ - return pay_flow_failed_adding_gossip(flow); + return pay_flow_failed_adding_gossip(pf); } /* Fix up the channel_update to include the type if it doesn't currently have @@ -277,7 +277,7 @@ static u8 *channel_update_from_onion_error(const tal_t *ctx, static struct command_result *flow_sent(struct command *cmd, const char *buf, const jsmntok_t *result, - struct pay_flow *flow) + struct pay_flow *pf) { plugin_log(pay_plugin->plugin,LOG_DBG,"calling %s",__PRETTY_FUNCTION__); return command_still_pending(cmd); @@ -290,9 +290,9 @@ static struct command_result *flow_sent(struct command *cmd, static struct command_result *flow_sendpay_failed(struct command *cmd, const char *buf, const jsmntok_t *err, - struct pay_flow *flow) + struct pay_flow *pf) { - struct payment *payment = flow->payment; + struct payment *payment = pf->payment; enum jsonrpc_errcode errcode; const char *msg; @@ -316,9 +316,9 @@ static struct command_result *flow_sendpay_failed(struct command *cmd, /* There is no new knowledge from this kind of failure. * We just disable this scid. */ - tal_arr_expand(&payment->disabled, flow->path_scids[0]); + tal_arr_expand(&payment->disabled, pf->path_scids[0]); - pay_flow_failed(flow); + pay_flow_failed(pf); return command_still_pending(cmd); } @@ -1053,21 +1053,21 @@ static struct command_result *json_pay(struct command *cmd, } /* Terminates flow */ -static struct pf_result *handle_sendpay_failure_payment(struct pay_flow *flow STEALS, +static struct pf_result *handle_sendpay_failure_payment(struct pay_flow *pf STEALS, const char *message, u32 erridx, enum onion_wire onionerr, const u8 *raw) { struct short_channel_id errscid; - struct payment *p = flow->payment; + struct payment *p = pf->payment; const u8 *update; - debug_assert(flow); + debug_assert(pf); debug_assert(p); /* Final node is usually a hard failure */ - if (erridx == tal_count(flow->path_scids)) { + if (erridx == tal_count(pf->path_scids)) { debug_paynote(p, "onion error %s from final node #%u: %s", onion_wire_name(onionerr), @@ -1075,14 +1075,14 @@ static struct pf_result *handle_sendpay_failure_payment(struct pay_flow *flow ST message); if (onionerr == WIRE_MPP_TIMEOUT) { - return pay_flow_failed(flow); + return pay_flow_failed(pf); } debug_paynote(p,"final destination failure"); - return pay_flow_failed_final(flow, PAY_DESTINATION_PERM_FAIL, message); + return pay_flow_failed_final(pf, PAY_DESTINATION_PERM_FAIL, message); } - errscid = flow->path_scids[erridx]; + errscid = pf->path_scids[erridx]; debug_paynote(p, "onion error %s from node #%u %s: %s", onion_wire_name(onionerr), @@ -1112,7 +1112,7 @@ static struct pf_result *handle_sendpay_failure_payment(struct pay_flow *flow ST debug_paynote(p, "we're removing scid %s", type_to_string(tmpctx,struct short_channel_id,&errscid)); tal_arr_expand(&p->disabled, errscid); - return pay_flow_failed(flow); + return pay_flow_failed(pf); /* These can be fixed (maybe) by applying the included channel_update */ case WIRE_AMOUNT_BELOW_MINIMUM: @@ -1124,21 +1124,21 @@ static struct pf_result *handle_sendpay_failure_payment(struct pay_flow *flow ST // TODO(eduardo): check update = channel_update_from_onion_error(tmpctx, raw); if (update) - return submit_update(flow, update, errscid); + return submit_update(pf, update, errscid); debug_paynote(p, "missing an update, so we're removing scid %s", type_to_string(tmpctx,struct short_channel_id,&errscid)); tal_arr_expand(&p->disabled, errscid); - return pay_flow_failed(flow); + return pay_flow_failed(pf); case WIRE_TEMPORARY_CHANNEL_FAILURE: /* These also contain a channel_update, but in this case it's simply * advisory, not necessary. */ update = channel_update_from_onion_error(tmpctx, raw); if (update) - return submit_update(flow, update, errscid); + return submit_update(pf, update, errscid); - return pay_flow_failed(flow); + return pay_flow_failed(pf); /* These should only come from the final distination. */ case WIRE_MPP_TIMEOUT: @@ -1152,48 +1152,48 @@ static struct pf_result *handle_sendpay_failure_payment(struct pay_flow *flow ST onionerr, type_to_string(tmpctx,struct short_channel_id,&errscid)); tal_arr_expand(&p->disabled, errscid); - return pay_flow_failed(flow); + return pay_flow_failed(pf); } -static void handle_sendpay_failure_flow(struct pay_flow *flow, +static void handle_sendpay_failure_flow(struct pay_flow *pf, const char *msg, u32 erridx, u32 onionerr) { - debug_assert(flow); + debug_assert(pf); - struct payment * const p = flow->payment; + struct payment * const p = pf->payment; plugin_log(pay_plugin->plugin, LOG_UNUSUAL, "onion error %s from node #%u %s: " "%s", onion_wire_name(onionerr), erridx, - erridx == tal_count(flow->path_scids) + erridx == tal_count(pf->path_scids) ? "final" - : type_to_string(tmpctx, struct short_channel_id, &flow->path_scids[erridx]), + : type_to_string(tmpctx, struct short_channel_id, &pf->path_scids[erridx]), msg); /* we know that all channels before erridx where able to commit to this payment */ uncertainty_network_channel_can_send( pay_plugin->chan_extra_map, - flow, + pf, erridx); /* Insufficient funds (not from final, that's weird!) */ if((enum onion_wire)onionerr == WIRE_TEMPORARY_CHANNEL_FAILURE - && erridx < tal_count(flow->path_scids)) + && erridx < tal_count(pf->path_scids)) { plugin_log(pay_plugin->plugin,LOG_DBG, "sendpay_failure says insufficient funds!"); chan_extra_cannot_send(p,pay_plugin->chan_extra_map, - flow->path_scids[erridx], - flow->path_dirs[erridx], + pf->path_scids[erridx], + pf->path_dirs[erridx], /* This channel can't send all that was * commited in HTLCs. * Had we removed the commited amount then - * we would have to put here flow->amounts[erridx]. */ + * we would have to put here pf->amounts[erridx]. */ AMOUNT_MSAT(0)); } } @@ -1229,13 +1229,13 @@ static struct command_result *notification_sendpay_success( const char *buf, const jsmntok_t *params) { - struct pay_flow *flow; + struct pay_flow *pf; struct preimage preimage; const char *err; const jsmntok_t *sub = json_get_member(buf, params, "sendpay_success"); - flow = pay_flow_from_notification(buf, sub); - if (!flow) + pf = pay_flow_from_notification(buf, sub); + if (!pf) return notification_handled(cmd); err = json_scan(tmpctx, buf, sub, "{payment_preimage:%}", @@ -1249,16 +1249,16 @@ static struct command_result *notification_sendpay_success( } // 2. update information - uncertainty_network_flow_success(pay_plugin->chan_extra_map, flow); + uncertainty_network_flow_success(pay_plugin->chan_extra_map, pf); - // 3. mark as success (frees flow) - pay_flow_succeeded(flow, &preimage); + // 3. mark as success (frees pf) + pay_flow_succeeded(pf, &preimage); return notification_handled(cmd); } /* Dummy return ensures all paths call pay_flow_* to close flow! */ -static struct pf_result *sendpay_failure(struct pay_flow *flow, +static struct pf_result *sendpay_failure(struct pay_flow *pf, enum jsonrpc_errcode errcode, const char *buf, const jsmntok_t *sub) @@ -1270,16 +1270,16 @@ static struct pf_result *sendpay_failure(struct pay_flow *flow, /* Only one code is really actionable */ switch (errcode) { case PAY_UNPARSEABLE_ONION: - debug_paynote(flow->payment, "Unparsable onion reply on route %s", - flow_path_to_str(tmpctx, flow)); - return handle_unhandleable_error(flow, "Unparsable onion reply"); + debug_paynote(pf->payment, "Unparsable onion reply on route %s", + flow_path_to_str(tmpctx, pf)); + return handle_unhandleable_error(pf, "Unparsable onion reply"); case PAY_TRY_OTHER_ROUTE: break; case PAY_DESTINATION_PERM_FAIL: break; default: - return pay_flow_failed_final(flow, + return pay_flow_failed_final(pf, errcode, "Unexpected errorcode from sendpay_failure"); } @@ -1295,19 +1295,19 @@ static struct pf_result *sendpay_failure(struct pay_flow *flow, JSON_SCAN(json_to_u32, &onionerr), JSON_SCAN_TAL(tmpctx, json_tok_bin_from_hex, &raw)); if (err) - return handle_unhandleable_error(flow, err); + return handle_unhandleable_error(pf, err); /* Answer must be sane: but note, erridx can be final node! */ - if (erridx > tal_count(flow->path_scids)) { + if (erridx > tal_count(pf->path_scids)) { plugin_err(pay_plugin->plugin, "Erring channel %u/%zu in path %s", - erridx, tal_count(flow->path_scids), - flow_path_to_str(tmpctx, flow)); + erridx, tal_count(pf->path_scids), + flow_path_to_str(tmpctx, pf)); } - handle_sendpay_failure_flow(flow, msg, erridx, onionerr); + handle_sendpay_failure_flow(pf, msg, erridx, onionerr); - return handle_sendpay_failure_payment(flow, msg, erridx, onionerr, raw); + return handle_sendpay_failure_payment(pf, msg, erridx, onionerr, raw); } static struct command_result *notification_sendpay_failure( @@ -1315,13 +1315,13 @@ static struct command_result *notification_sendpay_failure( const char *buf, const jsmntok_t *params) { - struct pay_flow *flow; + struct pay_flow *pf; const char *err; enum jsonrpc_errcode errcode; const jsmntok_t *sub = json_get_member(buf, params, "sendpay_failure"); - flow = pay_flow_from_notification(buf, json_get_member(buf, sub, "data")); - if (!flow) + pf = pay_flow_from_notification(buf, json_get_member(buf, sub, "data")); + if (!pf) return notification_handled(cmd); err = json_scan(tmpctx, buf, sub, "{code:%}", @@ -1334,7 +1334,7 @@ static struct command_result *notification_sendpay_failure( json_tok_full(buf, params)); } - sendpay_failure(flow, errcode, buf, sub); + sendpay_failure(pf, errcode, buf, sub); return notification_handled(cmd); } diff --git a/plugins/renepay/pay_flow.c b/plugins/renepay/pay_flow.c index 06c340e1c272..7454bf626fc8 100644 --- a/plugins/renepay/pay_flow.c +++ b/plugins/renepay/pay_flow.c @@ -37,29 +37,29 @@ static void remove_htlc_payflow( struct chan_extra_map *chan_extra_map, - struct pay_flow *flow) + struct pay_flow *pf) { - for (size_t i = 0; i < tal_count(flow->path_scids); i++) { + for (size_t i = 0; i < tal_count(pf->path_scids); i++) { struct chan_extra_half *h = get_chan_extra_half_by_scid( chan_extra_map, - flow->path_scids[i], - flow->path_dirs[i]); + pf->path_scids[i], + pf->path_dirs[i]); if(!h) { plugin_err(pay_plugin->plugin, "%s could not resolve chan_extra_half", __PRETTY_FUNCTION__); } - if (!amount_msat_sub(&h->htlc_total, h->htlc_total, flow->amounts[i])) + if (!amount_msat_sub(&h->htlc_total, h->htlc_total, pf->amounts[i])) { plugin_err(pay_plugin->plugin, "%s could not substract HTLC amounts, " "half total htlc amount = %s, " - "flow->amounts[%lld] = %s.", + "pf->amounts[%lld] = %s.", __PRETTY_FUNCTION__, type_to_string(tmpctx, struct amount_msat, &h->htlc_total), i, - type_to_string(tmpctx, struct amount_msat, &flow->amounts[i])); + type_to_string(tmpctx, struct amount_msat, &pf->amounts[i])); } if (h->num_htlcs == 0) { @@ -73,27 +73,27 @@ static void remove_htlc_payflow( static void commit_htlc_payflow( struct chan_extra_map *chan_extra_map, - const struct pay_flow *flow) + const struct pay_flow *pf) { - for (size_t i = 0; i < tal_count(flow->path_scids); i++) { + for (size_t i = 0; i < tal_count(pf->path_scids); i++) { struct chan_extra_half *h = get_chan_extra_half_by_scid( chan_extra_map, - flow->path_scids[i], - flow->path_dirs[i]); + pf->path_scids[i], + pf->path_dirs[i]); if(!h) { plugin_err(pay_plugin->plugin, "%s could not resolve chan_extra_half", __PRETTY_FUNCTION__); } - if (!amount_msat_add(&h->htlc_total, h->htlc_total, flow->amounts[i])) + if (!amount_msat_add(&h->htlc_total, h->htlc_total, pf->amounts[i])) { plugin_err(pay_plugin->plugin, "%s could not add HTLC amounts, " - "flow->amounts[%lld] = %s.", + "pf->amounts[%lld] = %s.", __PRETTY_FUNCTION__, i, - type_to_string(tmpctx, struct amount_msat, &flow->amounts[i])); + type_to_string(tmpctx, struct amount_msat, &pf->amounts[i])); } h->num_htlcs++; } diff --git a/plugins/renepay/uncertainty_network.c b/plugins/renepay/uncertainty_network.c index 832d7fb7f9a0..4821de47eb31 100644 --- a/plugins/renepay/uncertainty_network.c +++ b/plugins/renepay/uncertainty_network.c @@ -186,34 +186,34 @@ void uncertainty_network_update( void uncertainty_network_flow_success( struct chan_extra_map *chan_extra_map, - struct pay_flow *flow) + struct pay_flow *pf) { - for (size_t i = 0; i < tal_count(flow->path_scids); i++) + for (size_t i = 0; i < tal_count(pf->path_scids); i++) { chan_extra_sent_success( chan_extra_map, - flow->path_scids[i], - flow->path_dirs[i], - flow->amounts[i]); + pf->path_scids[i], + pf->path_dirs[i], + pf->amounts[i]); } } /* All parts up to erridx succeeded, so we know something about min * capacity! */ void uncertainty_network_channel_can_send( struct chan_extra_map * chan_extra_map, - struct pay_flow *flow, + struct pay_flow *pf, u32 erridx) { for (size_t i = 0; i < erridx; i++) { chan_extra_can_send(chan_extra_map, - flow->path_scids[i], - flow->path_dirs[i], + pf->path_scids[i], + pf->path_dirs[i], /* This channel can send all that was * commited in HTLCs. * Had we removed the commited amount then - * we would have to put here flow->amounts[i]. */ + * we would have to put here pf->amounts[i]. */ AMOUNT_MSAT(0)); } } From 4f25609451bc956da0af4a2584522f24a95a2bf5 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Thu, 10 Aug 2023 11:29:44 +0930 Subject: [PATCH 22/22] renepay: add command notifications These show that we should clean up our notes. Here's the result from test_hardmpp: # we have computed a set of 1 flows with probability 0.328, fees 0msat and delay 23 # No MPP, so added 0msat shadow fee # Shadow route on flow 0/1 added 0 block delay. now 5 # sendpay flow groupid=1, partid=1, delivering=1800000000msat, probability=0.328 # Update chan knowledge scid=103x2x0, dir=0: [0msat,1799999999msat] # onion error WIRE_TEMPORARY_CHANNEL_FAILURE from node #1 103x2x0: failed: WIRE_TEMPORARY_CHANNEL_FAILURE (reply from remote) # we have computed a set of 2 flows with probability 0.115, fees 0msat and delay 23 # Shadow route on flow 0/2 added 0 block delay. now 5 # Shadow route on flow 1/2 added 0 block delay. now 5 # sendpay flow groupid=1, partid=3, delivering=500000000msat, probability=0.475 # sendpay flow groupid=1, partid=2, delivering=1300000000msat, probability=0.242 # Update chan knowledge scid=103x2x0, dir=0: [0msat,1299999999msat] # onion error WIRE_TEMPORARY_CHANNEL_FAILURE from node #1 103x2x0: failed: WIRE_TEMPORARY_CHANNEL_FAILURE (reply from remote) # we have computed a set of 2 flows with probability 0.084, fees 0msat and delay 23 # Shadow route on flow 0/2 added 0 block delay. now 5 # Shadow route on flow 1/2 added 0 block delay. now 5 # sendpay flow groupid=1, partid=5, delivering=260000000msat, probability=0.467 # sendpay flow groupid=1, partid=4, delivering=1040000000msat, probability=0.179 # Update chan knowledge scid=103x2x0, dir=0: [0msat,1039999999msat] # onion error WIRE_TEMPORARY_CHANNEL_FAILURE from node #1 103x2x0: failed: WIRE_TEMPORARY_CHANNEL_FAILURE (reply from remote) # we have computed a set of 2 flows with probability 0.052, fees 0msat and delay 23 # Shadow route on flow 0/2 added 0 block delay. now 5 # Shadow route on flow 1/2 added 0 block delay. now 5 # sendpay flow groupid=1, partid=7, delivering=120000000msat, probability=0.494 # sendpay flow groupid=1, partid=6, delivering=920000000msat, probability=0.105 Ideally it would look something like: # Computed 1 flows, probability=0.328: # Flow 1: 103x2x0 1800000000msat fee=0msat probability=0.328 shadow=+0msat/0blocks # Flow 1: FAIL: TEMPORARY_CHANNEL_FAILURE for 103x2x0. # Computed 2 flows, probability=0.115: # Flow 2: XXX->XXX 1300000000msat fee=XXX, probability=0.475 shadow=+0msat/0blocks # Flow 3: XXX->XXX 500000000msat fee=XXX, probability=0.475 shadow=+0msat/0blocks # Flow 2: FAIL: TEMPORARY_CHANNEL_FAILURE from node #1 103x2x0 # Computed 2 flows (3 total), probability=0.084, fee=0msat, delay=23 ... # Flow 4: SUCCESS, 2 in progress should succeed soon. Signed-off-by: Rusty Russell --- plugins/renepay/payment.c | 3 +++ tests/test_renepay.py | 18 +++++++++++++++--- 2 files changed, 18 insertions(+), 3 deletions(-) diff --git a/plugins/renepay/payment.c b/plugins/renepay/payment.c index 9012acee0642..da40162847b0 100644 --- a/plugins/renepay/payment.c +++ b/plugins/renepay/payment.c @@ -108,6 +108,9 @@ void payment_note(struct payment *p, const char *fmt, ...) va_end(ap); tal_arr_expand(&p->paynotes, str); debug_info("%s",str); + + if (p->cmd) + plugin_notify_message(p->cmd, LOG_INFORM, "%s", str); } void payment_assert_delivering_incomplete(const struct payment *p) diff --git a/tests/test_renepay.py b/tests/test_renepay.py index 5f24889f4622..7029f213f3ee 100644 --- a/tests/test_renepay.py +++ b/tests/test_renepay.py @@ -1,10 +1,11 @@ from fixtures import * # noqa: F401,F403 from pyln.client import RpcError, Millisatoshi -from utils import only_one, wait_for, mine_funding_to_announce, sync_blockheight +from utils import only_one, wait_for, mine_funding_to_announce, sync_blockheight, TEST_NETWORK import pytest import random import time import json +import subprocess def test_simple(node_factory): @@ -309,8 +310,19 @@ def test_hardmpp(node_factory): print(json.dumps(l3.rpc.listpeerchannels()), file=f) inv2 = l6.rpc.invoice("1800000sat", "inv2", 'description') - l1.rpc.call( - 'renepay', {'invstring': inv2['bolt11']}) + + out = subprocess.check_output(['cli/lightning-cli', + '--network={}'.format(TEST_NETWORK), + '--lightning-dir={}' + .format(l1.daemon.lightning_dir), + '-k', + 'renepay', 'invstring={}'.format(inv2['bolt11'])]).decode('utf-8') + lines = out.split('\n') + # First comes commentry + assert any([l.startswith('#') for l in lines]) + + # Now comes JSON + json.loads("".join([l for l in lines if not l.startswith('#')])) l1.wait_for_htlcs() invoice = only_one(l6.rpc.listinvoices('inv2')['invoices']) assert isinstance(invoice['amount_received_msat'], Millisatoshi)