diff --git a/lightningd/invoice.c b/lightningd/invoice.c index 866f054f6fac..87ef0e31d196 100644 --- a/lightningd/invoice.c +++ b/lightningd/invoice.c @@ -302,8 +302,15 @@ invoice_check_payment(const tal_t *ctx, * - MUST fail the HTLC. * - MUST return an `incorrect_or_unknown_payment_details` error. */ - if (!wallet_invoice_find_unpaid(ld->wallet, &invoice, payment_hash)) + if (!wallet_invoice_find_unpaid(ld->wallet, &invoice, payment_hash)) { + log_debug(ld->log, "Unknown paid invoice %s", + type_to_string(tmpctx, struct sha256, payment_hash)); + if (wallet_invoice_find_by_rhash(ld->wallet, &invoice, payment_hash)) { + log_debug(ld->log, "ALREADY paid invoice %s", + type_to_string(tmpctx, struct sha256, payment_hash)); + } return NULL; + } details = wallet_invoice_details(ctx, ld->wallet, invoice); @@ -342,11 +349,22 @@ invoice_check_payment(const tal_t *ctx, if (details->msat != NULL) { struct amount_msat twice; - if (amount_msat_less(msat, *details->msat)) + if (amount_msat_less(msat, *details->msat)) { + log_debug(ld->log, "Attept to pay %s with amount %s < %s", + type_to_string(tmpctx, struct sha256, + &details->rhash), + type_to_string(tmpctx, struct amount_msat, &msat), + type_to_string(tmpctx, struct amount_msat, details->msat)); return tal_free(details); + } if (amount_msat_add(&twice, *details->msat, *details->msat) && amount_msat_greater(msat, twice)) { + log_debug(ld->log, "Attept to pay %s with amount %s > %s", + type_to_string(tmpctx, struct sha256, + &details->rhash), + type_to_string(tmpctx, struct amount_msat, details->msat), + type_to_string(tmpctx, struct amount_msat, &twice)); /* BOLT #4: * * - if the amount paid is more than twice the amount diff --git a/lightningd/peer_htlcs.c b/lightningd/peer_htlcs.c index 641a325b2c8e..3d6a0eead17c 100644 --- a/lightningd/peer_htlcs.c +++ b/lightningd/peer_htlcs.c @@ -283,10 +283,13 @@ void local_fail_in_htlc_needs_update(struct htlc_in *hin, } /* Helper to create (common) WIRE_INCORRECT_OR_UNKNOWN_PAYMENT_DETAILS */ -const u8 *failmsg_incorrect_or_unknown(const tal_t *ctx, - struct lightningd *ld, - const struct htlc_in *hin) +const u8 *failmsg_incorrect_or_unknown_(const tal_t *ctx, + struct lightningd *ld, + const struct htlc_in *hin, + const char *file, int line) { + log_debug(ld->log, "WIRE_INCORRECT_OR_UNKNOWN_PAYMENT_DETAILS: %s:%u", + file, line); return towire_incorrect_or_unknown_payment_details( ctx, hin->msat, get_block_height(ld->topology)); diff --git a/lightningd/peer_htlcs.h b/lightningd/peer_htlcs.h index c24f76555643..6d786c510a74 100644 --- a/lightningd/peer_htlcs.h +++ b/lightningd/peer_htlcs.h @@ -81,7 +81,11 @@ void json_format_forwarding_object(struct json_stream *response, const char *fie const struct forwarding *cur); /* Helper to create (common) WIRE_INCORRECT_OR_UNKNOWN_PAYMENT_DETAILS */ -const u8 *failmsg_incorrect_or_unknown(const tal_t *ctx, - struct lightningd *ld, - const struct htlc_in *hin); +#define failmsg_incorrect_or_unknown(ctx, ld, hin) \ + failmsg_incorrect_or_unknown_((ctx), (ld), (hin), __FILE__, __LINE__) + +const u8 *failmsg_incorrect_or_unknown_(const tal_t *ctx, + struct lightningd *ld, + const struct htlc_in *hin, + const char *file, int line); #endif /* LIGHTNING_LIGHTNINGD_PEER_HTLCS_H */ diff --git a/lightningd/test/run-invoice-select-inchan.c b/lightningd/test/run-invoice-select-inchan.c index e7d426b0c0d5..1d8e4ba9d395 100644 --- a/lightningd/test/run-invoice-select-inchan.c +++ b/lightningd/test/run-invoice-select-inchan.c @@ -88,11 +88,12 @@ char *encode_scriptpubkey_to_addr(const tal_t *ctx UNNEEDED, const struct chainparams *chainparams UNNEEDED, const u8 *scriptPubkey UNNEEDED) { fprintf(stderr, "encode_scriptpubkey_to_addr called!\n"); abort(); } -/* Generated stub for failmsg_incorrect_or_unknown */ -const u8 *failmsg_incorrect_or_unknown(const tal_t *ctx UNNEEDED, - struct lightningd *ld UNNEEDED, - const struct htlc_in *hin UNNEEDED) -{ fprintf(stderr, "failmsg_incorrect_or_unknown called!\n"); abort(); } +/* Generated stub for failmsg_incorrect_or_unknown_ */ +const u8 *failmsg_incorrect_or_unknown_(const tal_t *ctx UNNEEDED, + struct lightningd *ld UNNEEDED, + const struct htlc_in *hin UNNEEDED, + const char *file UNNEEDED, int line UNNEEDED) +{ fprintf(stderr, "failmsg_incorrect_or_unknown_ called!\n"); abort(); } /* Generated stub for fatal */ void fatal(const char *fmt UNNEEDED, ...) { fprintf(stderr, "fatal called!\n"); abort(); } diff --git a/plugins/autoclean.c b/plugins/autoclean.c index acdab496ea0a..eecdd52f719d 100644 --- a/plugins/autoclean.c +++ b/plugins/autoclean.c @@ -88,7 +88,7 @@ static const struct plugin_command commands[] = { { int main(int argc, char *argv[]) { setup_locale(); - plugin_main(argv, init, PLUGIN_STATIC, NULL, commands, ARRAY_SIZE(commands), + plugin_main(argv, init, PLUGIN_STATIC, true, NULL, commands, ARRAY_SIZE(commands), NULL, 0, NULL, 0, plugin_option("autocleaninvoice-cycle", "string", diff --git a/plugins/bcli.c b/plugins/bcli.c index e3936a359558..2aae8acc2d0f 100644 --- a/plugins/bcli.c +++ b/plugins/bcli.c @@ -961,7 +961,8 @@ int main(int argc, char *argv[]) /* Initialize our global context object here to handle startup options. */ bitcoind = new_bitcoind(NULL); - plugin_main(argv, init, PLUGIN_STATIC, NULL, commands, ARRAY_SIZE(commands), + plugin_main(argv, init, PLUGIN_STATIC, false /* Do not init RPC on startup*/, + NULL, commands, ARRAY_SIZE(commands), NULL, 0, NULL, 0, plugin_option("bitcoin-datadir", "string", diff --git a/plugins/fundchannel.c b/plugins/fundchannel.c index 7caf703880a6..06b73308859d 100644 --- a/plugins/fundchannel.c +++ b/plugins/fundchannel.c @@ -446,6 +446,6 @@ static const struct plugin_command commands[] = { { int main(int argc, char *argv[]) { setup_locale(); - plugin_main(argv, init, PLUGIN_RESTARTABLE, NULL, commands, + plugin_main(argv, init, PLUGIN_RESTARTABLE, true, NULL, commands, ARRAY_SIZE(commands), NULL, 0, NULL, 0, NULL); } diff --git a/plugins/keysend.c b/plugins/keysend.c index 8d664caab080..8dc5a419b9b2 100644 --- a/plugins/keysend.c +++ b/plugins/keysend.c @@ -358,7 +358,7 @@ int main(int argc, char *argv[]) features.bits[i] = tal_arr(NULL, u8, 0); set_feature_bit(&features.bits[NODE_ANNOUNCE_FEATURE], KEYSEND_FEATUREBIT); - plugin_main(argv, init, PLUGIN_STATIC, &features, commands, + plugin_main(argv, init, PLUGIN_STATIC, true, &features, commands, ARRAY_SIZE(commands), NULL, 0, hooks, ARRAY_SIZE(hooks), NULL); } diff --git a/plugins/libplugin-pay.c b/plugins/libplugin-pay.c index aa414b2f07b8..b4f612cd1109 100644 --- a/plugins/libplugin-pay.c +++ b/plugins/libplugin-pay.c @@ -25,6 +25,7 @@ struct payment *payment_new(tal_t *ctx, struct command *cmd, p->failreason = NULL; p->getroute->riskfactorppm = 10000000; p->abort = false; + p->route = NULL; /* Copy over the relevant pieces of information. */ if (parent != NULL) { @@ -91,8 +92,7 @@ struct payment_tree_result payment_collect_result(struct payment *p) struct payment_tree_result res; size_t numchildren = tal_count(p->children); res.sent = AMOUNT_MSAT(0); - /* If we didn't have a route, we didn't attempt. */ - res.attempts = p->route == NULL ? 0 : 1; + res.attempts = 1; res.treestates = p->step; res.leafstates = 0; res.preimage = NULL; @@ -180,12 +180,63 @@ void payment_start(struct payment *p) payment_rpc_failure, p)); } -static void payment_exclude_most_expensive(struct payment *p) +static void channel_hints_update(struct payment *p, + const struct short_channel_id scid, + int direction, bool enabled, bool local, + struct amount_msat *estimated_capacity, + u16 *htlc_budget) { struct payment *root = payment_root(p); + struct channel_hint hint; + + /* If the channel is marked as enabled it must have an estimate. */ + assert(!enabled || estimated_capacity != NULL); + + /* Try and look for an existing hint: */ + for (size_t i=0; ichannel_hints); i++) { + struct channel_hint *hint = &root->channel_hints[i]; + if (short_channel_id_eq(&hint->scid.scid, &scid) && + hint->scid.dir == direction) { + /* Prefer to disable a channel. */ + hint->enabled = hint->enabled & enabled; + + /* Prefer the more conservative estimate. */ + if (estimated_capacity != NULL && + amount_msat_greater(hint->estimated_capacity, + *estimated_capacity)) + hint->estimated_capacity = *estimated_capacity; + if (htlc_budget != NULL && *htlc_budget < hint->htlc_budget) + hint->htlc_budget = *htlc_budget; + return; + } + } + + /* No hint found, create one. */ + hint.enabled = enabled; + hint.scid.scid = scid; + hint.scid.dir = direction; + hint.local = local; + if (estimated_capacity != NULL) + hint.estimated_capacity = *estimated_capacity; + + if (htlc_budget != NULL) + hint.htlc_budget = *htlc_budget; + + tal_arr_expand(&root->channel_hints, hint); + + plugin_log( + root->plugin, LOG_DBG, + "Added a channel hint for %s: enabled %s, estimated capacity %s", + type_to_string(tmpctx, struct short_channel_id_dir, &hint.scid), + hint.enabled ? "true" : "false", + type_to_string(tmpctx, struct amount_msat, + &hint.estimated_capacity)); +} + +static void payment_exclude_most_expensive(struct payment *p) +{ struct route_hop *e = &p->route[0]; struct amount_msat fee, worst = AMOUNT_MSAT(0); - struct channel_hint hint; for (size_t i = 0; i < tal_count(p->route)-1; i++) { if (!amount_msat_sub(&fee, p->route[i].amount, p->route[i+1].amount)) @@ -196,18 +247,14 @@ static void payment_exclude_most_expensive(struct payment *p) worst = fee; } } - hint.scid.scid = e->channel_id; - hint.scid.dir = e->direction; - hint.enabled = false; - tal_arr_expand(&root->channel_hints, hint); + channel_hints_update(p, e->channel_id, e->direction, false, false, + NULL, NULL); } static void payment_exclude_longest_delay(struct payment *p) { - struct payment *root = payment_root(p); struct route_hop *e = &p->route[0]; u32 delay, worst = 0; - struct channel_hint hint; for (size_t i = 0; i < tal_count(p->route)-1; i++) { delay = p->route[i].delay - p->route[i+1].delay; @@ -216,10 +263,8 @@ static void payment_exclude_longest_delay(struct payment *p) worst = delay; } } - hint.scid.scid = e->channel_id; - hint.scid.dir = e->direction; - hint.enabled = false; - tal_arr_expand(&root->channel_hints, hint); + channel_hints_update(p, e->channel_id, e->direction, false, false, + NULL, NULL); } static struct amount_msat payment_route_fee(struct payment *p) @@ -279,6 +324,14 @@ static void payment_chanhints_apply_route(struct payment *p, bool remove) if (short_channel_id_eq(&curhint->scid.scid, &curhop->channel_id) && curhint->scid.dir == curhop->direction) { + + /* Update the number of htlcs for any local + * channel in the route */ + if (curhint->local && remove) + curhint->htlc_budget++; + else if (curhint->local) + curhint->htlc_budget--; + if (remove && !amount_msat_add( &curhint->estimated_capacity, curhint->estimated_capacity, @@ -393,6 +446,10 @@ payment_get_excluded_channels(const tal_t *ctx, struct payment *p) else if (amount_msat_greater_eq(p->amount, hint->estimated_capacity)) tal_arr_expand(&res, hint->scid); + else if (hint->local && hint->htlc_budget == 0) + /* If we cannot add any HTLCs to the channel we + * shouldn't look for a route through that channel */ + tal_arr_expand(&res, hint->scid); } return res; } @@ -592,36 +649,6 @@ static struct payment_result *tal_sendpay_result_from_json(const tal_t *ctx, return tal_free(result); } -static void channel_hints_update(struct payment *root, - struct short_channel_id *scid, int direction, - bool enabled, - struct amount_msat estimated_capacity) -{ - struct channel_hint hint; - /* Try and look for an existing hint: */ - for (size_t i=0; ichannel_hints); i++) { - struct channel_hint *hint = &root->channel_hints[i]; - if (short_channel_id_eq(&hint->scid.scid, scid) && - hint->scid.dir == direction) { - /* Prefer to disable a channel. */ - hint->enabled = hint->enabled & enabled; - - /* Prefer the more conservative estimate. */ - if (amount_msat_greater(hint->estimated_capacity, - estimated_capacity)) - hint->estimated_capacity = estimated_capacity; - return; - } - } - - /* No hint found, create one. */ - hint.enabled = enabled; - hint.scid.scid = *scid; - hint.scid.dir = direction; - hint.estimated_capacity = estimated_capacity; - tal_arr_expand(&root->channel_hints, hint); -} - /* Try to infer the erring_node, erring_channel and erring_direction from what * we know, but don't override the values that are returned by `waitsendpay`. */ static void payment_result_infer(struct route_hop *route, @@ -653,113 +680,299 @@ static void payment_result_infer(struct route_hop *route, r->erring_direction = &route[i].direction; } -static struct command_result * -payment_waitsendpay_finished(struct command *cmd, const char *buffer, - const jsmntok_t *toks, struct payment *p) +/* If a node takes too much fee or cltv, the next one reports it. We don't + * know who to believe, but log it */ +static void report_tampering(struct payment *p, + size_t report_pos, + const char *style) { - struct payment *root; - struct route_hop *hop; - assert(p->route != NULL); + const struct node_id *id = &p->route[report_pos].nodeid; - p->end_time = time_now(); - p->result = tal_sendpay_result_from_json(p, buffer, toks); - - if (p->result == NULL) { + if (report_pos == 0) { plugin_log(p->plugin, LOG_UNUSUAL, - "Unable to parse `waitsendpay` result: %.*s", - json_tok_full_len(toks), - json_tok_full(buffer, toks)); - payment_set_step(p, PAYMENT_STEP_FAILED); - payment_continue(p); - return command_still_pending(cmd); + "Node #%zu (%s) claimed we sent them invalid %s", + report_pos + 1, + type_to_string(tmpctx, struct node_id, id), + style); + } else { + plugin_log(p->plugin, LOG_UNUSUAL, + "Node #%zu (%s) claimed #%zu (%s) sent them invalid %s", + report_pos + 1, + type_to_string(tmpctx, struct node_id, id), + report_pos, + type_to_string(tmpctx, struct node_id, + &p->route[report_pos-1].nodeid), + style); } +} - payment_result_infer(p->route, p->result); +static struct command_result * +handle_final_failure(struct command *cmd, + struct payment *p, + const struct node_id *final_id, + enum onion_type failcode) +{ + /* We use an exhaustive switch statement here so you get a compile + * warning when new ones are added, and can think about where they go */ + switch (failcode) { + case WIRE_FINAL_INCORRECT_CLTV_EXPIRY: + report_tampering(p, tal_count(p->route)-1, "cltv"); + goto error; + case WIRE_FINAL_INCORRECT_HTLC_AMOUNT: + report_tampering(p, tal_count(p->route)-1, "amount"); + goto error; + + /* BOLT #4: + * + * A _forwarding node_ MAY, but a _final node_ MUST NOT: + *... + * - return an `invalid_onion_version` error. + *... + * - return an `invalid_onion_hmac` error. + *... + * - return an `invalid_onion_key` error. + *... + * - return a `temporary_channel_failure` error. + *... + * - return a `permanent_channel_failure` error. + *... + * - return a `required_channel_feature_missing` error. + *... + * - return an `unknown_next_peer` error. + *... + * - return an `amount_below_minimum` error. + *... + * - return a `fee_insufficient` error. + *... + * - return an `incorrect_cltv_expiry` error. + *... + * - return an `expiry_too_soon` error. + *... + * - return an `expiry_too_far` error. + *... + * - return a `channel_disabled` error. + */ + case WIRE_INVALID_ONION_VERSION: + case WIRE_INVALID_ONION_HMAC: + case WIRE_INVALID_ONION_KEY: + case WIRE_TEMPORARY_CHANNEL_FAILURE: + case WIRE_PERMANENT_CHANNEL_FAILURE: + case WIRE_REQUIRED_CHANNEL_FEATURE_MISSING: + case WIRE_UNKNOWN_NEXT_PEER: + case WIRE_AMOUNT_BELOW_MINIMUM: + case WIRE_FEE_INSUFFICIENT: + case WIRE_INCORRECT_CLTV_EXPIRY: + case WIRE_EXPIRY_TOO_FAR: + case WIRE_EXPIRY_TOO_SOON: + case WIRE_CHANNEL_DISABLED: + goto strange_error; - if (p->result->state == PAYMENT_COMPLETE) { - payment_set_step(p, PAYMENT_STEP_SUCCESS); - payment_continue(p); - return command_still_pending(cmd); + case WIRE_INVALID_ONION_PAYLOAD: + case WIRE_INVALID_REALM: + case WIRE_PERMANENT_NODE_FAILURE: + case WIRE_TEMPORARY_NODE_FAILURE: + case WIRE_REQUIRED_NODE_FEATURE_MISSING: +#if EXPERIMENTAL_FEATURES + case WIRE_INVALID_ONION_BLINDING: +#endif + case WIRE_INCORRECT_OR_UNKNOWN_PAYMENT_DETAILS: + case WIRE_MPP_TIMEOUT: + goto error; } - root = payment_root(p); - payment_chanhints_apply_route(p, true); +strange_error: + plugin_log(p->plugin, LOG_UNUSUAL, + "Final node %s reported strange error code %u", + type_to_string(tmpctx, struct node_id, final_id), + failcode); + +error: + p->result->code = PAY_DESTINATION_PERM_FAIL; + payment_root(p)->abort = true; + + payment_fail(p, "%s", p->result->message); + return command_still_pending(cmd); +} + + +static struct command_result * +handle_intermediate_failure(struct command *cmd, + struct payment *p, + const struct node_id *errnode, + const struct route_hop *errchan, + enum onion_type failcode) +{ + struct payment *root = payment_root(p); + + /* We use an exhaustive switch statement here so you get a compile + * warning when new ones are added, and can think about where they go */ + switch (failcode) { + /* BOLT #4: + * + * An _intermediate hop_ MUST NOT, but the _final node_: + *... + * - MUST return an `incorrect_or_unknown_payment_details` error. + *... + * - MUST return `final_incorrect_cltv_expiry` error. + *... + * - MUST return a `final_incorrect_htlc_amount` error. + */ + case WIRE_INCORRECT_OR_UNKNOWN_PAYMENT_DETAILS: + case WIRE_FINAL_INCORRECT_CLTV_EXPIRY: + case WIRE_FINAL_INCORRECT_HTLC_AMOUNT: + /* FIXME: Document in BOLT that intermediates must not return this! */ + case WIRE_MPP_TIMEOUT: + goto strange_error; - switch (p->result->failcode) { case WIRE_PERMANENT_CHANNEL_FAILURE: case WIRE_CHANNEL_DISABLED: case WIRE_UNKNOWN_NEXT_PEER: case WIRE_REQUIRED_CHANNEL_FEATURE_MISSING: /* All of these result in the channel being marked as disabled. */ - assert(*p->result->erring_index < tal_count(p->route)); - hop = &p->route[*p->result->erring_index]; - channel_hints_update(root, &hop->channel_id, hop->direction, - false, AMOUNT_MSAT(0)); + channel_hints_update(root, errchan->channel_id, + errchan->direction, false, false, NULL, + NULL); break; - case WIRE_TEMPORARY_CHANNEL_FAILURE: + + case WIRE_TEMPORARY_CHANNEL_FAILURE: { /* These are an indication that the capacity was insufficient, * remember the amount we tried as an estimate. */ - assert(*p->result->erring_index < tal_count(p->route)); - hop = &p->route[*p->result->erring_index]; - struct amount_msat est = { - .millisatoshis = hop->amount.millisatoshis * 0.75}; /* Raw: Multiplication */ - channel_hints_update(root, &hop->channel_id, hop->direction, - true, est); - break; + struct amount_msat est = errchan->amount; + est.millisatoshis *= 0.75; /* Raw: Multiplication */ + channel_hints_update(root, errchan->channel_id, + errchan->direction, true, false, &est, + NULL); + goto error; + } + + case WIRE_INCORRECT_CLTV_EXPIRY: + report_tampering(p, errchan - p->route, "cltv"); + goto error; - case WIRE_INVALID_ONION_PAYLOAD: - case WIRE_INVALID_REALM: - case WIRE_PERMANENT_NODE_FAILURE: - case WIRE_TEMPORARY_NODE_FAILURE: - case WIRE_REQUIRED_NODE_FEATURE_MISSING: case WIRE_INVALID_ONION_VERSION: case WIRE_INVALID_ONION_HMAC: case WIRE_INVALID_ONION_KEY: + case WIRE_PERMANENT_NODE_FAILURE: + case WIRE_TEMPORARY_NODE_FAILURE: + case WIRE_REQUIRED_NODE_FEATURE_MISSING: + case WIRE_INVALID_ONION_PAYLOAD: + case WIRE_INVALID_REALM: #if EXPERIMENTAL_FEATURES case WIRE_INVALID_ONION_BLINDING: #endif - /* These are reported by the last hop, i.e., the destination of hop i-1. */ - assert(*p->result->erring_index - 1 < tal_count(p->route)); - hop = &p->route[*p->result->erring_index - 1]; - tal_arr_expand(&root->excluded_nodes, hop->nodeid); - break; - - case WIRE_INCORRECT_OR_UNKNOWN_PAYMENT_DETAILS: - p->result->code = PAY_DESTINATION_PERM_FAIL; - root->abort = true; - case WIRE_MPP_TIMEOUT: - /* These are permanent failures that should abort all of our - * attempts right away. We'll still track pending partial - * payments correctly, just not start new ones. */ - root->abort = true; - break; + tal_arr_expand(&root->excluded_nodes, *errnode); + goto error; case WIRE_AMOUNT_BELOW_MINIMUM: + case WIRE_FEE_INSUFFICIENT: case WIRE_EXPIRY_TOO_FAR: case WIRE_EXPIRY_TOO_SOON: - case WIRE_FEE_INSUFFICIENT: - case WIRE_INCORRECT_CLTV_EXPIRY: - case WIRE_FINAL_INCORRECT_CLTV_EXPIRY: - /* These are issues that are due to gossipd being out of date, - * we ignore them here, and wait for gossipd to adjust - * instead. */ - break; - case WIRE_FINAL_INCORRECT_HTLC_AMOUNT: - /* These are symptoms of intermediate hops tampering with the - * payment. */ - hop = &p->route[*p->result->erring_index]; - plugin_log( - p->plugin, LOG_UNUSUAL, - "Node %s reported an incorrect HTLC amount, this could be " - "a prior hop messing with the amounts.", - type_to_string(tmpctx, struct node_id, &hop->nodeid)); - break; + goto error; } +strange_error: + plugin_log(p->plugin, LOG_UNUSUAL, + "Intermediate node %s reported strange error code %u", + type_to_string(tmpctx, struct node_id, errnode), + failcode); + +error: payment_fail(p, "%s", p->result->message); return command_still_pending(cmd); } +/* From the docs: + * + * - *erring_index*: The index of the node along the route that + * reported the error. 0 for the local node, 1 for the first hop, + * and so on. + * + * The only difficulty is mapping the erring_index to the correct hop. + * We split into the erring node, and the error channel, since they're + * used in different contexts. NULL error_channel means it's the final + * node, whose errors are treated differently. + */ +static bool assign_blame(const struct payment *p, + const struct node_id **errnode, + const struct route_hop **errchan) +{ + int index; + + if (p->result->erring_index == NULL) + return false; + + index = *p->result->erring_index; + + /* BADONION errors are reported on behalf of the next node. */ + if (p->result->failcode & BADONION) + index++; + + /* Final node *shouldn't* report BADONION, but don't assume. */ + if (index >= tal_count(p->route)) { + *errchan = NULL; + *errnode = &p->route[tal_count(p->route) - 1].nodeid; + return true; + } + + *errchan = &p->route[index]; + if (index == 0) + *errnode = p->local_id; + else + *errnode = &p->route[index - 1].nodeid; + return true; +} + +static struct command_result * +payment_waitsendpay_finished(struct command *cmd, const char *buffer, + const jsmntok_t *toks, struct payment *p) +{ + const struct node_id *errnode; + const struct route_hop *errchan; + + assert(p->route != NULL); + + p->end_time = time_now(); + p->result = tal_sendpay_result_from_json(p, buffer, toks); + + if (p->result == NULL) { + plugin_log(p->plugin, LOG_UNUSUAL, + "Unable to parse `waitsendpay` result: %.*s", + json_tok_full_len(toks), + json_tok_full(buffer, toks)); + payment_set_step(p, PAYMENT_STEP_FAILED); + payment_continue(p); + return command_still_pending(cmd); + } + + payment_result_infer(p->route, p->result); + + if (p->result->state == PAYMENT_COMPLETE) { + payment_set_step(p, PAYMENT_STEP_SUCCESS); + payment_continue(p); + return command_still_pending(cmd); + } + + payment_chanhints_apply_route(p, true); + + if (!assign_blame(p, &errnode, &errchan)) { + plugin_log(p->plugin, LOG_UNUSUAL, + "No erring_index set in `waitsendpay` result: %.*s", + json_tok_full_len(toks), + json_tok_full(buffer, toks)); + /* FIXME: Pick a random channel to fail? */ + payment_set_step(p, PAYMENT_STEP_FAILED); + payment_continue(p); + return command_still_pending(cmd); + } + + if (!errchan) + return handle_final_failure(cmd, p, errnode, + p->result->failcode); + + return handle_intermediate_failure(cmd, p, errnode, errchan, + p->result->failcode); +} + static struct command_result *payment_sendonion_success(struct command *cmd, const char *buffer, const jsmntok_t *toks, @@ -840,7 +1053,7 @@ static void payment_add_hop_onion_payload(struct payment *p, struct secret *payment_secret) { struct createonion_request *cr = p->createonion_request; - u32 cltv = p->start_block + next->delay; + u32 cltv = p->start_block + next->delay + 1; u64 msat = next->amount.millisatoshis; /* Raw: TLV payload generation*/ struct tlv_field **fields; static struct short_channel_id all_zero_scid = {.u64 = 0}; @@ -1409,7 +1622,8 @@ static struct command_result * local_channel_hints_listpeers(struct command *cmd, const char *buffer, const jsmntok_t *toks, struct payment *p) { - const jsmntok_t *peers, *peer, *channels, *channel, *spendsats, *scid, *dir, *connected; + const jsmntok_t *peers, *peer, *channels, *channel, *spendsats, *scid, + *dir, *connected, *max_htlc, *htlcs; size_t i, j; peers = json_get_member(buffer, toks, "peers"); @@ -1428,14 +1642,31 @@ local_channel_hints_listpeers(struct command *cmd, const char *buffer, spendsats = json_get_member(buffer, channel, "spendable_msat"); scid = json_get_member(buffer, channel, "short_channel_id"); dir = json_get_member(buffer, channel, "direction"); - assert(spendsats != NULL && scid != NULL && dir != NULL); + max_htlc = json_get_member(buffer, channel, "max_accepted_htlcs"); + htlcs = json_get_member(buffer, channel, "htlcs"); + if (spendsats == NULL || scid == NULL || dir == NULL || + max_htlc == NULL || + max_htlc->type != JSMN_PRIMITIVE || htlcs == NULL || + htlcs->type != JSMN_ARRAY) + continue; json_to_bool(buffer, connected, &h.enabled); json_to_short_channel_id(buffer, scid, &h.scid.scid); json_to_int(buffer, dir, &h.scid.dir); json_to_msat(buffer, spendsats, &h.estimated_capacity); - tal_arr_expand(&p->channel_hints, h); + + /* Take the configured number of max_htlcs and + * subtract any HTLCs that might already be added to + * the channel. This is a best effort estimate and + * mostly considers stuck htlcs, concurrent payments + * may throw us off a bit. */ + json_to_u16(buffer, max_htlc, &h.htlc_budget); + h.htlc_budget -= htlcs->size; + h.local = true; + + channel_hints_update(p, h.scid.scid, h.scid.dir, + h.enabled, true, &h.estimated_capacity, &h.htlc_budget); } } @@ -1539,14 +1770,19 @@ static bool routehint_excluded(struct payment *p, static struct route_info *next_routehint(struct routehints_data *d, struct payment *p) { - while (tal_count(d->routehints) > 0) { - if (!routehint_excluded(p, d->routehints[0])) { - d->current_routehint = d->routehints[0]; - tal_arr_remove(&d->routehints, 0); - return d->current_routehint; - } - tal_free(d->routehints[0]); - tal_arr_remove(&d->routehints, 0); + /* Implements a random selection of a routehint, or none in 1/numhints + * cases, by starting the iteration of the routehints in a random + * order, and adding a virtual NULL result at the end. */ + size_t numhints = tal_count(d->routehints); + size_t offset = pseudorand(numhints + 1); + + for (size_t i=0; iroutehints); i++) { + size_t curr = (offset + i) % (numhints + 1); + if (curr == numhints) + return NULL; + + if (!routehint_excluded(p, d->routehints[curr])) + return d->routehints[i]; } return NULL; } @@ -1595,17 +1831,19 @@ static void routehint_step_cb(struct routehints_data *d, struct payment *p) if (root->invoice == NULL || root->invoice->routes == NULL) return payment_continue(p); - /* The root payment gets the unmodified routehints, children may - * start dropping some as they learn that they were not - * functional. */ + /* We filter out non-functional routehints once at the + * beginning, and every other payment will filter out the + * exluded ones on the fly. */ if (p->parent == NULL) { d->routehints = filter_routehints(d, p->local_id, p->invoice->routes); } else { pd = payment_mod_get_data(p->parent, &routehints_pay_mod); - d->routehints = tal_dup_talarr(d, struct route_info *, - pd->routehints); + /* Since we don't modify the list of routehints after + * the root has filtered them we can just shared a + * pointer here. */ + d->routehints = pd->routehints; } d->current_routehint = next_routehint(d, p); @@ -1621,6 +1859,11 @@ static void routehint_step_cb(struct routehints_data *d, struct payment *p) p->getroute->cltv = route_cltv(p->getroute->cltv, d->current_routehint, tal_count(d->current_routehint)); + plugin_log(p->plugin, LOG_DBG, "Using routehint %s (%s) cltv_delta=%d", + type_to_string(tmpctx, struct node_id, &d->current_routehint->pubkey), + type_to_string(tmpctx, struct short_channel_id, &d->current_routehint->short_channel_id), + d->current_routehint->cltv_expiry_delta + ); } } else if (p->step == PAYMENT_STEP_GOT_ROUTE) { /* Now it's time to stitch the two partial routes together. */ @@ -1660,8 +1903,8 @@ static void routehint_step_cb(struct routehints_data *d, struct payment *p) static struct routehints_data *routehint_data_init(struct payment *p) { - /* We defer the actual initialization to the step callback when we have - * the invoice attached. */ + /* We defer the actual initialization to the step callback when + * we have the invoice attached. */ return talz(p, struct routehints_data); } @@ -1727,13 +1970,21 @@ REGISTER_PAYMENT_MODIFIER(exemptfee, struct exemptfee_data *, static struct shadow_route_data *shadow_route_init(struct payment *p) { + struct shadow_route_data *d = tal(p, struct shadow_route_data), *pd; + + /* If we're not the root we need to inherit the flags set only on the + * root payment. Since we inherit them at each step it's sufficient to + * do so from our direct parent. */ if (p->parent != NULL) { - return payment_mod_shadowroute_get_data(p->parent); + pd = payment_mod_shadowroute_get_data(p->parent); + d->fuzz_amount = pd->fuzz_amount; +#if DEVELOPER + d->use_shadow = pd->use_shadow; +#endif } else { - struct shadow_route_data *d = tal(p, struct shadow_route_data); d->fuzz_amount = true; - return d; } + return d; } /* Mutual recursion */ @@ -1767,7 +2018,12 @@ static struct command_result *shadow_route_listchannels(struct command *cmd, u64 sample = 0; struct amount_msat best_fee; const jsmntok_t *sattok, *delaytok, *basefeetok, *propfeetok, *desttok, - *channelstok, *chan; + *channelstok, *chan, *scidtok; + + /* Check the invariants on the constraints between payment and modifier. */ + assert(d->constraints.cltv_budget <= p->constraints.cltv_budget / 4); + assert(amount_msat_greater_eq(p->constraints.fee_budget, + d->constraints.fee_budget)); channelstok = json_get_member(buf, result, "channels"); json_for_each_arr(i, chan, channelstok) { @@ -1780,18 +2036,21 @@ static struct command_result *shadow_route_listchannels(struct command *cmd, delaytok = json_get_member(buf, chan, "delay"); basefeetok = json_get_member(buf, chan, "base_fee_millisatoshi"); propfeetok = json_get_member(buf, chan, "fee_per_millionth"); + scidtok = json_get_member(buf, chan, "short_channel_id"); desttok = json_get_member(buf, chan, "destination"); if (sattok == NULL || delaytok == NULL || delaytok->type != JSMN_PRIMITIVE || basefeetok == NULL || basefeetok->type != JSMN_PRIMITIVE || propfeetok == NULL || - propfeetok->type != JSMN_PRIMITIVE || desttok == NULL) + propfeetok->type != JSMN_PRIMITIVE || desttok == NULL || + scidtok == NULL) continue; json_to_u16(buf, delaytok, &curr.cltv_expiry_delta); json_to_number(buf, basefeetok, &curr.fee_base_msat); json_to_number(buf, propfeetok, &curr.fee_proportional_millionths); + json_to_short_channel_id(buf, scidtok, &curr.short_channel_id); json_to_sat(buf, sattok, &capacity); json_to_node_id(buf, desttok, &curr.pubkey); @@ -1821,33 +2080,64 @@ static struct command_result *shadow_route_listchannels(struct command *cmd, } if (best != NULL) { - bool ok; - /* Ok, we found an extension, let's add it. */ - d->destination = best->pubkey; - - /* Apply deltas to the constraints in the shadow route so we - * don't overshoot our 1/4th target. */ - if (!payment_constraints_update(&d->constraints, best_fee, - best->cltv_expiry_delta)) { + /* Check that we could apply the shadow route extension. Check + * against both the shadow route budget as well as the + * original payment's budget. */ + if (best->cltv_expiry_delta > d->constraints.cltv_budget || + best->cltv_expiry_delta > p->constraints.cltv_budget) { best = NULL; goto next; } - /* Now do the same to the payment constraints so other - * modifiers don't do it either. */ - ok = payment_constraints_update(&p->constraints, best_fee, - best->cltv_expiry_delta); - - /* And now the thing that caused all of this: adjust the call - * to getroute. */ - if (d->fuzz_amount) { - /* Only fuzz the amount to route to the destination if - * we didn't opt-out earlier. */ - ok &= amount_msat_add(&p->getroute->amount, - p->getroute->amount, best_fee); + /* Check the fee budget only if we didn't opt out, since + * testing against a virtual budget is not useful if we do not + * actually use it (it could give false positives and fail + * attempts that might have gone through, */ + if (d->fuzz_amount && + (amount_msat_greater(best_fee, d->constraints.fee_budget) || + (amount_msat_greater(best_fee, + p->constraints.fee_budget)))) { + best = NULL; + goto next; } + + /* Now we can be sure that adding the shadow route will succeed */ + plugin_log( + p->plugin, LOG_DBG, + "Adding shadow_route hop over channel %s: adding %s " + "in fees and %d CLTV delta", + type_to_string(tmpctx, struct short_channel_id, + &best->short_channel_id), + type_to_string(tmpctx, struct amount_msat, &best_fee), + best->cltv_expiry_delta); + + d->destination = best->pubkey; + d->constraints.cltv_budget -= best->cltv_expiry_delta; p->getroute->cltv += best->cltv_expiry_delta; - assert(ok); + + if (!d->fuzz_amount) + goto next; + + /* Only try to apply the fee budget changes if we want to fuzz + * the amount. Virtual fees that we then don't deliver to the + * destination could otherwise cause the route to be too + * expensive, while really being ok. If any of these fail then + * the above checks are insufficient. */ + if (!amount_msat_sub(&d->constraints.fee_budget, + d->constraints.fee_budget, best_fee) || + !amount_msat_sub(&p->constraints.fee_budget, + p->constraints.fee_budget, best_fee)) + plugin_err(p->plugin, + "Could not update fee constraints " + "for shadow route extension. " + "payment fee budget %s, modifier " + "fee budget %s, shadow fee to add %s", + type_to_string(tmpctx, struct amount_msat, + &p->constraints.fee_budget), + type_to_string(tmpctx, struct amount_msat, + &d->constraints.fee_budget), + type_to_string(tmpctx, struct amount_msat, + &best_fee)); } next: @@ -2100,10 +2390,16 @@ REGISTER_PAYMENT_MODIFIER(waitblockheight, void *, NULL, waitblockheight_cb); * really the case, so this is likely a lower bound on the success rate. * * As the network evolves these numbers are also likely to change. + * + * Finally, if applied trivially this splitter may end up creating more splits + * than the sum of all channels can support, i.e., each split results in an + * HTLC, and each channel has an upper limit on the number of HTLCs it'll + * allow us to add. If the initial split would result in more than 1/3rd of + * the total available HTLCs we clamp the number of splits to 1/3rd. We don't + * use 3/3rds in order to retain flexibility in the adaptive splitter. */ #define MPP_TARGET_SIZE (10 * 1000 * 1000) -#define MPP_TARGET_MSAT AMOUNT_MSAT(MPP_TARGET_SIZE) -#define MPP_TARGET_FUZZ ( 1 * 1000 * 1000) +#define PRESPLIT_MAX_HTLC_SHARE 3 static struct presplit_mod_data *presplit_mod_data_init(struct payment *p) { @@ -2117,6 +2413,18 @@ static struct presplit_mod_data *presplit_mod_data_init(struct payment *p) } } +static u32 payment_max_htlcs(const struct payment *p) +{ + struct channel_hint *h; + u32 res = 0; + for (size_t i = 0; i < tal_count(p->channel_hints); i++) { + h = &p->channel_hints[i]; + if (h->local && h->enabled) + res += h->htlc_budget; + } + return res; +} + static bool payment_supports_mpp(struct payment *p) { if (p->invoice == NULL || p->invoice->features == NULL) @@ -2125,15 +2433,28 @@ static bool payment_supports_mpp(struct payment *p) return feature_offered(p->invoice->features, OPT_BASIC_MPP); } +/* Return fuzzed amount ~= target, but never exceeding max */ +static struct amount_msat fuzzed_near(struct amount_msat target, + struct amount_msat max) +{ + s64 fuzz; + struct amount_msat res = target; + + /* Somewhere within 25% of target please. */ + fuzz = pseudorand(target.millisatoshis / 2) /* Raw: fuzz */ + - target.millisatoshis / 4; /* Raw: fuzz */ + res.millisatoshis = target.millisatoshis + fuzz; /* Raw: fuzz < msat */ + + if (amount_msat_greater(res, max)) + res = max; + return res; +} + static void presplit_cb(struct presplit_mod_data *d, struct payment *p) { struct payment *root = payment_root(p); - struct amount_msat amt = root->amount; - if (d->disable) - return payment_continue(p); - - if (!payment_supports_mpp(p)) + if (d->disable || p->parent != NULL || !payment_supports_mpp(p)) return payment_continue(p); if (p->step == PAYMENT_STEP_ONION_PAYLOAD) { @@ -2148,41 +2469,49 @@ static void presplit_cb(struct presplit_mod_data *d, struct payment *p) fields, root->payment_secret, root->amount.millisatoshis); /* Raw: onion payload */ } - } else if (p == root && p->step == PAYMENT_STEP_INITIALIZED) { + } else if (p->step == PAYMENT_STEP_INITIALIZED) { /* The presplitter only acts on the root and only in the first * step. */ size_t count = 0; + u32 htlcs = payment_max_htlcs(p) / PRESPLIT_MAX_HTLC_SHARE; + struct amount_msat target, amt = p->amount; /* We need to opt-in to the MPP sending facility no matter * what we do. That means setting all partids to a non-zero * value. */ root->partid++; + /* Bump the next_partid as well so we don't have duplicate + * partids. Not really necessary since the root payment whose + * id could be reused will never reach the `sendonion` step, + * but makes debugging a bit easier. */ + root->next_partid++; + + if (htlcs == 0) { + p->abort = true; + return payment_fail( + p, "Cannot attempt payment, we have no channel to " + "which we can add an HTLC"); + } else if (p->amount.millisatoshis / MPP_TARGET_SIZE > htlcs) /* Raw: division */ + target.millisatoshis = p->amount.millisatoshis / htlcs; /* Raw: division */ + else + target = AMOUNT_MSAT(MPP_TARGET_SIZE); + /* If we are already below the target size don't split it * either. */ - if (amount_msat_greater(MPP_TARGET_MSAT, p->amount)) + if (amount_msat_greater(target, p->amount)) return payment_continue(p); /* Ok, we know we should split, so split here and then skip this * payment and start the children instead. */ - while (!amount_msat_eq(amt, AMOUNT_MSAT(0))) { - struct payment *c = - payment_new(p, NULL, p, p->modifiers); - - /* Pseudorandom number in the range [-1, 1]. */ - double rand = pseudorand_double() * 2 - 1; double multiplier; - c->amount.millisatoshis = rand * MPP_TARGET_FUZZ + MPP_TARGET_SIZE; /* Raw: Multiplication */ - - /* Clamp the value to the total amount, so the fuzzing - * doesn't go above the total. */ - if (amount_msat_greater(c->amount, amt)) - c->amount = amt; + struct payment *c = + payment_new(p, NULL, p, p->modifiers); - multiplier = - (double)c->amount.millisatoshis / (double)p->amount.millisatoshis; /* Raw: msat division. */ + /* Get ~ target, but don't exceed amt */ + c->amount = fuzzed_near(target, amt); if (!amount_msat_sub(&amt, amt, c->amount)) plugin_err( @@ -2195,22 +2524,22 @@ static void presplit_cb(struct presplit_mod_data *d, struct payment *p) /* Now adjust the constraints so we don't multiply them * when splitting. */ + multiplier = (double)c->amount.millisatoshis / (double)p->amount.millisatoshis; /* Raw: msat division. */ c->constraints.fee_budget.millisatoshis *= multiplier; /* Raw: Multiplication */ payment_start(c); count++; } - p->step = PAYMENT_STEP_SPLIT; - p->end_time = time_now(); + + p->result = NULL; + p->route = NULL; p->why = tal_fmt( p, - "Split into %zu sub-payments due to initial size (%s > " - "%dmsat)", + "Split into %zu sub-payments due to initial size (%s > %s)", count, type_to_string(tmpctx, struct amount_msat, &root->amount), - MPP_TARGET_SIZE); + type_to_string(tmpctx, struct amount_msat, &target)); + payment_set_step(p, PAYMENT_STEP_SPLIT); plugin_log(p->plugin, LOG_INFORM, "%s", p->why); - p->result = NULL; - p->route = NULL; } payment_continue(p); } @@ -2225,32 +2554,56 @@ REGISTER_PAYMENT_MODIFIER(presplit, struct presplit_mod_data *, * +/- 10% randomness, and then starts two attempts, one for either side of * the split. The goal is to find two smaller routes, that still adhere to our * constraints, but that can complete the payment. + * + * This modifier also checks whether we can split and still have enough HTLCs + * available on the channels and aborts if that's no longer the case. */ #define MPP_ADAPTIVE_LOWER_LIMIT AMOUNT_MSAT(100 * 1000) -static struct presplit_mod_data *adaptive_splitter_data_init(struct payment *p) +static struct adaptive_split_mod_data *adaptive_splitter_data_init(struct payment *p) { - struct presplit_mod_data *d; + struct adaptive_split_mod_data *d; if (p->parent == NULL) { - d = tal(p, struct presplit_mod_data); + d = tal(p, struct adaptive_split_mod_data); d->disable = false; + d->htlc_budget = 0; return d; } else { - return payment_mod_presplit_get_data(p->parent); + return payment_mod_adaptive_splitter_get_data(p->parent); } } -static void adaptive_splitter_cb(struct presplit_mod_data *d, struct payment *p) +static void adaptive_splitter_cb(struct adaptive_split_mod_data *d, struct payment *p) { struct payment *root = payment_root(p); - + struct adaptive_split_mod_data *root_data = + payment_mod_adaptive_splitter_get_data(root); if (d->disable) return payment_continue(p); if (!payment_supports_mpp(p) || root->abort) return payment_continue(p); + if (p->parent == NULL && d->htlc_budget == 0) { + /* Now that we potentially had an early splitter run, let's + * update our htlc_budget that we own exclusively from now + * on. We do this by subtracting the number of payment + * attempts an eventual presplitter has already performed. */ + struct payment_tree_result res; + res = payment_collect_result(p); + d->htlc_budget = payment_max_htlcs(p); + if (res.attempts > d->htlc_budget) { + p->abort = true; + return payment_fail( + p, + "Cannot add %d HTLCs to our channels, we " + "only have %d HTLCs available.", + res.attempts, d->htlc_budget); + } + d->htlc_budget -= res.attempts; + } + if (p->step == PAYMENT_STEP_ONION_PAYLOAD) { /* We need to tell the last hop the total we're going to * send. Presplit disables amount fuzzing, so we should always @@ -2270,6 +2623,18 @@ static void adaptive_splitter_cb(struct presplit_mod_data *d, struct payment *p) double rand = pseudorand_double() * 0.2 + 0.9; u64 mid = p->amount.millisatoshis / 2 * rand; /* Raw: multiplication */ bool ok; + /* Use the start constraints, not the ones updated by routes and shadow-routes. */ + struct payment_constraints *pconstraints = p->start_constraints; + + /* First check that splitting doesn't exceed our HTLC budget */ + if (root_data->htlc_budget == 0) { + root->abort = true; + return payment_fail( + p, + "Cannot split payment any further without " + "exceeding the maximum number of HTLCs " + "allowed by our channels"); + } a = payment_new(p, NULL, p, p->modifiers); b = payment_new(p, NULL, p, p->modifiers); @@ -2277,11 +2642,15 @@ static void adaptive_splitter_cb(struct presplit_mod_data *d, struct payment *p) a->amount.millisatoshis = mid; /* Raw: split. */ b->amount.millisatoshis -= mid; /* Raw: split. */ + double multiplier = (double)a->amount.millisatoshis / (double)p->amount.millisatoshis; /* Raw: msat division */ + assert(multiplier >= 0.4 && multiplier < 0.6); + /* Adjust constraints since we don't want to double our * fee allowance when we split. */ - a->constraints.fee_budget.millisatoshis *= (double)a->amount.millisatoshis / (double)p->amount.millisatoshis; /* Raw: msat division. */ + a->constraints.fee_budget.millisatoshis = pconstraints->fee_budget.millisatoshis * multiplier; /* Raw: msat multiplication. */ + ok = amount_msat_sub(&b->constraints.fee_budget, - p->constraints.fee_budget, + pconstraints->fee_budget, a->constraints.fee_budget); /* Should not fail, mid is less than 55% of original @@ -2292,6 +2661,10 @@ static void adaptive_splitter_cb(struct presplit_mod_data *d, struct payment *p) payment_start(a); payment_start(b); p->step = PAYMENT_STEP_SPLIT; + + /* Take note that we now have an additional split that + * may end up using an HTLC. */ + root_data->htlc_budget--; } else { plugin_log(p->plugin, LOG_INFORM, "Lower limit of adaptive splitter reached " @@ -2305,5 +2678,5 @@ static void adaptive_splitter_cb(struct presplit_mod_data *d, struct payment *p) payment_continue(p); } -REGISTER_PAYMENT_MODIFIER(adaptive_splitter, struct presplit_mod_data *, +REGISTER_PAYMENT_MODIFIER(adaptive_splitter, struct adaptive_split_mod_data *, adaptive_splitter_data_init, adaptive_splitter_cb); diff --git a/plugins/libplugin-pay.h b/plugins/libplugin-pay.h index 7c52173d69cb..209c2edce650 100644 --- a/plugins/libplugin-pay.h +++ b/plugins/libplugin-pay.h @@ -69,6 +69,14 @@ struct channel_hint { /* Is the channel enabled? */ bool enabled; + + /* True if we are one endpoint of this channel */ + bool local; + + /* How many more htlcs can we send over this channel? Only set if this + * is a local channel, because those are the channels we have exact + * numbers on, and they are the bottleneck onto the network. */ + u16 htlc_budget; }; /* Each payment goes through a number of steps that are always processed in @@ -320,13 +328,15 @@ struct direct_pay_data { struct short_channel_id_dir *chan; }; -/* Since presplit and adaptive mpp modifiers share the same information we - * just use the same backing struct. Should they deviate we can create an - * adaptive_splitter_mod_data struct and populate that. */ struct presplit_mod_data { bool disable; }; +struct adaptive_split_mod_data { + bool disable; + u32 htlc_budget; +}; + /* List of globally available payment modifiers. */ REGISTER_PAYMENT_MODIFIER_HEADER(retry, struct retry_mod_data); REGISTER_PAYMENT_MODIFIER_HEADER(routehints, struct routehints_data); @@ -335,7 +345,7 @@ REGISTER_PAYMENT_MODIFIER_HEADER(shadowroute, struct shadow_route_data); REGISTER_PAYMENT_MODIFIER_HEADER(directpay, struct direct_pay_data); extern struct payment_modifier waitblockheight_pay_mod; REGISTER_PAYMENT_MODIFIER_HEADER(presplit, struct presplit_mod_data); -REGISTER_PAYMENT_MODIFIER_HEADER(adaptive_splitter, struct presplit_mod_data); +REGISTER_PAYMENT_MODIFIER_HEADER(adaptive_splitter, struct adaptive_split_mod_data); /* For the root payment we can seed the channel_hints with the result from * `listpeers`, hence avoid channels that we know have insufficient capacity diff --git a/plugins/libplugin.c b/plugins/libplugin.c index c5c5500a66dd..e9c9cd12448a 100644 --- a/plugins/libplugin.c +++ b/plugins/libplugin.c @@ -83,6 +83,10 @@ struct plugin { /* Feature set for lightningd */ struct feature_set *our_features; + + /* Location of the RPC filename in case we need to defer RPC + * initialization or need to recover from a disconnect. */ + const char *rpc_location; }; /* command_result is mainly used as a compile-time check to encourage you @@ -763,7 +767,7 @@ static struct command_result *handle_init(struct command *cmd, char *dir, *network; struct json_out *param_obj; struct plugin *p = cmd->plugin; - bool with_rpc = true; + bool with_rpc = p->rpc_conn != NULL; configtok = json_delve(buf, params, ".configuration"); @@ -780,27 +784,39 @@ static struct command_result *handle_init(struct command *cmd, fsettok = json_delve(buf, configtok, ".feature_set"); p->our_features = json_to_feature_set(p, buf, fsettok); + /* Only attempt to connect if the plugin has configured the rpc_conn + * already, if that's not the case we were told to run without an RPC + * connection, so don't even log an error. */ rpctok = json_delve(buf, configtok, ".rpc-file"); - p->rpc_conn->fd = socket(AF_UNIX, SOCK_STREAM, 0); - if (rpctok->end - rpctok->start + 1 > sizeof(addr.sun_path)) - plugin_err(p, "rpc filename '%.*s' too long", - rpctok->end - rpctok->start, - buf + rpctok->start); - memcpy(addr.sun_path, buf + rpctok->start, rpctok->end - rpctok->start); - addr.sun_path[rpctok->end - rpctok->start] = '\0'; - addr.sun_family = AF_UNIX; - - if (connect(p->rpc_conn->fd, (struct sockaddr *)&addr, sizeof(addr)) != 0) { - with_rpc = false; - plugin_log(p, LOG_UNUSUAL, "Could not connect to '%.*s': %s", - rpctok->end - rpctok->start, buf + rpctok->start, - strerror(errno)); - } else { + p->rpc_location = json_strdup(p, buf, rpctok); + /* FIXME: Move this to its own function so we can initialize at a + * later point in time. */ + if (p->rpc_conn != NULL) { + p->rpc_conn->fd = socket(AF_UNIX, SOCK_STREAM, 0); + if (strlen(p->rpc_location) + 1 > sizeof(addr.sun_path)) + plugin_err(p, "rpc filename '%s' too long", + p->rpc_location); + memcpy(addr.sun_path, buf + rpctok->start, + rpctok->end - rpctok->start); + addr.sun_path[rpctok->end - rpctok->start] = '\0'; + addr.sun_family = AF_UNIX; + + if (connect(p->rpc_conn->fd, (struct sockaddr *)&addr, + sizeof(addr)) != 0) { + with_rpc = false; + plugin_log(p, LOG_UNUSUAL, + "Could not connect to '%s': %s", + p->rpc_location, strerror(errno)); + } + + membuf_init(&p->rpc_conn->mb, tal_arr(p, char, READ_CHUNKSIZE), + READ_CHUNKSIZE, membuf_tal_realloc); + param_obj = json_out_obj(NULL, "config", "allow-deprecated-apis"); - deprecated_apis = streq(rpc_delve(tmpctx, p, "listconfigs", - take(param_obj), - ".allow-deprecated-apis"), - "true"); + deprecated_apis = + streq(rpc_delve(tmpctx, p, "listconfigs", take(param_obj), + ".allow-deprecated-apis"), + "true"); } opttok = json_get_member(buf, params, "options"); @@ -1182,6 +1198,7 @@ static struct plugin *new_plugin(const tal_t *ctx, void (*init)(struct plugin *p, const char *buf, const jsmntok_t *), const enum plugin_restartability restartability, + bool init_rpc, struct feature_set *features, const struct plugin_command *commands, size_t num_commands, @@ -1207,11 +1224,12 @@ static struct plugin *new_plugin(const tal_t *ctx, uintmap_init(&p->out_reqs); p->our_features = features; - /* Sync RPC FIXME: maybe go full async ? */ - p->rpc_conn = tal(p, struct rpc_conn); - membuf_init(&p->rpc_conn->mb, - tal_arr(p, char, READ_CHUNKSIZE), READ_CHUNKSIZE, - membuf_tal_realloc); + if (init_rpc) { + /* Sync RPC FIXME: maybe go full async ? */ + p->rpc_conn = tal(p, struct rpc_conn); + } else { + p->rpc_conn = NULL; + } p->init = init; p->manifested = p->initialized = false; @@ -1244,6 +1262,7 @@ void plugin_main(char *argv[], void (*init)(struct plugin *p, const char *buf, const jsmntok_t *), const enum plugin_restartability restartability, + bool init_rpc, struct feature_set *features, const struct plugin_command *commands, size_t num_commands, @@ -1264,7 +1283,7 @@ void plugin_main(char *argv[], daemon_setup(argv[0], NULL, NULL); va_start(ap, num_hook_subs); - plugin = new_plugin(NULL, init, restartability, features, commands, + plugin = new_plugin(NULL, init, restartability, init_rpc, features, commands, num_commands, notif_subs, num_notif_subs, hook_subs, num_hook_subs, ap); va_end(ap); diff --git a/plugins/libplugin.h b/plugins/libplugin.h index bc1e2ac21b4b..87ec13e1c970 100644 --- a/plugins/libplugin.h +++ b/plugins/libplugin.h @@ -248,6 +248,7 @@ void NORETURN LAST_ARG_NULL plugin_main(char *argv[], void (*init)(struct plugin *p, const char *buf, const jsmntok_t *), const enum plugin_restartability restartability, + bool init_rpc, struct feature_set *features, const struct plugin_command *commands, size_t num_commands, diff --git a/plugins/pay.c b/plugins/pay.c index 423816c4c214..77e81703778d 100644 --- a/plugins/pay.c +++ b/plugins/pay.c @@ -2007,7 +2007,7 @@ static const struct plugin_command commands[] = { int main(int argc, char *argv[]) { setup_locale(); - plugin_main(argv, init, PLUGIN_RESTARTABLE, NULL, commands, + plugin_main(argv, init, PLUGIN_RESTARTABLE, true, NULL, commands, ARRAY_SIZE(commands), NULL, 0, NULL, 0, plugin_option("disable-mpp", "flag", "Disable multi-part payments.", diff --git a/tests/plugins/test_libplugin.c b/tests/plugins/test_libplugin.c index bff60e89b6ef..66dcb7c182ec 100644 --- a/tests/plugins/test_libplugin.c +++ b/tests/plugins/test_libplugin.c @@ -124,7 +124,8 @@ static const struct plugin_notification notifs[] = { { int main(int argc, char *argv[]) { setup_locale(); - plugin_main(argv, init, PLUGIN_RESTARTABLE, NULL, commands, ARRAY_SIZE(commands), + plugin_main(argv, init, PLUGIN_RESTARTABLE, true, NULL, + commands, ARRAY_SIZE(commands), notifs, ARRAY_SIZE(notifs), hooks, ARRAY_SIZE(hooks), plugin_option("name", "string", diff --git a/tests/test_misc.py b/tests/test_misc.py index e9ac8a9455c3..d235837cb235 100644 --- a/tests/test_misc.py +++ b/tests/test_misc.py @@ -1368,8 +1368,11 @@ def test_reserve_enforcement(node_factory, executor): def test_htlc_send_timeout(node_factory, bitcoind, compat): """Test that we don't commit an HTLC to an unreachable node.""" # Feerates identical so we don't get gratuitous commit to update them - l1 = node_factory.get_node(options={'log-level': 'io'}, - feerates=(7500, 7500, 7500, 7500)) + l1 = node_factory.get_node( + options={'log-level': 'io'}, + feerates=(7500, 7500, 7500, 7500) + ) + # Blackhole it after it sends HTLC_ADD to l3. l2 = node_factory.get_node(disconnect=['0WIRE_UPDATE_ADD_HTLC'], options={'log-level': 'io'}, @@ -1395,7 +1398,7 @@ def test_htlc_send_timeout(node_factory, bitcoind, compat): timedout = True inv = l3.rpc.invoice(123000, 'test_htlc_send_timeout', 'description') - with pytest.raises(RpcError, match=r'Ran out of routes to try after 1 attempt') as excinfo: + with pytest.raises(RpcError, match=r'Ran out of routes to try after [0-9]+ attempt[s]?') as excinfo: l1.rpc.pay(inv['bolt11']) err = excinfo.value diff --git a/tests/test_pay.py b/tests/test_pay.py index 48c38dab6c64..e4b7c2df6751 100644 --- a/tests/test_pay.py +++ b/tests/test_pay.py @@ -1672,8 +1672,9 @@ def listpays_nofail(b11): # It will try l1->l2->l5, which fails. # It will try l1->l2->l3->l5, which fails. # It will try l1->l2->l3->l4->l5, which fails. + # Finally, fails to find a route. inv = l5.rpc.invoice(10**8, 'test_retry2', 'test_retry2')['bolt11'] - with pytest.raises(RpcError, match=r'3 attempts'): + with pytest.raises(RpcError, match=r'4 attempts'): l1.rpc.dev_pay(inv, use_shadow=False) @@ -3102,10 +3103,10 @@ def test_mpp_adaptive(node_factory, bitcoind): ```dot digraph { - l1 -> l2; - l2 -> l4; - l1 -> l3; - l3 -> l4; + l1 -> l2 [label="scid=103x1x1, cap=amt-1"]; + l2 -> l4 [label="scid=105x1x1, cap=max"]; + l1 -> l3 [label="scid=107x1x1, cap=max"]; + l3 -> l4 [label="scid=109x1x1, cap=amt-1"]; } """ amt = 10**7 - 1 @@ -3154,3 +3155,40 @@ def test_mpp_adaptive(node_factory, bitcoind): from pprint import pprint pprint(p) pprint(l1.rpc.paystatus(inv)) + + +def test_pay_fail_unconfirmed_channel(node_factory, bitcoind): + ''' + Replicate #3855. + `pay` crash when any direct channel is still + unconfirmed. + ''' + l1, l2 = node_factory.get_nodes(2) + + amount_sat = 10 ** 6 + + # create l2->l1 channel. + l2.fundwallet(amount_sat * 5) + l1.rpc.connect(l2.info['id'], 'localhost', l2.port) + l2.rpc.fundchannel(l1.info['id'], amount_sat * 3) + # channel is still unconfirmed. + + # Attempt to pay from l1 to l2. + # This should fail since the channel capacities are wrong. + invl2 = l2.rpc.invoice(Millisatoshi(amount_sat * 1000), 'i', 'i')['bolt11'] + with pytest.raises(RpcError): + l1.rpc.pay(invl2) + + # Let the channel confirm. + bitcoind.generate_block(6) + sync_blockheight(bitcoind, [l1, l2]) + + # Now give enough capacity so l1 can pay. + invl1 = l1.rpc.invoice(Millisatoshi(amount_sat * 2 * 1000), 'j', 'j')['bolt11'] + l2.rpc.pay(invl1) + + # Wait for us to recognize that the channel is available + wait_for(lambda: l1.rpc.listpeers()['peers'][0]['channels'][0]['spendable_msat'].millisatoshis > amount_sat * 1000) + + # Now l1 can pay to l2. + l1.rpc.pay(invl2)