Wallet: do not return warnings from UpgradeWallet() #20139

stackman27 · 2020-10-12T21:07:20Z

The warning variable was unused in upgradewallet so I removed it

maflcko · 2020-10-12T21:10:23Z

src/wallet/rpcwallet.cpp

@@ -4438,7 +4438,9 @@ static RPCHelpMan upgradewallet()
        {
            {"version", RPCArg::Type::NUM, /* default */ strprintf("%d", FEATURE_LATEST), "The version number to upgrade to. Default is the latest wallet version"}
        },
-        RPCResults{},
+        RPCResult{
+            RPCResult::Type::STR, "Error", "error description during upgrading wallet"


Would be good to return an object instead. See

doc/developer-notes.md:- Try to make the RPC response a JSON object.

RPC Result changed from str to object

achow101 · 2020-10-13T18:03:26Z

ACK c370e82

laanwj · 2020-10-15T09:01:20Z

I think this is a fairly confusing combination of changes in one commit. There's the doc update and the warnings unused field removal that make sense individually, but don't have much in common.
(you could split it into two commits within this same PR maybe)

stackman27 · 2020-10-15T16:40:35Z

I think this is a fairly confusing combination of changes in one commit. There's the doc update and the warnings unused field removal that make sense individually, but don't have much in common.
(you could split it into two commits within this same PR maybe)

Do you suggest one commit as doc update and another as removal of unused field?

stackman27 · 2020-10-19T05:51:39Z

I think this is a fairly confusing combination of changes in one commit. There's the doc update and the warnings unused field removal that make sense individually, but don't have much in common.
(you could split it into two commits within this same PR maybe)

Fixed!

S3RK · 2020-10-22T03:06:53Z

Code review ACK d218f9e

maflcko · 2020-10-22T07:25:32Z

src/wallet/rpcwallet.cpp

    bilingual_str error;
-    std::vector<bilingual_str> warnings;
-    if (!pwallet->UpgradeWallet(version, error, warnings)) {
+    if (!pwallet->UpgradeWallet(version, error)) {
        throw JSONRPCError(RPC_WALLET_ERROR, error.original);
    }
    return error.original;


This is not an object what is returned here

so should I have UpgradeWallet return an object? I'm a little confused

luke-jr · 2020-10-24T18:53:14Z

NACK, this doesn't make sense to me as-is.

And even if we don't have any warnings today, it might be nice to keep the interface in case we do later.

hebasto · 2020-10-25T12:59:21Z

Weak Concept NACK. I lean to agree with @luke-jr about keeping (yet unused) warnings in the interface.

maflcko · 2020-10-25T21:22:05Z

Why? If they are kept you'd have to return an always empty warnings array. Otherwise, it will get used in the future and surely it will be forgotten to update the RPC method to return the warnings.

fanquake · 2020-10-27T14:05:43Z

Concept ACK removing unused code.

jnewbery · 2020-10-28T09:10:26Z

There's the doc update and the warnings unused field removal that make sense individually, but don't have much in common.

I agree with this. This should be split into two PRs.

I think we can remove the 0.21 milestone. This isn't a bugfix and doesn't need to included in the next release.

maflcko · 2020-10-28T09:16:15Z

Changing the return type is a breaking change if done after the 0.21 release. I don't see why such a trivial fix should pushed back.

jnewbery · 2020-10-28T10:03:19Z

Ah. For some reason I thought that upgradewallet had been in a previous release. I agree that we should fix the return format now to avoid a breaking RPC interface change.

@stackman27 - I've fixed up the return value and tests (as well as cleaned up the commit logs) here: https://github.com/jnewbery/bitcoin/tree/pr20139.1. Feel free to take that branch.

stackman27 · 2020-10-28T18:52:22Z

Ah. For some reason I thought that upgradewallet had been in a previous release. I agree that we should fix the return format now to avoid a breaking RPC interface change.

@stackman27 - I've fixed up the return value and tests (as well as cleaned up the commit logs) here: https://github.com/jnewbery/bitcoin/tree/pr20139.1. Feel free to take that branch.

Should I have 2 different commits or 2 different PR's one for documentation and one for changing the return type ?

jnewbery · 2020-10-28T19:45:37Z

Both commits in this PR is fine

maflcko · 2020-10-30T06:43:44Z

src/wallet/rpcwallet.cpp

+        RPCResult{
+            RPCResult::Type::OBJ, "", "",
+            {
+                {RPCResult::Type::STR, "Error", "error description during upgrading wallet"}


No need to return an error if there is none. Please make this optional like in analyzepsbt

meshcollider

utACK c93f2ac

maflcko · 2020-11-02T07:19:55Z

src/wallet/rpcwallet.cpp

        throw JSONRPCError(RPC_WALLET_ERROR, error.original);
    }
-    return error.original;
+    UniValue obj(UniValue::VOBJ);
+    obj.pushKV("error", error.original);


This is not optionally pushed

jnewbery · 2020-11-02T08:42:19Z

@stackman27, since we need the Return object from upgradewallet RPC for 0.21, I've pulled it into a separate PR #20282 and addressed Marco's review comment. You can use this PR for the Remove unused warning parameter in UpgradeWallet() commit, which doesn't require the 0.21 milestone.

DrahtBot · 2020-11-02T15:49:53Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

wallet: upgradewallet fixes, improvements, test coverage #20403 (wallet: upgradewallet fixes, improvements, test coverage by jonatack)

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.

stackman27 · 2020-11-02T22:33:07Z

@stackman27, since we need the Return object from upgradewallet RPC for 0.21, I've pulled it into a separate PR #20282 and addressed Marco's review comment. You can use this PR for the Remove unused warning parameter in UpgradeWallet() commit, which doesn't require the 0.21 milestone.

Done! Could you please check and verify?

meshcollider · 2020-11-04T01:53:32Z

@stackman27 you may like to include the suggested changes from #20282 (review) in this PR too

maflcko · 2020-11-04T08:05:09Z

review ACK bca5420

stackman27 · 2020-11-13T19:20:40Z

@stackman27 you may like to include the suggested changes from #20282 (review) in this PR too

Done! Could you please verify?

jonatack

Concept ACK on just removing the unused warnings. Will re-review after update.

jonatack · 2020-11-16T18:53:01Z

src/wallet/rpcwallet.cpp

+    } else {
+        if(!request.params[0].isNull()) {
+            obj.pushKV("result", "The wallet was upgraded successfully to version 169900.");
+        }


I think this addition can be dropped along with the test change (see also #20403, and apologies--I didn't realize you were working on this until DrahtBot mentioned it).

okay so I'll just take the result object out

practicalswift · 2020-11-17T09:56:27Z

ACK 9636962: diff looks correct

Thanks for removing unused stuff.

maflcko · 2020-11-17T10:11:32Z

review ACK 9636962

jonatack · 2020-11-17T11:05:11Z

ACK 9636962

9636962 [upgradewallet] removed unused warning param (Sishir Giri) Pull request description: The `warning` variable was unused in `upgradewallet` so I removed it ACKs for top commit: practicalswift: ACK 9636962: diff looks correct MarcoFalke: review ACK 9636962 jonatack: ACK 9636962 Tree-SHA512: 1d63186ce1e05e86a778340f2d7986c2cee1523de0a11cea39e8d148ac7ee26c49741dfa302b5c1cd1c8d74e67c1f9baee2763720c2d850b57da9a3fdce24565

Summary: This is a backport of [[bitcoin/bitcoin#20139 | core#20139]] Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D10734

stackman27 mentioned this pull request Oct 12, 2020

upgradewallet rpc warnings and error maflcko/bitcoin-core#28

Closed

maflcko reviewed Oct 12, 2020

View reviewed changes

maflcko added this to the 0.21.0 milestone Oct 12, 2020

stackman27 force-pushed the upgradewallet_rpc_cleanup branch 2 times, most recently from 7348d82 to c370e82 Compare October 12, 2020 21:37

DrahtBot added RPC/REST/ZMQ Wallet labels Oct 12, 2020

stackman27 force-pushed the upgradewallet_rpc_cleanup branch from 97d16f2 to 627f57c Compare October 19, 2020 00:59

maflcko reviewed Oct 22, 2020

View reviewed changes

jnewbery changed the title ~~Removed unused warning and formatted RPC result~~ Wallet: Change upgradewallet return type to be an object Oct 28, 2020

stackman27 force-pushed the upgradewallet_rpc_cleanup branch from d218f9e to f922fc8 Compare October 29, 2020 21:05

maflcko reviewed Oct 30, 2020

View reviewed changes

stackman27 force-pushed the upgradewallet_rpc_cleanup branch from 32a1aea to f3a9928 Compare October 30, 2020 20:12

meshcollider reviewed Nov 1, 2020

View reviewed changes

maflcko reviewed Nov 2, 2020

View reviewed changes

fanquake removed this from the 0.21.0 milestone Nov 2, 2020

maflcko added the Waiting for author label Nov 2, 2020

jnewbery changed the title ~~Wallet: Change upgradewallet return type to be an object~~ Wallet: do not return warnings from UpgradeWallet() Nov 2, 2020

DrahtBot mentioned this pull request Nov 2, 2020

wallet: change upgradewallet return type to be an object #20282

Merged

stackman27 force-pushed the upgradewallet_rpc_cleanup branch 2 times, most recently from 793efcb to bca5420 Compare November 2, 2020 22:32

maflcko added Refactoring and removed Waiting for author RPC/REST/ZMQ labels Nov 4, 2020

stackman27 force-pushed the upgradewallet_rpc_cleanup branch 4 times, most recently from a6853cc to e72d8c5 Compare November 13, 2020 19:20

stackman27 force-pushed the upgradewallet_rpc_cleanup branch from e72d8c5 to 481f640 Compare November 13, 2020 21:45

DrahtBot mentioned this pull request Nov 16, 2020

wallet: upgradewallet fixes, improvements, test coverage #20403

Merged

jonatack reviewed Nov 16, 2020

View reviewed changes

[upgradewallet] removed unused warning param

9636962

stackman27 force-pushed the upgradewallet_rpc_cleanup branch from 481f640 to 9636962 Compare November 16, 2020 21:23

maflcko merged commit c463f70 into bitcoin:master Nov 17, 2020

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

Wallet: do not return warnings from UpgradeWallet() #20139

Wallet: do not return warnings from UpgradeWallet() #20139

Uh oh!

Conversation

stackman27 commented Oct 12, 2020 • edited by maflcko Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko Oct 12, 2020

Choose a reason for hiding this comment

Uh oh!

stackman27 Oct 14, 2020

Choose a reason for hiding this comment

Uh oh!

achow101 commented Oct 13, 2020

Uh oh!

laanwj commented Oct 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stackman27 commented Oct 15, 2020

Uh oh!

stackman27 commented Oct 19, 2020

Uh oh!

S3RK commented Oct 22, 2020

Uh oh!

maflcko Oct 22, 2020

Choose a reason for hiding this comment

Uh oh!

stackman27 Oct 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

luke-jr commented Oct 24, 2020

Uh oh!

hebasto commented Oct 25, 2020

Uh oh!

maflcko commented Oct 25, 2020

Uh oh!

fanquake commented Oct 27, 2020

Uh oh!

jnewbery commented Oct 28, 2020

Uh oh!

maflcko commented Oct 28, 2020

Uh oh!

jnewbery commented Oct 28, 2020

Uh oh!

stackman27 commented Oct 28, 2020

Uh oh!

jnewbery commented Oct 28, 2020

Uh oh!

maflcko Oct 30, 2020

Choose a reason for hiding this comment

Uh oh!

stackman27 Oct 31, 2020

Choose a reason for hiding this comment

Uh oh!

meshcollider left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko Nov 2, 2020

Choose a reason for hiding this comment

Uh oh!

jnewbery commented Nov 2, 2020

Uh oh!

DrahtBot commented Nov 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

stackman27 commented Nov 2, 2020

Uh oh!

meshcollider commented Nov 4, 2020

Uh oh!

maflcko commented Nov 4, 2020

Uh oh!

stackman27 commented Nov 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

jonatack Nov 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

stackman27 commented Oct 12, 2020 •

edited by maflcko

Loading

laanwj commented Oct 15, 2020 •

edited

Loading

stackman27 Oct 25, 2020 •

edited

Loading

DrahtBot commented Nov 2, 2020 •

edited

Loading

stackman27 commented Nov 13, 2020 •

edited

Loading

jonatack Nov 16, 2020 •

edited

Loading