Merge bitcoin/bitcoin#29253: wallet: guard against dangling to-be-reverted db transactions

b298242c8d test: sqlite, add coverage for dangling to-be-reverted db txns (furszy)
fc0e747192 sqlite: guard against dangling to-be-reverted db transactions (furszy)
472d2ca981 sqlite: introduce HasActiveTxn method (furszy)
dca874e838 sqlite: add ability to interrupt statements (furszy)
fdf9f66909 test: wallet db, exercise deadlock after write failure (furszy)

Pull request description:

  Discovered while was reviewing #29112, specifically https://github.com/bitcoin/bitcoin/pull/29112#pullrequestreview-1821862931.

  If the db handler that initiated the database transaction is destroyed,
  the ongoing transaction cannot be left dangling when the db txn fails
  to abort. It must be forcefully reverted; otherwise, any subsequent
  db handler executing a write operation will dump the dangling,
  to-be-reverted transaction data to disk.

  This not only breaks the isolation property but also results in the
  improper storage of incomplete information on disk, impacting
  the wallet consistency.

  This PR fixes the issue by resetting the db connection, automatically
  rolling back the transaction (per https://www.sqlite.org/c3ref/close.html)
  when the handler object is being destroyed and the txn abortion failed.

  Testing Notes
  Can verify the failure by reverting the fix e5217fea and running the test.
  It will fail without e5217fea and pass with it.

ACKs for top commit:
  achow101:
    ACK b298242c8d
  ryanofsky:
    Code review ACK b298242c8d. Just fix for exec result codes and comment update since last review.

Tree-SHA512: 44ba0323ab21440e79e9d7791bc1c56a8873c8bd3e8f6a85641b91576e1293011fa8032d8ae5b0580f3fb7a949356f7b9676693d7ceffa617aaad9f6569993eb

src/wallet/sqlite.cpp

@@ -377,6 +377,17 @@ void SQLiteDatabase::Close()
     m_db = nullptr;
 }
 
+bool SQLiteDatabase::HasActiveTxn()
+{
+    // 'sqlite3_get_autocommit' returns true by default, and false if a transaction has begun and not been committed or rolled back.
+    return m_db && sqlite3_get_autocommit(m_db) == 0;
+}
+
+int SQliteExecHandler::Exec(SQLiteDatabase& database, const std::string& statement)
+{
+    return sqlite3_exec(database.m_db, statement.data(), nullptr, nullptr, nullptr);
+}
+
 std::unique_ptr<DatabaseBatch> SQLiteDatabase::MakeBatch(bool flush_on_close)
 {
     // We ignore flush_on_close because we don't do manual flushing for SQLite
@@ -394,12 +405,18 @@ SQLiteBatch::SQLiteBatch(SQLiteDatabase& database)
 
 void SQLiteBatch::Close()
 {
-    // If m_db is in a transaction (i.e. not in autocommit mode), then abort the transaction in progress
-    if (m_database.m_db && sqlite3_get_autocommit(m_database.m_db) == 0) {
+    bool force_conn_refresh = false;
+
+    // If we began a transaction, and it wasn't committed, abort the transaction in progress
+    if (m_database.HasActiveTxn()) {
         if (TxnAbort()) {
             LogPrintf("SQLiteBatch: Batch closed unexpectedly without the transaction being explicitly committed or aborted\n");
         } else {
-            LogPrintf("SQLiteBatch: Batch closed and failed to abort transaction\n");
+            // If transaction cannot be aborted, it means there is a bug or there has been data corruption. Try to recover in this case
+            // by closing and reopening the database. Closing the database should also ensure that any changes made since the transaction
+            // was opened will be rolled back and future transactions can succeed without committing old data.
+            force_conn_refresh = true;
+            LogPrintf("SQLiteBatch: Batch closed and failed to abort transaction, resetting db connection..\n");
         }
     }
 
@@ -420,6 +437,17 @@ void SQLiteBatch::Close()
         }
         *stmt_prepared = nullptr;
     }
+
+    if (force_conn_refresh) {
+        m_database.Close();
+        try {
+            m_database.Open();
+        } catch (const std::runtime_error&) {
+            // If open fails, cleanup this object and rethrow the exception
+            m_database.Close();
+            throw;
+        }
+    }
 }
 
 bool SQLiteBatch::ReadKey(DataStream&& key, DataStream& value)
@@ -606,8 +634,8 @@ std::unique_ptr<DatabaseCursor> SQLiteBatch::GetNewPrefixCursor(Span<const std::
 
 bool SQLiteBatch::TxnBegin()
 {
-    if (!m_database.m_db || sqlite3_get_autocommit(m_database.m_db) == 0) return false;
-    int res = sqlite3_exec(m_database.m_db, "BEGIN TRANSACTION", nullptr, nullptr, nullptr);
+    if (!m_database.m_db || m_database.HasActiveTxn()) return false;
+    int res = Assert(m_exec_handler)->Exec(m_database, "BEGIN TRANSACTION");
     if (res != SQLITE_OK) {
         LogPrintf("SQLiteBatch: Failed to begin the transaction\n");
     }
@@ -616,8 +644,8 @@ bool SQLiteBatch::TxnBegin()
 
 bool SQLiteBatch::TxnCommit()
 {
-    if (!m_database.m_db || sqlite3_get_autocommit(m_database.m_db) != 0) return false;
-    int res = sqlite3_exec(m_database.m_db, "COMMIT TRANSACTION", nullptr, nullptr, nullptr);
+    if (!m_database.HasActiveTxn()) return false;
+    int res = Assert(m_exec_handler)->Exec(m_database, "COMMIT TRANSACTION");
     if (res != SQLITE_OK) {
         LogPrintf("SQLiteBatch: Failed to commit the transaction\n");
     }
@@ -626,8 +654,8 @@ bool SQLiteBatch::TxnCommit()
 
 bool SQLiteBatch::TxnAbort()
 {
-    if (!m_database.m_db || sqlite3_get_autocommit(m_database.m_db) != 0) return false;
-    int res = sqlite3_exec(m_database.m_db, "ROLLBACK TRANSACTION", nullptr, nullptr, nullptr);
+    if (!m_database.HasActiveTxn()) return false;
+    int res = Assert(m_exec_handler)->Exec(m_database, "ROLLBACK TRANSACTION");
     if (res != SQLITE_OK) {
         LogPrintf("SQLiteBatch: Failed to abort the transaction\n");
     }

src/wallet/sqlite.h

@@ -36,11 +36,21 @@ public:
     Status Next(DataStream& key, DataStream& value) override;
 };
 
+/** Class responsible for executing SQL statements in SQLite databases.
+ *  Methods are virtual so they can be overridden by unit tests testing unusual database conditions. */
+class SQliteExecHandler
+{
+public:
+    virtual ~SQliteExecHandler() {}
+    virtual int Exec(SQLiteDatabase& database, const std::string& statement);
+};
+
 /** RAII class that provides access to a WalletDatabase */
 class SQLiteBatch : public DatabaseBatch
 {
 private:
     SQLiteDatabase& m_database;
+    std::unique_ptr<SQliteExecHandler> m_exec_handler{std::make_unique<SQliteExecHandler>()};
 
     sqlite3_stmt* m_read_stmt{nullptr};
     sqlite3_stmt* m_insert_stmt{nullptr};
@@ -61,6 +71,8 @@ public:
     explicit SQLiteBatch(SQLiteDatabase& database);
     ~SQLiteBatch() override { Close(); }
 
+    void SetExecHandler(std::unique_ptr<SQliteExecHandler>&& handler) { m_exec_handler = std::move(handler); }
+
     /* No-op. See comment on SQLiteDatabase::Flush */
     void Flush() override {}
 
@@ -142,6 +154,9 @@ public:
     /** Make a SQLiteBatch connected to this database */
     std::unique_ptr<DatabaseBatch> MakeBatch(bool flush_on_close = true) override;
 
+    /** Return true if there is an on-going txn in this connection */
+    bool HasActiveTxn();
+
     sqlite3* m_db{nullptr};
     bool m_use_unsafe_sync;
 };

src/wallet/test/db_tests.cpp

@@ -205,5 +205,82 @@ BOOST_AUTO_TEST_CASE(db_cursor_prefix_byte_test)
     }
 }
 
+BOOST_AUTO_TEST_CASE(db_availability_after_write_error)
+{
+    // Ensures the database remains accessible without deadlocking after a write error.
+    // To simulate the behavior, record overwrites are disallowed, and the test verifies
+    // that the database remains active after failing to store an existing record.
+    for (const auto& database : TestDatabases(m_path_root)) {
+        // Write original record
+        std::unique_ptr<DatabaseBatch> batch = database->MakeBatch();
+        std::string key = "key";
+        std::string value = "value";
+        std::string value2 = "value_2";
+        BOOST_CHECK(batch->Write(key, value));
+        // Attempt to overwrite the record (expect failure)
+        BOOST_CHECK(!batch->Write(key, value2, /*fOverwrite=*/false));
+        // Successfully overwrite the record
+        BOOST_CHECK(batch->Write(key, value2, /*fOverwrite=*/true));
+        // Sanity-check; read and verify the overwritten value
+        std::string read_value;
+        BOOST_CHECK(batch->Read(key, read_value));
+        BOOST_CHECK_EQUAL(read_value, value2);
+    }
+}
+
+#ifdef USE_SQLITE
+
+// Test-only statement execution error
+constexpr int TEST_SQLITE_ERROR = -999;
+
+class DbExecBlocker : public SQliteExecHandler
+{
+private:
+    SQliteExecHandler m_base_exec;
+    std::set<std::string> m_blocked_statements;
+public:
+    DbExecBlocker(std::set<std::string> blocked_statements) : m_blocked_statements(blocked_statements) {}
+    int Exec(SQLiteDatabase& database, const std::string& statement) override {
+        if (m_blocked_statements.contains(statement)) return TEST_SQLITE_ERROR;
+        return m_base_exec.Exec(database, statement);
+    }
+};
+
+BOOST_AUTO_TEST_CASE(txn_close_failure_dangling_txn)
+{
+    // Verifies that there is no active dangling, to-be-reversed db txn
+    // after the batch object that initiated it is destroyed.
+    DatabaseOptions options;
+    DatabaseStatus status;
+    bilingual_str error;
+    std::unique_ptr<SQLiteDatabase> database = MakeSQLiteDatabase(m_path_root / "sqlite", options, status, error);
+
+    std::string key = "key";
+    std::string value = "value";
+
+    std::unique_ptr<SQLiteBatch> batch = std::make_unique<SQLiteBatch>(*database);
+    BOOST_CHECK(batch->TxnBegin());
+    BOOST_CHECK(batch->Write(key, value));
+    // Set a handler to prevent txn abortion during destruction.
+    // Mimicking a db statement execution failure.
+    batch->SetExecHandler(std::make_unique<DbExecBlocker>(std::set<std::string>{"ROLLBACK TRANSACTION"}));
+    // Destroy batch
+    batch.reset();
+
+    // Ensure there is no dangling, to-be-reversed db txn
+    BOOST_CHECK(!database->HasActiveTxn());
+
+    // And, just as a sanity check; verify that new batchs only write what they suppose to write
+    // and nothing else.
+    std::string key2 = "key2";
+    std::unique_ptr<SQLiteBatch> batch2 = std::make_unique<SQLiteBatch>(*database);
+    BOOST_CHECK(batch2->Write(key2, value));
+    // The first key must not exist
+    BOOST_CHECK(!batch2->Exists(key));
+}
+
+#endif // USE_SQLITE
+
+
 BOOST_AUTO_TEST_SUITE_END()
 } // namespace wallet