Wallet: do not return warnings from UpgradeWallet()

stackman27 commented at 9:07 pm on October 12, 2020: contributor

The warning variable was unused in upgradewallet so I removed it

in src/wallet/rpcwallet.cpp:4442 in 13952e451f outdated

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

MarcoFalke commented at 9:10 pm on October 12, 2020:

Would be good to return an object instead. See

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

stackman27 commented at 5:49 am on October 14, 2020:

RPC Result changed from str to object

MarcoFalke added this to the milestone 0.21.0 on Oct 12, 2020

stackman27 force-pushed on Oct 12, 2020

DrahtBot added the label RPC/REST/ZMQ on Oct 12, 2020

DrahtBot added the label Wallet on Oct 12, 2020

achow101 commented at 6:03 pm on October 13, 2020: member

ACK c370e82bce3640d0ed99cff16e815b66853a8320

laanwj commented at 9:01 am on October 15, 2020: member

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 commented at 4:40 pm on October 15, 2020: contributor

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 force-pushed on Oct 19, 2020

stackman27 commented at 5:51 am on October 19, 2020: contributor

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 commented at 3:06 am on October 22, 2020: member

Code review ACK d218f9e5ca0a39902abd55d0bb1a5e9d7829761d

in src/wallet/rpcwallet.cpp:4464 in d218f9e5ca outdated

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

MarcoFalke commented at 7:25 am on October 22, 2020:

This is not an object what is returned here

stackman27 commented at 7:16 am on October 25, 2020:

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

luke-jr commented at 6:53 pm on October 24, 2020: member

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 commented at 12:59 pm on October 25, 2020: member

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

MarcoFalke commented at 9:22 pm on October 25, 2020: member

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 commented at 2:05 pm on October 27, 2020: member

Concept ACK removing unused code.

jnewbery commented at 9:10 am on October 28, 2020: member

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.

MarcoFalke commented at 9:16 am on October 28, 2020: member

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 commented at 10:03 am on October 28, 2020: member

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.

jnewbery renamed this:
~~Removed unused warning and formatted RPC result~~
Wallet: Change upgradewallet return type to be an object
on Oct 28, 2020

stackman27 commented at 6:52 pm on October 28, 2020: contributor

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 commented at 7:45 pm on October 28, 2020: member

Both commits in this PR is fine

stackman27 force-pushed on Oct 29, 2020

in src/wallet/rpcwallet.cpp:4444 in 32a1aea7b2 outdated

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

MarcoFalke commented at 6:43 am on October 30, 2020:

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

stackman27 commented at 4:31 am on October 31, 2020:

Done!

stackman27 force-pushed on Oct 30, 2020

meshcollider commented at 11:03 pm on November 1, 2020: contributor

utACK c93f2ac1054ec07dbe5f3c80bfda3ca8cc2e48c0

in src/wallet/rpcwallet.cpp:4470 in c93f2ac105 outdated

4469+    if (!pwallet->UpgradeWallet(version, error)) {
4470         throw JSONRPCError(RPC_WALLET_ERROR, error.original);
4471     }
4472-    return error.original;
4473+    UniValue obj(UniValue::VOBJ);
4474+    obj.pushKV("error", error.original);

MarcoFalke commented at 7:19 am on November 2, 2020:

This is not optionally pushed

jnewbery commented at 8:42 am on November 2, 2020: member

@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.

fanquake removed this from the milestone 0.21.0 on Nov 2, 2020

MarcoFalke added the label Waiting for author on Nov 2, 2020

jnewbery renamed this:
~~Wallet: Change upgradewallet return type to be an object~~
Wallet: do not return warnings from UpgradeWallet()
on Nov 2, 2020

DrahtBot commented at 3:49 pm on November 2, 2020: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#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 force-pushed on Nov 2, 2020

stackman27 commented at 10:33 pm on November 2, 2020: contributor

@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 commented at 1:53 am on November 4, 2020: contributor

@stackman27 you may like to include the suggested changes from #20282#pullrequestreview-521620062 in this PR too

MarcoFalke removed the label Waiting for author on Nov 4, 2020

MarcoFalke removed the label RPC/REST/ZMQ on Nov 4, 2020

MarcoFalke added the label Refactoring on Nov 4, 2020

MarcoFalke commented at 8:05 am on November 4, 2020: member

review ACK bca54204b54e4ce82b59daa6ebac60020a810f76

stackman27 force-pushed on Nov 9, 2020

stackman27 force-pushed on Nov 13, 2020

stackman27 commented at 7:20 pm on November 13, 2020: contributor

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

Done! Could you please verify?

stackman27 force-pushed on Nov 13, 2020

in src/wallet/rpcwallet.cpp:4486 in 481f640495 outdated

4484     if (!error.empty()) {
4485         obj.pushKV("error", error.original);
4486+    } else {
4487+        if(!request.params[0].isNull()) {
4488+            obj.pushKV("result", "The wallet was upgraded successfully to version 169900.");
4489+        }

jonatack commented at 6:53 pm on November 16, 2020:

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).

stackman27 commented at 8:50 pm on November 16, 2020:

okay so I’ll just take the result object out

jonatack commented at 6:55 pm on November 16, 2020: member

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

[upgradewallet] removed unused warning param 9636962889

stackman27 force-pushed on Nov 16, 2020

practicalswift commented at 9:56 am on November 17, 2020: contributor

ACK 963696288955dc31b3a4fd136bfb791a9d99755b: diff looks correct

Thanks for removing unused stuff.

MarcoFalke commented at 10:11 am on November 17, 2020: member

review ACK 963696288955dc31b3a4fd136bfb791a9d99755b

jonatack commented at 11:05 am on November 17, 2020: member

ACK 963696288955dc31b3a4fd136bfb791a9d99755b

MarcoFalke merged this on Nov 17, 2020

MarcoFalke closed this on Nov 17, 2020

sidhujag referenced this in commit 8412b5d478 on Nov 17, 2020

Fabcien referenced this in commit dbfb1067ae on Dec 23, 2021

DrahtBot locked this on Feb 15, 2022

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

Conflicts