Merge a01da41112 into merged_master (Bitcoin PR bitcoin/bitcoin#29253)

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