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 ex-ante fork choice test cases #2752

Merged
merged 19 commits into from
Dec 12, 2021
Merged

Add ex-ante fork choice test cases #2752

merged 19 commits into from
Dec 12, 2021

Conversation

hwwhww
Copy link
Contributor

@hwwhww hwwhww commented Nov 30, 2021

Thank @casparschwa @djrtwo for the testing plan: https://notes.ethereum.org/m1upI_XcRpSskeG4UvgNtA?view

Differences:

  • I combined Scenario 1 and Scenario 2 since the only difference is Step 4.
  • [Adversarial attestations > proposer boost] I just use full attestation instead of exact proposer_boost + 1 votes.

It's still verbose. Will think about refactoring tomorrow.

TODOs:

@hwwhww
Copy link
Contributor Author

hwwhww commented Dec 2, 2021

Discussion: about the test vector release

This PR now sets configuration PROPOSER_SCORE_BOOST:= 0 in some test cases via @spec_configured_state_test decorator. I'd suggest that we release these PROPOSER_SCORE_BOOST:= 0 test vectors separately from the regular test vector releases.

Reasons:

  • It is only useful for debugging + clients haven't implemented the proposer score boost feature. These test cases can be removed later.
  • It introduces a run-time configuration overridden in clients' test suite. Probably not worth complexity for just this case.

/cc @ralexstokes @casparschwa @djrtwo

@djrtwo
Copy link
Contributor

djrtwo commented Dec 2, 2021

I'm okay not releasing proposer_score_boost 0. But it's good to just write a helper that allows for creating attestations as a function of proposer_score_boost + 1. This allows us to easily test the boundaries around proposer score boost

(Which is like our tests when score boost = 0 and there is 1 conflicting attstaiton)

@hwwhww
Copy link
Contributor Author

hwwhww commented Dec 6, 2021

@djrtwo good points. I made test_ex_ante_attestations_is_greater_than_proposer_boost_* cases be tested with minimum attestation participant count such that attestation_score > proposer_score now.

if we can release separately, I won't have to change the test format & do deeper refactoring since half of these cases will be deleted.

@hwwhww hwwhww changed the title [WIP] Add ex-ante fork choice test cases Add ex-ante fork choice test cases Dec 9, 2021
@hwwhww hwwhww marked this pull request as ready for review December 9, 2021 10:14
@hwwhww
Copy link
Contributor Author

hwwhww commented Dec 9, 2021

@ralexstokes @djrtwo

I deleted PROPOSER_SCORE_BOOST: 0 tests from this PR, and created another PR for PROPOSER_SCORE_BOOST: 0: #2767 tests. I suggest that:

  1. we only merge this PR to the next release.
  2. Generate PROPOSER_SCORE_BOOST: 0 special test vector tarball files from Add test cases with configured PROPOSER_SCORE_BOOST:= 0 #2767 separately.

@hwwhww hwwhww requested a review from adiasg December 9, 2021 10:19
Copy link
Member

@casparschwa casparschwa left a comment

Choose a reason for hiding this comment

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

Mostly on formatting consistency

@with_all_phases
@with_presets([MAINNET], reason="to create non-duplicate committee")
@spec_state_test
def test_ex_ante_attestations_is_greater_than_proposer_boost_with_boost(spec, state):
Copy link
Member

Choose a reason for hiding this comment

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

Given that we only have tests with boost now, I propose to remove the with_boost naming convention.

Suggested change
def test_ex_ante_attestations_is_greater_than_proposer_boost_with_boost(spec, state):
def test_ex_ante_attestations_is_greater_than_proposer_boost(spec, state):

hwwhww and others added 2 commits December 11, 2021 09:01
Co-authored-by: Caspar Schwarz-Schilling <31305984+casparschwa@users.noreply.github.com>
@hwwhww hwwhww merged commit 4cd2334 into dev Dec 12, 2021
@djrtwo djrtwo deleted the ex-ante-tests branch January 17, 2022 21:43
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