wallet: Avoid showing GUI popups on RPC errors #17070

maflcko · 2019-10-07T14:19:49Z

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

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

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

After this pull request:

$ ./src/bitcoin-cli loadwallet $(pwd)/./test/functional/data/wallets/high_minversion/
error code: -4
error message:
Wallet 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

maflcko · 2019-10-07T14:20:25Z

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

promag · 2019-10-07T14:37:01Z

Concept ACK.

DrahtBot · 2019-10-07T14:37:32Z

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 · 2019-10-07T16:31:05Z

Concept ACK.

laanwj · 2019-10-08T08:25:22Z

Whoops, yes this is kind of an oversight.

src/wallet/load.cpp

jonasschnelli · 2019-10-08T09:35:50Z

Tested ACK fa99a26 - 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.

promag

Tested ACK fa99a26.

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.

src/wallet/load.cpp

maflcko · 2019-10-08T16:58:22Z

Force pushed changes requested by @promag and @jonasschnelli

promag · 2019-10-09T07:10:31Z

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.

maflcko · 2019-10-09T14:39:36Z

@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 · 2019-10-21T11:47:07Z

Code review ACK facec1c

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

~0 on backporting this or not.

facec1c wallet: Avoid showing GUI popups on RPC errors (MarcoFalke) Pull request description: RPC errors and warnings are shown as popups in the GUI instead of being returned to the RPC caller. For example, ``` $ ./src/bitcoin-cli loadwallet $(pwd)/./test/functional/data/wallets/high_minversion/ error code: -4 error message: Wallet loading failed. ``` gives me a GUI popup and no reason why loading the wallet failed. After this pull request: ``` $ ./src/bitcoin-cli loadwallet $(pwd)/./test/functional/data/wallets/high_minversion/ error code: -4 error message: Wallet 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 ACKs for top commit: laanwj: Code review ACK facec1c Tree-SHA512: c8274bbb02cfcf71676eeec1e773e51fb3538cf93f82e7cb8536f4716d44ed819cdc162dfc039ac7386a4db381a734cdb27fd32567043a1180c02519fbcba194

faffa7f wallet: Avoid showing GUI popups on RPC errors (take 2) (MarcoFalke) Pull request description: Commit 8b0d82b claims "This commit does not change behavior." However, it re-introduced the bug I tried to fix in #17070 ACKs for top commit: ryanofsky: Code review ACK faffa7f Tree-SHA512: 99987f80c76414dca40c7d76b2fe4ea853debbe3c49e7acdeab2596c726a2935c468f4484d49212e65ecc9c8b0d861c0c2b83c1ddfc07670540699199dbfecb0

…take 2) faffa7f wallet: Avoid showing GUI popups on RPC errors (take 2) (MarcoFalke) Pull request description: Commit 8b0d82b claims "This commit does not change behavior." However, it re-introduced the bug I tried to fix in bitcoin#17070 ACKs for top commit: ryanofsky: Code review ACK faffa7f Tree-SHA512: 99987f80c76414dca40c7d76b2fe4ea853debbe3c49e7acdeab2596c726a2935c468f4484d49212e65ecc9c8b0d861c0c2b83c1ddfc07670540699199dbfecb0

6d6a7a8 gui: Fix duplicate wallet showing up (João Barbosa) 81ea66c Drop signal CClientUIInterface::LoadWallet (Russell Yanofsky) Pull request description: This PR includes 2 fixes: - prevent GUI LoadWallet handlers from crashing on startup when multiple handlers are attached, because the first handler takes ownership of the wallet unique pointer. Now every handler will receive its own unique pointer; - prevent showing a wallet twice in the GUI on startup due to a race with `loadwallet`. Fixes #16937 ACKs for top commit: fjahr: code review ACK 6d6a7a8 ryanofsky: Code review ACK 6d6a7a8. No changes since last ACK other than rebase due to #17070 kallewoof: Code review ACK 6d6a7a8 Tree-SHA512: 7f0658c9011f81dfa176a094c2263448ee1d14fda7dc94e8b55ee9c8b81538bd2d1e4bf8a8dbfcd029ebfc9feb6d3cda9dee3f911122df0a4b1e0ca75f653ba4

6d6a7a8 gui: Fix duplicate wallet showing up (João Barbosa) 81ea66c Drop signal CClientUIInterface::LoadWallet (Russell Yanofsky) Pull request description: This PR includes 2 fixes: - prevent GUI LoadWallet handlers from crashing on startup when multiple handlers are attached, because the first handler takes ownership of the wallet unique pointer. Now every handler will receive its own unique pointer; - prevent showing a wallet twice in the GUI on startup due to a race with `loadwallet`. Fixes bitcoin#16937 ACKs for top commit: fjahr: code review ACK 6d6a7a8 ryanofsky: Code review ACK 6d6a7a8. No changes since last ACK other than rebase due to bitcoin#17070 kallewoof: Code review ACK 6d6a7a8 Tree-SHA512: 7f0658c9011f81dfa176a094c2263448ee1d14fda7dc94e8b55ee9c8b81538bd2d1e4bf8a8dbfcd029ebfc9feb6d3cda9dee3f911122df0a4b1e0ca75f653ba4

…take 2) faffa7f wallet: Avoid showing GUI popups on RPC errors (take 2) (MarcoFalke) Pull request description: Commit 8b0d82b claims "This commit does not change behavior." However, it re-introduced the bug I tried to fix in bitcoin#17070 ACKs for top commit: ryanofsky: Code review ACK faffa7f Tree-SHA512: 99987f80c76414dca40c7d76b2fe4ea853debbe3c49e7acdeab2596c726a2935c468f4484d49212e65ecc9c8b0d861c0c2b83c1ddfc07670540699199dbfecb0

6d6a7a8 gui: Fix duplicate wallet showing up (João Barbosa) 81ea66c Drop signal CClientUIInterface::LoadWallet (Russell Yanofsky) Pull request description: This PR includes 2 fixes: - prevent GUI LoadWallet handlers from crashing on startup when multiple handlers are attached, because the first handler takes ownership of the wallet unique pointer. Now every handler will receive its own unique pointer; - prevent showing a wallet twice in the GUI on startup due to a race with `loadwallet`. Fixes bitcoin#16937 ACKs for top commit: fjahr: code review ACK 6d6a7a8 ryanofsky: Code review ACK 6d6a7a8. No changes since last ACK other than rebase due to bitcoin#17070 kallewoof: Code review ACK 6d6a7a8 Tree-SHA512: 7f0658c9011f81dfa176a094c2263448ee1d14fda7dc94e8b55ee9c8b81538bd2d1e4bf8a8dbfcd029ebfc9feb6d3cda9dee3f911122df0a4b1e0ca75f653ba4

maflcko added this to the 0.20.0 milestone Oct 7, 2019

fanquake added the Wallet label Oct 7, 2019

jonasschnelli reviewed Oct 8, 2019

View reviewed changes

src/wallet/load.cpp Outdated Show resolved Hide resolved

promag reviewed Oct 8, 2019

View reviewed changes

src/wallet/load.cpp Outdated Show resolved Hide resolved

maflcko force-pushed the 1909-walletGuiPopupRpc branch from fa99a26 to fa5fdcc Compare October 8, 2019 16:57

wallet: Avoid showing GUI popups on RPC errors

facec1c

maflcko force-pushed the 1909-walletGuiPopupRpc branch from fa5fdcc to facec1c Compare October 8, 2019 17:02

maflcko mentioned this pull request Oct 15, 2019

gui: RPC responses should not be translated #17072

Closed

laanwj merged commit facec1c into bitcoin:master Oct 21, 2019

hebasto mentioned this pull request Oct 26, 2019

gui: Bilingual GUI error messages #16224

Merged

maflcko mentioned this pull request Nov 11, 2019

wallet: Avoid showing GUI popups on RPC errors (take 2) #17444

Merged

maflcko deleted the 1909-walletGuiPopupRpc branch November 11, 2019 19:16

MarkLTZ added a commit to litecoinz-core/litecoinz that referenced this pull request Nov 17, 2019

[wallet] Avoid showing GUI popups on RPC errors (bitcoin#17070)

4f89048

MarkLTZ mentioned this pull request Apr 4, 2020

Bitcoin PR tracking litecoinz-core/litecoinz#14

Closed

ryanofsky mentioned this pull request May 11, 2021

rpc: The RPC result contains '\n' #21916

Closed

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

wallet: Avoid showing GUI popups on RPC errors #17070

wallet: Avoid showing GUI popups on RPC errors #17070

Uh oh!

maflcko commented Oct 7, 2019

Uh oh!

maflcko commented Oct 7, 2019

Uh oh!

promag commented Oct 7, 2019

Uh oh!

DrahtBot commented Oct 7, 2019 •

edited

Loading

Uh oh!

hebasto commented Oct 7, 2019

Uh oh!

laanwj commented Oct 8, 2019

Uh oh!

Uh oh!

jonasschnelli commented Oct 8, 2019

Uh oh!

promag left a comment •

edited

Loading

Uh oh!

Uh oh!

maflcko commented Oct 8, 2019

Uh oh!

promag commented Oct 9, 2019

Uh oh!

maflcko commented Oct 9, 2019

Uh oh!

laanwj commented Oct 21, 2019 •

edited

Loading

Uh oh!

Uh oh!

wallet: Avoid showing GUI popups on RPC errors #17070

wallet: Avoid showing GUI popups on RPC errors #17070

Uh oh!

Conversation

maflcko commented Oct 7, 2019

Uh oh!

maflcko commented Oct 7, 2019

Uh oh!

promag commented Oct 7, 2019

Uh oh!

DrahtBot commented Oct 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

hebasto commented Oct 7, 2019

Uh oh!

laanwj commented Oct 8, 2019

Uh oh!

Uh oh!

jonasschnelli commented Oct 8, 2019

Uh oh!

promag left a comment • 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!

Uh oh!

maflcko commented Oct 8, 2019

Uh oh!

promag commented Oct 9, 2019

Uh oh!

maflcko commented Oct 9, 2019

Uh oh!

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

Uh oh!

Uh oh!

Uh oh!

DrahtBot commented Oct 7, 2019 •

edited

Loading

promag left a comment •

edited

Loading

laanwj commented Oct 21, 2019 •

edited

Loading