Handle exceptions instead of crash

hebasto commented at 6:14 pm on March 26, 2021: member

This PR is an alternative to https://github.com/bitcoin/bitcoin/pull/18897, and is based on Russ’ idea:

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

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

display it to a user:

Screenshot from 2021-04-01 02-55-59

prints a message to stderr:

0
1
2************************
3EXCEPTION: 18NonFatalCheckError       
4wallet/wallet.cpp:2677 (IsCurrentForAntiFeeSniping)
5Internal bug detected: '!chain.findBlock(block_hash, FoundBlock().time(block_time))'
6You may report this issue here: https://github.com/bitcoin/bitcoin/issues
7       
8bitcoin 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 commented at 6:16 pm on March 26, 2021: member

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

hebasto added the label Bug on Mar 26, 2021

hebasto force-pushed on Mar 27, 2021

hebasto commented at 7:26 pm on March 27, 2021: member

Note to reviewers

To test this PR you could:

apply this patch

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

and initiate a new transaction creation, or

apply this patch

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

and initiate opening a wallet.

hebasto commented at 7:38 pm on March 27, 2021: member

@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 marked this as ready for review on Mar 27, 2021

hebasto commented at 7:45 pm on March 27, 2021: member

@promag @ryanofsky @jonasschnelli

Kindly asking for reviewing :)

:tiger2:

hebasto closed this on Mar 27, 2021

hebasto reopened this on Mar 27, 2021

leonardojobim approved

leonardojobim commented at 10:44 pm on March 28, 2021: none

Tested ACK https://github.com/bitcoin-core/gui/pull/260/commits/63cca51a9956b4333237416833722d93df43c9ff on Ubuntu 20.04 Qt Qt 5.12.8.

On master branch:

On this PR branch:

The exception was also displayed on terminal:

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

laanwj commented at 12:02 pm on March 29, 2021: member

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.

in src/qt/guiutil.h:372 in 60a9940c1e outdated

362+                        blockingGUIThreadConnection(),
363+                        Q_ARG(QString, QString::fromStdString(e.what())));
364+                } catch (const std::exception& e) {
365+                    PrintSlotException(&e, sender, receiver);
366+                    ok = QMetaObject::invokeMethod(
367+                        qApp, "handleRunawayException",

ryanofsky commented at 8:22 pm on March 31, 2021:

In commit “qt: Add GUIUtil::ExceptionSafeConnect function” (60a9940c1e47f75aa539b31b530433b7de794146)

It seems like PrintExceptionContinue will be called twice here because PrintSlotException calls it and handleRunawayException also calls it. Same applies to catch(…) below.

hebasto commented at 11:23 pm on March 31, 2021:

PrintSlotException invokes BitcoinApplication::handleRunawayException, not BitcoinCore::handleRunawayException.

Therefore, no calling PrintExceptionContinue twice, right?

ryanofsky commented at 0:04 am on April 1, 2021:

re: #260 (review)

Thanks, you’re right. I didn’t realize these methods had the same name. If I am confirmed as Official Namer Of Stuff I will decree that gui code must use different names for things that are not the same. Might start with addNewRequest’s first, though.

in src/qt/bitcoin.cpp:427 in 60a9940c1e outdated

420@@ -421,6 +421,12 @@ void BitcoinApplication::handleRunawayException(const QString &message)
421     ::exit(EXIT_FAILURE);
422 }
423 
424+void BitcoinApplication::handleNonFatalException(const QString& exception_message)
425+{
426+    assert(QThread::currentThread() == qApp->thread());
427+    QMessageBox::warning(nullptr, "Internal error", exception_message);

ryanofsky commented at 8:56 pm on March 31, 2021:

In commit “qt: Add GUIUtil::ExceptionSafeConnect function” (60a9940c1e47f75aa539b31b530433b7de794146)

I agree with laanwj #260 (comment) that a warning dialog beginning with “wallet/wallet.cpp:2678 (IsCurrentForAntiFeeSniping)…” #260#pullrequestreview-622791979 is not friendly and would be difficult to understand. Could add some descriptive text and replace the URL with a link:

0QMessageBox::warning(nullptr, "Internal error",
1    BitcoinGUI::tr("An internal error occurred. %1 will attempt to continue safely. This is "
2                   "an unexpected bug which can be reported as described below.")
3    .arg(PACKAGE_NAME) + "<br><br>" + QString(exception_message)
4    .replace(PACKAGE_BUGREPORT, QString("<a href=\"") + PACKAGE_BUGREPORT + "\">" +
5             PACKAGE_BUGREPORT + "</a>"));

hebasto commented at 0:12 am on April 1, 2021:

Thanks! Done.

ryanofsky approved

ryanofsky commented at 9:12 pm on March 31, 2021: contributor

Code review ACK 63cca51a9956b4333237416833722d93df43c9ff. 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.

ryanofsky commented at 9:14 pm on March 31, 2021: contributor

re: #260 (comment)

Is it logged to the debug log?

Yep, PrintExceptionContinue logs it.

qt: Make PACKAGE_BUGREPORT link clickable

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

af7e365b15

qt: Add BitcoinApplication::handleNonFatalException function

This helper function will be used in the following commits.

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

64a8755af3

qt: Add GUIUtil::ExceptionSafeConnect function

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.

f7e260a471

qt: Handle exceptions in BitcoinGUI::addWallet slot eb6156ba1b

hebasto force-pushed on Apr 1, 2021

hebasto commented at 0:11 am on April 1, 2021: member

@laanwj @ryanofsky

Thank you for your reviews and suggestions!

Updated 63cca51a9956b4333237416833722d93df43c9ff -> f81672e65b443c00a650b2d920996ea738fa2d7f (pr260.10 -> pr260.11, diff):

addressed reviewers’ comments
small improvements

qt: Handle exceptions in WalletModel::pollBalanceChanged slot

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

bc00e13bc8

qt: Handle exceptions in TransactionView::bumpFee slot

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

1ac2bc7ac0

qt: Handle exceptions in SendCoinsDialog::sendButtonClicked slot

Also, uic automatic connection replaced with an explicit one.

b8e5d0d3fe

in src/qt/walletmodel.cpp:70 in 07d60c3c63 outdated

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

laanwj commented at 12:38 pm on April 5, 2021:

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

hebasto commented at 1:52 pm on April 5, 2021:

Thanks! Updated.

hebasto force-pushed on Apr 5, 2021

hebasto commented at 1:52 pm on April 5, 2021: member

Updated f81672e65b443c00a650b2d920996ea738fa2d7f -> b8e5d0d3fe3386807d47f50d13ac34fcd2a538fd (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)

in src/qt/bitcoin.cpp:425 in af7e365b15 outdated

418@@ -417,7 +419,10 @@ void BitcoinApplication::shutdownResult()
419 
420 void BitcoinApplication::handleRunawayException(const QString &message)
421 {
422-    QMessageBox::critical(nullptr, "Runaway exception", BitcoinGUI::tr("A fatal error occurred. %1 can no longer continue safely and will quit.").arg(PACKAGE_NAME) + QString("<br><br>") + message);
423+    QMessageBox::critical(
424+        nullptr, tr("Runaway exception"),
425+        tr("A fatal error occurred. %1 can no longer continue safely and will quit.").arg(PACKAGE_NAME) %
426+        QLatin1String("<br><br>") % GUIUtil::MakeHtmlLink(message, PACKAGE_BUGREPORT));

ryanofsky commented at 7:17 pm on April 7, 2021:

In commit “qt: Make PACKAGE_BUGREPORT link clickable” (af7e365b1516d660d271475fdfe0c20ae09e66a8)

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.

in src/qt/guiutil.cpp:910 in f7e260a471 outdated

901@@ -902,4 +902,15 @@ QString MakeHtmlLink(const QString& source, const QString& link)
902         QLatin1String("<a href=\"") % link % QLatin1String("\">") % link % QLatin1String("</a>"));
903 }
904 
905+void PrintSlotException(
906+    const std::exception* exception,
907+    const QObject* sender,
908+    const QObject* receiver)
909+{
910+    std::string description = sender->metaObject()->className();

ryanofsky commented at 7:26 pm on April 7, 2021:

In commit “qt: Add GUIUtil::ExceptionSafeConnect function” (f7e260a471010e2d656fbc5ea8c310f6d94c26b9)

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

ryanofsky approved

ryanofsky commented at 7:31 pm on April 7, 2021: contributor

Code review ACK b8e5d0d3fe3386807d47f50d13ac34fcd2a538fd. 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.

hebasto commented at 11:23 am on April 13, 2021: member

@MarcoFalke

Do you mind looking into this PR as it resolves your https://github.com/bitcoin/bitcoin/issues/18643?

laanwj commented at 10:36 am on April 14, 2021: member

Code review ACK b8e5d0d3fe3386807d47f50d13ac34fcd2a538fd

laanwj merged this on Apr 14, 2021

laanwj closed this on Apr 14, 2021

hebasto deleted the branch on Apr 14, 2021

bitcoin-core locked this on Aug 16, 2022

Handle exceptions instead of crash #260

Note to reviewers