wallet: change upgradewallet return type to be an object#20282
Merged
meshcollider merged 1 commit intobitcoin:masterfrom Nov 4, 2020
Merged
wallet: change upgradewallet return type to be an object#20282meshcollider merged 1 commit intobitcoin:masterfrom
meshcollider merged 1 commit intobitcoin:masterfrom
Conversation
jnewbery
changed the title
jnewbery
changed the title
Member
|
ACK 2ead31f Thanks for picking this up to avoid a delay in the 0.21 release! |
jonatack
reviewed
Nov 2, 2020
Member
jonatack
left a comment
jonatack
left a comment
There was a problem hiding this comment.
ACK, tested the upgradewallet help and output manually both for successful and failure cases. Two thoughts: (a) it might be user-friendly to provide a message that the upgrade was successful rather than an empty JSON object with a newline in the middle, e.g. {"result": "the wallet was upgraded successfully to version 16990"}, and (b) could add a functional test case for the result in case of an error.
$ bitcoin-cli -regtest upgradewallet
{
}
$ bitcoin-cli -regtest upgradewallet
{
"error": "Cannot downgrade wallet"
}
$ bitcoin-cli -regtest help upgradewallet
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)
"error" : "str" (string, optional) Error message (if there is one)
}
Examples:
> bitcoin-cli upgradewallet 169900
> curl --user myusername --data-binary '{"jsonrpc": "1.0", "id": "curltest", "method": "upgradewallet", "params": [169900]}' -H 'content-type: text/plain;' http://127.0.0.1:8332/
Member
|
a) seems like a feature, so should be a separate pull |
Contributor
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsNo conflicts as of last run. |
sidhujag
pushed a commit
to syscoin/syscoin
that referenced
this pull request
Nov 4, 2020
…n object 2ead31f [wallet] Return object from upgradewallet RPC (Sishir Giri) Pull request description: Change the return type of upgradewallet to be an object for future extensibility. Also return any error string returned from the `UpgradeWallet()` function. ACKs for top commit: MarcoFalke: ACK 2ead31f meshcollider: Tested ACK 2ead31f Tree-SHA512: bcc7432d2f35093ec2463ea19e894fa885b698c0e8d8e4bd2f979bd4d722cbfed53ec589d6280968917893c64649dc9e40800b8d854273b0f9a1380f51afbdb1
Member
Done in #20403, which also fixes an issue of upgradewallet silently not upgrading. |
maflcko
pushed a commit
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 #18836 (comment). This PR fixes 4 upgradewallet issues: - this bug: #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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
7 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Change the return type of upgradewallet to be an object for future extensibility.
Also return any error string returned from the
UpgradeWallet()function.