Add new Altair transition tests #2664

hwwhww · 2021-10-12T15:09:24Z

Fix #2662
Thank @potuz for the ideas. ✨

A validator is slashed in phase0 and is active but slashing at the fork and thus leaking. It may or not be a member of a sync committee, we need to check the right penalties.
- test_transition_with_one_fourth_slashed_active_validators_pre_fork
A validator started an exit in phase0 and is put in a queue, check these queues, either active or leaking at the fork.
- test_transition_with_one_fourth_exiting_validators_exit_post_fork
- test_transition_with_one_fourth_exiting_validators_exit_at_fork
Quadratic leaking in case of network not finalizing at the fork.
- test_transition_with_leaking_pre_fork
- test_transition_with_leaking_at_fork
- ~~test_transition_with_leaking_post_fork~~
An activator deposited in phase0 and is in the queue at the fork.
- test_transition_with_non_empty_activation_queue

Operations at the fork block:

test_transition_with_proposer_slashing_right_after_fork
test_transition_with_proposer_slashing_right_before_fork
test_transition_with_attester_slashing_right_after_fork
test_transition_with_attester_slashing_right_before_fork
test_transition_with_deposit_right_after_fork
test_transition_with_deposit_right_before_fork
test_transition_with_voluntary_exit_right_after_fork
test_transition_with_voluntary_exit_right_before_fork

TODO

clean up, refactoring
add tests to include slashings at the fork block
add tests to include voluntary exit at the fork block

potuz · 2021-10-12T15:15:28Z

Thanks a lot! perhaps it may be worth adding the case that the slashed validator is assigned to a sync committee? Since in this case the penalties are active and the implementations may differ in how to add a slashing validator from phase 0?

djrtwo · 2021-10-12T17:44:56Z

The approach looks good!

Question -- what do the _post_fork type tests buy us? Do you expect this uncovers any special cases that standard altair-only tests don't already?

… at the fork block

hwwhww · 2021-10-12T18:18:10Z

@potuz
Ah, yes, I forgot to add

    # ensure that some of the current sync committee members are the slashed
    slashed_pubkeys = [state.validators[index].pubkey for index in slashed_indices]
    assert any(set(slashed_pubkeys).intersection(list(state.current_sync_committee.pubkeys)))

I updated it in the new commits. Thank you!

@djrtwo

what do the _post_fork type tests buy us?

test_transition_with_leaking_post_fork: the fork_epoch is one of the non-finalized epochs that trigger the leaking.
test_transition_with_one_fourth_exiting_validators_exit_post_fork: the exit was initiated before the fork but exit_epoch > fork_epoch. I don't expect it would be different from standard altair-only tests.

hwwhww · 2021-10-12T18:22:57Z

tests/core/pyspec/eth2spec/test/helpers/fork_transition.py

@@ -0,0 +1,134 @@
+import random


Sorry for the increasing diff. 😅
This file is mainly copied from test_transition.py.

p.s. decided to name it fork_transition.py rather than transition.py because the most important "transition" helpers are in state.py.

- sanity check: deposit operation is independent of spec fork versions - refactoring - add comments

djrtwo

Looking good!

The tests are a bit verbose. Some might be able to be modularized in helper functions.. Not critical but consider if you see some opportunity.

We also might consider some of these tests to be done in both a leaking and non-leaking context. Maybe we do a mixed-use test (combination of all types here -- slashed, exited, queued, etc) and do a non-leaking and leaking form.

tests/core/pyspec/eth2spec/test/altair/transition/test_activation.py

djrtwo · 2021-10-13T19:21:12Z

tests/core/pyspec/eth2spec/test/altair/transition/test_activation.py

+    operation_dict = {'voluntary_exits': signed_exits}
+
+    # irregular state transition to handle fork:
+    state, block = do_altair_fork(state, spec, post_spec, fork_epoch, operation_dict=operation_dict)


So this exit happens at the 0th block immediately after the fork transition?

Would it make sense to also do one at the N-1 block to the fork?

right, it seems that this exit is valid starting in the prior epoch (fork_epoch - 1) and included in the "fork block", i.e. the first block after the fork transition occurs.

we could include an exit at the N-1 block but that kind of raises the question of the scope of this PR -- we could include operations at each N-1 block and N block (if the "fork block" is N) but then we have to ask if doing this by hand is the best way to test it...

there is also the question of operations in the N+1 block, but we could argue those cases are already covered by existing tests that just start in an Altair fork...

Right. Doing some random-operation fork-span tests might be the right way to capture more funcitonality beyond the hand written tests

Reworked these tests into transition/test_operations.py (3a242a1).

use a run_transition_with_operation function to handle all types of operations

it could be done with some factories and cleaner, but I think it probably doesn't worth it for these tests. 😅

added fork slot-1 cases

we could argue those cases are already covered by existing tests that just start in an Altair fork...

I think the difference is that we use pre_spec to create the operations here. Is this kind of tests already covered?
And indeed random-operation fork-span would cover more! Maybe in later PRs?

tests/core/pyspec/eth2spec/test/helpers/inactivity_scores.py

tests/core/pyspec/eth2spec/test/altair/transition/test_activation.py

tests/core/pyspec/eth2spec/test/altair/transition/test_slashing.py

tests/core/pyspec/eth2spec/test/helpers/random.py

ralexstokes

looks great!

thanks to @potuz for raising the original issue and thanks @hwwhww for writing these tests!

overall the direction is good and these tests cover the cases we are concerned about. i left some minor feedback across the PR of various things to tighten/clean up before we merge.

one point that runs across the PR: you use transition_to_next_epoch_and_append_blocks to generate an epoch's worth of blocks after the fork; in most (all?) of the tests, it isn't relevant to the thing we are testing to have a full epoch of blocks. to keep testing load smaller, i'd consider only making a handful of blocks after the fork (maybe even just 1 block).

tests/core/pyspec/eth2spec/test/helpers/inactivity_scores.py

tests/core/pyspec/eth2spec/test/helpers/fork_transition.py

tests/core/pyspec/eth2spec/test/altair/transition/test_activation.py

ralexstokes · 2021-10-14T10:47:11Z

tests/core/pyspec/eth2spec/test/altair/transition/test_activation.py

+    operation_dict = {'voluntary_exits': signed_exits}
+
+    # irregular state transition to handle fork:
+    state, block = do_altair_fork(state, spec, post_spec, fork_epoch, operation_dict=operation_dict)


right, it seems that this exit is valid starting in the prior epoch (fork_epoch - 1) and included in the "fork block", i.e. the first block after the fork transition occurs.

ralexstokes · 2021-10-14T10:49:07Z

tests/core/pyspec/eth2spec/test/altair/transition/test_activation.py

+    operation_dict = {'voluntary_exits': signed_exits}
+
+    # irregular state transition to handle fork:
+    state, block = do_altair_fork(state, spec, post_spec, fork_epoch, operation_dict=operation_dict)


we could include an exit at the N-1 block but that kind of raises the question of the scope of this PR -- we could include operations at each N-1 block and N block (if the "fork block" is N) but then we have to ask if doing this by hand is the best way to test it...

there is also the question of operations in the N+1 block, but we could argue those cases are already covered by existing tests that just start in an Altair fork...

tests/core/pyspec/eth2spec/test/altair/transition/test_activation.py

ralexstokes · 2021-10-14T10:59:28Z

tests/core/pyspec/eth2spec/test/altair/transition/test_activation.py

+
+    yield "blocks", blocks
+    yield "post", state
+


not sure if we should inflate this PR further, but following the symmetry of exits, there is a test case where a validator is de-queued for activation at the fork epoch...

good suggestion. I think transition/test_operation.py::test_transition_with_deposit_right_before_fork now covers it?

not just making a deposit but the case where a validator is already deposited and their activation_epoch == FORK_EPOCH... not sure it is worth blocking this PR on this case though

added test_transition_with_activation_at_fork_epoch!

tests/core/pyspec/eth2spec/test/altair/transition/test_activation.py

hwwhww · 2021-10-14T16:35:59Z

@djrtwo @ralexstokes
thanks a lot for the reviews 🙏❤️

I think I addressed or replied to the suggestions. I'm running make gen_transition locally now.

djrtwo · 2021-10-14T22:02:20Z

I'm okay to release right after @ralexstokes puts his eyes on it one more time

tests/core/pyspec/eth2spec/test/altair/epoch_processing/test_process_inactivity_updates.py

tests/core/pyspec/eth2spec/test/helpers/random.py

tests/core/pyspec/eth2spec/test/helpers/fork_transition.py

tests/core/pyspec/eth2spec/test/altair/transition/test_activations_and_exits.py

ralexstokes · 2021-10-15T10:27:52Z

tests/core/pyspec/eth2spec/test/altair/transition/test_activations_and_exits.py

+#
+
+@fork_transition_test(PHASE0, ALTAIR, fork_epoch=2)
+@with_presets([MINIMAL], reason="only test with non-full committee")


how do you mean "non-full"? like duplicates vs none in the sync committee?

updated reason description: "only test with enough validators such that at least one exited index is not in sync committee"

It is when (almost) all validator indices are in sync committee, we may get False on
any(set(slashed_pubkeys).difference(list(state.current_sync_committee.pubkeys))) check.

ralexstokes · 2021-10-15T10:29:00Z

tests/core/pyspec/eth2spec/test/altair/transition/test_slashing.py

+    yield "pre", state
+
+    # irregular state transition to handle fork:
+    state, _ = do_altair_fork(state, spec, post_spec, fork_epoch, with_block=False)


why do we not want the block here?

because slash_random_validators may select the next block proposer. I didn't want to change slash_random_validators too much. Do you feel strongly that we should add an avoid_next_proposer flag to slash_random_validators helper?

ralexstokes · 2021-10-15T10:32:02Z

tests/core/pyspec/eth2spec/test/helpers/fork_transition.py

+            spec, state, slashed_index=selected_validator_index, signed_1=True, signed_2=True)
+        operation_dict = {'proposer_slashings': [proposer_slashing]}
+    elif operation_type == OperetionType.ATTESTER_SLASHING:
+        attester_slashing = get_valid_attester_slashing(spec, state, signed_1=True, signed_2=True)


we could also slash the proposer here, right?

Yes...!
Good catch! I updated state_transition_across_slots_with_ignoring_proposers helper to avoid such cases. (162711e)

ralexstokes

nice updates! i left a few more comments on minor things about updating some code and/or formatting

ralexstokes · 2021-10-15T15:38:02Z

tests/core/pyspec/eth2spec/test/helpers/random.py

@@ -53,7 +53,7 @@ def exit_random_validators(spec, state, rng, fraction=0.5, exit_epoch=None, with
    """
    if from_epoch is None:
        from_epoch = spec.MAX_SEED_LOOKAHEAD + 1
-    epoch_diff = from_epoch - spec.get_current_epoch(state)
+    epoch_diff = int(from_epoch) - int(spec.get_current_epoch(state))


ah, right we can't do signed arith here... whoops!

ralexstokes

looks great! thanks for this and incorporating all the feedback :)

[WIP] Add new transition tests

4dd8b7c

hwwhww added the scope:CI/tests/pyspec label Oct 12, 2021

hwwhww changed the title ~~[WIP] Add new transition tests~~ [WIP] Add new Altair transition tests Oct 12, 2021

hwwhww added 2 commits October 12, 2021 23:33

ensure that some of the current sync committee members are the slashed

95f940c

Refactoring. Sort tests to specific files.

f0980a4

Make operation (attester_slashing, proposer_slashing, voluntary_exit)…

53d4fa5

… at the fork block

hwwhww commented Oct 12, 2021

View reviewed changes

hwwhww marked this pull request as ready for review October 12, 2021 18:23

hwwhww changed the title ~~[WIP] Add new Altair transition tests~~ Add new Altair transition tests Oct 12, 2021

hwwhww requested review from djrtwo and ralexstokes October 12, 2021 19:22

Minor refactoring

67da1ba

- sanity check: deposit operation is independent of spec fork versions - refactoring - add comments

hwwhww force-pushed the new-transition-test-cases branch from 91f63bb to 67da1ba Compare October 13, 2021 16:06

djrtwo reviewed Oct 13, 2021

View reviewed changes

ralexstokes reviewed Oct 13, 2021

View reviewed changes

tests/core/pyspec/eth2spec/test/altair/transition/test_activation.py Outdated Show resolved Hide resolved

ralexstokes reviewed Oct 13, 2021

View reviewed changes

tests/core/pyspec/eth2spec/test/altair/transition/test_activation.py Outdated Show resolved Hide resolved

ralexstokes reviewed Oct 13, 2021

View reviewed changes

tests/core/pyspec/eth2spec/test/altair/transition/test_slashing.py Outdated Show resolved Hide resolved

ralexstokes reviewed Oct 13, 2021

View reviewed changes

tests/core/pyspec/eth2spec/test/helpers/random.py Outdated Show resolved Hide resolved

ralexstokes reviewed Oct 14, 2021

View reviewed changes

tests/core/pyspec/eth2spec/test/altair/transition/test_activation.py Outdated Show resolved Hide resolved

ralexstokes reviewed Oct 14, 2021

View reviewed changes

tests/core/pyspec/eth2spec/test/altair/transition/test_activation.py Outdated Show resolved Hide resolved

ralexstokes reviewed Oct 14, 2021

View reviewed changes

tests/core/pyspec/eth2spec/test/altair/transition/test_activation.py Outdated Show resolved Hide resolved

ralexstokes reviewed Oct 14, 2021

View reviewed changes

tests/core/pyspec/eth2spec/test/altair/transition/test_activation.py Outdated Show resolved Hide resolved

PR feedback

cbba542

hwwhww force-pushed the new-transition-test-cases branch 2 times, most recently from b861207 to 5081895 Compare October 14, 2021 14:40

hwwhww force-pushed the new-transition-test-cases branch from 5081895 to 49bf78d Compare October 14, 2021 14:40

hwwhww added 2 commits October 15, 2021 00:02

Refactor and add test cases of having operation right before the fork

3a242a1

pr feedback

f62167c

minor: Use pre spec for running prepare_state_and_deposit

52235a9

Fix/ignore mainnet preset cases

a4e5d50

ralexstokes reviewed Oct 15, 2021

View reviewed changes

tests/core/pyspec/eth2spec/test/altair/epoch_processing/test_process_inactivity_updates.py Show resolved Hide resolved

ralexstokes reviewed Oct 15, 2021

View reviewed changes

tests/core/pyspec/eth2spec/test/helpers/random.py Outdated Show resolved Hide resolved

ralexstokes reviewed Oct 15, 2021

View reviewed changes

tests/core/pyspec/eth2spec/test/helpers/random.py Outdated Show resolved Hide resolved

ralexstokes reviewed Oct 15, 2021

View reviewed changes

tests/core/pyspec/eth2spec/test/helpers/random.py Outdated Show resolved Hide resolved

ralexstokes reviewed Oct 15, 2021

View reviewed changes

tests/core/pyspec/eth2spec/test/helpers/fork_transition.py Outdated Show resolved Hide resolved

ralexstokes reviewed Oct 15, 2021

View reviewed changes

tests/core/pyspec/eth2spec/test/helpers/fork_transition.py Outdated Show resolved Hide resolved

ralexstokes reviewed Oct 15, 2021

View reviewed changes

tests/core/pyspec/eth2spec/test/altair/transition/test_activations_and_exits.py Outdated Show resolved Hide resolved

ralexstokes reviewed Oct 15, 2021

View reviewed changes

tests/core/pyspec/eth2spec/test/altair/transition/test_activations_and_exits.py Show resolved Hide resolved

ralexstokes reviewed Oct 15, 2021

View reviewed changes

tests/core/pyspec/eth2spec/test/altair/transition/test_activations_and_exits.py Outdated Show resolved Hide resolved

ralexstokes reviewed Oct 15, 2021

View reviewed changes

hwwhww added 6 commits October 15, 2021 22:00

PR feedback. Rework transition_to_next_epoch_and_append_blocks a bit

162711e

fix typo

be6d201

PR feedback on exit_random_validators helper

40869d6

Fix randomize_state default params

b0b3733

Add test_transition_with_activation_at_fork_epoch

7480fad

Fix SSZ underflow

e70ef11

ralexstokes reviewed Oct 15, 2021

View reviewed changes

ralexstokes approved these changes Oct 15, 2021

View reviewed changes

ralexstokes merged commit bf01e11 into dev Oct 15, 2021

hwwhww deleted the new-transition-test-cases branch October 15, 2021 15:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add new Altair transition tests #2664

Add new Altair transition tests #2664

hwwhww commented Oct 12, 2021 •

edited

Loading

potuz commented Oct 12, 2021

djrtwo commented Oct 12, 2021

hwwhww commented Oct 12, 2021

hwwhww Oct 12, 2021 •

edited

Loading

djrtwo left a comment •

edited

Loading

djrtwo Oct 13, 2021

djrtwo Oct 13, 2021

ralexstokes Oct 14, 2021

ralexstokes Oct 14, 2021

djrtwo Oct 14, 2021

hwwhww Oct 14, 2021 •

edited

Loading

ralexstokes left a comment

ralexstokes Oct 14, 2021

ralexstokes Oct 14, 2021

ralexstokes Oct 14, 2021

hwwhww Oct 14, 2021

ralexstokes Oct 15, 2021

hwwhww Oct 15, 2021

hwwhww commented Oct 14, 2021

djrtwo commented Oct 14, 2021

ralexstokes Oct 15, 2021

hwwhww Oct 15, 2021

ralexstokes Oct 15, 2021

hwwhww Oct 15, 2021

ralexstokes Oct 15, 2021

hwwhww Oct 15, 2021 •

edited

Loading

ralexstokes left a comment

ralexstokes Oct 15, 2021

ralexstokes left a comment

Add new Altair transition tests #2664

Add new Altair transition tests #2664

Conversation

hwwhww commented Oct 12, 2021 • edited Loading

potuz commented Oct 12, 2021

djrtwo commented Oct 12, 2021

hwwhww commented Oct 12, 2021

hwwhww Oct 12, 2021 • edited Loading

Choose a reason for hiding this comment

djrtwo left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hwwhww Oct 14, 2021 • edited Loading

Choose a reason for hiding this comment

ralexstokes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hwwhww commented Oct 14, 2021

djrtwo commented Oct 14, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hwwhww Oct 15, 2021 • edited Loading

Choose a reason for hiding this comment

ralexstokes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ralexstokes left a comment

Choose a reason for hiding this comment

hwwhww commented Oct 12, 2021 •

edited

Loading

hwwhww Oct 12, 2021 •

edited

Loading

djrtwo left a comment •

edited

Loading

hwwhww Oct 14, 2021 •

edited

Loading

hwwhww Oct 15, 2021 •

edited

Loading