Impl Ord on ForkName for ChainSpec usage

[dapplion]

Issue Addressed

When having fork-dependant logic there's a trade-off between exhaustive match pattern and ordering. LH maintainers should consider that this client is used quite a bit to develop future features, which may also need to implement a fork.

• exhaustive: You ensure that at least one developer considers if this config value has been changed for the new fork.
• ordering: Reverse the above, and reduce the amount of boilerplate a PoC developer has to go through. On an Ethereum mainnet fork, I find it very unlikely that we would forget to update such values. Spec tests cover the state transition values, and the spec diff will make it very obvious that we need to change some networking constants.

Can't ignore the fact that there still quite a bit of fork boiler-plate out there, but hopefully this PR is the first step into a less boiled future :)

Original discussion in #5507 (comment)

Proposed Changes

Impl Ord on ForkName for ChainSpec usage

[michaelsproul]

would be good to add a test that checks agreement between ForkName::list_all and Ord

[realbigsean]

added one here c34b26e

[macladson]

I agree that for Lighthouse's current design, this change makes a lot of sense.

However, I think being able to support arbitrary fork ordering would be really useful for a variety of reasons.

Consider the following set of forks and their associated features and dependencies:

Fork	Features	Dependencies
A	a, b, c
B	d, e	a
C	f, g
D	h	d

The natural fork ordering A -> B -> C -> D is valid since B comes after A and D comes after B and thus the dependency requirements are satisfied.

However, the fork ordering A -> C -> B -> D is also valid since the dependency requirements are also met.

Splitting out Forks into sets of Features would mean in the code, instead of having certain features gated behind match ForkName checks, we could gate behind an is_eip_420_active() check. I think this would have the same effect as cleaning up a lot of the boilerplate we are currently dealing with, while giving us additional flexibility when defining new forks.

For example, two devs could simultaneously be working on different features on the same Fork and their code would be completely discrete. We could even test them separately from the same binary. We want to test a new fork without a certain feature? Just remove it from the Fork/Feature ordering. Or, a dev could be working on a far-future feature, and simply add it to the latest fork for testing. Then when the feature is confirmed for a new hard fork, it is trivial to move it over to the latest fork.

The work I've been doing in Verkle would massively benefit from this change, since the current testnet is running Capella -> Verge, which means maintaining it involves stripping out the Deneb related code. If I could simply reorder Deneb to go after Verge, it would solve that issue.

In short, I think there would be massive developer experience benefits to moving towards a Fork = {Features} paradigm.

This is something @michaelsproul and I have been experimenting with in sigp/superstruct#33, but it's not trivial and will likely require a large rework of Superstruct (or an entirely separate library) (not to mention will require us to go back and split old forks into their corresponding features).

With all that said, I would support this PR in the short term, and in fact it might be a good stepping stone to what I think our future should be, which is a Fork/Feature/Dependency paradigm.

[realbigsean]

Maybe a first version of sigp/superstruct#33 could just include the fork ordering aspect (not features themselves). So leaning on from and until. And figuring out how to make a single fork order global for superstructs. That plus shaving down a lot of the per-fork boilerplate may make what we have currently a lot more tenable.

[realbigsean]

@Mergifyio queue

[mergify]

queue

🛑 The pull request has been removed from the queue default

The queue conditions cannot be satisfied due to failing checks.

You can take a look at Queue: Embarked in merge queue check runs for more details.

In case of a failure due to a flaky test, you should first retrigger the CI.
Then, re-embark the pull request into the merge queue by posting the comment
@mergifyio refresh on the pull request.

[realbigsean]

@Mergifyio requeue

[mergify]

requeue

✅ This pull request will be re-embarked automatically

The followup queue command will be automatically executed to re-embark the pull request

[mergify]

queue

✅ The pull request has been merged automatically

The pull request has been merged automatically at ced6538