Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

ethcore/res: enable ecip-1088 phoenix upgrade for kotti and mordor testnets #11529

Merged
merged 8 commits into from
Mar 13, 2020
Merged

ethcore/res: enable ecip-1088 phoenix upgrade for kotti and mordor testnets #11529

merged 8 commits into from
Mar 13, 2020

Conversation

q9f
Copy link
Member

@q9f q9f commented Feb 28, 2020

this pull request removes ECIP-1061 "aztlan", ECIP-1078 "phoenix fix", and enables ECIP-1088 "phoenix" for mordor, kotti, and classic.

ref https://ecips.ethereumclassic.org/ECIPs/ecip-1088

ref etclabscore/core-geth#48

ref ChainSafe/besu#26

once accepted, a release would be appreciated.

@ordian ordian requested a review from sorpaas February 28, 2020 10:15
@ordian ordian added A0-pleasereview 🤓 Pull request needs code review. M2-config 📂 Chain specifications and node configurations. labels Feb 28, 2020
Copy link
Collaborator

@sorpaas sorpaas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd need a few days to do some scrutiny of the specification. Last time OpenEthereum and MultiGeth were accused of "implementing Aztlan too fast". We'd better avoid that situation this time.

@q9f
Copy link
Member Author

q9f commented Feb 28, 2020

@sorpaas thanks 👍

@sorpaas
Copy link
Collaborator

sorpaas commented Feb 28, 2020

The Phoenix hard fork multi-geth pre-RFC is on. We're now requesting those who wish to apply those changes to provide justifications for a few questions. multi-geth/rfcs#1

@meowsbits
Copy link
Contributor

meowsbits commented Feb 28, 2020

That RFJ/RFC is a github.com/multi-geth project... not sure how that's related to github.com/OpenEthereum? Is there an OpenEthereum equivalent we can use to be more relevant to this repository?

Have any other Parity/OpenEthereum chain configuration modification cases filled out these questions before? Maybe we can use their answers as a guide for ours. I have a few questions about what "sufficient," for example, would mean exactly.

@sorpaas
Copy link
Collaborator

sorpaas commented Feb 28, 2020

@meowsbits No. The RFC process is currently the only known process of Request for Justifications. In the future we may be able to move it out of multi-geth so that it's more independent, but for now, the scrutiny would require those questions to be answered for OpenEthereum as well.

If you don't feel comfortable, you can just say "we don't have sufficient analysis" or similar to all those questions. We'd, however, need to properly document those replies to reduce the chance that if anything wrong happens in ETC Phoenix hard fork, OpenEthereum and MultiGeth are blamed for specification bugs again like it happened last time.

@meowsbits
Copy link
Contributor

meowsbits commented Feb 28, 2020

OpenEthereum and MultiGeth are blamed for specification bugs again like it happened last time

I wasn't aware this was the case (?).

Anyways, afaik, I don't think most would reason to blame a protocol provider implementation for specification bugs (those bugs are with the specification... right?). But... to be on the safe side, maybe these codebases can or should publish a "Use at your own risk" disclaimer if it's actually a serious concern?

As I understand it, in this case, the job of the protocol provider (client) should be to implement, rather than audit, the cited chain configuration specification (eg ECIP1088).

@sorpaas
Copy link
Collaborator

sorpaas commented Feb 28, 2020

these codebases can or should publish a "Use at your own risk" disclaimer if it's actually a serious concern?

@meowsbits That's why Request for Justifications. You can answer those justifications, or you can also refuse. If any of the problems in the RFC remain unanswered or with insufficient justification, we can do nothing else but publish a "use at your own risk" disclaimer for ETC with links to the RFJ -- not necessarily in release notes, but just somewhere for ETC community. The codebase license does already indicate everything is provided without warranty. However, we do want to be as responsible as we can, and we did get blamed last time due to specification bugs.

@sorpaas
Copy link
Collaborator

sorpaas commented Feb 29, 2020

@meowsbits

I wasn't aware this was the case (?).

Just for your information, if you still don't believe it, take a look at multi-geth/rfcs#1 (comment). OpenEthereum/MultiGeth are still being blamed for "implementing ECIP-1061/1072".

That you made many mistakes when (...) implementing ECIP-1061/1072, even knowing they had flaws, is your own responsibility.

This is why I think the Request for Justification process is necessary here, and we seriously need a "use it at your own risk" disclaimer for ETC. Those are precautions to avoid similar situations from happening again.

@bobsummerwill
Copy link

Wei's Request for Justification has nothing to do with OpenEthereum and is not part of the ECIP process used to reach human consensus on changes to the ETC protocol.

ECIP-1088 is in Last Call status with consensus having been reached on 26th Feb in the ETC Core Developers conference call:

https://etccooperative.org/update-on-etc-phoenix-hardfork/

Activation on the ETC mainnet is not expected until June.

Wei is welcome to impose whatever additional process he wishes for Multi-Geth but I consider it highly inappropriate of him to attempt to block the merge of this changelist into OpenEthereum.

It is a continuation of the same repeated abusive behaviour from him since November which has led to the proposal to remove him as an ECIP editor, and to replace ECIP-1000 (of which he is the author) with a new ECIP-0001 which clarifies some of the wording and also removes him as an editor and as the author/champion.

https://ecips.ethereumclassic.org/ECIPs/ECIP-0001

Aside - his "I got a private reply from bobsummerwill of ETC Coop providing the following justifications" was actually an out of context cut-and-paste from a private ETC security channel, not a "private reply". He has been removed from that channel since. His breach of trust there is just another example of his pattern of behaviour.

What he did not share was that I have contacted the individuals who did that analysis for ETH to see if we can get the same analysis done for ETC. Even then, that analysis is not gating for merging of this pull request so that we can actually do our testing.

@sorpaas
Copy link
Collaborator

sorpaas commented Feb 29, 2020

@bobsummerwill I understand you're trying to save face for ETC Coop because it strongly supported Aztlan last time. However, continuing the blame game does not help anyone, especially ETC.

For the record, I do find the blame game quite funny. First is "abuse process", but more like "someone disagreed with ETC Coop and Afri so they tried using administrative power to remove him". Not everyone follows ETC Coop, so then just accuse "doxxing" in a public document everyone agreed 6 months ago, accuse "blocking the process" for concerns about Aztlan specification. Repeat it until it becomes truth. Finally when bugs were found, it changed target to blame "rushed implementation in OpenEthereum and MultiGeth", on the time when the specification has already been moved to "accepted" status.

OpenEthereum does have a Code of Conduct, so please be mindful to others, keep discussions technical. Bullying, deliberate intimidation or other similar behaviors are highly inappropriate.

@bobsummerwill
Copy link

^ and so it continues.

@bobsummerwill
Copy link

No face-saving is required when you have the ability to admit human fallibility and course-correct:

https://etccooperative.org/etc-phoenix-hardfork/
https://etccooperative.org/update-on-etc-phoenix-hardfork/

You can even repeatedly reach consensus between parties which are mutually distrustful (ETC Coop and ETC Labs). That is the wonder of aligned incentives.

@sorpaas
Copy link
Collaborator

sorpaas commented Feb 29, 2020

Well thank you for admitting it. Then I do hope you learned something from Aztlan. Accusing others "doxxing" / "blocking the process" does not help you at all when legitimate and serious questions are being raised (like it happened last time). You'd still have your protocol flaws unfixed even if you try to surpass the opposition voices.

I see all questions in the RFC process multi-geth/rfcs#1 serious enough that they should be answered during OpenEthereum review, one way or another, before we can declare the Phoenix hard fork to be "relatively safe".

@q9f
Copy link
Member Author

q9f commented Mar 1, 2020

Hi @sorpaas, did you have the chance to do some scrutiny of the specification? I would love to get a release done with the new specs.

@sorpaas
Copy link
Collaborator

sorpaas commented Mar 1, 2020

@q9f The scrutiny I mentioned is multi-geth/rfcs#1

We have a preliminary review period ends on 6 March. If those questions are not answered by then, we'll merge it with a "use it at your own risk" disclaimer (to avoid OpenEthereum/MultiGeth being blamed for specification bugs again). You can still provide additional justifications after that point.

@Georgi87
Copy link

Georgi87 commented Mar 3, 2020

Wei is welcome to impose whatever additional process he wishes for Multi-Geth but I consider it highly inappropriate of him to attempt to block the merge of this changelist into OpenEthereum.

It is a continuation of the same repeated abusive behaviour from him since November which has led to the proposal to remove him as an ECIP editor, and to replace ECIP-1000 (of which he is the author) with a new ECIP-0001 which clarifies some of the wording and also removes him as an editor and as the author/champion.

@bobsummerwill this is not a constructive discussion. It is abusing contributors and is against the Code of Conduct. @sorpaas objections are all justified. Let us be constructive and lead a respectful and non-personal discussion. Otherwise, we will have to interfere and impose bans, which I want to avoid.

@sorpaas
Copy link
Collaborator

sorpaas commented Mar 9, 2020

I've decided to address point (4) in the scrutiny directly from our side in the RFC process -- backward compatibility promise for Phoenix multi-geth/rfcs#2

In the mean time, we still need to address point (1), (2), (3) in the scrutiny though. I'd suggest we take time to address them and do the impact analysis the same as it was done on Ethereum, because I'd think everyone wants the ETC network to be safe. If anyone still really insists on going the "use it at your own risk" route, please let us know the reasons and we'll try our best to accommodate!

@bobsummerwill
Copy link

I would like to suggest that these changes are integrated, but with activation only for Mordor and Kotti at the moment.

The current situation is seriously impeding Phoenix testing.

@dvdplm
Copy link
Collaborator

dvdplm commented Mar 9, 2020

The current situation is seriously impeding Phoenix testing.

@bobsummerwill can you elaborate on this? ETC devs and community members should be able to use the "beta" chainspec proposed here and run nodes with the --chain=/path/to/their/modified/classic.json file? Or…?

@bobsummerwill
Copy link

That is not convenient for everyone, @dvdplm.
Fine for manual testing, perhaps?

Work is being done to address these concerns, but blocking merge of this PR does not help.

ECIP-1088 is the canonical plan for ETC as agreed in the ETC Core Devs call.

Perhaps that consensus will change before June, but blocking this PR does not help.

@bobsummerwill
Copy link

ETC Phoenix has activated on the Mordor testnet already.
OpenEthereum is currently out of consensus, pending merging of this PR, which has been sitting here for 10 days now.

@sorpaas
Copy link
Collaborator

sorpaas commented Mar 9, 2020

@q9f If you change the scope of this PR to Mordor and Kotti testnets only, we can merge it first. Mainnet is still pending some form of resolutions of multi-geth/rfcs#1 though.

pending merging of this PR, which has been sitting here for 10 days now.

@bobsummerwill Please refrain from posting flaming comments like this. Our priority is to do as much as we can to provide a safe experience for ETC users and ETC community. If we had to choose between ETC network security vs making you happy, we'd definitely choose the former.

Really, if you had focused on addressing review questions in multi-geth/rfcs#1 rather than doing what you're doing, we might have already merged the PR.

@dvdplm
Copy link
Collaborator

dvdplm commented Mar 9, 2020

That is not convenient for everyone, @dvdplm.
Fine for manual testing, perhaps?

I guess my point is that regardless of this PR getting merged or not, there is not a release ready with these changes included. In other words, anyone who wishes to run ETC with these changes need to either use --chain=/path/to/classic.json with 2.7.2 or compile this branch. Merging this to master doesn't change that.
If we had a release ready to go out the door tomorrow your argument makes more sense, but we don't.

@q9f
Copy link
Member Author

q9f commented Mar 13, 2020

sorry, I was pretty sick this week. reverted the classic spec.

@sorpaas sorpaas changed the title ethcore/res: enable ecip-1088 phoenix upgrade for classic ethcore/res: enable ecip-1088 phoenix upgrade for kotti and mordor testnets Mar 13, 2020
@ordian ordian merged commit ba0a380 into openethereum:master Mar 13, 2020
@q9f q9f deleted the q9-classic-1088 branch March 14, 2020 11:21
ordian added a commit that referenced this pull request Mar 24, 2020
* master:
  informant: display I/O stats (#11523)
  [devp2p discovery]: remove `deprecated_echo_hash` (#11564)
  [secretstore] create db_version file when database doesn't exist (#11570)
  Remove Parity's Security Policy (#11565)
  ethcore/res: enable ecip-1088 phoenix upgrade for kotti and mordor testnets (#11529)
  Misc docs and renames …and one less clone (#11556)
  [secretstore]: don't sign message with only zeroes (#11561)
  [devp2p discovery]: cleanup (#11547)
  Code cleanup in the sync module (#11552)
  initial cleanup (#11542)
  Warn if genesis constructor revert (#11550)
@q9f q9f mentioned this pull request Apr 11, 2020
2 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-pleasereview 🤓 Pull request needs code review. M2-config 📂 Chain specifications and node configurations.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants