Conversation
|
Concept ACK. |
|
Concept ACK |
|
@jonasschnelli rebased |
|
Concept ACK. I had an earlier try at this with #6260, but tt's good that there is an alternative in the form of a mailing list now - that was pretty much the only concern. |
src/test/alert_tests.cpp
Outdated
|
Concept ACK. |
946e128 to
93f23bc
Compare
|
This code is actually very useful for other projects that build off of bitcoin code base, and could be useful within the context of bitcoin if reconfigured. Perhaps just disable the code, not remove it entirely? |
Sorry, but I don't think that's a valid reason to maintain code that we shouldn't have anymore. And I'm sure there's much better ways of doing this in derived projects as well, which don't rely on one network-wide secret key. |
|
concept ACK |
|
So what has changed between now and several months ago when this was last attempted? Aren't the same reasons for not removing the alerts then still applicable today? |
Just read #6260 and the OP. The alert system suffers from many problems:
(and Satoshi, and possibly others who shouldn't really be able to) A notification mailing list doesn't have any of these problems - it will be about announcements and alerts about this specific software, and we can directly control who has post access. Note that I'm in no way against a 'better' alert system later on, such as one that doesn't rely on a special P2P message. There are some suggestions in #6260. But this one should go, and soon. |
|
A mailing list would work for this specific client, but what about network wide issues like a blockchain fork like the fourth of July fork? Also, since the alert system is network wide, what will be done about other clients that still implement the alerts? |
Network wide issues will also be posted to the mailing list. Also, other software can have their own mailing lists. Decentralization, you know. No one should be trusted with central responsibility to send alerts over the network.
They'll likely remove the code as well. Or not. In any case it will never be triggered again. It was never very useful for other clients, as they couldn't send messages of themselves (see #5160). |
|
@achow101 Please note the alert system was not even used for the "July fork". If Mark Karpeles has the key, how do we know he wasn't forced to hand it over to the Japanese police or that they have obtained it from accessing his computers? At this stage the key should be considered compromised at the very least, but in any case, a network wide, privileged messaging system is pretty outrageous for Bitcoin. It might have been a reasonable compromise in the early days, but we've definitely outgrown the need now. |
|
Concept ACK |
1 similar comment
|
Concept ACK |
|
@btcdrak Wait, it wasn't used in that fork? I thought it was. Anyways, since it looks like there are better alternatives which allow for more decentralization, I agree with removing this. Although, if/when this is merged, all of the other wallet developers should be informed so that they remove the code for processing alerts. Also, maybe the community should be made aware of this decision since this is a protocol rule. I think that if this was merged without letting other people "vote" or debate this, it would probably result in a shitstorm about "The core developers are taking too much power by forcing protocol rules". |
|
Concept ACK |
We're removing our own privileged position from the P2P protocol (note: not consensus) rules. Oh no! Taking so much power. |
Yeah, and people can be irrational and there are also shills and conspiracy theorists trying to find every reason to discredit the Core devs |
|
@achow101 This is not a consensus rule. We are choosing to remove centralisation from the Bitcoin Core distribution. |
This is going very far off-topic. Let's keep it at this. |
|
I'd prefer to see an equivalent alert system replacement first, but the risks to the current one are probably significant enough to warrant its early removal. |
|
The fact that this introduces a greater attack surface for an unknown group of people should be enough to remove it immediately. Edit: Also isn't there clear conflict of interest issues with anyone who holds a key from arguing/voting on the existence of this feature?
They can look at previous commits, thats the point of version control. |
|
@whatisgravity It's removed now-- it's worth noting that the main contributors to Bitcoin Core have been trying to remove it for a couple years now, but have (and continue to) suffered pushback from some parties... it took a while to overcome that. |
That's the wonder of open source -- having code in a repository doesn't mean that you or the other core committers are required to personally support it, other than make sure that your own merged patches don't break automated unit tests. If the features of the bitcoin core repository are limited to those which some subset of developers are specifically interested in supporting, it makes bitcoin core a rather uninteresting project to the wider community. |
No, I disagree - at least how our project is structured - trying hard to handle issues and fix bugs that come up, for example - there is at least a little responsibility to the maintainers for what is in the repository. Only passing the automated tests is short-sighted. At least as long as the automated tests don't cover everything on every scenario on every platform (and some things, like people that act in unpredictable ways, can hardly be covered by automated tests). I do agree that you could structure an open source project that way. We're hampered also by the monolithic structure of the code. E.g. if the alert system was an external plugin, people who care about it could still maintain it, and we'd only have to make sure that our side of the API does what is advertised. But for better or worse, we have this bottleneck.
Possibly. But on the other hand, what we do support we try to keep working as well as possible. It's a bit of a compromise, where on one side you have a heap of barely-third-party-maintained hacks and on the other side you have a cathedral. I try to keep to a sensible middle, as said above, as far as the code structure allows. |
cfd519e Add release note documentation (BtcDrak) 6601ce5 protocol.h/cpp: Removes NetMsgType::ALERT (Thomas Kerin) ad72104 Formatting (BtcDrak) 1b77471 Remove alert keys (BtcDrak) 01fdfef Remove `-alerts` option (BtcDrak) 9206634 Update alert notification and GUI (BtcDrak) bbb9d1d Remove p2p alert handling (BtcDrak)
cfd519e Add release note documentation (BtcDrak) 6601ce5 protocol.h/cpp: Removes NetMsgType::ALERT (Thomas Kerin) ad72104 Formatting (BtcDrak) 1b77471 Remove alert keys (BtcDrak) 01fdfef Remove `-alerts` option (BtcDrak) 9206634 Update alert notification and GUI (BtcDrak) bbb9d1d Remove p2p alert handling (BtcDrak) manual fixes Signed-off-by: Pasta <[email protected]> remove sendalert.cpp Signed-off-by: Pasta <[email protected]> CAlertNotify -> AlertNotify Signed-off-by: Pasta <[email protected]> remove alert.h Signed-off-by: Pasta <[email protected]> remove vAlertPubKey for DevNet Signed-off-by: Pasta <[email protected]> remove src/main.cpp
ce8ff4b [Doc] Document removal of p2p alert system (random-zebra) a296c6f [Trivial] Fix NotifyAlertChanged comment (cs_mapAlerts not required) (random-zebra) 06fdc6a [Build] CMake: remove alert.cpp (random-zebra) 39c412a Remove alert keys (random-zebra) 936ca82 Remove p2p alert handling (random-zebra) d06eceb Remove `-alerts` option (random-zebra) 2ebd863 Update alert notification and GUI (random-zebra) Pull request description: This completely removes the p2p network alert messaging system. Based on upstream bitcoin#7692 Patches CVE-2016-10724 / CVE-2016-10725 ACKs for top commit: Fuzzbawls: utACK ce8ff4b furszy: utACK ce8ff4b Tree-SHA512: 40b2a023a53af880388337d52680fa100265ff704da8d51e889ca12cf97293cc0914ef3c4a57052ed342682fda08457f8f0fd8c0184d2aa408647ea6a0e5078d
ce8ff4b [Doc] Document removal of p2p alert system (random-zebra) a296c6f [Trivial] Fix NotifyAlertChanged comment (cs_mapAlerts not required) (random-zebra) 06fdc6a [Build] CMake: remove alert.cpp (random-zebra) 39c412a Remove alert keys (random-zebra) 936ca82 Remove p2p alert handling (random-zebra) d06eceb Remove `-alerts` option (random-zebra) 2ebd863 Update alert notification and GUI (random-zebra) Pull request description: This completely removes the p2p network alert messaging system. Based on upstream bitcoin#7692 Patches CVE-2016-10724 / CVE-2016-10725 ACKs for top commit: Fuzzbawls: utACK ce8ff4b furszy: utACK ce8ff4b Tree-SHA512: 40b2a023a53af880388337d52680fa100265ff704da8d51e889ca12cf97293cc0914ef3c4a57052ed342682fda08457f8f0fd8c0184d2aa408647ea6a0e5078d
This completely removes the p2p network alert messaging system; however, internal alerts, partition detection warnings and the
-alertnotifyoption features remain.The purpose of the p2p alert messaging system is to communicate severe network issues which can be achieved using a variety of traditional means rather than the Bitcoin p2p messaging layer. A decentralised system should not have privileged users able to send alert messages on the Bitcoin network.
From the perspective of the Bitcoin Core project, if we need to communicate with Core specific users, it can be done using existing public channels (website, twitter, reddit, Slack) as well as an opt-in Bitcoin Core announce only mailing list.