Show error to user when corrupt wallet unlock fails #14478

pull meshcollider wants to merge 1 commits into bitcoin:master from meshcollider:201810_wallet_corruption_error changing 2 files +10 −9

meshcollider commented at 7:31 AM on October 14, 2018: contributor

Mentioned here: #14461 (comment)

Current behavior is to assert(false) and crash, only info is printed in the log. This shows the message to the user before abort() instead.
meshcollider added the label Wallet on Oct 14, 2018
donaloconnor commented at 8:39 PM on October 14, 2018: contributor

utACK https://github.com/bitcoin/bitcoin/pull/14478/commits/025afbe139684a467928958cebf1fb01930f5354
ryanofsky approved
ryanofsky commented at 4:28 PM on October 15, 2018: member

utACK 025afbe139684a467928958cebf1fb01930f5354
promag commented at 5:07 PM on October 15, 2018: member

utACK 025afbe.
promag commented at 6:24 PM on October 16, 2018: member

Is there a reason to not shutdown gracefully? And why is this a fatal error? Today we could jut unload it?

in src/wallet/crypter.cpp:205 in 025afbe139 outdated

 201 | @@ -201,8 +202,9 @@ bool CCryptoKeyStore::Unlock(const CKeyingMaterial& vMasterKeyIn)
 202 |          }
 203 |          if (keyPass && keyFail)
 204 |          {
 205 | +            uiInterface.ThreadSafeMessageBox(_("Error unlocking wallet. Your wallet file may be corrupt."), "", CClientUIInterface::MSG_ERROR);

sipa commented at 7:21 AM on October 17, 2018:

Pretty ugly to call into the UI from such a low-level routine. Is it possible to instead return a failure status, and have the caller deal with this?

jonasschnelli commented at 7:37 AM on October 17, 2018: contributor

What @sipa said. Just returning an error state will prevent from unnecessary file/class-coupling
in src/qt/askpassphrasedialog.cpp:155 in 48062643f1 outdated
```
 158 | -                                  tr("The passphrase entered for the wallet decryption was incorrect."));
 159 | -        }
 160 | -        else
 161 | -        {
 162 | -            QDialog::accept(); // Success
 163 | +        try {
```
promag commented at 12:37 AM on October 18, 2018:

I think this is as ugly as calling UI from low level, only inverted.

One alternative is to catch the exception in WalletImpl::unlock and return an error.

Orthogonally CCryptoKeyStore::Unlock could also return an error instead of assert(false).

ryanofsky commented at 1:01 AM on October 18, 2018:

I think this is as ugly as calling UI from low level, only inverted.

I don't think I understand why this ugly. Can you explain? Is the problem just that the code throws an exception instead of returning an error code?
promag commented at 1:20 AM on October 18, 2018:

Hope I can explain:

removing the assert doesn't affect this code and the try/catch would stay unnecessarily

it is required to see the implementation to understand why is a try/catch here

Since we don't use throw specifiers, I think typed errors are more readable.
ryanofsky commented at 5:05 PM on October 19, 2018:

Thanks for clarifying. It sounds like either switching to a return code, or switching from std::runtime_error to a specific exception type would address your concerns. I agree these things would be improvements. I'm also ok with current PR, though.

MarcoFalke commented at 2:40 PM on October 20, 2018:

ignore my diatribe, but exceptions are just the cpp-ish way of goto. Especially with our wildcard catch blocks, this is hard to review and easy to break in the future. Less braindead programming languages like rust show that error handling can be done without exceptions or gotos.

in src/qt/askpassphrasedialog.cpp:163 in 48062643f1 outdated

 166 | +                                      tr("The passphrase entered for the wallet decryption was incorrect."));
 167 | +            } else {
 168 | +                QDialog::accept(); // Success
 169 | +            }
 170 | +        } catch (const std::runtime_error& e) {
 171 | +            QMessageBox::critical(this, tr("Wallet unlock failed"), e.what());

promag commented at 12:37 AM on October 18, 2018:

The wallet remains loaded?

ryanofsky commented at 5:11 PM on October 19, 2018:

The wallet remains loaded?

This seems ok but it might be nice to add a code comment about why it's intended.

ryanofsky commented at 5:16 PM on October 19, 2018: member

utACK 48062643f13b5442dec84cfa3d64e98e3815530b. Change since last review is replacing uiinterface callback and abort with an exception that gets bubbled up.
MarcoFalke commented at 8:32 PM on November 8, 2018: member

utACK 48062643f13b5442dec84cfa3d64e98e3815530b. Could squash?
Better error message for user when corrupt wallet unlock fails b4f6e58ca5
meshcollider commented at 2:08 AM on November 10, 2018: contributor

Squashed 👍
ryanofsky approved
ryanofsky commented at 3:49 PM on November 14, 2018: member

utACK b4f6e58ca5a74225b6db9deb3ffe1ce3ab19a790. No changes since last review except to squash.

This change might be ready to merge.
MarcoFalke referenced this in commit c7366d2399 on Nov 14, 2018
MarcoFalke merged this on Nov 14, 2018
MarcoFalke closed this on Nov 14, 2018
meshcollider deleted the branch on Nov 14, 2018
deadalnix referenced this in commit b8b40a6e66 on Sep 2, 2020
knst referenced this in commit 3be9f4e684 on Aug 10, 2021
knst referenced this in commit 39c2c4be8a on Aug 10, 2021
knst referenced this in commit 32f1460b4f on Aug 16, 2021
PastaPastaPasta referenced this in commit 1809b8f038 on Aug 23, 2021
UdjinM6 referenced this in commit f38aa44bfd on Aug 29, 2021
MarcoFalke locked this on Dec 16, 2021

Contributors

Labels

Linked (view graph)

#14461 Can't Unlock My BTC Core Wallet With Correct Password