Merge #10916: add missing lock to crypter GetKeys()

fe09b0197 add missing lock to crypter GetKeys() (Marko Bencun)
5cb3da04b keystore GetKeys(): return result instead of writing to reference (Marko Bencun)

Pull request description:

  Issue: #10905

  First commit makes GetKeys() return the result instead of writing to a reference to remove some useless lines.

Tree-SHA512: bb51255b5a6cf5488c3d5dee89f539d41f0717f018441d120047f877e0a705a133fb3b7a97d1cf8f73b5d2ed93dd2dbdfcd6f394e40105af2a12e01d397cb402

src/keystore.h

@@ -30,7 +30,7 @@ public:
     //! Check whether a key corresponding to a given address is present in the store.
     virtual bool HaveKey(const CKeyID &address) const =0;
     virtual bool GetKey(const CKeyID &address, CKey& keyOut) const =0;
-    virtual void GetKeys(std::set<CKeyID> &setAddress) const =0;
+    virtual std::set<CKeyID> GetKeys() const =0;
     virtual bool GetPubKey(const CKeyID &address, CPubKey& vchPubKeyOut) const =0;
 
     //! Support for BIP 0013 : see https://github.com/bitcoin/bips/blob/master/bip-0013.mediawiki
@@ -71,18 +71,14 @@ public:
         }
         return result;
     }
-    void GetKeys(std::set<CKeyID> &setAddress) const override
+    std::set<CKeyID> GetKeys() const override
-    {
-        setAddress.clear();
     {
         LOCK(cs_KeyStore);
-            KeyMap::const_iterator mi = mapKeys.begin();
+        std::set<CKeyID> set_address;
-            while (mi != mapKeys.end())
+        for (const auto& mi : mapKeys) {
-            {
+            set_address.insert(mi.first);
-                setAddress.insert((*mi).first);
-                mi++;
-            }
         }
+        return set_address;
     }
     bool GetKey(const CKeyID &address, CKey &keyOut) const override
     {

src/wallet/crypter.h

@@ -162,28 +162,26 @@ public:
     {
         {
             LOCK(cs_KeyStore);
-            if (!IsCrypted())
+            if (!IsCrypted()) {
                 return CBasicKeyStore::HaveKey(address);
+            }
             return mapCryptedKeys.count(address) > 0;
         }
         return false;
     }
     bool GetKey(const CKeyID &address, CKey& keyOut) const override;
     bool GetPubKey(const CKeyID &address, CPubKey& vchPubKeyOut) const override;
-    void GetKeys(std::set<CKeyID> &setAddress) const override
+    std::set<CKeyID> GetKeys() const override
-    {
-        if (!IsCrypted())
     {
-            CBasicKeyStore::GetKeys(setAddress);
+        LOCK(cs_KeyStore);
-            return;
+        if (!IsCrypted()) {
+            return CBasicKeyStore::GetKeys();
         }
-        setAddress.clear();
+        std::set<CKeyID> set_address;
-        CryptedKeyMap::const_iterator mi = mapCryptedKeys.begin();
+        for (const auto& mi : mapCryptedKeys) {
-        while (mi != mapCryptedKeys.end())
+            set_address.insert(mi.first);
-        {
-            setAddress.insert((*mi).first);
-            mi++;
         }
+        return set_address;
     }
 
     /**

src/wallet/wallet.cpp

@@ -3600,13 +3600,10 @@ void CWallet::GetKeyBirthTimes(std::map<CTxDestination, int64_t> &mapKeyBirth) c
     // map in which we'll infer heights of other keys
     CBlockIndex *pindexMax = chainActive[std::max(0, chainActive.Height() - 144)]; // the tip can be reorganized; use a 144-block safety margin
     std::map<CKeyID, CBlockIndex*> mapKeyFirstBlock;
-    std::set<CKeyID> setKeys;
+    for (const CKeyID &keyid : GetKeys()) {
-    GetKeys(setKeys);
-    for (const CKeyID &keyid : setKeys) {
         if (mapKeyBirth.count(keyid) == 0)
             mapKeyFirstBlock[keyid] = pindexMax;
     }
-    setKeys.clear();
 
     // if there are no such keys, we're done
     if (mapKeyFirstBlock.empty())