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

additonalForks in client config files #63

Closed
gumb0 opened this issue Nov 12, 2019 · 7 comments
Closed

additonalForks in client config files #63

gumb0 opened this issue Nov 12, 2019 · 7 comments

Comments

@gumb0
Copy link
Member

gumb0 commented Nov 12, 2019

  1. It is misspelled (should be additionalForks)

  2. Is it possible to make them optional? Currently nothing works if they're omitted.

> retesteth/retesteth -t GeneralStateTests/stRevertTest -- --clients alethIPCDebug
Running 1 test case...
Running tests using path: /home/andrei/dev/aleth/test/jsontests/
Active client configurations: 'alethIPCDebug '
Finishing retesteth run
unknown location(0): fatal error: in "GeneralStateTests/stRevertTest": std::runtime_error: additonalForks not found in ClientConfig  section! 
/home/andrei/dev/retesteth/retesteth/testSuites/StateTestsBoost.cpp(119): last checkpoint: "stRevertTest" fixture ctor

*** 1 failure is detected in the test module "Master Test Suite"
@gumb0
Copy link
Member Author

gumb0 commented Nov 14, 2019

Ok I see from https://github.com/ethereum/tests/blob/develop/PRLOG.md that they're used in transition tests. Then I have the following questions/suggestions:

  1. Why is this distinction between "regular forks" and "transition test forks" really needed in config? I think retesteth knows which chain configs are required for state tests (it's written in the test) and which others are required for transition tests. So it seems they all could be in a single list, not to complicate configuration for the user.

  2. If you insist that it's needed, I'd say a better name could be maybe transitionTestConfigs, to more clearly show what's its purpose (maybe later you would need some other type of tests/configs, what would you call it then, "additionalForks2"?)

@winsvega
Copy link
Collaborator

no. the reason is that forks from regular forks section are ordered and dynamic. retesteth just takes names from those and apply operators from tests.
so the client could fill up tests a < b < c < d given that a,b,c,d could be any name.

so I had to extend the allowed fork names by another section. cause transition tests use unique fork names. and for that fork names tests should not be generated when parsing >=a

the transition tests still not quite work with retesteth + geth.

@gumb0
Copy link
Member Author

gumb0 commented Nov 21, 2019

no. the reason is that forks from regular forks section are ordered and dynamic. retesteth just takes names from those and apply operators from tests.
so the client could fill up tests a < b < c < d given that a,b,c,d could be any name.

Running a test or filling a test on a particular fork is independent of other forks, so I don't see why order matters.

So do you mean that the point of forks is to allow filling only for the subset of forks? As I pointed out in #67 (striked out part) I think this should be better fine-tuned with command line options.

so I had to extend the allowed fork names by another section. cause transition tests use unique fork names. and for that fork names tests should not be generated when parsing >=a

I see, but to me this is an internal logic of retesteth. I think other clients would use these same srtings like FrontierToHomesteadAt5 if they would want to generate transition tests.
So my doubts from #67 apply here.

the transition tests still not quite work with retesteth + geth.

I guess they might yet not support at all these transition config names (but I don't know)

@winsvega
Copy link
Collaborator

Thats the point of configs. Any new client could use custom names and change any string

@gumb0
Copy link
Member Author

gumb0 commented Nov 22, 2019

I argue in #67 that this flexibility is not really needed by anyone, all the clients adapt their fork name strings when they integrate retesteth support anyway. It's just an additional configuration hassle to make it work right + additional maintenance pain for you.

@winsvega
Copy link
Collaborator

what about fork+EIP config?
I'd really like to keep custom fork support.
Maybe it should be split to a different config file then socket configuration.

@gumb0
Copy link
Member Author

gumb0 commented Nov 27, 2019

what about fork+EIP config?

In case clients would use the same chain config format for this case, too, then I think retesteth could also simplify this for the user (by creating the config with +EIP activated in it at runtime)

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

2 participants