design discussion: keeping experimental features out of mainline testing

[ralexstokes]

As I was working on #3421, I was surprised to find that not only did I need to merge into the 4788 feature into the Deneb specs but I also ran into issues with an unrelated feature from EIP-6110.

Looking a bit more closely, it seems that we should keep "experimental" features out of the fork infrastructure for spec testing to avoid this situation as we don't want spec feature authors to have to think about other features when working on their own.

To avoid this, it seems that we can keep a repo-wide policy to avoid adding features in as "phases" the testing infra is aware of.

I see two places this is configured:

1. https://github.com/ethereum/consensus-specs/blob/dev/tests/core/pyspec/eth2spec/test/helpers/constants.py#L47 esp. ALL_PHASES, ALL_FORK_UPGRADES, TESTGEN_FORKS
2. https://github.com/ethereum/consensus-specs/blob/dev/tests/core/pyspec/eth2spec/test/helpers/forks.py

although I think the definitions in (2) are just filters/selectors and so aren't as important if they are just defined there. So we can achieve the outcome we want by just being careful with how we define the constants in (1).

I'm opening this issue in part to document this for future feature authors, but also gather input from the 6110 authors about their testing needs. I hear we wanted to generate test vectors for this feature and so I imagine that is why the edits to constants in (1) happened in the first place. If this is the case and the only way to generate tests is to touch these constants then I'd suggest we either keep feature tests on a branch (not the feature spec itself, just tests) or find a way to refactor the testgen infra so phases/forks can live alongside each other and not conflict.

Tagging @mkalinin @hwwhww @djrtwo who may have input here.

[mkalinin]

I am echoing that and open to any suggestions of making features uncoupled with the fork definitions. As for EIP-6110 definitions, yes one of the main reasons was the ability to generate tests vectors as part of general test vectors generation happening upon a spec release. Though, this approach might become suboptimal with the growth of the number of features.

I think that we can remove EIP6110 (particularly) from ALL_FORK_UPGRADES and probably add a new constant complimentary to ALL_PHASES which will also be used by with_feature_name and with_all_phases helpers.

EDIT: opened a PR to remove eip6110 from ALL_FORK_UPGRADES, #3423

[hwwhww]

We now have some configuration to separate the features and mainnet forks:

consensus-specs/tests/core/pyspec/eth2spec/test/helpers/constants.py

Lines 42 to 47 in efb554f

	# The forks that have light client specs
	LIGHT_CLIENT_TESTING_FORKS = (*[item for item in MAINNET_FORKS if item != PHASE0], ELECTRA)
	# The forks that output to the test vectors.
	TESTGEN_FORKS = (*MAINNET_FORKS, ELECTRA, EIP7594, WHISK)
	# Forks allowed in the test runner `--fork` flag, to fail fast in case of typos
	ALLOWED_TEST_RUNNER_FORKS = (*ALL_PHASES, WHISK, EIP7732)

closing the issue now.