Merge bitcoin/bitcoin#29403: wallet: batch erase procedures and improve 'EraseRecords' performance

77331aa wallet: simplify EraseRecords by using 'ErasePrefix' (furszy)
3375781 wallet: bdb batch 'ErasePrefix', do not create txn internally (furszy)
cf4d72a wallet: db, introduce 'RunWithinTxn()' helper function (furszy)

Pull request description:

  Seeks to optimize and simplify `WalletBatch::EraseRecords`. Currently, this process opens a cursor to iterate over the entire database, searching for records that match the type prefixes, to then call the `WalletBatch::Erase` function for each of the matching records.
  This PR rewrites this 40-line manual process into a single line; instead of performing all of those actions manually, we can simply utilize the `ErasePrefix()` functionality. The result is 06216b344dea6ad6c385fda0b37808ff9ae5273b.

  Moreover, it expands the test coverage for the `ErasePrefix` functionality and documents the db txn requirement for `BerkeleyBatch::ErasePrefix` .

ACKs for top commit:
  achow101:
    reACK 77331aa
  josibake:
    code review ACK bitcoin/bitcoin@77331aa

Tree-SHA512: 9f78dda658677ff19b5979ba0efd11cf9fabf3d315feb79ed1160526f010fe843c41903fc18c0b092f78aa88bc874cf24edad8fc1ea6e96aabdc4fd1daf21ca5

Author: achow101

src/wallet/bdb.cpp

@@ -887,7 +887,12 @@ bool BerkeleyBatch::HasKey(DataStream&& key)
 
 bool BerkeleyBatch::ErasePrefix(Span<const std::byte> prefix)
 {
-    if (!TxnBegin()) return false;
+    // Because this function erases records one by one, ensure that it is executed within a txn context.
+    // Otherwise, consistency is at risk; it's possible that certain records are removed while others
+    // remain due to an internal failure during the procedure.
+    // Additionally, the Dbc::del() cursor delete call below would fail without an active transaction.
+    if (!Assume(activeTxn)) return false;
+
     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
@@ -901,7 +906,7 @@ bool BerkeleyBatch::ErasePrefix(Span<const std::byte> prefix)
         ret = cursor->dbc()->del(0);
     }
     cursor.reset();
-    return TxnCommit() && (ret == 0 || ret == DB_NOTFOUND);
+    return ret == 0 || ret == DB_NOTFOUND;
 }
 
 void BerkeleyDatabase::AddRef()

src/wallet/test/db_tests.cpp

@@ -228,6 +228,39 @@ BOOST_AUTO_TEST_CASE(db_availability_after_write_error)
     }
 }
 
+// Verify 'ErasePrefix' functionality using db keys similar to the ones used by the wallet.
+// Keys are in the form of std::pair<TYPE, ENTRY_ID>
+BOOST_AUTO_TEST_CASE(erase_prefix)
+{
+    const std::string key = "key";
+    const std::string key2 = "key2";
+    const std::string value = "value";
+    const std::string value2 = "value_2";
+    auto make_key = [](std::string type, std::string id) { return std::make_pair(type, id); };
+
+    for (const auto& database : TestDatabases(m_path_root)) {
+        std::unique_ptr<DatabaseBatch> batch = database->MakeBatch();
+
+        // Write two entries with the same key type prefix, a third one with a different prefix
+        // and a fourth one with the type-id values inverted
+        BOOST_CHECK(batch->Write(make_key(key, value), value));
+        BOOST_CHECK(batch->Write(make_key(key, value2), value2));
+        BOOST_CHECK(batch->Write(make_key(key2, value), value));
+        BOOST_CHECK(batch->Write(make_key(value, key), value));
+
+        // Erase the ones with the same prefix and verify result
+        BOOST_CHECK(batch->TxnBegin());
+        BOOST_CHECK(batch->ErasePrefix(DataStream() << key));
+        BOOST_CHECK(batch->TxnCommit());
+
+        BOOST_CHECK(!batch->Exists(make_key(key, value)));
+        BOOST_CHECK(!batch->Exists(make_key(key, value2)));
+        // Also verify that entries with a different prefix were not erased
+        BOOST_CHECK(batch->Exists(make_key(key2, value)));
+        BOOST_CHECK(batch->Exists(make_key(value, key)));
+    }
+}
+
 #ifdef USE_SQLITE
 
 // Test-only statement execution error

src/wallet/test/wallet_tests.cpp

@@ -445,9 +445,11 @@ BOOST_FIXTURE_TEST_CASE(LoadReceiveRequests, TestingSetup)
             auto requests = wallet->GetAddressReceiveRequests();
             auto erequests = {"val_rr11", "val_rr20"};
             BOOST_CHECK_EQUAL_COLLECTIONS(requests.begin(), requests.end(), std::begin(erequests), std::end(erequests));
-            WalletBatch batch{wallet->GetDatabase()};
-            BOOST_CHECK(batch.WriteAddressPreviouslySpent(PKHash(), false));
-            BOOST_CHECK(batch.EraseAddressData(ScriptHash()));
+            RunWithinTxn(wallet->GetDatabase(), /*process_desc*/"test", [](WalletBatch& batch){
+                BOOST_CHECK(batch.WriteAddressPreviouslySpent(PKHash(), false));
+                BOOST_CHECK(batch.EraseAddressData(ScriptHash()));
+                return true;
+            });
         });
         TestLoadWallet(name, format, [](std::shared_ptr<CWallet> wallet) EXCLUSIVE_LOCKS_REQUIRED(wallet->cs_wallet) {
             BOOST_CHECK(!wallet->IsAddressPreviouslySpent(PKHash()));

src/wallet/wallet.cpp

@@ -2411,8 +2411,9 @@ bool CWallet::SetAddressBook(const CTxDestination& address, const std::string& s
 
 bool CWallet::DelAddressBook(const CTxDestination& address)
 {
-    WalletBatch batch(GetDatabase());
-    return DelAddressBookWithDB(batch, address);
+    return RunWithinTxn(GetDatabase(), /*process_desc=*/"address book entry removal", [&](WalletBatch& batch){
+        return DelAddressBookWithDB(batch, address);
+    });
 }
 
 bool CWallet::DelAddressBookWithDB(WalletBatch& batch, const CTxDestination& address)

src/wallet/walletdb.cpp

@@ -1230,6 +1230,35 @@ DBErrors WalletBatch::LoadWallet(CWallet* pwallet)
     return result;
 }
 
+static bool RunWithinTxn(WalletBatch& batch, std::string_view process_desc, const std::function<bool(WalletBatch&)>& func)
+{
+    if (!batch.TxnBegin()) {
+        LogPrint(BCLog::WALLETDB, "Error: cannot create db txn for %s\n", process_desc);
+        return false;
+    }
+
+    // Run procedure
+    if (!func(batch)) {
+        LogPrint(BCLog::WALLETDB, "Error: %s failed\n", process_desc);
+        batch.TxnAbort();
+        return false;
+    }
+
+    if (!batch.TxnCommit()) {
+        LogPrint(BCLog::WALLETDB, "Error: cannot commit db txn for %s\n", process_desc);
+        return false;
+    }
+
+    // All good
+    return true;
+}
+
+bool RunWithinTxn(WalletDatabase& database, std::string_view process_desc, const std::function<bool(WalletBatch&)>& func)
+{
+    WalletBatch batch(database);
+    return RunWithinTxn(batch, process_desc, func);
+}
+
 void MaybeCompactWalletDB(WalletContext& context)
 {
     static std::atomic<bool> fOneThread(false);
@@ -1292,47 +1321,11 @@ 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)
-    {
-        return false;
-    }
-
-    // Iterate the DB and look for any records that have the type prefixes
-    while (true) {
-        // Read next record
-        DataStream key{};
-        DataStream value{};
-        DatabaseCursor::Status status = cursor->Next(key, value);
-        if (status == DatabaseCursor::Status::DONE) {
-            break;
-        } else if (status == DatabaseCursor::Status::FAIL) {
-            cursor.reset(nullptr);
-            m_batch->TxnAbort(); // abort db txn
-            return false;
-        }
-
-        // Make a copy of key to avoid data being deleted by the following read of the type
-        const SerializeData key_data{key.begin(), key.end()};
-
-        std::string type;
-        key >> type;
-
-        if (types.count(type) > 0) {
-            if (!m_batch->Erase(Span{key_data})) {
-                cursor.reset(nullptr);
-                m_batch->TxnAbort();
-                return false; // erase failed
-            }
-        }
-    }
-    // Finish db txn
-    cursor.reset(nullptr);
-    return m_batch->TxnCommit();
+    return RunWithinTxn(*this, "erase records", [&types](WalletBatch& self) {
+        return std::all_of(types.begin(), types.end(), [&self](const std::string& type) {
+            return self.m_batch->ErasePrefix(DataStream() << type);
+        });
+    });
 }
 
 bool WalletBatch::TxnBegin()

src/wallet/walletdb.h

@@ -294,6 +294,20 @@ class WalletBatch
     WalletDatabase& m_database;
 };
 
+/**
+ * Executes the provided function 'func' within a database transaction context.
+ *
+ * This function ensures that all db modifications performed within 'func()' are
+ * atomically committed to the db at the end of the process. And, in case of a
+ * failure during execution, all performed changes are rolled back.
+ *
+ * @param database The db connection instance to perform the transaction on.
+ * @param process_desc A description of the process being executed, used for logging purposes in the event of a failure.
+ * @param func The function to be executed within the db txn context. It returns a boolean indicating whether to commit or roll back the txn.
+ * @return true if the db txn executed successfully, false otherwise.
+ */
+bool RunWithinTxn(WalletDatabase& database, std::string_view process_desc, const std::function<bool(WalletBatch&)>& func);
+
 //! Compacts BDB state so that wallet.dat is self-contained (if there are changes)
 void MaybeCompactWalletDB(WalletContext& context);