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

Some fixes to finality tests #1228

Merged
merged 12 commits into from
Jun 29, 2019
Merged

Some fixes to finality tests #1228

merged 12 commits into from
Jun 29, 2019

Conversation

djrtwo
Copy link
Contributor

@djrtwo djrtwo commented Jun 29, 2019

Fix finality_12. This should be a good base to fix the others.

  • need to set the associated epoch boundary block roots to ensure that get_matching_target_ works
  • justification bit 0 should be set at first so it gets shift to position 1. Then 0 gets justified and you have 0 and 1 for finality.
  • aggregation_bitfield -> aggregation_bits in add_mock_attestations (use bool array for init)
  • add_mock_attestations had bug, setting target=source. fixed
  • add_mock_attestations sets shard in attestation.data.crosslink. This piece of data is required to figure out what committee is attesting. Previous, all were defaulting to zero so each committee added looked the same.

@djrtwo djrtwo force-pushed the cov-hunt-fix-ish branch from 14d4176 to f484b1e Compare June 29, 2019 05:21
@CarlBeek CarlBeek changed the title some fixes to finality_12 Some fixes to finality Jun 29, 2019
@CarlBeek CarlBeek changed the title Some fixes to finality Some fixes to finality tests Jun 29, 2019

old_finalized = state.finalized_checkpoint
state.previous_justified_checkpoint = c3
state.previous_justified_checkpoint = c4
Copy link
Contributor

Choose a reason for hiding this comment

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

It feels strange to do this as it is not the behaviour one would expect from this test. The problem, however, is that otherwise, a new checkpoint is finalised as the condition of "2nd/3rd most recent epochs are justified, the 2nd using the 3rd as source" is met. This means that in the case where there is insufficient support, assert state.finalized_checkpoint == old_finalized # no new finalized fails. @djrtwo, please can you check me here (and propose an alternative solution if you have one.)

TLDR: this is required because of an artifact of how the tests are constructed by manipulating what is and isn't justified in the state.

@CarlBeek
Copy link
Contributor

I extended these finalisation fixes to all of the finalisation cases. Fixes include:

  • all of the boundry_epochs are now set (needed for the correct calculation of attestations).
  • Many list-slicing bugs
  • correction of what is justified in each of the tests.

… committees per slot, and improve bitfield index descriptions
@protolambda
Copy link
Collaborator

Fixed the attestation creation, since it was broken on mainnet config (multi committees per slot). And also reworded the bitfield indices and justification, it's clear now.

@protolambda protolambda merged commit b0510fe into cov-hunt Jun 29, 2019
@protolambda protolambda deleted the cov-hunt-fix-ish branch June 29, 2019 16:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants