Make internal (core) errors show up in the Qt client. #7461

zander commented at 12:57 PM on February 3, 2016: none

Serious warnings wrt to state of the chain were only printed to the logs, with this change these issues are also shown in the Qt Gui..

laanwj added the label GUI on Feb 3, 2016

laanwj added the label Needs backport on Feb 3, 2016

laanwj commented at 1:24 PM on February 3, 2016: member

Concept ACK. My only concern is that these warnings will never go away (no equiv CT_DELETED).

Make internal (core) errors show up in the Qt client. 671b81950b

zander commented at 3:08 PM on February 3, 2016: none

Integrated feedback and made the diff minimal.

in src/alert.cpp:None in 671b81950b outdated

 246 | @@ -245,8 +247,20 @@ bool CAlert::ProcessAlert(const std::vector<unsigned char>& alertKey, bool fThre
 247 |      return true;
 248 |  }
 249 |  
 250 | -void
 251 | -CAlert::Notify(const std::string& strMessage, bool fThread)
 252 | +// static

MarcoFalke commented at 8:15 PM on February 3, 2016:

Nit: What is the meaning of this comment?

zander commented at 8:26 PM on February 3, 2016:

The comment is a habit I have to indicate that the method just below is static. Something that is in the signature in the header, but not possible to have in any other way in the cpp file.

jtimon commented at 8:26 PM on February 3, 2016: contributor

Concept ACK

Scope it

LOCK is a wrapper around boost::recursive_mutex, but it can't hurt
to unlock before we pass it on.

b36b0d573a

in src/alert.h:None in 671b81950b outdated

 101 | @@ -102,7 +102,7 @@ class CAlert : public CUnsignedAlert
 102 |      bool RelayTo(CNode* pnode) const;
 103 |      bool CheckSignature(const std::vector<unsigned char>& alertKey) const;
 104 |      bool ProcessAlert(const std::vector<unsigned char>& alertKey, bool fThread = true); // fThread means run -alertnotify in a free-running thread
 105 | -    static void Notify(const std::string& strMessage, bool fThread);
 106 | +    static void NotifyInternal(const std::string& strMessage);

jtimon commented at 8:47 PM on February 3, 2016:

Wouldn't "NotifyExternal" be a more appropriate name that it is exposed while the static function is only used internally?

zander commented at 9:00 PM on February 3, 2016:

I'm not sure what you mean, the difference between the Notify and the Notify Internal is that the latter also sends a message to the uiClient. Which is internal, not external...

jtimon commented at 9:12 PM on February 3, 2016:

Never mind, somehow I was thinking about it being internal to alert.o...

in src/alert.cpp:None in 671b81950b outdated

  26 | @@ -27,6 +27,8 @@ using namespace std;
  27 |  map<uint256, CAlert> mapAlerts;
  28 |  CCriticalSection cs_mapAlerts;
  29 |  
  30 | +static void Notify(const std::string& strMessage, bool fThread);

jtimon commented at 8:49 PM on February 3, 2016:

This declaration could be saved if the the definition of the newly created method was moved below the definition of the static function.

zander commented at 9:01 PM on February 3, 2016:

Actually, there are various usages of the method Notify throughout the file, not just in the method I added.

jtimon commented at 9:13 PM on February 3, 2016:

You are right, sorry for not making sure that wasn't the case before talking.

in src/alert.cpp:None in 671b81950b outdated

 253 | +void CAlert::NotifyInternal(const std::string& strMessage)
 254 | +{
 255 | +    static CAlert internalAlert;
 256 | +    internalAlert.vchMsg = std::vector<unsigned char>(strMessage.begin(), strMessage.end());
 257 | +    uint256 zero;
 258 | +    LOCK(cs_mapAlerts);

gavinandresen commented at 3:50 PM on February 4, 2016:

It would be better to hold the cs_mapAlerts lock for as little time as possible, so good to add a local scope like this:

{
    LOCK(cs_mapAlerts);
    mapAlerts[zero] = internalAlert;
}

zander commented at 7:21 PM on February 7, 2016: none

To protect users running the GUI (which really means the wallet in most cases) I would like this to end up in the upcoming release.

Can you please make sure it gets included?

laanwj commented at 8:52 AM on February 8, 2016: member

@zander:

Review-wise, it first at least needs a tested ACK.

Whether it can be included in 0.12.0 depends on whether there needs to be another Release Candidate. It looks like rc3 may pretty well be the last one (no new critical issues have come up), if so it will be relabeled as final this week. This means no further code can be included. It will make it into 0.12.1 of course.

Also I've heard some discussion that the threshold for some of the warning messages needs to be bumped (@gmaxwell); they will too easily trigger, especially in the current state of affairs with people playing with forks on the network. That needs to be done in the same release, otherwise adding this to the GUI is going to result in too many false positive complaints.

(so may be blocked on #7488?)

jonasschnelli commented at 9:14 AM on February 22, 2016: contributor

Hmm... not sure about that. Just gave this some testing. In regtest, the nBlocks > BLOCKS_EXPECTED check will very likely be positive, so, this is a good way to test the alert system.

Main problem is probably that the GUI alert system can only display the most recent alert and there is no way of removing the alert.

IMO there should be a GUI alert log and a way of "accepting" (hiding) an alert. Something like OSX Notification Center (or iOS / Android notification mechanism).

zander commented at 9:31 AM on February 22, 2016: none

Main problem is probably that the GUI alert system can only display the most recent alert and there is no way of removing the alert. IMO there should be a GUI alert log and a way of "accepting" (hiding) an alert. Something like OSX Notification Center (or iOS / Android notification mechanism).

I do think that this is a future feature that would make a lot of sense to add. But such an extension to this rather simple, yet essential feature would likely make it unavailable to the 0.12 branch.

I would be happy if you could give the credit for what this patch actually tries to do; which is to bubble up essential networking errors to the UI. Most people running the Qt client will not look at the log file, and so they will miss that they are moved off of the net due to a future hardfork or similar.

The goal is to get this into the first release.

jonasschnelli commented at 9:37 AM on February 22, 2016: contributor

The goal is to get this into the first release.

0.12 final has been tagged (binaries soon). I don't see this as a urgent feature that would allow dispatching another RC round (which is to late anyway now).

IMO adding log-based-alerts to the GUI without a actually "log" (only the most recent one without a timestamp), can make things worse. If we are going to change this, it should also respect time and multiple possible alerts.

zander commented at 9:43 AM on February 22, 2016: none

I don't see this as a urgent feature

Having the GUI show you when your client stopped accepting blocks hours ago (for instance) is not an urgent feature?

I welcome anyone wanting to make this better, however you want, as long as the end user does indeed get informed when his software is too old to be able to participate on the network.

jonasschnelli commented at 9:48 AM on February 22, 2016: contributor

Having the GUI show you when your client stopped accepting blocks hours ago (for instance) is not an urgent feature?

A such feature would be very nice. Although, compared to other issues, this is not a reason add a quick-fix with conceptual problems (map log-alert-system to only-display-most-recent alert) and doing other RC-round or the whole bitcoin-core packaged.

We should wait until we have a proper solution and aim this for 0.13 and (maybe) for a BP to 0.12.1.

zander commented at 9:50 AM on February 22, 2016: none

Well, I made this request 17 days ago, if you can't get it into 0.12.0, then what about 0.12.1 ?

Please write a tested-ack confirming that it works (which you wrote above you did), so it can be merged at least.

jonasschnelli commented at 9:59 AM on February 22, 2016: contributor

Well, I made this request 17 days ago, if you can't get it into 0.12.0, then what about 0.12.1 ?

Please respect the way how Core does do quality assurance. Most PR take much longer than 17 days until they get merged. Time does not matter in this case. We need reviews.

Please write a tested-ack confirming that it works (which you wrote above you did), so it can be merged at least.

I have tested It and I don't agree it would be wise to merge. As said: this maps a log based alert system to a single, non-timestamped alert display. IMO this is not a solution we should merge. Example: If a user faces network problems (close 8333p, etc.), he would see WARNING: check your network connection, %d blocks received in the last %d hours (%d expected) in the GUI, he might resolve the problem then, but the alert would never disappear (unless he restarts BitcoinQt).

At least this PR should display the age (timestamp calculation) of the most recent alert and a way of hiding alerts when the condition are fulfilled (solved network issues, etc.), than I could agree that this is a step towards correct CAlert display in GUI

zander commented at 10:08 AM on February 22, 2016: none

Example: If a user faces network problems (close 8333p, etc.), he would see WARNING: check your network connection, %d blocks received in the last %d hours (%d expected) in the GUI, he might resolve the problem then,

Good enough example, the current situation is that the user never sees this warning. In your example he has applied this patch, sees the problem and can fix it.
Without this patch, he won't fix it...

Again, if its not perfect, then refine it, make it better for a future release. Don't stop real users from seeing real problems in the first available release.

jonasschnelli commented at 10:18 AM on February 22, 2016: contributor

Without this patch, he won't fix it...

There is still the debug.log (there is a "open" button from the debug window).

Again, if its not perfect, then refine it, make it better for a future release. Don't stop real users from seeing real problems in the first available release.

I just try to avoid a fix that introduces new issues.

What if we ...

...would add a timestamp (displays in age in m/h/d) at the top-left?
...Maybe add a (X) close symbol at the top right.
...(And a way to scroll down in multiple alerts are there)

gavinandresen commented at 10:38 AM on February 22, 2016: contributor

Seeing an error/alert is strictly better than not.

Tested ACK from me.

jonasschnelli commented at 1:19 PM on February 22, 2016: contributor

Did some additional testing. If a large chain fork was reported to the GUI, the error gets overwritten by WARNING: check your network connection, 0 blocks received in the last 4 hours (24 expected) after 4 hours. There is a high chance of missing the error (4h time window) and confusing the user with "network troubles".

Working on a alternative solution now.

in src/alert.cpp:None in b36b0d573a

 252 | +// static
 253 | +void CAlert::NotifyInternal(const std::string& strMessage)
 254 | +{
 255 | +    static CAlert internalAlert;
 256 | +    internalAlert.vchMsg = std::vector<unsigned char>(strMessage.begin(), strMessage.end());
 257 | +    uint256 zero;

jonasschnelli commented at 2:06 PM on February 22, 2016:

I guess you need at least a internalAlert.nExpiration time here.

in src/alert.cpp:None in b36b0d573a

 251 | -CAlert::Notify(const std::string& strMessage, bool fThread)
 252 | +// static
 253 | +void CAlert::NotifyInternal(const std::string& strMessage)
 254 | +{
 255 | +    static CAlert internalAlert;
 256 | +    internalAlert.vchMsg = std::vector<unsigned char>(strMessage.begin(), strMessage.end());

jonasschnelli commented at 2:08 PM on February 22, 2016:

IMO you ~~also~~ should set internalAlert.strStatusBar instead. (vchMsg is the raw data of a network alert that comes with a signature).

zander commented at 2:40 PM on February 22, 2016:

Why? There is no change at all in the user experience...

jonasschnelli commented at 2:43 PM on February 22, 2016:

internalAlert.vchMsg will never be shown in your case. What's the reason in instantiating a CAlert? IMO it's completely unused.

in src/alert.cpp:None in b36b0d573a

 255 | +    static CAlert internalAlert;
 256 | +    internalAlert.vchMsg = std::vector<unsigned char>(strMessage.begin(), strMessage.end());
 257 | +    uint256 zero;
 258 | +    {
 259 | +        LOCK(cs_mapAlerts);
 260 | +        mapAlerts[zero] = internalAlert;

jonasschnelli commented at 2:16 PM on February 22, 2016:

Does this includes a risk that the alert gets relayed to other peers?

laanwj commented at 7:26 PM on March 7, 2016: member

Closing in favor of #7579

laanwj closed this on Mar 7, 2016

zander deleted the branch on Apr 26, 2016

MarcoFalke commented at 11:14 PM on June 9, 2016: member

Pull is closed, removing 'Needs backport'

MarcoFalke removed the label Needs backport on Jun 9, 2016

MarcoFalke locked this on Sep 8, 2021