Skip to content
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

Rename EIP150 and EIP158 to TangerineWhistle and SpuriousDragon respectively #488

Closed
pipermerriam opened this issue Aug 24, 2018 · 23 comments

Comments

@pipermerriam
Copy link
Member

pipermerriam commented Aug 24, 2018

I would like to propose renaming the strings we use to refer to the forks currently referred to as "EIP150" and "EIP158"

  • EIP150 -> TangerineWhistle
  • EIP158 -> SpuriousDragon

My motivation is as follows:

  • EIP158 is not even the correct EIP number as it was replaced with EIP160
  • Using the EIP to reference it is somewhat confusing as most forks contain multiple EIPs, TangerineWhistle just happened to be special in that it only contained one.
  • Naming consistency, Using the "fancy" names instead of their EIP numbers is more inline with how the other forks are named.
@axic
Copy link
Member

axic commented Aug 31, 2018

Also adding to this: the "meta" EIPs describing these hardforks kind of codified these "fancy names" as the ones used to refer to them.

@pipermerriam pipermerriam changed the title Rename EIP150 and EIP158 Rename EIP150 and EIP158 to TangerineWhistle and SpuriousDragon respectively Aug 31, 2018
@pipermerriam
Copy link
Member Author

@winsvega how hard would this be?

@pipermerriam
Copy link
Member Author

Oh, I'd also advocate we do this after Constantinople to reduce any extra complexity or confusion this causes since it will almost definitely require every client to update their test suite.

@winsvega
Copy link
Collaborator

I think we could actually do this before the Constantinople. with this changes
ethereum/aleth#5154

@holiman
Copy link
Contributor

holiman commented Sep 17, 2018

Oh, I'd also advocate we do this after Constantinople

👍 from me

@winsvega
Copy link
Collaborator

delayed for after Constantinople

@winsvega
Copy link
Collaborator

need to come up with a fork naming procedure.
renaming forks retrospectively is a bother.
ethereum/EIPs#1848

@winsvega
Copy link
Collaborator

winsvega commented Sep 1, 2019

the actual fork names for setChainParams could now be configured with retesteth configs.
https://github.com/ethereum/tests/tree/develop/Retesteth/default/genesis

so the test repo could use existing names

@winsvega winsvega closed this as completed Sep 1, 2019
@axic
Copy link
Member

axic commented Sep 5, 2019

What is the resolution? I'm still much in favour of @pipermerriam's proposal, was it implemented?

More specifically I think the test suit should always use the "codename" from the hardfork metas:

  • homestead
  • daofork
  • tangerine whistle
  • spurious dragon
  • byzantium
  • constantinople
  • petersburg
  • istanbul
  • berlin

So basically rename:

  • EIP150 -> TangerineWhistle
  • EIP158 -> SpuriousDragon
  • ConstantinopleFix -> Petersburg

@winsvega
Copy link
Collaborator

winsvega commented Sep 8, 2019

It doesn't really matter cause the tests are like closed eco system. Clients could not to care about this.

@axic
Copy link
Member

axic commented Sep 8, 2019

Tests aren't a closed ecosystem, since every client supposed to run them. I'm confused by this.

@holgerd77
Copy link
Contributor

Yes, I would also very much suggest to still do this, doesn't need to be now in the current Istanbul stress - but at least mid-term after dust from Istanbul has settled.

@pipermerriam
Copy link
Member Author

I think it should be done now if it's going to happen (which I'm in agreement that it should). All of the clients are or have recently been updating their test suites so this one extra name change should be an easy extra change for them to make.

@winsvega
Copy link
Collaborator

winsvega commented Sep 8, 2019

'''
Tests aren't a closed ecosystem, since every client supposed to run them. I'm confused by this.
'''

Retesteth read tests and execute with chainparams. Chainparams could be configured on for any client separately with retesteth. Clients don't have to parse the tests. Therefore tests does not interact eith the clients directly. Hive do the same.

@winsvega
Copy link
Collaborator

winsvega commented Sep 8, 2019

''
All of the clients are or have recently been updating their test suites so this one extra name change should be an easy extra change for them to make.
'''
Not for the legacy tests folder

@axic
Copy link
Member

axic commented Sep 9, 2019

I think it should be done now if it's going to happen

Agree. It was postponed last year for the same reason. And it will be postponed again if it is not done 😉

@axic
Copy link
Member

axic commented Sep 9, 2019

@winsvega clients don't use retesteth to run tests. Retesteth was only supposed to be used to generate the tests. There are some clients which don't even have RPC support, such as ethereumjs, yet they run the tests.

@holiman
Copy link
Contributor

holiman commented Sep 9, 2019

I totally agree that we should not consider retesteth a replacement for actually running tests in client-specific test harnesses. In those cases that clients do implement retesteth, it can replace it somewhat. However, most clients, I would assume, run statetests in their CI flows, and use whatever testing infrastructure their platform supports. For go, it's go test, which does not lend itself well to spinning up the whole retesteth-infra.

So yeah, it's not a closed ecosystem.

@pipermerriam
Copy link
Member Author

pipermerriam commented Sep 9, 2019

@winsvega I think the hanging question is where you stand.

  • -1 you are opposed and we need to come to agreement to move forward
  • -0 you are opposed but you'll go with whatever group decides
  • +0 you are in favor but you'll go with whatever group decides
  • +1 you are strongly in favor and we need to come to agreement to move forward

If we can move forward then either you can make the necessary changes or since this is an effort I've proposed I'm happy to make the changes if you'll give me a little guidance on what should be changed and the right way to do it.

@winsvega
Copy link
Collaborator

winsvega commented Sep 9, 2019

-1
The renaming will require to change testeth/retesteth and to regenerate the legacy tests. I don't want to touch and change any of legacy tests so not to break anything in it.

Especially if that tests could be executed by hive/retesteth with any fork names replaced by config scripts

@pipermerriam
Copy link
Member Author

Thanks @winsvega

I'm happy to respect that decision. I'll consider this issue closed as "wontfix". 👍

@axic
Copy link
Member

axic commented Sep 9, 2019

Why is there a need to regenerate anything? Cannot this be done with a simple sed script?

@winsvega
Copy link
Collaborator

winsvega commented Sep 9, 2019

all tests have source and verified by source hash.
although.. you could replce the name in already generated test while not touching the source but that would be like a hardfork for any test tool. an exeption rule in a code. I don't really get why is it so crucial, if you have your own test tool you could just parse the test whatever you like

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants