spesmilo / electrum

Electrum Bitcoin Wallet
https://electrum.org
MIT License
7.39k stars 3.08k forks source link

qt ElectrumWindow.on_error catches all exceptions before crash reporter #4361

Open SomberNight opened 6 years ago

SomberNight commented 6 years ago

Exceptions in some threads do not get a chance at all to reach the crash reporter. Specifically, see ElectrumWindow.on_error: https://github.com/spesmilo/electrum/blob/b175c6b6094525c34039eecc5babd9d798eb9146/gui/qt/main_window.py#L283-L286

which is e.g. used when signing transactions/messages: https://github.com/spesmilo/electrum/blob/b175c6b6094525c34039eecc5babd9d798eb9146/gui/qt/main_window.py#L342 https://github.com/spesmilo/electrum/blob/b175c6b6094525c34039eecc5babd9d798eb9146/gui/qt/main_window.py#L1581

and for hw wallet-related operations: https://github.com/spesmilo/electrum/blob/b175c6b6094525c34039eecc5babd9d798eb9146/plugins/hw_wallet/qt.py#L208

The hw wallet code often communicates with the user by raising an Exception, which then gets caught here and displayed.


In this reddit thread, the exception the user is getting is presumably a KeyError, which makes it nearly impossible to debug without a trace. We should change something so that this exception has a chance to reach the crash reporter. (note: that user since opened an issue at https://github.com/spesmilo/electrum/issues/4364)

SomberNight commented 6 years ago

One thing we could do is introduce a new Exception subclass, which we would raise all the time except for coding/logical/programming errors/assertion failures. So e.g. if we just want to communicate with the user and we do it via an exception, that would definitely be this type. This would be the base class of almost all our exceptions. We could then branch on the type of the exception in e.g. on_error and perhaps also at other places.

So my current idea would be, roughly, to treat all of the exceptions that are not raised by us and do not get handled to be critical problems that need to be reported, and most of the exceptions we raise not needing to be reported. However I think we should not distinguish based on this for exceptions that already reach the crash reporter; as ideally in a perfect program, nothing should reach the crash reporter.