From 178892bd8c94cec9e707d42c651a81a4e51520c2 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Thu, 12 Dec 2019 09:46:23 +1030 Subject: [PATCH 01/22] db: add partid, total_msat fields to payment entries. This is in preparation for partial payments. For existing payments, partid is 0 (arbitrarity) and total_msat is msatoshi. Signed-off-by: Rusty Russell --- lightningd/pay.c | 57 ++++++++++++++++++---------- lightningd/pay.h | 3 +- lightningd/peer_htlcs.c | 3 +- wallet/db.c | 78 +++++++++++++++++++++++++++++++++++++++ wallet/test/run-wallet.c | 18 ++++++--- wallet/wallet.c | 80 +++++++++++++++++++++++++++++----------- wallet/wallet.h | 26 +++++++++---- 7 files changed, 211 insertions(+), 54 deletions(-) diff --git a/lightningd/pay.c b/lightningd/pay.c index 525ec955bd5c..33608db98ab6 100644 --- a/lightningd/pay.c +++ b/lightningd/pay.c @@ -81,6 +81,8 @@ void json_add_payment_fields(struct json_stream *response, { json_add_u64(response, "id", t->id); json_add_sha256(response, "payment_hash", &t->payment_hash); + if (t->partid) + json_add_u64(response, "partid", t->partid); if (t->destination != NULL) json_add_node_id(response, "destination", t->destination); @@ -278,9 +280,11 @@ void payment_succeeded(struct lightningd *ld, struct htlc_out *hout, struct wallet_payment *payment; wallet_payment_set_status(ld->wallet, &hout->payment_hash, + /* FIXME: Set partid! */ 0, PAYMENT_COMPLETE, rval); payment = wallet_payment_by_hash(tmpctx, ld->wallet, - &hout->payment_hash); + &hout->payment_hash, + /* FIXME: Set partid! */ 0); assert(payment); tell_waiters_success(ld, &hout->payment_hash, payment); @@ -467,14 +471,16 @@ remote_routing_failure(const tal_t *ctx, return routing_failure; } -void payment_store(struct lightningd *ld, const struct sha256 *payment_hash) +void payment_store(struct lightningd *ld, + const struct sha256 *payment_hash, u64 partid) { struct sendpay_command *pc; struct sendpay_command *next; const struct wallet_payment *payment; - wallet_payment_store(ld->wallet, payment_hash); - payment = wallet_payment_by_hash(tmpctx, ld->wallet, payment_hash); + wallet_payment_store(ld->wallet, payment_hash, partid); + payment = wallet_payment_by_hash(tmpctx, ld->wallet, + payment_hash, partid); assert(payment); /* Trigger any sendpay commands waiting for the store to occur. */ @@ -496,7 +502,8 @@ void payment_failed(struct lightningd *ld, const struct htlc_out *hout, int pay_errcode; payment = wallet_payment_by_hash(tmpctx, ld->wallet, - &hout->payment_hash); + &hout->payment_hash, + /* FIXME: Set partid! */0); #ifdef COMPAT_V052 /* Prior to "pay: delete HTLC when we delete payment." we would @@ -566,11 +573,11 @@ void payment_failed(struct lightningd *ld, const struct htlc_out *hout, } /* Save to DB */ - payment_store(ld, &hout->payment_hash); - wallet_payment_set_status(ld->wallet, &hout->payment_hash, + payment_store(ld, &hout->payment_hash, /* FIXME: Set partid! */ 0); + wallet_payment_set_status(ld->wallet, &hout->payment_hash, /* FIXME: Set partid! */ 0, PAYMENT_FAILED, NULL); wallet_payment_set_failinfo(ld->wallet, - &hout->payment_hash, + &hout->payment_hash, /* FIXME: Set partid! */ 0, fail ? NULL : hout->failuremsg, pay_errcode == PAY_DESTINATION_PERM_FAIL, fail ? fail->erring_index : -1, @@ -590,7 +597,8 @@ void payment_failed(struct lightningd *ld, const struct htlc_out *hout, * Return callback if we called already, otherwise NULL. */ static struct command_result *wait_payment(struct lightningd *ld, struct command *cmd, - const struct sha256 *payment_hash) + const struct sha256 *payment_hash, + u64 partid) { struct wallet_payment *payment; u8 *failonionreply; @@ -604,7 +612,8 @@ static struct command_result *wait_payment(struct lightningd *ld, struct routing_failure *fail; int faildirection; - payment = wallet_payment_by_hash(tmpctx, ld->wallet, payment_hash); + payment = wallet_payment_by_hash(tmpctx, ld->wallet, + payment_hash, partid); if (!payment) { return command_fail(cmd, PAY_NO_SUCH_PAYMENT, "Never attempted payment for '%s'", @@ -622,7 +631,9 @@ static struct command_result *wait_payment(struct lightningd *ld, case PAYMENT_FAILED: /* Get error from DB */ - wallet_payment_get_failinfo(tmpctx, ld->wallet, payment_hash, + wallet_payment_get_failinfo(tmpctx, ld->wallet, + payment_hash, + partid, &failonionreply, &faildestperm, &failindex, @@ -706,8 +717,10 @@ static struct command_result * send_payment(struct lightningd *ld, struct command *cmd, const struct sha256 *rhash, + u64 partid, const struct route_hop *route, struct amount_msat msat, + struct amount_msat total_msat, const char *label TAKES, const char *b11str TAKES, const struct secret *payment_secret) @@ -779,7 +792,7 @@ send_payment(struct lightningd *ld, sphinx_add_hop(path, &pubkey, onion); /* Now, do we already have a payment? */ - payment = wallet_payment_by_hash(tmpctx, ld->wallet, rhash); + payment = wallet_payment_by_hash(tmpctx, ld->wallet, rhash, partid); if (payment) { /* FIXME: We should really do something smarter here! */ if (payment->status == PAYMENT_PENDING) { @@ -853,18 +866,21 @@ send_payment(struct lightningd *ld, * onchain_failed_our_htlc->payment_failed with no payment. */ if (payment) { - wallet_payment_delete(ld->wallet, rhash); - wallet_local_htlc_out_delete(ld->wallet, channel, rhash); + wallet_payment_delete(ld->wallet, rhash, payment->partid); + wallet_local_htlc_out_delete(ld->wallet, channel, rhash, + payment->partid); } /* If hout fails, payment should be freed too. */ payment = tal(hout, struct wallet_payment); payment->id = 0; payment->payment_hash = *rhash; + payment->partid = partid; payment->destination = tal_dup(payment, struct node_id, &ids[n_hops - 1]); payment->status = PAYMENT_PENDING; payment->msatoshi = msat; payment->msatoshi_sent = route[0].amount; + payment->total_msat = total_msat; payment->timestamp = time_now().ts.tv_sec; payment->payment_preimage = NULL; payment->path_secrets = tal_steal(payment, path_secrets); @@ -998,7 +1014,7 @@ static struct command_result *json_sendonion(struct command *cmd, failcode); /* Now, do we already have a payment? */ - payment = wallet_payment_by_hash(tmpctx, ld->wallet, payment_hash); + payment = wallet_payment_by_hash(tmpctx, ld->wallet, payment_hash, /* FIXME: Set partid! */0); if (payment) { if (payment->status == PAYMENT_PENDING) { log_debug(ld->log, "send_payment: previous still in progress"); @@ -1028,8 +1044,8 @@ static struct command_result *json_sendonion(struct command *cmd, /* Cleanup any prior payment. We're about to retry. */ if (payment) { - wallet_payment_delete(ld->wallet, payment_hash); - wallet_local_htlc_out_delete(ld->wallet, channel, payment_hash); + wallet_payment_delete(ld->wallet, payment_hash, /* FIXME: Set partid! */0); + wallet_local_htlc_out_delete(ld->wallet, channel, payment_hash, /* FIXME: Set partid! */0); } failcode = send_onion(cmd->ld, &packet, first_hop, payment_hash, channel, @@ -1247,7 +1263,10 @@ static struct command_result *json_sendpay(struct command *cmd, } #endif - res = send_payment(cmd->ld, cmd, rhash, route, + res = send_payment(cmd->ld, cmd, rhash, /* FIXME: Set partid! */ 0, + route, + msat ? *msat : route[routetok->size-1].amount, + /* FIXME: Set total_msat! */ msat ? *msat : route[routetok->size-1].amount, label, b11str, payment_secret); if (res) @@ -1284,7 +1303,7 @@ static struct command_result *json_waitsendpay(struct command *cmd, NULL)) return command_param_failed(); - res = wait_payment(cmd->ld, cmd, rhash); + res = wait_payment(cmd->ld, cmd, rhash, /* FIXME: Set partid! */0); if (res) return res; diff --git a/lightningd/pay.h b/lightningd/pay.h index 53f0e0cdd77a..acfb394a4ee1 100644 --- a/lightningd/pay.h +++ b/lightningd/pay.h @@ -18,7 +18,8 @@ void payment_failed(struct lightningd *ld, const struct htlc_out *hout, const char *localfail); /* Inform payment system to save the payment. */ -void payment_store(struct lightningd *ld, const struct sha256 *payment_hash); +void payment_store(struct lightningd *ld, + const struct sha256 *payment_hash, u64 partid); /* This json will be also used in 'sendpay_success' notifictaion. */ void json_add_payment_fields(struct json_stream *response, diff --git a/lightningd/peer_htlcs.c b/lightningd/peer_htlcs.c index c71e23fc929a..39df3f2a9dea 100644 --- a/lightningd/peer_htlcs.c +++ b/lightningd/peer_htlcs.c @@ -1274,7 +1274,8 @@ static bool update_out_htlc(struct channel *channel, /* For our own HTLCs, we commit payment to db lazily */ if (hout->origin_htlc_id == 0) payment_store(ld, - &hout->payment_hash); + &hout->payment_hash, + /* FIXME: Set partid! */ 0); } if (!htlc_out_update_state(channel, hout, newstate)) diff --git a/wallet/db.c b/wallet/db.c index 0a3d4559c2ca..6497ee29372c 100644 --- a/wallet/db.c +++ b/wallet/db.c @@ -481,6 +481,84 @@ static struct migration dbmigrations[] = { {SQL("UPDATE forwarded_payments SET received_time=0 WHERE received_time IS NULL;"), NULL}, {SQL("ALTER TABLE invoices ADD COLUMN features BLOB DEFAULT '';"), NULL}, + /* We can now have multiple payments in progress for a single hash, so + * add two fields; combination of payment_hash & partid is unique. */ + {SQL("ALTER TABLE payments RENAME TO temp_payments;"), NULL}, + {SQL("CREATE TABLE payments (" + " id BIGSERIAL" + ", timestamp INTEGER" + ", status INTEGER" + ", payment_hash BLOB" + ", destination BLOB" + ", msatoshi BIGINT" + ", payment_preimage BLOB" + ", path_secrets BLOB" + ", route_nodes BLOB" + ", route_channels BLOB" + ", failonionreply BLOB" + ", faildestperm INTEGER" + ", failindex INTEGER" + ", failcode INTEGER" + ", failnode BLOB" + ", failchannel TEXT" + ", failupdate BLOB" + ", msatoshi_sent BIGINT" + ", faildetail TEXT" + ", description TEXT" + ", faildirection INTEGER" + ", bolt11 TEXT" + ", total_msat BIGINT" + ", partid BIGINT" + ", PRIMARY KEY (id)" + ", UNIQUE (payment_hash, partid))"), NULL}, + {SQL("INSERT INTO payments (" + "id" + ", timestamp" + ", status" + ", payment_hash" + ", destination" + ", msatoshi" + ", payment_preimage" + ", path_secrets" + ", route_nodes" + ", route_channels" + ", failonionreply" + ", faildestperm" + ", failindex" + ", failcode" + ", failnode" + ", failchannel" + ", failupdate" + ", msatoshi_sent" + ", faildetail" + ", description" + ", faildirection" + ", bolt11)" + "SELECT id" + ", timestamp" + ", status" + ", payment_hash" + ", destination" + ", msatoshi" + ", payment_preimage" + ", path_secrets" + ", route_nodes" + ", route_channels" + ", failonionreply" + ", faildestperm" + ", failindex" + ", failcode" + ", failnode" + ", failchannel" + ", failupdate" + ", msatoshi_sent" + ", faildetail" + ", description" + ", faildirection" + ", bolt11 FROM temp_payments;"), NULL}, + {SQL("UPDATE payments SET total_msat = msatoshi;"), NULL}, + {SQL("UPDATE payments SET partid = 0;"), NULL}, + {SQL("DROP TABLE temp_payments;"), NULL}, }; /* Leak tracking. */ diff --git a/wallet/test/run-wallet.c b/wallet/test/run-wallet.c index 56c359e8a807..4fc199d9a433 100644 --- a/wallet/test/run-wallet.c +++ b/wallet/test/run-wallet.c @@ -486,7 +486,8 @@ void payment_failed(struct lightningd *ld UNNEEDED, const struct htlc_out *hout const char *localfail UNNEEDED) { fprintf(stderr, "payment_failed called!\n"); abort(); } /* Generated stub for payment_store */ -void payment_store(struct lightningd *ld UNNEEDED, const struct sha256 *payment_hash UNNEEDED) +void payment_store(struct lightningd *ld UNNEEDED, + const struct sha256 *payment_hash UNNEEDED, u64 partid UNNEEDED) { fprintf(stderr, "payment_store called!\n"); abort(); } /* Generated stub for payment_succeeded */ void payment_succeeded(struct lightningd *ld UNNEEDED, struct htlc_out *hout UNNEEDED, @@ -1254,29 +1255,36 @@ static bool test_payment_crud(struct lightningd *ld, const tal_t *ctx) t->id = 0; t->msatoshi = AMOUNT_MSAT(100); t->msatoshi_sent = AMOUNT_MSAT(101); + t->total_msat = t->msatoshi; t->status = PAYMENT_PENDING; t->payment_preimage = NULL; memset(&t->payment_hash, 1, sizeof(t->payment_hash)); + t->partid = 0; db_begin_transaction(w->db); wallet_payment_setup(w, tal_dup(NULL, struct wallet_payment, t)); - wallet_payment_store(w, &t->payment_hash); - t2 = wallet_payment_by_hash(ctx, w, &t->payment_hash); + wallet_payment_store(w, &t->payment_hash, 0); + t2 = wallet_payment_by_hash(ctx, w, &t->payment_hash, 0); CHECK(t2 != NULL); CHECK(t2->status == t->status); + CHECK(sha256_eq(&t2->payment_hash, &t->payment_hash)); + CHECK(t2->partid == t->partid); CHECK(node_id_cmp(t2->destination, t->destination) == 0); CHECK(amount_msat_eq(t2->msatoshi, t->msatoshi)); CHECK(amount_msat_eq(t2->msatoshi_sent, t->msatoshi_sent)); + CHECK(amount_msat_eq(t2->total_msat, t->total_msat)); CHECK(!t2->payment_preimage); t->status = PAYMENT_COMPLETE; t->payment_preimage = tal(w, struct preimage); memset(t->payment_preimage, 2, sizeof(*t->payment_preimage)); - wallet_payment_set_status(w, &t->payment_hash, t->status, + wallet_payment_set_status(w, &t->payment_hash, t->partid, t->status, t->payment_preimage); - t2 = wallet_payment_by_hash(ctx, w, &t->payment_hash); + t2 = wallet_payment_by_hash(ctx, w, &t->payment_hash, t->partid); CHECK(t2 != NULL); CHECK(t2->status == t->status); + CHECK(sha256_eq(&t2->payment_hash, &t->payment_hash)); + CHECK(t2->partid == t->partid); CHECK(node_id_eq(t2->destination, t->destination)); CHECK(amount_msat_eq(t2->msatoshi, t->msatoshi)); CHECK(amount_msat_eq(t2->msatoshi_sent, t->msatoshi_sent)); diff --git a/wallet/wallet.c b/wallet/wallet.c index 27a31fe180d8..e452a42340e8 100644 --- a/wallet/wallet.c +++ b/wallet/wallet.c @@ -2042,10 +2042,12 @@ struct htlc_stub *wallet_htlc_stubs(const tal_t *ctx, struct wallet *wallet, void wallet_local_htlc_out_delete(struct wallet *wallet, struct channel *chan, - const struct sha256 *payment_hash) + const struct sha256 *payment_hash, + u64 partid) { struct db_stmt *stmt; + /* FIXME: Put partid into locally-generated htlc_out, select here! */ stmt = db_prepare_v2(wallet->db, SQL("DELETE FROM channel_htlcs" " WHERE direction = ?" " AND origin_htlc = ?" @@ -2058,12 +2060,15 @@ void wallet_local_htlc_out_delete(struct wallet *wallet, } static struct wallet_payment * -find_unstored_payment(struct wallet *wallet, const struct sha256 *payment_hash) +find_unstored_payment(struct wallet *wallet, + const struct sha256 *payment_hash, + u64 partid) { struct wallet_payment *i; list_for_each(&wallet->unstored_payments, i, list) { - if (sha256_eq(payment_hash, &i->payment_hash)) + if (sha256_eq(payment_hash, &i->payment_hash) + && i->partid == partid) return i; } return NULL; @@ -2076,19 +2081,21 @@ static void destroy_unstored_payment(struct wallet_payment *payment) void wallet_payment_setup(struct wallet *wallet, struct wallet_payment *payment) { - assert(!find_unstored_payment(wallet, &payment->payment_hash)); + assert(!find_unstored_payment(wallet, &payment->payment_hash, + payment->partid)); list_add_tail(&wallet->unstored_payments, &payment->list); tal_add_destructor(payment, destroy_unstored_payment); } void wallet_payment_store(struct wallet *wallet, - const struct sha256 *payment_hash) + const struct sha256 *payment_hash, + u64 partid) { struct db_stmt *stmt; struct wallet_payment *payment; - payment = find_unstored_payment(wallet, payment_hash); + payment = find_unstored_payment(wallet, payment_hash, partid); if (!payment) { /* Already stored on-disk */ #if DEVELOPER @@ -2098,8 +2105,10 @@ void wallet_payment_store(struct wallet *wallet, bool res; stmt = db_prepare_v2(wallet->db, SQL("SELECT status FROM payments" - " WHERE payment_hash=?;")); + " WHERE payment_hash=?" + " AND partid = ?;")); db_bind_sha256(stmt, 0, payment_hash); + db_bind_u64(stmt, 1, partid); db_query_prepared(stmt); res = db_step(stmt); assert(res); @@ -2124,8 +2133,10 @@ void wallet_payment_store(struct wallet *wallet, " route_channels," " msatoshi_sent," " description," - " bolt11" - ") VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?);")); + " bolt11," + " total_msat," + " partid" + ") VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?);")); db_bind_int(stmt, 0, payment->status); db_bind_sha256(stmt, 1, &payment->payment_hash); @@ -2164,26 +2175,32 @@ void wallet_payment_store(struct wallet *wallet, else db_bind_null(stmt, 10); + db_bind_amount_msat(stmt, 11, &payment->total_msat); + db_bind_u64(stmt, 12, payment->partid); + db_exec_prepared_v2(take(stmt)); tal_free(payment); } void wallet_payment_delete(struct wallet *wallet, - const struct sha256 *payment_hash) + const struct sha256 *payment_hash, + u64 partid) { struct db_stmt *stmt; struct wallet_payment *payment; - payment = find_unstored_payment(wallet, payment_hash); + payment = find_unstored_payment(wallet, payment_hash, partid); if (payment) { tal_free(payment); return; } stmt = db_prepare_v2( - wallet->db, SQL("DELETE FROM payments WHERE payment_hash = ?")); + wallet->db, SQL("DELETE FROM payments WHERE payment_hash = ?" + " AND partid = ?")); db_bind_sha256(stmt, 0, payment_hash); + db_bind_u64(stmt, 1, partid); db_exec_prepared_v2(take(stmt)); } @@ -2251,18 +2268,21 @@ static struct wallet_payment *wallet_stmt2payment(const tal_t *ctx, else payment->failonion = NULL; + db_column_amount_msat(stmt, 14, &payment->total_msat); + payment->partid = db_column_u64(stmt, 15); return payment; } struct wallet_payment * wallet_payment_by_hash(const tal_t *ctx, struct wallet *wallet, - const struct sha256 *payment_hash) + const struct sha256 *payment_hash, + u64 partid) { struct db_stmt *stmt; struct wallet_payment *payment; /* Present the illusion that it's in the db... */ - payment = find_unstored_payment(wallet, payment_hash); + payment = find_unstored_payment(wallet, payment_hash, partid); if (payment) return payment; @@ -2281,10 +2301,14 @@ wallet_payment_by_hash(const tal_t *ctx, struct wallet *wallet, ", description" ", bolt11" ", failonionreply" + ", total_msat" + ", partid" " FROM payments" - " WHERE payment_hash = ?")); + " WHERE payment_hash = ?" + " AND partid = ?")); db_bind_sha256(stmt, 0, payment_hash); + db_bind_u64(stmt, 1, partid); db_query_prepared(stmt); if (db_step(stmt)) { payment = wallet_stmt2payment(ctx, stmt); @@ -2295,6 +2319,7 @@ wallet_payment_by_hash(const tal_t *ctx, struct wallet *wallet, void wallet_payment_set_status(struct wallet *wallet, const struct sha256 *payment_hash, + u64 partid, const enum wallet_payment_status newstatus, const struct preimage *preimage) { @@ -2302,7 +2327,7 @@ void wallet_payment_set_status(struct wallet *wallet, struct wallet_payment *payment; /* We can only fail an unstored payment! */ - payment = find_unstored_payment(wallet, payment_hash); + payment = find_unstored_payment(wallet, payment_hash, partid); if (payment) { assert(newstatus == PAYMENT_FAILED); tal_free(payment); @@ -2311,19 +2336,21 @@ void wallet_payment_set_status(struct wallet *wallet, stmt = db_prepare_v2(wallet->db, SQL("UPDATE payments SET status=? " - "WHERE payment_hash=?")); + "WHERE payment_hash=? AND partid=?")); db_bind_int(stmt, 0, wallet_payment_status_in_db(newstatus)); db_bind_sha256(stmt, 1, payment_hash); + db_bind_u64(stmt, 2, partid); db_exec_prepared_v2(take(stmt)); if (preimage) { stmt = db_prepare_v2(wallet->db, SQL("UPDATE payments SET payment_preimage=? " - "WHERE payment_hash=?")); + "WHERE payment_hash=? AND partid=?")); db_bind_preimage(stmt, 0, preimage); db_bind_sha256(stmt, 1, payment_hash); + db_bind_u64(stmt, 2, partid); db_exec_prepared_v2(take(stmt)); } if (newstatus != PAYMENT_PENDING) { @@ -2332,8 +2359,10 @@ void wallet_payment_set_status(struct wallet *wallet, " SET path_secrets = NULL" " , route_nodes = NULL" " , route_channels = NULL" - " WHERE payment_hash = ?;")); + " WHERE payment_hash = ?" + " AND partid = ?;")); db_bind_sha256(stmt, 0, payment_hash); + db_bind_u64(stmt, 1, partid); db_exec_prepared_v2(take(stmt)); } } @@ -2341,6 +2370,7 @@ void wallet_payment_set_status(struct wallet *wallet, void wallet_payment_get_failinfo(const tal_t *ctx, struct wallet *wallet, const struct sha256 *payment_hash, + u64 partid, /* outputs */ u8 **failonionreply, bool *faildestperm, @@ -2362,8 +2392,9 @@ void wallet_payment_get_failinfo(const tal_t *ctx, ", failnode, failchannel" ", failupdate, faildetail, faildirection" " FROM payments" - " WHERE payment_hash=?;")); + " WHERE payment_hash=? AND partid=?;")); db_bind_sha256(stmt, 0, payment_hash); + db_bind_u64(stmt, 1, partid); db_query_prepared(stmt); resb = db_step(stmt); assert(resb); @@ -2412,6 +2443,7 @@ void wallet_payment_get_failinfo(const tal_t *ctx, void wallet_payment_set_failinfo(struct wallet *wallet, const struct sha256 *payment_hash, + u64 partid, const u8 *failonionreply /*tal_arr*/, bool faildestperm, int failindex, @@ -2434,7 +2466,8 @@ void wallet_payment_set_failinfo(struct wallet *wallet, " , failupdate=?" " , faildetail=?" " , faildirection=?" - " WHERE payment_hash=?;")); + " WHERE payment_hash=?" + " AND partid=?;")); if (failonionreply) db_bind_blob(stmt, 0, failonionreply, tal_count(failonionreply)); @@ -2469,6 +2502,7 @@ void wallet_payment_set_failinfo(struct wallet *wallet, db_bind_null(stmt, 7); db_bind_sha256(stmt, 9, payment_hash); + db_bind_u64(stmt, 10, partid); db_exec_prepared_v2(take(stmt)); } @@ -2501,6 +2535,8 @@ wallet_payment_list(const tal_t *ctx, ", description" ", bolt11" ", failonionreply" + ", total_msat" + ", partid" " FROM payments" " WHERE payment_hash = ?;")); db_bind_sha256(stmt, 0, payment_hash); @@ -2520,6 +2556,8 @@ wallet_payment_list(const tal_t *ctx, ", description" ", bolt11" ", failonionreply" + ", total_msat" + ", partid" " FROM payments;")); } db_query_prepared(stmt); diff --git a/wallet/wallet.h b/wallet/wallet.h index 66a1f0e8fcde..927007748a12 100644 --- a/wallet/wallet.h +++ b/wallet/wallet.h @@ -249,13 +249,18 @@ struct wallet_payment { struct list_node list; u64 id; u32 timestamp; + + /* The combination of these two fields is unique: */ struct sha256 payment_hash; + u64 partid; + enum wallet_payment_status status; /* The destination may not be known if we used `sendonion` */ struct node_id *destination; struct amount_msat msatoshi; struct amount_msat msatoshi_sent; + struct amount_msat total_msat; /* If and only if PAYMENT_COMPLETE */ struct preimage *payment_preimage; /* Needed for recovering from routing failures. */ @@ -930,7 +935,8 @@ void wallet_payment_setup(struct wallet *wallet, struct wallet_payment *payment) * Stores the payment in the database. */ void wallet_payment_store(struct wallet *wallet, - const struct sha256 *payment_hash); + const struct sha256 *payment_hash, + u64 partid); /** * wallet_payment_delete - Remove a payment @@ -938,7 +944,8 @@ void wallet_payment_store(struct wallet *wallet, * Removes the payment from the database. */ void wallet_payment_delete(struct wallet *wallet, - const struct sha256 *payment_hash); + const struct sha256 *payment_hash, + u64 partid); /** * wallet_local_htlc_out_delete - Remove a local outgoing failed HTLC @@ -949,7 +956,8 @@ void wallet_payment_delete(struct wallet *wallet, */ void wallet_local_htlc_out_delete(struct wallet *wallet, struct channel *chan, - const struct sha256 *payment_hash); + const struct sha256 *payment_hash, + u64 partid); /** * wallet_payment_by_hash - Retrieve a specific payment @@ -958,7 +966,8 @@ void wallet_local_htlc_out_delete(struct wallet *wallet, */ struct wallet_payment * wallet_payment_by_hash(const tal_t *ctx, struct wallet *wallet, - const struct sha256 *payment_hash); + const struct sha256 *payment_hash, + u64 partid); /** * wallet_payment_set_status - Update the status of the payment @@ -967,9 +976,10 @@ wallet_payment_by_hash(const tal_t *ctx, struct wallet *wallet, * its state. */ void wallet_payment_set_status(struct wallet *wallet, - const struct sha256 *payment_hash, - const enum wallet_payment_status newstatus, - const struct preimage *preimage); + const struct sha256 *payment_hash, + u64 partid, + const enum wallet_payment_status newstatus, + const struct preimage *preimage); /** * wallet_payment_get_failinfo - Get failure information for a given @@ -981,6 +991,7 @@ void wallet_payment_set_status(struct wallet *wallet, void wallet_payment_get_failinfo(const tal_t *ctx, struct wallet *wallet, const struct sha256 *payment_hash, + u64 partid, /* outputs */ u8 **failonionreply, bool *faildestperm, @@ -997,6 +1008,7 @@ void wallet_payment_get_failinfo(const tal_t *ctx, */ void wallet_payment_set_failinfo(struct wallet *wallet, const struct sha256 *payment_hash, + u64 partid, const u8 *failonionreply, bool faildestperm, int failindex, From ad4ed97da5a4818f3235fded2bbb9b2fd705db55 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Thu, 12 Dec 2019 10:09:07 +1030 Subject: [PATCH 02/22] db: add partid field to htlc_out. This is in preparation for partial payments. For existing payments, partid is 0 (to match the corresponding payment). Signed-off-by: Rusty Russell --- lightningd/htlc_end.c | 3 +++ lightningd/htlc_end.h | 4 ++++ lightningd/pay.c | 30 +++++++++++++++++------------- lightningd/peer_htlcs.c | 9 +++++---- lightningd/peer_htlcs.h | 1 + wallet/db.c | 2 ++ wallet/wallet.c | 15 +++++++++++---- 7 files changed, 43 insertions(+), 21 deletions(-) diff --git a/lightningd/htlc_end.c b/lightningd/htlc_end.c index ab3737767196..ff89ada43438 100644 --- a/lightningd/htlc_end.c +++ b/lightningd/htlc_end.c @@ -252,6 +252,7 @@ struct htlc_out *new_htlc_out(const tal_t *ctx, const struct sha256 *payment_hash, const u8 *onion_routing_packet, bool am_origin, + u64 partid, struct htlc_in *in) { struct htlc_out *hout = tal(ctx, struct htlc_out); @@ -273,6 +274,8 @@ struct htlc_out *new_htlc_out(const tal_t *ctx, hout->preimage = NULL; hout->am_origin = am_origin; + if (am_origin) + hout->partid = partid; hout->in = NULL; if (in) htlc_out_connect_htlc_in(hout, in); diff --git a/lightningd/htlc_end.h b/lightningd/htlc_end.h index a955487bc46f..16e20bb1e702 100644 --- a/lightningd/htlc_end.h +++ b/lightningd/htlc_end.h @@ -82,6 +82,9 @@ struct htlc_out { /* Is this a locally-generated payment? Implies ->in is NULL. */ bool am_origin; + /* If am_origin, this is the partid of the payment. */ + u64 partid; + /* Where it's from, if not going to us. */ struct htlc_in *in; }; @@ -140,6 +143,7 @@ struct htlc_out *new_htlc_out(const tal_t *ctx, const struct sha256 *payment_hash, const u8 *onion_routing_packet, bool am_origin, + u64 partid, struct htlc_in *in); void connect_htlc_in(struct htlc_in_map *map, struct htlc_in *hin); diff --git a/lightningd/pay.c b/lightningd/pay.c index 33608db98ab6..809b2a3de50a 100644 --- a/lightningd/pay.c +++ b/lightningd/pay.c @@ -503,7 +503,7 @@ void payment_failed(struct lightningd *ld, const struct htlc_out *hout, payment = wallet_payment_by_hash(tmpctx, ld->wallet, &hout->payment_hash, - /* FIXME: Set partid! */0); + hout->partid); #ifdef COMPAT_V052 /* Prior to "pay: delete HTLC when we delete payment." we would @@ -573,11 +573,13 @@ void payment_failed(struct lightningd *ld, const struct htlc_out *hout, } /* Save to DB */ - payment_store(ld, &hout->payment_hash, /* FIXME: Set partid! */ 0); - wallet_payment_set_status(ld->wallet, &hout->payment_hash, /* FIXME: Set partid! */ 0, + payment_store(ld, &hout->payment_hash, hout->partid); + wallet_payment_set_status(ld->wallet, &hout->payment_hash, + hout->partid, PAYMENT_FAILED, NULL); wallet_payment_set_failinfo(ld->wallet, - &hout->payment_hash, /* FIXME: Set partid! */ 0, + &hout->payment_hash, + hout->partid, fail ? NULL : hout->failuremsg, pay_errcode == PAY_DESTINATION_PERM_FAIL, fail ? fail->erring_index : -1, @@ -697,19 +699,20 @@ static bool should_use_tlv(enum route_hop_style style) } static enum onion_type send_onion(struct lightningd *ld, - const struct onionpacket *packet, - const struct route_hop *first_hop, - const struct sha256 *payment_hash, - struct channel *channel, - struct htlc_out **hout) + const struct onionpacket *packet, + const struct route_hop *first_hop, + const struct sha256 *payment_hash, + u64 partid, + struct channel *channel, + struct htlc_out **hout) { const u8 *onion; unsigned int base_expiry; base_expiry = get_block_height(ld->topology) + 1; onion = serialize_onionpacket(tmpctx, packet); return send_htlc_out(channel, first_hop->amount, - base_expiry + first_hop->delay, - payment_hash, onion, NULL, hout); + base_expiry + first_hop->delay, + payment_hash, partid, onion, NULL, hout); } /* Returns command_result if cmd was resolved, NULL if not yet called. */ @@ -840,7 +843,8 @@ send_payment(struct lightningd *ld, } packet = create_onionpacket(tmpctx, path, &path_secrets); - failcode = send_onion(ld, packet, &route[0], rhash, channel, &hout); + failcode = send_onion(ld, packet, &route[0], rhash, partid, + channel, &hout); log_info(ld->log, "Sending %s over %zu hops to deliver %s", type_to_string(tmpctx, struct amount_msat, &route[0].amount), n_hops, type_to_string(tmpctx, struct amount_msat, &msat)); @@ -1048,7 +1052,7 @@ static struct command_result *json_sendonion(struct command *cmd, wallet_local_htlc_out_delete(ld->wallet, channel, payment_hash, /* FIXME: Set partid! */0); } - failcode = send_onion(cmd->ld, &packet, first_hop, payment_hash, channel, + failcode = send_onion(cmd->ld, &packet, first_hop, payment_hash, /* FIXME: Set partid! */0, channel, &hout); payment = tal(hout, struct wallet_payment); diff --git a/lightningd/peer_htlcs.c b/lightningd/peer_htlcs.c index 39df3f2a9dea..1edb6b50b5e3 100644 --- a/lightningd/peer_htlcs.c +++ b/lightningd/peer_htlcs.c @@ -452,6 +452,7 @@ static void htlc_offer_timeout(struct channel *channel) enum onion_type send_htlc_out(struct channel *out, struct amount_msat amount, u32 cltv, const struct sha256 *payment_hash, + u64 partid, const u8 *onion_routing_packet, struct htlc_in *in, struct htlc_out **houtp) @@ -479,7 +480,8 @@ enum onion_type send_htlc_out(struct channel *out, /* Make peer's daemon own it, catch if it dies. */ hout = new_htlc_out(out->owner, out, amount, cltv, - payment_hash, onion_routing_packet, in == NULL, in); + payment_hash, onion_routing_packet, in == NULL, + partid, in); tal_add_destructor(hout, destroy_hout_subd_died); /* Give channel 30 seconds to commit (first) htlc. */ @@ -590,7 +592,7 @@ static void forward_htlc(struct htlc_in *hin, hout = tal(tmpctx, struct htlc_out); failcode = send_htlc_out(next, amt_to_forward, outgoing_cltv_value, &hin->payment_hash, - next_onion, hin, &hout); + 0, next_onion, hin, &hout); if (!failcode) return; @@ -1274,8 +1276,7 @@ static bool update_out_htlc(struct channel *channel, /* For our own HTLCs, we commit payment to db lazily */ if (hout->origin_htlc_id == 0) payment_store(ld, - &hout->payment_hash, - /* FIXME: Set partid! */ 0); + &hout->payment_hash, hout->partid); } if (!htlc_out_update_state(channel, hout, newstate)) diff --git a/lightningd/peer_htlcs.h b/lightningd/peer_htlcs.h index fdf0b4e477b1..da8e75623768 100644 --- a/lightningd/peer_htlcs.h +++ b/lightningd/peer_htlcs.h @@ -51,6 +51,7 @@ void update_per_commit_point(struct channel *channel, enum onion_type send_htlc_out(struct channel *out, struct amount_msat amount, u32 cltv, const struct sha256 *payment_hash, + u64 partid, const u8 *onion_routing_packet, struct htlc_in *in, struct htlc_out **houtp); diff --git a/wallet/db.c b/wallet/db.c index 6497ee29372c..8652224b5122 100644 --- a/wallet/db.c +++ b/wallet/db.c @@ -559,6 +559,8 @@ static struct migration dbmigrations[] = { {SQL("UPDATE payments SET total_msat = msatoshi;"), NULL}, {SQL("UPDATE payments SET partid = 0;"), NULL}, {SQL("DROP TABLE temp_payments;"), NULL}, + {SQL("ALTER TABLE channel_htlcs ADD partid BIGINT;"), NULL}, + {SQL("UPDATE channel_htlcs SET partid = 0;"), NULL}, }; /* Leak tracking. */ diff --git a/wallet/wallet.c b/wallet/wallet.c index e452a42340e8..1af89b55dba4 100644 --- a/wallet/wallet.c +++ b/wallet/wallet.c @@ -1661,7 +1661,8 @@ void wallet_htlc_save_out(struct wallet *wallet, " payment_hash," " payment_key," " hstate," - " routing_onion) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?);")); + " routing_onion," + " partid) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?);")); db_bind_u64(stmt, 0, chan->dbid); db_bind_u64(stmt, 1, out->key.id); @@ -1682,6 +1683,10 @@ void wallet_htlc_save_out(struct wallet *wallet, db_bind_blob(stmt, 9, out->onion_routing_packet, sizeof(out->onion_routing_packet)); + if (!out->am_origin) + db_bind_null(stmt, 10); + else + db_bind_u64(stmt, 10, out->partid); db_exec_prepared_v2(stmt); out->dbid = db_last_insert_id_v2(stmt); @@ -1796,6 +1801,7 @@ static bool wallet_stmt2htlc_out(struct channel *channel, out->am_origin = false; } else { out->origin_htlc_id = 0; + out->partid = db_column_u64(stmt, 13); out->am_origin = true; } @@ -1900,6 +1906,7 @@ bool wallet_htlcs_load_for_channel(struct wallet *wallet, ", origin_htlc" ", shared_secret" ", received_time" + ", partid" " FROM channel_htlcs" " WHERE direction = ?" " AND channel_id = ?" @@ -2047,15 +2054,15 @@ void wallet_local_htlc_out_delete(struct wallet *wallet, { struct db_stmt *stmt; - /* FIXME: Put partid into locally-generated htlc_out, select here! */ stmt = db_prepare_v2(wallet->db, SQL("DELETE FROM channel_htlcs" " WHERE direction = ?" " AND origin_htlc = ?" - " AND payment_hash = ?")); + " AND payment_hash = ?" + " AND partid = ?;")); db_bind_int(stmt, 0, DIRECTION_OUTGOING); db_bind_int(stmt, 1, 0); db_bind_sha256(stmt, 2, payment_hash); - + db_bind_u64(stmt, 3, partid); db_exec_prepared_v2(take(stmt)); } From 2a034343a6ef27c1a33fb058afa26429182adce7 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Thu, 12 Dec 2019 10:09:10 +1030 Subject: [PATCH 03/22] htlcs: remove origin_htlc_id from htlc_out. This is a transient field, so rework things so we don't leave it in struct htlc_out. Instead, load htlc_in first and connect htlc_out to them as we go. This also changes one place where we use it instead of the am_origin flag. Signed-off-by: Rusty Russell --- lightningd/htlc_end.c | 16 ++++ lightningd/htlc_end.h | 5 +- lightningd/lightningd.c | 6 +- lightningd/peer_control.c | 34 ++++++-- lightningd/peer_htlcs.c | 95 ++++----------------- lightningd/peer_htlcs.h | 8 +- lightningd/test/run-find_my_abspath.c | 3 +- lightningd/test/run-invoice-select-inchan.c | 25 +++--- wallet/test/run-wallet.c | 13 ++- wallet/wallet.c | 61 +++++++++---- wallet/wallet.h | 44 +++++----- 11 files changed, 166 insertions(+), 144 deletions(-) diff --git a/lightningd/htlc_end.c b/lightningd/htlc_end.c index ff89ada43438..02a5ce97e3df 100644 --- a/lightningd/htlc_end.c +++ b/lightningd/htlc_end.c @@ -28,6 +28,22 @@ struct htlc_in *find_htlc_in(const struct htlc_in_map *map, return htlc_in_map_get(map, &key); } +struct htlc_in *remove_htlc_in_by_dbid(struct htlc_in_map *remaining_htlcs_in, + u64 dbid) +{ + struct htlc_in *hin; + struct htlc_in_map_iter ini; + + for (hin = htlc_in_map_first(remaining_htlcs_in, &ini); hin; + hin = htlc_in_map_next(remaining_htlcs_in, &ini)) { + if (hin->dbid == dbid) { + htlc_in_map_del(remaining_htlcs_in, hin); + return hin; + } + } + return NULL; +} + static void destroy_htlc_in(struct htlc_in *hend, struct htlc_in_map *map) { htlc_in_map_del(map, hend); diff --git a/lightningd/htlc_end.h b/lightningd/htlc_end.h index 16e20bb1e702..bd9bd883a4fc 100644 --- a/lightningd/htlc_end.h +++ b/lightningd/htlc_end.h @@ -58,7 +58,6 @@ struct htlc_out { * is saved to the database, must be >0 after saving to the * database. */ u64 dbid; - u64 origin_htlc_id; struct htlc_key key; struct amount_msat msat; u32 cltv_expiry; @@ -123,6 +122,10 @@ struct htlc_in *find_htlc_in(const struct htlc_in_map *map, const struct channel *channel, u64 htlc_id); +/* FIXME: Slow function only used at startup. */ +struct htlc_in *remove_htlc_in_by_dbid(struct htlc_in_map *remaining_htlcs_in, + u64 dbid); + struct htlc_out *find_htlc_out(const struct htlc_out_map *map, const struct channel *channel, u64 htlc_id); diff --git a/lightningd/lightningd.c b/lightningd/lightningd.c index 75f250a4a316..876cc98341f4 100644 --- a/lightningd/lightningd.c +++ b/lightningd/lightningd.c @@ -630,7 +630,7 @@ int main(int argc, char *argv[]) int stop_fd; struct timers *timers; const char *stop_response; - struct htlc_in_map *unprocessed_htlcs; + struct htlc_in_map *unconnected_htlcs_in; struct rlimit nofile = {1024, 1024}; /*~ Make sure that we limit ourselves to something reasonable. Modesty @@ -778,7 +778,7 @@ int main(int argc, char *argv[]) * topology is initialized since some decisions rely on being able to * know the blockheight. */ db_begin_transaction(ld->wallet->db); - unprocessed_htlcs = load_channels_from_wallet(ld); + unconnected_htlcs_in = load_channels_from_wallet(ld); db_commit_transaction(ld->wallet->db); /*~ Create RPC socket: now lightning-cli can send us JSON RPC commands @@ -792,7 +792,7 @@ int main(int argc, char *argv[]) /*~ Process any HTLCs we were in the middle of when we exited, now * that plugins (who might want to know via htlc_accepted hook) are * active. */ - htlcs_resubmit(ld, unprocessed_htlcs); + htlcs_resubmit(ld, unconnected_htlcs_in); /*~ Activate connect daemon. Needs to be after the initialization of * chaintopology, otherwise peers may connect and ask for diff --git a/lightningd/peer_control.c b/lightningd/peer_control.c index 22806dca7447..676f4fa17111 100644 --- a/lightningd/peer_control.c +++ b/lightningd/peer_control.c @@ -1579,27 +1579,47 @@ void activate_peers(struct lightningd *ld) struct htlc_in_map *load_channels_from_wallet(struct lightningd *ld) { struct peer *peer; + struct htlc_in_map *unconnected_htlcs_in = tal(ld, struct htlc_in_map); /* Load channels from database */ if (!wallet_init_channels(ld->wallet)) fatal("Could not load channels from the database"); - /* This is a poor-man's db join :( */ + /* First we load the incoming htlcs */ list_for_each(&ld->peers, peer, list) { struct channel *channel; list_for_each(&peer->channels, channel, list) { - if (!wallet_htlcs_load_for_channel(ld->wallet, - channel, - &ld->htlcs_in, - &ld->htlcs_out)) { + if (!wallet_htlcs_load_in_for_channel(ld->wallet, + channel, + &ld->htlcs_in)) { fatal("could not load htlcs for channel"); } } } - /* Now connect HTLC pointers together */ - return htlcs_reconnect(ld, &ld->htlcs_in, &ld->htlcs_out); + /* Make a copy of the htlc_map: entries removed as they're matched */ + htlc_in_map_copy(unconnected_htlcs_in, &ld->htlcs_in); + + /* Now we load the outgoing HTLCs, so we can connect them. */ + list_for_each(&ld->peers, peer, list) { + struct channel *channel; + + list_for_each(&peer->channels, channel, list) { + if (!wallet_htlcs_load_out_for_channel(ld->wallet, + channel, + &ld->htlcs_out, + unconnected_htlcs_in)) { + fatal("could not load outgoing htlcs for channel"); + } + } + } + +#ifdef COMPAT_V061 + fixup_htlcs_out(ld); +#endif /* COMPAT_V061 */ + + return unconnected_htlcs_in; } static struct command_result *json_disconnect(struct command *cmd, diff --git a/lightningd/peer_htlcs.c b/lightningd/peer_htlcs.c index 1edb6b50b5e3..77370da4a724 100644 --- a/lightningd/peer_htlcs.c +++ b/lightningd/peer_htlcs.c @@ -1274,7 +1274,7 @@ static bool update_out_htlc(struct channel *channel, } /* For our own HTLCs, we commit payment to db lazily */ - if (hout->origin_htlc_id == 0) + if (hout->am_origin) payment_store(ld, &hout->payment_hash, hout->partid); } @@ -2089,94 +2089,35 @@ static void fixup_hout(struct lightningd *ld, struct htlc_out *hout) &hout->key.channel->peer->id), fix); } -#endif /* COMPAT_V061 */ -/** - * htlcs_reconnect -- Link outgoing HTLCs to their origins after initial db load - * - * For each outgoing HTLC find the incoming HTLC that triggered it. If - * we are the origin of the transfer then we cannot resolve the - * incoming HTLC in which case we just leave it `NULL`. - * - * Returns a map of any htlcs we need to retry. - */ -struct htlc_in_map *htlcs_reconnect(struct lightningd *ld, - struct htlc_in_map *htlcs_in, - struct htlc_out_map *htlcs_out) +void fixup_htlcs_out(struct lightningd *ld) { - struct htlc_in_map_iter ini; struct htlc_out_map_iter outi; - struct htlc_in *hin; struct htlc_out *hout; - struct htlc_in_map *unprocessed = tal(NULL, struct htlc_in_map); - - /* Any HTLCs which happened to be incoming and weren't forwarded before - * we shutdown/crashed: fail them now. - * - * Note that since we do local processing synchronously, so this never - * captures local payments. But if it did, it would be a tiny corner - * case. */ - htlc_in_map_init(unprocessed); - for (hin = htlc_in_map_first(htlcs_in, &ini); hin; - hin = htlc_in_map_next(htlcs_in, &ini)) { - if (hin->hstate == RCVD_ADD_ACK_REVOCATION) - htlc_in_map_add(unprocessed, hin); - } - - for (hout = htlc_out_map_first(htlcs_out, &outi); hout; - hout = htlc_out_map_next(htlcs_out, &outi)) { - - if (hout->am_origin) { - continue; - } - - /* For fulfilled HTLCs, we fulfill incoming before outgoing is - * completely resolved, so it's possible that we don't find - * the incoming. */ - for (hin = htlc_in_map_first(htlcs_in, &ini); hin; - hin = htlc_in_map_next(htlcs_in, &ini)) { - if (hout->origin_htlc_id == hin->dbid) { - log_debug(ld->log, - "Found corresponding htlc_in %" PRIu64 - " for htlc_out %" PRIu64, - hin->dbid, hout->dbid); - htlc_out_connect_htlc_in(hout, hin); - break; - } - } - - if (!hout->in && !hout->preimage) { -#ifdef COMPAT_V061 - log_broken(ld->log, - "Missing preimage for orphaned HTLC; replacing with zeros"); - hout->preimage = talz(hout, struct preimage); -#else - fatal("Unable to find corresponding htlc_in %"PRIu64 - " for unfulfilled htlc_out %"PRIu64, - hout->origin_htlc_id, hout->dbid); -#endif - } -#ifdef COMPAT_V061 - fixup_hout(ld, hout); -#endif - if (hout->in) - htlc_in_map_del(unprocessed, hout->in); + for (hout = htlc_out_map_first(&ld->htlcs_out, &outi); + hout; + hout = htlc_out_map_next(&ld->htlcs_out, &outi)) { + if (!hout->am_origin) + fixup_hout(ld, hout); } - - return unprocessed; } +#endif /* COMPAT_V061 */ -void htlcs_resubmit(struct lightningd *ld, struct htlc_in_map *unprocessed) +void htlcs_resubmit(struct lightningd *ld, + struct htlc_in_map *unconnected_htlcs_in) { struct htlc_in *hin; struct htlc_in_map_iter ini; enum onion_type failcode COMPILER_WANTS_INIT("gcc7.4.0 bad, 8.3 OK"); - /* Now fail any which were stuck. */ - for (hin = htlc_in_map_first(unprocessed, &ini); + /* Now retry any which were stuck. */ + for (hin = htlc_in_map_first(unconnected_htlcs_in, &ini); hin; - hin = htlc_in_map_next(unprocessed, &ini)) { + hin = htlc_in_map_next(unconnected_htlcs_in, &ini)) { + if (hin->hstate != RCVD_ADD_ACK_REVOCATION) + continue; + log_unusual(hin->key.channel->log, "Replaying old unprocessed HTLC #%"PRIu64, hin->key.id); @@ -2192,8 +2133,8 @@ void htlcs_resubmit(struct lightningd *ld, struct htlc_in_map *unprocessed) } /* Don't leak memory! */ - htlc_in_map_clear(unprocessed); - tal_free(unprocessed); + htlc_in_map_clear(unconnected_htlcs_in); + tal_free(unconnected_htlcs_in); } #if DEVELOPER diff --git a/lightningd/peer_htlcs.h b/lightningd/peer_htlcs.h index da8e75623768..f6634d12b8ee 100644 --- a/lightningd/peer_htlcs.h +++ b/lightningd/peer_htlcs.h @@ -64,11 +64,11 @@ void onchain_fulfilled_htlc(struct channel *channel, void htlcs_notify_new_block(struct lightningd *ld, u32 height); -struct htlc_in_map *htlcs_reconnect(struct lightningd *ld, - struct htlc_in_map *htlcs_in, - struct htlc_out_map *htlcs_out); +/* Only defined if COMPAT_V061 */ +void fixup_htlcs_out(struct lightningd *ld); -void htlcs_resubmit(struct lightningd *ld, struct htlc_in_map *unprocessed); +void htlcs_resubmit(struct lightningd *ld, + struct htlc_in_map *unconnected_htlcs_in); /* For HTLCs which terminate here, invoice payment calls one of these. */ void fulfill_htlc(struct htlc_in *hin, const struct preimage *preimage); diff --git a/lightningd/test/run-find_my_abspath.c b/lightningd/test/run-find_my_abspath.c index c769c41d4202..deb55d49301c 100644 --- a/lightningd/test/run-find_my_abspath.c +++ b/lightningd/test/run-find_my_abspath.c @@ -97,7 +97,8 @@ void hsm_init(struct lightningd *ld UNNEEDED) void htlcs_notify_new_block(struct lightningd *ld UNNEEDED, u32 height UNNEEDED) { fprintf(stderr, "htlcs_notify_new_block called!\n"); abort(); } /* Generated stub for htlcs_resubmit */ -void htlcs_resubmit(struct lightningd *ld UNNEEDED, struct htlc_in_map *unprocessed UNNEEDED) +void htlcs_resubmit(struct lightningd *ld UNNEEDED, + struct htlc_in_map *unconnected_htlcs_in UNNEEDED) { fprintf(stderr, "htlcs_resubmit called!\n"); abort(); } /* Generated stub for jsonrpc_listen */ void jsonrpc_listen(struct jsonrpc *rpc UNNEEDED, struct lightningd *ld UNNEEDED) diff --git a/lightningd/test/run-invoice-select-inchan.c b/lightningd/test/run-invoice-select-inchan.c index 8768e22eb60a..33e078df938e 100644 --- a/lightningd/test/run-invoice-select-inchan.c +++ b/lightningd/test/run-invoice-select-inchan.c @@ -96,6 +96,9 @@ void fatal(const char *fmt UNNEEDED, ...) /* Generated stub for feature_is_set */ bool feature_is_set(const u8 *features UNNEEDED, size_t bit UNNEEDED) { fprintf(stderr, "feature_is_set called!\n"); abort(); } +/* Generated stub for fixup_htlcs_out */ +void fixup_htlcs_out(struct lightningd *ld UNNEEDED) +{ fprintf(stderr, "fixup_htlcs_out called!\n"); abort(); } /* Generated stub for fromwire_channel_dev_memleak_reply */ bool fromwire_channel_dev_memleak_reply(const void *p UNNEEDED, bool *leak UNNEEDED) { fprintf(stderr, "fromwire_channel_dev_memleak_reply called!\n"); abort(); } @@ -133,11 +136,6 @@ bool htlc_is_trimmed(enum side htlc_owner UNNEEDED, struct amount_sat dust_limit UNNEEDED, enum side side UNNEEDED) { fprintf(stderr, "htlc_is_trimmed called!\n"); abort(); } -/* Generated stub for htlcs_reconnect */ -struct htlc_in_map *htlcs_reconnect(struct lightningd *ld UNNEEDED, - struct htlc_in_map *htlcs_in UNNEEDED, - struct htlc_out_map *htlcs_out UNNEEDED) -{ fprintf(stderr, "htlcs_reconnect called!\n"); abort(); } /* Generated stub for json_add_address */ void json_add_address(struct json_stream *response UNNEEDED, const char *fieldname UNNEEDED, const struct wireaddr *addr UNNEEDED) @@ -504,12 +502,17 @@ void wallet_channeltxs_add(struct wallet *w UNNEEDED, struct channel *chan UNNEE const int type UNNEEDED, const struct bitcoin_txid *txid UNNEEDED, const u32 input_num UNNEEDED, const u32 blockheight UNNEEDED) { fprintf(stderr, "wallet_channeltxs_add called!\n"); abort(); } -/* Generated stub for wallet_htlcs_load_for_channel */ -bool wallet_htlcs_load_for_channel(struct wallet *wallet UNNEEDED, - struct channel *chan UNNEEDED, - struct htlc_in_map *htlcs_in UNNEEDED, - struct htlc_out_map *htlcs_out UNNEEDED) -{ fprintf(stderr, "wallet_htlcs_load_for_channel called!\n"); abort(); } +/* Generated stub for wallet_htlcs_load_in_for_channel */ +bool wallet_htlcs_load_in_for_channel(struct wallet *wallet UNNEEDED, + struct channel *chan UNNEEDED, + struct htlc_in_map *htlcs_in UNNEEDED) +{ fprintf(stderr, "wallet_htlcs_load_in_for_channel called!\n"); abort(); } +/* Generated stub for wallet_htlcs_load_out_for_channel */ +bool wallet_htlcs_load_out_for_channel(struct wallet *wallet UNNEEDED, + struct channel *chan UNNEEDED, + struct htlc_out_map *htlcs_out UNNEEDED, + struct htlc_in_map *remaining_htlcs_in UNNEEDED) +{ fprintf(stderr, "wallet_htlcs_load_out_for_channel called!\n"); abort(); } /* Generated stub for wallet_init_channels */ bool wallet_init_channels(struct wallet *w UNNEEDED) { fprintf(stderr, "wallet_init_channels called!\n"); abort(); } diff --git a/wallet/test/run-wallet.c b/wallet/test/run-wallet.c index 4fc199d9a433..ca2c5b785207 100644 --- a/wallet/test/run-wallet.c +++ b/wallet/test/run-wallet.c @@ -1156,7 +1156,7 @@ static bool test_htlc_crud(struct lightningd *ld, const tal_t *ctx) struct channel *chan = tal(ctx, struct channel); struct peer *peer = talz(ctx, struct peer); struct wallet *w = create_test_wallet(ld, ctx); - struct htlc_in_map *htlcs_in = tal(ctx, struct htlc_in_map); + struct htlc_in_map *htlcs_in = tal(ctx, struct htlc_in_map), *rem; struct htlc_out_map *htlcs_out = tal(ctx, struct htlc_out_map); /* Make sure we have our references correct */ @@ -1220,11 +1220,16 @@ static bool test_htlc_crud(struct lightningd *ld, const tal_t *ctx) db_begin_transaction(w->db); CHECK(!wallet_err); - CHECK_MSG(wallet_htlcs_load_for_channel(w, chan, htlcs_in, htlcs_out), - "Failed loading HTLCs"); + CHECK_MSG(wallet_htlcs_load_in_for_channel(w, chan, htlcs_in), + "Failed loading in HTLCs"); + /* Freed by htlcs_resubmit */ + rem = tal(NULL, struct htlc_in_map); + htlc_in_map_copy(rem, htlcs_in); + CHECK_MSG(wallet_htlcs_load_out_for_channel(w, chan, htlcs_out, rem), + "Failed loading out HTLCs"); db_commit_transaction(w->db); - htlcs_resubmit(w->ld, htlcs_reconnect(w->ld, htlcs_in, htlcs_out)); + htlcs_resubmit(w->ld, rem); CHECK(!wallet_err); hin = htlc_in_map_get(htlcs_in, &in.key); diff --git a/wallet/wallet.c b/wallet/wallet.c index 1af89b55dba4..3379c2b9702e 100644 --- a/wallet/wallet.c +++ b/wallet/wallet.c @@ -1647,7 +1647,6 @@ void wallet_htlc_save_out(struct wallet *wallet, /* We absolutely need the incoming HTLC to be persisted before * we can persist it's dependent */ assert(out->in == NULL || out->in->dbid != 0); - out->origin_htlc_id = out->in?out->in->dbid:0; stmt = db_prepare_v2( wallet->db, @@ -1770,8 +1769,11 @@ static bool wallet_stmt2htlc_in(struct channel *channel, return ok; } -static bool wallet_stmt2htlc_out(struct channel *channel, - struct db_stmt *stmt, struct htlc_out *out) +/* Removes matching htlc from unconnected_htlcs_in */ +static bool wallet_stmt2htlc_out(struct wallet *wallet, + struct channel *channel, + struct db_stmt *stmt, struct htlc_out *out, + struct htlc_in_map *unconnected_htlcs_in) { bool ok = true; out->dbid = db_column_u64(stmt, 0); @@ -1795,20 +1797,32 @@ static bool wallet_stmt2htlc_out(struct channel *channel, out->failuremsg = db_column_arr(out, stmt, 8, u8); out->failcode = db_column_int_or_default(stmt, 9, 0); + out->in = NULL; if (!db_column_is_null(stmt, 10)) { - out->origin_htlc_id = db_column_u64(stmt, 10); + u64 in_id = db_column_u64(stmt, 10); + struct htlc_in *hin; + + hin = remove_htlc_in_by_dbid(unconnected_htlcs_in, in_id); + if (hin) + htlc_out_connect_htlc_in(out, hin); out->am_origin = false; + if (!out->in && !out->preimage) { +#ifdef COMPAT_V061 + log_broken(wallet->log, + "Missing preimage for orphaned HTLC; replacing with zeros"); + out->preimage = talz(out, struct preimage); +#else + fatal("Unable to find corresponding htlc_in %"PRIu64 + " for unfulfilled htlc_out %"PRIu64, + in_id, out->dbid); +#endif + } } else { - out->origin_htlc_id = 0; out->partid = db_column_u64(stmt, 13); out->am_origin = true; } - /* Need to defer wiring until we can look up all incoming - * htlcs, will wire using origin_htlc_id */ - out->in = NULL; - return ok; } @@ -1849,16 +1863,15 @@ static void fixup_hin(struct wallet *wallet, struct htlc_in *hin) #endif } -bool wallet_htlcs_load_for_channel(struct wallet *wallet, - struct channel *chan, - struct htlc_in_map *htlcs_in, - struct htlc_out_map *htlcs_out) +bool wallet_htlcs_load_in_for_channel(struct wallet *wallet, + struct channel *chan, + struct htlc_in_map *htlcs_in) { struct db_stmt *stmt; bool ok = true; - int incount = 0, outcount = 0; + int incount = 0; - log_debug(wallet->log, "Loading HTLCs for channel %"PRIu64, chan->dbid); + log_debug(wallet->log, "Loading in HTLCs for channel %"PRIu64, chan->dbid); stmt = db_prepare_v2(wallet->db, SQL("SELECT" " id" ", channel_htlc_id" @@ -1892,6 +1905,19 @@ bool wallet_htlcs_load_for_channel(struct wallet *wallet, } tal_free(stmt); + log_debug(wallet->log, "Restored %d incoming HTLCS", incount); + return ok; +} + +bool wallet_htlcs_load_out_for_channel(struct wallet *wallet, + struct channel *chan, + struct htlc_out_map *htlcs_out, + struct htlc_in_map *unconnected_htlcs_in) +{ + struct db_stmt *stmt; + bool ok = true; + int outcount = 0; + stmt = db_prepare_v2(wallet->db, SQL("SELECT" " id" ", channel_htlc_id" @@ -1918,7 +1944,8 @@ bool wallet_htlcs_load_for_channel(struct wallet *wallet, while (db_step(stmt)) { struct htlc_out *out = tal(chan, struct htlc_out); - ok &= wallet_stmt2htlc_out(chan, stmt, out); + ok &= wallet_stmt2htlc_out(wallet, chan, stmt, out, + unconnected_htlcs_in); connect_htlc_out(htlcs_out, out); /* Cannot htlc_out_check because we haven't wired the * dependencies in yet */ @@ -1926,7 +1953,7 @@ bool wallet_htlcs_load_for_channel(struct wallet *wallet, } tal_free(stmt); - log_debug(wallet->log, "Restored %d incoming and %d outgoing HTLCS", incount, outcount); + log_debug(wallet->log, "Restored %d outgoing HTLCS", outcount); return ok; } diff --git a/wallet/wallet.h b/wallet/wallet.h index 927007748a12..9cfbda3001ab 100644 --- a/wallet/wallet.h +++ b/wallet/wallet.h @@ -594,29 +594,35 @@ void wallet_htlc_update(struct wallet *wallet, const u64 htlc_dbid, enum onion_type failcode, const u8 *failuremsg); /** - * wallet_htlcs_load_for_channel - Load HTLCs associated with chan from DB. + * wallet_htlcs_load_in_for_channel - Load incoming HTLCs associated with chan from DB. * * @wallet: wallet to load from * @chan: load HTLCs associated with this channel * @htlcs_in: htlc_in_map to store loaded htlc_in in - * @htlcs_out: htlc_out_map to store loaded htlc_out in - * - * This function looks for HTLCs that are associated with the given - * channel and loads them into the provided maps. One caveat is that - * the `struct htlc_out` instances are not wired up with the - * corresponding `struct htlc_in` in the forwarding case nor are they - * associated with a `struct pay_command` in the case we originated - * the payment. In the former case the corresponding `struct htlc_in` - * may not have been loaded yet. In the latter case the pay_command - * does not exist anymore since we restarted. - * - * Use `htlcs_reconnect` to wire htlc_out instances to the - * corresponding htlc_in after loading all channels. - */ -bool wallet_htlcs_load_for_channel(struct wallet *wallet, - struct channel *chan, - struct htlc_in_map *htlcs_in, - struct htlc_out_map *htlcs_out); + * + * This function looks for incoming HTLCs that are associated with the given + * channel and loads them into the provided map. + */ +bool wallet_htlcs_load_in_for_channel(struct wallet *wallet, + struct channel *chan, + struct htlc_in_map *htlcs_in); + +/** + * wallet_htlcs_load_out_for_channel - Load outgoing HTLCs associated with chan from DB. + * + * @wallet: wallet to load from + * @chan: load HTLCs associated with this channel + * @htlcs_out: htlc_out_map to store loaded htlc_out in. + * @remaining_htlcs_in: htlc_in_map with unconnected htlcs (removed as we progress) + * + * We populate htlc_out->in by looking up in remaining_htlcs_in. It's + * possible that it's still NULL, since we can have outgoing HTLCs + * outlive their corresponding incoming. + */ +bool wallet_htlcs_load_out_for_channel(struct wallet *wallet, + struct channel *chan, + struct htlc_out_map *htlcs_out, + struct htlc_in_map *remaining_htlcs_in); /** * wallet_announcement_save - Save remote announcement information with channel. From c6544783294c447d0064cd42a98f7f252fbd6e84 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Thu, 12 Dec 2019 10:20:22 +1030 Subject: [PATCH 04/22] lightningd: share more code between sendpay and sendonion. In particular, we're about to do surgery on the detection-of-previous-payments logic, and we should not do this in two places. Signed-off-by: Rusty Russell --- lightningd/pay.c | 313 +++++++++++++++++++++-------------------------- 1 file changed, 138 insertions(+), 175 deletions(-) diff --git a/lightningd/pay.c b/lightningd/pay.c index 809b2a3de50a..8c48599773e9 100644 --- a/lightningd/pay.c +++ b/lightningd/pay.c @@ -715,84 +715,29 @@ static enum onion_type send_onion(struct lightningd *ld, payment_hash, partid, onion, NULL, hout); } -/* Returns command_result if cmd was resolved, NULL if not yet called. */ +/* destination/route_channels/route_nodes are NULL (and path_secrets may be NULL) + * if we're sending a raw onion. */ static struct command_result * -send_payment(struct lightningd *ld, - struct command *cmd, - const struct sha256 *rhash, - u64 partid, - const struct route_hop *route, - struct amount_msat msat, - struct amount_msat total_msat, - const char *label TAKES, - const char *b11str TAKES, - const struct secret *payment_secret) +send_payment_core(struct lightningd *ld, + struct command *cmd, + const struct sha256 *rhash, + u64 partid, + const struct route_hop *first_hop, + struct amount_msat msat, + struct amount_msat total_msat, + const char *label TAKES, + const char *b11str TAKES, + const struct onionpacket *packet, + const struct node_id *destination, + struct node_id *route_nodes TAKES, + struct short_channel_id *route_channels TAKES, + struct secret *path_secrets) { - unsigned int base_expiry; - struct onionpacket *packet; - struct secret *path_secrets; + struct wallet_payment *payment; + struct channel *channel; enum onion_type failcode; - size_t i, n_hops = tal_count(route); - struct node_id *ids = tal_arr(tmpctx, struct node_id, n_hops); - struct wallet_payment *payment = NULL; struct htlc_out *hout; - struct short_channel_id *channels; struct routing_failure *fail; - struct channel *channel; - struct sphinx_path *path; - struct pubkey pubkey; - bool final_tlv, ret; - u8 *onion; - - /* Expiry for HTLCs is absolute. And add one to give some margin. */ - base_expiry = get_block_height(ld->topology) + 1; - - path = sphinx_path_new(tmpctx, rhash->u.u8); - /* Extract IDs for each hop: create_onionpacket wants array. */ - for (i = 0; i < n_hops; i++) - ids[i] = route[i].nodeid; - - /* Create sphinx path */ - for (i = 0; i < n_hops - 1; i++) { - ret = pubkey_from_node_id(&pubkey, &ids[i]); - assert(ret); - - sphinx_add_hop(path, &pubkey, - take(onion_nonfinal_hop(NULL, - should_use_tlv(route[i].style), - &route[i + 1].channel_id, - route[i + 1].amount, - base_expiry + route[i + 1].delay))); - } - - /* And finally set the final hop to the special values in - * BOLT04 */ - ret = pubkey_from_node_id(&pubkey, &ids[i]); - assert(ret); - - final_tlv = should_use_tlv(route[i].style); - /* BOLT-3a09bc54f8443c4757b47541a5310aff6377ee21 #4: - * - Unless `node_announcement`, `init` message or the - * [BOLT #11](11-payment-encoding.md#tagged-fields) offers feature - * `var_onion_optin`: - * - MUST use the legacy payload format instead. - */ - /* In our case, we don't use it unless we also have a payment_secret; - * everyone should support this eventually */ - if (!final_tlv && payment_secret) - final_tlv = true; - - onion = onion_final_hop(cmd, - final_tlv, - route[i].amount, - base_expiry + route[i].delay, - route[i].amount, payment_secret); - if (!onion) { - return command_fail(cmd, PAY_DESTINATION_PERM_FAIL, - "Destination does not support" - " payment_secret"); - } - sphinx_add_hop(path, &pubkey, onion); /* Now, do we already have a payment? */ payment = wallet_payment_by_hash(tmpctx, ld->wallet, rhash, partid); @@ -813,9 +758,8 @@ send_payment(struct lightningd *ld, struct amount_msat, &payment->msatoshi)); } - if (payment->destination && - !node_id_eq(payment->destination, - &ids[n_hops - 1])) { + if (payment->destination && destination + && !node_id_eq(payment->destination, destination)) { return command_fail(cmd, PAY_RHASH_ALREADY_USED, "Already succeeded to %s", type_to_string(tmpctx, @@ -827,7 +771,7 @@ send_payment(struct lightningd *ld, log_debug(ld->log, "send_payment: found previous, retrying"); } - channel = active_channel_by_id(ld, &ids[0], NULL); + channel = active_channel_by_id(ld, &first_hop->nodeid, NULL); if (!channel) { struct json_stream *data = json_stream_fail(cmd, PAY_TRY_OTHER_ROUTE, @@ -835,35 +779,26 @@ send_payment(struct lightningd *ld, "peer found"); json_add_routefail_info(data, 0, WIRE_UNKNOWN_NEXT_PEER, - &ld->id, &route[0].channel_id, - node_id_idx(&ld->id, &route[0].nodeid), + &ld->id, &first_hop->channel_id, + node_id_idx(&ld->id, &first_hop->nodeid), NULL); json_object_end(data); return command_failed(cmd, data); } - packet = create_onionpacket(tmpctx, path, &path_secrets); - failcode = send_onion(ld, packet, &route[0], rhash, partid, + failcode = send_onion(ld, packet, first_hop, rhash, partid, channel, &hout); - log_info(ld->log, "Sending %s over %zu hops to deliver %s", - type_to_string(tmpctx, struct amount_msat, &route[0].amount), - n_hops, type_to_string(tmpctx, struct amount_msat, &msat)); if (failcode) { fail = immediate_routing_failure(cmd, ld, failcode, - &route[0].channel_id, + &first_hop->channel_id, &channel->peer->id); return sendpay_fail(cmd, payment, PAY_TRY_OTHER_ROUTE, NULL, fail, "First peer not ready"); } - /* Copy channels used along the route. */ - channels = tal_arr(tmpctx, struct short_channel_id, n_hops); - for (i = 0; i < n_hops; ++i) - channels[i] = route[i].channel_id; - /* If we're retrying, delete all trace of previous one. We delete * outgoing HTLC, too, otherwise it gets reported to onchaind as * a possibility, and we end up in handle_missing_htlc_output-> @@ -880,16 +815,25 @@ send_payment(struct lightningd *ld, payment->id = 0; payment->payment_hash = *rhash; payment->partid = partid; - payment->destination = tal_dup(payment, struct node_id, &ids[n_hops - 1]); + if (destination) + payment->destination = tal_dup(payment, struct node_id, destination); + else + payment->destination = NULL; payment->status = PAYMENT_PENDING; payment->msatoshi = msat; - payment->msatoshi_sent = route[0].amount; + payment->msatoshi_sent = first_hop->amount; payment->total_msat = total_msat; payment->timestamp = time_now().ts.tv_sec; payment->payment_preimage = NULL; payment->path_secrets = tal_steal(payment, path_secrets); - payment->route_nodes = tal_steal(payment, ids); - payment->route_channels = tal_steal(payment, channels); + if (route_nodes) + payment->route_nodes = tal_steal(payment, route_nodes); + else + payment->route_nodes = NULL; + if (route_channels) + payment->route_channels = tal_steal(payment, route_channels); + else + payment->route_channels = NULL; payment->failonion = NULL; if (label != NULL) payment->label = tal_strdup(payment, label); @@ -904,7 +848,95 @@ send_payment(struct lightningd *ld, wallet_payment_setup(ld->wallet, payment); add_sendpay_waiter(ld, cmd, rhash); - return NULL; + return command_still_pending(cmd); +} + +static struct command_result * +send_payment(struct lightningd *ld, + struct command *cmd, + const struct sha256 *rhash, + u64 partid, + const struct route_hop *route, + struct amount_msat msat, + struct amount_msat total_msat, + const char *label TAKES, + const char *b11str TAKES, + const struct secret *payment_secret) +{ + unsigned int base_expiry; + struct onionpacket *packet; + struct secret *path_secrets; + size_t i, n_hops = tal_count(route); + struct node_id *ids = tal_arr(tmpctx, struct node_id, n_hops); + struct short_channel_id *channels; + struct sphinx_path *path; + struct pubkey pubkey; + bool final_tlv, ret; + u8 *onion; + + /* Expiry for HTLCs is absolute. And add one to give some margin. */ + base_expiry = get_block_height(ld->topology) + 1; + + path = sphinx_path_new(tmpctx, rhash->u.u8); + /* Extract IDs for each hop: create_onionpacket wants array. */ + for (i = 0; i < n_hops; i++) + ids[i] = route[i].nodeid; + + /* Create sphinx path */ + for (i = 0; i < n_hops - 1; i++) { + ret = pubkey_from_node_id(&pubkey, &ids[i]); + assert(ret); + + sphinx_add_hop(path, &pubkey, + take(onion_nonfinal_hop(NULL, + should_use_tlv(route[i].style), + &route[i + 1].channel_id, + route[i + 1].amount, + base_expiry + route[i + 1].delay))); + } + + /* And finally set the final hop to the special values in + * BOLT04 */ + ret = pubkey_from_node_id(&pubkey, &ids[i]); + assert(ret); + + final_tlv = should_use_tlv(route[i].style); + /* BOLT-3a09bc54f8443c4757b47541a5310aff6377ee21 #4: + * - Unless `node_announcement`, `init` message or the + * [BOLT #11](11-payment-encoding.md#tagged-fields) offers feature + * `var_onion_optin`: + * - MUST use the legacy payload format instead. + */ + /* In our case, we don't use it unless we also have a payment_secret; + * everyone should support this eventually */ + if (!final_tlv && payment_secret) + final_tlv = true; + + onion = onion_final_hop(cmd, + final_tlv, + route[i].amount, + base_expiry + route[i].delay, + route[i].amount, payment_secret); + if (!onion) { + return command_fail(cmd, PAY_DESTINATION_PERM_FAIL, + "Destination does not support" + " payment_secret"); + } + sphinx_add_hop(path, &pubkey, onion); + + /* Copy channels used along the route. */ + channels = tal_arr(tmpctx, struct short_channel_id, n_hops); + for (i = 0; i < n_hops; ++i) + channels[i] = route[i].channel_id; + + log_info(ld->log, "Sending %s over %zu hops to deliver %s", + type_to_string(tmpctx, struct amount_msat, &route[0].amount), + n_hops, type_to_string(tmpctx, struct amount_msat, &msat)); + packet = create_onionpacket(tmpctx, path, &path_secrets); + return send_payment_core(ld, cmd, rhash, partid, &route[0], + msat, total_msat, label, b11str, + packet, &ids[n_hops - 1], ids, + channels, path_secrets); } static struct command_result * @@ -991,12 +1023,9 @@ static struct command_result *json_sendonion(struct command *cmd, u8 *onion; struct onionpacket packet; enum onion_type failcode; - struct htlc_out *hout; struct route_hop *first_hop; struct sha256 *payment_hash; - struct channel *channel; struct lightningd *ld = cmd->ld; - struct wallet_payment *payment; const char *label; struct secret *path_secrets; @@ -1017,74 +1046,12 @@ static struct command_result *json_sendonion(struct command *cmd, "with failcode=%d", failcode); - /* Now, do we already have a payment? */ - payment = wallet_payment_by_hash(tmpctx, ld->wallet, payment_hash, /* FIXME: Set partid! */0); - if (payment) { - if (payment->status == PAYMENT_PENDING) { - log_debug(ld->log, "send_payment: previous still in progress"); - return json_sendpay_in_progress(cmd, payment); - } - if (payment->status == PAYMENT_COMPLETE) { - log_debug(ld->log, "send_payment: previous succeeded"); - return sendpay_success(cmd, payment); - } - log_debug(ld->log, "send_payment: found previous, retrying"); - } - - channel = active_channel_by_id(ld, &first_hop->nodeid, NULL); - if (!channel) { - struct json_stream *data - = json_stream_fail(cmd, PAY_TRY_OTHER_ROUTE, - "No connection to first " - "peer found"); - - json_add_routefail_info(data, 0, WIRE_UNKNOWN_NEXT_PEER, - &ld->id, &first_hop->channel_id, - node_id_idx(&ld->id, &first_hop->nodeid), - NULL); - json_object_end(data); - return command_failed(cmd, data); - } - - /* Cleanup any prior payment. We're about to retry. */ - if (payment) { - wallet_payment_delete(ld->wallet, payment_hash, /* FIXME: Set partid! */0); - wallet_local_htlc_out_delete(ld->wallet, channel, payment_hash, /* FIXME: Set partid! */0); - } - - failcode = send_onion(cmd->ld, &packet, first_hop, payment_hash, /* FIXME: Set partid! */0, channel, - &hout); - - payment = tal(hout, struct wallet_payment); - payment->id = 0; - payment->payment_hash = *payment_hash; - payment->status = PAYMENT_PENDING; - payment->msatoshi = AMOUNT_MSAT(0); - payment->msatoshi_sent = first_hop->amount; - payment->timestamp = time_now().ts.tv_sec; - - /* These are not available for sendonion payments since the onion is - * opaque and we can't extract them. Errors have to be handled - * externally, since we can't decrypt them.*/ - payment->destination = NULL; - payment->payment_preimage = NULL; - payment->route_nodes = NULL; - payment->route_channels = NULL; - payment->bolt11 = NULL; - payment->failonion = NULL; - payment->path_secrets = tal_steal(payment, path_secrets); - - if (label != NULL) - payment->label = tal_strdup(payment, label); - else - payment->label = NULL; - - /* We write this into db when HTLC is actually sent. */ - wallet_payment_setup(ld->wallet, payment); - - add_sendpay_waiter(ld, cmd, payment_hash); - return command_still_pending(cmd); + return send_payment_core(ld, cmd, payment_hash, /* FIXME: Set partid! */0, + first_hop, AMOUNT_MSAT(0), AMOUNT_MSAT(0), + label, NULL, &packet, NULL, NULL, NULL, + path_secrets); } + static const struct json_command sendonion_command = { "sendonion", "payment", @@ -1130,7 +1097,6 @@ static struct command_result *json_sendpay(struct command *cmd, struct route_hop *route; struct amount_msat *msat; const char *b11str, *label = NULL; - struct command_result *res; struct secret *payment_secret; /* For generating help, give new-style. */ @@ -1267,15 +1233,12 @@ static struct command_result *json_sendpay(struct command *cmd, } #endif - res = send_payment(cmd->ld, cmd, rhash, /* FIXME: Set partid! */ 0, - route, - msat ? *msat : route[routetok->size-1].amount, - /* FIXME: Set total_msat! */ - msat ? *msat : route[routetok->size-1].amount, - label, b11str, payment_secret); - if (res) - return res; - return command_still_pending(cmd); + return send_payment(cmd->ld, cmd, rhash, /* FIXME: Set partid! */ 0, + route, + msat ? *msat : route[routetok->size-1].amount, + /* FIXME: Set total_msat! */ + msat ? *msat : route[routetok->size-1].amount, + label, b11str, payment_secret); } static const struct json_command sendpay_command = { From 04a46f07db6df6cfe320b227be5dd45566eb7cfe Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Thu, 12 Dec 2019 10:20:24 +1030 Subject: [PATCH 05/22] lightningd: change amount-in-flight check to be more nuanced. We currently refuse a payment if one is already in flight. For parallel payments, it's a bit more subtle: we want to refuse if it we already have the total-amount-of-invoice in flight. So we get all the current payments, and sum the pending ones. Signed-off-by: Rusty Russell --- lightningd/pay.c | 114 ++++++++++++++++++++++++++++++++++++---------- tests/test_pay.py | 4 +- 2 files changed, 92 insertions(+), 26 deletions(-) diff --git a/lightningd/pay.c b/lightningd/pay.c index 8c48599773e9..e4d83726b821 100644 --- a/lightningd/pay.c +++ b/lightningd/pay.c @@ -733,42 +733,108 @@ send_payment_core(struct lightningd *ld, struct short_channel_id *route_channels TAKES, struct secret *path_secrets) { - struct wallet_payment *payment; + const struct wallet_payment **payments, *old_payment = NULL; struct channel *channel; enum onion_type failcode; struct htlc_out *hout; struct routing_failure *fail; + struct amount_msat msat_already_pending = AMOUNT_MSAT(0); + + /* Now, do we already have one or more payments? */ + payments = wallet_payment_list(tmpctx, ld->wallet, rhash); + for (size_t i = 0; i < tal_count(payments); i++) { + log_debug(ld->log, "Payment %zu/%zu: %s %s", + i, tal_count(payments), + type_to_string(tmpctx, struct amount_msat, + &payments[i]->msatoshi), + payments[i]->status == PAYMENT_COMPLETE ? "COMPLETE" + : payments[i]->status == PAYMENT_PENDING ? "PENDING" + : "FAILED"); + + switch (payments[i]->status) { + case PAYMENT_COMPLETE: + if (payments[i]->partid != partid) + continue; - /* Now, do we already have a payment? */ - payment = wallet_payment_by_hash(tmpctx, ld->wallet, rhash, partid); - if (payment) { - /* FIXME: We should really do something smarter here! */ - if (payment->status == PAYMENT_PENDING) { - log_debug(ld->log, "send_payment: previous still in progress"); - return json_sendpay_in_progress(cmd, payment); - } - if (payment->status == PAYMENT_COMPLETE) { - log_debug(ld->log, "send_payment: previous succeeded"); /* Must match successful payment parameters. */ - if (!amount_msat_eq(payment->msatoshi, msat)) { + if (!amount_msat_eq(payments[i]->msatoshi, msat)) { return command_fail(cmd, PAY_RHASH_ALREADY_USED, "Already succeeded " "with amount %s", type_to_string(tmpctx, struct amount_msat, - &payment->msatoshi)); + &payments[i]->msatoshi)); } - if (payment->destination && destination - && !node_id_eq(payment->destination, destination)) { + if (payments[i]->destination && destination + && !node_id_eq(payments[i]->destination, + destination)) { return command_fail(cmd, PAY_RHASH_ALREADY_USED, "Already succeeded to %s", type_to_string(tmpctx, struct node_id, - payment->destination)); + payments[i]->destination)); } - return sendpay_success(cmd, payment); - } - log_debug(ld->log, "send_payment: found previous, retrying"); + return sendpay_success(cmd, payments[i]); + + case PAYMENT_PENDING: + /* Can't mix non-parallel and parallel payments! */ + if (!payments[i]->partid != !partid) { + return command_fail(cmd, PAY_IN_PROGRESS, + "Already have %s payment in progress", + payments[i]->partid ? "parallel" : "non-parallel"); + } + if (payments[i]->partid == partid) + return json_sendpay_in_progress(cmd, payments[i]); + /* You shouldn't change your mind about amount being + * sent, since we'll use it in onion! */ + else if (!amount_msat_eq(payments[i]->total_msat, + total_msat)) + return command_fail(cmd, JSONRPC2_INVALID_PARAMS, + "msatoshi was previously %s, now %s", + type_to_string(tmpctx, + struct amount_msat, + &payments[i]->total_msat), + type_to_string(tmpctx, + struct amount_msat, + &total_msat)); + + + if (!amount_msat_add(&msat_already_pending, + msat_already_pending, + payments[i]->msatoshi)) { + return command_fail(cmd, LIGHTNINGD, + "Internal amount overflow!" + " %s + %s in %zu/%zu", + type_to_string(tmpctx, + struct amount_msat, + &msat_already_pending), + type_to_string(tmpctx, + struct amount_msat, + &payments[i]->msatoshi), + i, tal_count(payments)); + } + break; + + case PAYMENT_FAILED: + if (payments[i]->partid == partid) + old_payment = payments[i]; + } + } + + /* BOLT-3a09bc54f8443c4757b47541a5310aff6377ee21 #4: + * + * - MUST NOT send another HTLC if the total `amount_msat` of the HTLC + * set is already greater or equal to `total_msat`. + */ + /* We don't do this for single 0-value payments (sendonion does this) */ + if (!amount_msat_eq(total_msat, AMOUNT_MSAT(0)) + && amount_msat_greater_eq(msat_already_pending, total_msat)) { + return command_fail(cmd, PAY_IN_PROGRESS, + "Already have %s of %s payments in progress", + type_to_string(tmpctx, struct amount_msat, + &msat_already_pending), + type_to_string(tmpctx, struct amount_msat, + &total_msat)); } channel = active_channel_by_id(ld, &first_hop->nodeid, NULL); @@ -795,7 +861,7 @@ send_payment_core(struct lightningd *ld, &first_hop->channel_id, &channel->peer->id); - return sendpay_fail(cmd, payment, PAY_TRY_OTHER_ROUTE, + return sendpay_fail(cmd, old_payment, PAY_TRY_OTHER_ROUTE, NULL, fail, "First peer not ready"); } @@ -804,14 +870,14 @@ send_payment_core(struct lightningd *ld, * a possibility, and we end up in handle_missing_htlc_output-> * onchain_failed_our_htlc->payment_failed with no payment. */ - if (payment) { - wallet_payment_delete(ld->wallet, rhash, payment->partid); + if (old_payment) { + wallet_payment_delete(ld->wallet, rhash, partid); wallet_local_htlc_out_delete(ld->wallet, channel, rhash, - payment->partid); + partid); } /* If hout fails, payment should be freed too. */ - payment = tal(hout, struct wallet_payment); + struct wallet_payment *payment = tal(hout, struct wallet_payment); payment->id = 0; payment->payment_hash = *rhash; payment->partid = partid; diff --git a/tests/test_pay.py b/tests/test_pay.py index 72332cdd8c90..cd20ed7c8cc9 100644 --- a/tests/test_pay.py +++ b/tests/test_pay.py @@ -532,12 +532,12 @@ def check_balances(): wait_for(check_balances) # Repeat will "succeed", but won't actually send anything (duplicate) - assert not l1.daemon.is_in_log('... succeeded') + assert not l1.daemon.is_in_log('Payment 0/1: .* COMPLETE') details = l1.rpc.sendpay([routestep], rhash) assert details['status'] == "complete" preimage2 = details['payment_preimage'] assert preimage == preimage2 - l1.daemon.wait_for_log('... succeeded') + l1.daemon.wait_for_log('Payment 0/1: .* COMPLETE') assert only_one(l2.rpc.listinvoices('testpayment2')['invoices'])['status'] == 'paid' assert only_one(l2.rpc.listinvoices('testpayment2')['invoices'])['msatoshi_received'] == rs['msatoshi'] From c61227b3822b5ea4efa5cf2b61c4c8f0bd93f974 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Thu, 12 Dec 2019 10:21:36 +1030 Subject: [PATCH 06/22] sendpay/sendonion: add optional partid arg, finesse msatoshi argument. msatoshi was used to indicate the amount the invoice asked for, but for parallel sendpay it's required, as it allows our sanity check of limiting the total payments in flight, ie. it becomes 'total_msat'. There's a special case for sendonion, which always tells us the value is 0. Signed-off-by: Rusty Russell --- lightningd/pay.c | 34 +++++++++++++++------------ tests/test_pay.py | 58 +++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 78 insertions(+), 14 deletions(-) diff --git a/lightningd/pay.c b/lightningd/pay.c index e4d83726b821..8043b04cb477 100644 --- a/lightningd/pay.c +++ b/lightningd/pay.c @@ -280,11 +280,11 @@ void payment_succeeded(struct lightningd *ld, struct htlc_out *hout, struct wallet_payment *payment; wallet_payment_set_status(ld->wallet, &hout->payment_hash, - /* FIXME: Set partid! */ 0, + hout->partid, PAYMENT_COMPLETE, rval); payment = wallet_payment_by_hash(tmpctx, ld->wallet, &hout->payment_hash, - /* FIXME: Set partid! */ 0); + hout->partid); assert(payment); tell_waiters_success(ld, &hout->payment_hash, payment); @@ -982,7 +982,7 @@ send_payment(struct lightningd *ld, final_tlv, route[i].amount, base_expiry + route[i].delay, - route[i].amount, payment_secret); + total_msat, payment_secret); if (!onion) { return command_fail(cmd, PAY_DESTINATION_PERM_FAIL, "Destination does not support" @@ -1094,6 +1094,7 @@ static struct command_result *json_sendonion(struct command *cmd, struct lightningd *ld = cmd->ld; const char *label; struct secret *path_secrets; + u64 *partid; if (!param(cmd, buffer, params, p_req("onion", param_bin_from_hex, &onion), @@ -1101,6 +1102,7 @@ static struct command_result *json_sendonion(struct command *cmd, p_req("payment_hash", param_sha256, &payment_hash), p_opt("label", param_escaped_string, &label), p_opt("shared_secrets", param_secrets_array, &path_secrets), + p_opt_def("partid", param_u64, &partid, 0), NULL)) return command_param_failed(); @@ -1112,7 +1114,7 @@ static struct command_result *json_sendonion(struct command *cmd, "with failcode=%d", failcode); - return send_payment_core(ld, cmd, payment_hash, /* FIXME: Set partid! */0, + return send_payment_core(ld, cmd, payment_hash, *partid, first_hop, AMOUNT_MSAT(0), AMOUNT_MSAT(0), label, NULL, &packet, NULL, NULL, NULL, path_secrets); @@ -1163,6 +1165,7 @@ static struct command_result *json_sendpay(struct command *cmd, struct route_hop *route; struct amount_msat *msat; const char *b11str, *label = NULL; + u64 *partid; struct secret *payment_secret; /* For generating help, give new-style. */ @@ -1175,6 +1178,7 @@ static struct command_result *json_sendpay(struct command *cmd, p_opt("bolt11", param_string, &b11str), p_opt("payment_secret", param_secret, &payment_secret), + p_opt_def("partid", param_u64, &partid, 0), NULL)) return command_param_failed(); } else if (params->type == JSMN_ARRAY) { @@ -1186,6 +1190,7 @@ static struct command_result *json_sendpay(struct command *cmd, p_opt("bolt11", param_string, &b11str), p_opt("payment_secret", param_secret, &payment_secret), + p_opt_def("partid", param_u64, &partid, 0), NULL)) return command_param_failed(); } else { @@ -1199,6 +1204,7 @@ static struct command_result *json_sendpay(struct command *cmd, p_opt("bolt11", param_string, &b11str), p_opt("payment_secret", param_secret, &payment_secret), + p_opt_def("partid", param_u64, &partid, 0), NULL)) return command_param_failed(); @@ -1265,20 +1271,21 @@ static struct command_result *json_sendpay(struct command *cmd, * requesting. The final hop amount is what we actually give, which can * be from the msatoshi to twice msatoshi. */ - /* if not: msatoshi <= finalhop.amount <= 2 * msatoshi, fail. */ + if (*partid && !msat) + return command_fail(cmd, JSONRPC2_INVALID_PARAMS, + "Must specify msatoshi with partid"); + + /* if not: finalhop.amount <= 2 * msatoshi, fail. */ if (msat) { struct amount_msat limit = route[routetok->size-1].amount; - if (amount_msat_less(*msat, limit)) + if (!amount_msat_add(&limit, limit, limit)) return command_fail(cmd, JSONRPC2_INVALID_PARAMS, - "msatoshi %s less than final %s", - type_to_string(tmpctx, - struct amount_msat, - msat), + "Unbelievable final amount %s", type_to_string(tmpctx, struct amount_msat, &route[routetok->size-1].amount)); - limit.millisatoshis *= 2; /* Raw: sanity check */ + if (amount_msat_greater(*msat, limit)) return command_fail(cmd, JSONRPC2_INVALID_PARAMS, "msatoshi %s more than twice final %s", @@ -1299,10 +1306,9 @@ static struct command_result *json_sendpay(struct command *cmd, } #endif - return send_payment(cmd->ld, cmd, rhash, /* FIXME: Set partid! */ 0, + return send_payment(cmd->ld, cmd, rhash, *partid, route, - msat ? *msat : route[routetok->size-1].amount, - /* FIXME: Set total_msat! */ + route[routetok->size-1].amount, msat ? *msat : route[routetok->size-1].amount, label, b11str, payment_secret); } diff --git a/tests/test_pay.py b/tests/test_pay.py index cd20ed7c8cc9..009a7cb4f711 100644 --- a/tests/test_pay.py +++ b/tests/test_pay.py @@ -2567,3 +2567,61 @@ def serialize_payload(n): except RpcError as e: assert(e.error['code'] == 204) assert(e.error['data']['raw_message'] == "400f00000000000003e80000006c") + + +@unittest.skipIf(not DEVELOPER, "needs DEVELOPER=1") +def test_partial_payment(node_factory, bitcoind, executor): + # We want to test two payments at the same time, before we send commit + l1, l2, l3, l4 = node_factory.get_nodes(4, [{}] + [{'disconnect': ['=WIRE_UPDATE_ADD_HTLC-nocommit']}] * 2 + [{}]) + + # Two routes to l4: one via l2, and one via l3. + l1.rpc.connect(l2.info['id'], 'localhost', l2.port) + l1.fund_channel(l2, 100000) + l1.rpc.connect(l3.info['id'], 'localhost', l3.port) + l1.fund_channel(l3, 100000) + l2.rpc.connect(l4.info['id'], 'localhost', l4.port) + scid24 = l2.fund_channel(l4, 100000) + l3.rpc.connect(l4.info['id'], 'localhost', l4.port) + scid34 = l3.fund_channel(l4, 100000) + bitcoind.generate_block(5) + + # Wait until l1 knows about all channels. + wait_for(lambda: len(l1.rpc.listchannels()['channels']) == 8) + + inv = l4.rpc.invoice(1000, 'inv', 'inv') + paysecret = l4.rpc.decodepay(inv['bolt11'])['payment_secret'] + + # Separate routes for each part of the payment. + r134 = l1.rpc.getroute(l4.info['id'], 500, 1, exclude=[scid24 + '/0', scid24 + '/1'])['route'] + r124 = l1.rpc.getroute(l4.info['id'], 500, 1, exclude=[scid34 + '/0', scid34 + '/1'])['route'] + + # These can happen in parallel. + l1.rpc.call('sendpay', [r134, inv['payment_hash'], None, 1000, inv['bolt11'], paysecret, 1]) + + # Can't mix non-parallel payment! + with pytest.raises(RpcError, match=r'Already have parallel payment in progress'): + l1.rpc.call('sendpay', {'route': r124, + 'payment_hash': inv['payment_hash'], + 'msatoshi': 1000, + 'payment_secret': paysecret}) + + # It will not allow a parallel with different msatoshi! + with pytest.raises(RpcError, match=r'msatoshi was previously 1000msat, now 999msat'): + l1.rpc.call('sendpay', [r124, inv['payment_hash'], None, 999, inv['bolt11'], paysecret, 2]) + + # This will work fine. + l1.rpc.call('sendpay', [r124, inv['payment_hash'], None, 1000, inv['bolt11'], paysecret, 2]) + + # Any more would exceed total payment + with pytest.raises(RpcError, match=r'Already have 1000msat of 1000msat payments in progress'): + l1.rpc.call('sendpay', [r124, inv['payment_hash'], None, 1000, inv['bolt11'], paysecret, 3]) + + # But repeat is a NOOP. + l1.rpc.call('sendpay', [r124, inv['payment_hash'], None, 1000, inv['bolt11'], paysecret, 1]) + l1.rpc.call('sendpay', [r134, inv['payment_hash'], None, 1000, inv['bolt11'], paysecret, 2]) + + # Now continue, payments will fail because receiver doesn't do MPP. + l2.rpc.dev_reenable_commit(l4.info['id']) + l3.rpc.dev_reenable_commit(l4.info['id']) + + # FIXME: waitsendpay needs a 'partid' field. From 15fa9727923cb740d2ab9fba8eefb322ec89bfd9 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Thu, 12 Dec 2019 10:22:28 +1030 Subject: [PATCH 07/22] configure: make partid payments only available with EXPERIMENTAL_FEATURES and payment_secret Explicit #if EXPERIMENTAL_FEATURES check in case we enable them at different times, but it requires a payment_secret since we put them in the same field. This incidently stops it working on legacy nodes. Signed-off-by: Rusty Russell --- lightningd/pay.c | 9 +++++++++ tests/test_pay.py | 1 + 2 files changed, 10 insertions(+) diff --git a/lightningd/pay.c b/lightningd/pay.c index 8043b04cb477..ff6f49964d32 100644 --- a/lightningd/pay.c +++ b/lightningd/pay.c @@ -978,6 +978,11 @@ send_payment(struct lightningd *ld, if (!final_tlv && payment_secret) final_tlv = true; + /* Parallel payments are invalid for legacy. */ + if (partid && !final_tlv) + return command_fail(cmd, PAY_DESTINATION_PERM_FAIL, + "Cannot do parallel payments to legacy node"); + onion = onion_final_hop(cmd, final_tlv, route[i].amount, @@ -1306,6 +1311,10 @@ static struct command_result *json_sendpay(struct command *cmd, } #endif + if (*partid && !payment_secret) + return command_fail(cmd, JSONRPC2_INVALID_PARAMS, + "partid requires payment_secret"); + return send_payment(cmd->ld, cmd, rhash, *partid, route, route[routetok->size-1].amount, diff --git a/tests/test_pay.py b/tests/test_pay.py index 009a7cb4f711..9b31982788b8 100644 --- a/tests/test_pay.py +++ b/tests/test_pay.py @@ -2570,6 +2570,7 @@ def serialize_payload(n): @unittest.skipIf(not DEVELOPER, "needs DEVELOPER=1") +@unittest.skipIf(not EXPERIMENTAL_FEATURES, "needs partid support") def test_partial_payment(node_factory, bitcoind, executor): # We want to test two payments at the same time, before we send commit l1, l2, l3, l4 = node_factory.get_nodes(4, [{}] + [{'disconnect': ['=WIRE_UPDATE_ADD_HTLC-nocommit']}] * 2 + [{}]) From 3bc463664932c82c5fa69fb68b1f5c7bfaeaabea Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Thu, 12 Dec 2019 10:22:32 +1030 Subject: [PATCH 08/22] waitsendpay: add partid arg. We need to be able to wait for a unique payment, now payment_hash is not always unique. Signed-off-by: Rusty Russell --- lightningd/pay.c | 24 +++++++++++++++++++----- tests/test_pay.py | 15 ++++++++++++--- 2 files changed, 31 insertions(+), 8 deletions(-) diff --git a/lightningd/pay.c b/lightningd/pay.c index ff6f49964d32..7e33b3d84c31 100644 --- a/lightningd/pay.c +++ b/lightningd/pay.c @@ -37,6 +37,7 @@ struct sendpay_command { struct list_node list; struct sha256 payment_hash; + u64 partid; struct command *cmd; }; @@ -50,11 +51,13 @@ static void destroy_sendpay_command(struct sendpay_command *pc) static void add_sendpay_waiter(struct lightningd *ld, struct command *cmd, - const struct sha256 *payment_hash) + const struct sha256 *payment_hash, + u64 partid) { struct sendpay_command *pc = tal(cmd, struct sendpay_command); pc->payment_hash = *payment_hash; + pc->partid = partid; pc->cmd = cmd; list_add(&ld->sendpay_commands, &pc->list); tal_add_destructor(pc, destroy_sendpay_command); @@ -65,11 +68,13 @@ add_sendpay_waiter(struct lightningd *ld, static void add_waitsendpay_waiter(struct lightningd *ld, struct command *cmd, - const struct sha256 *payment_hash) + const struct sha256 *payment_hash, + u64 partid) { struct sendpay_command *pc = tal(cmd, struct sendpay_command); pc->payment_hash = *payment_hash; + pc->partid = partid; pc->cmd = cmd; list_add(&ld->waitsendpay_commands, &pc->list); tal_add_destructor(pc, destroy_sendpay_command); @@ -252,6 +257,8 @@ static void tell_waiters_failed(struct lightningd *ld, list_for_each_safe(&ld->waitsendpay_commands, pc, next, list) { if (!sha256_eq(payment_hash, &pc->payment_hash)) continue; + if (payment->partid != pc->partid) + continue; sendpay_fail(pc->cmd, payment, pay_errcode, onionreply, fail, details); @@ -269,6 +276,8 @@ static void tell_waiters_success(struct lightningd *ld, list_for_each_safe(&ld->waitsendpay_commands, pc, next, list) { if (!sha256_eq(payment_hash, &pc->payment_hash)) continue; + if (payment->partid != pc->partid) + continue; sendpay_success(pc->cmd, payment); } @@ -623,9 +632,12 @@ static struct command_result *wait_payment(struct lightningd *ld, payment_hash)); } + log_debug(cmd->ld->log, "Payment part %"PRIu64"/%"PRIu64" status %u", + partid, payment->partid, payment->status); + switch (payment->status) { case PAYMENT_PENDING: - add_waitsendpay_waiter(ld, cmd, payment_hash); + add_waitsendpay_waiter(ld, cmd, payment_hash, partid); return NULL; case PAYMENT_COMPLETE: @@ -913,7 +925,7 @@ send_payment_core(struct lightningd *ld, /* We write this into db when HTLC is actually sent. */ wallet_payment_setup(ld->wallet, payment); - add_sendpay_waiter(ld, cmd, rhash); + add_sendpay_waiter(ld, cmd, rhash, partid); return command_still_pending(cmd); } @@ -1344,14 +1356,16 @@ static struct command_result *json_waitsendpay(struct command *cmd, struct sha256 *rhash; unsigned int *timeout; struct command_result *res; + u64 *partid; if (!param(cmd, buffer, params, p_req("payment_hash", param_sha256, &rhash), p_opt("timeout", param_number, &timeout), + p_opt_def("partid", param_u64, &partid, 0), NULL)) return command_param_failed(); - res = wait_payment(cmd->ld, cmd, rhash, /* FIXME: Set partid! */0); + res = wait_payment(cmd->ld, cmd, rhash, *partid); if (res) return res; diff --git a/tests/test_pay.py b/tests/test_pay.py index 9b31982788b8..1438acc92e2c 100644 --- a/tests/test_pay.py +++ b/tests/test_pay.py @@ -2569,11 +2569,11 @@ def serialize_payload(n): assert(e.error['data']['raw_message'] == "400f00000000000003e80000006c") -@unittest.skipIf(not DEVELOPER, "needs DEVELOPER=1") +@unittest.skipIf(not DEVELOPER, "needs dev-disconnect, dev-no-htlc-timeout") @unittest.skipIf(not EXPERIMENTAL_FEATURES, "needs partid support") def test_partial_payment(node_factory, bitcoind, executor): # We want to test two payments at the same time, before we send commit - l1, l2, l3, l4 = node_factory.get_nodes(4, [{}] + [{'disconnect': ['=WIRE_UPDATE_ADD_HTLC-nocommit']}] * 2 + [{}]) + l1, l2, l3, l4 = node_factory.get_nodes(4, [{}] + [{'disconnect': ['=WIRE_UPDATE_ADD_HTLC-nocommit'], 'dev-no-htlc-timeout': None}] * 2 + [{}]) # Two routes to l4: one via l2, and one via l3. l1.rpc.connect(l2.info['id'], 'localhost', l2.port) @@ -2621,8 +2621,17 @@ def test_partial_payment(node_factory, bitcoind, executor): l1.rpc.call('sendpay', [r124, inv['payment_hash'], None, 1000, inv['bolt11'], paysecret, 1]) l1.rpc.call('sendpay', [r134, inv['payment_hash'], None, 1000, inv['bolt11'], paysecret, 2]) + # Make sure they've done the suppress-commitment thing before we unsuppress + l2.daemon.wait_for_log(r'dev_disconnect') + l3.daemon.wait_for_log(r'dev_disconnect') + # Now continue, payments will fail because receiver doesn't do MPP. l2.rpc.dev_reenable_commit(l4.info['id']) l3.rpc.dev_reenable_commit(l4.info['id']) - # FIXME: waitsendpay needs a 'partid' field. + # See FIXME in peer_htlcs: WIRE_INVALID_ONION_PAYLOAD would be better. + with pytest.raises(RpcError, match=r'WIRE_FINAL_INCORRECT_HTLC_AMOUNT'): + l1.rpc.call('waitsendpay', [inv['payment_hash'], None, 1]) + with pytest.raises(RpcError, match=r'WIRE_FINAL_INCORRECT_HTLC_AMOUNT'): + l1.rpc.call('waitsendpay', {'payment_hash': inv['payment_hash'], + 'partid': 2}) From 94d3897d34a0eb1ff838a721a5d07781d41cc9ca Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Thu, 12 Dec 2019 10:22:34 +1030 Subject: [PATCH 09/22] pytest: Add tests to make sure received onion is as expected. Signed-off-by: Rusty Russell --- tests/test_pay.py | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/tests/test_pay.py b/tests/test_pay.py index 1438acc92e2c..f83e433188d7 100644 --- a/tests/test_pay.py +++ b/tests/test_pay.py @@ -2573,7 +2573,7 @@ def serialize_payload(n): @unittest.skipIf(not EXPERIMENTAL_FEATURES, "needs partid support") def test_partial_payment(node_factory, bitcoind, executor): # We want to test two payments at the same time, before we send commit - l1, l2, l3, l4 = node_factory.get_nodes(4, [{}] + [{'disconnect': ['=WIRE_UPDATE_ADD_HTLC-nocommit'], 'dev-no-htlc-timeout': None}] * 2 + [{}]) + l1, l2, l3, l4 = node_factory.get_nodes(4, [{}] + [{'disconnect': ['=WIRE_UPDATE_ADD_HTLC-nocommit'], 'dev-no-htlc-timeout': None}] * 2 + [{'plugin': os.path.join(os.getcwd(), 'tests/plugins/print_htlc_onion.py')}]) # Two routes to l4: one via l2, and one via l3. l1.rpc.connect(l2.info['id'], 'localhost', l2.port) @@ -2635,3 +2635,10 @@ def test_partial_payment(node_factory, bitcoind, executor): with pytest.raises(RpcError, match=r'WIRE_FINAL_INCORRECT_HTLC_AMOUNT'): l1.rpc.call('waitsendpay', {'payment_hash': inv['payment_hash'], 'partid': 2}) + + for i in range(2): + line = l4.daemon.wait_for_log('print_htlc_onion.py: Got onion') + assert "'type': 'tlv'" in line + assert "'forward_amount': '500msat'" in line + assert "'total_msat': '1000msat'" in line + assert "'payment_secret': '{}'".format(paysecret) in line From 2e4416ef2bf6dfee635ac3354dddb0a128ecbd66 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Thu, 12 Dec 2019 10:24:45 +1030 Subject: [PATCH 10/22] doc: update experimental bolt version quotes. Signed-off-by: Rusty Russell --- common/bolt11.c | 4 ++-- common/features.h | 2 +- common/onion.c | 4 ++-- lightningd/invoice.c | 7 +------ lightningd/pay.c | 4 ++-- lightningd/peer_htlcs.c | 2 +- 6 files changed, 9 insertions(+), 14 deletions(-) diff --git a/common/bolt11.c b/common/bolt11.c index d431659dcb62..e892a96117f1 100644 --- a/common/bolt11.c +++ b/common/bolt11.c @@ -302,7 +302,7 @@ static char *decode_n(struct bolt11 *b11, return NULL; } -/* BOLT-412c260b537be95b105ae2062463f1d992024ca7 #11: +/* BOLT-9441a66faad63edc8cd89860b22fbf24a86f0dcd #11: * * * `s` (16): `data_length` 52. This 256-bit secret prevents * forwarding nodes from probing the payment recipient. @@ -317,7 +317,7 @@ static char *decode_s(struct bolt11 *b11, return unknown_field(b11, hu5, data, data_len, 's', data_length); - /* BOLT-412c260b537be95b105ae2062463f1d992024ca7 #11: + /* BOLT-9441a66faad63edc8cd89860b22fbf24a86f0dcd #11: * * A reader... MUST skip over unknown fields, OR an `f` field * with unknown `version`, OR `p`, `h`, `s` or `n` fields that do diff --git a/common/features.h b/common/features.h index cb81503505a1..987f7e1b42cf 100644 --- a/common/features.h +++ b/common/features.h @@ -57,7 +57,7 @@ void set_feature_bit(u8 **ptr, u32 bit); #define OPT_GOSSIP_QUERIES_EX 10 #define OPT_STATIC_REMOTEKEY 12 -/* BOLT-3a09bc54f8443c4757b47541a5310aff6377ee21 #9: +/* BOLT-9441a66faad63edc8cd89860b22fbf24a86f0dcd #9: * * | 14/15 | `payment_secret` |... IN9 ... * | 16/17 | `basic_mpp` |... IN9 ... diff --git a/common/onion.c b/common/onion.c index 1c1fe02c3608..82865953fc52 100644 --- a/common/onion.c +++ b/common/onion.c @@ -231,7 +231,7 @@ struct onion_payload *onion_decode(const tal_t *ctx, if (rs->nextcase == ONION_FORWARD) { p->total_msat = NULL; } else { - /* BOLT-e36f7b6517e1173dcbd49da3b516cfe1f48ae556 #4: + /* BOLT-9441a66faad63edc8cd89860b22fbf24a86f0dcd #4: * - if it is the final node: * - MUST treat `total_msat` as if it were equal to * `amt_to_forward` if it is not present. */ @@ -279,7 +279,7 @@ struct onion_payload *onion_decode(const tal_t *ctx, p->total_msat = NULL; } else { p->forward_channel = NULL; - /* BOLT-e36f7b6517e1173dcbd49da3b516cfe1f48ae556 #4: + /* BOLT-9441a66faad63edc8cd89860b22fbf24a86f0dcd #4: * - if it is the final node: * - MUST treat `total_msat` as if it were equal to * `amt_to_forward` if it is not present. */ diff --git a/lightningd/invoice.c b/lightningd/invoice.c index 4965aed1148b..ac6fc4e5e5ec 100644 --- a/lightningd/invoice.c +++ b/lightningd/invoice.c @@ -252,7 +252,7 @@ void invoice_try_pay(struct lightningd *ld, log_debug(ld->log, "payment_secret is %s", payment_secret ? "set": "NULL"); - /* BOLT-e36f7b6517e1173dcbd49da3b516cfe1f48ae556 #4: + /* BOLT-9441a66faad63edc8cd89860b22fbf24a86f0dcd #4: * * - if the `payment_secret` doesn't match the expected value for that * `payment_hash`, or the `payment_secret` is required and is not @@ -260,11 +260,6 @@ void invoice_try_pay(struct lightningd *ld, * - MUST fail the HTLC. * - MUST return an `incorrect_or_unknown_payment_details` error. */ - /* BOLT-e36f7b6517e1173dcbd49da3b516cfe1f48ae556 #1: - * - * - if `payment_secret` is required in the onion: - * - MUST set the even feature `var_onion_optin`. - */ if (feature_is_set(details->features, COMPULSORY_FEATURE(OPT_VAR_ONION)) && !payment_secret) { fail_htlc(hin, WIRE_INCORRECT_OR_UNKNOWN_PAYMENT_DETAILS); diff --git a/lightningd/pay.c b/lightningd/pay.c index 7e33b3d84c31..7d20aecf8eb9 100644 --- a/lightningd/pay.c +++ b/lightningd/pay.c @@ -833,7 +833,7 @@ send_payment_core(struct lightningd *ld, } } - /* BOLT-3a09bc54f8443c4757b47541a5310aff6377ee21 #4: + /* BOLT-9441a66faad63edc8cd89860b22fbf24a86f0dcd #4: * * - MUST NOT send another HTLC if the total `amount_msat` of the HTLC * set is already greater or equal to `total_msat`. @@ -979,7 +979,7 @@ send_payment(struct lightningd *ld, assert(ret); final_tlv = should_use_tlv(route[i].style); - /* BOLT-3a09bc54f8443c4757b47541a5310aff6377ee21 #4: + /* BOLT-9441a66faad63edc8cd89860b22fbf24a86f0dcd #4: * - Unless `node_announcement`, `init` message or the * [BOLT #11](11-payment-encoding.md#tagged-fields) offers feature * `var_onion_optin`: diff --git a/lightningd/peer_htlcs.c b/lightningd/peer_htlcs.c index 77370da4a724..dd3807b6fe84 100644 --- a/lightningd/peer_htlcs.c +++ b/lightningd/peer_htlcs.c @@ -301,7 +301,7 @@ static void handle_localpay(struct htlc_in *hin, goto fail; } - /* BOLT- #4: + /* BOLT-9441a66faad63edc8cd89860b22fbf24a86f0dcd #4: * - if it does not support `basic_mpp`: * - MUST fail the HTLC if `total_msat` is not exactly equal to * `amt_to_forward`. From d94ae31f3244d1939f1f4f8a317d730012d82703 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Thu, 12 Dec 2019 10:25:39 +1030 Subject: [PATCH 11/22] lightningd: cleanup redundant args from handle_localpay The cltv_expiry and payment_hash are in hin, so no need to hand them in here. Signed-off-by: Rusty Russell --- lightningd/peer_htlcs.c | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/lightningd/peer_htlcs.c b/lightningd/peer_htlcs.c index dd3807b6fe84..9bd2c4a9d805 100644 --- a/lightningd/peer_htlcs.c +++ b/lightningd/peer_htlcs.c @@ -278,8 +278,6 @@ void fulfill_htlc(struct htlc_in *hin, const struct preimage *preimage) } static void handle_localpay(struct htlc_in *hin, - u32 cltv_expiry, - const struct sha256 *payment_hash, struct amount_msat amt_to_forward, u32 outgoing_cltv_value, struct amount_msat total_msat, @@ -321,7 +319,7 @@ static void handle_localpay(struct htlc_in *hin, * * The CLTV expiry in the HTLC doesn't match the value in the onion. */ - if (!check_cltv(hin, cltv_expiry, outgoing_cltv_value, 0)) { + if (!check_cltv(hin, hin->cltv_expiry, outgoing_cltv_value, 0)) { failcode = WIRE_FINAL_INCORRECT_CLTV_EXPIRY; goto fail; } @@ -333,17 +331,17 @@ static void handle_localpay(struct htlc_in *hin, * - MUST return an `incorrect_or_unknown_payment_details` error. */ if (get_block_height(ld->topology) + ld->config.cltv_final - > cltv_expiry) { + > hin->cltv_expiry) { log_debug(hin->key.channel->log, "Expiry cltv too soon %u < %u + %u", - cltv_expiry, + hin->cltv_expiry, get_block_height(ld->topology), ld->config.cltv_final); failcode = WIRE_INCORRECT_OR_UNKNOWN_PAYMENT_DETAILS; goto fail; } - invoice_try_pay(ld, hin, payment_hash, amt_to_forward, payment_secret); + invoice_try_pay(ld, hin, &hin->payment_hash, amt_to_forward, payment_secret); return; fail: @@ -842,7 +840,7 @@ htlc_accepted_hook_callback(struct htlc_accepted_hook_payload *request, subd_req(hin, ld->gossip, req, -1, 0, channel_resolve_reply, gr); } else - handle_localpay(hin, hin->cltv_expiry, &hin->payment_hash, + handle_localpay(hin, request->payload->amt_to_forward, request->payload->outgoing_cltv, *request->payload->total_msat, From 3c6e33ab8b4e74b01f58f0c9d050aaf229e5bdf9 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Thu, 12 Dec 2019 10:25:45 +1030 Subject: [PATCH 12/22] lightningd: split invoice check into separate function. We now return the same error for various "does not match this invoice", so it makes sense to encapsulate these checks. We'll also want to expose this for multi-part payments. Signed-off-by: Rusty Russell --- lightningd/invoice.c | 95 ++++++++++++++++++++++++-------------------- lightningd/invoice.h | 18 +++++++++ 2 files changed, 69 insertions(+), 44 deletions(-) diff --git a/lightningd/invoice.c b/lightningd/invoice.c index ac6fc4e5e5ec..5c13212e26a6 100644 --- a/lightningd/invoice.c +++ b/lightningd/invoice.c @@ -233,51 +233,46 @@ REGISTER_PLUGIN_HOOK(invoice_payment, invoice_payment_serialize, struct invoice_payment_hook_payload *); -void invoice_try_pay(struct lightningd *ld, - struct htlc_in *hin, - const struct sha256 *payment_hash, - const struct amount_msat msat, - const struct secret *payment_secret) +const struct invoice_details * +invoice_check_payment(const tal_t *ctx, + struct lightningd *ld, + const struct sha256 *payment_hash, + const struct amount_msat msat, + const struct secret *payment_secret) { struct invoice invoice; const struct invoice_details *details; - struct invoice_payment_hook_payload *payload; - if (!wallet_invoice_find_unpaid(ld->wallet, &invoice, payment_hash)) { - fail_htlc(hin, WIRE_INCORRECT_OR_UNKNOWN_PAYMENT_DETAILS); - return; - } - details = wallet_invoice_details(tmpctx, ld->wallet, invoice); + /* BOLT #4: + * - if the payment hash has already been paid: + * - MAY treat the payment hash as unknown. + * - MAY succeed in accepting the HTLC. + *... + * - if the payment hash is unknown: + * - MUST fail the HTLC. + * - MUST return an `incorrect_or_unknown_payment_details` error. + */ + if (!wallet_invoice_find_unpaid(ld->wallet, &invoice, payment_hash)) + return NULL; - log_debug(ld->log, "payment_secret is %s", - payment_secret ? "set": "NULL"); + details = wallet_invoice_details(ctx, ld->wallet, invoice); /* BOLT-9441a66faad63edc8cd89860b22fbf24a86f0dcd #4: - * - * - if the `payment_secret` doesn't match the expected value for that - * `payment_hash`, or the `payment_secret` is required and is not - * present: - * - MUST fail the HTLC. - * - MUST return an `incorrect_or_unknown_payment_details` error. + * - if the `payment_secret` doesn't match the expected value for that + * `payment_hash`, or the `payment_secret` is required and is not + * present: + * - MUST fail the HTLC. */ if (feature_is_set(details->features, COMPULSORY_FEATURE(OPT_VAR_ONION)) - && !payment_secret) { - fail_htlc(hin, WIRE_INCORRECT_OR_UNKNOWN_PAYMENT_DETAILS); - return; - } + && !payment_secret) + return tal_free(details); if (payment_secret) { struct secret expected; invoice_secret(&details->r, &expected); - log_debug(ld->log, "payment_secret %s vs %s", - type_to_string(tmpctx, struct secret, payment_secret), - type_to_string(tmpctx, struct secret, &expected)); - if (!secret_eq_consttime(payment_secret, &expected)) { - fail_htlc(hin, - WIRE_INCORRECT_OR_UNKNOWN_PAYMENT_DETAILS); - return; - } + if (!secret_eq_consttime(payment_secret, &expected)) + return tal_free(details); } /* BOLT #4: @@ -290,26 +285,39 @@ void invoice_try_pay(struct lightningd *ld, if (details->msat != NULL) { struct amount_msat twice; - if (amount_msat_less(msat, *details->msat)) { - fail_htlc(hin, - WIRE_INCORRECT_OR_UNKNOWN_PAYMENT_DETAILS); - return; - } + if (amount_msat_less(msat, *details->msat)) + return tal_free(details); if (amount_msat_add(&twice, *details->msat, *details->msat) && amount_msat_greater(msat, twice)) { - /* FIXME: bolt update fixes this quote! */ /* BOLT #4: * - * - if the amount paid is more than twice the amount expected: - * - SHOULD fail the HTLC. - * - SHOULD return an `incorrect_or_unknown_payment_details` error. + * - if the amount paid is more than twice the amount + * expected: + * - SHOULD fail the HTLC. */ - fail_htlc(hin, - WIRE_INCORRECT_OR_UNKNOWN_PAYMENT_DETAILS); - return; + return tal_free(details); } } + return details; +} + +void invoice_try_pay(struct lightningd *ld, + struct htlc_in *hin, + const struct sha256 *payment_hash, + const struct amount_msat msat, + const struct secret *payment_secret) +{ + const struct invoice_details *details; + struct invoice_payment_hook_payload *payload; + + details = invoice_check_payment(tmpctx, ld, payment_hash, msat, + payment_secret); + + if (!details) { + fail_htlc(hin, WIRE_INCORRECT_OR_UNKNOWN_PAYMENT_DETAILS); + return; + } payload = tal(ld, struct invoice_payment_hook_payload); payload->ld = ld; @@ -319,7 +327,6 @@ void invoice_try_pay(struct lightningd *ld, payload->hin = hin; tal_add_destructor2(hin, invoice_payload_remove_hin, payload); - log_debug(ld->log, "Calling hook for invoice '%s'", details->label->s); plugin_hook_call_invoice_payment(ld, payload, payload); } diff --git a/lightningd/invoice.h b/lightningd/invoice.h index 954c8c36ef50..199ab4b902e9 100644 --- a/lightningd/invoice.h +++ b/lightningd/invoice.h @@ -8,6 +8,24 @@ struct htlc_in; struct lightningd; struct sha256; + +/** + * invoice_check_payment - check if this payment would be valid + * @ctx: tal context to allocate return off + * @ld: lightningd + * @payment_hash: hash of preimage they want. + * @msat: amount they offer to pay. + * @payment_secret: they payment secret they sent, if any. + * + * Returns NULL if there's a problem, otherwise returns the invoice details. + */ +const struct invoice_details * +invoice_check_payment(const tal_t *ctx, + struct lightningd *ld, + const struct sha256 *payment_hash, + const struct amount_msat msat, + const struct secret *payment_secret); + /** * invoice_try_pay - process payment for this payment_hash, amount msat. * @ld: lightningd From 555b217f42c2f52b6ff5a452084ebd268c18d04b Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Thu, 12 Dec 2019 10:25:45 +1030 Subject: [PATCH 13/22] lightningd: implement htlc sets. This isn't plumbed in yet, but the idea is that every htlc gets put into a "set" and then we process them once the set is satisfied. For the !EXPERIMENTAL_FEATURES, the set is simply always size 1. Signed-off-by: Rusty Russell --- lightningd/Makefile | 1 + lightningd/htlc_set.c | 190 ++++++++++++++++++++++++++++++++++++++++ lightningd/htlc_set.h | 55 ++++++++++++ lightningd/lightningd.c | 6 ++ lightningd/lightningd.h | 6 ++ 5 files changed, 258 insertions(+) create mode 100644 lightningd/htlc_set.c create mode 100644 lightningd/htlc_set.h diff --git a/lightningd/Makefile b/lightningd/Makefile index 11686eae920a..0f74f5af8d99 100644 --- a/lightningd/Makefile +++ b/lightningd/Makefile @@ -75,6 +75,7 @@ LIGHTNINGD_SRC := \ lightningd/gossip_msg.c \ lightningd/hsm_control.c \ lightningd/htlc_end.c \ + lightningd/htlc_set.c \ lightningd/invoice.c \ lightningd/io_loop_with_timers.c \ lightningd/json.c \ diff --git a/lightningd/htlc_set.c b/lightningd/htlc_set.c new file mode 100644 index 000000000000..fc59c64a5d67 --- /dev/null +++ b/lightningd/htlc_set.c @@ -0,0 +1,190 @@ +#include +#include +#include +#include +#include +#include + +#if EXPERIMENTAL_FEATURES +/* If an HTLC times out, we need to free entire set, since we could be processing + * it in invoice.c right now. */ +static void htlc_set_hin_destroyed(struct htlc_in *hin, + struct htlc_set *set) +{ + for (size_t i = 0; i < tal_count(set->htlcs); i++) { + if (set->htlcs[i] == hin) { + /* Don't try to re-fail this HTLC! */ + tal_arr_remove(&set->htlcs, i); + /* Kind of the correct failure code. */ + htlc_set_fail(set, WIRE_MPP_TIMEOUT); + return; + } + } + abort(); +} + +static void destroy_htlc_set(struct htlc_set *set, + struct htlc_set_map *map) +{ + htlc_set_map_del(map, set); +} + +/* BOLT-9441a66faad63edc8cd89860b22fbf24a86f0dcd #4: + * - MUST fail all HTLCs in the HTLC set after some reasonable + * timeout. + * - SHOULD use `mpp_timeout` for the failure message. + */ +static void timeout_htlc_set(struct htlc_set *set) +{ + htlc_set_fail(set, WIRE_MPP_TIMEOUT); +} +#endif /* EXPERIMENTAL_FEATURES */ + +void htlc_set_fail(struct htlc_set *set, enum onion_type failcode) +{ + for (size_t i = 0; i < tal_count(set->htlcs); i++) { +#if EXPERIMENTAL_FEATURES + /* Don't remove from set */ + tal_del_destructor2(set->htlcs[i], htlc_set_hin_destroyed, set); +#endif + fail_htlc(set->htlcs[i], failcode); + } + tal_free(set); +} + +void htlc_set_fulfill(struct htlc_set *set, const struct preimage *preimage) +{ + for (size_t i = 0; i < tal_count(set->htlcs); i++) { +#if EXPERIMENTAL_FEATURES + /* Don't remove from set */ + tal_del_destructor2(set->htlcs[i], htlc_set_hin_destroyed, set); +#endif + fulfill_htlc(set->htlcs[i], preimage); + } + tal_free(set); +} + +static struct htlc_set *new_htlc_set(struct lightningd *ld, + struct htlc_in *hin, + struct amount_msat total_msat) +{ + struct htlc_set *set; + + set = tal(ld, struct htlc_set); + set->total_msat = total_msat; + set->payment_hash = hin->payment_hash; + set->so_far = AMOUNT_MSAT(0); + set->htlcs = tal_arr(set, struct htlc_in *, 1); + set->htlcs[0] = hin; + +#if EXPERIMENTAL_FEATURES + /* BOLT-9441a66faad63edc8cd89860b22fbf24a86f0dcd #4: + * - MUST fail all HTLCs in the HTLC set after some reasonable + * timeout. + * - SHOULD wait for at least 60 seconds after the initial + * HTLC. + */ + notleak(new_reltimer(ld->timers, set, time_from_sec(70), + timeout_htlc_set, set)); + htlc_set_map_add(&ld->htlc_sets, set); + tal_add_destructor2(set, destroy_htlc_set, &ld->htlc_sets); +#endif + return set; +} + +void htlc_set_add(struct lightningd *ld, + struct htlc_in *hin, + struct amount_msat total_msat, + const struct secret *payment_secret) +{ + struct htlc_set *set; + const struct invoice_details *details; + + /* BOLT-9441a66faad63edc8cd89860b22fbf24a86f0dcd #4: + * The final node: + * - MUST fail the HTLC if dictated by Requirements under + * [Failure Messages](#failure-messages) + * - Note: "amount paid" specified there is the `total_msat` field. + */ + details = invoice_check_payment(tmpctx, ld, &hin->payment_hash, + total_msat, payment_secret); + if (!details) { + fail_htlc(hin, WIRE_INCORRECT_OR_UNKNOWN_PAYMENT_DETAILS); + return; + } + +#if !EXPERIMENTAL_FEATURES + /* BOLT-9441a66faad63edc8cd89860b22fbf24a86f0dcd #4: + * - if it does not support `basic_mpp`: + * - MUST fail the HTLC if `total_msat` is not exactly equal to + * `amt_to_forward`. + */ + if (!amount_msat_eq(hin->msat, total_msat)) { + fail_htlc(hin, WIRE_FINAL_INCORRECT_HTLC_AMOUNT); + return; + } + + /* We create a transient set which just has one entry. */ + set = new_htlc_set(ld, hin, total_msat); +#else + /* BOLT-9441a66faad63edc8cd89860b22fbf24a86f0dcd #4: + * - otherwise, if it supports `basic_mpp`: + * - MUST add it to the HTLC set corresponding to that `payment_hash`. + * - if the total `amount_msat` of this HTLC set equals `total_msat`: + * - SHOULD fulfill all HTLCs in the HTLC set + */ + set = htlc_set_map_get(&ld->htlc_sets, &hin->payment_hash); + if (!set) + set = new_htlc_set(ld, hin, total_msat); + else + tal_arr_expand(&set->htlcs, hin); + + /* Remove from set should hin get destroyed somehow */ + tal_add_destructor2(hin, htlc_set_hin_destroyed, set); + + /* BOLT-9441a66faad63edc8cd89860b22fbf24a86f0dcd #4: + * - SHOULD fail the entire HTLC set if `total_msat` is not + * the same for all HTLCs in the set. + */ + if (!amount_msat_eq(total_msat, set->total_msat)) { + log_unusual(ld->log, "Failing HTLC set %s:" + " total_msat %s new htlc total %s", + type_to_string(tmpctx, struct sha256, + &set->payment_hash), + type_to_string(tmpctx, struct amount_msat, + &set->total_msat), + type_to_string(tmpctx, struct amount_msat, + &total_msat)); + htlc_set_fail(set, WIRE_FINAL_INCORRECT_HTLC_AMOUNT); + return; + } +#endif /* EXPERIMENTAL_FEATURES */ + + /* BOLT-9441a66faad63edc8cd89860b22fbf24a86f0dcd #4: + * - if the total `amount_msat` of this HTLC set equals `total_msat`: + * - SHOULD fulfill all HTLCs in the HTLC set + */ + if (!amount_msat_add(&set->so_far, set->so_far, hin->msat)) { + log_unusual(ld->log, "Failing HTLC set %s:" + " overflow adding %s+%s", + type_to_string(tmpctx, struct sha256, + &set->payment_hash), + type_to_string(tmpctx, struct amount_msat, + &set->so_far), + type_to_string(tmpctx, struct amount_msat, + &hin->msat)); + htlc_set_fail(set, WIRE_FINAL_INCORRECT_HTLC_AMOUNT); + return; + } + + if (amount_msat_eq(set->so_far, total_msat)) { + /* FIXME: hand to invoice_try_pay! */ + tal_free(set); + return; + } + + /* BOLT-9441a66faad63edc8cd89860b22fbf24a86f0dcd #4: + * - otherwise, if the total `amount_msat` of this HTLC set is less than + * `total_msat`: + * - MUST NOT fulfill any HTLCs in the HTLC set */ +} diff --git a/lightningd/htlc_set.h b/lightningd/htlc_set.h new file mode 100644 index 000000000000..11e57adcbb3c --- /dev/null +++ b/lightningd/htlc_set.h @@ -0,0 +1,55 @@ +#ifndef LIGHTNING_LIGHTNINGD_HTLC_SET_H +#define LIGHTNING_LIGHTNINGD_HTLC_SET_H +#include "config.h" +#include +#include +#include +#include +#include +#include +#include + +struct htlc_in; +struct lightningd; + +/* Set of incoming HTLCs for multi-part-payments */ +struct htlc_set { + struct amount_msat total_msat, so_far; + struct sha256 payment_hash; + struct htlc_in **htlcs; +}; + +static inline const struct sha256 *keyof_htlc_set(const struct htlc_set *set) +{ + return &set->payment_hash; +} + +static inline size_t hash_payment_hash(const struct sha256 *payment_hash) +{ + return siphash24(siphash_seed(), payment_hash, sizeof(&payment_hash)); +} + +static inline bool htlc_set_eq(const struct htlc_set *set, + const struct sha256 *payment_hash) +{ + return sha256_eq(payment_hash, &set->payment_hash); +} + +HTABLE_DEFINE_TYPE(struct htlc_set, + keyof_htlc_set, + hash_payment_hash, + htlc_set_eq, + htlc_set_map); + +/* Handles hin: if it completes a set, hands that to invoice_try_pay */ +void htlc_set_add(struct lightningd *ld, + struct htlc_in *hin, + struct amount_msat total_msat, + const struct secret *payment_secret); + +/* Fail every htlc in the set: frees set */ +void htlc_set_fail(struct htlc_set *set, enum onion_type failcode); + +/* Fulfill every htlc in the set: frees set */ +void htlc_set_fulfill(struct htlc_set *set, const struct preimage *preimage); +#endif /* LIGHTNING_LIGHTNINGD_HTLC_SET_H */ diff --git a/lightningd/lightningd.c b/lightningd/lightningd.c index 876cc98341f4..156de206aa2f 100644 --- a/lightningd/lightningd.c +++ b/lightningd/lightningd.c @@ -161,6 +161,12 @@ static struct lightningd *new_lightningd(const tal_t *ctx) htlc_in_map_init(&ld->htlcs_in); htlc_out_map_init(&ld->htlcs_out); +#if EXPERIMENTAL_FEATURES + /*~ For multi-part payments, we need to keep some incoming payments + * in limbo until we get all the parts, or we time them out. */ + htlc_set_map_init(&ld->htlc_sets); +#endif /* EXPERIMENTAL_FEATURES */ + /*~ We have a multi-entry log-book infrastructure: we define a 100MB log * book to hold all the entries (and trims as necessary), and multiple * log objects which each can write into it, each with a unique diff --git a/lightningd/lightningd.h b/lightningd/lightningd.h index 7e1bfc85d41a..d8a8b172d9be 100644 --- a/lightningd/lightningd.h +++ b/lightningd/lightningd.h @@ -7,6 +7,7 @@ #include #include #include +#include #include #include #include @@ -162,6 +163,11 @@ struct lightningd { struct htlc_in_map htlcs_in; struct htlc_out_map htlcs_out; +#if EXPERIMENTAL_FEATURES + /* Sets of HTLCs we are holding onto for MPP. */ + struct htlc_set_map htlc_sets; +#endif + struct wallet *wallet; /* Outstanding waitsendpay commands. */ From 73bf9e0b194cf811cd7bdb3ec6cd608ef48100f5 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Thu, 12 Dec 2019 10:25:45 +1030 Subject: [PATCH 14/22] lightningd: wrap htlc replay in a database transaction. AFAICT this only "worked" previously because replay htlc simply failed them all (no peers are currently connected). With upcoming changes (foreshadowed by the comment) this is no longer true: Attempting to prepare a db_stmt outside of a transaction: wallet/invoices.c:373 lightningd: FATAL SIGNAL 6 (version v0.7.3-188-g45b0af4-modded) 0x55b475590a73 send_backtrace common/daemon.c:41 0x55b475590b1d crashdump common/daemon.c:54 0x7f16c557b46f ??? ???:0 0x7f16c557b3eb ??? ???:0 0x7f16c555a898 ??? ???:0 0x55b475564c8f fatal lightningd/log.c:814 0x55b4755c3ed5 db_prepare_v2_ wallet/db.c:605 0x55b4755c76b5 invoices_find_unpaid wallet/invoices.c:373 0x55b4755ce91c wallet_invoice_find_unpaid wallet/wallet.c:1990 0x55b47555861f invoice_check_payment lightningd/invoice.c:257 0x55b475557a7c htlc_add_set lightningd/htlc_set.c:112 0x55b47557b294 handle_localpay lightningd/peer_htlcs.c:332 0x55b47557c63c htlc_accepted_hook_callback lightningd/peer_htlcs.c:857 0x55b475585573 plugin_hook_call_ lightningd/plugin_hook.c:118 0x55b47557c747 plugin_hook_call_htlc_accepted lightningd/peer_htlcs.c:882 0x55b47557ca3e peer_accepted_htlc lightningd/peer_htlcs.c:991 0x55b47557ffb9 htlcs_resubmit lightningd/peer_htlcs.c:2131 0x55b4755620f7 main lightningd/lightningd.c:801 Signed-off-by: Rusty Russell --- lightningd/lightningd.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/lightningd/lightningd.c b/lightningd/lightningd.c index 156de206aa2f..2aa02f35122b 100644 --- a/lightningd/lightningd.c +++ b/lightningd/lightningd.c @@ -797,8 +797,11 @@ int main(int argc, char *argv[]) /*~ Process any HTLCs we were in the middle of when we exited, now * that plugins (who might want to know via htlc_accepted hook) are - * active. */ + * active. These will immediately fail, since no peers are connected, + * however partial payments may still be absorbed into htlc_set. */ + db_begin_transaction(ld->wallet->db); htlcs_resubmit(ld, unconnected_htlcs_in); + db_commit_transaction(ld->wallet->db); /*~ Activate connect daemon. Needs to be after the initialization of * chaintopology, otherwise peers may connect and ask for From 183948333f4b72414001151acd6011c63cebe073 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Thu, 12 Dec 2019 10:25:45 +1030 Subject: [PATCH 15/22] lightningd: sew in htlc set. The invoice_try_pay code now takes a set, rather than a single htlc, but it's basically the same thing. Signed-off-by: Rusty Russell --- lightningd/htlc_set.c | 3 +- lightningd/invoice.c | 49 +++++++++------------ lightningd/invoice.h | 18 +++----- lightningd/peer_htlcs.c | 15 +------ lightningd/test/run-invoice-select-inchan.c | 12 ++--- tests/test_pay.py | 13 +++--- wallet/test/run-wallet.c | 13 +++--- 7 files changed, 48 insertions(+), 75 deletions(-) diff --git a/lightningd/htlc_set.c b/lightningd/htlc_set.c index fc59c64a5d67..fa5b1d032160 100644 --- a/lightningd/htlc_set.c +++ b/lightningd/htlc_set.c @@ -178,8 +178,7 @@ void htlc_set_add(struct lightningd *ld, } if (amount_msat_eq(set->so_far, total_msat)) { - /* FIXME: hand to invoice_try_pay! */ - tal_free(set); + invoice_try_pay(ld, set, details); return; } diff --git a/lightningd/invoice.c b/lightningd/invoice.c index 5c13212e26a6..114900379f23 100644 --- a/lightningd/invoice.c +++ b/lightningd/invoice.c @@ -123,7 +123,7 @@ static void invoice_secret(const struct preimage *payment_preimage, struct invoice_payment_hook_payload { struct lightningd *ld; /* Set to NULL if it is deleted while waiting for plugin */ - struct htlc_in *hin; + struct htlc_set *set; /* What invoice it's trying to pay. */ const struct json_escape *label; /* Amount it's offering. */ @@ -146,12 +146,13 @@ invoice_payment_serialize(struct invoice_payment_hook_payload *payload, json_object_end(stream); /* .payment */ } -/* Peer dies? Remove hin ptr from payload so we know to ignore plugin return */ -static void invoice_payload_remove_hin(struct htlc_in *hin, +/* Set times out or HTLC deleted? Remove set ptr from payload so we + * know to ignore plugin return */ +static void invoice_payload_remove_set(struct htlc_set *set, struct invoice_payment_hook_payload *payload) { - assert(payload->hin == hin); - payload->hin = NULL; + assert(payload->set == set); + payload->set = NULL; } static bool hook_gives_failcode(const char *buffer, @@ -195,12 +196,12 @@ invoice_payment_hook_cb(struct invoice_payment_hook_payload *payload, * called even if the hook is not registered. */ notify_invoice_payment(ld, payload->msat, payload->preimage, payload->label); - tal_del_destructor2(payload->hin, invoice_payload_remove_hin, payload); + tal_del_destructor2(payload->set, invoice_payload_remove_set, payload); /* We want to free this, whatever happens. */ tal_steal(tmpctx, payload); /* If peer dies or something, this can happen. */ - if (!payload->hin) { + if (!payload->set) { log_debug(ld->log, "invoice '%s' paying htlc_in has gone!", payload->label->s); return; @@ -210,21 +211,22 @@ invoice_payment_hook_cb(struct invoice_payment_hook_payload *payload, * we can also fail */ if (!wallet_invoice_find_by_label(ld->wallet, &invoice, payload->label)) { failcode = WIRE_INCORRECT_OR_UNKNOWN_PAYMENT_DETAILS; - fail_htlc(payload->hin, failcode); + htlc_set_fail(payload->set, failcode); return; } /* Did we have a hook result? */ if (hook_gives_failcode(buffer, toks, &failcode)) { - fail_htlc(payload->hin, failcode); + htlc_set_fail(payload->set, failcode); return; } - log_info(ld->log, "Resolved invoice '%s' with amount %s", - payload->label->s, - type_to_string(tmpctx, struct amount_msat, &payload->msat)); + log_info(ld->log, "Resolved invoice '%s' with amount %s in %zu htlcs", + payload->label->s, + type_to_string(tmpctx, struct amount_msat, &payload->msat), + tal_count(payload->set->htlcs)); wallet_invoice_resolve(ld->wallet, invoice, payload->msat); - fulfill_htlc(payload->hin, &payload->preimage); + htlc_set_fulfill(payload->set, &payload->preimage); } REGISTER_PLUGIN_HOOK(invoice_payment, @@ -303,29 +305,18 @@ invoice_check_payment(const tal_t *ctx, } void invoice_try_pay(struct lightningd *ld, - struct htlc_in *hin, - const struct sha256 *payment_hash, - const struct amount_msat msat, - const struct secret *payment_secret) + struct htlc_set *set, + const struct invoice_details *details) { - const struct invoice_details *details; struct invoice_payment_hook_payload *payload; - details = invoice_check_payment(tmpctx, ld, payment_hash, msat, - payment_secret); - - if (!details) { - fail_htlc(hin, WIRE_INCORRECT_OR_UNKNOWN_PAYMENT_DETAILS); - return; - } - payload = tal(ld, struct invoice_payment_hook_payload); payload->ld = ld; payload->label = tal_steal(payload, details->label); - payload->msat = msat; + payload->msat = set->so_far; payload->preimage = details->r; - payload->hin = hin; - tal_add_destructor2(hin, invoice_payload_remove_hin, payload); + payload->set = set; + tal_add_destructor2(set, invoice_payload_remove_set, payload); plugin_hook_call_invoice_payment(ld, payload, payload); } diff --git a/lightningd/invoice.h b/lightningd/invoice.h index 199ab4b902e9..e50d87ca2ce8 100644 --- a/lightningd/invoice.h +++ b/lightningd/invoice.h @@ -4,7 +4,7 @@ #include struct amount_msat; -struct htlc_in; +struct htlc_set; struct lightningd; struct sha256; @@ -27,19 +27,15 @@ invoice_check_payment(const tal_t *ctx, const struct secret *payment_secret); /** - * invoice_try_pay - process payment for this payment_hash, amount msat. + * invoice_try_pay - process payment for these incoming payments. * @ld: lightningd - * @hin: the input HTLC which is offering to pay. - * @payment_hash: hash of preimage they want. - * @msat: amount they offer to pay. - * @payment_secret: they payment secret they sent, if any. + * @set: the htlc_set used to pay this. + * @details: returned from successful invoice_check_payment. * - * Either calls fulfill_htlc() or fail_htlcs(). + * Either calls fulfill_htlc_set() or fail_htlc_set(). */ void invoice_try_pay(struct lightningd *ld, - struct htlc_in *hin, - const struct sha256 *payment_hash, - const struct amount_msat msat, - const struct secret *payment_secret); + struct htlc_set *set, + const struct invoice_details *details); #endif /* LIGHTNING_LIGHTNINGD_INVOICE_H */ diff --git a/lightningd/peer_htlcs.c b/lightningd/peer_htlcs.c index 9bd2c4a9d805..a43b9807e0b0 100644 --- a/lightningd/peer_htlcs.c +++ b/lightningd/peer_htlcs.c @@ -17,6 +17,7 @@ #include #include #include +#include #include #include #include @@ -299,18 +300,6 @@ static void handle_localpay(struct htlc_in *hin, goto fail; } - /* BOLT-9441a66faad63edc8cd89860b22fbf24a86f0dcd #4: - * - if it does not support `basic_mpp`: - * - MUST fail the HTLC if `total_msat` is not exactly equal to - * `amt_to_forward`. - */ - if (!amount_msat_eq(amt_to_forward, total_msat)) { - /* FIXME: Ideally, we use WIRE_INVALID_ONION_PAYLOAD and - * point at the total_msat field */ - failcode = WIRE_FINAL_INCORRECT_HTLC_AMOUNT; - goto fail; - } - /* BOLT #4: * * 1. type: 18 (`final_incorrect_cltv_expiry`) @@ -341,7 +330,7 @@ static void handle_localpay(struct htlc_in *hin, goto fail; } - invoice_try_pay(ld, hin, &hin->payment_hash, amt_to_forward, payment_secret); + htlc_set_add(ld, hin, total_msat, payment_secret); return; fail: diff --git a/lightningd/test/run-invoice-select-inchan.c b/lightningd/test/run-invoice-select-inchan.c index 33e078df938e..eab27f7d3346 100644 --- a/lightningd/test/run-invoice-select-inchan.c +++ b/lightningd/test/run-invoice-select-inchan.c @@ -87,9 +87,6 @@ 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 fail_htlc */ -void fail_htlc(struct htlc_in *hin UNNEEDED, enum onion_type failcode UNNEEDED) -{ fprintf(stderr, "fail_htlc called!\n"); abort(); } /* Generated stub for fatal */ void fatal(const char *fmt UNNEEDED, ...) { fprintf(stderr, "fatal called!\n"); abort(); } @@ -120,9 +117,6 @@ bool fromwire_hsm_sign_invoice_reply(const void *p UNNEEDED, secp256k1_ecdsa_rec /* Generated stub for fromwire_onchain_dev_memleak_reply */ bool fromwire_onchain_dev_memleak_reply(const void *p UNNEEDED, bool *leak UNNEEDED) { fprintf(stderr, "fromwire_onchain_dev_memleak_reply called!\n"); abort(); } -/* Generated stub for fulfill_htlc */ -void fulfill_htlc(struct htlc_in *hin UNNEEDED, const struct preimage *preimage UNNEEDED) -{ fprintf(stderr, "fulfill_htlc called!\n"); abort(); } /* Generated stub for get_block_height */ u32 get_block_height(const struct chain_topology *topo UNNEEDED) { fprintf(stderr, "get_block_height called!\n"); abort(); } @@ -136,6 +130,12 @@ bool htlc_is_trimmed(enum side htlc_owner UNNEEDED, struct amount_sat dust_limit UNNEEDED, enum side side UNNEEDED) { fprintf(stderr, "htlc_is_trimmed called!\n"); abort(); } +/* Generated stub for htlc_set_fail */ +void htlc_set_fail(struct htlc_set *set UNNEEDED, enum onion_type failcode UNNEEDED) +{ fprintf(stderr, "htlc_set_fail called!\n"); abort(); } +/* Generated stub for htlc_set_fulfill */ +void htlc_set_fulfill(struct htlc_set *set UNNEEDED, const struct preimage *preimage UNNEEDED) +{ fprintf(stderr, "htlc_set_fulfill called!\n"); abort(); } /* Generated stub for json_add_address */ void json_add_address(struct json_stream *response UNNEEDED, const char *fieldname UNNEEDED, const struct wireaddr *addr UNNEEDED) diff --git a/tests/test_pay.py b/tests/test_pay.py index f83e433188d7..3c5119f1a55b 100644 --- a/tests/test_pay.py +++ b/tests/test_pay.py @@ -2625,16 +2625,15 @@ def test_partial_payment(node_factory, bitcoind, executor): l2.daemon.wait_for_log(r'dev_disconnect') l3.daemon.wait_for_log(r'dev_disconnect') - # Now continue, payments will fail because receiver doesn't do MPP. + # Now continue, payments will succeed due to MPP. l2.rpc.dev_reenable_commit(l4.info['id']) l3.rpc.dev_reenable_commit(l4.info['id']) - # See FIXME in peer_htlcs: WIRE_INVALID_ONION_PAYLOAD would be better. - with pytest.raises(RpcError, match=r'WIRE_FINAL_INCORRECT_HTLC_AMOUNT'): - l1.rpc.call('waitsendpay', [inv['payment_hash'], None, 1]) - with pytest.raises(RpcError, match=r'WIRE_FINAL_INCORRECT_HTLC_AMOUNT'): - l1.rpc.call('waitsendpay', {'payment_hash': inv['payment_hash'], - 'partid': 2}) + res = l1.rpc.call('waitsendpay', [inv['payment_hash'], None, 1]) + assert res['partid'] == 1 + res = l1.rpc.call('waitsendpay', {'payment_hash': inv['payment_hash'], + 'partid': 2}) + assert res['partid'] == 2 for i in range(2): line = l4.daemon.wait_for_log('print_htlc_onion.py: Got onion') diff --git a/wallet/test/run-wallet.c b/wallet/test/run-wallet.c index ca2c5b785207..98cc0b127bb6 100644 --- a/wallet/test/run-wallet.c +++ b/wallet/test/run-wallet.c @@ -130,6 +130,12 @@ bool htlc_is_trimmed(enum side htlc_owner UNNEEDED, struct amount_sat dust_limit UNNEEDED, enum side side UNNEEDED) { fprintf(stderr, "htlc_is_trimmed called!\n"); abort(); } +/* Generated stub for htlc_set_add */ +void htlc_set_add(struct lightningd *ld UNNEEDED, + struct htlc_in *hin UNNEEDED, + struct amount_msat total_msat UNNEEDED, + const struct secret *payment_secret UNNEEDED) +{ fprintf(stderr, "htlc_set_add called!\n"); abort(); } /* Generated stub for invoices_create */ bool invoices_create(struct invoices *invoices UNNEEDED, struct invoice *pinvoice UNNEEDED, @@ -203,13 +209,6 @@ void invoices_waitone(const tal_t *ctx UNNEEDED, void (*cb)(const struct invoice * UNNEEDED, void*) UNNEEDED, void *cbarg UNNEEDED) { fprintf(stderr, "invoices_waitone called!\n"); abort(); } -/* Generated stub for invoice_try_pay */ -void invoice_try_pay(struct lightningd *ld UNNEEDED, - struct htlc_in *hin UNNEEDED, - const struct sha256 *payment_hash UNNEEDED, - const struct amount_msat msat UNNEEDED, - const struct secret *payment_secret UNNEEDED) -{ fprintf(stderr, "invoice_try_pay called!\n"); abort(); } /* Generated stub for json_add_address */ void json_add_address(struct json_stream *response UNNEEDED, const char *fieldname UNNEEDED, const struct wireaddr *addr UNNEEDED) From 8cee3755bb7b0318f5e720b6362ae956f5e0c67f Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Thu, 12 Dec 2019 10:25:45 +1030 Subject: [PATCH 16/22] plugins: listpays ignores pre-0.7.0 or manual sendpay payments w/ no bolt11. The pay plugin has been supplying the bolt11 string since 0.7.0, so only ancient "pay" commands would be omitted by this change. You can create a no-bolt11 "sendpay" manually, but then you'll find it in 'listsendpays'. Changelog-Removed: JSON: `listpays` won't shown payments made via sendpay without a bolt11 string, or before 0.7.0. Signed-off-by: Rusty Russell --- plugins/pay.c | 14 +++----------- 1 file changed, 3 insertions(+), 11 deletions(-) diff --git a/plugins/pay.c b/plugins/pay.c index 3dfc0a4dcc2c..1cdd339377c2 100644 --- a/plugins/pay.c +++ b/plugins/pay.c @@ -1300,18 +1300,10 @@ static struct command_result *listsendpays_done(struct command *cmd, const jsmntok_t *status, *b11; json_out_start(ret, NULL, '{'); - /* Old payments didn't have bolt11 field */ b11 = copy_member(ret, buf, t, "bolt11"); - if (!b11) { - if (b11str) { - /* If it's a single query, we can fake it */ - json_out_addstr(ret, "bolt11", b11str); - } else { - copy_member(ret, buf, t, "payment_hash"); - copy_member(ret, buf, t, "destination"); - copy_member(ret, buf, t, "amount_msat"); - } - } + /* Old (or manual) payments didn't have bolt11 field */ + if (!b11) + continue; /* listsendpays might say it failed, but we're still retrying */ status = json_get_member(buf, t, "status"); From 84a2753231768050d90f20e37a668fe4bd094b19 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Thu, 12 Dec 2019 10:25:45 +1030 Subject: [PATCH 17/22] plugins: listpays will now consolidate multi-part payments. This won't usually be visible to the end-user, since the pay plugin doesn't do multi-part yet (and mpp requires EXPERIMENTAL_FEATURES), but we're ready once it does. Signed-off-by: Rusty Russell --- plugins/pay.c | 149 +++++++++++++++++++++++++++++++++++++++------- tests/test_pay.py | 6 ++ 2 files changed, 135 insertions(+), 20 deletions(-) diff --git a/plugins/pay.c b/plugins/pay.c index 1cdd339377c2..b53dc9b71ad0 100644 --- a/plugins/pay.c +++ b/plugins/pay.c @@ -1,5 +1,7 @@ #include #include +#include +#include #include #include #include @@ -1265,13 +1267,13 @@ static struct command_result *json_paystatus(struct command *cmd, return command_success(cmd, ret); } -static bool attempt_ongoing(const char *buf, const jsmntok_t *b11) +static bool attempt_ongoing(const char *b11) { struct pay_status *ps; struct pay_attempt *attempt; list_for_each(&pay_status, ps, list) { - if (!json_tok_streq(buf, b11, ps->bolt11)) + if (!streq(b11, ps->bolt11)) continue; attempt = &ps->attempts[tal_count(ps->attempts)-1]; return attempt->result == NULL && attempt->failure == NULL; @@ -1279,6 +1281,79 @@ static bool attempt_ongoing(const char *buf, const jsmntok_t *b11) return false; } +/* We consolidate multi-part payments into a single entry. */ +struct pay_mpp { + /* This is the bolt11 string, and lookup key */ + const char *b11; + /* Status of combined payment */ + const char *status; + /* Optional label (of first one!) */ + const jsmntok_t *label; + /* Optional preimage (iff status is successful) */ + const jsmntok_t *preimage; + /* Only counts "complete" or "pending" payments. */ + size_t num_nonfailed_parts; + /* Total amount sent ("complete" or "pending" only). */ + struct amount_msat amount_sent; +}; + +static const char *pay_mpp_key(const struct pay_mpp *pm) +{ + return pm->b11; +} + +static size_t b11str_hash(const char *b11) +{ + return siphash24(siphash_seed(), b11, strlen(b11)); +} + +static bool pay_mpp_eq(const struct pay_mpp *pm, const char *b11) +{ + return streq(pm->b11, b11); +} + +HTABLE_DEFINE_TYPE(struct pay_mpp, pay_mpp_key, b11str_hash, pay_mpp_eq, + pay_map); + +static void add_amount_sent(const char *b11, + struct amount_msat *total, + const char *buf, + const jsmntok_t *t) +{ + struct amount_msat sent; + json_to_msat(buf, json_get_member(buf, t, "amount_sent_msat"), &sent); + if (!amount_msat_add(total, *total, sent)) + plugin_log(LOG_BROKEN, + "Cannot add amount_sent_msat for %s: %s + %s", + b11, + type_to_string(tmpctx, struct amount_msat, total), + type_to_string(tmpctx, struct amount_msat, &sent)); +} + +static void add_new_entry(struct json_out *ret, + const char *buf, + const struct pay_mpp *pm) +{ + json_out_start(ret, NULL, '{'); + json_out_addstr(ret, "bolt11", pm->b11); + json_out_addstr(ret, "status", pm->status); + if (pm->label) + json_out_add_raw_len(ret, "label", + json_tok_full(buf, pm->label), + json_tok_full_len(pm->label)); + if (pm->preimage) + json_out_add_raw_len(ret, "preimage", + json_tok_full(buf, pm->preimage), + json_tok_full_len(pm->preimage)); + json_out_addstr(ret, "amount_sent_msat", + fmt_amount_msat(tmpctx, &pm->amount_sent)); + + if (pm->num_nonfailed_parts > 1) + json_out_add_u64(ret, "number_of_parts", + pm->num_nonfailed_parts); + json_out_end(ret, '}'); +} + static struct command_result *listsendpays_done(struct command *cmd, const char *buf, const jsmntok_t *result, @@ -1287,6 +1362,11 @@ static struct command_result *listsendpays_done(struct command *cmd, size_t i; const jsmntok_t *t, *arr; struct json_out *ret; + struct pay_map pay_map; + struct pay_map_iter it; + struct pay_mpp *pm; + + pay_map_init(&pay_map); arr = json_get_member(buf, result, "payments"); if (!arr || arr->type != JSMN_ARRAY) @@ -1297,31 +1377,60 @@ static struct command_result *listsendpays_done(struct command *cmd, json_out_start(ret, NULL, '{'); json_out_start(ret, "pays", '['); json_for_each_arr(i, t, arr) { - const jsmntok_t *status, *b11; + const jsmntok_t *status, *b11tok; + const char *b11; - json_out_start(ret, NULL, '{'); - b11 = copy_member(ret, buf, t, "bolt11"); + b11tok = json_get_member(buf, t, "bolt11"); /* Old (or manual) payments didn't have bolt11 field */ - if (!b11) + if (!b11tok) continue; - /* listsendpays might say it failed, but we're still retrying */ + b11 = json_strdup(cmd, buf, b11tok); + + pm = pay_map_get(&pay_map, b11); + if (!pm) { + pm = tal(cmd, struct pay_mpp); + pm->b11 = tal_steal(pm, b11); + pm->label = json_get_member(buf, t, "label"); + pm->preimage = NULL; + pm->amount_sent = AMOUNT_MSAT(0); + pm->num_nonfailed_parts = 0; + pm->status = NULL; + pay_map_add(&pay_map, pm); + } + status = json_get_member(buf, t, "status"); - if (status) { - if (json_tok_streq(buf, status, "failed") - && attempt_ongoing(buf, b11)) { - json_out_addstr(ret, "status", "pending"); - } else { - copy_member(ret, buf, t, "status"); - if (json_tok_streq(buf, status, "complete")) - copy_member(ret, buf, t, - "payment_preimage"); - } + if (json_tok_streq(buf, status, "complete")) { + add_amount_sent(pm->b11, &pm->amount_sent, buf, t); + pm->num_nonfailed_parts++; + pm->status = "complete"; + pm->preimage + = json_get_member(buf, t, "payment_preimage"); + } else if (json_tok_streq(buf, status, "pending")) { + add_amount_sent(pm->b11, &pm->amount_sent, buf, t); + pm->num_nonfailed_parts++; + /* Failed -> pending; don't downgrade success. */ + if (!pm->status || !streq(pm->status, "complete")) + pm->status = "pending"; + } else { + if (attempt_ongoing(pm->b11)) { + /* Failed -> pending; don't downgrade success. */ + if (!pm->status + || !streq(pm->status, "complete")) + pm->status = "pending"; + } else if (!pm->status) + /* Only failed if they all failed */ + pm->status = "failed"; } - copy_member(ret, buf, t, "label"); - copy_member(ret, buf, t, "amount_sent_msat"); - json_out_end(ret, '}'); } + + /* Now we've collapsed them, provide summary (free mem as we go). */ + while ((pm = pay_map_first(&pay_map, &it)) != NULL) { + add_new_entry(ret, buf, pm); + pay_map_del(&pay_map, pm); + } + pay_map_clear(&pay_map); + json_out_end(ret, ']'); json_out_end(ret, '}'); return command_success(cmd, ret); diff --git a/tests/test_pay.py b/tests/test_pay.py index 3c5119f1a55b..f2626f30a7c1 100644 --- a/tests/test_pay.py +++ b/tests/test_pay.py @@ -2641,3 +2641,9 @@ def test_partial_payment(node_factory, bitcoind, executor): assert "'forward_amount': '500msat'" in line assert "'total_msat': '1000msat'" in line assert "'payment_secret': '{}'".format(paysecret) in line + + pay = only_one(l1.rpc.listpays()['pays']) + assert pay['bolt11'] == inv['bolt11'] + assert pay['status'] == 'complete' + assert pay['number_of_parts'] == 2 + assert pay['amount_sent_msat'] == Millisatoshi(1002) From c6bbb41bf8484a569b5b82caed0b7ef6567cc652 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Thu, 12 Dec 2019 10:25:45 +1030 Subject: [PATCH 18/22] common: offer option_basic_mpp for EXPERIMENTAL_FEATURES. Signed-off-by: Rusty Russell --- common/features.c | 1 + tests/test_gossip.py | 7 +++++-- tests/test_misc.py | 1 + tests/utils.py | 4 ++-- 4 files changed, 9 insertions(+), 4 deletions(-) diff --git a/common/features.c b/common/features.c index 77f6935010fe..57c6fda58225 100644 --- a/common/features.c +++ b/common/features.c @@ -11,6 +11,7 @@ static const u32 our_features[] = { #if EXPERIMENTAL_FEATURES OPTIONAL_FEATURE(OPT_VAR_ONION), OPTIONAL_FEATURE(OPT_PAYMENT_SECRET), + OPTIONAL_FEATURE(OPT_BASIC_MPP), #endif OPTIONAL_FEATURE(OPT_GOSSIP_QUERIES_EX), OPTIONAL_FEATURE(OPT_STATIC_REMOTEKEY), diff --git a/tests/test_gossip.py b/tests/test_gossip.py index 87898bc03996..5aa7c5eeb8cd 100644 --- a/tests/test_gossip.py +++ b/tests/test_gossip.py @@ -2,7 +2,7 @@ from fixtures import * # noqa: F401,F403 from fixtures import TEST_NETWORK from lightning import RpcError -from utils import wait_for, TIMEOUT, only_one, sync_blockheight, expected_features +from utils import wait_for, TIMEOUT, only_one, sync_blockheight, expected_features, EXPERIMENTAL_FEATURES import json import logging @@ -1450,7 +1450,10 @@ def test_gossip_store_compact_on_load(node_factory, bitcoind): l2.restart() wait_for(lambda: l2.daemon.is_in_log(r'gossip_store_compact_offline: [5-8] deleted, 9 copied')) - wait_for(lambda: l2.daemon.is_in_log(r'gossip_store: Read 1/4/2/0 cannounce/cupdate/nannounce/cdelete from store \(0 deleted\) in 1450 bytes')) + if EXPERIMENTAL_FEATURES: + wait_for(lambda: l2.daemon.is_in_log(r'gossip_store: Read 1/4/2/0 cannounce/cupdate/nannounce/cdelete from store \(0 deleted\) in 1452 bytes')) + else: + wait_for(lambda: l2.daemon.is_in_log(r'gossip_store: Read 1/4/2/0 cannounce/cupdate/nannounce/cdelete from store \(0 deleted\) in 1450 bytes')) def test_gossip_announce_invalid_block(node_factory, bitcoind): diff --git a/tests/test_misc.py b/tests/test_misc.py index 4842f09cac75..277960a6aa06 100644 --- a/tests/test_misc.py +++ b/tests/test_misc.py @@ -1721,6 +1721,7 @@ def test_list_features_only(node_factory): 'option_gossip_queries/odd', 'option_var_onion_optin/odd', 'option_payment_secret/odd', + 'option_basic_mpp/odd', 'option_gossip_queries_ex/odd', 'option_static_remotekey/odd', ] diff --git a/tests/utils.py b/tests/utils.py index 3dbbe4e1ae8b..a39d6568a0fa 100644 --- a/tests/utils.py +++ b/tests/utils.py @@ -9,8 +9,8 @@ def expected_features(): """Return the expected features hexstring for this configuration""" if EXPERIMENTAL_FEATURES: - # features 1, 3, 7, 9, 11, 13 and 15 (0xaaa2). - return "aaa2" + # features 1, 3, 7, 9, 11, 13, 15 and 17 (0x02aaa2). + return "02aaa2" else: # features 1, 3, 7, 11 and 13 (0x28a2). return "28a2" From cbfc84f644c29d7acfba7e75bae7c484ecb5242a Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Thu, 12 Dec 2019 10:25:45 +1030 Subject: [PATCH 19/22] pytest: add more multi-part-payment tests. Signed-off-by: Rusty Russell --- tests/test_pay.py | 73 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 73 insertions(+) diff --git a/tests/test_pay.py b/tests/test_pay.py index f2626f30a7c1..a72a1cfa3dbf 100644 --- a/tests/test_pay.py +++ b/tests/test_pay.py @@ -2647,3 +2647,76 @@ def test_partial_payment(node_factory, bitcoind, executor): assert pay['status'] == 'complete' assert pay['number_of_parts'] == 2 assert pay['amount_sent_msat'] == Millisatoshi(1002) + + +@unittest.skipIf(not EXPERIMENTAL_FEATURES, "needs partid support") +def test_partial_payment_timeout(node_factory, bitcoind): + l1, l2 = node_factory.line_graph(2) + + inv = l2.rpc.invoice(1000, 'inv', 'inv') + paysecret = l2.rpc.decodepay(inv['bolt11'])['payment_secret'] + + route = l1.rpc.getroute(l2.info['id'], 500, 1)['route'] + l1.rpc.call('sendpay', [route, inv['payment_hash'], None, 1000, inv['bolt11'], paysecret, 1]) + + with pytest.raises(RpcError, match=r'WIRE_MPP_TIMEOUT'): + l1.rpc.call('waitsendpay', [inv['payment_hash'], 70 + TIMEOUT // 4, 1]) + + # We can still pay it normally. + l1.rpc.call('sendpay', [route, inv['payment_hash'], None, 1000, inv['bolt11'], paysecret, 1]) + l1.rpc.call('sendpay', [route, inv['payment_hash'], None, 1000, inv['bolt11'], paysecret, 2]) + l1.rpc.call('waitsendpay', [inv['payment_hash'], TIMEOUT, 1]) + l1.rpc.call('waitsendpay', [inv['payment_hash'], TIMEOUT, 2]) + + +@unittest.skipIf(not EXPERIMENTAL_FEATURES, "needs partid support") +def test_partial_payment_restart(node_factory, bitcoind): + """Test that we recover a set when we restart""" + l1, l2, l3 = node_factory.line_graph(3, wait_for_announce=True, + opts=[{}] + + [{'may_reconnect': True}] * 2) + + inv = l3.rpc.invoice(1000, 'inv', 'inv') + paysecret = l3.rpc.decodepay(inv['bolt11'])['payment_secret'] + + route = l1.rpc.getroute(l3.info['id'], 500, 1)['route'] + + l1.rpc.call('sendpay', [route, inv['payment_hash'], None, 1000, inv['bolt11'], paysecret, 1]) + + wait_for(lambda: [f['status'] for f in l2.rpc.listforwards()['forwards']] == ['offered']) + + # Restart, and make sure it's reconnected to l2. + l3.restart() + print(l2.rpc.listpeers()) + wait_for(lambda: [p['connected'] for p in l2.rpc.listpeers()['peers']] == [True, True]) + + # Pay second part. + l1.rpc.call('sendpay', [route, inv['payment_hash'], None, 1000, inv['bolt11'], paysecret, 2]) + + l1.rpc.call('waitsendpay', [inv['payment_hash'], TIMEOUT, 1]) + l1.rpc.call('waitsendpay', [inv['payment_hash'], TIMEOUT, 2]) + + +@unittest.skipIf(not DEVELOPER, "needs dev-fail") +@unittest.skipIf(not EXPERIMENTAL_FEATURES, "needs partid support") +def test_partial_payment_htlc_loss(node_factory, bitcoind): + """Test that we discard a set when the HTLC is lost""" + l1, l2, l3 = node_factory.line_graph(3, wait_for_announce=True) + + inv = l3.rpc.invoice(1000, 'inv', 'inv') + paysecret = l3.rpc.decodepay(inv['bolt11'])['payment_secret'] + + route = l1.rpc.getroute(l3.info['id'], 500, 1)['route'] + + l1.rpc.call('sendpay', [route, inv['payment_hash'], None, 1000, inv['bolt11'], paysecret, 1]) + wait_for(lambda: [f['status'] for f in l2.rpc.listforwards()['forwards']] == ['offered']) + + l2.rpc.dev_fail(l3.info['id']) + + # Since HTLC is missing from commit (dust), it's closed as soon as l2 sees + # it onchain. l3 shouldn't crash though. + bitcoind.generate_block(1, wait_for_mempool=1) + + with pytest.raises(RpcError, + match=r'WIRE_PERMANENT_CHANNEL_FAILURE \(reply from remote\)'): + l1.rpc.call('waitsendpay', [inv['payment_hash'], TIMEOUT, 1]) From 2b4ca094d7ab90c87b0e2e4bf386ae888ab63aec Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Thu, 12 Dec 2019 10:29:33 +1030 Subject: [PATCH 20/22] lightningd: require payment_secret for MPP. It makes sense, and it's been proposed for addition to the spec to broad agreement: https://github.com/lightningnetwork/lightning-rfc/pull/712 Signed-off-by: Rusty Russell --- lightningd/htlc_set.c | 27 +++++++++++++++++++++++++-- 1 file changed, 25 insertions(+), 2 deletions(-) diff --git a/lightningd/htlc_set.c b/lightningd/htlc_set.c index fa5b1d032160..dced43782065 100644 --- a/lightningd/htlc_set.c +++ b/lightningd/htlc_set.c @@ -136,8 +136,24 @@ void htlc_set_add(struct lightningd *ld, set = htlc_set_map_get(&ld->htlc_sets, &hin->payment_hash); if (!set) set = new_htlc_set(ld, hin, total_msat); - else + else { + /* BOLT-0729433704dd11cc07a0535c09e5f64de7a5017b #4: + * + * if it supports `basic_mpp`: + * ... + * - otherwise, if the total `amount_msat` of this HTLC set is + * less than `total_msat`: + * ... + * - MUST require `payment_secret` for all HTLCs in the set. + */ + /* We check this now, since we want to fail with this as soon + * as possible, to avoid other probing attacks. */ + if (!payment_secret) { + fail_htlc(hin, WIRE_INCORRECT_OR_UNKNOWN_PAYMENT_DETAILS); + return; + } tal_arr_expand(&set->htlcs, hin); + } /* Remove from set should hin get destroyed somehow */ tal_add_destructor2(hin, htlc_set_hin_destroyed, set); @@ -185,5 +201,12 @@ void htlc_set_add(struct lightningd *ld, /* BOLT-9441a66faad63edc8cd89860b22fbf24a86f0dcd #4: * - otherwise, if the total `amount_msat` of this HTLC set is less than * `total_msat`: - * - MUST NOT fulfill any HTLCs in the HTLC set */ + * - MUST NOT fulfill any HTLCs in the HTLC set + *... + * - MUST require `payment_secret` for all HTLCs in the set. */ + /* This catches the case of the first payment in a set. */ + if (!payment_secret) { + htlc_set_fail(set, WIRE_INCORRECT_OR_UNKNOWN_PAYMENT_DETAILS); + return; + } } From 207ae69c6156b2bf08f47259ce8462d78c520895 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Thu, 12 Dec 2019 10:48:27 +1030 Subject: [PATCH 21/22] lightningd: fix spurious "more than twice final" error. Bastien TEINTURIER writes: > It looks like the split on c-lightning side is quite limited at the moment: > the only option is to split a payment in exactly its two halves, > otherwise I get rejected because of the rule of overpaying more than > twice the amount? We only tested exactly two equal-size payments; indeed, our finalhop test was backwards. We only complain if the final hop pays more than twice msat (technically, this test is still too loose for mpp: the spec says we should sum to the exact amount). Reported-by: @t-bast Signed-off-by: Rusty Russell --- lightningd/pay.c | 18 +++++++++--------- tests/test_pay.py | 6 +++--- 2 files changed, 12 insertions(+), 12 deletions(-) diff --git a/lightningd/pay.c b/lightningd/pay.c index 7d20aecf8eb9..446fd9823a95 100644 --- a/lightningd/pay.c +++ b/lightningd/pay.c @@ -1292,26 +1292,26 @@ static struct command_result *json_sendpay(struct command *cmd, return command_fail(cmd, JSONRPC2_INVALID_PARAMS, "Must specify msatoshi with partid"); - /* if not: finalhop.amount <= 2 * msatoshi, fail. */ + /* finalhop.amount > 2 * msatoshi, fail. */ if (msat) { - struct amount_msat limit = route[routetok->size-1].amount; + struct amount_msat limit; - if (!amount_msat_add(&limit, limit, limit)) + if (!amount_msat_add(&limit, *msat, *msat)) return command_fail(cmd, JSONRPC2_INVALID_PARAMS, - "Unbelievable final amount %s", + "Unbelievable msatoshi %s", type_to_string(tmpctx, struct amount_msat, - &route[routetok->size-1].amount)); + msat)); - if (amount_msat_greater(*msat, limit)) + if (amount_msat_greater(route[routetok->size-1].amount, limit)) return command_fail(cmd, JSONRPC2_INVALID_PARAMS, - "msatoshi %s more than twice final %s", + "final %s more than twice msatoshi %s", type_to_string(tmpctx, struct amount_msat, - msat), + &route[routetok->size-1].amount), type_to_string(tmpctx, struct amount_msat, - &route[routetok->size-1].amount)); + msat)); } /* It's easier to leave this in the API, then ignore it here. */ diff --git a/tests/test_pay.py b/tests/test_pay.py index a72a1cfa3dbf..547bf50fc044 100644 --- a/tests/test_pay.py +++ b/tests/test_pay.py @@ -2593,8 +2593,8 @@ def test_partial_payment(node_factory, bitcoind, executor): paysecret = l4.rpc.decodepay(inv['bolt11'])['payment_secret'] # Separate routes for each part of the payment. - r134 = l1.rpc.getroute(l4.info['id'], 500, 1, exclude=[scid24 + '/0', scid24 + '/1'])['route'] - r124 = l1.rpc.getroute(l4.info['id'], 500, 1, exclude=[scid34 + '/0', scid34 + '/1'])['route'] + r134 = l1.rpc.getroute(l4.info['id'], 501, 1, exclude=[scid24 + '/0', scid24 + '/1'])['route'] + r124 = l1.rpc.getroute(l4.info['id'], 499, 1, exclude=[scid34 + '/0', scid34 + '/1'])['route'] # These can happen in parallel. l1.rpc.call('sendpay', [r134, inv['payment_hash'], None, 1000, inv['bolt11'], paysecret, 1]) @@ -2638,7 +2638,7 @@ def test_partial_payment(node_factory, bitcoind, executor): for i in range(2): line = l4.daemon.wait_for_log('print_htlc_onion.py: Got onion') assert "'type': 'tlv'" in line - assert "'forward_amount': '500msat'" in line + assert "'forward_amount': '499msat'" in line or "'forward_amount': '501msat'" in line assert "'total_msat': '1000msat'" in line assert "'payment_secret': '{}'".format(paysecret) in line From e6edb76864dc2f44ef8b47858eb8862853db4791 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Thu, 12 Dec 2019 10:50:46 +1030 Subject: [PATCH 22/22] lightningd: fix failure message in waitsendpay with multi-part payments. Bastien TEINTURIER writes: > One thing I noticed but didn't investigate much: after sending the two > payments, I tried using `waitsendpay` and it reported an error *208* > (*"Never attempted payment for > '98ee736d29d860948e436546a88b0cc84f267de8818531b0fdbe6ce3d080f22a'"*). > > I was expecting the result to be something like: "payment succeeded for > that payment hash" (the HTLCs were correctly settled). Indeed, if you waitsendpay without specifying a partid, you are waiting for 0, which may not exist. Clarify the error msg. Reported-by: @t-bast Signed-off-by: Rusty Russell --- lightningd/pay.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/lightningd/pay.c b/lightningd/pay.c index 446fd9823a95..f56fad7b65eb 100644 --- a/lightningd/pay.c +++ b/lightningd/pay.c @@ -627,7 +627,9 @@ static struct command_result *wait_payment(struct lightningd *ld, payment_hash, partid); if (!payment) { return command_fail(cmd, PAY_NO_SUCH_PAYMENT, - "Never attempted payment for '%s'", + "Never attempted payment part %"PRIu64 + " for '%s'", + partid, type_to_string(tmpctx, struct sha256, payment_hash)); }