diff --git a/src/wallet/sqlite.cpp b/src/wallet/sqlite.cpp index db9989163df..cff36280496 100644 --- a/src/wallet/sqlite.cpp +++ b/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 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 SQLiteBatch::GetNewPrefixCursor(SpanExec(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"); } diff --git a/src/wallet/sqlite.h b/src/wallet/sqlite.h index f1ce0567e18..ad91be10645 100644 --- a/src/wallet/sqlite.h +++ b/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 m_exec_handler{std::make_unique()}; 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&& 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 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; }; diff --git a/src/wallet/test/db_tests.cpp b/src/wallet/test/db_tests.cpp index d341e84d9b5..7e6219378ff 100644 --- a/src/wallet/test/db_tests.cpp +++ b/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 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 m_blocked_statements; +public: + DbExecBlocker(std::set 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 database = MakeSQLiteDatabase(m_path_root / "sqlite", options, status, error); + + std::string key = "key"; + std::string value = "value"; + + std::unique_ptr batch = std::make_unique(*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(std::set{"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 batch2 = std::make_unique(*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