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 <rusty@rustcorp.com.au>

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,
+	wallet_channel_delete(channel->peer->ld->wallet, channel->dbid);
-			      channel->peer->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! */

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);

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;
+		struct channel *c;
-
-		/* Careful: deleting last channel frees p! */
-		do {
-			c = list_top(&p->channels, struct channel, list);
-			next = list_next(&p->channels, c, list);
 
+		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);
 }

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;

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,

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;
 }
 

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,
+void wallet_peer_delete(struct wallet *w, u64 peer_dbid)
-			  "DELETE FROM peers WHERE id = ? AND NOT EXISTS"
+{
-			  " (SELECT 1 FROM channels WHERE peer_id = ?)");
+	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);
 }
 

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