db: we must always be in a transaction, remove nested, call fatal()

We save location where transaction was started, in case we try to nest.
There's now no error case; db_exec_mayfail() is the only one.

This means the tests need to override fatal() if they want to intercept
these errors.

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

lightningd/invoice.c

@@ -287,8 +287,8 @@ static void json_delinvoice(struct command *cmd,
 	}
 
 	if (!wallet_invoice_remove(cmd->ld->wallet, i)) {
-		log_broken(cmd->ld->log, "Error attempting to remove invoice %"PRIu64": %s",
-			   i->id, cmd->ld->wallet->db->err);
+		log_broken(cmd->ld->log, "Error attempting to remove invoice %"PRIu64,
+			   i->id);
 		command_fail(cmd, "Database error");
 		return;
 	}

lightningd/test/run-find_my_path.c

@@ -7,11 +7,11 @@ int unused_main(int argc, char *argv[]);
 /* Generated stub for crashlog_activate */
 void crashlog_activate(const char *argv0 UNNEEDED, struct log *log UNNEEDED)
 { fprintf(stderr, "crashlog_activate called!\n"); abort(); }
-/* Generated stub for db_begin_transaction */
-bool db_begin_transaction(struct db *db UNNEEDED)
-{ fprintf(stderr, "db_begin_transaction called!\n"); abort(); }
+/* Generated stub for db_begin_transaction_ */
+void db_begin_transaction_(struct db *db UNNEEDED, const char *location UNNEEDED)
+{ fprintf(stderr, "db_begin_transaction_ called!\n"); abort(); }
 /* Generated stub for db_commit_transaction */
-bool db_commit_transaction(struct db *db UNNEEDED)
+void db_commit_transaction(struct db *db UNNEEDED)
 { fprintf(stderr, "db_commit_transaction called!\n"); abort(); }
 /* Generated stub for debug_poll */
 int debug_poll(struct pollfd *fds UNNEEDED, nfds_t nfds UNNEEDED, int timeout UNNEEDED)

wallet/db.c

@@ -122,86 +122,64 @@ char *dbmigrations[] = {
     NULL,
 };
 
-/**
- * db_clear_error - Clear any errors from previous queries
- */
-static void db_clear_error(struct db *db)
-{
-	db->err = tal_free(db->err);
-}
-
 sqlite3_stmt *db_prepare_(const char *caller, struct db *db, const char *query)
 {
 	int err;
 	sqlite3_stmt *stmt;
-	if (db->in_transaction && db->err)
-		return NULL;
 
-	db_clear_error(db);
+	assert(db->in_transaction);
+
 	err = sqlite3_prepare_v2(db->sql, query, -1, &stmt, NULL);
 
-	if (err != SQLITE_OK) {
-		db->err = tal_fmt(db, "%s: %s: %s", caller, query,
-				  sqlite3_errmsg(db->sql));
-	}
+	if (err != SQLITE_OK)
+		fatal("%s: %s: %s", caller, query, sqlite3_errmsg(db->sql));
+
 	return stmt;
 }
 
-bool db_exec_prepared_(const char *caller, struct db *db, sqlite3_stmt *stmt)
+void db_exec_prepared_(const char *caller, struct db *db, sqlite3_stmt *stmt)
 {
-	if (db->in_transaction && db->err) {
-		goto fail;
-	}
-
-	db_clear_error(db);
+	assert(db->in_transaction);
 
-	if (sqlite3_step(stmt) !=  SQLITE_DONE) {
-		db->err =
-		    tal_fmt(db, "%s: %s", caller, sqlite3_errmsg(db->sql));
-		goto fail;
-	}
+	if (sqlite3_step(stmt) !=  SQLITE_DONE)
+		fatal("%s: %s", caller, sqlite3_errmsg(db->sql));
 
 	sqlite3_finalize(stmt);
-	return true;
-fail:
-	sqlite3_finalize(stmt);
-	return false;
 }
 
-bool PRINTF_FMT(3, 4)
-    db_exec(const char *caller, struct db *db, const char *fmt, ...)
+/* This one doesn't check if we're in a transaction. */
+static void db_do_exec(const char *caller, struct db *db, const char *cmd)
 {
-	va_list ap;
-	char *cmd, *errmsg;
+	char *errmsg;
 	int err;
 
-	if (db->in_transaction && db->err)
-		return false;
+	err = sqlite3_exec(db->sql, cmd, NULL, NULL, &errmsg);
+	if (err != SQLITE_OK) {
+		fatal("%s:%s:%s:%s", caller, sqlite3_errstr(err), cmd, errmsg);
+		/* Only reached in testing */
+		sqlite3_free(errmsg);
+	}
+}
 
-	db_clear_error(db);
+void PRINTF_FMT(3, 4)
+    db_exec(const char *caller, struct db *db, const char *fmt, ...)
+{
+	va_list ap;
+	char *cmd;
+
+	assert(db->in_transaction);
 
 	va_start(ap, fmt);
 	cmd = tal_vfmt(db, fmt, ap);
 	va_end(ap);
 
-	err = sqlite3_exec(db->sql, cmd, NULL, NULL, &errmsg);
-	if (err != SQLITE_OK) {
-		tal_free(db->err);
-		db->err = tal_fmt(db, "%s:%s:%s:%s", caller,
-				  sqlite3_errstr(err), cmd, errmsg);
-		sqlite3_free(errmsg);
-		tal_free(cmd);
-		return false;
-	}
+	db_do_exec(caller, db, cmd);
 	tal_free(cmd);
-	return true;
 }
 
 bool db_exec_prepared_mayfail_(const char *caller, struct db *db, sqlite3_stmt *stmt)
 {
-	if (db->in_transaction && db->err) {
-		goto fail;
-	}
+	assert(db->in_transaction);
 
 	if (sqlite3_step(stmt) != SQLITE_DONE) {
 		goto fail;
@@ -221,10 +199,7 @@ sqlite3_stmt *PRINTF_FMT(3, 4)
 	char *query;
 	sqlite3_stmt *stmt;
 
-	if (db->in_transaction && db->err)
-		return NULL;
-
-	db_clear_error(db);
+	assert(db->in_transaction);
 
 	va_start(ap, fmt);
 	query = tal_vfmt(db, fmt, ap);
@@ -237,50 +212,20 @@ sqlite3_stmt *PRINTF_FMT(3, 4)
 
 static void close_db(struct db *db) { sqlite3_close(db->sql); }
 
-bool db_begin_transaction(struct db *db)
-{
-	if (!db->in_transaction) {
-		/* Clear any errors from previous transactions and
-		 * non-transactional queries */
-		db_clear_error(db);
-		db->in_transaction = db_exec(__func__, db, "BEGIN TRANSACTION;");
-		assert(db->in_transaction);
-		return db->in_transaction;
-	}
-	db->in_transaction++;
-	return false;
-}
-
-bool db_commit_transaction(struct db *db)
+void db_begin_transaction_(struct db *db, const char *location)
 {
-	bool ret;
+	if (db->in_transaction)
+		fatal("Already in transaction from %s", db->in_transaction);
 
-	assert(db->in_transaction);
-	if (db->err) {
-		char *errmsg;
-		int err;
-
-		/* Do this manually: db_exec is a NOOP with db->err */
-		err = sqlite3_exec(db->sql, "ROLLBACK;", NULL, NULL, &errmsg);
-		if (err != SQLITE_OK) {
-			db->err = tal_fmt(db, "%s then ROLLBACK failed:%s:%s",
-					  db->err, sqlite3_errstr(err), errmsg);
-			sqlite3_free(errmsg);
-		}
-		ret = false;
-	} else {
-		ret = db_exec(__func__, db, "COMMIT;");
-	}
-	db->in_transaction--;
-	return ret;
+	db_do_exec(location, db, "BEGIN TRANSACTION;");
+	db->in_transaction = location;
 }
 
-bool db_rollback_transaction(struct db *db)
+void db_commit_transaction(struct db *db)
 {
 	assert(db->in_transaction);
-	bool ret = db_exec(__func__, db, "ROLLBACK;");
-	db->in_transaction--;
-	return ret;
+	db_exec(__func__, db, "COMMIT;");
+	db->in_transaction = NULL;
 }
 
 /**
@@ -308,11 +253,8 @@ static struct db *db_open(const tal_t *ctx, char *filename)
 	db->filename = tal_dup_arr(db, char, filename, strlen(filename), 0);
 	db->sql = sql;
 	tal_add_destructor(db, close_db);
-	db->in_transaction = false;
-	db->err = NULL;
-	if (!db_exec(__func__, db, "PRAGMA foreign_keys = ON;")) {
-		fatal("Could not enable foreignkeys on database: %s", db->err);
-	}
+	db->in_transaction = NULL;
+	db_do_exec(__func__, db, "PRAGMA foreign_keys = ON;");
 
 	return db;
 }
@@ -411,16 +353,14 @@ s64 db_get_intvar(struct db *db, char *varname, s64 defval)
 	return res;
 }
 
-bool db_set_intvar(struct db *db, char *varname, s64 val)
+void db_set_intvar(struct db *db, char *varname, s64 val)
 {
 	/* Attempt to update */
 	db_exec(__func__, db,
 		"UPDATE vars SET val='%" PRId64 "' WHERE name='%s';", val,
 		varname);
-	if (sqlite3_changes(db->sql) > 0)
-		return true;
-	else
-		return db_exec(
+	if (sqlite3_changes(db->sql) == 0)
+		db_exec(
 		    __func__, db,
 		    "INSERT INTO vars (name, val) VALUES ('%s', '%" PRId64
 		    "');",

wallet/db.h

@@ -15,8 +15,7 @@
 
 struct db {
 	char *filename;
-	unsigned int in_transaction;
-	const char *err;
+	const char *in_transaction;
 	sqlite3 *sql;
 };
 
@@ -25,6 +24,7 @@ struct db {
  *
  * Opens the database, creating it if necessary, and applying
  * migrations until the schema is updated to the current state.
+ * Calls fatal() on error.
  *
  * Params:
  *  @ctx: the tal_t context to allocate from
@@ -33,37 +33,33 @@ struct db {
 struct db *db_setup(const tal_t *ctx);
 
 /**
- * db_query - Prepare and execute a query, and return the result
+ * db_query - Prepare and execute a query, and return the result (or NULL)
  */
 sqlite3_stmt *PRINTF_FMT(3, 4)
 	db_query(const char *caller, struct db *db, const char *fmt, ...);
 
-bool PRINTF_FMT(3, 4)
+/**
+ * db_exec - execute a statement, call fatal() if it fails.
+ */
+void PRINTF_FMT(3, 4)
 	db_exec(const char *caller, struct db *db, const char *fmt, ...);
 
 /**
  * db_begin_transaction - Begin a transaction
  *
- * Begin a new DB transaction if we aren't already in one. Returns
- * true if the call started a transaction, i.e., the caller MUST take
- * care to either commit or rollback. If false, this is a nested
- * transaction and the caller MUST not commit/rollback, since the
- * transaction is handled at a higher level in the callstack.
+ * Begin a new DB transaction.  fatal() on database error.
  */
-bool db_begin_transaction(struct db *db);
+#define db_begin_transaction(db) \
+	db_begin_transaction_((db), __FILE__ ":" stringify(__LINE__))
+void db_begin_transaction_(struct db *db, const char *location);
 
 /**
  * db_commit_transaction - Commit a running transaction
  *
- * Requires that we are currently in a transaction. Returns whether
- * the commit was successful.
- */
-bool db_commit_transaction(struct db *db);
-
-/**
- * db_rollback_transaction - Whoops... undo! undo!
+ * Requires that we are currently in a transaction.  fatal() if we
+ * fail to commit.
  */
-bool db_rollback_transaction(struct db *db);
+void db_commit_transaction(struct db *db);
 
 /**
  * db_set_intvar - Set an integer variable in the database
@@ -71,7 +67,7 @@ bool db_rollback_transaction(struct db *db);
  * Utility function to store generic integer values in the
  * database.
  */
-bool db_set_intvar(struct db *db, char *varname, s64 val);
+void db_set_intvar(struct db *db, char *varname, s64 val);
 
 /**
  * db_get_intvar - Retrieve an integer variable from the database
@@ -102,18 +98,18 @@ sqlite3_stmt *db_prepare_(const char *caller, struct db *db, const char *query);
  * After preparing a statement using `db_prepare`, and after binding
  * all non-null variables using the `sqlite3_bind_*` functions, it can
  * be executed with this function. It is a small, transaction-aware,
- * wrapper around `sqlite3_step`, that also sets `db->err` if the
- * execution fails. This will take ownership of `stmt` and will free
+ * wrapper around `sqlite3_step`, that calls fatal() if the execution
+ * fails. This will take ownership of `stmt` and will free
  * it before returning.
  *
  * @db: The database to execute on
  * @stmt: The prepared statement to execute
  */
 #define db_exec_prepared(db,stmt) db_exec_prepared_(__func__,db,stmt)
-bool db_exec_prepared_(const char *caller, struct db *db, sqlite3_stmt *stmt);
+void db_exec_prepared_(const char *caller, struct db *db, sqlite3_stmt *stmt);
 
 /**
- * db_exec_prepared_mayfail - db_exec_prepared, but don't set db->err if it fails.
+ * db_exec_prepared_mayfail - db_exec_prepared, but don't fatal() it fails.
  */
 #define db_exec_prepared_mayfail(db,stmt) \
 	db_exec_prepared_mayfail_(__func__,db,stmt)

wallet/db_tests.c

@@ -1,3 +1,8 @@
+  #include <lightningd/log.h>
+
+static void db_fatal(const char *fmt, ...);
+#define fatal db_fatal
+
 #include "db.c"
 
 #include "wallet/test_utils.h"
@@ -5,6 +10,19 @@
 #include <stdio.h>
 #include <unistd.h>
 
+static char *db_err;
+static void db_fatal(const char *fmt, ...)
+{
+	va_list ap;
+
+	/* Fail hard if we're complaining about not being in transaction */
+	assert(!strstarts(fmt, "No longer in transaction"));
+
+	va_start(ap, fmt);
+	db_err = tal_vfmt(NULL, fmt, ap);
+	va_end(ap);
+}
+
 static struct db *create_test_db(const char *testname)
 {
 	struct db *db;
@@ -23,9 +41,13 @@ static bool test_empty_db_migrate(void)
 {
 	struct db *db = create_test_db(__func__);
 	CHECK(db);
+	db_begin_transaction(db);
 	CHECK(db_get_version(db) == -1);
+	db_commit_transaction(db);
 	db_migrate(db);
+	db_begin_transaction(db);
 	CHECK(db_get_version(db) == db_migration_count());
+	db_commit_transaction(db);
 
 	tal_free(db);
 	return true;
@@ -34,17 +56,22 @@ static bool test_empty_db_migrate(void)
 static bool test_primitives(void)
 {
 	struct db *db = create_test_db(__func__);
-	CHECK_MSG(db_begin_transaction(db), "Starting a new transaction");
+	db_begin_transaction(db);
 	CHECK(db->in_transaction);
-	CHECK_MSG(db_commit_transaction(db), "Committing a transaction");
+	db_commit_transaction(db);
 	CHECK(!db->in_transaction);
-	CHECK_MSG(db_begin_transaction(db), "Starting a transaction after commit");
-	CHECK(db_rollback_transaction(db));
+	db_begin_transaction(db);
+	db_commit_transaction(db);
+
+	db_begin_transaction(db);
+	db_exec(__func__, db, "SELECT name FROM sqlite_master WHERE type='table';");
+	CHECK_MSG(!db_err, "Simple correct SQL command");
 
-	CHECK_MSG(db_exec(__func__, db, "SELECT name FROM sqlite_master WHERE type='table';"), "Simple correct SQL command");
-	CHECK_MSG(!db_exec(__func__, db, "not a valid SQL statement"), "Failing SQL command");
+	db_exec(__func__, db, "not a valid SQL statement");
+	CHECK_MSG(db_err, "Failing SQL command");
+	db_err = tal_free(db_err);
+	db_commit_transaction(db);
 	CHECK(!db->in_transaction);
-	CHECK_MSG(db_begin_transaction(db), "Starting a transaction after a failed transaction");
 	tal_free(db);
 
 	return true;
@@ -57,16 +84,18 @@ static bool test_vars(void)
 	CHECK(db);
 	db_migrate(db);
 
+	db_begin_transaction(db);
 	/* Check default behavior */
 	CHECK(db_get_intvar(db, varname, 42) == 42);
 
 	/* Check setting and getting */
-	CHECK(db_set_intvar(db, varname, 1));
+	db_set_intvar(db, varname, 1);
 	CHECK(db_get_intvar(db, varname, 42) == 1);
 
 	/* Check updating */
-	CHECK(db_set_intvar(db, varname, 2));
+	db_set_intvar(db, varname, 2);
 	CHECK(db_get_intvar(db, varname, 42) == 2);
+	db_commit_transaction(db);
 
 	tal_free(db);
 	return true;

wallet/wallet.c

@@ -106,7 +106,7 @@ static void unreserve_utxo(struct wallet *w, const struct utxo *unres)
 	if (!wallet_update_output_status(w, &unres->txid, unres->outnum,
 					 output_state_reserved,
 					 output_state_available)) {
-		fatal("Unable to unreserve output: %s", w->db->err);
+		fatal("Unable to unreserve output");
 	}
 }
 
@@ -126,7 +126,7 @@ void wallet_confirm_utxos(struct wallet *w, const struct utxo **utxos)
 		if (!wallet_update_output_status(
 			w, &utxos[i]->txid, utxos[i]->outnum,
 			output_state_reserved, output_state_spent)) {
-			fatal("Unable to mark output as spent: %s", w->db->err);
+			fatal("Unable to mark output as spent");
 		}
 	}
 }
@@ -154,7 +154,7 @@ const struct utxo **wallet_select_coins(const tal_t *ctx, struct wallet *w,
 		if (!wallet_update_output_status(
 			w, &available[i]->txid, available[i]->outnum,
 			output_state_available, output_state_reserved))
-			fatal("Unable to reserve output: %s", w->db->err);
+			fatal("Unable to reserve output");
 
 		weight += (32 + 4 + 4) * 4;
 		if (utxos[i]->is_p2sh)
@@ -229,19 +229,16 @@ s64 wallet_get_newindex(struct lightningd *ld)
 	return newidx;
 }
 
-bool wallet_shachain_init(struct wallet *wallet, struct wallet_shachain *chain)
+void wallet_shachain_init(struct wallet *wallet, struct wallet_shachain *chain)
 {
 	sqlite3_stmt *stmt;
 	/* Create shachain */
 	shachain_init(&chain->chain);
 	stmt = db_prepare(wallet->db, "INSERT INTO shachains (min_index, num_valid) VALUES (?, 0);");
 	sqlite3_bind_int64(stmt, 1, chain->chain.min_index);
-	if (!db_exec_prepared(wallet->db, stmt)) {
-		return false;
-	}
+	db_exec_prepared(wallet->db, stmt);
 
 	chain->id = sqlite3_last_insert_rowid(wallet->db->sql);
-	return true;
 }
 
 /* TODO(cdecker) Stolen from shachain, move to some appropriate location */
@@ -971,7 +968,7 @@ bool wallet_htlcs_load_for_channel(struct wallet *wallet,
 	    DIRECTION_INCOMING, chan->id, SENT_REMOVE_ACK_REVOCATION);
 
 	if (!stmt) {
-		log_broken(wallet->log, "Could not select htlc_ins: %s", wallet->db->err);
+		log_broken(wallet->log, "Could not select htlc_ins");
 		return false;
 	}
 
@@ -992,7 +989,7 @@ bool wallet_htlcs_load_for_channel(struct wallet *wallet,
 	    DIRECTION_OUTGOING, chan->id, RCVD_REMOVE_ACK_REVOCATION);
 
 	if (!stmt) {
-		log_broken(wallet->log, "Could not select htlc_outs: %s", wallet->db->err);
+		log_broken(wallet->log, "Could not select htlc_outs");
 		return false;
 	}
 
@@ -1058,8 +1055,6 @@ void wallet_invoice_save(struct wallet *wallet, struct invoice *inv)
 	if (!inv->id) {
 		stmt = db_prepare(wallet->db,
 			"INSERT INTO invoices (payment_hash, payment_key, state, msatoshi, label) VALUES (?, ?, ?, ?, ?);");
-		if (!stmt)
-			fatal("Could not prepare statement: %s", wallet->db->err);
 
 		sqlite3_bind_blob(stmt, 1, &inv->rhash, sizeof(inv->rhash), SQLITE_TRANSIENT);
 		sqlite3_bind_blob(stmt, 2, &inv->r, sizeof(inv->r), SQLITE_TRANSIENT);
@@ -1067,21 +1062,16 @@ void wallet_invoice_save(struct wallet *wallet, struct invoice *inv)
 		sqlite3_bind_int64(stmt, 4, inv->msatoshi);
 		sqlite3_bind_text(stmt, 5, inv->label, strlen(inv->label), SQLITE_TRANSIENT);
 
-		if (!db_exec_prepared(wallet->db, stmt))
-			fatal("Could not exec prepared statement: %s", wallet->db->err);
+		db_exec_prepared(wallet->db, stmt);
 
 		inv->id = sqlite3_last_insert_rowid(wallet->db->sql);
 	} else {
 		stmt = db_prepare(wallet->db, "UPDATE invoices SET state=? WHERE id=?;");
 
-		if (!stmt)
-			fatal("Could not prepare statement: %s", wallet->db->err);
-
 		sqlite3_bind_int(stmt, 1, inv->state);
 		sqlite3_bind_int64(stmt, 2, inv->id);
 
-		if (!db_exec_prepared(wallet->db, stmt))
-			fatal("Could not exec prepared statement: %s", wallet->db->err);
+		db_exec_prepared(wallet->db, stmt);
 	}
 }
 
@@ -1109,7 +1099,7 @@ bool wallet_invoices_load(struct wallet *wallet, struct invoices *invs)
 				"SELECT id, state, payment_key, payment_hash, "
 				"label, msatoshi FROM invoices;");
 	if (!stmt) {
-		log_broken(wallet->log, "Could not load invoices: %s", wallet->db->err);
+		log_broken(wallet->log, "Could not load invoices");
 		return false;
 	}
 
@@ -1131,7 +1121,8 @@ bool wallet_invoice_remove(struct wallet *wallet, struct invoice *inv)
 {
 	sqlite3_stmt *stmt = db_prepare(wallet->db, "DELETE FROM invoices WHERE id=?");
 	sqlite3_bind_int64(stmt, 1, inv->id);
-	return db_exec_prepared(wallet->db, stmt) && sqlite3_changes(wallet->db->sql) == 1;
+	db_exec_prepared(wallet->db, stmt);
+	return sqlite3_changes(wallet->db->sql) == 1;
 }
 
 struct htlc_stub *wallet_htlc_stubs(tal_t *ctx, struct wallet *wallet,
@@ -1143,9 +1134,6 @@ struct htlc_stub *wallet_htlc_stubs(tal_t *ctx, struct wallet *wallet,
 		"SELECT channel_id, direction, cltv_expiry, payment_hash "
 		"FROM channel_htlcs WHERE channel_id = ?;");
 
-	if (!stmt)
-		fatal("Error preparing select: %s", wallet->db->err);
-
 	sqlite3_bind_int64(stmt, 1, chan->id);
 
 	stubs = tal_arr(ctx, struct htlc_stub, 0);

wallet/wallet.h

@@ -141,7 +141,7 @@ s64 wallet_get_newindex(struct lightningd *ld);
 /**
  * wallet_shachain_init -- wallet wrapper around shachain_init
  */
-bool wallet_shachain_init(struct wallet *wallet, struct wallet_shachain *chain);
+void wallet_shachain_init(struct wallet *wallet, struct wallet_shachain *chain);
 
 /**
  * wallet_shachain_add_hash -- wallet wrapper around shachain_add_hash

wallet/wallet_tests.c

@@ -22,13 +22,16 @@ static void wallet_fatal(const char *fmt, ...)
 	/* Fail hard if we're complaining about not being in transaction */
 	assert(!strstarts(fmt, "No longer in transaction"));
 
+	/* Fail hard if we're complaining about not being in transaction */
+	assert(!strstarts(fmt, "No longer in transaction"));
+
 	va_start(ap, fmt);
 	wallet_err = tal_vfmt(NULL, fmt, ap);
 	va_end(ap);
 }
 
 #define transaction_wrap(db, ...)					\
-	(db_begin_transaction(db), __VA_ARGS__, db_commit_transaction(db))
+	(db_begin_transaction(db), __VA_ARGS__, db_commit_transaction(db), wallet_err == NULL)
 
 void invoice_add(struct invoices *invs,
 		 struct invoice *inv){}
@@ -85,6 +88,8 @@ static bool test_wallet_outputs(void)
 
 	memset(&u, 0, sizeof(u));
 
+	db_begin_transaction(w->db);
+
 	/* Should work, it's the first time we add it */
 	CHECK_MSG(wallet_add_utxo(w, &u, p2sh_wpkh),
 		  "wallet_add_utxo failed on first add");
@@ -117,6 +122,7 @@ static bool test_wallet_outputs(void)
 					      output_state_spent),
 		  "could not change output state ignoring oldstate");
 
+	db_commit_transaction(w->db);
 	tal_free(w);
 	return true;
 }
@@ -143,7 +149,10 @@ static bool test_shachain_crud(void)
 	memset(&b, 0, sizeof(b));
 
 	w->db = db_open(w, filename);
-	CHECK(wallet_shachain_init(w, &a));
+	db_begin_transaction(w->db);
+	CHECK_MSG(!wallet_err, "db_begin_transaction failed");
+	wallet_shachain_init(w, &a);
+	CHECK(!wallet_err);
 
 	CHECK(a.id == 1);
 
@@ -158,6 +167,9 @@ static bool test_shachain_crud(void)
 
 	CHECK(wallet_shachain_load(w, a.id, &b));
 	CHECK_MSG(memcmp(&a, &b, sizeof(a)) == 0, "Loading from database doesn't match");
+
+	db_commit_transaction(w->db);
+	CHECK(!wallet_err);
 	tal_free(w);
 	return true;
 }
@@ -261,11 +273,16 @@ static bool test_channel_crud(const tal_t *ctx)
 	ci.remote_per_commit = pk;
 	ci.old_remote_per_commit = pk;
 
+	db_begin_transaction(w->db);
+	CHECK(!wallet_err);
+
 	/* Variant 1: insert with null for scid, funding_tx_id, channel_info, last_tx */
 	wallet_channel_save(w, &c1);
 	CHECK_MSG(!wallet_err,
-		  tal_fmt(w, "Insert into DB: %s", w->db->err));
-	CHECK_MSG(wallet_channel_load(w, c1.id, c2), tal_fmt(w, "Load from DB: %s", w->db->err));
+		  tal_fmt(w, "Insert into DB: %s", wallet_err));
+	CHECK_MSG(wallet_channel_load(w, c1.id, c2), tal_fmt(w, "Load from DB"));
+	CHECK_MSG(!wallet_err,
+		  tal_fmt(w, "Load from DB: %s", wallet_err));
 	CHECK_MSG(channelseq(&c1, c2), "Compare loaded with saved (v1)");
 
 	/* We just inserted them into an empty DB so this must be 1 */
@@ -277,8 +294,10 @@ static bool test_channel_crud(const tal_t *ctx)
 	c1.peer->scid = talz(w, struct short_channel_id);
 	wallet_channel_save(w, &c1);
 	CHECK_MSG(!wallet_err,
-		  tal_fmt(w, "Insert into DB: %s", w->db->err));
-	CHECK_MSG(wallet_channel_load(w, c1.id, c2), tal_fmt(w, "Load from DB: %s", w->db->err));
+		  tal_fmt(w, "Insert into DB: %s", wallet_err));
+	CHECK_MSG(wallet_channel_load(w, c1.id, c2), tal_fmt(w, "Load from DB"));
+	CHECK_MSG(!wallet_err,
+		  tal_fmt(w, "Insert into DB: %s", wallet_err));
 	CHECK_MSG(channelseq(&c1, c2), "Compare loaded with saved (v2)");
 
 	/* Updates should not result in new ids */
@@ -290,39 +309,51 @@ static bool test_channel_crud(const tal_t *ctx)
 	c1.peer->our_msatoshi = &msat;
 
 	wallet_channel_save(w, &c1);
-	CHECK_MSG(!wallet_err, tal_fmt(w, "Insert into DB: %s", w->db->err));
-	CHECK_MSG(wallet_channel_load(w, c1.id, c2), tal_fmt(w, "Load from DB: %s", w->db->err));
+	CHECK_MSG(!wallet_err, tal_fmt(w, "Insert into DB: %s", wallet_err));
+	CHECK_MSG(wallet_channel_load(w, c1.id, c2), tal_fmt(w, "Load from DB"));
+	CHECK_MSG(!wallet_err,
+		  tal_fmt(w, "Insert into DB: %s", wallet_err));
 	CHECK_MSG(channelseq(&c1, c2), "Compare loaded with saved (v3)");
 
 	/* Variant 4: update with funding_tx_id */
 	c1.peer->funding_txid = hash;
 	wallet_channel_save(w, &c1);
-	CHECK_MSG(!wallet_err, tal_fmt(w, "Insert into DB: %s", w->db->err));
-	CHECK_MSG(wallet_channel_load(w, c1.id, c2), tal_fmt(w, "Load from DB: %s", w->db->err));
+	CHECK_MSG(!wallet_err, tal_fmt(w, "Insert into DB: %s", wallet_err));
+	CHECK_MSG(wallet_channel_load(w, c1.id, c2), tal_fmt(w, "Load from DB"));
+	CHECK_MSG(!wallet_err,
+		  tal_fmt(w, "Insert into DB: %s", wallet_err));
 	CHECK_MSG(channelseq(&c1, c2), "Compare loaded with saved (v4)");
 
 	/* Variant 5: update with channel_info */
 	p.channel_info = &ci;
 	wallet_channel_save(w, &c1);
-	CHECK_MSG(!wallet_err, tal_fmt(w, "Insert into DB: %s", w->db->err));
-	CHECK_MSG(wallet_channel_load(w, c1.id, c2), tal_fmt(w, "Load from DB: %s", w->db->err));
+	CHECK_MSG(!wallet_err, tal_fmt(w, "Insert into DB: %s", wallet_err));
+	CHECK_MSG(wallet_channel_load(w, c1.id, c2), tal_fmt(w, "Load from DB"));
+	CHECK_MSG(!wallet_err,
+		  tal_fmt(w, "Insert into DB: %s", wallet_err));
 	CHECK_MSG(channelseq(&c1, c2), "Compare loaded with saved (v5)");
 
 	/* Variant 6: update with last_commit_sent */
 	p.last_sent_commit = &last_commit;
 	wallet_channel_save(w, &c1);
-	CHECK_MSG(!wallet_err, tal_fmt(w, "Insert into DB: %s", w->db->err));
-	CHECK_MSG(wallet_channel_load(w, c1.id, c2), tal_fmt(w, "Load from DB: %s", w->db->err));
+	CHECK_MSG(!wallet_err, tal_fmt(w, "Insert into DB: %s", wallet_err));
+	CHECK_MSG(wallet_channel_load(w, c1.id, c2), tal_fmt(w, "Load from DB"));
+	CHECK_MSG(!wallet_err,
+		  tal_fmt(w, "Insert into DB: %s", wallet_err));
 	CHECK_MSG(channelseq(&c1, c2), "Compare loaded with saved (v6)");
 
 	/* Variant 7: update with last_tx (taken from BOLT #3) */
 	p.last_tx = bitcoin_tx_from_hex(w, "02000000000101bef67e4e2fb9ddeeb3461973cd4c62abb35050b1add772995b820b584a488489000000000038b02b8003a00f0000000000002200208c48d15160397c9731df9bc3b236656efb6665fbfe92b4a6878e88a499f741c4c0c62d0000000000160014ccf1af2f2aabee14bb40fa3851ab2301de843110ae8f6a00000000002200204adb4e2f00643db396dd120d4e7dc17625f5f2c11a40d857accc862d6b7dd80e040047304402206a2679efa3c7aaffd2a447fd0df7aba8792858b589750f6a1203f9259173198a022008d52a0e77a99ab533c36206cb15ad7aeb2aa72b93d4b571e728cb5ec2f6fe260147304402206d6cb93969d39177a09d5d45b583f34966195b77c7e585cf47ac5cce0c90cefb022031d71ae4e33a4e80df7f981d696fbdee517337806a3c7138b7491e2cbb077a0e01475221023da092f6980e58d2c037173180e9a465476026ee50f96695963e8efe436f54eb21030e9f7b623d2ccc7c9bd44d66d5ce21ce504c0acf6385a132cec6d3c39fa711c152ae3e195220", strlen("02000000000101bef67e4e2fb9ddeeb3461973cd4c62abb35050b1add772995b820b584a488489000000000038b02b8003a00f0000000000002200208c48d15160397c9731df9bc3b236656efb6665fbfe92b4a6878e88a499f741c4c0c62d0000000000160014ccf1af2f2aabee14bb40fa3851ab2301de843110ae8f6a00000000002200204adb4e2f00643db396dd120d4e7dc17625f5f2c11a40d857accc862d6b7dd80e040047304402206a2679efa3c7aaffd2a447fd0df7aba8792858b589750f6a1203f9259173198a022008d52a0e77a99ab533c36206cb15ad7aeb2aa72b93d4b571e728cb5ec2f6fe260147304402206d6cb93969d39177a09d5d45b583f34966195b77c7e585cf47ac5cce0c90cefb022031d71ae4e33a4e80df7f981d696fbdee517337806a3c7138b7491e2cbb077a0e01475221023da092f6980e58d2c037173180e9a465476026ee50f96695963e8efe436f54eb21030e9f7b623d2ccc7c9bd44d66d5ce21ce504c0acf6385a132cec6d3c39fa711c152ae3e195220"));
 	p.last_sig = sig;
 	wallet_channel_save(w, &c1);
-	CHECK_MSG(!wallet_err, tal_fmt(w, "Insert into DB: %s", w->db->err));
-	CHECK_MSG(wallet_channel_load(w, c1.id, c2), tal_fmt(w, "Load from DB: %s", w->db->err));
+	CHECK_MSG(!wallet_err, tal_fmt(w, "Insert into DB: %s", wallet_err));
+	CHECK_MSG(wallet_channel_load(w, c1.id, c2), tal_fmt(w, "Load from DB"));
+	CHECK_MSG(!wallet_err,
+		  tal_fmt(w, "Insert into DB: %s", wallet_err));
 	CHECK_MSG(channelseq(&c1, c2), "Compare loaded with saved (v7)");
 
+	db_commit_transaction(w->db);
+	CHECK(!wallet_err);
 	tal_free(w);
 	return true;
 }
@@ -346,7 +377,7 @@ static bool test_channel_config_crud(const tal_t *ctx)
 	    cc1->id == 1,
 	    tal_fmt(ctx, "channel_config->id != 1; got %" PRIu64, cc1->id));
 
-	CHECK(wallet_channel_config_load(w, cc1->id, cc2));
+	CHECK(transaction_wrap(w->db, wallet_channel_config_load(w, cc1->id, cc2)));
 	CHECK(memeq(cc1, sizeof(*cc1), cc2, sizeof(*cc2)));
        	return true;
 }
@@ -363,7 +394,8 @@ static bool test_htlc_crud(const tal_t *ctx)
 	struct htlc_out_map *htlcs_out = tal(ctx, struct htlc_out_map);
 
 	/* Make sure we have our references correct */
-	db_exec(__func__, w->db, "INSERT INTO channels (id) VALUES (1);");
+	CHECK(transaction_wrap(w->db,
+			       db_exec(__func__, w->db, "INSERT INTO channels (id) VALUES (1);")));
 	chan->id = 1;
 	chan->peer = peer;
 
@@ -383,11 +415,13 @@ static bool test_htlc_crud(const tal_t *ctx)
 
 	/* Store the htlc_in */
 	CHECK_MSG(transaction_wrap(w->db, wallet_htlc_save_in(w, chan, &in)),
-		  tal_fmt(ctx, "Save htlc_in failed: %s", w->db->err));
+		  tal_fmt(ctx, "Save htlc_in failed: %s", wallet_err));
 	CHECK_MSG(in.dbid != 0, "HTLC DB ID was not set.");
 	/* Saving again should get us a collision */
 	CHECK_MSG(!transaction_wrap(w->db, wallet_htlc_save_in(w, chan, &in)),
 		  "Saving two HTLCs with the same data must not succeed.");
+	CHECK(wallet_err);
+	wallet_err = tal_free(wallet_err);
 
 	/* Update */
 	CHECK_MSG(transaction_wrap(w->db, wallet_htlc_update(w, in.dbid, RCVD_ADD_HTLC, NULL)),
@@ -397,21 +431,28 @@ static bool test_htlc_crud(const tal_t *ctx)
 	    "Update HTLC with payment_key failed");
 
 	CHECK_MSG(transaction_wrap(w->db, wallet_htlc_save_out(w, chan, &out)),
-		  tal_fmt(ctx, "Save htlc_out failed: %s", w->db->err));
+		  tal_fmt(ctx, "Save htlc_out failed: %s", wallet_err));
 	CHECK_MSG(out.dbid != 0, "HTLC DB ID was not set.");
 
 	CHECK_MSG(!transaction_wrap(w->db, wallet_htlc_save_out(w, chan, &out)),
 		  "Saving two HTLCs with the same data must not succeed.");
+	CHECK(wallet_err);
+	wallet_err = tal_free(wallet_err);
 
 	/* Attempt to load them from the DB again */
 	htlc_in_map_init(htlcs_in);
 	htlc_out_map_init(htlcs_out);
 
+	db_begin_transaction(w->db);
+	CHECK(!wallet_err);
+
 	CHECK_MSG(wallet_htlcs_load_for_channel(w, chan, htlcs_in, htlcs_out),
 		  "Failed loading HTLCs");
 
 	CHECK_MSG(wallet_htlcs_reconnect(w, htlcs_in, htlcs_out),
 		  "Unable to reconnect htlcs.");
+	db_commit_transaction(w->db);
+	CHECK(!wallet_err);
 
 	hin = htlc_in_map_get(htlcs_in, &in.key);
 	hout = htlc_out_map_get(htlcs_out, &out.key);