From 38e8601cf6e4086725080504f83433412d85c489 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Thu, 18 Jan 2018 06:59:49 +1030 Subject: [PATCH] wallet: abstract away delayed entry of wallet_payment. For performance, we delay entering the 'wallet_payment' into the db until we actually commit to the HTLC (when we have to touch the DB anyway). This opens a race where we can try to pay twice, and since it's not in the database yet, we don't notice the duplicate. So remove the temporary payment field from htlc_out, which was always an uncomfortable hack, and make the wallet code abstract over the deferred entry a little by maintaining a 'unstored_payments' list and incorporating that in results. Signed-off-by: Rusty Russell --- lightningd/htlc_end.c | 2 -- lightningd/htlc_end.h | 6 +--- lightningd/pay.c | 28 +++++++++-------- lightningd/peer_htlcs.c | 20 ++++-------- lightningd/peer_htlcs.h | 1 - wallet/test/run-wallet.c | 51 +++++++++++++++--------------- wallet/wallet.c | 67 +++++++++++++++++++++++++++++++++++----- wallet/wallet.h | 20 ++++++++++-- 8 files changed, 123 insertions(+), 72 deletions(-) diff --git a/lightningd/htlc_end.c b/lightningd/htlc_end.c index d770b292e..dd5939824 100644 --- a/lightningd/htlc_end.c +++ b/lightningd/htlc_end.c @@ -140,7 +140,6 @@ struct htlc_out *new_htlc_out(const tal_t *ctx, const struct sha256 *payment_hash, const u8 *onion_routing_packet, struct htlc_in *in, - struct wallet_payment *payment, struct command *cmd) { struct htlc_out *hout = tal(ctx, struct htlc_out); @@ -153,7 +152,6 @@ struct htlc_out *new_htlc_out(const tal_t *ctx, hout->msatoshi = msatoshi; hout->cltv_expiry = cltv_expiry; hout->payment_hash = *payment_hash; - hout->payment = tal_steal(hout, payment); hout->cmd = cmd; memcpy(hout->onion_routing_packet, onion_routing_packet, sizeof(hout->onion_routing_packet)); diff --git a/lightningd/htlc_end.h b/lightningd/htlc_end.h index 7cb0156f2..c6901c998 100644 --- a/lightningd/htlc_end.h +++ b/lightningd/htlc_end.h @@ -78,9 +78,6 @@ struct htlc_out { /* Otherwise, this MAY be non-null if there's a pay command waiting */ struct command *cmd; - - /* Temporary payment store, so we can save everything in one go */ - struct wallet_payment *payment; }; static inline const struct htlc_key *keyof_htlc_in(const struct htlc_in *in) @@ -129,14 +126,13 @@ struct htlc_in *new_htlc_in(const tal_t *ctx, const struct secret *shared_secret, const u8 *onion_routing_packet); -/* You need to set the ID, then connect_htlc_out this! Steals @payment. */ +/* You need to set the ID, then connect_htlc_out this! */ struct htlc_out *new_htlc_out(const tal_t *ctx, struct peer *peer, u64 msatoshi, u32 cltv_expiry, const struct sha256 *payment_hash, const u8 *onion_routing_packet, struct htlc_in *in, - struct wallet_payment *payment, struct command *cmd); void connect_htlc_in(struct htlc_in_map *map, struct htlc_in *hin); diff --git a/lightningd/pay.c b/lightningd/pay.c index 478b6582f..fe54b32b3 100644 --- a/lightningd/pay.c +++ b/lightningd/pay.c @@ -199,24 +199,12 @@ static bool send_payment(struct command *cmd, sizeof(struct sha256), &path_secrets); onion = serialize_onionpacket(cmd, packet); - /* We write this into db when HTLC is actually sent. */ - payment = tal(tmpctx, struct wallet_payment); - payment->id = 0; - payment->payment_hash = *rhash; - payment->destination = ids[n_hops - 1]; - payment->status = PAYMENT_PENDING; - payment->msatoshi = route[n_hops-1].amount; - payment->timestamp = time_now().ts.tv_sec; - payment->payment_preimage = NULL; - payment->path_secrets = tal_steal(payment, path_secrets); - log_info(cmd->ld->log, "Sending %u over %zu hops to deliver %u", route[0].amount, n_hops, route[n_hops-1].amount); - /* Steals payment into hout */ failcode = send_htlc_out(peer, route[0].amount, base_expiry + route[0].delay, - rhash, onion, NULL, payment, cmd, + rhash, onion, NULL, cmd, &hout); if (failcode) { command_fail(cmd, "first peer not ready: %s", @@ -224,6 +212,20 @@ static bool send_payment(struct command *cmd, return false; } + /* If hout fails, payment should be freed too. */ + payment = tal(hout, struct wallet_payment); + payment->id = 0; + payment->payment_hash = *rhash; + payment->destination = ids[n_hops - 1]; + payment->status = PAYMENT_PENDING; + payment->msatoshi = route[n_hops-1].amount; + payment->timestamp = time_now().ts.tv_sec; + payment->payment_preimage = NULL; + payment->path_secrets = tal_steal(payment, path_secrets); + + /* We write this into db when HTLC is actually sent. */ + wallet_payment_setup(cmd->ld->wallet, payment); + /* If we fail, remove cmd ptr from htlc_out. */ tal_add_destructor2(cmd, remove_cmd_from_hout, hout); diff --git a/lightningd/peer_htlcs.c b/lightningd/peer_htlcs.c index 0512b523e..80fb83e59 100644 --- a/lightningd/peer_htlcs.c +++ b/lightningd/peer_htlcs.c @@ -372,7 +372,6 @@ enum onion_type send_htlc_out(struct peer *out, u64 amount, u32 cltv, const struct sha256 *payment_hash, const u8 *onion_routing_packet, struct htlc_in *in, - struct wallet_payment *payment, struct command *cmd, struct htlc_out **houtp) { @@ -394,7 +393,7 @@ enum onion_type send_htlc_out(struct peer *out, u64 amount, u32 cltv, /* 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, - payment, cmd); + cmd); tal_add_destructor(hout, hout_subd_died); msg = towire_channel_offer_htlc(out, amount, cltv, payment_hash, @@ -486,7 +485,7 @@ static void forward_htlc(struct htlc_in *hin, failcode = send_htlc_out(next, amt_to_forward, outgoing_cltv_value, &hin->payment_hash, - next_onion, hin, NULL, NULL, NULL); + next_onion, hin, NULL, NULL); if (!failcode) return; @@ -864,18 +863,11 @@ static bool update_out_htlc(struct peer *peer, u64 id, enum htlc_state newstate) if (!hout->dbid) { wallet_htlc_save_out(peer->ld->wallet, peer->channel, hout); - } - - /* We only have a payment if we initiated the payment. */ - if (hout->payment) { - /* Now that we are committed, and inside the - * transaction context of the update, add the payment - * to the history. */ - wallet_payment_add(peer->ld->wallet, hout->payment); - /* No need to carry the payment info around anymore, - * we'll update in the database directly */ - hout->payment = tal_free(hout->payment); + /* For our own HTLCs, we commit payment to db lazily */ + if (hout->origin_htlc_id == 0) + wallet_payment_store(peer->ld->wallet, + &hout->payment_hash); } if (!htlc_out_update_state(peer, hout, newstate)) diff --git a/lightningd/peer_htlcs.h b/lightningd/peer_htlcs.h index ba60cd191..5f46297c5 100644 --- a/lightningd/peer_htlcs.h +++ b/lightningd/peer_htlcs.h @@ -39,7 +39,6 @@ enum onion_type send_htlc_out(struct peer *out, u64 amount, u32 cltv, const struct sha256 *payment_hash, const u8 *onion_routing_packet, struct htlc_in *in, - struct wallet_payment *payment, struct command *cmd, struct htlc_out **houtp); diff --git a/wallet/test/run-wallet.c b/wallet/test/run-wallet.c index a53b0578f..7293c5475 100644 --- a/wallet/test/run-wallet.c +++ b/wallet/test/run-wallet.c @@ -72,6 +72,7 @@ static struct wallet *create_test_wallet(const tal_t *ctx) ltmp = tal_tmpctx(ctx); log_book = new_log_book(w, 20*1024*1024, LOG_DBG); w->log = new_log(w, log_book, "wallet_tests(%u):", (int)getpid()); + list_head_init(&w->unstored_payments); CHECK_MSG(w->db, "Failed opening the db"); db_migrate(w->db, w->log); @@ -549,41 +550,39 @@ static bool test_htlc_crud(const tal_t *ctx) static bool test_payment_crud(const tal_t *ctx) { - struct wallet_payment t, *t2; + struct wallet_payment *t = tal(ctx, struct wallet_payment), *t2; struct wallet *w = create_test_wallet(ctx); - mempat(&t, sizeof(t)); - memset(&t.destination, 1, sizeof(t.destination)); + mempat(t, sizeof(*t)); + memset(&t->destination, 1, sizeof(t->destination)); - t.id = 0; - t.msatoshi = 100; - t.status = PAYMENT_PENDING; - t.payment_preimage = NULL; - memset(&t.payment_hash, 1, sizeof(t.payment_hash)); + t->id = 0; + t->msatoshi = 100; + t->status = PAYMENT_PENDING; + t->payment_preimage = NULL; + memset(&t->payment_hash, 1, sizeof(t->payment_hash)); db_begin_transaction(w->db); - CHECK(wallet_payment_add(w, &t)); - CHECK(t.id != 0); - t2 = wallet_payment_by_hash(ctx, w, &t.payment_hash); + 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); CHECK(t2 != NULL); - CHECK(t2->id == t.id); - CHECK(t2->status == t.status); - CHECK(pubkey_cmp(&t2->destination, &t.destination) == 0); - CHECK(t2->msatoshi == t.msatoshi); + CHECK(t2->status == t->status); + CHECK(pubkey_cmp(&t2->destination, &t->destination) == 0); + CHECK(t2->msatoshi == t->msatoshi); 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, - t.payment_preimage); - t2 = wallet_payment_by_hash(ctx, w, &t.payment_hash); + 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, + t->payment_preimage); + t2 = wallet_payment_by_hash(ctx, w, &t->payment_hash); CHECK(t2 != NULL); - CHECK(t2->id == t.id); - CHECK(t2->status == t.status); - CHECK(pubkey_cmp(&t2->destination, &t.destination) == 0); - CHECK(t2->msatoshi == t.msatoshi); - CHECK(structeq(t.payment_preimage, t2->payment_preimage)); + CHECK(t2->status == t->status); + CHECK(pubkey_cmp(&t2->destination, &t->destination) == 0); + CHECK(t2->msatoshi == t->msatoshi); + CHECK(structeq(t->payment_preimage, t2->payment_preimage)); db_commit_transaction(w->db); return true; diff --git a/wallet/wallet.c b/wallet/wallet.c index 676b81606..056001514 100644 --- a/wallet/wallet.c +++ b/wallet/wallet.c @@ -2,6 +2,7 @@ #include "wallet.h" #include +#include #include #include #include @@ -22,6 +23,7 @@ struct wallet *wallet_new(const tal_t *ctx, struct log *log) wallet->log = log; wallet->bip32_base = NULL; wallet->invoices = invoices_new(wallet, wallet->db, log); + list_head_init(&wallet->unstored_payments); return wallet; } @@ -1056,8 +1058,6 @@ static bool wallet_stmt2htlc_out(const struct wallet_channel *channel, * htlcs, will wire using origin_htlc_id */ out->in = NULL; - out->payment = NULL; - return ok; } @@ -1243,10 +1243,38 @@ struct htlc_stub *wallet_htlc_stubs(const tal_t *ctx, struct wallet *wallet, return stubs; } -bool wallet_payment_add(struct wallet *wallet, - struct wallet_payment *payment) +static struct wallet_payment * +find_unstored_payment(struct wallet *wallet, const struct sha256 *payment_hash) +{ + struct wallet_payment *i; + + list_for_each(&wallet->unstored_payments, i, list) { + if (structeq(payment_hash, &i->payment_hash)) + return i; + } + return NULL; +} + +static void remove_unstored_payment(struct wallet_payment *payment) +{ + list_del(&payment->list); +} + +void wallet_payment_setup(struct wallet *wallet, struct wallet_payment *payment) +{ + assert(!find_unstored_payment(wallet, &payment->payment_hash)); + + list_add_tail(&wallet->unstored_payments, &payment->list); + tal_add_destructor(payment, remove_unstored_payment); +} + +void wallet_payment_store(struct wallet *wallet, + const struct sha256 *payment_hash) { sqlite3_stmt *stmt; + struct wallet_payment *payment; + + payment = find_unstored_payment(wallet, payment_hash); /* Don't attempt to add the same payment twice */ assert(!payment->id); @@ -1268,14 +1296,21 @@ bool wallet_payment_add(struct wallet *wallet, sqlite3_bind_int(stmt, 5, payment->timestamp); db_exec_prepared(wallet->db, stmt); - payment->id = sqlite3_last_insert_rowid(wallet->db->sql); - return true; + + tal_free(payment); } void wallet_payment_delete(struct wallet *wallet, const struct sha256 *payment_hash) { sqlite3_stmt *stmt; + struct wallet_payment *payment; + + payment = find_unstored_payment(wallet, payment_hash); + if (payment) { + tal_free(payment); + return; + } stmt = db_prepare( wallet->db, @@ -1314,7 +1349,12 @@ wallet_payment_by_hash(const tal_t *ctx, struct wallet *wallet, const struct sha256 *payment_hash) { sqlite3_stmt *stmt; - struct wallet_payment *payment = NULL; + struct wallet_payment *payment; + + /* Present the illusion that it's in the db... */ + payment = find_unstored_payment(wallet, payment_hash); + if (payment) + return payment; stmt = db_prepare(wallet->db, "SELECT id, status, destination," @@ -1358,6 +1398,9 @@ void wallet_payment_set_status(struct wallet *wallet, { sqlite3_stmt *stmt; + /* We should never try this on an unstored payment! */ + assert(!find_unstored_payment(wallet, payment_hash)); + stmt = db_prepare(wallet->db, "UPDATE payments SET status=? " "WHERE payment_hash=?"); @@ -1382,6 +1425,8 @@ const struct wallet_payment **wallet_payment_list(const tal_t *ctx, { const struct wallet_payment **payments; sqlite3_stmt *stmt; + struct wallet_payment *p; + size_t i; payments = tal_arr(ctx, const struct wallet_payment *, 0); stmt = db_prepare( @@ -1391,12 +1436,18 @@ const struct wallet_payment **wallet_payment_list(const tal_t *ctx, "path_secrets " "FROM payments;"); - for (int i = 0; sqlite3_step(stmt) == SQLITE_ROW; i++) { + for (i = 0; sqlite3_step(stmt) == SQLITE_ROW; i++) { tal_resize(&payments, i+1); payments[i] = wallet_stmt2payment(payments, stmt); } sqlite3_finalize(stmt); + /* Now attach payments not yet in db. */ + list_for_each(&wallet->unstored_payments, p, list) { + tal_resize(&payments, i+1); + payments[i++] = p; + } + return payments; } diff --git a/wallet/wallet.h b/wallet/wallet.h index a10f7c4ae..dc8778847 100644 --- a/wallet/wallet.h +++ b/wallet/wallet.h @@ -23,6 +23,7 @@ struct wallet { struct log *log; struct ext_key *bip32_base; struct invoices *invoices; + struct list_head unstored_payments; }; /* Possible states for tracked outputs in the database. Not sure yet @@ -82,6 +83,8 @@ enum wallet_payment_status { * a UI (alongside invoices) to display the balance history. */ struct wallet_payment { + /* If it's in unstored_payments */ + struct list_node list; u64 id; u32 timestamp; struct sha256 payment_hash; @@ -530,12 +533,23 @@ struct htlc_stub *wallet_htlc_stubs(const tal_t *ctx, struct wallet *wallet, struct wallet_channel *chan); /** - * wallet_payment_add - Record a new incoming/outgoing payment + * wallet_payment_setup - Remember this payment for later committing. + * + * Either wallet_payment_store() gets called to put in db once hout + * is ready to go (and frees @payment), or @payment is tal_free'd. + * + * @wallet: wallet we're going to store it in. + * @payment: the payment for later committing. + */ +void wallet_payment_setup(struct wallet *wallet, struct wallet_payment *payment); + +/** + * wallet_payment_store - Record a new incoming/outgoing payment * * Stores the payment in the database. */ -bool wallet_payment_add(struct wallet *wallet, - struct wallet_payment *payment); +void wallet_payment_store(struct wallet *wallet, + const struct sha256 *payment_hash); /** * wallet_payment_delete - Remove a payment