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.cpp

@@ -185,22 +185,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/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();
@@ -141,7 +143,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;
         }
 
@@ -170,7 +172,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

@@ -465,25 +465,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

@@ -664,7 +664,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;
 }
 
@@ -790,6 +790,22 @@ bool BerkeleyBatch::HasKey(CDataStream&& key)
     return ret == 0;
 }
 
+bool BerkeleyBatch::ErasePrefix(const char* data, size_t size)
+{
+    TxnBegin();
+    StartCursor();
+    Dbt prefix((void*)data, size), prefix_value;
+    int ret = m_cursor->get(&prefix, &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() < size || memcmp(key.get_data(), data, 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

@@ -195,6 +195,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(const char* data, size_t size) override;
 
 protected:
     Db* pdb;

src/wallet/db.h

@@ -90,6 +90,8 @@ class DatabaseBatch
         return HasKey(std::move(ssKey));
     }
 
+    virtual bool ErasePrefix(const char* data, size_t size) = 0;
+
     virtual bool StartCursor() = 0;
     virtual bool ReadAtCursor(CDataStream& ssKey, CDataStream& ssValue, bool& complete) = 0;
     virtual void CloseCursor() = 0;
@@ -160,6 +162,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(const char* data, size_t size) override { return true; }
 
 public:
     void Flush() override {}

src/wallet/test/wallet_tests.cpp

@@ -395,19 +395,50 @@ BOOST_AUTO_TEST_CASE(ComputeTimeSmart)
     SetMockTime(0);
 }
 
-BOOST_AUTO_TEST_CASE(LoadReceiveRequests)
+template<typename F>
+void TestLoadWallet(std::shared_ptr<BerkeleyEnvironment> env, F&& 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);
+    auto wallet = std::make_shared<CWallet>(chain.get(), WalletLocation(), MakeUnique<BerkeleyDatabase>(env, ""));
+    bool first_run;
+    wallet->LoadWallet(first_run);
+    WITH_LOCK(wallet->cs_wallet, f(wallet));
+}
+
+BOOST_FIXTURE_TEST_CASE(LoadReceiveRequests, TestingSetup)
+{
+    auto env = std::make_shared<BerkeleyEnvironment>();
+    TestLoadWallet(env, [](std::shared_ptr<CWallet> wallet) {
+        AssertLockHeld(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(env, [](std::shared_ptr<CWallet> wallet) {
+        AssertLockHeld(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(env, [](std::shared_ptr<CWallet> wallet) {
+        AssertLockHeld(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),