Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Show error to user when corrupt wallet unlock fails #14478

Merged

Conversation

Projects
None yet
7 participants
@meshcollider
Copy link
Member

commented Oct 14, 2018

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 meshcollider added the Wallet label Oct 14, 2018

@donaloconnor

This comment has been minimized.

Copy link
Contributor

commented Oct 14, 2018

utACK 025afbe

@ryanofsky
Copy link
Contributor

left a comment

utACK 025afbe139684a467928958cebf1fb01930f5354

@promag

This comment has been minimized.

Copy link
Member

commented Oct 15, 2018

utACK 025afbe.

@promag

This comment has been minimized.

Copy link
Member

commented Oct 16, 2018

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

This comment has been minimized.

Copy link
@sipa

sipa Oct 17, 2018

Member

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

This comment has been minimized.

Copy link
Member

commented Oct 17, 2018

What @sipa said. Just returning an error state will prevent from unnecessary file/class-coupling

else
{
QDialog::accept(); // Success
try {

This comment has been minimized.

Copy link
@promag

promag Oct 18, 2018

Member

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

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Oct 18, 2018

Contributor

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?

This comment has been minimized.

Copy link
@promag

promag Oct 18, 2018

Member

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.

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Oct 19, 2018

Contributor

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.

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Oct 20, 2018

Member

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());

This comment has been minimized.

Copy link
@promag

promag Oct 18, 2018

Member

The wallet remains loaded?

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Oct 19, 2018

Contributor

The wallet remains loaded?

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

@ryanofsky
Copy link
Contributor

left a comment

utACK 48062643f13b5442dec84cfa3d64e98e3815530b. Change since last review is replacing uiinterface callback and abort with an exception that gets bubbled up.

else
{
QDialog::accept(); // Success
try {

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Oct 19, 2018

Contributor

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());

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Oct 19, 2018

Contributor

The wallet remains loaded?

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

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Nov 8, 2018

utACK 48062643f13b5442dec84cfa3d64e98e3815530b. Could squash?

@meshcollider meshcollider force-pushed the meshcollider:201810_wallet_corruption_error branch to b4f6e58 Nov 10, 2018

@meshcollider

This comment has been minimized.

Copy link
Member Author

commented Nov 10, 2018

Squashed 👍

@ryanofsky
Copy link
Contributor

left a comment

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

This change might be ready to merge.

MarcoFalke added a commit to MarcoFalke/bitcoin that referenced this pull request Nov 14, 2018

Merge bitcoin#14478: Show error to user when corrupt wallet unlock fails
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

@MarcoFalke MarcoFalke merged commit b4f6e58 into bitcoin:master Nov 14, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@meshcollider meshcollider deleted the meshcollider:201810_wallet_corruption_error branch Nov 14, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.