Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

wallet: upgradewallet fixes and additional tests #18836

Merged
merged 10 commits into from
Nov 16, 2020

Conversation

achow101
Copy link
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.

@achow101 achow101 changed the title Upgradewallet tests wallet: tests: upgradewallet fixes and additional tests Apr 30, 2020
@maflcko maflcko removed the Tests label Apr 30, 2020
@maflcko maflcko changed the title wallet: tests: upgradewallet fixes and additional tests wallet: upgradewallet fixes and additional tests Apr 30, 2020
Copy link
Contributor

@brakmic brakmic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code reviewed, only a small typo found.

test/functional/test_framework/bdb.py Outdated Show resolved Hide resolved
@brakmic
Copy link
Contributor

brakmic commented Apr 30, 2020

ACK 0c96108

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 bitcoin deleted a comment from shahramkha Apr 30, 2020
@DrahtBot
Copy link
Contributor

DrahtBot commented May 1, 2020

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

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.

src/wallet/wallet.cpp Outdated Show resolved Hide resolved
@luke-jr
Copy link
Member

luke-jr commented May 4, 2020

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
Copy link
Member Author

achow101 commented May 4, 2020

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.

@achow101
Copy link
Member Author

Rebased for silent merge conflict

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Oct 19, 2020
…t/scriptpubkeyman.cp

95fedd3 refactor: Clean up -Wlogical-op warning (maskoficarus)

Pull request description:

  This is a quick patch that fixes bitcoin#19912 . This change prevents a -Wlogical-op warning that occurs because we're treating a const int value as a boolean. There's no sense checking if a non-zero constant has a value, so I've removed the check.

  bitcoin#18836 also addresses the same warning, but has a larger scope and will require more review. This pull request will act as a patch to prevent this compile warning until 18836 is merged.

ACKs for top commit:
  MarcoFalke:
    review ACK 95fedd3
  hebasto:
    ACK 95fedd3, tested on Linux Mint 20 (x86_64):

Tree-SHA512: 13a9d7f7cb472f4c22a01ca2f9771a75267ff769bdae9d0dc6b2c7f3b05369f6dfa859be2b172b39c15ede6c44cddf556380b3565e42850faa65ccd3fe6e175b
@S3RK
Copy link
Contributor

S3RK commented Oct 22, 2020

Re-ACK c8d1401

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

Copy link
Member

@maflcko maflcko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approach ACK c8d1401

some style nits

src/wallet/scriptpubkeyman.cpp Show resolved Hide resolved
src/wallet/wallet.cpp Outdated Show resolved Hide resolved
src/dummywallet.cpp Outdated Show resolved Hide resolved
test/functional/wallet_upgradewallet.py Outdated Show resolved Hide resolved
achow101 and others added 8 commits November 4, 2020 12:10
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.
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.
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.
@maflcko
Copy link
Member

maflcko commented Nov 4, 2020

approach ACK 5f9c0b6

Copy link
Contributor

@jonatack jonatack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 5f9c0b6, 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

@@ -29,7 +29,8 @@ enum WalletFeature
FEATURE_LATEST = FEATURE_PRE_SPLIT_KEYPOOL
};


bool IsFeatureSupported(int wallet_version, int feature_version);
WalletFeature GetClosestWalletFeature(int version);
Copy link
Contributor

@jonatack jonatack Nov 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Suggested change
WalletFeature GetClosestWalletFeature(int version);
WalletFeature GetClosestWalletFeature(int wallet_version);

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if (version >= FEATURE_WALLETCRYPT) return FEATURE_WALLETCRYPT;
if (version >= FEATURE_BASE) return FEATURE_BASE;
return static_cast<WalletFeature>(0);
}
Copy link
Contributor

@jonatack jonatack Nov 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);
 }

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in #20403

}
if (nMaxVersion < GetVersion())
if (version < prev_version)
{
error = _("Cannot downgrade wallet");
return false;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

while len(d) > 0:
h.update(d)
d = f.read(4096)
return h.digest()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can be done in a followup.

wallet = node_master.get_wallet_rpc('')
def copy_split_hd():
node_master.get_wallet_rpc(self.default_wallet_name).unloadwallet()
# Copy the 0.15.2 split hd wallet to the last Bitcoin Core version and open it:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -137,5 +182,165 @@ def run_test(self):
# after conversion master key hash should be present
assert_is_hex_string(wallet.getwalletinfo()['hdseedid'])

self.log.info('Intermediary versions don\'t effect anything')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bf76359

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Meh. Will leave this alone to avoid invalidating ACKs.

int nMaxVersion = version;
if (nMaxVersion == 0) // the -upgradewallet without argument case
{
if (version == 0) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

pages are not implemented but may be needed in the future if dealing with wallets with large
transactions.

`db_dump -da wallet.dat` is useful to see the data in a wallet.dat BDB file
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 🙂

@laanwj
Copy link
Member

laanwj commented Nov 16, 2020

Code review ACK 5f9c0b6

@laanwj laanwj merged commit c48e788 into bitcoin:master Nov 16, 2020
@ryanofsky
Copy link
Contributor

Is there any summary of the user visible parts of this change? The 8e32e1c 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 0bd995a 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
Copy link
Member

laanwj commented Nov 16, 2020

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.

@maflcko
Copy link
Member

maflcko commented Nov 16, 2020

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

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Nov 16, 2020
maflcko pushed a commit to bitcoin-core/gui that referenced this pull request Nov 25, 2020
3eb6f8b wallet (not for backport): improve upgradewallet error messages (Jon Atack)
ca8cd89 wallet: fix and improve upgradewallet error responses (Jon Atack)
99d56e3 wallet: fix and improve upgradewallet result responses (Jon Atack)
2498b04 Don't upgrade to HD split if it is already supported (Andrew Chow)
c46c18b wallet: refactor GetClosestWalletFeature() (Jon Atack)

Pull request description:

  This follows up on #18836 and #20282 to fix and improve the as-yet unreleased `upgradewallet` feature and also implement review follow-up in bitcoin/bitcoin#18836 (comment).

  This PR fixes 4 upgradewallet issues:

  - this bug: bitcoin/bitcoin#20403 (comment)
  - it returns nothing in the absence of an RPC error, which isn't reassuring for users
  - it returns the same thing both in the case of a successful upgrade and when no upgrade took place
  - the error message object is currently dead code

  This PR fixes the above and provides:

  ...user feedback to not silently return without upgrading
  ```
  {
    "wallet_name": "disable private keys",
    "previous_version": 169900,
    "current_version": 169900,
    "result": "Already at latest version. Wallet version unchanged."
  }
  ```
  ...better feedback after successfully upgrading
  ```
  {
    "wallet_name": "watch-only",
    "previous_version": 159900,
    "current_version": 169900,
    "result": "Wallet upgraded successfully from version 159900 to version 169900."
  }
  ```
  ...helpful error responses
  ```
  {
    "wallet_name": "blank",
    "previous_version": 169900,
    "current_version": 169900,
    "error": "Cannot downgrade wallet from version 169900 to version 159900. Wallet version unchanged."
  }
  {
    "wallet_name": "blank",
    "previous_version": 130000,
    "current_version": 130000,
    "error": "Cannot upgrade a non HD split wallet from version 130000 to version 169899 without upgrading to support pre-split keypool. Please use version 169900 or no version specified."
  }
  ```
  updated help:
  ```
  upgradewallet ( version )

  Upgrade the wallet. Upgrades to the latest version if no version number is specified.
  New keys may be generated and a new wallet backup will need to be made.
  Arguments:
  1. version    (numeric, optional, default=169900) The version number to upgrade to. Default is the latest wallet version.

  Result:
  {                            (json object)
    "wallet_name" : "str",     (string) Name of wallet this operation was performed on
    "previous_version" : n,    (numeric) Version of wallet before this operation
    "current_version" : n,     (numeric) Version of wallet after this operation
    "result" : "str",          (string, optional) Description of result, if no error
    "error" : "str"            (string, optional) Error message (if there is one)
  }
  ```

ACKs for top commit:
  achow101:
    ACK  3eb6f8b
  MarcoFalke:
    review ACK 3eb6f8b 🛡

Tree-SHA512: b767314069e26b5933b123acfea6aa40708507f504bdb22884da020a4ca1332af38a7072b061e36281533af9f4e236d94d3c129daf6fe5b55241127537038eed
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet