-
Notifications
You must be signed in to change notification settings - Fork 11
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
The alert message thread #242
Comments
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. |
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): bitcoin#5356 Anyway, I pushed my code to the public public repo as well: https://github.com/dexX7/bitcoin/commits/mcore-alerts-with-safemode
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 |
@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. |
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.
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 |
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. |
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:
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:
Opinions? Also ping @m21, @faizkhan00, @dacoinminster, @achamely, @marv-engine, @DavidJohnstonCEO. (sorry for the multi mentions in before..) |
Hey DexX,
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 |
Oh sorry, also FYI:
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) |
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. |
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. |
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:
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. |
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 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); |
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:
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?The text was updated successfully, but these errors were encountered: