Handle exceptions instead of crash #260

hebasto · 2021-03-26T18:14:45Z

This PR is an alternative to bitcoin/bitcoin#18897, and is based on Russ' idea:

IMO it would be nice to have a followup PR that eliminated the one-line forwarding methods ...

Related issues

Qt docs: https://doc.qt.io/qt-5.12/exceptionsafety.html#exceptions-in-client-code

With this PR the GUI handles the wallet-related exception, and:

display it to a user:

prints a message to stderr:



************************
EXCEPTION: 18NonFatalCheckError       
wallet/wallet.cpp:2677 (IsCurrentForAntiFeeSniping)
Internal bug detected: '!chain.findBlock(block_hash, FoundBlock().time(block_time))'
You may report this issue here: https://github.com/bitcoin/bitcoin/issues
       
bitcoin in QPushButton->SendCoinsDialog

writes a message to the debug.log
and, if the exception is a non-fatal error, leaves the main window running.

hebasto · 2021-03-26T18:16:01Z

~~Draft for now as @ryanofsky's comments should be addressed soon.~~

hebasto · 2021-03-27T19:26:30Z

Note to reviewers

To test this PR you could:

apply this patch

--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ -2674,7 +2674,7 @@ static bool IsCurrentForAntiFeeSniping(interfaces::Chain& chain, const uint256&
     }
     constexpr int64_t MAX_ANTI_FEE_SNIPING_TIP_AGE = 8 * 60 * 60; // in seconds
     int64_t block_time;
-    CHECK_NONFATAL(chain.findBlock(block_hash, FoundBlock().time(block_time)));
+    CHECK_NONFATAL(!chain.findBlock(block_hash, FoundBlock().time(block_time)));
     if (block_time < (GetTime() - MAX_ANTI_FEE_SNIPING_TIP_AGE)) {
         return false;
     }

and initiate a new transaction creation, or

apply this patch

--- a/src/wallet/interfaces.cpp
+++ b/src/wallet/interfaces.cpp
@@ -330,7 +330,7 @@ public:
         }
         num_blocks = m_wallet->GetLastBlockHeight();
         block_time = -1;
-        CHECK_NONFATAL(m_wallet->chain().findBlock(m_wallet->GetLastBlockHash(), FoundBlock().time(block_time)));
+        CHECK_NONFATAL(!m_wallet->chain().findBlock(m_wallet->GetLastBlockHash(), FoundBlock().time(block_time)));
         tx_status = MakeWalletTxStatus(*m_wallet, mi->second);
         return true;
     }

and initiate opening a wallet.

hebasto · 2021-03-27T19:38:43Z

@ryanofsky
re comment:

Would drop this special case for NonFatalCheckError and simply catch std::exception below. NonFatalCheckError is really an internal implementation of the CHECK macro and most useful to unit tests. Exceptions aren't valid in qt callbacks so NonFatalCheckError is no different here than any other exception. Just another unexpected condition that should never happen

The idea is to distinguish non-fatal errors when no need to stop the client; described by @MarcoFalke:

... it is more user friendly for a GUI to catch the exception and display it to the user, then abort the current action and leave the main window running.

Also, from Developer Notes:

CHECK_NONFATAL should be used for recoverable internal logic bugs. On failure, it will throw an exception, which can be caught to recover from the error.

hebasto · 2021-03-27T19:45:58Z

@promag @ryanofsky @jonasschnelli

Kindly asking for reviewing :)

🐅

leonardojobim

Tested ACK 63cca51 on Ubuntu 20.04 Qt Qt 5.12.8.

On master branch:

On this PR branch:

The exception was also displayed on terminal:

************************
EXCEPTION: 18NonFatalCheckError       
wallet/wallet.cpp:2678 (IsCurrentForAntiFeeSniping)
Internal bug detected: '!chain.findBlock(block_hash, FoundBlock().time(block_time))'
You may report this issue here: https://github.com/bitcoin/bitcoin/issues
       
bitcoin in QPushButton->SendCoinsDialog

laanwj · 2021-03-29T12:02:35Z

Personally I like the "A fatal error occured. Bitcoin Core can no longer …" message with a seperator and then the detailed error message much better than an "Internal error" popup with some developer text.

I think we should keep that.

prints a message to stderr:

Is it logged to the debug log? Anything printed to stderr tends to get lost (or is logged to a really hard to find place) on graphical operating systems.

ryanofsky

Code review ACK 63cca51. Agree with laanwj that error text could be improved and I made a specific suggestion below, but this is already better than crashing, and I like the approach of catching exceptions in the connect handler so they don't cause problems for Qt.

src/qt/guiutil.h

src/qt/bitcoin.cpp

ryanofsky · 2021-03-31T21:14:29Z

re: #260 (comment)

Is it logged to the debug log?

Yep, PrintExceptionContinue logs it.

Co-authored-by: Russell Yanofsky <russ@yanofsky.org>

This helper function will be used in the following commits. Co-authored-by: Russell Yanofsky <russ@yanofsky.org>

Throwing an exception from a slot invoked by Qt's signal-slot connection mechanism is considered undefined behavior, unless it is handled within the slot. The GUIUtil::ExceptionSafeConnect function should be used for exception handling within slots.

hebasto · 2021-04-01T00:11:47Z

@laanwj @ryanofsky

Thank you for your reviews and suggestions!

Updated 63cca51 -> f81672e (pr260.10 -> pr260.11, diff):

addressed reviewers' comments
small improvements

laanwj · 2021-04-05T12:38:22Z

src/qt/walletmodel.cpp

@@ -65,7 +65,8 @@ WalletModel::~WalletModel()
 void WalletModel::startPollBalance()
 {
    // This timer will be fired repeatedly to update the balance
-    connect(timer, &QTimer::timeout, this, &WalletModel::pollBalanceChanged);
+    connect(timer, &QTimer::timeout, this, &WalletModel::timerTimeout);


Might want to add a comment here why the signal is not directly connected (e.g. because QTimer::timeout is a private signal)

Thanks! Updated.

Actually, the private QTimer::timeout signal has one QTimer::QPrivateSignal parameter.

Also the parameter list of the TransactionView::bumpFee slot is made compatible with one of the QAction::triggered signal.

Also, uic automatic connection replaced with an explicit one.

hebasto · 2021-04-05T13:52:05Z

Updated f81672e -> b8e5d0d (pr260.11 -> pr260.12, diff):

addressed @laanwj's comment:

Might want to add a comment here why the signal is not directly connected (e.g. because QTimer::timeout is a private signal)

ryanofsky

Code review ACK b8e5d0d. This is great! I think more improvements are possible but implementation is very clean and I love how targeted each commit is. Changes since last review: adding more explanatory text, making links clickable, reorganizing.

ryanofsky · 2021-04-07T19:17:57Z

src/qt/bitcoin.cpp

+    QMessageBox::critical(
+        nullptr, tr("Runaway exception"),
+        tr("A fatal error occurred. %1 can no longer continue safely and will quit.").arg(PACKAGE_NAME) %
+        QLatin1String("<br><br>") % GUIUtil::MakeHtmlLink(message, PACKAGE_BUGREPORT));


In commit "qt: Make PACKAGE_BUGREPORT link clickable" (af7e365)

Since our project only uses the issue tracker for development, not user support, it would be good in the future to link to something like stack exchange or a support resources page if a fatal error occurs, instead of the issue tracker.

But this commit is only making existing issue tracker links clickable, not adding new links, so it doesn't cause problems and is good because it keeps the links rendered in consistent way.

In the future it might be be good stop having "Runaway exception" dialogs and instead have distinct "Internal bug" (e.g. unhandled preconditions or postconditions) and "Fatal error" (e.g. failing disk writes) dialogs, treating the first kind like bugs and the second kind like support issues. But it seems good to keep the scope of this PR narrow and focused on the nonfatal checks like it is now.

ryanofsky · 2021-04-07T19:26:49Z

src/qt/guiutil.cpp

+    const QObject* sender,
+    const QObject* receiver)
+{
+    std::string description = sender->metaObject()->className();


In commit "qt: Add GUIUtil::ExceptionSafeConnect function" (f7e260a)

Not important, but just in case you revisit this, I think it might be a little clearer to call this variable "location" instead of "description". (I was a little confused at first about why it didn't seem to include information from the exception.)

hebasto · 2021-04-13T11:23:41Z

@MarcoFalke

Do you mind looking into this PR as it resolves your bitcoin/bitcoin#18643?

laanwj · 2021-04-14T10:36:05Z

Code review ACK b8e5d0d

This was referenced Mar 26, 2021

qt: Handle exceptions instead of crash bitcoin/bitcoin#18897

Closed

The gui doesn't handle exceptions very well #91

Closed

hebasto added the Bug Something isn't working label Mar 26, 2021

hebasto force-pushed the 210326-ex branch 2 times, most recently from dee52c2 to 63cca51 Compare March 27, 2021 18:21

hebasto marked this pull request as ready for review March 27, 2021 19:43

hebasto closed this Mar 27, 2021

hebasto reopened this Mar 27, 2021

leonardojobim approved these changes Mar 28, 2021

View reviewed changes

ryanofsky approved these changes Mar 31, 2021

View reviewed changes

src/qt/guiutil.h Show resolved Hide resolved

src/qt/bitcoin.cpp Outdated Show resolved Hide resolved

hebasto and others added 4 commits April 1, 2021 03:05

qt: Make PACKAGE_BUGREPORT link clickable

af7e365

Co-authored-by: Russell Yanofsky <russ@yanofsky.org>

qt: Add BitcoinApplication::handleNonFatalException function

64a8755

This helper function will be used in the following commits. Co-authored-by: Russell Yanofsky <russ@yanofsky.org>

qt: Handle exceptions in BitcoinGUI::addWallet slot

eb6156b

hebasto force-pushed the 210326-ex branch from 63cca51 to f81672e Compare April 1, 2021 00:07

laanwj reviewed Apr 5, 2021

View reviewed changes

hebasto added 3 commits April 5, 2021 16:47

qt: Handle exceptions in WalletModel::pollBalanceChanged slot

bc00e13

Actually, the private QTimer::timeout signal has one QTimer::QPrivateSignal parameter.

qt: Handle exceptions in TransactionView::bumpFee slot

1ac2bc7

Also the parameter list of the TransactionView::bumpFee slot is made compatible with one of the QAction::triggered signal.

qt: Handle exceptions in SendCoinsDialog::sendButtonClicked slot

b8e5d0d

Also, uic automatic connection replaced with an explicit one.

hebasto force-pushed the 210326-ex branch from f81672e to b8e5d0d Compare April 5, 2021 13:48

ryanofsky approved these changes Apr 7, 2021

View reviewed changes

laanwj merged commit 03eccee into bitcoin-core:master Apr 14, 2021

hebasto deleted the 210326-ex branch April 14, 2021 13:07

This was referenced Apr 14, 2021

Revamp context menus #263

Merged

Add "Copy address" item to the context menu in the Peers table #264

Closed

luke-jr mentioned this pull request Jun 22, 2022

[21.x] Qt: Handle exceptions instead of crash bitcoin/bitcoin#25449

Closed

bitcoin-core locked as resolved and limited conversation to collaborators Aug 16, 2022

Handle exceptions instead of crash #260

Handle exceptions instead of crash #260

Uh oh!

Conversation

hebasto commented Mar 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hebasto commented Mar 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hebasto commented Mar 27, 2021

Note to reviewers

Uh oh!

hebasto commented Mar 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hebasto commented Mar 27, 2021

Uh oh!

leonardojobim 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!

laanwj commented Mar 29, 2021

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ryanofsky commented Mar 31, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hebasto commented Apr 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

laanwj Apr 5, 2021

Choose a reason for hiding this comment

Uh oh!

hebasto Apr 5, 2021

Choose a reason for hiding this comment

Uh oh!

hebasto commented Apr 5, 2021

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

ryanofsky Apr 7, 2021

Choose a reason for hiding this comment

Uh oh!

ryanofsky Apr 7, 2021

Choose a reason for hiding this comment

Uh oh!

hebasto commented Apr 13, 2021

Uh oh!

laanwj commented Apr 14, 2021

Uh oh!

Uh oh!

hebasto commented Mar 26, 2021 •

edited

Loading

hebasto commented Mar 26, 2021 •

edited

Loading

hebasto commented Mar 27, 2021 •

edited

Loading

leonardojobim left a comment •

edited

Loading

ryanofsky commented Mar 31, 2021 •

edited

Loading

hebasto commented Apr 1, 2021 •

edited

Loading