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

Add additional get_proposer_head test coverage #3633

Open
wants to merge 3 commits into
base: dev
Choose a base branch
from

Conversation

eserilev
Copy link

@eserilev eserilev commented Mar 25, 2024

Currently clients can potentially pass all get_proposer_head spec tests without having a fully spec compliant implementation. This PR attempts to improve get_proposer_head test coverage. A new test_parent_weak test is added to check for the following scenario:

  • is_head_late, is_shuffling_stable, is_shuffling_stable, is_finalization_ok, is_proposing_on_time is_head_weak all evaluate to true
  • is_parent_strong evaluates to false

In this situation, the parent block doesn't have enough attestation votes and therefore the head root is returned from get_proposer_head. This new test is pretty much a copy paste of test_basic_is_parent_root with a few minor tweaks.

There are several more tests needed to further improve test coverage. This is my first time writing a consensus spec test so I'm looking for some feedback before writing additional tests.

@eserilev eserilev force-pushed the add-additional-fork-choice-test branch 2 times, most recently from 6ec394e to 672f29f Compare March 25, 2024 15:21
@eserilev eserilev force-pushed the add-additional-fork-choice-test branch from 672f29f to 86e279a Compare March 25, 2024 20:16
@hwwhww
Copy link
Contributor

hwwhww commented Mar 28, 2024

Thank you @eserilev! Have you run this new test vector with Lighthouse?

@eserilev
Copy link
Author

@hwwhww yes I have. On Lighthouse unstable this new test vector fails, but on this branch the test passes

@michaelsproul
Copy link
Contributor

@hwwhww The test passes on LH unstable now so we are happy to merge if you are! Thanks

Copy link
Contributor

@ensi321 ensi321 left a comment

Choose a reason for hiding this comment

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

Ran this test on Lodestar. Can confirm everything up to and include is_head_weak() are evaluated to True and is_parent_strong() is evaluated to False.

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.

4 participants