Fix and modify alerts system as per DexX's feedback

[zathras-crypto]

@DexX7 thanks for all the suggestions, have taken in several as part of this PR. Thoughts?

[zathras-crypto]

@DexX7 re your commit 8a01b8f816addf0eccd7ad6436e1dbf61b998941 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 https://github.com/mastercoin-MSC/mastercore/pull/244#issuecomment-66979385)?

Thanks Z

[dexX7]

Nice, I'm going to test this later. Especially having a dedicated function such as parseAlertMessage is great imho.

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: https://bitcointalk.org/index.php?topic=203438.msg2134545#msg2134545

When using Alert::Notify("msg", false) it is passed through, otherwise the call is wrapped into boost::thread t(runCommand, strCmd).

What the consequences are in this particular case and how to guard against unsafe use is something I can't answer at the moment.

[dexX7]

@zathras-crypto: a few questions:

1) You mentioned there can only be one alert. Is that correct? What happens, if there are two? The old one is overwritten?

2) Please explain very briefly the impact of the MC alert system on Bitcoin's alert system. I'm asking in particular in the context of Q1.

3) You commented:

Mastercore alerts come as block transactions and do not trip bitcoin alertsChanged() signal so let's check the alert status with the update balance signal that comes in after each block to see if it had any alerts in it

Maybe I misunderstand, but I was wondering, how to raise an update as well, and came up with a new signal: https://github.com/dexX7/bitcoin/commit/03d16c9f99934f9d7b361f16b4b891f6f55cc327

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:

1. parse new block
2. parse transaction
3. identify alert transaction type
4. parse alert
5. store alert
6. process alert and..
7. notify client (ui, console)
8. shutdown, if necessary

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.. :)

[dexX7]

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 CAlert::Notify is part of CMPTransaction::step2_Alert.

Am I correct that this may trigger a bunch of alert notifications during each startup?

[zathras-crypto]

Hey bud,

1) You mentioned there can only be one alert. Is that correct? What happens, if there are two? The old one is overwritten?

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).

2) Please explain very briefly the impact of the MC alert system on Bitcoin's alert system. I'm asking in particular in the context of Q1.

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.

3) You commented: Mastercore alerts come as blo....

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:

• Bitcoin alert received - show immediately as this is a P2P message received directly on the stack and not a bitcoin transaction that goes into mempool
• Our alert received - do not show immediately as we do not currently process unconfirmed transactions. Once it is confirmed and processed by the block handler, block handler triggers an update to the UI which includes picking up the new alert (or clearing an expired one).

An alert is currently not stored as alert, but during startup historical transactions are searched for an alert. 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 CAlert::Notify is part of CMPTransaction::step2_Alert. Am I correct that this may trigger a bunch of alert notifications during each startup?

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. Now that does open an interesting question - should we be firing whatever cmd set by -alertnotify at startup even once on reload of an existing alert?

Thanks bud Z

[zathras-crypto]

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 :)

[dexX7]

Now that does open an interesting question - should we be firing whatever cmd set by -alertnotify at startup even once on reload of an existing alert?

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 gettransaction_MP "alert-tx". I suggest to move the notification into case 4 of checkExpiredAlerts.

[zathras-crypto]

Another case: I believe an -alertnotify would also be triggered, if someone uses gettransaction_MP "alert-tx".

Good point, let me look at that, thanks

[zathras-crypto]

Another case: I believe an -alertnotify would also be triggered, if someone uses gettransaction_MP "alert-tx". I suggest to move the notification into case 4 of checkExpiredAlerts.

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 gettransactionMP 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 Z

[dexX7]

Moving to case 4 of checkExpiredAlerts would result in alertnotify only being triggered for type 4 ...

Yeah, it would be fired for the most important type. And...

What are we looking at on this PR left to test ...

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 https://github.com/mastercoin-MSC/mastercore/pull/244#issuecomment-66979385 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.

---

FYI gettransaction_MP calls step1, and then drops into ...

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.

[zathras-crypto]

@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 Z

[m21]

Done Zathras. Sorry on the road now, was awaiting your reply to Dexx or blessing otherwise.

[zathras-crypto]

No worries mate all good - we'd do further work before merging if we had the time, but current state as merged will at least get the job done... Safe trip home mate :) Z On 09/01/2015 7:56 AM, "Michael" notifications@github.com wrote:

Done Zathras. Sorry on the road now, was awaiting your reply to Dexx or blessing otherwise.

— Reply to this email directly or view it on GitHub https://github.com/mastercoin-MSC/mastercore/pull/246#issuecomment-69247697 .

[dexX7]

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 system client.