Fix and modify alerts system as per DexX's feedback #246
Conversation
|
@dexX7 thanks for all the suggestions, have taken in several as part of this PR. Thoughts? |
|
@dexX7 re your commit 8a01b8f and comments on pull 244 it sounds like you are saying spawning a new thread for the notify is not always viable? If change to false as per your commit what is the behavior? Is the command executed for alertnotify run from the main thread? Does the thread pause until this child process returns? If so I see that as problematic. I'm interested in the feedback about being called before fully init, @m21 thoughts (see #244 (comment))? Thanks |
|
Nice, I'm going to test this later. Especially having a dedicated function such as So regarding notification behavior: a user can pass a string to -alertnotify=, similar to -blocknotify and -walletnotify. The underlying function which is called by all three is int system(const char *command) of stdlib.h. Here is an example of how it could be used: When using What the consequences are in this particular case and how to guard against unsafe use is something I can't answer at the moment. |
|
@zathras-crypto: a few questions:
Maybe I misunderstand, but I was wondering, how to raise an update as well, and came up with a new signal: dexX7@03d16c9 It can be used this way to set a message and update or clear the status bar in the UI: std::string strAlertMessage = "This is a new alert!";
uiInterface.NotifyStatusBarChanged(strAlertMessage);It's implementation is basically a copy of similar signals. Maybe this helps? So the flow in general is roughly:
An alert is currently not stored as alert, but during startup historical transactions are searched for an alert. The list is not related to a question, summarizing, but please correct me, if I'm wrong.. :) |
|
Ah, forgot something important: Because there could be several historical alerts, they would be processed during startup each time. Currently the alert notification for -alertnotify Am I correct that this may trigger a bunch of alert notifications during each startup? |
|
Hey bud,
That's correct. It's a last_message_wins system by design because at this early stage we need a way to overwrite the previous alert if it's incorrect or issued maliciously or whatever - alerts is still brand new so I haven't built confidence in it and didn't want to rely on alerts we couldn't override at the beginning. Long story short, every new alert replaces any existing alert if one is already active. This is also used to 'clear' an alert (broadcast a new alert that immediately expires).
I tried to do it as a completely independent system. IIRC there is only one re-usage and that's of the bitcoin alert label in the UI (to which we append our alert if there is a pre-existing bitcoin alert), everything else is independent to bitcoin's alert system eg our alert info is in getinfo_MP not getinfo and our alert auth and processing are mastercore not bitcoin core functions.
Yeah so originally I had the function to update the alert info in the UI tied to the alertsChanged() signal, but that signal is only fired when a bitcoin alert comes in, not a mastercore alert (again, different auth and processing functions). I went with a global simplification a little while ago which replaced a timer with a signal from the block handler. TL:DR; every time a block is processed we trigger an update on the UI, I just meant that update now checks for alert changes too. Behaviour should basically thus be as follows:
That's absolutely correct. I had two directions to go in, either creating a new piece to persistently store alert data which is reorg safe and such, or just re-use the existing levelDB txlist where we already wipe transactions higher than chainHeight in a reorg. Just a question of time really, I went with the easier model. So at init, we actually loop through the txlist list looking at alerts (but not parsing them), and pick the most recent alert. Only then do we move onto parsing - so we should only call step2_Alert once, on the most recent alert txid we cherry picked from txlistdb. Thanks bud |
|
P.S. I know this looks exploitable at first glance (an end user broadcasts a more recent alert and that's the one that gets picked on startup and when parsed is not auth'd and dropped so effectively no alert is loaded on startup). FYI only after alerts are auth'd do they get added to txlistdb, unauth'd alerts are dropped on parsing and not written anywhere but log to protect against this. Thanks :) |
Let's say it this way: if the alert is still active, then it should fire by all means - every restart and as often as possible. Another case: I believe an -alertnotify would also be triggered, if someone uses |
Good point, let me look at that, thanks |
Moving to case 4 of checkExpiredAlerts would result in alertnotify only being triggered for type 4 alerts and not 1/2/3. Looked into this some more, gettransaction_MP cannet trigger an alertnotify because there is no case for an alert type to run the step2 parsing. FYI gettransaction_MP calls step1, and then drops into a switch for the type of packet calling the various step2_{type} parsing functions. Without a case for the alert message, no second stage parsing :) What are we looking at on this PR left to test & merge @dexX7 @m21? Thanks |
Yeah, it would be fired for the most important type. And...
I'm +1 for a merge, but in the future I'd welcome to avoid triggering alert notifications for alerts that are no longer active or don't apply for a client at all - and similar. Think for example of the case where an alert transaction appears on-chain, but the user's client is already up-to-date. With -alertnotify within the parsing logic, it's not even known, if the client is up-to-date or not, but still executed. I think #244 (comment) should be addressed rather sooner than later, too. But short term most of this isn't relevant, and let me be a bit extreme: I would even be fine with a hard client crash, as long the message is received by the user and the initial goal thus satisfied. This is based on the assumption that the alert system is going to be used in an exceptional situation and to avoid danger by all means. Anything else is just a question of how far you'd like to go at this point. This is further based on the assumption that there is probably only one alert broadcasted until the next version is published and once an alert was received by an user, the user is going to update the client to the new version. And within the next client version, refinements could be shipped or a smarter logic to handle historical alerts, etc. etc.
Ah, that's nice. My concern: run-time reparsing and reinterpretation is risky to begin with imho, and adding more functionality to the parsing logic only fuels the fire, whether that is that alerts might be triggered when not intended (or luckly not, as in this case), or some other side effects get introduced by accident, for example as happend in the context of #204. |
|
@m21 can we please get this merged ready for 0.0.9? We can still make improvements ongoing sure and DexX has some more good ideas, but for 0.0.9 we need a working alerts system and this PR is required to activate it on mainnet. Thanks dude |
Fix and modify alerts system as per DexX's feedback
|
Done Zathras. Sorry on the road now, was awaiting your reply to Dexx or blessing otherwise. |
|
No worries mate all good - we'd do further work before merging if we had
|
|
I assumed I expressed my support already, but if it wasn't clear: imho this should be a merge. It's obvious, but key holders should take great care, because the alert system literally allows to shutdown the |
No description provided.