Show error to user when corrupt wallet unlock fails #14478
Conversation
|
utACK 025afbe |
|
utACK 025afbe. |
|
Is there a reason to not shutdown gracefully? And why is this a fatal error? Today we could jut unload it? |
src/wallet/crypter.cpp
Outdated
| @@ -201,8 +202,9 @@ bool CCryptoKeyStore::Unlock(const CKeyingMaterial& vMasterKeyIn) | |||
| } | |||
| if (keyPass && keyFail) | |||
| { | |||
| uiInterface.ThreadSafeMessageBox(_("Error unlocking wallet. Your wallet file may be corrupt."), "", CClientUIInterface::MSG_ERROR); | |||
There was a problem hiding this comment.
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?
|
What @sipa said. Just returning an error state will prevent from unnecessary file/class-coupling |
| else | ||
| { | ||
| QDialog::accept(); // Success | ||
| try { |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| QDialog::accept(); // Success | ||
| } | ||
| } catch (const std::runtime_error& e) { | ||
| QMessageBox::critical(this, tr("Wallet unlock failed"), e.what()); |
There was a problem hiding this comment.
The wallet remains loaded?
This seems ok but it might be nice to add a code comment about why it's intended.
| else | ||
| { | ||
| QDialog::accept(); // Success | ||
| try { |
There was a problem hiding this comment.
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.
| QDialog::accept(); // Success | ||
| } | ||
| } catch (const std::runtime_error& e) { | ||
| QMessageBox::critical(this, tr("Wallet unlock failed"), e.what()); |
There was a problem hiding this comment.
The wallet remains loaded?
This seems ok but it might be nice to add a code comment about why it's intended.
|
utACK 48062643f13b5442dec84cfa3d64e98e3815530b. Could squash? |
|
Squashed 👍 |
b4f6e58 Better error message for user when corrupt wallet unlock fails (MeshCollider) Pull request description: Mentioned here: bitcoin#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. Tree-SHA512: 526f9ed9262257fca55caf7153ab913ed958b13b079d2f01db797485614d8c375815a1554276e8cf73d3838104b2691a9cf85c8d097973127ae8de9e111446bf
Summary: Better error message for user when corrupt wallet unlock fails (MeshCollider) Pull request description: Mentioned here: bitcoin/bitcoin#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. bitcoin/bitcoin@b4f6e58 --- Backport of Core [[bitcoin/bitcoin#14478 | PR14478]] Test Plan: ninja check Reviewers: #bitcoin_abc, jasonbcox Reviewed By: #bitcoin_abc, jasonbcox Differential Revision: https://reviews.bitcoinabc.org/D7312
b4f6e58 Better error message for user when corrupt wallet unlock fails (MeshCollider) Pull request description: Mentioned here: bitcoin#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. Tree-SHA512: 526f9ed9262257fca55caf7153ab913ed958b13b079d2f01db797485614d8c375815a1554276e8cf73d3838104b2691a9cf85c8d097973127ae8de9e111446bf
b4f6e58 Better error message for user when corrupt wallet unlock fails (MeshCollider) Pull request description: Mentioned here: bitcoin#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. Tree-SHA512: 526f9ed9262257fca55caf7153ab913ed958b13b079d2f01db797485614d8c375815a1554276e8cf73d3838104b2691a9cf85c8d097973127ae8de9e111446bf
b4f6e58 Better error message for user when corrupt wallet unlock fails (MeshCollider) Pull request description: Mentioned here: bitcoin#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. Tree-SHA512: 526f9ed9262257fca55caf7153ab913ed958b13b079d2f01db797485614d8c375815a1554276e8cf73d3838104b2691a9cf85c8d097973127ae8de9e111446bf
b4f6e58 Better error message for user when corrupt wallet unlock fails (MeshCollider) Pull request description: Mentioned here: bitcoin#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. Tree-SHA512: 526f9ed9262257fca55caf7153ab913ed958b13b079d2f01db797485614d8c375815a1554276e8cf73d3838104b2691a9cf85c8d097973127ae8de9e111446bf
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.