db: save and restore last_sent_commit correctly.

It's an array: we were only saving the single element; if there was more than
one changed HTLC we'd get a bad signature!

The report in ElementsProject#1907 is probably caused by the other side re-requesting
something we considered already finalized; to avoid this particular error,
we should set the field to NULL if there's no last_sent_commit.

I'm increasingly of the opinion we want to just save all the update
packets to the db and blast them out, instead of doing this
second-guessing dance.

Fixes: ElementsProject#1907
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

Makefile

@@ -35,7 +35,7 @@ endif
 
 ifeq ($(COMPAT),1)
 # We support compatibility with pre-0.6.
-COMPAT_CFLAGS=-DCOMPAT_V052=1
+COMPAT_CFLAGS=-DCOMPAT_V052=1 -DCOMPAT_V060=1
 endif
 
 PYTEST_OPTS := -v -x

tests/test_connection.py

@@ -1297,7 +1297,6 @@ def test_dataloss_protection(node_factory, bitcoind):
     assert (closetxid, "confirmed") in set([(o['txid'], o['status']) for o in l2.rpc.listfunds()['outputs']])
 
 
-@pytest.mark.xfail(strict=True)
 @unittest.skipIf(not DEVELOPER, "needs dev_disconnect")
 def test_restart_multi_htlc_rexmit(node_factory, bitcoind, executor):
     # l1 disables commit timer once we send first htlc, dies on commit

wallet/db.c

@@ -337,6 +337,8 @@ char *dbmigrations[] = {
     "ALTER TABLE payments ADD description TEXT;",
     /* future_per_commitment_point if other side proves we're out of date -- */
     "ALTER TABLE channels ADD future_per_commitment_point BLOB;",
+    /* last_sent_commit array fix */
+    "ALTER TABLE channels ADD last_sent_commit BLOB;",
     NULL,
 };
 

wallet/test/Makefile

@@ -6,6 +6,7 @@ WALLET_TEST_COMMON_OBJS :=			\
 	common/base32.o				\
 	common/derive_basepoints.o		\
 	common/htlc_state.o			\
+	common/htlc_wire.o			\
 	common/type_to_string.o			\
 	common/memleak.o			\
 	common/key_derive.o			\

wallet/test/run-wallet.c

@@ -747,9 +747,10 @@ static bool channelseq(struct channel *c1, struct channel *c2)
 
 	CHECK(c1->our_config.id != 0 && c1->our_config.id == c2->our_config.id);
 	CHECK((lc1 != NULL) ==  (lc2 != NULL));
-	if(lc1) {
-		CHECK(lc1->newstate == lc2->newstate);
-		CHECK(lc1->id == lc2->id);
+	CHECK(tal_count(lc1) == tal_count(lc2));
+	for (size_t i = 0; i < tal_count(lc1); i++) {
+		CHECK(lc1[i].newstate == lc2[i].newstate);
+		CHECK(lc1[i].id == lc2[i].id);
 	}
 
 	CHECK((c1->last_tx != NULL) ==  (c2->last_tx != NULL));
@@ -795,7 +796,7 @@ static bool test_channel_crud(struct lightningd *ld, const tal_t *ctx)
 	struct channel_info *ci = &c1.channel_info;
 	struct bitcoin_txid *hash = tal(w, struct bitcoin_txid);
 	struct pubkey pk;
-	struct changed_htlc last_commit;
+	struct changed_htlc *last_commit;
 	secp256k1_ecdsa_signature *sig = tal(w, secp256k1_ecdsa_signature);
 	u8 *scriptpubkey = tal_arr(ctx, u8, 100);
 
@@ -804,7 +805,8 @@ static bool test_channel_crud(struct lightningd *ld, const tal_t *ctx)
 	memset(ci, 3, sizeof(*ci));
 	mempat(hash, sizeof(*hash));
 	mempat(sig, sizeof(*sig));
-	mempat(&last_commit, sizeof(last_commit));
+	last_commit = tal_arr(w, struct changed_htlc, 2);
+	mempat(last_commit, tal_bytelen(last_commit));
 	pubkey_from_der(tal_hexdata(w, "02a1633cafcc01ebfb6d78e39f687a1f0995c62fc95f51ead10a02ee0be551b5dc", 66), 33, &pk);
 	ci->feerate_per_kw[LOCAL] = ci->feerate_per_kw[REMOTE] = 31337;
 	mempat(scriptpubkey, tal_count(scriptpubkey));
@@ -864,7 +866,7 @@ static bool test_channel_crud(struct lightningd *ld, const tal_t *ctx)
 	CHECK(c1.their_shachain.id == 1);
 
 	/* Variant 3: update with last_commit_sent */
-	c1.last_sent_commit = &last_commit;
+	c1.last_sent_commit = last_commit;
 	wallet_channel_save(w, &c1);
 	CHECK_MSG(!wallet_err, tal_fmt(w, "Insert into DB: %s", wallet_err));
 	CHECK_MSG(c2 = wallet_channel_load(w, c1.dbid), tal_fmt(w, "Load from DB"));

wallet/wallet.c

@@ -609,13 +609,26 @@ static struct channel *wallet_stmt2channel(const tal_t *ctx, struct wallet *w, s
 	remote_shutdown_scriptpubkey = sqlite3_column_arr(tmpctx, stmt, 28, u8);
 
 	/* Do we have a last_sent_commit, if yes, populate */
-	if (sqlite3_column_type(stmt, 30) != SQLITE_NULL) {
+	if (sqlite3_column_type(stmt, 41) != SQLITE_NULL) {
+		const u8 *cursor = sqlite3_column_blob(stmt, 41);
+		size_t len = sqlite3_column_bytes(stmt, 41);
+		size_t n = 0;
+		last_sent_commit = tal_arr(tmpctx, struct changed_htlc, n);
+		while (len) {
+			tal_resize(&last_sent_commit, n+1);
+			fromwire_changed_htlc(&cursor, &len,
+					      &last_sent_commit[n++]);
+		}
+	} else
+		last_sent_commit = NULL;
+
+#ifdef COMPAT_V060
+	if (!last_sent_commit && sqlite3_column_type(stmt, 30) != SQLITE_NULL) {
 		last_sent_commit = tal(tmpctx, struct changed_htlc);
 		last_sent_commit->newstate = sqlite3_column_int64(stmt, 30);
 		last_sent_commit->id = sqlite3_column_int64(stmt, 31);
-	} else {
-		last_sent_commit = NULL;
 	}
+#endif
 
 	if (sqlite3_column_type(stmt, 40) != SQLITE_NULL) {
 		future_per_commitment_point = tal(tmpctx, struct pubkey);
@@ -715,7 +728,8 @@ static const char *channel_fields =
     /*30*/ "last_sent_commit_state, last_sent_commit_id, "
     /*32*/ "last_tx, last_sig, last_was_revoke, first_blocknum, "
     /*36*/ "min_possible_feerate, max_possible_feerate, "
-    /*38*/ "msatoshi_to_us_min, msatoshi_to_us_max, future_per_commitment_point ";
+    /*38*/ "msatoshi_to_us_min, msatoshi_to_us_max, future_per_commitment_point, "
+    /*41*/ "last_sent_commit";
 
 bool wallet_channels_load_active(const tal_t *ctx, struct wallet *w)
 {
@@ -893,6 +907,7 @@ u64 wallet_get_channel_dbid(struct wallet *wallet)
 void wallet_channel_save(struct wallet *w, struct channel *chan)
 {
 	sqlite3_stmt *stmt;
+	u8 *last_sent_commit;
 	assert(chan->first_blocknum);
 
 	wallet_channel_config_save(w, &chan->our_config);
@@ -996,17 +1011,23 @@ void wallet_channel_save(struct wallet *w, struct channel *chan)
 	db_exec_prepared(w->db, stmt);
 
 	/* If we have a last_sent_commit, store it */
-	if (chan->last_sent_commit) {
-		stmt = db_prepare(w->db,
-				  "UPDATE channels SET"
-				  "  last_sent_commit_state=?,"
-				  "  last_sent_commit_id=?"
-				  " WHERE id=?");
-		sqlite3_bind_int(stmt, 1, chan->last_sent_commit->newstate);
-		sqlite3_bind_int64(stmt, 2, chan->last_sent_commit->id);
-		sqlite3_bind_int64(stmt, 3, chan->dbid);
-		db_exec_prepared(w->db, stmt);
-	}
+	last_sent_commit = tal_arr(tmpctx, u8, 0);
+	for (size_t i = 0; i < tal_count(chan->last_sent_commit); i++)
+		towire_changed_htlc(&last_sent_commit,
+				    &chan->last_sent_commit[i]);
+
+	stmt = db_prepare(w->db,
+			  "UPDATE channels SET"
+			  "  last_sent_commit=?"
+			  " WHERE id=?");
+	if (tal_count(last_sent_commit))
+		sqlite3_bind_blob(stmt, 1,
+				  last_sent_commit, tal_count(last_sent_commit),
+				  SQLITE_TRANSIENT);
+	else
+		sqlite3_bind_null(stmt, 1);
+	sqlite3_bind_int64(stmt, 2, chan->dbid);
+	db_exec_prepared(w->db, stmt);
 }
 
 void wallet_channel_insert(struct wallet *w, struct channel *chan)