Merge #15588: Log the actual wallet file version and no longer publicly expose the "version" record

35e60e7 Remove ReadVersion and WriteVersion (Andrew Chow)
b3d4f6c Log the actual wallet file version (Andrew Chow)
c88e87c Remove nFileVersion from CWalletScanState (Andrew Chow)

Pull request description:

  The wallet file version is stored in the "minversion" record, not the "version" record. However "version" is no longer used anywhere except to record the highest versioned client which has opened a wallet file (which is currently only used to check whether this was most recently opened by a 0.4.0 or 0.5.0rc1 client which had a broken wallet encryption implementation). Furthermore, "version" was logged to the debug.log which is confusing because it is not the actual wallet file version.

  This PR changes it so that this confusion largely no longer exists. The wallet file version logging is changed to use "minversion" and reading and writing the "version" record is no longer publicly exposed to prevent potential confusion about whether the actual file version is being read or written. Lastly, in the one place it is actually used, the variable name is changed from nFileVersion to last_client to better reflect what that record actually represents.

ACKs for top commit:
  jb55:
    ACK 35e60e7, I compiled locally as a quick sanity check.
  ryanofsky:
    utACK 35e60e7. This code still pretty confusing, but a little simpler now. And the previous log statement was really misleading and useless compared to the new one here.
  meshcollider:
    Looks good, thanks! utACK 35e60e7

Tree-SHA512: f782b2f215d07fbc9b806322bda8085445b81c02b65ca674a8c6a3e1de505a0abd050669afe0ead4778816144a1c18462e13930071cedb7227a058aeb39493f7

Author: meshcollider

src/wallet/db.cpp

@@ -585,7 +585,7 @@ BerkeleyBatch::BerkeleyBatch(BerkeleyDatabase& database, const char* pszMode, bo
             if (fCreate && !Exists(std::string("version"))) {
                 bool fTmp = fReadOnly;
                 fReadOnly = false;
-                WriteVersion(CLIENT_VERSION);
+                Write(std::string("version"), CLIENT_VERSION);
                 fReadOnly = fTmp;
             }
         }

src/wallet/db.h

@@ -399,17 +399,6 @@ class BerkeleyBatch
         return (ret == 0);
     }
 
-    bool ReadVersion(int& nVersion)
-    {
-        nVersion = 0;
-        return Read(std::string("version"), nVersion);
-    }
-
-    bool WriteVersion(int nVersion)
-    {
-        return Write(std::string("version"), nVersion);
-    }
-
     bool static Rewrite(BerkeleyDatabase& database, const char* pszSkip = nullptr);
 };
 

src/wallet/walletdb.cpp

@@ -165,7 +165,6 @@ class CWalletScanState {
     unsigned int m_unknown_records{0};
     bool fIsEncrypted{false};
     bool fAnyUnordered{false};
-    int nFileVersion{0};
     std::vector<uint256> vWalletUpgrade;
 
     CWalletScanState() {
@@ -376,12 +375,6 @@ ReadKeyValue(CWallet* pwallet, CDataStream& ssKey, CDataStream& ssValue,
 
             pwallet->LoadKeyPool(nIndex, keypool);
         }
-        else if (strType == "version")
-        {
-            ssValue >> wss.nFileVersion;
-            if (wss.nFileVersion == 10300)
-                wss.nFileVersion = 300;
-        }
         else if (strType == "cscript")
         {
             uint160 hash;
@@ -419,7 +412,7 @@ ReadKeyValue(CWallet* pwallet, CDataStream& ssKey, CDataStream& ssValue,
                 return false;
             }
         } else if (strType != "bestblock" && strType != "bestblock_nomerkle" &&
-                strType != "minversion" && strType != "acentry") {
+                strType != "minversion" && strType != "acentry" && strType != "version") {
             wss.m_unknown_records++;
         }
     } catch (const std::exception& e) {
@@ -519,7 +512,12 @@ DBErrors WalletBatch::LoadWallet(CWallet* pwallet)
     if (result != DBErrors::LOAD_OK)
         return result;
 
-    pwallet->WalletLogPrintf("nFileVersion = %d\n", wss.nFileVersion);
+    // Last client version to open this wallet, was previously the file version number
+    int last_client = CLIENT_VERSION;
+    m_batch.Read(std::string("version"), last_client);
+
+    int wallet_version = pwallet->GetVersion();
+    pwallet->WalletLogPrintf("Wallet File Version = %d\n", wallet_version > 0 ? wallet_version : last_client);
 
     pwallet->WalletLogPrintf("Keys: %u plaintext, %u encrypted, %u w/ metadata, %u total. Unknown wallet records: %u\n",
            wss.nKeys, wss.nCKeys, wss.nKeyMeta, wss.nKeys + wss.nCKeys, wss.m_unknown_records);
@@ -532,11 +530,11 @@ DBErrors WalletBatch::LoadWallet(CWallet* pwallet)
         WriteTx(pwallet->mapWallet.at(hash));
 
     // Rewrite encrypted wallets of versions 0.4.0 and 0.5.0rc:
-    if (wss.fIsEncrypted && (wss.nFileVersion == 40000 || wss.nFileVersion == 50000))
+    if (wss.fIsEncrypted && (last_client == 40000 || last_client == 50000))
         return DBErrors::NEED_REWRITE;
 
-    if (wss.nFileVersion < CLIENT_VERSION) // Update
-        WriteVersion(CLIENT_VERSION);
+    if (last_client < CLIENT_VERSION) // Update
+        m_batch.Write(std::string("version"), CLIENT_VERSION);
 
     if (wss.fAnyUnordered)
         result = pwallet->ReorderTransactions();
@@ -779,13 +777,3 @@ bool WalletBatch::TxnAbort()
 {
     return m_batch.TxnAbort();
 }
-
-bool WalletBatch::ReadVersion(int& nVersion)
-{
-    return m_batch.ReadVersion(nVersion);
-}
-
-bool WalletBatch::WriteVersion(int nVersion)
-{
-    return m_batch.WriteVersion(nVersion);
-}

src/wallet/walletdb.h

@@ -249,10 +249,6 @@ class WalletBatch
     bool TxnCommit();
     //! Abort current transaction
     bool TxnAbort();
-    //! Read wallet version
-    bool ReadVersion(int& nVersion);
-    //! Write wallet version
-    bool WriteVersion(int nVersion);
 private:
     BerkeleyBatch m_batch;
     WalletDatabase& m_database;