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

Use attestation epoch in beacon fork choice rule #1394

Closed
JustinDrake opened this issue Sep 3, 2019 · 3 comments
Closed

Use attestation epoch in beacon fork choice rule #1394

JustinDrake opened this issue Sep 3, 2019 · 3 comments
Labels
post-freeze (substantive) Substantive consensus change non-critical for long-lived cross-client testnets scope:fork-choice

Comments

@JustinDrake
Copy link
Contributor

Currently the fork choice rule ignores the attestation epoch. The suggestion (from @djrtwo here) is to replace

if i not in store.latest_messages or target.epoch > store.latest_messages[i].epoch:
    store.latest_messages[i] = LatestMessage(epoch=target.epoch, root=attestation.data.beacon_block_root)

with

attestation_epoch = compute_epoch_of_slot(get_attestation_data_slot(attestation.data))
if i not in store.latest_messages or attestation_epoch > store.latest_messages[i].epoch:
    store.latest_messages[i] = LatestMessage(epoch=attestation_epoch, root=attestation.data.beacon_block_root)

In addition, the suggestion is to add slot to AttestationData.

@JustinDrake JustinDrake added scope:fork-choice post-freeze (substantive) Substantive consensus change non-critical for long-lived cross-client testnets labels Sep 3, 2019
@mkalinin
Copy link
Contributor

mkalinin commented Sep 4, 2019

Shouldn't attestation_epoch and attestation.data.target.epoch be the same thing?

According to 0_beacon-chain-validator.md#ffg-vote target.epoch is the current epoch calculated for the state being attested to.

@mkalinin
Copy link
Contributor

It looks like attestation.data.slot is enforced to be of target.epoch since #1509 PR is merged. We may close this issue I guess.

@djrtwo
Copy link
Contributor

djrtwo commented Dec 12, 2019

yup, good to go

@djrtwo djrtwo closed this as completed Dec 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
post-freeze (substantive) Substantive consensus change non-critical for long-lived cross-client testnets scope:fork-choice
Projects
None yet
Development

No branches or pull requests

3 participants