< MarcoFalke>
But it seems you forgot to remove parameter interaction from init?
< morcos>
sipa: ping
< morcos>
sipa: I'm just trying to reason through whether your new warning logic is sufficient or not for BIP 9.
< morcos>
It's unclear what should be fixed in this PR and what should be fixed in a separate PR which cleans up alert logic (for instance making sure the RPC getinfo returns the warnings probably belongs there)
< morcos>
But part of what concerns me about your PR is the if (!fWarned) guard, it seems like its too easy to accidentally miss whatever warning has happened, especially due to the buggy alert logic where strMiscWarning could be overwritten by too many blocks than expected or something silly
< morcos>
What do you think it is that we are trying to avoid happening more than once with that guard? The Notify? perhaps we should just guard that.
< morcos>
I'd like to make sure that before we release this code we have something in place such that 0.12.1 code with this released won't accidentally miss future upgrades of either known or unknown type if the user isn't paying very close attention.
< morcos>
I'm not sure we're quite there yet.
< sipa>
morcos: i tried to leave as much of the way warnings are issued untouched
< morcos>
sipa: yeah, but i think the fact that the elegant way your warnings checker doesn't need to run during IBD (which i'd missed before) is a bit hampered by that !fWarned guard.
< morcos>
which might be a problem with the existing code honestly
< sipa>
morcos: and i missed that that fWarned flag was a static, so i had removed it in an earlier version
< sipa>
i think the whole warning system is broken (not visible enough, no way to have multiple parallel warnings, not having transient warnings, ...)
< sipa>
but i don't feel that should be fixed in this PR
< sipa>
maybe it does need fixing separately before we release any bip9 code
< morcos>
sipa: yeah i was just looking at that
< morcos>
thats what i think
< morcos>
i don't think it has to be that hard
< morcos>
but i'm just a bit hesitant b/c i'm not familiar with the statusBar/GUI stuff
< morcos>
but for the RPC warnings, it seems we could just concatenate any active warnings
< morcos>
but i think its worth fixing at least to some degree before release
< morcos>
but if you dont' want to do that, i'd say an alternative
< sipa>
i think you want warnings to be a set of strings, that is constructed by merging the warning sets computed by different subsystems
< morcos>
is to put the !fWarned guard on Notify and strMiscWarning, but keep logprintf's happening period
< sipa>
each subsystem is responsible for caching its own warnings for as long as they are applicable
< morcos>
so then at the very least the debug log will be constantly telling you if your version is out of date
< morcos>
sipa: yes agree
< morcos>
the problem is do we want to back port all those changes to .11 and .12?
< morcos>
It seems like its the case now that anybody running .11.2 or .12.0 might miss warnings of the upgrade to BIP9 right? they'll have that warning set once, but then it might be wiped out by one of the frivolous warnings
< morcos>
brb
< morcos>
sipa: heh, shows how much i know. that strRPC is only for determining whether safeMode is required for RPC commands. strStatusBar is what is returned by RPC commands, so that does get updated.
< morcos>
but we still have the problem of transient warnings
< dgenr8>
morcos: is it silly to warn the user if some miner breaks sha256 and starts issuing a block per 10s?
< morcos>
dgenr8: i called them silly because they happen all the time as false positives now, not that the idea is silly
< morcos>
although maybe that just got fixed?
< dgenr8>
oh ok. i think #7568 fixes it
< morcos>
sipa: i think we should just do the quick and dirty fix for backports. which in my mind is only use the !fWarned guard on calling Notify.
< morcos>
This means that unknown block versions will always overwrite partition checks
< morcos>
But LargeWorkForks will overwrite either of those
< morcos>
Then we can clean up the Alert system for 0.13 properly, but right now there is no locking around any of these variables used in alerts and it seems a bit invasive to do it properly