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; 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 diff --git a/src/wallet/walletdb.cpp b/src/wallet/walletdb.cpp index e99ac53b16..ddabdac99f 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()