From fdf9f66909a354a95f4b7c5f092f0e9fbe1baa7c Mon Sep 17 00:00:00 2001 From: furszy Date: Wed, 20 Dec 2023 12:07:13 -0300 Subject: [PATCH 1/5] test: wallet db, exercise deadlock after write failure --- src/wallet/test/db_tests.cpp | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/src/wallet/test/db_tests.cpp b/src/wallet/test/db_tests.cpp index d341e84d9b5..c6b2154efdd 100644 --- a/src/wallet/test/db_tests.cpp +++ b/src/wallet/test/db_tests.cpp @@ -205,5 +205,29 @@ 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); + } +} + + BOOST_AUTO_TEST_SUITE_END() } // namespace wallet From dca874e838c2599bd24433675b291168f8e7b055 Mon Sep 17 00:00:00 2001 From: furszy Date: Mon, 8 Jan 2024 11:18:25 -0300 Subject: [PATCH 2/5] sqlite: add ability to interrupt statements By encapsulating sqlite3_exec into its own standalone method and introducing the 'SQliteExecHandler' class, we enable the ability to test db statements execution failures within the unit test framework. This is used in the following-up commit to exercise a deadlock and improve our wallet db error handling code. Moreover, the future encapsulation of other sqlite functions within this class will contribute to minimize the impact of any future API changes. --- src/wallet/sqlite.cpp | 11 ++++++++--- src/wallet/sqlite.h | 12 ++++++++++++ 2 files changed, 20 insertions(+), 3 deletions(-) diff --git a/src/wallet/sqlite.cpp b/src/wallet/sqlite.cpp index db9989163df..bcb77c18f77 100644 --- a/src/wallet/sqlite.cpp +++ b/src/wallet/sqlite.cpp @@ -377,6 +377,11 @@ void SQLiteDatabase::Close() m_db = nullptr; } +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 @@ -607,7 +612,7 @@ std::unique_ptr SQLiteBatch::GetNewPrefixCursor(SpanExec(m_database, "BEGIN TRANSACTION"); if (res != SQLITE_OK) { LogPrintf("SQLiteBatch: Failed to begin the transaction\n"); } @@ -617,7 +622,7 @@ 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); + int res = Assert(m_exec_handler)->Exec(m_database, "COMMIT TRANSACTION"); if (res != SQLITE_OK) { LogPrintf("SQLiteBatch: Failed to commit the transaction\n"); } @@ -627,7 +632,7 @@ 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); + 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..de9ba8fd99c 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 {} From 472d2ca98170049e0edec830e2d11c5ef23740a4 Mon Sep 17 00:00:00 2001 From: furszy Date: Mon, 15 Jan 2024 20:18:34 -0300 Subject: [PATCH 3/5] sqlite: introduce HasActiveTxn method Util function to clean up code and let us verify, in the following-up commit, that dangling, to-be-reverted db transactions cannot occur anymore. --- src/wallet/sqlite.cpp | 16 +++++++++++----- src/wallet/sqlite.h | 3 +++ 2 files changed, 14 insertions(+), 5 deletions(-) diff --git a/src/wallet/sqlite.cpp b/src/wallet/sqlite.cpp index bcb77c18f77..89bb917b526 100644 --- a/src/wallet/sqlite.cpp +++ b/src/wallet/sqlite.cpp @@ -377,6 +377,12 @@ 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); @@ -399,8 +405,8 @@ 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) { + // 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 { @@ -611,7 +617,7 @@ std::unique_ptr SQLiteBatch::GetNewPrefixCursor(SpanExec(m_database, "BEGIN TRANSACTION"); if (res != SQLITE_OK) { LogPrintf("SQLiteBatch: Failed to begin the transaction\n"); @@ -621,7 +627,7 @@ bool SQLiteBatch::TxnBegin() bool SQLiteBatch::TxnCommit() { - if (!m_database.m_db || sqlite3_get_autocommit(m_database.m_db) != 0) return false; + 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"); @@ -631,7 +637,7 @@ bool SQLiteBatch::TxnCommit() bool SQLiteBatch::TxnAbort() { - if (!m_database.m_db || sqlite3_get_autocommit(m_database.m_db) != 0) return false; + 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 de9ba8fd99c..ad91be10645 100644 --- a/src/wallet/sqlite.h +++ b/src/wallet/sqlite.h @@ -154,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; }; From fc0e747192e98e779c5f31f2df808f62b3fdd071 Mon Sep 17 00:00:00 2001 From: furszy Date: Mon, 15 Jan 2024 20:43:09 -0300 Subject: [PATCH 4/5] sqlite: guard against dangling to-be-reverted db transactions If the 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 reversed; 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 database isolation property but also results in the improper storage of incomplete information on disk, impacting the wallet consistency. --- src/wallet/sqlite.cpp | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-) diff --git a/src/wallet/sqlite.cpp b/src/wallet/sqlite.cpp index 89bb917b526..cff36280496 100644 --- a/src/wallet/sqlite.cpp +++ b/src/wallet/sqlite.cpp @@ -405,12 +405,18 @@ SQLiteBatch::SQLiteBatch(SQLiteDatabase& database) void SQLiteBatch::Close() { + 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"); } } @@ -431,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) From b298242c8d495c36072415e1b95eaa7bf485a38a Mon Sep 17 00:00:00 2001 From: furszy Date: Mon, 8 Jan 2024 11:19:25 -0300 Subject: [PATCH 5/5] test: sqlite, add coverage for dangling to-be-reverted db txns --- src/wallet/test/db_tests.cpp | 53 ++++++++++++++++++++++++++++++++++++ 1 file changed, 53 insertions(+) diff --git a/src/wallet/test/db_tests.cpp b/src/wallet/test/db_tests.cpp index c6b2154efdd..7e6219378ff 100644 --- a/src/wallet/test/db_tests.cpp +++ b/src/wallet/test/db_tests.cpp @@ -228,6 +228,59 @@ BOOST_AUTO_TEST_CASE(db_availability_after_write_error) } } +#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