wallet: upgradewallet fixes and additional tests #18836

achow101 commented at 10:19 PM on April 30, 2020: member

This PR cleans up the wallet upgrade mechanism a bit, fixes some probably bugs, and adds more test cases.

The nWalletMaxVersion member variable has been removed as it made CanSupportFeature unintuitive and was causing a couple of bugs. The reason this was introduced originally was to allow a wallet upgrade to only occur when the new feature is first used. While this makes sense for the old -upgradewallet option, for an RPC, this does not quite make sense. It's more intuitive for an upgrade to occur if possible if the upgradewallet RPC is used as that's an explicit request to upgrade a particular wallet to a newer version. nWalletMaxVersion was only relevant for upgrades to FEATURE_WALLETCRYPT and FEATURE_COMPRPUBKEY both of which are incredibly old features. So for such wallets, the behavior of upgradewallet will be that the feature is enabled immediately without the wallet needing to be encrypted at that time (note that FEATURE_WALLETCRYPT indicates support for encryption, not that the wallet is encrypted) or for a new key to be generated.

CanSupportFeature would previously indicate whether we could upgrade to nWalletMaxVersion not just whether the current wallet version supported a feature. While this property was being used to determine whether we should upgrade to HD and HD chain split, it was also causing a few bugs. Determining whether we should upgrade to HD or HD chain split is resolved by passing into ScriptPubKeyMan::Upgrade the version we are upgrading to and checking against that. By removing nWalletMaxVersion we also fix a bug where you could upgrade to HD chain split without the pre-split keypool.

nWalletMaxVersion was also the version that was being reported by getwalletinfo which meant that the version reported was not always consistent across restarts as it depended on whether upgradewallet was used. Additionally to make the wallet versions consistent with actually supported versions, instead of just setting the wallet version to whatever is given to upgradewallet, we normalize the version number to the closest supported version number. For example, if given 150000, we would store and report 139900.

Another bug where CHDChain was not being upgraded to the version supporting HD chain split is also fixed by this PR.

Lastly several more tests have been added. Some refactoring to the test was made to make these tests easier. These tests check specific upgrading scenarios, such as from non-HD (version 60000) to HD to pre-split keypool. Although not specifically related to upgradewallet, UpgradeKeyMetadata is now being tested too.

Part of the new tests is checking that the wallet files are identical before and after failed upgrades. To facilitate this, a utility function sha256sum_file has been added. Another part of the tests is to examine the wallet file itself to ensure that the records in the wallet.dat file have been correctly modified. So a new bdb.py module has been added to deserialize the BDB db of the wallet.dat file. This format isn't explicitly documented anywhere, but the code and comments in BDB's source code in file dbinc/db_page.h describe it. This module just dumps all of the fields into a dict.

DrahtBot added the label Tests on Apr 30, 2020

DrahtBot added the label Wallet on Apr 30, 2020

achow101 renamed this:
~~Upgradewallet tests~~
wallet: tests: upgradewallet fixes and additional tests
on Apr 30, 2020

MarcoFalke removed the label Tests on Apr 30, 2020

MarcoFalke renamed this:
~~wallet: tests: upgradewallet fixes and additional tests~~
wallet: upgradewallet fixes and additional tests
on Apr 30, 2020

in test/functional/test_framework/bdb.py:6 in 78d1c21533 outdated

   0 | @@ -0,0 +1,152 @@
   1 | +#!/usr/bin/env python3
   2 | +# Copyright (c) 2020 The Bitcoin Core developers
   3 | +# Distributed under the MIT software license, see the accompanying
   4 | +# file COPYING or http://www.opensource.org/licenses/mit-license.php.
   5 | +"""
   6 | +Utilities for working directly with the wallet's BDB databse file

brakmic commented at 10:55 PM on April 30, 2020:

nit: typo

Utilities for working directly with the wallet's BDB database file

achow101 commented at 4:12 PM on September 1, 2020:

Fixed

brakmic changes_requested

brakmic commented at 10:57 PM on April 30, 2020: contributor

Code reviewed, only a small typo found.

brakmic commented at 11:00 PM on April 30, 2020: contributor

ACK 0c961088a608b92851642ef9406f5362a63e9b15

Built, run and tested on macOS Catalina 10.15.4

./test/functional/wallet_upgradewallet.py  (3m 25s 785ms)  
2020-04-30T22:45:54.168000Z TestFramework (INFO): Initializing test directory /var/folders/7q/4ffytzk562dd2ky4bfg9_w7h0000gn/T/bitcoin_func_test_aq11myt5
2020-04-30T22:45:57.594000Z TestFramework (INFO): Test upgradewallet RPC...
2020-04-30T22:46:05.290000Z TestFramework (INFO): Intermediary versions don't effect anything
2020-04-30T22:46:05.995000Z TestFramework (INFO): Wallets cannot be downgraded
2020-04-30T22:46:06.480000Z TestFramework (INFO): Can upgrade to HD
2020-04-30T22:46:07.020000Z TestFramework (INFO): Cannot upgrade to HD Split, needs Pre Split Keypool
2020-04-30T22:46:07.027000Z TestFramework (INFO): Upgrade HD to HD chain split
2020-04-30T22:46:07.371000Z TestFramework (INFO): Upgrade non-HD to HD chain split
2020-04-30T22:46:08.167000Z TestFramework (INFO): KeyMetadata should upgrade when loading into master
2020-04-30T22:46:08.422000Z TestFramework (INFO): Upgrading to NO_DEFAULT_KEY should not remove the defaultkey
2020-04-30T22:46:08.813000Z TestFramework (INFO): Stopping nodes
2020-04-30T22:46:09.184000Z TestFramework (INFO): Cleaning up /var/folders/7q/4ffytzk562dd2ky4bfg9_w7h0000gn/T/bitcoin_func_test_aq11myt5 on exit
2020-04-30T22:46:09.184000Z TestFramework (INFO): Tests successful

bitcoin deleted a comment on Apr 30, 2020

DrahtBot commented at 1:57 AM on May 1, 2020: contributor

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#20362 (test: Implicitly sync after generate* to preempt races and intermittent test failures by MarcoFalke)
#20275 (wallet: List SQLite wallets in non-SQLite builds by ryanofsky)

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.

DrahtBot cross-referenced this on May 1, 2020 from issue test: Strip down previous releases boilerplate by MarcoFalke

DrahtBot cross-referenced this on May 1, 2020 from issue wallet: Remove -upgradewallet from dummywallet by MarcoFalke

DrahtBot cross-referenced this on May 1, 2020 from issue wallet: Avoid translating RPC errors by MarcoFalke

DrahtBot added the label Needs rebase on May 1, 2020

achow101 force-pushed on May 1, 2020

DrahtBot removed the label Needs rebase on May 1, 2020

achow101 force-pushed on May 2, 2020

in src/wallet/wallet.cpp:4021 in c3fdecccb8 outdated

4016 | @@ -4037,33 +4017,32 @@ const CAddressBookData* CWallet::FindAddressBookEntry(const CTxDestination& dest
4017 |  bool CWallet::UpgradeWallet(int version, std::string& error, std::vector<std::string>& warnings)
4018 |  {
4019 |      int prev_version = GetVersion();
4020 | -    int nMaxVersion = version;
4021 | -    if (nMaxVersion == 0) // the -upgradewallet without argument case
4022 | +    if (version == 0) // the -upgradewallet without argument case

MarcoFalke commented at 12:47 PM on May 2, 2020:

nit in commit d0982de633783ba285772d159e87672762e0c400

-upgradewallet has been removed

    if (version == 0) {

Or, if you feel like it you may also just cherry-pick the commits from #18730

achow101 commented at 4:37 PM on May 4, 2020:

I've cherry picked those commits.

DrahtBot added the label Needs rebase on May 4, 2020

achow101 force-pushed on May 4, 2020

DrahtBot removed the label Needs rebase on May 4, 2020

luke-jr commented at 6:19 PM on May 4, 2020: member

How does this handle very old wallets that were previously -upgradewallet (to increment max version), but never actually upgraded (due to not using the new features)?

achow101 commented at 6:50 PM on May 4, 2020: member

How does this handle very old wallets that were previously -upgradewallet (to increment max version), but never actually upgraded (due to not using the new features)?

nWalletMaxVersion is an in-memory variable. So nothing was written to the wallet. Such users would not see anything different happen to their wallets.

DrahtBot cross-referenced this on May 9, 2020 from issue wallet: Move salvagewallet into wallettool by achow101

DrahtBot cross-referenced this on May 14, 2020 from issue wallet: Refactor the classes in wallet/db.{cpp/h} by achow101

achow101 cross-referenced this on May 21, 2020 from issue Sqlite wallet storage by Sjors

DrahtBot added the label Needs rebase on May 27, 2020

achow101 force-pushed on May 27, 2020

DrahtBot removed the label Needs rebase on May 27, 2020

achow101 force-pushed on Jun 2, 2020

achow101 commented at 10:09 PM on June 2, 2020: member

Had to rebase this on top of #19054 as the bug it fixes has an effect on the UpgradeKeyMetaData test.

DrahtBot cross-referenced this on Jun 15, 2020 from issue wallet: Skip hdKeypath of 'm' when determining inactive hd seeds by achow101

DrahtBot added the label Needs rebase on Jun 16, 2020

achow101 force-pushed on Jun 17, 2020

DrahtBot removed the label Needs rebase on Jun 17, 2020

DrahtBot added the label Needs rebase on Jun 19, 2020

achow101 force-pushed on Jun 19, 2020

DrahtBot removed the label Needs rebase on Jun 19, 2020

DrahtBot cross-referenced this on Jun 25, 2020 from issue Remove the automatic creation and loading of the default wallet by achow101

DrahtBot cross-referenced this on Jul 2, 2020 from issue script: previous_release.sh rewritten in python by bliotti

DrahtBot cross-referenced this on Jul 29, 2020 from issue Remove wallet.dat path handling from wallet.cpp, rpcwallet.cpp by ryanofsky

DrahtBot cross-referenced this on Aug 4, 2020 from issue wallet: Replace -zapwallettxes with zapwallettxes RPC by achow101

DrahtBot cross-referenced this on Aug 6, 2020 from issue wallet: Remove -zapwallettxes by achow101

achow101 cross-referenced this on Aug 17, 2020 from issue wallet: restore condition for HD chain split upgrade by S3RK

MarcoFalke added this to the milestone 0.21.0 on Aug 17, 2020

MarcoFalke commented at 5:30 PM on August 17, 2020: member

Assigned milestone because this might be a required bugfix

in src/wallet/scriptpubkeyman.h:209 in a63d3c95ba outdated

 206 | @@ -207,6 +207,7 @@ class ScriptPubKeyMan
 207 |  
 208 |      /** Upgrades the wallet to the specified version */
 209 |      virtual bool Upgrade(int prev_version, bilingual_str& error) { return false; }

S3RK commented at 12:13 PM on August 19, 2020:

I believe we can drop old version of Upgrade now

achow101 commented at 5:04 PM on August 20, 2020:

Done

in src/wallet/scriptpubkeyman.cpp:461 in a63d3c95ba outdated

 456 | +    if (IsFeatureSupported(new_version, FEATURE_HD_SPLIT)) {
 457 |          WalletLogPrintf("Upgrading wallet to use HD chain split\n");
 458 |          m_storage.SetMinVersion(FEATURE_PRE_SPLIT_KEYPOOL);
 459 |          split_upgrade = FEATURE_HD_SPLIT > prev_version;
 460 | +        // Upgrade the HDChain
 461 | +        if (m_hd_chain.nVersion < CHDChain::VERSION_HD_CHAIN_SPLIT) {

S3RK commented at 10:48 AM on August 20, 2020:

It's slightly better to move this condition to parent if to avoid log entry in case when we don't actually upgrade anything.

achow101 commented at 4:42 PM on August 20, 2020:

We are still upgrading to HD chain split even if the HDChain version is VERSION_HD_CHAIN_SPLIT. Theoretically, it is possible to have a HDChain with VERSION_HD_CHAIN_SPLIT but still be using non-split HD. But in practice, I don't think that can actually happen.

S3RK commented at 11:02 AM on August 20, 2020: contributor

I reviewed all the code, except part with deserialization of BDB format, that I just skimmed over. Only a couple nit suggestions. Built (macOS 10.13), run and played with the code in debugger.

Overall the changes proposed are clear improvement over current state. Current mechanism is confusing indeed and could lead to bugs.

Happy see this merged instead of #19747

achow101 force-pushed on Aug 20, 2020

achow101 commented at 5:31 PM on August 20, 2020: member

Rebased as apparently there was a silent merge conflict with master.

achow101 force-pushed on Aug 20, 2020

DrahtBot cross-referenced this on Aug 20, 2020 from issue wallet: Replace -zapwallettxes with wallet tool command by achow101

DrahtBot cross-referenced this on Aug 20, 2020 from issue wallet: Add sqlite as an alternative wallet database and use it for new descriptor wallets by achow101

S3RK commented at 8:05 AM on August 21, 2020: contributor

ACK 7f625c0c9a2ab214b23929cecb1e703be7e17058

Btw, any reasons why not fix typo pointed out by brakmic?

achow101 commented at 5:07 PM on August 21, 2020: member

Btw, any reasons why not fix typo pointed out by brakmic?

Forgot to do that. I'll do it if I need to push again.

DrahtBot added the label Needs rebase on Sep 1, 2020

achow101 force-pushed on Sep 1, 2020

DrahtBot removed the label Needs rebase on Sep 1, 2020

achow101 force-pushed on Sep 1, 2020

DrahtBot added the label Needs rebase on Sep 7, 2020

achow101 force-pushed on Sep 7, 2020

DrahtBot removed the label Needs rebase on Sep 7, 2020

achow101 cross-referenced this on Sep 8, 2020 from issue -Wlogical-op warning in wallet/scriptpubkeyman.cpp when building current master by kristapsk

DrahtBot added the label Needs rebase on Sep 18, 2020

achow101 force-pushed on Sep 18, 2020

DrahtBot removed the label Needs rebase on Sep 18, 2020

maskoficarus cross-referenced this on Sep 20, 2020 from issue refactor: clean up -Wlogical-op warning in wallet/scriptpubkeyman.cpp by maskoficarus

fanquake commented at 12:35 PM on September 29, 2020: member

@ryanofsky any chance you'd be interested in reviewing here?

michaelfolkson commented at 12:52 PM on September 29, 2020: contributor

Are any of your Twitch streams focused on the writing of this PR @achow101 or any particular guidance on review?

I'd like to dig into it and provide more than a cursory review but unsure where to start.

DrahtBot cross-referenced this on Sep 29, 2020 from issue test: Get rid of default wallet hacks by ryanofsky

achow101 commented at 3:24 PM on September 29, 2020: member

Are any of your Twitch streams focused on the writing of this PR

Unfortunately no.

or any particular guidance on review?

The goal is to make explicitly clear as to when an upgrade is occurring for a particular version. There are two main components to this, although both boil down to the same fix.

CanSupportFeature doesn't just check whether the current wallet version (the version written to the wallet) is able to support a particular version, it also checks the nWalletMaxVersion memory only variable. This makes some components in UpgradeWallet not work or behave unintuitively because we assume CanSupportFeature only does the obvious thing of checking the actual wallet version.
Because of nWalletMaxVersion, for some features, the actual wallet version doesn't change until that feature is used. However for other features, UpgradeWallet immediately changes the version number and activates the new feature. This behavior is also unintuitive and could result in the user expecting an upgrade to have occurred when one did not.

The solution to these issues is to remove nWalletMaxVersion. This makes CanSupportFeature intuitive and easy to reason about. This removes the delayed upgrade behavior so UpgradeWallet behaves as we would expect it to and the behavior of upgrading is unified for all features.

achow101 force-pushed on Oct 1, 2020

DrahtBot added the label Needs rebase on Oct 2, 2020

achow101 force-pushed on Oct 2, 2020

DrahtBot removed the label Needs rebase on Oct 2, 2020

fanquake referenced this in commit a1e0359618 on Oct 19, 2020

wallet: Add utility method for CanSupportFeature 842ae3842d

wallet: Add GetClosestWalletFeature function

Given a version number, get the closest supported WalletFeature
for a version number.

5f720544f3

achow101 force-pushed on Oct 19, 2020

achow101 commented at 4:24 AM on October 19, 2020: member

Rebased for silent merge conflict

sidhujag referenced this in commit cfc58deb8b on Oct 19, 2020

S3RK commented at 2:56 AM on October 22, 2020: contributor

Re-ACK c8d14017f12e79c48f4d4f1dd1b420982bf45ffc

Reviewed the diff between 7f625c0c9a2ab214b23929cecb1e703be7e17058.. c8d14017f12e79c48f4d4f1dd1b420982bf45ffc Only changes related to new self.default_wallet_name in tests, an additional assert added and typo fixed.

DrahtBot cross-referenced this on Oct 30, 2020 from issue wallet: List all wallets in non-SQLite and non-BDB builds by ryanofsky

in src/wallet/scriptpubkeyman.cpp:456 in 6d2935b1f3 outdated

 452 | @@ -453,7 +453,7 @@ bool LegacyScriptPubKeyMan::Upgrade(int prev_version, bilingual_str& error)
 453 |          hd_upgrade = true;
 454 |      }
 455 |      // Upgrade to HD chain split if necessary
 456 | -    if (m_storage.CanSupportFeature(FEATURE_HD_SPLIT)) {
 457 | +    if (IsFeatureSupported(new_version, FEATURE_HD_SPLIT)) {

MarcoFalke commented at 1:16 PM on November 4, 2020:

new_version is generally 0 to upgrade all, so this check will fail

This is fixed in the next commit, but tests will probably fail on this commit (if we have tests for this).

May be possible to fix by adding an & between int nMaxVersion: int& nMaxVersion

achow101 commented at 5:22 PM on November 4, 2020:

It appears the tests didn't cover this, so there was no test failure here. However I've changed nMaxVersion to a reference as suggested.

in src/wallet/wallet.cpp:4108 in a772fa8c7f outdated

4109 | -        SetMinVersion(FEATURE_LATEST); // permanently upgrade the wallet immediately
4110 | -    } else {
4111 | -        WalletLogPrintf("Allowing wallet upgrade up to %i\n", nMaxVersion);
4112 | +        version = FEATURE_LATEST;
4113 | +    }
4114 | +    else {

MarcoFalke commented at 1:37 PM on November 4, 2020:

nit in a772fa8c7f8c8130c3400fd2a9288e1280b4dac8: any reason to change the formatting here?

achow101 commented at 5:22 PM on November 4, 2020:

Fixed

in src/dummywallet.cpp:5 in c8d14017f1 outdated

   1 | @@ -2,6 +2,7 @@
   2 |  // Distributed under the MIT software license, see the accompanying
   3 |  // file COPYING or http://www.opensource.org/licenses/mit-license.php.
   4 |  
   5 | +#include <support/allocators/secure.h>

MarcoFalke commented at 2:59 PM on November 4, 2020:

nit: remove (silent merge conflict)

achow101 commented at 5:22 PM on November 4, 2020:

Removed

in test/functional/wallet_upgradewallet.py:231 in 91125ca0d4 outdated

 226 | +            info = wallet.getaddressinfo(wallet.getnewaddress())
 227 | +            assert 'hdkeypath' not in info
 228 | +            assert 'hdseedid' not in info
 229 | +        # Next key should be HD
 230 | +        info = wallet.getaddressinfo(wallet.getnewaddress())
 231 | +        assert_equal(binascii.hexlify(seed_id).decode(), info['hdseedid'])

MarcoFalke commented at 3:00 PM on November 4, 2020:

in commit 91125ca0d459c112e48db9bbaa10dd1380a3e32c:

Pretty sure you can just write seed_id.hex() (without binascii)

achow101 commented at 5:22 PM on November 4, 2020:

Done.

MarcoFalke commented at 3:01 PM on November 4, 2020: member

Approach ACK c8d14017f1

some style nits

wallet: have ScriptPubKeyMan::Upgrade check against the new version

Instead of using CanSupportFeature and relying on nWalletMaxVersion,
take the new version we are upgrading to and use IsSupportedFeature
with that and the previous wallet version.

bd7398cc62

wallet: remove nWalletMaxVersion

nWalletMaxVersion was used to allow an upgrade to a version only
when the new feature was used. This makes sense for the old
-upgradewallet startup option. But because upgradewallet is now a RPC,
putting off the version bump like this does not make sense. Instead,
immediately upgrading to the given version number makes sense.

8e32e1c41c

wallet: upgrade the CHDChain version number when upgrading to split hd 0bd995aa19

tests: Add a sha256sum_file function to util 092fc43485

test: Add test_framework/bdb.py module for inspecting bdb files

For upgrade tests and possibly other tests, it is useful to inspect the
bdb file for the wallet (i.e. the wallet.dat file).
test_framework/bdb.py is an implementation of bdb file deserialization
specific for Bitcoin Core's usage.

4b418a9dec

tests: Test specific upgradewallet scenarios and that upgrades work bf7635963c

test: Remove unused wallet.dat a314271f08

wallet: Remove -upgradewallet from dummywallet 5f9c0b6360

achow101 force-pushed on Nov 4, 2020

MarcoFalke commented at 7:37 PM on November 4, 2020: member

approach ACK 5f9c0b6360

in src/wallet/walletutil.h:33 in 5f9c0b6360

  28 | @@ -29,7 +29,8 @@ enum WalletFeature
  29 |      FEATURE_LATEST = FEATURE_PRE_SPLIT_KEYPOOL
  30 |  };
  31 |  
  32 | -
  33 | +bool IsFeatureSupported(int wallet_version, int feature_version);
  34 | +WalletFeature GetClosestWalletFeature(int version);

jonatack commented at 9:24 AM on November 8, 2020:

5f72054 here and in the function definition, would be clearer to specify kind of version (wallet or feature) by using the same param naming as IsFeatureSupported() (maybe it should be named feature_version; not clear to me)

WalletFeature GetClosestWalletFeature(int wallet_version);

achow101 commented at 5:56 PM on November 8, 2020:

There's only one version number, the wallet version number. The feature_version above is for the version number constants.

in src/wallet/walletutil.cpp:90 in 5f9c0b6360

  85 | +    if (version >= FEATURE_HD) return FEATURE_HD;
  86 | +    if (version >= FEATURE_COMPRPUBKEY) return FEATURE_COMPRPUBKEY;
  87 | +    if (version >= FEATURE_WALLETCRYPT) return FEATURE_WALLETCRYPT;
  88 | +    if (version >= FEATURE_BASE) return FEATURE_BASE;
  89 | +    return static_cast<WalletFeature>(0);
  90 | +}

jonatack commented at 9:59 AM on November 8, 2020:

5f72054 could do this (tested); shorter, easier to update, less error-prone as no double entry of each constant name

WalletFeature GetClosestWalletFeature(int version)
 {
-    if (version >= FEATURE_LATEST) return FEATURE_LATEST;
-    if (version >= FEATURE_PRE_SPLIT_KEYPOOL) return FEATURE_PRE_SPLIT_KEYPOOL;
-    if (version >= FEATURE_NO_DEFAULT_KEY) return FEATURE_NO_DEFAULT_KEY;
-    if (version >= FEATURE_HD_SPLIT) return FEATURE_HD_SPLIT;
-    if (version >= FEATURE_HD) return FEATURE_HD;
-    if (version >= FEATURE_COMPRPUBKEY) return FEATURE_COMPRPUBKEY;
-    if (version >= FEATURE_WALLETCRYPT) return FEATURE_WALLETCRYPT;
-    if (version >= FEATURE_BASE) return FEATURE_BASE;
+    const std::array<WalletFeature, 8> wallet_features{{FEATURE_LATEST, FEATURE_PRE_SPLIT_KEYPOOL, FEATURE_NO_DEFAULT_KEY, FEATURE_HD_SPLIT, FEATURE_HD, FEATURE_COMPRPUBKEY, FEATURE_WALLETCRYPT, FEATURE_BASE}};
+    for (const WalletFeature& wf : wallet_features) {
+        if (version >= wf) return wf;
+    }
     return static_cast<WalletFeature>(0);
 }

achow101 commented at 6:05 PM on November 8, 2020:

Can be done in a followup. The WalletFeature enum needs to be cleaned up a bit too.

jonatack commented at 6:21 PM on November 16, 2020:

Done in #20403

in src/wallet/wallet.cpp:4112 in 5f9c0b6360

4114 |      }
4115 | -    if (nMaxVersion < GetVersion())
4116 | +    if (version < prev_version)
4117 |      {
4118 |          error = _("Cannot downgrade wallet");
4119 |          return false;

jonatack commented at 10:19 AM on November 8, 2020:

Should this downgrade check and error message be done first, e.g. at the top of UpgradeWallet(), and the two values printed in the error message for user friendliness

achow101 commented at 6:01 PM on November 8, 2020:

No, version needs to be determined first. Otherwise it could be 0 here and that would not be correct.

in test/functional/test_framework/util.py:271 in 5f9c0b6360

 266 | +    with open(filename, 'rb') as f:
 267 | +        d = f.read(4096)
 268 | +        while len(d) > 0:
 269 | +            h.update(d)
 270 | +            d = f.read(4096)
 271 | +    return h.digest()

jonatack commented at 10:28 AM on November 8, 2020:

092fc43 A similar function exists in test/functional/tool_wallet.py.wallet_shasum, perhaps combine the two. Nit: add a newline before and after this function.

achow101 commented at 6:05 PM on November 8, 2020:

Can be done in a followup.

in test/functional/wallet_upgradewallet.py:149 in 5f9c0b6360

 155 | +            node_master.loadwallet(self.default_wallet_name)
 156 |  
 157 | -        wallet = node_master.get_wallet_rpc('')
 158 | +        def copy_split_hd():
 159 | +            node_master.get_wallet_rpc(self.default_wallet_name).unloadwallet()
 160 | +            # Copy the 0.15.2 split hd wallet to the last Bitcoin Core version and open it:

jonatack commented at 10:31 AM on November 8, 2020:

bf76359 maybe make this comment a docstring, idem for the two preceding new functions

achow101 commented at 6:06 PM on November 8, 2020:

This was moved so I will leave it as is for now.

in test/functional/wallet_upgradewallet.py:185 in 5f9c0b6360

 181 | @@ -137,5 +182,165 @@ def run_test(self):
 182 |          # after conversion master key hash should be present
 183 |          assert_is_hex_string(wallet.getwalletinfo()['hdseedid'])
 184 |  
 185 | +        self.log.info('Intermediary versions don\'t effect anything')

jonatack commented at 10:31 AM on November 8, 2020:

bf76359

        self.log.info("Intermediary versions don't effect anything")

achow101 commented at 6:07 PM on November 8, 2020:

Meh. Will leave this alone to avoid invalidating ACKs.

in src/wallet/wallet.cpp:4103 in 5f9c0b6360

4099 | @@ -4120,33 +4100,31 @@ const CAddressBookData* CWallet::FindAddressBookEntry(const CTxDestination& dest
4100 |  bool CWallet::UpgradeWallet(int version, bilingual_str& error, std::vector<bilingual_str>& warnings)
4101 |  {
4102 |      int prev_version = GetVersion();
4103 | -    int nMaxVersion = version;
4104 | -    if (nMaxVersion == 0) // the -upgradewallet without argument case
4105 | -    {
4106 | +    if (version == 0) {

jonatack commented at 10:37 AM on November 8, 2020:

5f9c0b6 Can you make this change in the "wallet: remove nWalletMaxVersion" commit instead

achow101 commented at 6:09 PM on November 8, 2020:

That commit still uses the nMaxVersion so I don't think this change would be appropriate there.

jonatack commented at 5:37 PM on November 12, 2020:

Re-looking, I agree it could be in either commit, nvm.

jonatack commented at 11:05 AM on November 8, 2020: contributor

ACK 5f9c0b6360215636cfa62a70d3a70f1feb3977ab, approach seems fine, code review, only skimmed the test changes but they look well done, rebased on current master, debug built and verified the wallet_upgradewallet.py test runs green both before and after running test/get_previous_releases.py -b v0.19.1 v0.18.1 v0.17.2 v0.16.3 v0.15.2

fanquake requested review from meshcollider on Nov 10, 2020

DrahtBot cross-referenced this on Nov 11, 2020 from issue test: Implicitly sync after generate* to preempt races and intermittent test failures by MarcoFalke

in test/functional/test_framework/bdb.py:24 in 5f9c0b6360

  19 | +Page format can be found in BDB source code dbinc/db_page.h
  20 | +This only implements the deserialization of btree metadata pages and normal btree pages. Overflow
  21 | +pages are not implemented but may be needed in the future if dealing with wallets with large
  22 | +transactions.
  23 | +
  24 | +`db_dump -da wallet.dat` is useful to see the data in a wallet.dat BDB file

laanwj commented at 6:59 PM on November 12, 2020:

I think it's awesome work… but I'm a bit surprised and ambivalent about maintaining our own bdb parsing. At least it's test only :slightly_smiling_face:

achow101 cross-referenced this on Nov 13, 2020 from issue "Can't generate a change-address key. No keys in the internal keypool and can't generate any keys." by redblade7

laanwj commented at 10:02 AM on November 16, 2020: member

Code review ACK 5f9c0b6360215636cfa62a70d3a70f1feb3977ab

laanwj merged this on Nov 16, 2020

laanwj closed this on Nov 16, 2020

ryanofsky commented at 12:16 PM on November 16, 2020: contributor

Is there any summary of the user visible parts of this change? The 8e32e1c41c995e832e643f605d35a7aa112837e6 commit seems like a user-visible change, but it's unclear how it manifests. It's also not clear if there are behavior changes in other commits like 0bd995aa19be65b0dd23df1df571c71428c2bc32 or if these are just refactorings.

If there are behavior changes, the best thing would be a followup PR adding some release notes. It seems like upgradewallet will now immediately make a wallet incompatible with previous versions instead of only allowing new features which might make it incompatible in the future. But it would be helpful to know any if there are specific features or versions that are relevant, or if there are other user-visible changes, or if maybe I'm incorrect and the upgradewallet RPC always made wallets incompatible before this PR.

laanwj commented at 1:21 PM on November 16, 2020: member

If there are behavior changes, the best thing would be a followup PR adding some release notes

Please edit release notes in the wiki: https://github.com/bitcoin-core/bitcoin-devwiki/wiki/0.21.0-Release-Notes-Draft

It seems like upgradewallet will now immediately make a wallet incompatible with previous versions instead of only allowing new features which might make it incompatible in the future.

I had expected this to already be the behavior of upgradewallet, to be honest. It's pretty standard predictable upgrading behavior. I don't think "it becomes incompatible at some unspecifid time in the future" is in any way more useful. But sure, I guess if that changed it needs to be documented.

MarcoFalke commented at 1:30 PM on November 16, 2020: member

Not sure if relevant to the discussion, but upgradewallet is a new rpc in this release

sidhujag referenced this in commit 94dbdc1de8 on Nov 16, 2020

jonatack cross-referenced this on Nov 16, 2020 from issue wallet: upgradewallet fixes, improvements, test coverage by jonatack

MarcoFalke referenced this in commit afdfd3c8c1 on Nov 25, 2020

bitcoin locked this on Feb 15, 2022