Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix fork choice on_block tests and update test format #2577

Merged
merged 5 commits into from
Sep 8, 2021

Conversation

hwwhww
Copy link
Contributor

@hwwhww hwwhww commented Aug 31, 2021

  1. Update test format of checks: add checkpoint epoch number and rephrase them in dict. Example:
- checks:
    time: 192
    head: {slot: 32, root: '0xdaa1d49d57594ced0c35688a6da133abb086d191a2ebdfd736fad95299325aeb'}
    justified_checkpoint: {epoch: 3, root: '0xc25faab4acab38d3560864ca01e4d5cc4dc2cd473da053fbc03c2669143a2de4'}
    finalized_checkpoint: {epoch: 2, root: '0x40d32d6283ec11c53317a46808bc88f55657d93b95a1af920403187accf48f4f'}
    best_justified_checkpoint: {epoch: 3, root: '0xc25faab4acab38d3560864ca01e4d5cc4dc2cd473da053fbc03c2669143a2de4'}
  1. Fix test_new_justified_is_later_than_store_justified: there was a missing tick step.
  2. Update test_new_finalized_slot_is_justified_checkpoint_ancestor: remove the useless tick step.
  3. Rewrite test_on_block_finalized_skip_slots_not_in_skip_chain and test_on_block_finalized_skip_slots:

@hwwhww
Copy link
Contributor Author

hwwhww commented Aug 31, 2021

@ajsutton @tuyennhv

If you'd like to try the updated test vectors, I dumped new fork choice test vectors to: https://github.com/hwwhww/consensus-spec-tests/tree/fork-choice-pr2577 (b23ed05 version)

p.s. It doesn't fix the test_new_finalized_slot_is_justified_checkpoint_ancestor issue.

@hwwhww hwwhww requested a review from djrtwo August 31, 2021 14:54
@hwwhww
Copy link
Contributor Author

hwwhww commented Sep 2, 2021

Update: @tuyennhv confirmed that Lodestar passed all fork choice tests against this new branch. 🙌

@ajsutton
Copy link
Contributor

ajsutton commented Sep 2, 2021

Sorry got distracted from this. I've never worked out how to build the actual test vectors from the source (only got as far as running them against the spec implementation), do you have instructions for that or is there a tarball somewhere?

@hwwhww
Copy link
Contributor Author

hwwhww commented Sep 2, 2021

@ajsutton

fork_choice tests only tarballs:


If you'd like to download from source, it required Git LFS installed.

git lfs install

If LFS has been installed correctly, git clone should download the large files by default:

git clone git@github.com:hwwhww/consensus-spec-tests.git -b fork-choice-pr2577 --single-branch
cd consensus-spec-tests

Or you can trigger it by

git lfs pull

The test vectors will be under consensus-spec-tests/tests folder.

Copy link
Contributor

@djrtwo djrtwo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i got a little hung up on what was actually being tested in the valid case, but figured it out!

Looks correct to me


# Now build a block at later slot than finalized *epoch*
# Includes finalized block in chain and the skipped slots
block = build_empty_block_for_next_slot(spec, target_state)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This block is valid wrt the finalized epoch (including the skip slots), but this block isn't the head, right?

apply_next_epoch_with_attestations for epoch 3 and 4 will have created blocks of much higher weight and win out

So -- Does adding this block change anything? or does it catch a regression in the prior fork choice implementations?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and if block is supposed to be the head? can we check that it is against a dynamic call to get_head()?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see. It's just checking in on_block can even be run anymore which would yield valid: False if it didn't work

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see. It's just checking in on_block can even be run anymore which would yield valid: False if it didn't work

Right, it is for comparing with test_on_block_finalized_skip_slots_not_in_skip_chain (valid: False) test case.

@hwwhww hwwhww merged commit d23444a into dev Sep 8, 2021
@hwwhww hwwhww deleted the fix-fork-choice-tests branch September 8, 2021 13:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants