fork-choice test vectors: starting with `get_head` tests #2202

hwwhww · 2021-02-19T05:25:45Z

I’m transforming the current pyspec fork choice test cases into test vectors. 🧶

This PR demonstrates a proposal based on @ericsson49's Fork-choice integration test format and example (WIP) consensus-spec-tests#17 format, but with some adjustments so that it's more friendly to pyspec utils.
Starting with get_head helper. test/phase0/fork_choice/test_get_head.py is basically copied from test/phase0/unittests/fork_choice/test_get_head.py, but with some extra yield to output the test vectors. Minimal config get_head test vectors are here.

Test formats

anchor_state.yaml and anchor_block.yaml: the inputs of get_forkchoice_store(anchor_state: BeaconState, anchor_block: BeaconBlock), to initialize the store.
- Note: Fork-choice integration test format and example (WIP) consensus-spec-tests#17 uses genesis. But in our case, the anchor state may not be genesis.
block_<block_root>.yaml: SignedBeaconBlock object
attestation_<attestation_root>.yaml: Attestation object
steps.yaml: the execution steps. Basically similar to Fork-choice integration test format and example (WIP) consensus-spec-tests#17 "steps". The member elements may be:
- tick: <time>: integer, the time for calling on_tick(store: Store, time: uint64)
  - Note: Fork-choice integration test format and example (WIP) consensus-spec-tests#17 uses slot.
- block: <block_file_name>: the SignedBeaconBlock object for calling on_block(store: Store, signed_block: SignedBeaconBlock)
- attestation: <attestation_file_name>: the Attestation object for calling on_attestation(store: Store, attestation: Attestation)
- checks dict:
  - head: the block root when calling get_head(store: Store)
  - justified_checkpoint_root: the hash tree root of Store.justified_checkpoint.
- Example: filtered_block_tree/steps.yaml

Discussions

Any suggestions on the above proposal?
It's easy to convert get_head tests. However, in our on_block tests, we directly set the fields of Store to prepare the tests. For example:
https://github.com/ethereum/eth2.0-specs/blob/8ec082fcf9ca58bf143977abb4dce9c8c417dbdd/tests/core/pyspec/eth2spec/test/phase0/unittests/fork_choice/test_on_block.py#L30-L31
https://github.com/ethereum/eth2.0-specs/blob/8ec082fcf9ca58bf143977abb4dce9c8c417dbdd/tests/core/pyspec/eth2spec/test/phase0/unittests/fork_choice/test_on_block.py#L129-L132
I may have to rewrite them to get rid of the Store field manipulations if we are going to follow the above test formats.
Alternatively, we can output the whole store object. However, some drawbacks:
- Store object could be really huge. We probably can only output Minimal config tests.
- Client teams may have implemented the concept of Store in a very different, optimized solution.

20210313 updated:

See fork choice test format doc

ericsson49 · 2021-02-19T11:19:02Z

When I was designing my fork choice tests, I avoided explicit instantiation of Store objects, for the same reasons.
My fork choice tests are integration tests rather than unit tests, so I use sequences of tick, on_block and on_attestation events (along with initial state) to reach desired Store configurations.
That's of course more tricky, but with the approach, one is making less assumptions about implementation details. Which I believe is important for such kind of tests, as it can hinder implementer's optimizations or similar.

While any implementation should know how to handle on_tick, on_block and on_attestation, so it shouldn't be a big deal to integrate such tests with an existing implementation. I validated that while integrating my tests with Teku.

djrtwo · 2021-02-22T15:51:18Z

The direct store manipulations are to avoid very inefficient state transitions and state copies found in on_block. We need to be careful not to inflate CI running time too much. A couple of options
1. mod them and take the hit on CI. Might be quite a hit though
2. put a conditional in the helpers that does full calls to on_block, etc, when running testgen, but store manipulations on CI. Do we have a conditional that can detect context? This is nice, but then there is a worry that the logic isn't 1:1 and some bug surfaces during testgen that you didn't catch in CI
3. write some separate testgen scenarios that only run during testgen and no CI. Probably better than (2) but still has the issue that something might surface during the rarely run testgens

ericsson49 · 2021-02-24T08:57:30Z

To reduce test vector generation time, I have been using either a caching BLS implementation or a dummy BLS implementation (which is completely insecure, but still able to distinguish good and bad signatures).
Actually, for most fork choice tests one can switch off BLS checks, with a a couple of exceptions (wrong block or attestation signatures).

djrtwo · 2021-02-25T00:05:10Z

Right, and we have a test function decorator never_bls to do just that. The state trnasitions themselves can be very expensive in the unoptimized python as well though

djrtwo · 2021-03-11T21:44:29Z

So do we leave the fork choice unit tests untouched?

djrtwo

awesome! only minor comments

Should we remove the old unittest test_get_head file? This new file is strictly the previous functionality + generators, right?

tests/formats/fork_choice/README.md

Co-authored-by: Danny Ryan <dannyjryan@gmail.com>

protolambda

Left some minor comments, looks good to me, nice work 👍

tests/formats/fork_choice/README.md

tests/core/pyspec/eth2spec/test/phase0/fork_choice/test_get_head.py

tests/formats/fork_choice/README.md

hwwhww · 2021-03-12T15:11:05Z

tests/core/pyspec/eth2spec/test/phase0/fork_choice/test_get_head.py

-    for attestation in attestations:
-        spec.on_attestation(store, attestation)


Note: run_on_block will run on_attestation for each attestation from block.message.body.attestations.

hwwhww · 2021-03-12T15:12:56Z

tests/core/pyspec/eth2spec/test/phase0/fork_choice/test_get_head.py



 @with_all_phases
 @spec_state_test
+@with_configs([MINIMAL], reason="too slow")


Note: the test generation time of other tests is acceptable, for me.

hwwhww · 2021-03-12T15:14:19Z

tests/formats/fork_choice/README.md

+    - For each execution, look up the corresponding ssz_snappy file. Execute the corresponding helper function on the current store.
+        - For the `on_block` execution step: if `len(block.message.body.attestations) > 0`, execute each attestation with `on_attestation(store, attestation)` after executing `on_block(store, block)`.


Note: on_block execution step implies running on_attestation.

Note that to execute on_attestation after on_block Output more checking field Disable mainnet config test_filtered_block_tree Fix after rectoring + use more run_on_block Fix and refactor `tick_and_run_on_attestation`

djrtwo

lgtm!

Add get_head test vectors

27507fb

hwwhww added general:RFC Request for Comments scope:CI/tests/pyspec scope:fork-choice labels Feb 19, 2021

mratsim mentioned this pull request Mar 11, 2021

Eth2 Call 59 ethereum/eth2.0-pm#208

Closed

hwwhww added 5 commits March 11, 2021 23:45

Merge branch 'dev' into fork-choice-test-vectors

96e2984

Fix generator to ssz_snappy mode

8e303b6

Add fork choice rule format doc

78a4897

Merge branch 'dev' into fork-choice-test-vectors

216bbb0

Fix conflicts

9254f1b

hwwhww marked this pull request as ready for review March 11, 2021 16:52

hwwhww requested review from djrtwo and protolambda March 11, 2021 17:01

djrtwo reviewed Mar 11, 2021

View reviewed changes

hwwhww and others added 2 commits March 12, 2021 20:40

Apply suggestions from @djrtwo code review

23b28b6

Co-authored-by: Danny Ryan <dannyjryan@gmail.com>

Remove the duplicate get_head unit test

d29926e

protolambda reviewed Mar 12, 2021

View reviewed changes

tests/formats/fork_choice/README.md Outdated Show resolved Hide resolved

tests/formats/fork_choice/README.md Outdated Show resolved Hide resolved

tests/core/pyspec/eth2spec/test/phase0/fork_choice/test_get_head.py Show resolved Hide resolved

protolambda reviewed Mar 12, 2021

View reviewed changes

tests/formats/fork_choice/README.md Outdated Show resolved Hide resolved

protolambda reviewed Mar 12, 2021

View reviewed changes

tests/formats/fork_choice/README.md Outdated Show resolved Hide resolved

hwwhww commented Mar 12, 2021

View reviewed changes

hwwhww force-pushed the fork-choice-test-vectors branch from 09fc717 to ee82203 Compare March 12, 2021 16:23

hwwhww force-pushed the fork-choice-test-vectors branch from ee82203 to e77ba91 Compare March 12, 2021 16:29

djrtwo approved these changes Mar 12, 2021

View reviewed changes

hwwhww merged commit 5dcc992 into dev Mar 13, 2021

hwwhww deleted the fork-choice-test-vectors branch March 13, 2021 04:08

dapplion mentioned this pull request Mar 19, 2021

Add fork choice tests ChainSafe/lodestar#1472

Closed

mratsim mentioned this pull request Apr 13, 2021

Fork Choice: EF test vectors integration status-im/nimbus-eth2#777

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fork-choice test vectors: starting with `get_head` tests #2202

fork-choice test vectors: starting with `get_head` tests #2202

hwwhww commented Feb 19, 2021 •

edited

Loading

ericsson49 commented Feb 19, 2021 •

edited

Loading

djrtwo commented Feb 22, 2021

ericsson49 commented Feb 24, 2021

djrtwo commented Feb 25, 2021

djrtwo commented Mar 11, 2021

djrtwo left a comment

protolambda left a comment

hwwhww Mar 12, 2021

hwwhww Mar 12, 2021

hwwhww Mar 12, 2021

djrtwo left a comment

		for attestation in attestations:
		spec.on_attestation(store, attestation)

		- For each execution, look up the corresponding ssz_snappy file. Execute the corresponding helper function on the current store.
		- For the `on_block` execution step: if `len(block.message.body.attestations) > 0`, execute each attestation with `on_attestation(store, attestation)` after executing `on_block(store, block)`.

fork-choice test vectors: starting with get_head tests #2202

fork-choice test vectors: starting with get_head tests #2202

Conversation

hwwhww commented Feb 19, 2021 • edited Loading

Test formats

Discussions

20210313 updated:

ericsson49 commented Feb 19, 2021 • edited Loading

djrtwo commented Feb 22, 2021

ericsson49 commented Feb 24, 2021

djrtwo commented Feb 25, 2021

djrtwo commented Mar 11, 2021

djrtwo left a comment

Choose a reason for hiding this comment

protolambda left a comment

Choose a reason for hiding this comment

hwwhww Mar 12, 2021

Choose a reason for hiding this comment

hwwhww Mar 12, 2021

Choose a reason for hiding this comment

hwwhww Mar 12, 2021

Choose a reason for hiding this comment

djrtwo left a comment

Choose a reason for hiding this comment

fork-choice test vectors: starting with `get_head` tests #2202

fork-choice test vectors: starting with `get_head` tests #2202

hwwhww commented Feb 19, 2021 •

edited

Loading

ericsson49 commented Feb 19, 2021 •

edited

Loading