-
Notifications
You must be signed in to change notification settings - Fork 35.6k
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: change upgradewallet return type to be an object #20282
wallet: change upgradewallet return type to be an object #20282
Conversation
ACK 2ead31f Thanks for picking this up to avoid a delay in the 0.21 release! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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/
a) seems like a feature, so should be a separate pull |
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsNo conflicts as of last run. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested ACK 2ead31f
…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
Done in #20403, which also fixes an issue of upgradewallet silently not upgrading. |
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
Change the return type of upgradewallet to be an object for future extensibility.
Also return any error string returned from the
UpgradeWallet()
function.