wallet: Avoid showing GUI popups on RPC errors

MarcoFalke commented at 2:19 pm on October 7, 2019: member

RPC errors and warnings are shown as popups in the GUI instead of being returned to the RPC caller. For example,

0$ ./src/bitcoin-cli loadwallet $(pwd)/./test/functional/data/wallets/high_minversion/
1error code: -4
2error message:
3Wallet loading failed.

gives me a GUI popup and no reason why loading the wallet failed.

After this pull request:

0$ ./src/bitcoin-cli loadwallet $(pwd)/./test/functional/data/wallets/high_minversion/
1error code: -4
2error message:
3Wallet loading failed: Error loading /home/marco/workspace/btc_bitcoin_core/./test/functional/data/wallets/high_minversion/wallet.dat: Wallet requires newer version of Bitcoin Core

MarcoFalke added this to the milestone 0.20.0 on Oct 7, 2019

fanquake added the label Wallet on Oct 7, 2019

MarcoFalke commented at 2:20 pm on October 7, 2019: member

While this is a bugfix, it is not a regression and it can wait until 0.20.0

promag commented at 2:37 pm on October 7, 2019: member

Concept ACK.

DrahtBot commented at 2:37 pm on October 7, 2019: 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:

#16963 (wallet: Fix unique_ptr usage in boost::signals2 by promag)
#16923 (wallet: Fix duplicates fileid exception on start by promag)
#16839 (Replace Connman and BanMan globals with Node local by ryanofsky)
#16224 (gui: Bilingual GUI error messages by hebasto)
#15761 (Replace -upgradewallet startup option with upgradewallet RPC by achow101)
#15204 (gui: Add Open External Wallet action by promag)

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.

hebasto commented at 4:31 pm on October 7, 2019: member

Concept ACK.

laanwj commented at 8:25 am on October 8, 2019: member

Whoops, yes this is kind of an oversight.

in src/wallet/load.cpp:57 in fa99a26fd8 outdated

53@@ -53,10 +54,10 @@ bool VerifyWallets(interfaces::Chain& chain, const std::vector<std::string>& wal
54         }
55 
56         std::string error_string;
57-        std::string warning_string;
58+        std::vector<std::string> warning_string;

jonasschnelli commented at 9:33 am on October 8, 2019:

µ-nit: use warning_strings (plural) to be consistent with other changes.

MarcoFalke commented at 1:58 pm on October 8, 2019:

Will do when there is other stuff to change

jonasschnelli commented at 9:35 am on October 8, 2019: contributor

Tested ACK fa99a26fd8a96525ae0046a7470eae9926e42153 - tested the loadwallet failure case with gui + -server as well as the same command in the GUI console. Works now as intended. Code review ACK.

It is a relatively large but somehow sane change. I’m okay with this as a 0.19 bugfix.

in src/wallet/load.cpp:74 in fa99a26fd8 outdated

66@@ -66,8 +67,14 @@ bool VerifyWallets(interfaces::Chain& chain, const std::vector<std::string>& wal
67 bool LoadWallets(interfaces::Chain& chain, const std::vector<std::string>& wallet_files)
68 {
69     for (const std::string& walletFile : wallet_files) {
70-        std::shared_ptr<CWallet> pwallet = CWallet::CreateWalletFromFile(chain, WalletLocation(walletFile));
71+        std::string error;
72+        std::vector<std::string> warnings;
73+        std::shared_ptr<CWallet> pwallet = CWallet::CreateWalletFromFile(chain, WalletLocation(walletFile), error, warnings);
74+        for (const auto& w : warnings) {
75+            chain.initWarning(w);

promag commented at 2:51 pm on October 8, 2019:

Probably makes sense to Join() warnings as it is already done in other places?

MarcoFalke commented at 4:54 pm on October 8, 2019:

Good point

promag commented at 2:58 pm on October 8, 2019: member

Tested ACK fa99a26fd8a96525ae0046a7470eae9926e42153.

Could split in 2 commits: the first refactors to a vector of warnings, the second moves calls to chain.initWarning from CWallet::CreateWalletFromFile to LoadWallets.

One comment inline.

MarcoFalke force-pushed on Oct 8, 2019

MarcoFalke commented at 4:58 pm on October 8, 2019: member

Force pushed changes requested by @promag and @jonasschnelli

wallet: Avoid showing GUI popups on RPC errors facec1c643

MarcoFalke force-pushed on Oct 8, 2019

promag commented at 7:10 am on October 9, 2019: member

Could split in 2 commits: the first refactors to a vector of warnings, the second moves calls to chain.initWarning from CWallet::CreateWalletFromFile to LoadWallets. @MarcoFalke just wondering if you have considered this, or it doesn’t even matter.

MarcoFalke commented at 2:39 pm on October 9, 2019: member

@promag Not sure if it does help review to split up, so I kept it in one commit for now. Waiting for other to chime in…

laanwj commented at 11:47 am on October 21, 2019: member

Code review ACK facec1c643105d0ae74b5d32cf33d593f9e82a36

I don’t think it’s necessary to split this up into multiple commits.

~0 on backporting this or not.

laanwj referenced this in commit a22b62481a on Oct 21, 2019

laanwj merged this on Oct 21, 2019

laanwj closed this on Oct 21, 2019

sidhujag referenced this in commit 5cf6b95062 on Oct 21, 2019

MarcoFalke deleted the branch on Nov 11, 2019

MarkLTZ referenced this in commit 4f89048cb5 on Nov 17, 2019

MarcoFalke referenced this in commit b7bc9b8330 on Nov 20, 2019

sidhujag referenced this in commit 32cde68f0b on Nov 21, 2019

laanwj referenced this in commit 6196e93001 on Jan 8, 2020

sidhujag referenced this in commit a46d74a339 on Jan 8, 2020

sidhujag referenced this in commit e45b937895 on Nov 10, 2020

sidhujag referenced this in commit fc6d53ddd2 on Nov 10, 2020

DrahtBot locked this on Feb 15, 2022

wallet: Avoid showing GUI popups on RPC errors #17070

Conflicts