Skip to content
This repository has been archived by the owner on Jul 1, 2021. It is now read-only.

Rework state finality fields #478

Merged
merged 6 commits into from
Apr 6, 2019

Conversation

hwwhww
Copy link
Contributor

@hwwhww hwwhww commented Apr 1, 2019

What was wrong?

part 2 of #418

How was it fixed?

  1. BeaconState fields
    • Rename
      • justified_epoch -> current_ justified_epoch
    • Add
      • previous_justified_root
      • current_justified_root
      • finalized_root
  2. Now the attestation validation also validates previous_justified_root. Combine validate_attestation_source_epoch and validate_attestation_source_root into validate_attestation_source_epoch_and_root.
  3. Update _get_finalized_epoch flow as the spec
  4. Update validator logic
    • Use state.current_justified_root as source_root
    • Use apply_state_transition_without_block

Cute Animal Picture

Elephant mammal wild, animals

"""
Validate ``source_epoch`` field of ``attestation_data``.
Validate ``source_epoch`` and ``source_root`` fields of ``attestation_data``.
Raise ``ValidationError`` if it's invalid.
"""
if slot_to_epoch(attestation_data.slot + 1, slots_per_epoch) >= current_epoch:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that + 1 will be removed when we have #391

Copy link
Contributor

@ChihChengLiang ChihChengLiang left a comment

Choose a reason for hiding this comment

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

Justification and finalization parts looks good

Copy link
Member

@ralexstokes ralexstokes left a comment

Choose a reason for hiding this comment

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

looks great! left a few comments about code but logic looks great :)

Raise ``ValidationError`` if it's invalid.
"""
if slot_to_epoch(attestation_data.slot + 1, slots_per_epoch) >= current_epoch:
if attestation_data.source_epoch != justified_epoch:
if attestation_data.source_epoch != state.current_justified_epoch:
Copy link
Member

Choose a reason for hiding this comment

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

could be nice to break this body out into a function (used here and in the body of the else arm) but i think this is fine for now. i would say it is required if we end up nesting another level, i.e. if inside if inside if.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed that would be ideal but I'm inclined to keep it simpler for now. i.e., two-level if seems "acceptable" right now.

Added some minor comment/formatting in b8a0843

crosslink_committees_at_slot = get_crosslink_committees_at_slot(
# To avoid the epoch boundary cases
state.copy(
slot=state.slot + 1,
slot=state.slot,
Copy link
Member

Choose a reason for hiding this comment

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

this is unnecessary now that we are not modifying the slot, correct?

Copy link
Member

Choose a reason for hiding this comment

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

state pre-copy and state post-copy are the same

@hwwhww hwwhww merged commit 45d5e17 into ethereum:master Apr 6, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants