Add support for electra fork epoch#15132
Conversation
terencechain
force-pushed
the
electra-fork-epoch
branch
2 times, most recently
from
19367ae to
2904700
Compare
terencechain
force-pushed
the
electra-fork-epoch
branch
3 times, most recently
from
3b9159d to
515c225
Compare
| params.SetupTestConfigCleanup(t) | ||
| config := params.BeaconConfig() | ||
| config.DenebForkEpoch = 0 | ||
| config.ElectraForkEpoch = math.MaxUint64 |
There was a problem hiding this comment.
Unsetting electra fork seems like the wrong approach. Why is this the fix for this test?
There was a problem hiding this comment.
SetupTestConfigCleanup backs up the config struct and sets up a t.Cleanup hook so that the original is restored after the test runs.
There was a problem hiding this comment.
(Preston's question is still worthwhile, just pointing out that this test helper limits the scope of config changes to the test)
There was a problem hiding this comment.
I think the test setup is generally broken. It doesn't work properly with the Electra fork, it uses a slot that’s already past the mainnet-defined Electra fork epoch—probably because it treated the Unix timestamp as the genesis time. Not sure if it’s worth going through all the unit tests and fixing the setup right now, but we could handle that in a separate PR. What do you think?
There was a problem hiding this comment.
I think we should have the correct fix, can't we just change the slot to one that is between deneb and electra rather than unsetting electra?
There was a problem hiding this comment.
i've been doing something similar i just add another defer params.SetupTestConfigCleanup(t) at the end
I think this is a bigger issue on how configs are used in our system like terence said. I think both ways should be fine for now as this will take much more to correctly fix
There was a problem hiding this comment.
i've been doing something similar i just add another defer params.SetupTestConfigCleanup(t) at the end
@james-prysm Do you have an example of this? I want to make sure this is being used correctly - calling it at the end of a test isn't going to protect the config from being mutated.
There was a problem hiding this comment.
I thought adding it would make it so that the next test would not need to add the cleanup at the top
#15161
|
Tests fail |
terencechain
force-pushed
the
electra-fork-epoch
branch
from
32a73be to
cfb67bc
Compare
terencechain
force-pushed
the
electra-fork-epoch
branch
from
cfb67bc to
be634e1
Compare
* Add support for electra fork epoch * Fix tests * fixing tests without needing to override electra fork config * reverting electra fork override on blob test too, replacing with custom timefetcher * reverting old changelog * Fix genesis time * Move mainnet test into mainnet_config_test.go * Update spec test to v1.5.0-beta.5 --------- Co-authored-by: james-prysm <[email protected]> Co-authored-by: Preston Van Loon <[email protected]>
* Add support for electra fork epoch * Fix tests * fixing tests without needing to override electra fork config * reverting electra fork override on blob test too, replacing with custom timefetcher * reverting old changelog * Fix genesis time * Move mainnet test into mainnet_config_test.go * Update spec test to v1.5.0-beta.5 --------- Co-authored-by: james-prysm <[email protected]> Co-authored-by: Preston Van Loon <[email protected]>
4 participants
eth-clients/mainnet#4