[Wallet] Crypter code cleanup, updates and styling corrections

[furszy]

Grouped some updates to the wallet crypter sources.

• Cleaned Sapling crypter code.
• Backported CKeystore/CCrypter: move relevant implementation out of the header bitcoin/bitcoin#11272.
• Return result in GetKeys() instead of writing to reference (add missing lock to crypter GetKeys() bitcoin/bitcoin#10916).

[random-zebra]

ACK 33371c9

[Fuzzbawls]

The last commit can be expanded to apply our code styling (clang-format) rules to the two files in their entirety:

Index: src/crypter.cpp
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/src/crypter.cpp b/src/crypter.cpp
--- a/src/crypter.cpp	(revision c590283d3f10160b5a4264012db2faabfffeb896)
+++ b/src/crypter.cpp	(date 1643010650066)
@@ -11,14 +11,14 @@
 #include "script/standard.h"
 #include "uint256.h"
 
-int CCrypter::BytesToKeySHA512AES(const std::vector<unsigned char>& chSalt, const SecureString& strKeyData, int count, unsigned char *key,unsigned char *iv) const
+int CCrypter::BytesToKeySHA512AES(const std::vector<unsigned char>& chSalt, const SecureString& strKeyData, int count, unsigned char* key, unsigned char* iv) const
 {
     // This mimics the behavior of openssl's EVP_BytesToKey with an aes256cbc
     // cipher and sha512 message digest. Because sha512's output size (64b) is
     // greater than the aes256 block size (16b) + aes256 key size (32b),
     // there's no need to process more than once (D_0).
 
-    if(!count || !key || !iv)
+    if (!count || !key || !iv)
         return 0;
 
     unsigned char buf[CSHA512::OUTPUT_SIZE];
@@ -28,7 +28,7 @@
     di.Write(chSalt.data(), chSalt.size());
     di.Finalize(buf);
 
-    for(int i = 0; i != count - 1; i++)
+    for (int i = 0; i != count - 1; i++)
         di.Reset().Write(buf, sizeof(buf)).Finalize(buf);
 
     memcpy(key, buf, WALLET_CRYPTO_KEY_SIZE);
@@ -79,7 +79,7 @@
 
     AES256CBCEncrypt enc(vchKey.data(), vchIV.data(), true);
     size_t nLen = enc.Encrypt(&vchPlaintext[0], vchPlaintext.size(), vchCiphertext.data());
-    if(nLen < vchPlaintext.size())
+    if (nLen < vchPlaintext.size())
         return false;
     vchCiphertext.resize(nLen);
     return true;
@@ -95,7 +95,7 @@
     vchPlaintext.resize(nLen);
     AES256CBCDecrypt dec(vchKey.data(), vchIV.data(), true);
     nLen = dec.Decrypt(vchCiphertext.data(), vchCiphertext.size(), &vchPlaintext[0]);
-    if(nLen == 0)
+    if (nLen == 0)
         return false;
     vchPlaintext.resize(nLen);
     return true;
@@ -124,7 +124,7 @@
 bool CCryptoKeyStore::DecryptKey(const CKeyingMaterial& vMasterKey, const std::vector<unsigned char>& vchCryptedSecret, const CPubKey& vchPubKey, CKey& key)
 {
     CKeyingMaterial vchSecret;
-    if(!DecryptSecret(vMasterKey, vchCryptedSecret, vchPubKey.GetHash(), vchSecret))
+    if (!DecryptSecret(vMasterKey, vchCryptedSecret, vchPubKey.GetHash(), vchSecret))
         return false;
 
     if (vchSecret.size() != 32)
@@ -185,7 +185,7 @@
     return mapCryptedKeys.count(address) > 0;
 }
 
-bool CCryptoKeyStore::AddCryptedKey(const CPubKey &vchPubKey, const std::vector<unsigned char> &vchCryptedSecret)
+bool CCryptoKeyStore::AddCryptedKey(const CPubKey& vchPubKey, const std::vector<unsigned char>& vchCryptedSecret)
 {
     LOCK(cs_KeyStore);
     if (!SetCrypted()) {
@@ -205,8 +205,8 @@
 
     CryptedKeyMap::const_iterator mi = mapCryptedKeys.find(address);
     if (mi != mapCryptedKeys.end()) {
-        const CPubKey &vchPubKey = (*mi).second.first;
-        const std::vector<unsigned char> &vchCryptedSecret = (*mi).second.second;
+        const CPubKey& vchPubKey = (*mi).second.first;
+        const std::vector<unsigned char>& vchCryptedSecret = (*mi).second.second;
         return DecryptKey(vMasterKey, vchCryptedSecret, vchPubKey, keyOut);
     }
     return false;
@@ -249,7 +249,7 @@
 
     fUseCrypto = true;
     for (KeyMap::value_type& mKey : mapKeys) {
-        const CKey &key = mKey.second;
+        const CKey& key = mKey.second;
         CPubKey vchPubKey = key.GetPubKey();
         CKeyingMaterial vchSecret(key.begin(), key.end());
         std::vector<unsigned char> vchCryptedSecret;
Index: src/keystore.cpp
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/src/keystore.cpp b/src/keystore.cpp
--- a/src/keystore.cpp	(revision c590283d3f10160b5a4264012db2faabfffeb896)
+++ b/src/keystore.cpp	(date 1643011133767)
@@ -16,7 +16,7 @@
     return AddKeyPubKey(key, key.GetPubKey());
 }
 
-bool CBasicKeyStore::GetPubKey(const CKeyID &address, CPubKey &vchPubKeyOut) const
+bool CBasicKeyStore::GetPubKey(const CKeyID& address, CPubKey& vchPubKeyOut) const
 {
     CKey key;
     if (!GetKey(address, key)) {
@@ -65,7 +65,7 @@
     return false;
 }
 
-static bool ExtractPubKey(const CScript &dest, CPubKey& pubKeyOut)
+static bool ExtractPubKey(const CScript& dest, CPubKey& pubKeyOut)
 {
     //TODO: Use Solver to extract this?
     CScript::const_iterator pc = dest.begin();
@@ -146,7 +146,7 @@
 
 //! Sapling
 bool CBasicKeyStore::AddSaplingSpendingKey(
-    const libzcash::SaplingExtendedSpendingKey &sk)
+    const libzcash::SaplingExtendedSpendingKey& sk)
 {
     LOCK(cs_KeyStore);
     auto extfvk = sk.ToXFVK();
@@ -162,7 +162,7 @@
 }
 
 bool CBasicKeyStore::AddSaplingFullViewingKey(
-    const libzcash::SaplingExtendedFullViewingKey &extfvk)
+    const libzcash::SaplingExtendedFullViewingKey& extfvk)
 {
     LOCK(cs_KeyStore);
     auto ivk = extfvk.fvk.in_viewing_key();
@@ -175,8 +175,8 @@
 // If we add an address that is already in the map, the map will
 // remain unchanged as each address only has one ivk.
 bool CBasicKeyStore::AddSaplingIncomingViewingKey(
-        const libzcash::SaplingIncomingViewingKey &ivk,
-        const libzcash::SaplingPaymentAddress &addr)
+    const libzcash::SaplingIncomingViewingKey& ivk,
+    const libzcash::SaplingPaymentAddress& addr)
 {
     LOCK(cs_KeyStore);
 
@@ -186,22 +186,22 @@
     return true;
 }
 
-bool CBasicKeyStore::HaveSaplingSpendingKey(const libzcash::SaplingExtendedFullViewingKey &extfvk) const
+bool CBasicKeyStore::HaveSaplingSpendingKey(const libzcash::SaplingExtendedFullViewingKey& extfvk) const
 {
     return WITH_LOCK(cs_KeyStore, return mapSaplingSpendingKeys.count(extfvk) > 0);
 }
 
-bool CBasicKeyStore::HaveSaplingFullViewingKey(const libzcash::SaplingIncomingViewingKey &ivk) const
+bool CBasicKeyStore::HaveSaplingFullViewingKey(const libzcash::SaplingIncomingViewingKey& ivk) const
 {
     return WITH_LOCK(cs_KeyStore, return mapSaplingFullViewingKeys.count(ivk) > 0);
 }
 
-bool CBasicKeyStore::HaveSaplingIncomingViewingKey(const libzcash::SaplingPaymentAddress &addr) const
+bool CBasicKeyStore::HaveSaplingIncomingViewingKey(const libzcash::SaplingPaymentAddress& addr) const
 {
     return WITH_LOCK(cs_KeyStore, return mapSaplingIncomingViewingKeys.count(addr) > 0);
 }
 
-bool CBasicKeyStore::GetSaplingSpendingKey(const libzcash::SaplingExtendedFullViewingKey &extfvk, libzcash::SaplingExtendedSpendingKey &skOut) const
+bool CBasicKeyStore::GetSaplingSpendingKey(const libzcash::SaplingExtendedFullViewingKey& extfvk, libzcash::SaplingExtendedSpendingKey& skOut) const
 {
     LOCK(cs_KeyStore);
     SaplingSpendingKeyMap::const_iterator mi = mapSaplingSpendingKeys.find(extfvk);
@@ -213,8 +213,8 @@
 }
 
 bool CBasicKeyStore::GetSaplingFullViewingKey(
-    const libzcash::SaplingIncomingViewingKey &ivk,
-    libzcash::SaplingExtendedFullViewingKey &extfvkOut) const
+    const libzcash::SaplingIncomingViewingKey& ivk,
+    libzcash::SaplingExtendedFullViewingKey& extfvkOut) const
 {
     LOCK(cs_KeyStore);
     SaplingFullViewingKeyMap::const_iterator mi = mapSaplingFullViewingKeys.find(ivk);
@@ -225,8 +225,8 @@
     return false;
 }
 
-bool CBasicKeyStore::GetSaplingIncomingViewingKey(const libzcash::SaplingPaymentAddress &addr,
-                                   libzcash::SaplingIncomingViewingKey &ivkOut) const
+bool CBasicKeyStore::GetSaplingIncomingViewingKey(const libzcash::SaplingPaymentAddress& addr,
+    libzcash::SaplingIncomingViewingKey& ivkOut) const
 {
     LOCK(cs_KeyStore);
     SaplingIncomingViewingKeyMap::const_iterator mi = mapSaplingIncomingViewingKeys.find(addr);
@@ -237,25 +237,25 @@
     return false;
 }
 
-bool CBasicKeyStore::GetSaplingExtendedSpendingKey(const libzcash::SaplingPaymentAddress &addr,
-                                    libzcash::SaplingExtendedSpendingKey &extskOut) const {
+bool CBasicKeyStore::GetSaplingExtendedSpendingKey(const libzcash::SaplingPaymentAddress& addr,
+    libzcash::SaplingExtendedSpendingKey& extskOut) const
+{
     libzcash::SaplingIncomingViewingKey ivk;
     libzcash::SaplingExtendedFullViewingKey extfvk;
 
     LOCK(cs_KeyStore);
     return GetSaplingIncomingViewingKey(addr, ivk) &&
-            GetSaplingFullViewingKey(ivk, extfvk) &&
-            GetSaplingSpendingKey(extfvk, extskOut);
+           GetSaplingFullViewingKey(ivk, extfvk) &&
+           GetSaplingSpendingKey(extfvk, extskOut);
 }
 
-void CBasicKeyStore::GetSaplingPaymentAddresses(std::set<libzcash::SaplingPaymentAddress> &setAddress) const
+void CBasicKeyStore::GetSaplingPaymentAddresses(std::set<libzcash::SaplingPaymentAddress>& setAddress) const
 {
     setAddress.clear();
     {
         LOCK(cs_KeyStore);
         auto mi = mapSaplingIncomingViewingKeys.begin();
-        while (mi != mapSaplingIncomingViewingKeys.end())
-        {
+        while (mi != mapSaplingIncomingViewingKeys.end()) {
             setAddress.insert((*mi).first);
             mi++;
         }

[furszy]

Done, code styling patch applied. Plus rebased it on master for conflicts with one of the merged circ dependencies removal PRs.

[random-zebra]

utACK 55293df

[Fuzzbawls]

ACK 55293df