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

luke-jr commented at 5:30 PM on June 22, 2022: member

This is a backport of https://github.com/bitcoin-core/gui/pull/260 to 21.x / C++11.

Since 21.x is now Maintenance End for Core, and the fix herein is mostly UX-related in practice, it's unclear if it makes sense to merge this PR, but even if not, I would appreciate any review since I intend to include it in Knots LTS.

For the sake of easier review, the C++11 backport is an additional unsquashed commit.

qt: Make PACKAGE_BUGREPORT link clickable

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

Github-Pull: gui#260
Rebased-From: af7e365b1516d660d271475fdfe0c20ae09e66a8

18d528f2bb

qt: Add BitcoinApplication::handleNonFatalException function

This helper function will be used in the following commits.

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

Github-Pull: gui#260
Rebased-From: 64a8755af396f1c2791018510e22b58114e68594

7c94b5b3db

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.

Github-Pull: gui#260
Rebased-From: f7e260a471010e2d656fbc5ea8c310f6d94c26b9

5556913a8c

qt: Handle exceptions in BitcoinGUI::addWallet slot

Github-Pull: gui#260
Rebased-From: eb6156ba1b4c303eb597e3fc4a9e42ce45e6e78d

d2a4ba2f4d

qt: Handle exceptions in WalletModel::pollBalanceChanged slot

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

Github-Pull: gui#260
Rebased-From: bc00e13bc800863641b3e1e64732a38418d3022f

0a299f4afe

DrahtBot added the label Backport on Jun 22, 2022

fanquake commented at 10:56 AM on July 4, 2022: member

@hebasto any thoughts here?

hebasto commented at 12:51 PM on July 4, 2022: member

Concept ACK on backporting, if other backports (like #25538) will be applied.

... I would appreciate any review...

Will do shortly.

hebasto commented at 2:53 PM on July 4, 2022: member

ACK 6a794f4737eef9a13a3963f87bd2f37916d11f32, I've verified that code has been backported correctly.

Do we really need to introduce #include <QStringBuilder> into the 21.x branch? Maybe combine these changes with a02c970eb001b456d74ddc30750fe8b55348ddac?

luke-jr commented at 9:10 PM on July 5, 2022: member

Do we really need to introduce #include <QStringBuilder> into the 21.x branch? Maybe combine these changes with a02c970?

An optimisation isn't a bugfix, but I guess there's no harm in using trivially slower code for normally-unused string building.

Backported just that commit from https://github.com/bitcoin-core/gui/pull/313

hebasto approved

hebasto commented at 11:07 PM on July 5, 2022: member

re-ACK 87028acd2f785b0f172b7a934acce0a2082ebca0

luke-jr force-pushed on Jul 6, 2022

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.

Github-Pull: gui#260
Rebased-From: 1ac2bc7ac070dfd1df1872d759540b0c92495301

17b4010aa8

qt: Handle exceptions in SendCoinsDialog::sendButtonClicked slot

Also, uic automatic connection replaced with an explicit one.

Github-Pull: gui#260
Rebased-From: b8e5d0d3fe3386807d47f50d13ac34fcd2a538fd

18a4f83188

qt: Backport GUIUtil::ExceptionSafeConnect to C++11 09d5d03f2e

qt, refactor: Revert explicit including QStringBuilder

Github-Pull: gui#313
Rebased-From: a02c970eb001b456d74ddc30750fe8b55348ddac

3609f2d1b0

luke-jr force-pushed on Jul 24, 2022

luke-jr commented at 10:05 PM on July 24, 2022: member

Had to replace [[maybe_unused]]s with (void)var; for C++11 compatibility

hebasto approved

hebasto commented at 4:38 PM on July 25, 2022: member

re-ACK 3609f2d1b0d6eb64da974f3e1ce3b8f26cd747dc

FWIW, while the maybe_unused attribute indeed is a C++17 feature, I was able to successfully compile f735ab016b96a273b364edb6c7940c54275c36ae with both gcc 8.4.0 and clang 8.0.0 on Ubuntu 18.04.6 LTS.

fanquake commented at 1:58 PM on October 3, 2022: member

Since 21.x is now Maintenance End for Core, and the fix herein is mostly UX-related in practice, it's unclear if it makes sense to merge this PR

21.x is now also EOL. I think we can close this rather than merging a gui backport into an EOL branch.

fanquake closed this on Oct 3, 2022

bitcoin locked this on Oct 3, 2023