Merge bitcoin/bitcoin#27556: wallet: fix deadlock in bdb read write operation

69d43905b7 test: add coverage for wallet read write db deadlock (furszy)
12daf6fcdc walletdb: scope bdb::EraseRecords under a single db txn (furszy)
043fcb0b05 wallet: bugfix, GetNewCursor() misses to provide batch ptr to BerkeleyCursor (furszy)

Pull request description:

  Decoupled from #26644 so it can closed in favor of #26715.

  Basically, with bdb, we can't make a write operation while we are traversing the db with the same db handler. These two operations are performed in different txn contexts and cause a deadlock.

  Added coverage by using `EraseRecords()` which is the simplest function that executes this process.

  To replicate it, need bdb support and drop the first commit. The test will run forever.

ACKs for top commit:
  achow101:
    ACK 69d43905b7
  hebasto:
    re-ACK 69d43905b7

Tree-SHA512: b3773be78925f674e962f4a5c54b398a9d0cfe697148c01c3ec0d68281cc5c1444b38165960d219ef3cf1a57c8ce6427f44a876275958d49bbc0808486e19d7d

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<DatabaseCursor> BerkeleyBatch::GetNewCursor()
 {
     if (!pdb) return nullptr;
-    return std::make_unique<BerkeleyCursor>(m_database);
+    return std::make_unique<BerkeleyCursor>(m_database, *this);
 }
 
 bool BerkeleyBatch::TxnBegin()
@@ -825,7 +825,7 @@ bool BerkeleyBatch::HasKey(DataStream&& key)
 bool BerkeleyBatch::ErasePrefix(Span<const std::byte> prefix)
 {
     if (!TxnBegin()) return false;
-    auto cursor{std::make_unique<BerkeleyCursor>(m_database, this)};
+    auto cursor{std::make_unique<BerkeleyCursor>(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

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;

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<CWallet> CreateSyncedWallet(interfaces::Chain& chain, CChain& cchain, const CKey& key);
 
 // Creates a copy of the provided database

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<void(std::shared_ptr<CWallet>)> f)
 {
     node::NodeContext node;

src/wallet/test/walletdb_tests.cpp

@@ -6,6 +6,8 @@
 #include <clientversion.h>
 #include <streams.h>
 #include <uint256.h>
+#include <wallet/test/util.h>
+#include <wallet/wallet.h>
 
 #include <boost/test/unit_test.hpp>
 
@@ -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<WalletDatabase> 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<CWallet> 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

src/wallet/walletdb.cpp

@@ -1136,6 +1136,9 @@ bool WalletBatch::WriteWalletFlags(const uint64_t flags)
 
 bool WalletBatch::EraseRecords(const std::unordered_set<std::string>& types)
 {
+    // Begin db txn
+    if (!m_batch->TxnBegin()) return false;
+
     // Get cursor
     std::unique_ptr<DatabaseCursor> cursor = m_batch->GetNewCursor();
     if (!cursor)
@@ -1144,8 +1147,7 @@ bool WalletBatch::EraseRecords(const std::unordered_set<std::string>& 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<std::string>& 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<std::string>& 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()