wallet: Fix logging of wallet version #32553

1. 
   achow101 commented at 7:07 pm on May 18, 2025: member

   The wallet’s version (in the minversion record) needs to be logged only after we have read it from disk. Otherwise, we always log the lowest version number of 10500 which is incorrect. Furthermore, it doesn’t make sense to log the last client version number if the record didn’t exist. This is a regression caused by #26021.

   The wallet file version logging is moved inside of LoadMinVersion so that it is logged after the record is read. It will also log unconditionally if a version is read so that the version number is reported even when there is an error. The last client logging is split into its own log line that will only occur if a last client record is read. The only situation where we expect no version numbers to be logged is when a wallet is being created.

   A test is added in the second commit to check that the version number is correctly logged on loading. This commit can be cherrypicked to master to verify that it fails there. The last commit adds an additional check that creating a new wallet does not log any version info at all.

2. walletdb: Log the wallet version after it has been read from disk

   Logging the wallet version before anything has been read from disk results
   in the wrong version being logged.
   
   Also split the last client version logging as it may not always be
   present to be logged.

   359ecd3704
3. 
   DrahtBot commented at 7:07 pm on May 18, 2025: contributor

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

   Code Coverage & Benchmarks

   For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32553.

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   ACK	laanwj, janb84, furszy, rkrux

   If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

4. DrahtBot added the label Wallet on May 18, 2025
5. DrahtBot added the label CI failed on May 18, 2025
6. 
   DrahtBot commented at 7:54 pm on May 18, 2025: contributor

   🚧 At least one of the CI tasks failed. Task lint: https://github.com/bitcoin/bitcoin/runs/42437801831 LLM reason (✨ experimental): The CI failure is due to a Python linting error related to an f-string in test/functional/wallet_createwallet.py.

   Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

   • Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

   • A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.

   • An intermittent issue.

   Leave a comment here, if you need help tracking down a confusing failure.

7. test: Check that the correct versions are logged on wallet load 39a483c8e9
8. test: check that creating a wallet does not log version info 4b2cd0b41f
9. achow101 force-pushed on May 18, 2025
10. DrahtBot removed the label CI failed on May 18, 2025
11. 
    furszy commented at 10:38 pm on May 18, 2025: member

    It would also be worth testing this in the compat test — mainly to create a wallet on a previous release and verify that their version and last client are properly reported when opened by the latest release.
12. 
    achow101 commented at 11:36 pm on May 18, 2025: member

    It would also be worth testing this in the compat test — mainly to create a wallet on a previous release and verify that their version and last client are properly reported when opened by the latest release.

    With legacy wallets removed, this wouldn’t really be possible to test as descriptor wallets all are version 169900.

13. laanwj approved
14. 
    laanwj commented at 9:06 am on May 19, 2025: member

    Code review ACK 4b2cd0b41ff4800c8801f2c44883eaec60a035fa Logging the value immediately after reading it, makes it clear the right value is logged.
15. fanquake added the label Needs backport (29.x) on May 19, 2025
16. 
    janb84 commented at 11:02 am on May 19, 2025: contributor

    ACK https://github.com/bitcoin/bitcoin/commit/4b2cd0b41ff4800c8801f2c44883eaec60a035fa

    Fixes regression, improves code readability, adds a test to check if the wallet file version is correctly logged.

    • Code review ✅
    • Build & run tests ✅
    • Ran the added tests against master (fails as expected):

    0wallet_createwallet.py                            | ✖ Failed  | 3 s
    1wallet_createwallet.py --usecli                   | ✖ Failed  | 3 s
    

    • looked into #26021
17. 
    furszy commented at 2:38 pm on May 19, 2025: member

    ACK 4b2cd0b41ff4800c8801f2c44883eaec60a035fa
18. 
    furszy commented at 2:42 pm on May 19, 2025: member

    With legacy wallets removed, this wouldn’t really be possible to test as descriptor wallets all are version 169900.

    I was thinking of checking the version during migration, when we load the wallet into memory at the beginning of the process. We shouldn’t be modifying the version field at that point, but np. Small nit.

19. 
    rkrux commented at 11:09 am on May 20, 2025: contributor

    ACK 4b2cd0b41ff4800c8801f2c44883eaec60a035fa

    I can see the difference in the logs, certainly better to have.

    Master

    02025-05-20T10:31:39Z init message: Loading wallet…
    12025-05-20T10:31:39Z [test] Wallet file version = 10500, last client version = 299900
    22025-05-20T10:31:40Z [test] Descriptors: 13, Descriptor Keys: 1 plaintext, 0 encrypted, 1 total.
    

    PR

    02025-05-20T10:22:15Z init message: Loading wallet…
    12025-05-20T10:22:15Z [test] Last client version = 299900
    22025-05-20T10:22:15Z [test] Wallet file version = 169900
    32025-05-20T10:22:15Z [test] Descriptors: 13, Descriptor Keys: 1 plaintext, 0 encrypted, 1 total.
    

    as descriptor wallets all are version 169900.

    I can see on the first run of the wallet, this^ version is set. https://github.com/bitcoin/bitcoin/blob/af65fd1a333011137dafd3df9a51704fd319feb4/src/wallet/wallet.cpp#L2900-L2905

    So wallet version is supposed to be actually the minimum version that can understand the wallet. Earlier I had assumed that this version is supposed to be the version of the node that the wallet was created in. https://github.com/bitcoin/bitcoin/blob/af65fd1a333011137dafd3df9a51704fd319feb4/src/wallet/wallet.h#L806-L810

    I found this slightly confusing, maybe explicitly mentioning this in the log might reduce the confusion.

    Edit: The RPC help section might be a place to mention this: https://github.com/bitcoin/bitcoin/blob/fad009af49c4d29c6cb264b06d383f6634318a61/src/wallet/rpc/wallet.cpp#L35-L43

20. fanquake merged this on May 20, 2025
21. fanquake closed this on May 20, 2025

    ---

22. fanquake referenced this in commit 25aa15ee7f on May 20, 2025
23. fanquake referenced this in commit e685b4eca2 on May 20, 2025
24. fanquake referenced this in commit 6c0f26d3bd on May 20, 2025
25. fanquake removed the label Needs backport (29.x) on May 20, 2025
26. 
    fanquake commented at 11:31 am on May 20, 2025: member

    Backported to 29.x in #32292.
27. fanquake referenced this in commit 589b56192f on May 22, 2025
28. 
    davidgumberg commented at 1:25 am on May 23, 2025: contributor

    post-merge ACK https://github.com/bitcoin/bitcoin/pull/32553/commits/4b2cd0b41ff4800c8801f2c44883eaec60a035fa

Contributors
achow101 DrahtBot furszy laanwj janb84 rkrux fanquake davidgumberg

Labels
Wallet