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
ryanofsky · Apr 12, 2020 · 0502b35 · 0502b35
1 parent eef90c1
commit 0502b35
Show file tree

Hide file tree

Showing 11 changed files with 166 additions and 141 deletions.
diff --git a/src/interfaces/wallet.cpp b/src/interfaces/wallet.cpp
@@ -177,22 +177,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
     {

diff --git a/src/interfaces/wallet.h b/src/interfaces/wallet.h
@@ -109,14 +109,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;

diff --git a/src/qt/recentrequeststablemodel.cpp b/src/qt/recentrequeststablemodel.cpp
@@ -10,18 +10,20 @@
 #include <qt/walletmodel.h>
 
 #include <clientversion.h>
+#include <key_io.h>
+#include <interfaces/wallet.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);

diff --git a/src/qt/walletmodel.cpp b/src/qt/walletmodel.cpp
@@ -459,25 +459,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;

diff --git a/src/qt/walletmodel.h b/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();

diff --git a/src/wallet/db.h b/src/wallet/db.h
@@ -341,7 +341,7 @@ class BerkeleyBatch
         if (!pdb)
             return nullptr;
         Dbc* pcursor = nullptr;
-        int ret = pdb->cursor(nullptr, &pcursor, 0);
+        int ret = pdb->cursor(activeTxn, &pcursor, 0);
         if (ret != 0)
             return nullptr;
         return pcursor;
@@ -368,6 +368,22 @@ class BerkeleyBatch
         return 0;
     }
 
+    bool ErasePrefix(const char* data, size_t size)
+    {
+        TxnBegin();
+        Dbc* pcursor = GetCursor();
+        Dbt prefix((void*)data, size), prefix_value;
+        int ret = pcursor->get(&prefix, &prefix_value, DB_SET_RANGE);
+        for (int flag = DB_CURRENT; ret == 0; flag = DB_NEXT) {
+            SafeDbt key, value;
+            if ((ret = pcursor->get(key, value, flag)) != 0 || key.get_size() < size || memcmp(key.get_data(), data, size) != 0) break;
+            pcursor->del(0);
+        }
+        pcursor->close();
+        TxnCommit();
+        return ret == 0 || ret == DB_NOTFOUND;
+    }
+
     bool TxnBegin()
     {
         if (!pdb || activeTxn)

diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp
@@ -358,19 +358,47 @@ 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<WalletDatabase>(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) 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(env, [](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(env, [](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),

diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
@@ -727,6 +727,12 @@ bool CWallet::MarkReplaced(const uint256& originalHash, const uint256& newHash)
     return success;
 }
 
+static bool IsUsedDest(const std::map<CTxDestination, CAddressBookData>& address_book, const CTxDestination& dest)
+{
+    auto it = address_book.find(dest);
+    return it != address_book.end() && it->second.IsUsed();
+}
+
 void CWallet::SetSpentKeyState(WalletBatch& batch, const uint256& hash, unsigned int n, bool used, std::set<CTxDestination>& tx_destinations)
 {
     AssertLockHeld(cs_wallet);
@@ -736,12 +742,12 @@ void CWallet::SetSpentKeyState(WalletBatch& batch, const uint256& hash, unsigned
     CTxDestination dst;
     if (ExtractDestination(srctx->tx->vout[n].scriptPubKey, dst)) {
         if (IsMine(dst)) {
-            if (used && !GetDestData(dst, "used", nullptr)) {
-                if (AddDestData(batch, dst, "used", "p")) { // p for "present", opposite of absent (null)
+            if (used != IsUsedDest(m_address_book, dst)) {
+                if (used) {
                     tx_destinations.insert(dst);
                 }
-            } else if (!used && GetDestData(dst, "used", nullptr)) {
-                EraseDestData(batch, dst, "used");
+                m_address_book[dst].SetUsed(used);
+                batch.WriteUsed(dst, used);
             }
         }
     }
@@ -754,6 +760,7 @@ bool CWallet::IsSpentKey(const uint256& hash, unsigned int n) const
     const CWalletTx* srctx = GetWalletTx(hash);
     if (srctx) {
         assert(srctx->tx->vout.size() > n);
+<<<<<<< HEAD
         CTxDestination dest;
         if (!ExtractDestination(srctx->tx->vout[n].scriptPubKey, dest)) {
             return false;
@@ -777,6 +784,41 @@ bool CWallet::IsSpentKey(const uint256& hash, unsigned int n) const
                 if (GetDestData(pkh_dest, "used", nullptr)) {
                     return true;
                 }
+||||||| merged common ancestors
+        LegacyScriptPubKeyMan* spk_man = GetLegacyScriptPubKeyMan();
+        // When descriptor wallets arrive, these additional checks are
+        // likely superfluous and can be optimized out
+        assert(spk_man != nullptr);
+        for (const auto& keyid : GetAffectedKeys(srctx->tx->vout[n].scriptPubKey, *spk_man)) {
+            WitnessV0KeyHash wpkh_dest(keyid);
+            if (GetDestData(wpkh_dest, "used", nullptr)) {
+                return true;
+            }
+            ScriptHash sh_wpkh_dest(GetScriptForDestination(wpkh_dest));
+            if (GetDestData(sh_wpkh_dest, "used", nullptr)) {
+                return true;
+            }
+            PKHash pkh_dest(keyid);
+            if (GetDestData(pkh_dest, "used", nullptr)) {
+                return true;
+=======
+        LegacyScriptPubKeyMan* spk_man = GetLegacyScriptPubKeyMan();
+        // When descriptor wallets arrive, these additional checks are
+        // likely superfluous and can be optimized out
+        assert(spk_man != nullptr);
+        for (const auto& keyid : GetAffectedKeys(srctx->tx->vout[n].scriptPubKey, *spk_man)) {
+            WitnessV0KeyHash wpkh_dest(keyid);
+            if (IsUsedDest(m_address_book, wpkh_dest)) {
+                return true;
+            }
+            ScriptHash sh_wpkh_dest(GetScriptForDestination(wpkh_dest));
+            if (IsUsedDest(m_address_book, sh_wpkh_dest)) {
+                return true;
+            }
+            PKHash pkh_dest(keyid);
+            if (IsUsedDest(m_address_book, pkh_dest)) {
+                return true;
+>>>>>>> refactor: Remove CAddressBookData::destdata
             }
         }
     }
@@ -3178,12 +3220,7 @@ bool CWallet::DelAddressBook(const CTxDestination& address)
         LOCK(cs_wallet);
 
         // Delete destdata tuples associated with address
-        std::string strAddress = EncodeDestination(address);
-        for (const std::pair<const std::string, std::string> &item : m_address_book[address].destdata)
-        {
-            WalletBatch(*database).EraseDestData(strAddress, item.first);
-        }
-        m_address_book.erase(address);
+        WalletBatch(*database).EraseDestData(address);
     }
 
     NotifyAddressBookChanged(this, address, "", IsMine(address) != ISMINE_NO, "", CT_DELETED);
@@ -3626,56 +3663,6 @@ unsigned int CWallet::ComputeTimeSmart(const CWalletTx& wtx) const
     return nTimeSmart;
 }
 
-bool CWallet::AddDestData(WalletBatch& batch, const CTxDestination &dest, const std::string &key, const std::string &value)
-{
-    if (boost::get<CNoDestination>(&dest))
-        return false;
-
-    m_address_book[dest].destdata.insert(std::make_pair(key, value));
-    return batch.WriteDestData(EncodeDestination(dest), key, value);
-}
-
-bool CWallet::EraseDestData(WalletBatch& batch, const CTxDestination &dest, const std::string &key)
-{
-    if (!m_address_book[dest].destdata.erase(key))
-        return false;
-    return batch.EraseDestData(EncodeDestination(dest), key);
-}
-
-void CWallet::LoadDestData(const CTxDestination &dest, const std::string &key, const std::string &value)
-{
-    m_address_book[dest].destdata.insert(std::make_pair(key, value));
-}
-
-bool CWallet::GetDestData(const CTxDestination &dest, const std::string &key, std::string *value) const
-{
-    std::map<CTxDestination, CAddressBookData>::const_iterator i = m_address_book.find(dest);
-    if(i != m_address_book.end())
-    {
-        CAddressBookData::StringMap::const_iterator j = i->second.destdata.find(key);
-        if(j != i->second.destdata.end())
-        {
-            if(value)
-                *value = j->second;
-            return true;
-        }
-    }
-    return false;
-}
-
-std::vector<std::string> CWallet::GetDestValues(const std::string& prefix) const
-{
-    std::vector<std::string> values;
-    for (const auto& address : m_address_book) {
-        for (const auto& data : address.second.destdata) {
-            if (!data.first.compare(0, prefix.size(), prefix)) {
-                values.emplace_back(data.second);
-            }
-        }
-    }
-    return values;
-}
-
 bool CWallet::Verify(interfaces::Chain& chain, const WalletLocation& location, bool salvage_wallet, std::string& error_string, std::vector<std::string>& warnings)
 {
     // Do some checking on wallet path. It should be either a: