From 6a3ccafaf962c2ff3a5dc1926c12b3fc3211d6b7 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Wed, 14 Feb 2018 12:23:04 +1030 Subject: [PATCH] wallet: don't implicitly remove peers, but do it explicitly. This provides a sanity check that we are in sync, and also keeps the logic in the program and out of the SQL. Since the destructor now doesn't clean up the peer, there are some wider changes to be made when cleaning up. Most notably we create lots of channels in run-wallet.c and they previously freed the peer: now we need free the peer explicitly, so we need to free them first. Suggested-by: @cdecker Signed-off-by: Rusty Russell --- lightningd/channel.c | 12 ++++++------ lightningd/channel.h | 4 +++- lightningd/lightningd.c | 16 ++++++++-------- lightningd/peer_control.c | 7 +++++++ lightningd/peer_control.h | 3 +++ wallet/test/run-wallet.c | 10 ++++++++++ wallet/wallet.c | 21 ++++++++++++++------- wallet/wallet.h | 9 ++++++--- 8 files changed, 57 insertions(+), 25 deletions(-) diff --git a/lightningd/channel.c b/lightningd/channel.c index b835964b1..238a90ff8 100644 --- a/lightningd/channel.c +++ b/lightningd/channel.c @@ -51,22 +51,22 @@ static void destroy_channel(struct channel *channel) channel_set_owner(channel, NULL); list_del_from(&channel->peer->channels, &channel->list); - - /* Last one out frees the peer */ - if (list_empty(&channel->peer->channels)) - tal_free(channel->peer); } /* This lets us give a more detailed error than just a destructor. */ void delete_channel(struct channel *channel, const char *why) { + struct peer *peer = channel->peer; if (channel->opening_cmd) { command_fail(channel->opening_cmd, "%s", why); channel->opening_cmd = NULL; } - wallet_channel_delete(channel->peer->ld->wallet, channel->dbid, - channel->peer->dbid); + wallet_channel_delete(channel->peer->ld->wallet, channel->dbid); tal_free(channel); + + /* Last one out frees the peer */ + if (list_empty(&peer->channels)) + delete_peer(peer); } /* FIXME: We have no business knowing this! */ diff --git a/lightningd/channel.h b/lightningd/channel.h index 5a68424c7..36778db39 100644 --- a/lightningd/channel.h +++ b/lightningd/channel.h @@ -86,7 +86,9 @@ struct channel { }; struct channel *new_channel(struct peer *peer, u64 dbid, u32 first_blocknum); -/* This lets us give a more detailed error than just a destructor. */ + +/* This lets us give a more detailed error than just a destructor, and + * deletes from db. */ void delete_channel(struct channel *channel, const char *why); const char *channel_state_name(const struct channel *channel); diff --git a/lightningd/lightningd.c b/lightningd/lightningd.c index be37a95c2..2b3ee7a9e 100644 --- a/lightningd/lightningd.c +++ b/lightningd/lightningd.c @@ -200,16 +200,16 @@ static void shutdown_subdaemons(struct lightningd *ld) free_htlcs(ld, NULL); while ((p = list_top(&ld->peers, struct peer, list)) != NULL) { - struct channel *c, *next; - - /* Careful: deleting last channel frees p! */ - do { - c = list_top(&p->channels, struct channel, list); - next = list_next(&p->channels, c, list); + struct channel *c; + while ((c = list_top(&p->channels, struct channel, list)) + != NULL) { + /* Removes itself from list as we free it */ tal_free(c); - c = next; - } while (c); + } + + /* Removes itself from list as we free it */ + tal_free(p); } db_commit_transaction(ld->wallet->db); } diff --git a/lightningd/peer_control.c b/lightningd/peer_control.c index a2178b55b..a4ee0accc 100644 --- a/lightningd/peer_control.c +++ b/lightningd/peer_control.c @@ -120,6 +120,13 @@ struct peer *new_peer(struct lightningd *ld, u64 dbid, return peer; } +void delete_peer(struct peer *peer) +{ + assert(list_empty(&peer->channels)); + wallet_peer_delete(peer->ld->wallet, peer->dbid); + tal_free(peer); +} + struct peer *find_peer_by_dbid(struct lightningd *ld, u64 dbid) { struct peer *p; diff --git a/lightningd/peer_control.h b/lightningd/peer_control.h index c29815bfc..fd9045e38 100644 --- a/lightningd/peer_control.h +++ b/lightningd/peer_control.h @@ -50,6 +50,9 @@ struct peer *new_peer(struct lightningd *ld, u64 dbid, const struct pubkey *id, const struct wireaddr *addr); +/* Also removes from db. */ +void delete_peer(struct peer *peer); + struct peer *peer_by_id(struct lightningd *ld, const struct pubkey *id); struct peer *peer_from_json(struct lightningd *ld, const char *buffer, diff --git a/wallet/test/run-wallet.c b/wallet/test/run-wallet.c index aaed66c19..618bd3755 100644 --- a/wallet/test/run-wallet.c +++ b/wallet/test/run-wallet.c @@ -887,6 +887,7 @@ static bool test_channel_crud(struct lightningd *ld, const tal_t *ctx) CHECK_MSG(!wallet_err, tal_fmt(w, "Load from DB: %s", wallet_err)); CHECK_MSG(channelseq(&c1, c2), "Compare loaded with saved (v1)"); + tal_free(c2); /* We just inserted them into an empty DB so this must be 1 */ CHECK(c1.dbid == 1); @@ -902,6 +903,7 @@ static bool test_channel_crud(struct lightningd *ld, const tal_t *ctx) CHECK_MSG(!wallet_err, tal_fmt(w, "Insert into DB: %s", wallet_err)); CHECK_MSG(channelseq(&c1, c2), "Compare loaded with saved (v2)"); + tal_free(c2); /* Updates should not result in new ids */ CHECK(c1.dbid == 1); @@ -918,6 +920,7 @@ static bool test_channel_crud(struct lightningd *ld, const tal_t *ctx) CHECK_MSG(!wallet_err, tal_fmt(w, "Insert into DB: %s", wallet_err)); CHECK_MSG(channelseq(&c1, c2), "Compare loaded with saved (v3)"); + tal_free(c2); /* Variant 4: update with funding_tx_id */ c1.funding_txid = hash; @@ -927,6 +930,7 @@ static bool test_channel_crud(struct lightningd *ld, const tal_t *ctx) CHECK_MSG(!wallet_err, tal_fmt(w, "Insert into DB: %s", wallet_err)); CHECK_MSG(channelseq(&c1, c2), "Compare loaded with saved (v4)"); + tal_free(c2); /* Variant 5: update with channel_info */ c1.channel_info = &ci; @@ -936,6 +940,7 @@ static bool test_channel_crud(struct lightningd *ld, const tal_t *ctx) CHECK_MSG(!wallet_err, tal_fmt(w, "Insert into DB: %s", wallet_err)); CHECK_MSG(channelseq(&c1, c2), "Compare loaded with saved (v5)"); + tal_free(c2); /* Variant 6: update with last_commit_sent */ c1.last_sent_commit = &last_commit; @@ -945,6 +950,7 @@ static bool test_channel_crud(struct lightningd *ld, const tal_t *ctx) CHECK_MSG(!wallet_err, tal_fmt(w, "Insert into DB: %s", wallet_err)); CHECK_MSG(channelseq(&c1, c2), "Compare loaded with saved (v6)"); + tal_free(c2); /* Variant 7: update with last_tx (taken from BOLT #3) */ c1.last_tx = bitcoin_tx_from_hex(w, "02000000000101bef67e4e2fb9ddeeb3461973cd4c62abb35050b1add772995b820b584a488489000000000038b02b8003a00f0000000000002200208c48d15160397c9731df9bc3b236656efb6665fbfe92b4a6878e88a499f741c4c0c62d0000000000160014ccf1af2f2aabee14bb40fa3851ab2301de843110ae8f6a00000000002200204adb4e2f00643db396dd120d4e7dc17625f5f2c11a40d857accc862d6b7dd80e040047304402206a2679efa3c7aaffd2a447fd0df7aba8792858b589750f6a1203f9259173198a022008d52a0e77a99ab533c36206cb15ad7aeb2aa72b93d4b571e728cb5ec2f6fe260147304402206d6cb93969d39177a09d5d45b583f34966195b77c7e585cf47ac5cce0c90cefb022031d71ae4e33a4e80df7f981d696fbdee517337806a3c7138b7491e2cbb077a0e01475221023da092f6980e58d2c037173180e9a465476026ee50f96695963e8efe436f54eb21030e9f7b623d2ccc7c9bd44d66d5ce21ce504c0acf6385a132cec6d3c39fa711c152ae3e195220", strlen("02000000000101bef67e4e2fb9ddeeb3461973cd4c62abb35050b1add772995b820b584a488489000000000038b02b8003a00f0000000000002200208c48d15160397c9731df9bc3b236656efb6665fbfe92b4a6878e88a499f741c4c0c62d0000000000160014ccf1af2f2aabee14bb40fa3851ab2301de843110ae8f6a00000000002200204adb4e2f00643db396dd120d4e7dc17625f5f2c11a40d857accc862d6b7dd80e040047304402206a2679efa3c7aaffd2a447fd0df7aba8792858b589750f6a1203f9259173198a022008d52a0e77a99ab533c36206cb15ad7aeb2aa72b93d4b571e728cb5ec2f6fe260147304402206d6cb93969d39177a09d5d45b583f34966195b77c7e585cf47ac5cce0c90cefb022031d71ae4e33a4e80df7f981d696fbdee517337806a3c7138b7491e2cbb077a0e01475221023da092f6980e58d2c037173180e9a465476026ee50f96695963e8efe436f54eb21030e9f7b623d2ccc7c9bd44d66d5ce21ce504c0acf6385a132cec6d3c39fa711c152ae3e195220")); @@ -955,6 +961,7 @@ static bool test_channel_crud(struct lightningd *ld, const tal_t *ctx) CHECK_MSG(!wallet_err, tal_fmt(w, "Insert into DB: %s", wallet_err)); CHECK_MSG(channelseq(&c1, c2), "Compare loaded with saved (v7)"); + tal_free(c2); /* Variant 8: update and add remote_shutdown_scriptpubkey */ c1.remote_shutdown_scriptpubkey = scriptpubkey; @@ -965,10 +972,13 @@ static bool test_channel_crud(struct lightningd *ld, const tal_t *ctx) CHECK_MSG(!wallet_err, tal_fmt(w, "Insert into DB: %s", wallet_err)); CHECK_MSG(channelseq(&c1, c2), "Compare loaded with saved (v8)"); + tal_free(c2); db_commit_transaction(w->db); CHECK(!wallet_err); tal_free(w); + /* Normally freed by destroy_channel, but we don't call that */ + tal_free(p); return true; } diff --git a/wallet/wallet.c b/wallet/wallet.c index 7e18de02f..361d87845 100644 --- a/wallet/wallet.c +++ b/wallet/wallet.c @@ -884,21 +884,28 @@ void wallet_channel_save(struct wallet *w, struct channel *chan) tal_free(tmpctx); } -void wallet_channel_delete(struct wallet *w, u64 wallet_id, u64 peer_dbid) +void wallet_channel_delete(struct wallet *w, u64 wallet_id) { sqlite3_stmt *stmt; - /* FIXME: The line to clean up if we're last channel for peer would - * be better as a trigger. */ stmt = db_prepare(w->db, "DELETE FROM channels WHERE id=?"); sqlite3_bind_int64(stmt, 1, wallet_id); db_exec_prepared(w->db, stmt); +} - stmt = db_prepare(w->db, - "DELETE FROM peers WHERE id = ? AND NOT EXISTS" - " (SELECT 1 FROM channels WHERE peer_id = ?)"); +void wallet_peer_delete(struct wallet *w, u64 peer_dbid) +{ + sqlite3_stmt *stmt; + + /* Must not have any channels still using this peer */ + stmt = db_query(__func__, w->db, + "SELECT * FROM channels WHERE peer_id = %"PRIu64, + peer_dbid); + assert(sqlite3_step(stmt) == SQLITE_DONE); + sqlite3_finalize(stmt); + + stmt = db_prepare(w->db, "DELETE FROM peers WHERE id=?"); sqlite3_bind_int64(stmt, 1, peer_dbid); - sqlite3_bind_int64(stmt, 2, peer_dbid); db_exec_prepared(w->db, stmt); } diff --git a/wallet/wallet.h b/wallet/wallet.h index 0fadfc58a..6120852b4 100644 --- a/wallet/wallet.h +++ b/wallet/wallet.h @@ -216,10 +216,13 @@ void wallet_channel_save(struct wallet *w, struct channel *chan); /** * wallet_channel_delete -- After resolving a channel, forget about it - * - * Deletes peer too if no more channels. */ -void wallet_channel_delete(struct wallet *w, u64 wallet_id, u64 peer_dbid); +void wallet_channel_delete(struct wallet *w, u64 wallet_id); + +/** + * wallet_peer_delete -- After no more channels in peer, forget about it + */ +void wallet_peer_delete(struct wallet *w, u64 peer_dbid); /** * wallet_channel_config_save -- Upsert a channel_config into the database