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

AttestationData.epoch_boundary_root for epoch starting block #652

Closed
Nashatyrev opened this issue Feb 19, 2019 · 4 comments
Closed

AttestationData.epoch_boundary_root for epoch starting block #652

Nashatyrev opened this issue Feb 19, 2019 · 4 comments
Assignees
Labels
general:bug Something isn't working

Comments

@Nashatyrev
Copy link
Member

The Honest Validator spec here states that

Set attestation_data.epoch_boundary_root = hash_tree_root(epoch_boundary) where epoch_boundary is the block at the most recent epoch boundary in the chain defined by head -- i.e. the BeaconBlock where block.slot == get_epoch_start_slot(head.slot).

Note: This can be looked up in the state using get_block_root(state, get_epoch_start_slot(head.slot)).

I.e. according to the statement if an attester creates the attestation for the beacon block which slot is the first in an epoch, then the epoch_boundary_root should be the root of the atteted beacon block itself.
But:

  1. get_block_root(state, get_epoch_start_slot(head.slot)) will fail due to assertion assert slot < state.slot
  2. Main spec describes the epoch_boundary_root as an ancestor of the beacon block in attestation, thus it shouldn't be the root of the block itself
AttestationData
    .....
    # Hash of root of the ancestor at the epoch boundary
    'epoch_boundary_root': 'bytes32',
Nashatyrev added a commit to harmony-dev/beacon-chain-java that referenced this issue Feb 19, 2019
@hwwhww hwwhww added the general:bug Something isn't working label Feb 19, 2019
@ChihChengLiang
Copy link
Contributor

Can confirm this issue, I encountered it when mocking attestations.

@hwwhww
Copy link
Contributor

hwwhww commented Feb 21, 2019

Hey @Nashatyrev, I noticed that you’re not referencing the latest dev branch spec so I interpret the issue again. 🙂

1. The definition of AttestationData.epoch_boundary_root

  1. Spec: Root of the ancestor at the epoch boundary

  2. V-guide (https://github.com/ethereum/eth2.0-specs/blob/f0b562054fa526322956500f3a6e19ed9007ce5d/specs/validator/0_beacon-chain-validator.md#epoch-boundary-root):

    ##### Epoch boundary root
    Set `attestation_data.epoch_boundary_root = hash_tree_root(epoch_boundary)` where `epoch_boundary` is the block at the most recent epoch boundary in the chain defined by `head` -- i.e. the `BeaconBlock` where `block.slot == get_epoch_start_slot(slot_to_epoch(head.slot))`. 
    
    /_Note:_/ This can be looked up in the state using `get_block_root(state, get_epoch_start_slot(slot_to_epoch(head.slot)))`.
    
  3. It seems when slot=64, the epoch_boundary_root would be 64, which is somehow not accurate to “Root of the ancestor at the epoch boundary”. /cc @vbuterin Am I correct that after we change the epoch boundary, it is different from the mini-spec now and it’s more like an epoch_start_root field?

2. get_block_root(state, slot) slot validation

It fails since both state.slot and the given slot are 64.

Proposed solution:

Fix the note by:

  • Set epoch_start_slot = get_epoch_start_slot(slot_to_epoch(head.slot))
  • Set epoch_boundary_root to hash_tree_root(head) if epoch_start_slot == head.slot else get_block_root(state, epoch_start_slot)

/cc @djrtwo

@djrtwo djrtwo self-assigned this Feb 21, 2019
@djrtwo
Copy link
Contributor

djrtwo commented Feb 21, 2019

It seems that the spec semantically uses "ancestor" of a block to mean any block in the chain defined by the block at or before the block. See get_ancestor in the fork choice -- this function treats the meaning of "ancestor" in this way.

Unfortunately, the calling get_block_root from the state only allows access to blocks previous to the head. This might change if we figure out and merge #649, but until then @hwwhww's fix looks correct, and I'll get it in the doc today.

@djrtwo
Copy link
Contributor

djrtwo commented Apr 17, 2019

handled in #671

@djrtwo djrtwo closed this as completed Apr 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
general:bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants