Add to epoch processing tests generation states before and after full epoch processing

[leolara]

According to #4025 epoch processing format test vectors cannot detect a class of bugs related to data dependencies between epoch processing sub-transitions.

Also, the current epoch processing format is incompatible with single pass optimised epoch processing like https://ethresear.ch/t/formally-verified-optimised-epoch-processing/17359

This PR, adds a backward compatible addition to the epoch processing format, that allows to run the full epoch processing transition for each of the test vectors of this format.

This has the advantages:

• Less code to maintain for the epoch processing handler.
• Works with single pass epoch processing.
• Can detect bugs related to data dependencies between different sub-transitions.

As a disadvantage this condition takes more resources to compute, but just a constant amount per test vector. And the previously test runner will still keep running as expected.

[jtraglia]

Hey @leolara, thanks for the PR! This is an interesting one. You're right, this is a shortcoming of that test format. We need to be very careful when making changes to test formats though. I'm glad to see it's backwards compatible, that's how it should be. It might take me a few days to properly review this.

[leolara]

@jtraglia no rush :-)

[GabrielAstieres]

Thanks for this PR!

Overall, I'm in a favor of having those changes. But before we merge it, would it be possible to have an example of a test (as stated on #4025) that would be caught thanks to this refactoring?

Additionally, I think I'd like to have @mkalinin's input to make sure those changes address the point he was making initially. I'd also like to have a client's team input to know how difficult it would be for clients to handle the extra outputs. I've sent a message in Teku's discord regarding this.

[lucassaldanha]

I took a quick look at our implementation in Teku and I don't think it would be too hard to adapt our tests to consume this updated format, maintaining it backward compatible.

I am not sure if you are planning to add a config for the test as part of its outputs so we can check the "type" of the test, and properly consume both pre and post epoch states. Otherwise we'd need to rely on the absence of pre_full and post_full ssz files to decide what to do. This can cause issues in the future (imagine some error on the test vector generation does not create the expected pre_full and post_full states, we will never know that we were supposed to execute a full epoch transition check).

[leolara]

Re what @GabrielAstieres says about a specific test, perhaps @mkalinin that made the issue have specific examples. For me is clear how this change will catch the bugs that the original unit test finds while will also catch bugs related to the data dependencies between sub-transitions of process_epoch, that currently were not being covered. But I have not specific examples as I have not been working on the consensus clients recently.

[leolara]

Re what @lucassaldanha says about how to consuming the vectors.

My idea is that the current consumer will keep working, and it can be substituted once this change is accepted by simpler code that simply loads pre_epoch, runs process_epoch and then asserts post_epoch.

After this current change is accepted the four states should be present always. I don't think there is a way that the new ones are absent after they are produced with this new version of the generation.

If you mean to make the consumer work also with older versions of the vectors, then yes you would have to check for the presence of the new states and fallback to the old version of the consumer if they are not present.

[jtraglia]

LGTM, thanks @leolara!

[leolara]

@jtraglia should we run this with a consumer to test that it doesn't break it? What do you think it is the simplest client to run that run the consumer for epoch_processing? Or @lucassaldanha has run this already with Teku? Should I run it with teku?

[mkalinin]

One of examples where this change would be useful is #4024. These tests use custom run_epoch_processing runner and slots sanity tests format as a workaround.

A thought, what if we add process_epoch as a special case sub-transition to the test format? Then clients will run the full epoch processing routine on the pre state and compare the output with the post.

I think making all epoch processing tests yielding full epoch transition test vectors might not have the desired effect on catching bugs. The reason is that each sub-transition test populates the state with data relevant to the sub-transition it is covering. And the class of tests that covers cases on the edge of dependency between different sub-transitions requires relevant data in the state. And this is great if test format will support this type of tests which this PR addresses 👍

[leolara]

I think making all epoch processing tests yielding full epoch transition test vectors might not have the desired effect on catching bugs. The reason is that each sub-transition test populates the state with data relevant to the sub-transition it is covering. And the class of tests that covers cases on the edge of dependency between different sub-transitions requires relevant data in the state. And this is great if test format will support this type of tests which this PR addresses 👍

Afaik, all epoch processing tests, use run_epoch_processing_with, and for that one you need to provide the pre-state of the full process_epoch, and then it runs all sub-transition until the one tested. So actually none of the epoch_processing tests create a state that is only to use right before the sub-transition. So, if I am right no test is doing this of creating a state that doesn't work with the rest of the sub-transitions. Considering this, the new format cover all the bugs previously detected (it runs the same up to the sub-transition in question), and will detect more if they are related to data dependencies bugs that appear after the sub-transition. Before, we were running all sub-transitions up to the one being tested, now we run all the sub-transitions including the ones after.

A thought, what if we add process_epoch as a special case sub-transition to the test format? Then clients will run the full epoch processing routine on the pre state and compare the output with the post.

I think this is not necessary, for what I said earlier, but it could be a transition mechanism to a new format when there are only full transition tests. Moving all of them to the new sub-format. I can implement it this way if everyone thinks it is better. Then basically all the other sub-formats will become empty.

[lucassaldanha]

@jtraglia should we run this with a consumer to test that it doesn't break it? What do you think it is the simplest client to run that run the consumer for epoch_processing? Or @lucassaldanha has run this already with Teku? Should I run it with teku?

I tested with Teku. But not with the whole test vector, just a few examples. However, it is mostly adding the extra ssz files to the test so I'd assume for any client running the tests, unless they actually consume the files it won't do anything.

[lucassaldanha]

Do we have an estimate of the size impact of having those extra 2x states per test? Just wondering if this is something we should consider.

Also I'll try to run some benchmarks to see if tests are taking too much longer for a full run.

[jtraglia]

Do we have an estimate of the size impact of having those extra 2x states per test?

A rough estimate would be an additional 226 MiB in total; double the current size.

~/Projects/ethereum/consensus-spec-tests/tests
$ find . -type d -name "epoch_processing" -exec du -ch {} + | grep total$
226M	total

This is noticeable but it shouldn't be an issue. The current size of everything is 2.7 GiB.

~/Projects/ethereum/consensus-spec-tests/tests
$ du -ch | grep total
2.7G	total

[lucassaldanha]

Also I'll try to run some benchmarks to see if tests are taking too much longer for a full run.

Ok! I tested with a subset of the tests and the time increase was negligible. I think we won't even feel the impact of these extra state checks in a full run

[mkalinin]

Afaik, all epoch processing tests, use run_epoch_processing_with,

There are a few tests that use run_epoch_processing_to and yield a pre-state right before a specific sub-transition, e.g. test_effective_balance_hysteresis and some of test_process_slashings. I am not sure if all of them can be simply switched to run_epoch_processing_with.

I am not against converting all tests into a new format. My point was not changing existing tests but rather adding a full epoch transition and use it in some already written tests that checks cases where an execution of one sub-transition may depend on the result of executing the others. But I agree that among already written tests there can be cases in which switching to the proposed format can increase the coverage.

[leolara]

@mkalinin

There are a few tests that use run_epoch_processing_to and yield a pre-state right before a specific sub-transition, e.g. test_effective_balance_hysteresis and some of test_process_slashings. I am not sure if all of them can be simply switched to run_epoch_processing_with.

I see there are 7 tests that use run_epoch_processing_to I am going to change them to produce pre_epoch and post_epoch.

use it in some already written tests that checks cases where an execution of one sub-transition may depend on the result of executing the others.

I can search for already existing tests, where the sub-transition data depends on a previous sub-transition.

[mkalinin]

I can search for already existing tests, where the sub-transition data depends on a previous sub-transition.

These are the tests that I am aware of:

• electra: Add pending deposit and consolidation tests #4024

[leolara]

I have changed run_pending_deposit_applying to the new format, for the pending deposit epoch sub-transition tests.

I have removed the test test_apply_pending_deposit_top_up__zero_balance that was the only that didn't pass. My idea was that a validator with zero balance is going to exit and it is not required how this method would behave with it in a situation where it is not exiting. If I am wrong and we need to test this let me know.

[leolara]

Currently, I have all the tests changed to the new format except test_scaled_penalties. It does a change in the state between epoch sub-transitions.

What do you suggest? Do I change the test completely so it does not have to do this manipulation between sub-transitions? Or I leave it as the only one with pre_epoch and post_epoch?

[leolara]

are the tests that I am aware of:

• electra: Add pending deposit and consolidation tests #4024

What are you suggesting, changing these tests to be epoch_processing format? Could we do that in another PR?

[mkalinin]

My idea was that a validator with zero balance is going to exit and it is not required how this method would behave with it in a situation where it is not exiting

In the existing protocol design top-ups are happening regardless of the validator state, this is what is checked by this state as 0 balance is one of the edge case conditions. I think we should not drop this test

What do you suggest? Do I change the test completely so it does not have to do this manipulation between sub-transitions? Or I leave it as the only one with pre_epoch and post_epoch?

Why can’t this test yield pre and post epoch transition states too?

What are you suggesting, changing these tests to be epoch_processing format? Could we do that in another PR?

Doing it in a separate PR is fine

[mkalinin]

Suggested change

	from eth2spec.test.helpers.epoch_processing import run_epoch_processing_from, run_epoch_processing_to, \
	run_process_slots_up_to_epoch_boundary
	from eth2spec.test.helpers.epoch_processing import (
	run_epoch_processing_from,
	run_epoch_processing_to,
	run_process_slots_up_to_epoch_boundary,
	)

[mkalinin]

I think it would be better to put it this way:

Suggested change

	if not disable_slots_processing:
	if enable_slot_processing:

[mkalinin]

Why it should be instead and couldn't be in addition to the pre and post state checks?