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

ethcore/res: activate ecip-1088 phoenix on classic #11598

Merged
merged 4 commits into from
Apr 11, 2020

Conversation

q9f
Copy link
Member

@q9f q9f commented Apr 3, 2020

ref ethereumclassic/ECIPs#315

ref https://github.com/openethereum/openethereum/pull/11529

@ordian ordian added A0-pleasereview 🤓 Pull request needs code review. M2-config 📂 Chain specifications and node configurations. labels Apr 3, 2020
@ordian
Copy link
Collaborator

ordian commented Apr 3, 2020

@sorpaas if this PR is accepted, should we remove eip2200AdvanceTransition entirely?

@sorpaas
Copy link
Collaborator

sorpaas commented Apr 3, 2020

@ordian Yeah sounds like a good idea. We should actively remove flags that are no longer used.

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.

@q9f I did hear on Ethereum Classic Discord that there's some users who do not yet quite like the hard fork. I'm personally fine with either way, Phoenix or not. But do you think it will be worthwhile to add an --oppose-phoenix-fork command line flag together with this PR, maybe even temporarily, so that we can accommodate their needs, just in case?

The flag will also be useful in case a hard fork has to be delayed last minute (for example, in the case of Constantinople), and Geth always has a feature flag like this for every hard fork since Constantinople/Petersburg. So this would definitely be a nice-to-have no matter the political situation in ETC.

@q9f
Copy link
Member Author

q9f commented Apr 4, 2020

Stefan told me on the last call that Open Ethereum will hesitate to accept Rust-related changes from the Ethereum Classic community, so this pull request exclusively fixes the JSON spec to avoid a consensus issue on Ethereum Classic mainnet between Geth, Besu, and Parity.

In the current state, the spec will create an "Open Ethereum" version of Classic. This pull request addresses this.

I'm happy that users raise their opinion. The community consensus is well documented in the four public calls we had over the course of the last half a year:

I don't mind if you want to remove flags or introduce a special command-line flag for Michael. But that is out of scope for this pull request.

The sole purpose of this contribution is to fix the chain specification of a running production mainnet to prevent consensus issues in future.

ref hyperledger/besu#434

ref etclabscore/core-geth#48

@sorpaas
Copy link
Collaborator

sorpaas commented Apr 4, 2020

I don't mind if you want to remove flags or introduce a special command-line flag for Michael. But that is out of scope for this pull request.

@q9f From what I know it's not only Michael who raised opposition. Anyway it's great that as you're one of the people from pro-Phoenix side, you're not opposed to the idea of adding --oppose-phoenix-fork for the anti-Phoenix side. So let's do it!

@q9f
Copy link
Member Author

q9f commented Apr 4, 2020

Great! 🎉

@q9f
Copy link
Member Author

q9f commented Apr 6, 2020

Good morning, can we get this in before the next release? Thanks 🙏

@stevanlohja
Copy link

stevanlohja commented Apr 6, 2020

The ETC community had consensus for Phoenix just as the ETH community had consensus for Istanbul. I have many consumers using OpenEthereum that want the go-ahead to update their clients to the Phoenix version. Here is the unedited call with participating core dev stakeholders https://www.youtube.com/watch?v=HzcJL3rVbpU&t=1416s agreeing on the scope of the hard-fork.

I support Phoenix too.

@sorpaas
Copy link
Collaborator

sorpaas commented Apr 6, 2020

@stevanlohja Looks exactly like the situation of ProgPow back then.

Anyway I think it's good for OpenEthereum to remain neutral in this "Phoenix or not Phoenix" debate, so let's just support both sides as previously discussed with Afri!

@tzdybal
Copy link
Contributor

tzdybal commented Apr 6, 2020

In my opinion being neutral in this case is equal to accepting the "human-consensus" agreed by ETC community within ETC standards. Quoting practices used by ETH is IMHO not relevant.

ECIP-1088 is in Accepted state. Currently (after Last call) the only valid way of "rising opposition" according to the process is to create new ECIP that can fix or replace ECIP-1088.

"Phoenix or not Phoenix" means "use ETC or fork ETC into new chain". Not using Phoenix means that client is not compatible with ETC network, as it not implement Accepted ECIP.

In my opinion, adding --oppose option, because of out-of-process complaints stated in informal channels (Discord, blog), is not being neutral. It's being involved deep in ETC "politics".

For me, ideal situation in perfect world is following:
As a neutral reviewer of a hypothetical PR about adding hard-fork support of hypothetical chain to some imaginary client, I would limit my validation of changes to official resources. I would check what's the "governance" / decision making process, I would check if change is accepted (according to mentioned process), and if all the details of a change are valid.
I wouldn't go deep into mechanics of this hypothetical chain governance, I wouldn't measure the community bias regarding the hard-fork.
Decision making process of the hypothetical chain community should be a black box for me, the neutral reviewer. I'm not a part of the discussion. I just check if PR is consistent with hard-fork definition. More "diligence" is politics.

@sorpaas
Copy link
Collaborator

sorpaas commented Apr 6, 2020

@tzdybal If we had used your argument we wouldn't have had supported Ethereum Classic, because at that time, supporting theDAO was the "official" rough consensus from AllCoreDevs. However, we indeed decided to remain neutral at that point, and that comes the long tradition of OpenEthereum having ETC supported.

I don't know what you're having trouble with. During theDAO, geth also supported --oppose-dao-fork even when the majority of core devs agreed on moving with it. Let me repeat myself here, we support pro-Phoenix side of the hard fork on Ethereum Classic. That's where you get what you want. We also support the other side anti-Phoenix of the hard fork, by the --oppose-phoenix-fork flag. That's where some other ETC users get what they want.

I don't hope we end up going down the drastic route such as removing ETC support from OpenEthereum, but if the political situation there is not clear and we cannot even remain neutral and support both sides, then that will indeed be the last resort of actions.

@gitr0n1n
Copy link

gitr0n1n commented Apr 6, 2020

For transparency's sake: ethereumclassic/ECIPs#215 (comment)

@q9f
Copy link
Member Author

q9f commented Apr 7, 2020

Thanks everyone for raising your opinion, but let's focus on the technical aspect of the pull request here.

What needs to be done to get this reviewed? Is there anything we can help with?

@sorpaas
Copy link
Collaborator

sorpaas commented Apr 7, 2020

@q9f I'll finish the review really soon. Today or tomorrow.

@sorpaas
Copy link
Collaborator

sorpaas commented Apr 7, 2020

@q9f By the way, can you give us write access to your branch? That will simplify things a lot.

@q9f
Copy link
Member Author

q9f commented Apr 7, 2020

Anything wrong with my branch? Please add comments, I can just fix it.

@MGiuseppi
Copy link

MGiuseppi commented Apr 9, 2020

Hey, an --oppose-phoenix-fork flag would allow more users to use Parity.

Not sure why there is any resistance to adding this to Parity.

@q9f
Copy link
Member Author

q9f commented Apr 9, 2020

Hi Michael, thanks for joining the conversation. Additional command-line flags are out of scope for this pull request which seeks to fix the chain specification of the Classic mainnet.

@q9f
Copy link
Member Author

q9f commented Apr 11, 2020

the 2200 transition will be removed in #11624

reverted baa0051 and moved the chainspec check to #11625, now this should be very straight forward to review.

@sorpaas sorpaas merged commit 56885fe into openethereum:master Apr 11, 2020
@q9f q9f deleted the q9-config-phoenix branch April 11, 2020 16:28
dvdplm added a commit that referenced this pull request Apr 14, 2020
* master: (25 commits)
  Update .gitmodules (#11628)
  ethcore/res: activate ecip-1088 phoenix on classic (#11598)
  Upgrade parity-common deps to latest (#11620)
  Fix Goerli syncing (#11604)
  deps: switch to upstream ctrlc (#11617)
  Deduplicating crate dependencies (part 3 of n) (#11614)
  Deduplicating crate dependencies (part 2 of n, `slab`) (#11613)
  Actually save ENR on creation and modification (#11602)
  Activate POSDAO on xDai chain and update bootnodes (#11610)
  Activate on-chain randomness in POA Core (#11609)
  Deduplicating crate dependencies (part 1 of n) (#11606)
  Update enodes for POA Sokol (#11611)
  Remove .git folder from dogerignore file so vergen library can get build date and commit hash in the binary generatio vergen library can get build date and commit hash in the binary generation (#11608)
  Reduced gas cost for static calls made to precompiles EIP2046/1352 (#11583)
  [easy] `ethcore-bloom-journal` was renamed to `accounts-bloom` (#11605)
  Use serde_json to export hardcoded sync (#11601)
  Node Discovery v4 ENR Extension (EIP-868) (#11540)
  Fix compile warnings (#11595)
  Update version to 3.0.0-alpha.1 (#11592)
  ethcore/res: bump canon fork hash for mordor and kotti testnets (#11584)
  ...
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.

8 participants