wallet: preserve released migration indices

tests/test_splicing.py

@@ -92,10 +92,13 @@ def test_splice_stored_feerate_repaired_on_upgrade(node_factory, bitcoind,
     l1.db_manip("UPDATE channel_funding_inflights"
                 " SET funding_feerate = {}".format(poison))
 
-    # Rewind past the two clamping migrations so they run again over the row
-    # we just planted, which is the upgrade an attacked node goes through.
-    # They are plain idempotent UPDATEs, so re-running them is safe.
-    l1.db_manip("UPDATE version SET version = version - 2")
+    # Recreate the schema immediately before the two clamping migrations so
+    # they run again over the row we just planted.  Drop the schema added by
+    # later, non-idempotent migrations before rewinding to version 282.
+    l1.db_manip("DROP TABLE our_outputs")
+    l1.db_manip("DROP TABLE our_txs")
+    l1.db_manip("ALTER TABLE payments DROP COLUMN failmsg")
+    l1.db_manip("UPDATE version SET version = 282")
     l1.daemon.opts['database-upgrade'] = 'true'
     l1.start()
 

wallet/migrations.c

@@ -1139,6 +1139,43 @@ static const struct db_migration dbmigrations[] = {
      SQL("ALTER TABLE offers DROP COLUMN force_paths"), NULL},
     /* ^v26.04 */
 
+    /* Nothing used to bound the feerate we record for an inflight funding
+     * transaction, so a broken fee estimator (ours or a peer's) could get an
+     * absurd value in here.  That is not merely cosmetic: the BOLT #2 rule
+     * that the next RBF attempt pays 25/24 times the last feerate is computed
+     * on a u32, so anything above UINT_MAX/25 overflows and trips the
+     * assert(next_feerate > last_feerate) in listpeerchannels, which plugins
+     * call at startup: the node crash-loops with no way out but to rewrite the
+     * stored value.  A stored 0 trips the assert immediately above it.
+     *
+     * The preceding commits close every path such a value could arrive on.
+     * This repairs what is already there, so that from here the bounds hold
+     * for stored feerates too and the read path can rely on them.
+     *
+     * We write the feerate with db_bind_int(), so a u32 above INT_MAX reads
+     * back negative; those are exactly the values that overflow, hence the
+     * second clause below.
+     *
+     * The bounds are spelled out rather than written as FEERATE_CEILING and
+     * FEERATE_FLOOR on purpose: a migration has to keep doing exactly what it
+     * did on the day it shipped, so it must not move when those constants do.
+     *
+     * Rewriting is safe: this feerate only sanity checks the fee the funding
+     * transaction already pays, and tells the user what the next RBF must
+     * beat.  It never feeds anything we have signed.
+     *
+     * These shipped in v26.06.8 as migrations 283 and 284. */
+    {SQL("UPDATE channel_funding_inflights"
+	 " SET funding_feerate = 1000000"
+	 " WHERE funding_feerate > 1000000 OR funding_feerate < 0;"), NULL,
+     /* Clamping is idempotent, so no revert needed */
+     NULL, NULL},
+    {SQL("UPDATE channel_funding_inflights"
+	 " SET funding_feerate = 253"
+	 " WHERE funding_feerate = 0 OR funding_feerate IS NULL;"), NULL,
+     /* Clamping is idempotent, so no revert needed */
+     NULL, NULL},
+
     /* Parallel wallet tables without the blocks(height) FK that
      * utxoset/transactions carry, so bwatch-driven writes don't need a
      * blocks table.  Legacy tables stay for one release to keep downgrade
@@ -1186,40 +1223,6 @@ static const struct db_migration dbmigrations[] = {
      * after the failure was recorded (issue #9341). */
     {SQL("ALTER TABLE payments ADD failmsg BLOB;"), NULL,
      SQL("ALTER TABLE payments DROP COLUMN failmsg"), NULL},
-    /* Nothing used to bound the feerate we record for an inflight funding
-     * transaction, so a broken fee estimator (ours or a peer's) could get an
-     * absurd value in here.  That is not merely cosmetic: the BOLT #2 rule
-     * that the next RBF attempt pays 25/24 times the last feerate is computed
-     * on a u32, so anything above UINT_MAX/25 overflows and trips the
-     * assert(next_feerate > last_feerate) in listpeerchannels, which plugins
-     * call at startup: the node crash-loops with no way out but to rewrite the
-     * stored value.  A stored 0 trips the assert immediately above it.
-     *
-     * The preceding commits close every path such a value could arrive on.
-     * This repairs what is already there, so that from here the bounds hold
-     * for stored feerates too and the read path can rely on them.
-     *
-     * We write the feerate with db_bind_int(), so a u32 above INT_MAX reads
-     * back negative; those are exactly the values that overflow, hence the
-     * second clause below.
-     *
-     * The bounds are spelled out rather than written as FEERATE_CEILING and
-     * FEERATE_FLOOR on purpose: a migration has to keep doing exactly what it
-     * did on the day it shipped, so it must not move when those constants do.
-     *
-     * Rewriting is safe: this feerate only sanity checks the fee the funding
-     * transaction already pays, and tells the user what the next RBF must
-     * beat.  It never feeds anything we have signed. */
-    {SQL("UPDATE channel_funding_inflights"
-	 " SET funding_feerate = 1000000"
-	 " WHERE funding_feerate > 1000000 OR funding_feerate < 0;"), NULL,
-     /* Clamping is idempotent, so no revert needed */
-     NULL, NULL},
-    {SQL("UPDATE channel_funding_inflights"
-	 " SET funding_feerate = 253"
-	 " WHERE funding_feerate = 0 OR funding_feerate IS NULL;"), NULL,
-     /* Clamping is idempotent, so no revert needed */
-     NULL, NULL},
     /* ^v26.09 */
 };
 

wallet/test/run-db.c

@@ -456,6 +456,42 @@ static bool test_empty_db_migrate(struct lightningd *ld)
 	return true;
 }
 
+static bool test_v26_06_8_migrate(struct lightningd *ld)
+{
+	struct db *db = create_test_db();
+	struct db_stmt *stmt;
+	const struct ext_key *bip32_base = NULL;
+
+	CHECK(db);
+	db_begin_transaction(db);
+	db_migrate(ld, db, bip32_base);
+
+	/* v26.06.8 used migrations 283 and 284 for the feerate repairs.
+	 * Recreate its schema at that version and verify master starts its new
+	 * migrations at 285 rather than skipping the wallet table creation. */
+	stmt = db_prepare_v2(db, SQL("DROP TABLE our_outputs;"));
+	db_exec_prepared_v2(take(stmt));
+	stmt = db_prepare_v2(db, SQL("DROP TABLE our_txs;"));
+	db_exec_prepared_v2(take(stmt));
+	stmt = db_prepare_v2(db, SQL("ALTER TABLE payments DROP COLUMN failmsg;"));
+	db_exec_prepared_v2(take(stmt));
+	stmt = db_prepare_v2(db, SQL("UPDATE version SET version=284;"));
+	db_exec_prepared_v2(take(stmt));
+	db_commit_transaction(db);
+
+	ld->db_upgrade_ok = tal(ld, bool);
+	*ld->db_upgrade_ok = true;
+	db_begin_transaction(db);
+	db_migrate(ld, db, bip32_base);
+	CHECK(db_get_version(db) == ARRAY_SIZE(dbmigrations) - 1);
+	db_commit_transaction(db);
+	tal_free(ld->db_upgrade_ok);
+	ld->db_upgrade_ok = NULL;
+
+	tal_free(db);
+	return true;
+}
+
 static bool test_primitives(void)
 {
 	struct db_stmt *stmt;
@@ -602,6 +638,7 @@ int main(int argc, char *argv[])
 	/* We do a runtime test here, so we still check compile! */
 	if (HAVE_SQLITE3) {
 		ok &= test_empty_db_migrate(ld);
+		ok &= test_v26_06_8_migrate(ld);
 		ok &= test_vars(ld);
 		ok &= test_primitives();
 		ok &= test_manip_columns();