refactor: Remove CAddressBookData::destdata

This is cleanup that doesn't change external behavior.

- Removes awkward `StringMap` intermediate representation
- Deals with receive request "rr" keys in walletdb.cpp instead of all over qt, wallet, and interfaces code
- Deals with destination "used" keys in walletdb.cpp instead of all over wallet code
- Adds test coverage
- Reduces code (+85/-138 lines)
- Reduces memory usage

This PR doesn't change externally observable behavior. Internally, only change in behavior is that EraseDestData deletes directly from database because the `StringMap` is gone. This is more direct and efficient because it uses a single btree lookup and scan instead of multiple lookups

Motivation for this cleanup is making changes like bitcoin#18550, bitcoin#18192, bitcoin#13756 easier to reason about and less likely to result in unintended behavior and bugs

src/interfaces/wallet.h

@@ -112,14 +112,11 @@ class Wallet
     //! Get wallet address list.
     virtual std::vector<WalletAddress> getAddresses() = 0;
 
-    //! Add dest data.
-    virtual bool addDestData(const CTxDestination& dest, const std::string& key, const std::string& value) = 0;
+    //! Get receive requests.
+    virtual std::vector<std::string> getReceiveRequests() = 0;
 
-    //! Erase dest data.
-    virtual bool eraseDestData(const CTxDestination& dest, const std::string& key) = 0;
-
-    //! Get dest values with prefix.
-    virtual std::vector<std::string> getDestValues(const std::string& prefix) = 0;
+    //! Save receive request.
+    virtual bool saveReceiveRequest(const CTxDestination& dest, const std::string& id, const std::string& value) = 0;
 
     //! Lock coin.
     virtual void lockCoin(const COutPoint& output) = 0;

src/qt/recentrequeststablemodel.cpp

@@ -10,18 +10,20 @@
 #include <qt/walletmodel.h>
 
 #include <clientversion.h>
+#include <interfaces/wallet.h>
+#include <key_io.h>
 #include <streams.h>
+#include <util/string.h>
 
 #include <utility>
 
 RecentRequestsTableModel::RecentRequestsTableModel(WalletModel *parent) :
     QAbstractTableModel(parent), walletModel(parent)
 {
     // Load entries from wallet
-    std::vector<std::string> vReceiveRequests;
-    parent->loadReceiveRequests(vReceiveRequests);
-    for (const std::string& request : vReceiveRequests)
+    for (const std::string& request : parent->wallet().getReceiveRequests()) {
         addNewRequest(request);
+    }
 
     /* These columns must match the indices in the ColumnIndex enumeration */
     columns << tr("Date") << tr("Label") << tr("Message") << getAmountTitle();
@@ -143,7 +145,7 @@ bool RecentRequestsTableModel::removeRows(int row, int count, const QModelIndex
         for (int i = 0; i < count; ++i)
         {
             const RecentRequestEntry* rec = &list[row+i];
-            if (!walletModel->saveReceiveRequest(rec->recipient.address.toStdString(), rec->id, ""))
+            if (!walletModel->wallet().saveReceiveRequest(DecodeDestination(rec->recipient.address.toStdString()), ToString(rec->id), ""))
                 return false;
         }
 
@@ -172,7 +174,7 @@ void RecentRequestsTableModel::addNewRequest(const SendCoinsRecipient &recipient
     CDataStream ss(SER_DISK, CLIENT_VERSION);
     ss << newEntry;
 
-    if (!walletModel->saveReceiveRequest(recipient.address.toStdString(), newEntry.id, ss.str()))
+    if (!walletModel->wallet().saveReceiveRequest(DecodeDestination(recipient.address.toStdString()), ToString(newEntry.id), ss.str()))
         return;
 
     addNewRequest(newEntry);

src/qt/walletmodel.cpp

@@ -462,25 +462,6 @@ void WalletModel::UnlockContext::CopyFrom(UnlockContext&& rhs)
     rhs.relock = false;
 }
 
-void WalletModel::loadReceiveRequests(std::vector<std::string>& vReceiveRequests)
-{
-    vReceiveRequests = m_wallet->getDestValues("rr"); // receive request
-}
-
-bool WalletModel::saveReceiveRequest(const std::string &sAddress, const int64_t nId, const std::string &sRequest)
-{
-    CTxDestination dest = DecodeDestination(sAddress);
-
-    std::stringstream ss;
-    ss << nId;
-    std::string key = "rr" + ss.str(); // "rr" prefix = "receive request" in destdata
-
-    if (sRequest.empty())
-        return m_wallet->eraseDestData(dest, key);
-    else
-        return m_wallet->addDestData(dest, key, sRequest);
-}
-
 bool WalletModel::bumpFee(uint256 hash, uint256& new_hash)
 {
     CCoinControl coin_control;

src/qt/walletmodel.h

@@ -135,9 +135,6 @@ class WalletModel : public QObject
 
     UnlockContext requestUnlock();
 
-    void loadReceiveRequests(std::vector<std::string>& vReceiveRequests);
-    bool saveReceiveRequest(const std::string &sAddress, const int64_t nId, const std::string &sRequest);
-
     bool bumpFee(uint256 hash, uint256& new_hash);
 
     static bool isWalletEnabled();

src/wallet/bdb.cpp

@@ -657,7 +657,7 @@ bool BerkeleyBatch::StartCursor()
     assert(!m_cursor);
     if (!pdb)
         return false;
-    int ret = pdb->cursor(nullptr, &m_cursor, 0);
+    int ret = pdb->cursor(activeTxn, &m_cursor, 0);
     return ret == 0;
 }
 
@@ -800,6 +800,22 @@ bool BerkeleyBatch::HasKey(CDataStream&& key)
     return ret == 0;
 }
 
+bool BerkeleyBatch::ErasePrefix(Span<char> prefix)
+{
+    TxnBegin();
+    StartCursor();
+    Dbt prefix_key(prefix.data(), prefix.size()), prefix_value;
+    int ret = m_cursor->get(&prefix_key, &prefix_value, DB_SET_RANGE);
+    for (int flag = DB_CURRENT; ret == 0; flag = DB_NEXT) {
+        SafeDbt key, value;
+        if ((ret = m_cursor->get(key, value, flag)) != 0 || key.get_size() < prefix.size() || memcmp(key.get_data(), prefix.data(), prefix.size()) != 0) break;
+        m_cursor->del(0);
+    }
+    CloseCursor();
+    TxnCommit();
+    return ret == 0 || ret == DB_NOTFOUND;
+}
+
 void BerkeleyDatabase::AddRef()
 {
     LOCK(cs_db);

src/wallet/bdb.h

@@ -192,6 +192,7 @@ class BerkeleyBatch : public DatabaseBatch
     bool WriteKey(CDataStream&& key, CDataStream&& value, bool overwrite = true) override;
     bool EraseKey(CDataStream&& key) override;
     bool HasKey(CDataStream&& key) override;
+    bool ErasePrefix(Span<char> prefix) override;
 
 protected:
     Db* pdb;

src/wallet/db.h

@@ -91,6 +91,8 @@ class DatabaseBatch
         return HasKey(std::move(ssKey));
     }
 
+    virtual bool ErasePrefix(Span<char> prefix) = 0;
+
     virtual bool StartCursor() = 0;
     virtual bool ReadAtCursor(CDataStream& ssKey, CDataStream& ssValue, bool& complete) = 0;
     virtual void CloseCursor() = 0;
@@ -163,6 +165,7 @@ class DummyBatch : public DatabaseBatch
     bool WriteKey(CDataStream&& key, CDataStream&& value, bool overwrite=true) override { return true; }
     bool EraseKey(CDataStream&& key) override { return true; }
     bool HasKey(CDataStream&& key) override { return true; }
+    bool ErasePrefix(Span<char> prefix) override { return true; }
 
 public:
     void Flush() override {}

src/wallet/interfaces.cpp

@@ -198,22 +198,20 @@ class WalletImpl : public Wallet
         }
         return result;
     }
-    bool addDestData(const CTxDestination& dest, const std::string& key, const std::string& value) override
-    {
+    std::vector<std::string> getReceiveRequests() override {
         LOCK(m_wallet->cs_wallet);
-        WalletBatch batch{m_wallet->GetDatabase()};
-        return m_wallet->AddDestData(batch, dest, key, value);
+        std::vector<std::string> requests;
+        for (const auto& dest : m_wallet->m_address_book) {
+            for (const auto& request : dest.second.GetReceiveRequests()) {
+                requests.emplace_back(request.second);
+            }
+        }
+        return requests;
     }
-    bool eraseDestData(const CTxDestination& dest, const std::string& key) override
-    {
+    bool saveReceiveRequest(const CTxDestination& dest, const std::string& id, const std::string& value) override {
         LOCK(m_wallet->cs_wallet);
         WalletBatch batch{m_wallet->GetDatabase()};
-        return m_wallet->EraseDestData(batch, dest, key);
-    }
-    std::vector<std::string> getDestValues(const std::string& prefix) override
-    {
-        LOCK(m_wallet->cs_wallet);
-        return m_wallet->GetDestValues(prefix);
+        return m_wallet->m_address_book[dest].SetReceiveRequest(id, value) && batch.WriteReceiveRequest(dest, id, value);
     }
     void lockCoin(const COutPoint& output) override
     {

src/wallet/sqlite.cpp

@@ -91,6 +91,11 @@ void SQLiteBatch::SetupSQLStatements()
             throw std::runtime_error(strprintf("SQLiteDatabase: Failed to setup SQL statements: %s\n", sqlite3_errstr(res)));
         }
     }
+    if (!m_delete_prefix_stmt) {
+        if ((res = sqlite3_prepare_v2(m_database.m_db, "DELETE FROM main WHERE instr(?, key) = 1", -1, &m_delete_prefix_stmt, nullptr)) != SQLITE_OK) {
+            throw std::runtime_error(strprintf("SQLiteDatabase: Failed to setup SQL statements: %s\n", sqlite3_errstr(res)));
+        }
+    }
     if (!m_cursor_stmt) {
         if ((res = sqlite3_prepare_v2(m_database.m_db, "SELECT key, value FROM main", -1, &m_cursor_stmt, nullptr)) != SQLITE_OK) {
             throw std::runtime_error(strprintf("SQLiteDatabase: Failed to setup SQL statements : %s\n", sqlite3_errstr(res)));
@@ -370,6 +375,10 @@ void SQLiteBatch::Close()
     if (ret != SQLITE_OK) {
         LogPrintf("SQLiteBatch: Batch closed but could not finalize delete statement: %s\n", sqlite3_errstr(ret));
     }
+    ret = sqlite3_finalize(m_delete_prefix_stmt);
+    if (ret != SQLITE_OK) {
+        LogPrintf("SQLiteBatch: Batch closed but could not finalize delete prefix statement: %s\n", sqlite3_errstr(ret));
+    }
     ret = sqlite3_finalize(m_cursor_stmt);
     if (ret != SQLITE_OK) {
         LogPrintf("SQLiteBatch: Batch closed but could not finalize cursor statement: %s\n", sqlite3_errstr(ret));
@@ -378,6 +387,7 @@ void SQLiteBatch::Close()
     m_insert_stmt = nullptr;
     m_overwrite_stmt = nullptr;
     m_delete_stmt = nullptr;
+    m_delete_prefix_stmt = nullptr;
     m_cursor_stmt = nullptr;
 }
 
@@ -453,30 +463,40 @@ bool SQLiteBatch::WriteKey(CDataStream&& key, CDataStream&& value, bool overwrit
     return res == SQLITE_DONE;
 }
 
-bool SQLiteBatch::EraseKey(CDataStream&& key)
+bool SQLiteBatch::ExecStatement(sqlite3_stmt* stmt, Span<const char> blob)
 {
     if (!m_database.m_db) return false;
-    assert(m_delete_stmt);
+    assert(stmt);
 
     // Bind: leftmost parameter in statement is index 1
-    int res = sqlite3_bind_blob(m_delete_stmt, 1, key.data(), key.size(), SQLITE_STATIC);
+    int res = sqlite3_bind_blob(stmt, 1, blob.data(), blob.size(), SQLITE_STATIC);
     if (res != SQLITE_OK) {
         LogPrintf("%s: Unable to bind statement: %s\n", __func__, sqlite3_errstr(res));
-        sqlite3_clear_bindings(m_delete_stmt);
-        sqlite3_reset(m_delete_stmt);
+        sqlite3_clear_bindings(stmt);
+        sqlite3_reset(stmt);
         return false;
     }
 
     // Execute
-    res = sqlite3_step(m_delete_stmt);
-    sqlite3_clear_bindings(m_delete_stmt);
-    sqlite3_reset(m_delete_stmt);
+    res = sqlite3_step(stmt);
+    sqlite3_clear_bindings(stmt);
+    sqlite3_reset(stmt);
     if (res != SQLITE_DONE) {
         LogPrintf("%s: Unable to execute statement: %s\n", __func__, sqlite3_errstr(res));
     }
     return res == SQLITE_DONE;
 }
 
+bool SQLiteBatch::EraseKey(CDataStream&& key)
+{
+    return ExecStatement(m_delete_stmt, key);
+}
+
+bool SQLiteBatch::ErasePrefix(Span<char> prefix)
+{
+    return ExecStatement(m_delete_prefix_stmt, prefix);
+}
+
 bool SQLiteBatch::HasKey(CDataStream&& key)
 {
     if (!m_database.m_db) return false;

src/wallet/sqlite.h

@@ -24,14 +24,17 @@ class SQLiteBatch : public DatabaseBatch
     sqlite3_stmt* m_insert_stmt{nullptr};
     sqlite3_stmt* m_overwrite_stmt{nullptr};
     sqlite3_stmt* m_delete_stmt{nullptr};
+    sqlite3_stmt* m_delete_prefix_stmt{nullptr};
     sqlite3_stmt* m_cursor_stmt{nullptr};
 
     void SetupSQLStatements();
+    bool ExecStatement(sqlite3_stmt* stmt, Span<const char> blob);
 
     bool ReadKey(CDataStream&& key, CDataStream& value) override;
     bool WriteKey(CDataStream&& key, CDataStream&& value, bool overwrite = true) override;
     bool EraseKey(CDataStream&& key) override;
     bool HasKey(CDataStream&& key) override;
+    bool ErasePrefix(Span<char> prefix) override;
 
 public:
     explicit SQLiteBatch(SQLiteDatabase& database);

src/wallet/test/wallet_tests.cpp

@@ -385,19 +385,51 @@ BOOST_AUTO_TEST_CASE(ComputeTimeSmart)
     SetMockTime(0);
 }
 
-BOOST_AUTO_TEST_CASE(LoadReceiveRequests)
+void TestLoadWallet(const std::string& name, std::function<void(std::shared_ptr<CWallet>)> f)
 {
-    CTxDestination dest = PKHash();
-    LOCK(m_wallet.cs_wallet);
-    WalletBatch batch{m_wallet.GetDatabase()};
-    m_wallet.AddDestData(batch, dest, "misc", "val_misc");
-    m_wallet.AddDestData(batch, dest, "rr0", "val_rr0");
-    m_wallet.AddDestData(batch, dest, "rr1", "val_rr1");
-
-    auto values = m_wallet.GetDestValues("rr");
-    BOOST_CHECK_EQUAL(values.size(), 2U);
-    BOOST_CHECK_EQUAL(values[0], "val_rr0");
-    BOOST_CHECK_EQUAL(values[1], "val_rr1");
+    NodeContext node;
+    auto chain = interfaces::MakeChain(node);
+    DatabaseOptions options;
+    DatabaseStatus status;
+    bilingual_str error;
+    std::vector<bilingual_str> warnings;
+    auto database = MakeWalletDatabase(name, options, status, error);
+    auto wallet = std::make_shared<CWallet>(chain.get(), "", std::move(database));
+    bool first_run;
+    wallet->LoadWallet(first_run);
+    WITH_LOCK(wallet->cs_wallet, f(wallet));
+}
+
+BOOST_FIXTURE_TEST_CASE(LoadReceiveRequests, TestingSetup)
+{
+    const std::string& name = "receive-requests";
+    TestLoadWallet(name, [](std::shared_ptr<CWallet> wallet) EXCLUSIVE_LOCKS_REQUIRED(wallet->cs_wallet) {
+        BOOST_CHECK(!wallet->m_address_book[PKHash()].IsUsed());
+        BOOST_CHECK(WalletBatch(wallet->GetDatabase()).WriteUsed(PKHash(), true));
+        BOOST_CHECK(WalletBatch(wallet->GetDatabase()).WriteUsed(ScriptHash(), true));
+        BOOST_CHECK(interfaces::MakeWallet(wallet)->saveReceiveRequest(PKHash(), "0", "val_rr00"));
+        BOOST_CHECK(interfaces::MakeWallet(wallet)->saveReceiveRequest(PKHash(), "0", ""));
+        BOOST_CHECK(!interfaces::MakeWallet(wallet)->saveReceiveRequest(PKHash(), "0", ""));
+        BOOST_CHECK(interfaces::MakeWallet(wallet)->saveReceiveRequest(PKHash(), "1", "val_rr10"));
+        BOOST_CHECK(interfaces::MakeWallet(wallet)->saveReceiveRequest(PKHash(), "1", "val_rr11"));
+        BOOST_CHECK(interfaces::MakeWallet(wallet)->saveReceiveRequest(ScriptHash(), "2", "val_rr20"));
+    });
+    TestLoadWallet(name, [](std::shared_ptr<CWallet> wallet) EXCLUSIVE_LOCKS_REQUIRED(wallet->cs_wallet) {
+        BOOST_CHECK(wallet->m_address_book[PKHash()].IsUsed());
+        BOOST_CHECK(wallet->m_address_book[ScriptHash()].IsUsed());
+        auto requests = interfaces::MakeWallet(wallet)->getReceiveRequests();
+        auto erequests = {"val_rr11", "val_rr20"};
+        BOOST_CHECK_EQUAL_COLLECTIONS(requests.begin(), requests.end(), std::begin(erequests), std::end(erequests));
+        BOOST_CHECK(WalletBatch(wallet->GetDatabase()).WriteUsed(PKHash(), false));
+        BOOST_CHECK(WalletBatch(wallet->GetDatabase()).EraseDestData(ScriptHash()));
+    });
+    TestLoadWallet(name, [](std::shared_ptr<CWallet> wallet) EXCLUSIVE_LOCKS_REQUIRED(wallet->cs_wallet) {
+        BOOST_CHECK(!wallet->m_address_book[PKHash()].IsUsed());
+        BOOST_CHECK(!wallet->m_address_book[ScriptHash()].IsUsed());
+        auto requests = interfaces::MakeWallet(wallet)->getReceiveRequests();
+        auto erequests = {"val_rr11"};
+        BOOST_CHECK_EQUAL_COLLECTIONS(requests.begin(), requests.end(), std::begin(erequests), std::end(erequests));
+    });
 }
 
 // Test some watch-only LegacyScriptPubKeyMan methods by the procedure of loading (LoadWatchOnly),