Refactor AlertNotifyOnce out of UpdateTip #7810

pull paveljanik wants to merge 3 commits into bitcoin:master from paveljanik:20160405_AlertNotifyOnce changing 1 files +19 −20

paveljanik commented at 7:44 AM on April 5, 2016: contributor

This simplifies UpdateTip() a bit.
in src/main.cpp:None in 9093c9f9d8 outdated
```
1589 | @@ -1590,6 +1590,18 @@ static void AlertNotify(const std::string& strMessage, bool fThread)
1590 |          runCommand(strCmd);
1591 |  }
1592 |  
1593 | +static void AlertNotifyOnce(const std::string& strMessage, bool fThread)
```
MarcoFalke commented at 10:18 AM on April 5, 2016:

If you are hardcoding this to true, there is no reason to keep it in the args. Am I missing something?

paveljanik commented at 4:44 PM on April 5, 2016:

This was a mistake, fixed.

First I though about removing the fThread argument completely (because it really is true everywhere, but then decided to keep this small). What do you think?

laanwj commented at 12:26 PM on April 25, 2016:

If we don't use it anywhere, remove the argument please.

paveljanik commented at 2:52 PM on April 25, 2016:

Done in [squashme] commit.
laanwj added the label Refactoring on Apr 5, 2016
Refactor AlertNotifyOnce out of UpdateTip 356e175372
paveljanik force-pushed on Apr 5, 2016
[squashme] Remove always-true argument. 2540dc408a
[squashme] Set strMiscWarning directly from AlertNotify 84171383bc
in src/main.cpp:None in 356e175372 outdated
```
1594 | +{
1595 | +    static bool fWarned = false;
1596 | +
1597 | +    if (!fWarned) {
1598 | +      // strMiscWarning is read by GetWarnings(), called by Qt and the JSON-RPC code to warn the user
1599 | +      strMiscWarning = strMessage;
```
laanwj commented at 12:26 PM on April 25, 2016:

What about moving the setting of strMiscWarning into AlertNotify? All uses of AlertNotify set strMiscWarning except one in CheckForkWarningConditions, and I think that's a mistake.

paveljanik commented at 3:01 PM on April 25, 2016:

Done in [squashme] commit.

in src/main.cpp:None in 84171383bc

2581 | @@ -2574,11 +2582,7 @@ void static UpdateTip(CBlockIndex *pindexNew) {
2582 |              ThresholdState state = checker.GetStateFor(pindex, chainParams.GetConsensus(), warningcache[bit]);
2583 |              if (state == THRESHOLD_ACTIVE || state == THRESHOLD_LOCKED_IN) {
2584 |                  if (state == THRESHOLD_ACTIVE) {
2585 | -                    strMiscWarning = strprintf(_("Warning: unknown new rules activated (versionbit %i)"), bit);
2586 | -                    if (!fWarned) {
2587 | -                        AlertNotify(strMiscWarning, true);
2588 | -                        fWarned = true;
2589 | -                    }
2590 | +                    AlertNotifyOnce(strprintf(_("Warning: unknown new rules activated (versionbit %i)"), bit));

paveljanik commented at 3:55 PM on April 25, 2016:

Reading this again, I think this one should not be AlertNotifyOnce, but AlertNotify. We can have several parallel version bits activation notified...

paveljanik commented at 4:04 PM on April 25, 2016: contributor

Hmm, I think there should be no AlertNotifyOnce at all. We should warn always (there is one exception - once per day alert for network partition - implemented already), the user will see the last warning only. If he wants to see previous warnings, he can grep the log and/or use -alertnotify.

Alternatively, we can store some number of previous warnings and provide them via some RPC/GUI.

AlertNotifyOnce for one version bit activation should not hide warnings about new version blocks being mined IMO.

Opinions?
laanwj commented at 10:54 AM on April 27, 2016: member

I do think it'd be better to get rid of the AlertNotifyOnce function now, it's so trivial, may as well go back to putting the code where it's needed. And the boolean really belongs in UpdateTip, as it relates to this specific case.

I disagree with removing that logic completely - the idea is to warn once, and at most once when the rules changed in a (for this implementation) unknown way. There is no use in spamming warnings for every block.
paveljanik commented at 4:01 PM on April 27, 2016: contributor

OK, so I'll PR the removal of 'true' as separate PR and closing this.
paveljanik closed this on Apr 27, 2016
laanwj commented at 9:55 AM on April 28, 2016: member

OK, so I'll PR the removal of 'true' as separate PR and closing this.

Moving the setting of strMisc into AlertNotify was also a good thing!
paveljanik commented at 8:08 PM on April 28, 2016: contributor

@laanwj I'll do it after #7958 is merged.
laanwj commented at 12:17 PM on May 4, 2016: member

Agreed. I don't think this is so simple after all, we have some further problems here regarding translation (see #7995).
DrahtBot locked this on Sep 8, 2021

Contributors

Labels