The alert message thread

[dexX7]

Since it looks like there is soon a new release, I consider a working message system as very pretty high priority. Let's use this thread for some discussion, if needed. I'll start with a few questions:

• What's the ETA for the release?
• Is it fully functional at this point?
• What is missing and should be done?
• @zathras-crypto: do you consider the current code as final?

I have a messy private branch with updated alert code which has, for example, support for the notification with -alertnotify, "color the status bar at run time, not only after startup" and finally, it has the ability to block non-safe RPC calls which might be more gentle than a forced shutdown (albeit this isn't really helpful for -Qt users). Is there anything in particular that justifies extracting some of those features?

[zathras-crypto]

Hey mate,

Release date is set for us - literally next few days with a live STO blockheight + 1 week. Craig is finalizing the release process as we speak (as I have said we need to make sure every integrator updates to 0.0.9 before the STO live block).

It's functional, not sure about fully haha - I'd prefer more testing but what can you do.

The main thing about this release will be STO and the UI so those are the areas that need the most focus I think.

I don't consider the current code as final nope, but I also think we're a long way from what I think we'll be comfortable calling final - we're at such a pace that I think there are bits that aren't as good as they need to be but again bus reqs tend to dictate these things :)

The blocking of non-safe RPC calls still allows balances to be queried and thus if taking the example of an exchange would stop withdrawals but not deposits - if receiving funds was the exploited vector then safe mode doesn't help much. And yep like you say for QT users not going to affect them. As such I prefer to keep a forced shutdown (there is a param to override) in place until we're a bit more stable.

Color status bar, alert notify both sound interesting - as long as we can use them safely without affecting their functionality in bitcoin.

[dexX7]

The blocking of non-safe RPC calls still allows balances to be queried ...

Hehe, actually no: I weren't referring to thread-safe calls, but safemode-safe calls: /src/rpcserver.cpp first row with "okSafeMode". Fire up your client with "-safemode" and try some calls to test. With 0.10.0 the test is also visible in the UI (but queries via QT are unfortunally still possible): https://github.com/bitcoin/bitcoin/pull/5356

Anyway, I pushed my code to the public public repo as well:

https://github.com/dexX7/bitcoin/commits/mcore-alerts-with-safemode

• "Refactor -alertnotify code" was a commit from Gavin
• "Add status bar update signal" is clean
• "Format, use safe mode, fire -alertnotify..." is basically just code format + using alertnotify.. ;)

Core functionallity of course untouched. Maybe this is of use. Edit: this was just some testing and at least the larger commit should not be adopted this way imho, because the mechanism to set and remove an alert isn't optimal.

I actually have a few ideas, but it's way to early for them at this point. You probably remember the dilemma regarding the block hole thread. I believe the notification system can solve it. Even without shutting down completely, we may announce the broad effect (e.g. "tx 7 is a subtracting transaction"), and old clients could still be functional - with a somewhat reduced view. But one step at the time.. ;)

Edit: since -alertnotify is the lowest hanging fruit: it's now a static function and the function body is probably explaining:

CAlert::Notify(const std::string& strMessage, bool fThread);

Can be accessed from anywhere, if alert.h is included.

[dexX7]

Release date is set for us - literally next few days with a live STO blockheight + 1 week.

@CraigSellars @m21 @zathras-crypto you're pretty close to repeating previous mistakes.

Pushing a release "in the next few days" with STO enabled one week later? Please start to announce mandatory client updates much earlier and with great visibility. Since the basic idea for STO is out there for quite some time, wouldn't it have been possible to push an intermediate update without full functionality, but with the starting block number a bit earlier?

And please don't forget: spec#unlocking-features

Also, what about the meta DEx? What's the ETA on this one?

Just to throw it in as potential better-than-worse solution until the notification system is fully functional: maybe there should be a defined block number (now + 1 month for example) after which the client shuts down and requires an update - whether new features are ready or not then. If it's the later, the update may simply carry an updated and new shutdown value.

As an alternative or addition: there could as well be true black hole behavior, even without implemented features. Once block n (as defined) is reached and transactions (of whatever future type) appear on chain, they are treated as balance reducing. In particular the meta DEx transaction type format is well known, so it's no black magic to identify future transactions, but more about finding a suitable n that roughly aligns with the schedule and roadmap.

[zathras-crypto]

Preaching to the choir here mate, don't disagree at all. I've requested @Craig draw up a release process which he's in the process of doing, which we can refine to add structure to the process.

until the notification system is fully functional

I've been focusing on trying to work out some of these bugs but intend to add in the mainnet addresses and run a test there - the alert system should be working afaik

Thanks Z

[dexX7]

I tested it while doing the above mentioned modifications and it worked for me as well, but I have a few points and edge cases to think about more. For example what would happen in the case where there are two overlapping alerts, etc.

If you have some free time, check out mcore-alerts-with-safemode. Imho it would be great, if there were an explicit "setAlertNotification" function which could also be used to fire -alertnotify and to update the status bar. Also some mechanism to identify a change of an alert that isn't triggered every time would likely be helpful. I avoided more than one fire with some global variables here, but it's probably not great.

Some goal I had was to leave the original OverviewPage::updateAlerts() untouched and do this stuff somewhere else. This part wasn't finished, but should probably takes only a few steps.

What's a bit annoying currently is the parsing in many different places over and over again.

[dexX7]

Okay - I was thinking a bit more about this. (@zathras-crypto)

The higher goal is a notification system that may, or may not shutdown clients, one which can fire alerts, check the client's support for specific transactions, which may hide or show notifications based on client version, and.. and.. and..

Let's step back for a moment.

This is all nice, but for the most parts nice to have, but not mandatory. The actual intend is to prevent loss of user coins due to not updated client versions in not very invasive way. The goal can likely be achieved by forcing the user to update or by getting a warning to the user about the potential risk.

Since the release is pretty near, I suggest to following:

1. Forget everything
2. Have one message or transaction type that simply consists of [prefix][message] (where [prefix] might be [version][type] to fit into the current parser and the already available code).
3. Show the message in the status bar, console and via alertnotify right after the message was parsed
4. Repeat step three each time the client is restarts

There is only need to find a solution for one single use, because everything else could as well be part of the next, updated version. Even step four is optional.

Why:

• It's simple. It's so simple that it could be implemented in one evening.
• Getting out a warning is 10000000 times better than nothing.
• It is likely sufficient to fullfil the initial goal, because it's noticed and visible, if the user uses the Qt version and probably noticed, if the user watches console output or subscribed to -alertnotify.
• The "simple" approach can be replaced in the next version.
• I consider it as very high priority to get "some" solution into the release.

Opinions? Also ping @m21, @faizkhan00, @dacoinminster, @achamely, @marv-engine, @DavidJohnstonCEO. (sorry for the multi mentions in before..)

[zathras-crypto]

Hey DexX,

1. Forget everything

While I concur with the essence of your view (that we need something fundamentally simple ASAP) I didn't realize you didn't see the current system as workable; I had seen it as taking in some of your stuff on alertnotify (but modifying to trip on alert receipt rather than on shutdown) but didn't envisage entirely replacing it at this stage in the game.

Your preference for starting anew rather than going with existing is usually an approach levied due to a critical flaw in said previous approach (thus requiring a new approach/way of thinking) - I actually thought the existing system was quite workable; I completely agree with all the statements above about the need for something hence my putting something together originally, I guess I'm just not entirely clear what the problem is with what we have now :)

Thanks Z

[zathras-crypto]

Oh sorry, also FYI:

For example what would happen in the case where there are two overlapping alerts, etc.

The alerts system is a last-message-wins system, only supports one alert message at any time, and any alert can be replaced/superseded simply by broadcasting another from one of the predefined array of approved addresses. This is actually the method for clearing a broadcasted alert (broadcast a new alert with any text, expirytype of blocknum, and expiryvalue < the current block which basically brings in a new alert (which replaces old) and expires it immediately)

[zathras-crypto]

check out mcore-alerts-with-safemode

You have some bits in there I'd like to merge in with mine (alertnotify etc) but will hold off until we've chatted more about the suggestion to replace.

[dexX7]

What is available is awesome, I tested it myself and I didn't meant to get rid of it, but the message is: a change of mindset might have significant implications. (it was the case for me.. :)

Let me ask differently: what is the reason why ithe alert system as it is right now might not become an active part of the next release? If the answer is "complexity and too few testing", then my post should read as " okay, then lets strip and simplify things for now, because it is only necessary to find a solution for one single use". Hope this describes my POV more clearly. :)

Edit: my alert branch is unrelated in this context and in general more meant as collection of some things I played with which are not intended as PR or alike, but I published it, because it might be helpful one way or the other.

[zathras-crypto]

Ahh I see - sure sure... I can say with absolute certainly that alerts in some form will be part of the 0.0.9 release...

It's interesting as I kind of went off the reservation to do this (was supposed to be working other things) but I feel very strongly about the need to be able to inform our users of critical bugs if we're going to release code with such limited testing.

I'm going to make some minor changes based on your feedback and submit a PR to add in the array of allowed addresses to make them live, then I would propose:

1) We compile a binary that only a few select people have (yourself, Faiz, Michael etc) 2) We send an alert, on mainnet (that no one else will see as they don't have alert capable clients) 3) We examine the behaviors, make sure things do what they are supposed to do 4) We send additional alerts, as needed to flesh out any bugs we find 5) Once done, we send a final 'clearing' alert (one that replaces existing alert and immediately expires)

I will test my PR on testnet first of course, but once I think it's ready I'd like for a group of us just to give it a run through.

[dexX7]

Yes, sounds like a good plan. Please go ahead and release the source as soon as possible, so I can get my hands on it before "binary release testing time" (but send me one too, please.. :). Or better: push to a public branch. Because it would be WIP, I won't comment, until you say "now you can". ;)

Responsive behavior during run time would be great. -alertnotify has a role here, but related to the GUI: maybe it's just me and something on Ubuntu, but I only see the alert notice after a client restart. Is this correct behavior?

---

Besides -alertnotify via Alert::Notify and AbortNode("Shutdown Message"), as mentioned above or somewhere else, I found ThreadSafeMessageBox very interesting. It's available via ui_interface.h which should be available globally through main.

For example it can be used to show a small notification box in GUI mode (similar to "..coins received") and it prints to standard output in daemon mode otherwise:

uiInterface.ThreadSafeMessageBox("Message body", "Caption", CClientUIInterface::ICON_WARNING);