wallet: don't use rowid for the channel's DBID.

We derive the seed from this, so it needs to be unique, but using
rowid forced us to put the channel into the db early, before it
was ready.

Instead, use a counter to ensure uniqueness, initialized when we load
existing peers.  This doesn't need to touch the database at all.

As we now have only two places where the channel is committed (the
funder and fundee paths), so we create a new explicit
'wallet_channel_insert()' function: 'wallet_channel_save()' now just
updates.

Note that this also fixes some weirdness in
wallet_channels_load_active: we strangely avoided loading channels in
CLOSINGD_COMPLETE (which fortunately was a transient state, so
unlikely anyone hit this).  Note that since the lines above already
delete all the OPENINGD channels, we now simply load them all.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

lightningd/channel.c

@@ -84,7 +84,7 @@ void delete_channel(struct channel *channel, const char *why)
  * reconnection. We use the DB channel ID to guarantee unique secrets
  * per channel.
  */
-void derive_channel_seed(struct lightningd *ld, struct privkey *seed,
+static void derive_channel_seed(struct lightningd *ld, struct privkey *seed,
 				const struct pubkey *peer_id,
 				const u64 dbid)
 {
@@ -106,6 +106,7 @@ struct channel *new_channel(struct peer *peer, u64 dbid, u32 first_blocknum)
 	struct channel *channel = talz(peer->ld, struct channel);
 	char *idname;
 
+	assert(dbid != 0);
 	channel->dbid = dbid;
 	channel->peer = peer;
 	channel->first_blocknum = first_blocknum;
@@ -120,9 +121,7 @@ struct channel *new_channel(struct peer *peer, u64 dbid, u32 first_blocknum)
 	tal_free(idname);
 	list_add_tail(&peer->channels, &channel->list);
 	tal_add_destructor(channel, destroy_channel);
-	if (channel->dbid != 0)
+	derive_channel_seed(peer->ld, &channel->seed, &peer->id, channel->dbid);
-		derive_channel_seed(peer->ld, &channel->seed, &peer->id,
-				    channel->dbid);
 
 	return channel;
 }

lightningd/channel.h

@@ -95,10 +95,6 @@ const char *channel_state_name(const struct channel *channel);
 
 void channel_set_owner(struct channel *channel, struct subd *owner);
 
-void derive_channel_seed(struct lightningd *ld, struct privkey *seed,
-			 const struct pubkey *peer_id,
-			 const u64 dbid);
-
 /* Channel has failed, but can try again. */
 PRINTF_FMT(2,3) void channel_fail_transient(struct channel *channel,
 					    const char *fmt,...);

lightningd/peer_control.c

@@ -1436,6 +1436,8 @@ static void opening_got_hsm_funding_sig(struct funding_channel *fc,
 		fatal("HSM gave bad sign_funding_reply %s",
 		      tal_hex(fc, resp));
 
+	wallet_channel_insert(ld->wallet, channel);
+
 	/* Send it out and watch for confirms. */
 	broadcast_tx(ld->topology, channel, tx, funding_broadcast_failed);
 	watch_tx(channel, ld->topology, channel, tx, funding_lockin_cb, NULL);
@@ -2219,11 +2221,14 @@ static void opening_fundee_finished(struct subd *opening,
 	/* old_remote_per_commit not valid yet, copy valid one. */
 	channel_info->old_remote_per_commit = channel_info->remote_per_commit;
 
+	channel_commit_initial(channel);
+
+	/* Now we finally put it in the database. */
+	wallet_channel_insert(ld->wallet, channel);
+
 	/* Now, keep the initial commit as our last-tx-to-broadcast. */
 	channel_set_last_tx(channel, remote_commit, &remote_commit_sig);
 
-	channel_commit_initial(channel);
-
 	log_debug(channel->log, "Watching funding tx %s",
 		     type_to_string(reply, struct bitcoin_txid,
 				    channel->funding_txid));
@@ -2281,6 +2286,24 @@ static unsigned int opening_negotiation_failed(struct subd *openingd,
 	return 0;
 }
 
+static struct channel *new_uncommitted_channel(struct lightningd *ld,
+					       const struct pubkey *peer_id,
+					       const struct wireaddr *addr)
+{
+	struct peer *peer;
+	struct channel *channel;
+
+	/* We make a new peer if necessary. */
+	peer = peer_by_id(ld, peer_id);
+	if (!peer)
+		peer = new_peer(ld, 0, peer_id, addr);
+
+	channel = new_channel(peer, wallet_get_channel_dbid(ld->wallet),
+			      get_block_height(ld->topology));
+	assert(channel->peer == peer);
+	return channel;
+}
+
 /* Peer has spontaneously exited from gossip due to open msg */
 static void peer_accept_channel(struct lightningd *ld,
 				const struct pubkey *peer_id,
@@ -2294,18 +2317,11 @@ static void peer_accept_channel(struct lightningd *ld,
 	u32 max_to_self_delay, max_minimum_depth;
 	u64 min_effective_htlc_capacity_msat;
 	u8 *msg;
-	struct peer *peer;
 	struct channel *channel;
 
 	assert(fromwire_peektype(open_msg) == WIRE_OPEN_CHANNEL);
 
-	/* We make a new peer if necessary. */
+	channel = new_uncommitted_channel(ld, peer_id, addr);
-	peer = peer_by_id(ld, peer_id);
-	if (!peer)
-		peer = new_peer(ld, 0, peer_id, addr);
-
-	channel = new_channel(peer, 0, get_block_height(ld->topology));
-	assert(channel->peer == peer);
 
 	channel_set_state(channel, UNINITIALIZED, OPENINGD);
 	channel_set_owner(channel,
@@ -2336,10 +2352,6 @@ static void peer_accept_channel(struct lightningd *ld,
 		       &max_to_self_delay, &max_minimum_depth,
 		       &min_effective_htlc_capacity_msat);
 
-	/* Store the channel in the database in order to get a channel
-	 * ID that is unique and which we can base the peer_seed on */
-	wallet_channel_save(ld->wallet, channel);
-
 	msg = towire_opening_init(channel, get_chainparams(ld)->index,
 				  &channel->our_config,
 				  max_to_self_delay,
@@ -2374,16 +2386,8 @@ static void peer_offer_channel(struct lightningd *ld,
 	u8 *msg;
 	u32 max_to_self_delay, max_minimum_depth;
 	u64 min_effective_htlc_capacity_msat;
-	struct peer *peer;
-
-	/* We make a new peer if necessary. */
-	peer = peer_by_id(ld, &fc->peerid);
-	if (!peer)
-		peer = new_peer(ld, 0, &fc->peerid, addr);
-
-	fc->channel = new_channel(peer, 0, get_block_height(ld->topology));
-	assert(fc->channel->peer == peer);
 
+	fc->channel = new_uncommitted_channel(ld, &fc->peerid, addr);
 	fc->channel->funding_satoshi = fc->funding_satoshi;
 	fc->channel->push_msat = fc->push_msat;
 
@@ -2403,17 +2407,6 @@ static void peer_offer_channel(struct lightningd *ld,
 		return;
 	}
 
-	/* FIXME: This is wrong in several ways.
-	 *
-	 * 1. We should set the temporary channel id *now*, so that's the
-	 *    key.
-	 * 2. We don't need the peer or channel in db until peer_persists().
-	 */
-
-	/* Store the channel in the database in order to get a channel
-	 * ID that is unique and which we can base the peer_seed on */
-	wallet_channel_save(ld->wallet, fc->channel);
-
 	/* We will fund channel */
 	fc->channel->funder = LOCAL;
 	channel_config(ld, &fc->channel->our_config,

lightningd/peer_state.h

@@ -33,5 +33,6 @@ enum peer_state {
 	ONCHAIND_OUR_UNILATERAL,
 	ONCHAIND_MUTUAL
 };
+#define CHANNEL_STATE_MAX ONCHAIND_MUTUAL
 
 #endif /* LIGHTNING_LIGHTNINGD_PEER_STATE_H */

wallet/test/run-wallet.c

@@ -611,7 +611,7 @@ static struct wallet *create_test_wallet(struct lightningd *ld, const tal_t *ctx
 	CHECK_MSG(w->db, "Failed opening the db");
 	db_migrate(w->db, w->log);
 	CHECK_MSG(!wallet_err, "DB migration failed");
-
+	w->max_channel_dbid = 0;
 
 	return w;
 }
@@ -864,6 +864,7 @@ static bool test_channel_crud(struct lightningd *ld, const tal_t *ctx)
 	c1.first_blocknum = 1;
 	p = new_peer(ld, 0, &pk, NULL);
 	c1.peer = p;
+	c1.dbid = wallet_get_channel_dbid(w);
 	c1.our_msatoshi = NULL;
 	c1.last_tx = NULL;
 	c1.state = CHANNELD_NORMAL;
@@ -879,6 +880,8 @@ static bool test_channel_crud(struct lightningd *ld, const tal_t *ctx)
 	db_begin_transaction(w->db);
 	CHECK(!wallet_err);
 
+	wallet_channel_insert(w, &c1);
+
 	/* Variant 1: insert with null for scid, funding_tx_id, channel_info, last_tx */
 	wallet_channel_save(w, &c1);
 	CHECK_MSG(!wallet_err,

wallet/wallet.c

@@ -525,6 +525,7 @@ static struct channel *wallet_stmt2channel(const tal_t *ctx, struct wallet *w, s
 	remote_config_id = sqlite3_column_int64(stmt, 4);
 
 	chan->state = sqlite3_column_int(stmt, 5);
+	assert(chan->state > OPENINGD && chan->state <= CHANNEL_STATE_MAX);
 	chan->funder = sqlite3_column_int(stmt, 6);
 	chan->channel_flags = sqlite3_column_int(stmt, 7);
 	chan->minimum_depth = sqlite3_column_int(stmt, 8);
@@ -647,11 +648,12 @@ bool wallet_channels_load_active(const tal_t *ctx, struct wallet *w)
 	sqlite3_bind_int64(stmt, 1, OPENINGD);
 	db_exec_prepared(w->db, stmt);
 
-	/* Channels are active if they have reached at least the
+	/* We load all channels */
-	 * opening state and they are not marked as complete */
 	stmt = db_query(
-	    __func__, w->db, "SELECT %s FROM channels WHERE state > %d AND state != %d;",
+	    __func__, w->db, "SELECT %s FROM channels;",
-	    channel_fields, OPENINGD, CLOSINGD_COMPLETE);
+	    channel_fields);
+
+	w->max_channel_dbid = 0;
 
 	int count = 0;
 	while (ok && stmt && sqlite3_step(stmt) == SQLITE_ROW) {
@@ -660,6 +662,8 @@ bool wallet_channels_load_active(const tal_t *ctx, struct wallet *w)
 			ok = false;
 			break;
 		}
+		if (c->dbid > w->max_channel_dbid)
+			w->max_channel_dbid = c->dbid;
 		count++;
 	}
 	log_debug(w->log, "Loaded %d channels from DB", count);
@@ -781,38 +785,17 @@ bool wallet_channel_config_load(struct wallet *w, const u64 id,
 	return ok;
 }
 
+u64 wallet_get_channel_dbid(struct wallet *wallet)
+{
+	return ++wallet->max_channel_dbid;
+}
+
 void wallet_channel_save(struct wallet *w, struct channel *chan)
 {
 	tal_t *tmpctx = tal_tmpctx(w);
 	sqlite3_stmt *stmt;
 	assert(chan->first_blocknum);
 
-	if (chan->peer->dbid == 0) {
-		/* Need to store the peer first */
-		stmt = db_prepare(w->db, "INSERT INTO peers (node_id, address) VALUES (?, ?);");
-		sqlite3_bind_pubkey(stmt, 1, &chan->peer->id);
-		if (chan->peer->addr.type == ADDR_TYPE_PADDING)
-			sqlite3_bind_null(stmt, 2);
-		else
-			sqlite3_bind_text(stmt, 2,
-					  type_to_string(tmpctx, struct wireaddr, &chan->peer->addr),
-					  -1, SQLITE_TRANSIENT);
-		db_exec_prepared(w->db, stmt);
-		chan->peer->dbid = sqlite3_last_insert_rowid(w->db->sql);
-	}
-
-	/* Insert a stub, that we can update, unifies INSERT and UPDATE paths */
-	if (chan->dbid == 0) {
-		stmt = db_prepare(w->db, "INSERT INTO channels ("
-				  "peer_id, first_blocknum) VALUES (?, ?);");
-		sqlite3_bind_int64(stmt, 1, chan->peer->dbid);
-		sqlite3_bind_int(stmt, 2, chan->first_blocknum);
-		db_exec_prepared(w->db, stmt);
-		chan->dbid = sqlite3_last_insert_rowid(w->db->sql);
-		derive_channel_seed(w->ld, &chan->seed, &chan->peer->id,
-				    chan->dbid);
-	}
-
 	/* Need to initialize the shachain first so we get an id */
 	if (chan->their_shachain.id == 0) {
 		wallet_shachain_init(w, &chan->their_shachain);
@@ -820,7 +803,6 @@ void wallet_channel_save(struct wallet *w, struct channel *chan)
 
 	wallet_channel_config_save(w, &chan->our_config);
 
-	/* Now do the real update */
 	stmt = db_prepare(w->db, "UPDATE channels SET"
 			  "  shachain_remote_id=?,"
 			  "  short_channel_id=?,"
@@ -925,6 +907,38 @@ void wallet_channel_save(struct wallet *w, struct channel *chan)
 	tal_free(tmpctx);
 }
 
+void wallet_channel_insert(struct wallet *w, struct channel *chan)
+{
+	tal_t *tmpctx = tal_tmpctx(w);
+	sqlite3_stmt *stmt;
+
+	if (chan->peer->dbid == 0) {
+		/* Need to create the peer first */
+		stmt = db_prepare(w->db, "INSERT INTO peers (node_id, address) VALUES (?, ?);");
+		sqlite3_bind_pubkey(stmt, 1, &chan->peer->id);
+		if (chan->peer->addr.type == ADDR_TYPE_PADDING)
+			sqlite3_bind_null(stmt, 2);
+		else
+			sqlite3_bind_text(stmt, 2,
+					  type_to_string(tmpctx, struct wireaddr, &chan->peer->addr),
+					  -1, SQLITE_TRANSIENT);
+		db_exec_prepared(w->db, stmt);
+		chan->peer->dbid = sqlite3_last_insert_rowid(w->db->sql);
+	}
+
+	/* Insert a stub, that we update, unifies INSERT and UPDATE paths */
+	stmt = db_prepare(w->db, "INSERT INTO channels ("
+			  "peer_id, first_blocknum, id) VALUES (?, ?, ?);");
+	sqlite3_bind_int64(stmt, 1, chan->peer->dbid);
+	sqlite3_bind_int(stmt, 2, chan->first_blocknum);
+	sqlite3_bind_int(stmt, 3, chan->dbid);
+	db_exec_prepared(w->db, stmt);
+
+	/* Now save path as normal */
+	wallet_channel_save(w, chan);
+	tal_free(tmpctx);
+}
+
 void wallet_channel_delete(struct wallet *w, u64 wallet_id)
 {
 	sqlite3_stmt *stmt;

wallet/wallet.h

@@ -30,6 +30,7 @@ struct wallet {
 	struct ext_key *bip32_base;
 	struct invoices *invoices;
 	struct list_head unstored_payments;
+	u64 max_channel_dbid;
 };
 
 /* Possible states for tracked outputs in the database. Not sure yet
@@ -206,6 +207,13 @@ bool wallet_shachain_load(struct wallet *wallet, u64 id,
 			  struct wallet_shachain *chain);
 
 
+/**
+ * wallet_get_uncommitted_channel_dbid -- get a unique channel dbid
+ *
+ * @wallet: the wallet
+ */
+u64 wallet_get_channel_dbid(struct wallet *wallet);
+
 /**
  * wallet_channel_save -- Upsert the channel into the database
  *
@@ -215,6 +223,14 @@ bool wallet_shachain_load(struct wallet *wallet, u64 id,
  */
 void wallet_channel_save(struct wallet *w, struct channel *chan);
 
+/**
+ * wallet_channel_insert -- Insert the initial channel into the database
+ *
+ * @wallet: the wallet to save into
+ * @chan: the instance to store
+ */
+void wallet_channel_insert(struct wallet *w, struct channel *chan);
+
 /**
  * wallet_channel_delete -- After resolving a channel, forget about it
  */