From 043fcb0b053eee6021cc75e3d3f41097f52698c0 Mon Sep 17 00:00:00 2001 From: furszy Date: Mon, 1 May 2023 10:21:36 -0300 Subject: [PATCH 1/3] wallet: bugfix, GetNewCursor() misses to provide batch ptr to BerkeleyCursor If the batch ptr is not passed, the cursor will not use the db active txn context which could lead to a deadlock if the code tries to modify the db while it is traversing it. E.g. the 'EraseRecords()' function. --- src/wallet/bdb.cpp | 8 ++++---- src/wallet/bdb.h | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/wallet/bdb.cpp b/src/wallet/bdb.cpp index fa6e56c6e4..6dce51fc12 100644 --- a/src/wallet/bdb.cpp +++ b/src/wallet/bdb.cpp @@ -668,14 +668,14 @@ void BerkeleyDatabase::ReloadDbEnv() env->ReloadDbEnv(); } -BerkeleyCursor::BerkeleyCursor(BerkeleyDatabase& database, BerkeleyBatch* batch) +BerkeleyCursor::BerkeleyCursor(BerkeleyDatabase& database, const BerkeleyBatch& batch) { if (!database.m_db.get()) { throw std::runtime_error(STR_INTERNAL_BUG("BerkeleyDatabase does not exist")); } // Transaction argument to cursor is only needed when using the cursor to // write to the database. Read-only cursors do not need a txn pointer. - int ret = database.m_db->cursor(batch ? batch->txn() : nullptr, &m_cursor, 0); + int ret = database.m_db->cursor(batch.txn(), &m_cursor, 0); if (ret != 0) { throw std::runtime_error(STR_INTERNAL_BUG(strprintf("BDB Cursor could not be created. Returned %d", ret))); } @@ -713,7 +713,7 @@ BerkeleyCursor::~BerkeleyCursor() std::unique_ptr BerkeleyBatch::GetNewCursor() { if (!pdb) return nullptr; - return std::make_unique(m_database); + return std::make_unique(m_database, *this); } bool BerkeleyBatch::TxnBegin() @@ -825,7 +825,7 @@ bool BerkeleyBatch::HasKey(DataStream&& key) bool BerkeleyBatch::ErasePrefix(Span prefix) { if (!TxnBegin()) return false; - auto cursor{std::make_unique(m_database, this)}; + auto cursor{std::make_unique(m_database, *this)}; // const_cast is safe below even though prefix_key is an in/out parameter, // because we are not using the DB_DBT_USERMEM flag, so BDB will allocate // and return a different output data pointer diff --git a/src/wallet/bdb.h b/src/wallet/bdb.h index 4fac128bf1..9134643b23 100644 --- a/src/wallet/bdb.h +++ b/src/wallet/bdb.h @@ -192,7 +192,7 @@ private: Dbc* m_cursor; public: - explicit BerkeleyCursor(BerkeleyDatabase& database, BerkeleyBatch* batch=nullptr); + explicit BerkeleyCursor(BerkeleyDatabase& database, const BerkeleyBatch& batch); ~BerkeleyCursor() override; Status Next(DataStream& key, DataStream& value) override; From 12daf6fcdcbf5c7f03038338d843df3877714b24 Mon Sep 17 00:00:00 2001 From: furszy Date: Mon, 5 Dec 2022 18:04:56 -0300 Subject: [PATCH 2/3] walletdb: scope bdb::EraseRecords under a single db txn so we erase all the records atomically or abort the entire procedure. and, at the same time, we can share the same db txn context for the db cursor and the erase functionality. extra note from the Db.cursor doc: "If transaction protection is enabled, cursors must be opened and closed within the context of a transaction" thus why added a `CloseCursor` call before calling to `TxnAbort/TxnCommit`. --- src/wallet/walletdb.cpp | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/src/wallet/walletdb.cpp b/src/wallet/walletdb.cpp index 072357022e..9a6c5afba6 100644 --- a/src/wallet/walletdb.cpp +++ b/src/wallet/walletdb.cpp @@ -1136,6 +1136,9 @@ bool WalletBatch::WriteWalletFlags(const uint64_t flags) bool WalletBatch::EraseRecords(const std::unordered_set& types) { + // Begin db txn + if (!m_batch->TxnBegin()) return false; + // Get cursor std::unique_ptr cursor = m_batch->GetNewCursor(); if (!cursor) @@ -1144,8 +1147,7 @@ bool WalletBatch::EraseRecords(const std::unordered_set& types) } // Iterate the DB and look for any records that have the type prefixes - while (true) - { + while (true) { // Read next record DataStream key{}; DataStream value{}; @@ -1153,6 +1155,8 @@ bool WalletBatch::EraseRecords(const std::unordered_set& types) if (status == DatabaseCursor::Status::DONE) { break; } else if (status == DatabaseCursor::Status::FAIL) { + cursor.reset(nullptr); + m_batch->TxnAbort(); // abort db txn return false; } @@ -1163,10 +1167,16 @@ bool WalletBatch::EraseRecords(const std::unordered_set& types) key >> type; if (types.count(type) > 0) { - m_batch->Erase(key_data); + if (!m_batch->Erase(key_data)) { + cursor.reset(nullptr); + m_batch->TxnAbort(); + return false; // erase failed + } } } - return true; + // Finish db txn + cursor.reset(nullptr); + return m_batch->TxnCommit(); } bool WalletBatch::TxnBegin() From 69d43905b7f1d4849dfaea1b5744ee473ccc8744 Mon Sep 17 00:00:00 2001 From: furszy Date: Tue, 2 May 2023 12:19:33 -0300 Subject: [PATCH 3/3] test: add coverage for wallet read write db deadlock --- src/wallet/test/util.h | 9 +++++++++ src/wallet/test/wallet_tests.cpp | 9 --------- src/wallet/test/walletdb_tests.cpp | 28 ++++++++++++++++++++++++++++ 3 files changed, 37 insertions(+), 9 deletions(-) diff --git a/src/wallet/test/util.h b/src/wallet/test/util.h index 01c2458a6c..eb1cfd9e21 100644 --- a/src/wallet/test/util.h +++ b/src/wallet/test/util.h @@ -22,6 +22,15 @@ namespace wallet { class CWallet; class WalletDatabase; +static const DatabaseFormat DATABASE_FORMATS[] = { +#ifdef USE_SQLITE + DatabaseFormat::SQLITE, +#endif +#ifdef USE_BDB + DatabaseFormat::BERKELEY, +#endif +}; + std::unique_ptr CreateSyncedWallet(interfaces::Chain& chain, CChain& cchain, const CKey& key); // Creates a copy of the provided database diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp index 805ddf3a57..194c8663db 100644 --- a/src/wallet/test/wallet_tests.cpp +++ b/src/wallet/test/wallet_tests.cpp @@ -426,15 +426,6 @@ BOOST_AUTO_TEST_CASE(ComputeTimeSmart) BOOST_CHECK_EQUAL(AddTx(*m_node.chainman, m_wallet, 5, 50, 600), 300); } -static const DatabaseFormat DATABASE_FORMATS[] = { -#ifdef USE_SQLITE - DatabaseFormat::SQLITE, -#endif -#ifdef USE_BDB - DatabaseFormat::BERKELEY, -#endif -}; - void TestLoadWallet(const std::string& name, DatabaseFormat format, std::function)> f) { node::NodeContext node; diff --git a/src/wallet/test/walletdb_tests.cpp b/src/wallet/test/walletdb_tests.cpp index 21842fe780..00b2f47e14 100644 --- a/src/wallet/test/walletdb_tests.cpp +++ b/src/wallet/test/walletdb_tests.cpp @@ -6,6 +6,8 @@ #include #include #include +#include +#include #include @@ -27,5 +29,31 @@ BOOST_AUTO_TEST_CASE(walletdb_readkeyvalue) BOOST_CHECK_THROW(ssValue >> dummy, std::ios_base::failure); } +BOOST_AUTO_TEST_CASE(walletdb_read_write_deadlock) +{ + // Exercises a db read write operation that shouldn't deadlock. + for (const DatabaseFormat& db_format : DATABASE_FORMATS) { + // Context setup + DatabaseOptions options; + options.require_format = db_format; + DatabaseStatus status; + bilingual_str error_string; + std::unique_ptr db = MakeDatabase(m_path_root / strprintf("wallet_%d_.dat", db_format).c_str(), options, status, error_string); + BOOST_ASSERT(status == DatabaseStatus::SUCCESS); + + std::shared_ptr wallet(new CWallet(m_node.chain.get(), "", std::move(db))); + wallet->m_keypool_size = 4; + + // Create legacy spkm + LOCK(wallet->cs_wallet); + auto legacy_spkm = wallet->GetOrCreateLegacyScriptPubKeyMan(); + BOOST_CHECK(legacy_spkm->SetupGeneration(true)); + wallet->Flush(); + + // Now delete all records, which performs a read write operation. + BOOST_CHECK(wallet->GetLegacyScriptPubKeyMan()->DeleteRecords()); + } +} + BOOST_AUTO_TEST_SUITE_END() } // namespace wallet