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

pull achow101 wants to merge 3 commits into bitcoin:master from achow101:rm-wallet-nfileversion changing 4 files +11 −38
  1. achow101 commented at 5:14 AM on March 13, 2019: member

    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.

  2. fanquake added the label Wallet on Mar 13, 2019
  3. in src/wallet/db.cpp:590 in 96a46f5bcd outdated 
     586 | @@ -587,7 +587,7 @@ BerkeleyBatch::BerkeleyBatch(BerkeleyDatabase& database, const char* pszMode, bo
 587 |              if (fCreate && !Exists(std::string("version"))) {
 588 |                  bool fTmp = fReadOnly;
 589 |                  fReadOnly = false;
 590 | -                WriteVersion(CLIENT_VERSION);
 591 | +                Write(std::string("version"), CLIENT_VERSION);
    ryanofsky commented at 3:42 PM on March 13, 2019:

    Maybe something for a separate PR, but can we just drop this write entirely? It seems redundant given the write already in WalletBatch::LoadWallet, and better not to write the same field in two different places.

    achow101 commented at 4:42 PM on April 16, 2019:

    I'm not sure that this is redundant. It's making sure that the version field exists, whereas WalletBatch::LoadWallet doesn't necessarily do that.

    But we can remove this in a separate PR if it is redundant.

  4. ryanofsky commented at 4:21 PM on March 13, 2019: member

    Just to clarify, is the only user-visible change here replacing:

    nFileVersion = [version number of the newest wallet software that ever opened the wallet in the past]

    with

    Wallet File Version = [minimum wallet software version required to open the wallet file]

    ?

  5. achow101 commented at 7:29 PM on March 13, 2019: member

    Just to clarify, is the only user-visible change here replacing:

    Yes.

  6. in src/wallet/walletdb.cpp:509 in 96a46f5bcd outdated 
     504 | @@ -512,7 +505,12 @@ DBErrors WalletBatch::LoadWallet(CWallet* pwallet)
 505 |      if (result != DBErrors::LOAD_OK)
 506 |          return result;
 507 |  
 508 | -    pwallet->WalletLogPrintf("nFileVersion = %d\n", wss.nFileVersion);
 509 | +    // Last client version to open this wallet, was previously the file version number
 510 | +    int last_client = CLIENT_VERSION;
    promag commented at 1:01 AM on March 14, 2019:

    nit, last_client_version?

    achow101 commented at 6:00 PM on July 9, 2019:

    I think I'll leave it as is.

  7. jjjerrmmmy approved
  8. DrahtBot commented at 12:30 PM on March 31, 2019: member

    <!--e57a25ab6845829454e8d69fc972939a-->

    The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #15709 (Do not add setting as unknown by Bushstar)

    If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

  9. laanwj commented at 11:47 AM on April 10, 2019: member

    utACK 96a46f5bcd5433c3a4c5900ecc8be547e48f5f74

  10. in src/wallet/walletdb.cpp:383 in 96a46f5bcd outdated 
     374 | @@ -376,12 +375,6 @@ ReadKeyValue(CWallet* pwallet, CDataStream& ssKey, CDataStream& ssValue,
 375 |  
 376 |              pwallet->LoadKeyPool(nIndex, keypool);
 377 |          }
 378 | -        else if (strType == "version")
 379 | -        {
 380 | -            ssValue >> wss.nFileVersion;
 381 | -            if (wss.nFileVersion == 10300)
 382 | -                wss.nFileVersion = 300;
    promag commented at 2:56 PM on June 28, 2019:

    Can you explain why this can be removed?

    achow101 commented at 1:18 AM on June 29, 2019:

    nFileVersion is being removed entirely, and all this version record did was fill it. At no point was nFileVersion being checked for anything less than 40000 so this if block is meaningless.

    jb55 commented at 3:21 PM on July 22, 2019:

    ACK @achow101's comment, I believe this is correct.

  11. laanwj commented at 6:57 PM on July 11, 2019: member

    @meshcollider can you please take a look here

  12. laanwj requested review from meshcollider on Jul 11, 2019
  13. in src/wallet/walletdb.cpp:530 in 96a46f5bcd outdated 
     526 | @@ -527,7 +527,7 @@ DBErrors WalletBatch::LoadWallet(CWallet* pwallet)
 527 |          return DBErrors::NEED_REWRITE;
 528 |  
 529 |      if (last_client < CLIENT_VERSION) // Update
 530 | -        WriteVersion(CLIENT_VERSION);
 531 | +        m_batch.Write(std::string("version"), CLIENT_VERSION);
    jb55 commented at 3:17 PM on July 22, 2019:

    I'm ok with open-coding this here since there is only one use of it, but for semantic clarity nit we could have just moved it into a static method named {Read,Write}LatestClientVersion or something.

    achow101 commented at 4:59 PM on July 22, 2019:

    I prefer to leave it as is since these are the only uses of it. I had originally planned on removing these reads and writes entirely, but I was not certain that doing so wouldn't break backwards compatibility so I left them in.

  14. in src/wallet/walletdb.cpp:415 in 521dd5d34d outdated 
     411 | @@ -419,7 +412,7 @@ ReadKeyValue(CWallet* pwallet, CDataStream& ssKey, CDataStream& ssValue,
 412 |                  return false;
 413 |              }
 414 |          } else if (strType != "bestblock" && strType != "bestblock_nomerkle" &&
 415 | -                strType != "minversion" && strType != "acentry") {
 416 | +                strType != "minversion" && strType != "acentry" && strType != "version") {
    jb55 commented at 3:30 PM on July 22, 2019:

    Leaving this comment for other reviewers when trying to understand it. The wallet logs any unknown records, since we're not handling the version record specifically anymore (it was a no-op), we don't want to log version as an unknown record. It is still read-in during LoadWallet.

  15. in src/wallet/walletdb.cpp:516 in 835e112b2b outdated 
     511 | @@ -512,7 +512,8 @@ DBErrors WalletBatch::LoadWallet(CWallet* pwallet)
 512 |      if (result != DBErrors::LOAD_OK)
 513 |          return result;
 514 |  
 515 | -    pwallet->WalletLogPrintf("nFileVersion = %d\n", wss.nFileVersion);
 516 | +    int wallet_version = pwallet->GetVersion();
 517 | +    pwallet->WalletLogPrintf("Wallet File Version = %d\n", wallet_version > 0 ? wallet_version : wss.nFileVersion);
    jb55 commented at 3:34 PM on July 22, 2019:

    In the subsequent commits, nFileVersion is removed from CWalletScanState, so reviewing this commit was confusing. perhaps this commit should come after the commit that removes nFileVersion from CWalletScanState

    achow101 commented at 5:03 PM on July 22, 2019:

    Done

  16. jb55 changes_requested
  17. jb55 commented at 4:43 PM on July 22, 2019: member

    Concept ACK, I reviewed the changes in detail and confirmed they make sense logically. I have a few comments before I Approach ACK. Let me know what you think.

  18. Remove nFileVersion from CWalletScanState
    nFileVersion is not the actual file version and is not used except
in one place. So it is removed from CWalletScanState and changed so
that it is just read at the place it is needed. Furthermore, the
"version" record now only indicates the version of the highest
versioned client that has opened a wallet file so the variable
name is changed accordingly
    c88e87c3b2
  19. Log the actual wallet file version
    The actual wallet file version is the minversion record, not the
version record.
    b3d4f6c961
  20. Remove ReadVersion and WriteVersion
    The "version" record that these functions read and write are not
used anywhere in the code except for one place. There is no reason
to expose these functions publicly. Furthermore, this avoids potential
confusion as developers may mistake these functions for actually
reading and writing the wallet version when they do not.
    35e60e790f
  21. achow101 force-pushed on Jul 22, 2019
  22. ryanofsky approved
  23. ryanofsky commented at 6:53 PM on July 22, 2019: member

    utACK 35e60e790f2cd602d1bdd0be835d27f0ba37efa9. 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.

  24. jb55 commented at 4:54 AM on July 23, 2019: member

    ACK 35e60e7, I compiled locally as a quick sanity check.

  25. meshcollider approved
  26. meshcollider commented at 10:44 AM on July 27, 2019: contributor

    Looks good, thanks! utACK 35e60e790f2cd602d1bdd0be835d27f0ba37efa9

  27. meshcollider merged this on Jul 27, 2019
  28. meshcollider closed this on Jul 27, 2019
  29. meshcollider referenced this in commit febf3a856b on Jul 27, 2019
  30. konez2k referenced this in commit dc8c17b962 on Jul 27, 2019
  31. kittywhiskers referenced this in commit 285c64fd19 on Nov 6, 2021
  32. kittywhiskers referenced this in commit 6f83f5a17a on Nov 30, 2021
  33. kittywhiskers referenced this in commit 843ca85239 on Nov 30, 2021
  34. kittywhiskers referenced this in commit 3a797e7c70 on Nov 30, 2021
  35. kittywhiskers referenced this in commit 89950a97a1 on Nov 30, 2021
  36. kittywhiskers referenced this in commit 508e507097 on Dec 3, 2021
  37. kittywhiskers referenced this in commit 35914aba5b on Dec 4, 2021
  38. kittywhiskers referenced this in commit e386da751b on Dec 8, 2021
  39. kittywhiskers referenced this in commit 0922bab856 on Dec 8, 2021
  40. kittywhiskers referenced this in commit aba943a9d6 on Dec 11, 2021
  41. kittywhiskers referenced this in commit c2d25d2b52 on Dec 12, 2021
  42. kittywhiskers referenced this in commit 41a5d5e2cc on Dec 12, 2021
  43. kittywhiskers referenced this in commit e1a40e554c on Dec 12, 2021
  44. UdjinM6 referenced this in commit 6af131f825 on Dec 12, 2021
  45. MarcoFalke locked this on Dec 16, 2021
Contributors
achow101ryanofskypromagjjjerrmmmyDrahtBotlaanwjjb55meshcollider
Labels
Wallet
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-04-19 00:14 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me