off-by-one validating justified_epoch for attestations

[arnetheduck]

3d5aa35

Currently, spec has this rule for validating attestations:

Verify that attestation.data.justified_epoch is equal to state.justified_epoch if attestation.data.slot >= get_epoch_start_slot(get_current_epoch(state)) else state.previous_justified_epoch.

Epoch transitions happen at (state.slot + 1) % EPOCH_LENGTH == 0. This means that when attestation.data.slot % EPOCH_LENGTH == 0, the epoch state update will not yet have happened but get_epoch_start_slot will already return a slot from the new epoch, and this check will fail due to an off-by-one.. it can be fixed by an extra + 1:

Verify that attestation.data.justified_epoch is equal to state.justified_epoch if attestation.data.slot + 1 >= get_epoch_start_slot(get_current_epoch(state)) else state.previous_justified_epoch.

It should be noted however that this feels like a workaround - the deeper issue here is that get_current_epoch returns the new epoch one slot earlier than the justified_epoch update happens, so we have a situation where according to one function, we are in a new epoch, but state has not yet been updated to include the changes that pertain to that epoch.

[arnetheduck]

related commit: f943361

[djrtwo]

It would be much cleaner to make this epoch based and arguably should have been done in the epoch rework

Verify that attestation.data.justified_epoch is equal to state.justified_epoch if slot_to_epoch(attestation.data.slot) >= get_current_epoch(state) else state.previous_justified_epoch.

If the attestation is included in the epoch during the slot it was for, then it uses justified_epoch. If not, it is from the prev, and uses previous_justified_epoch

[djrtwo]

This is clean because we don't actually transition into the next upon until the end of the state transition. so when processing the block, the current epoch check is valid

[djrtwo]

This means that when attestation.data.slot % EPOCH_LENGTH == 0, the epoch state update will not yet have happened but get_epoch_start_slot will already return a slot from the new epoch, and this check will fail due to an off-by-one.. it can be fixed by an extra + 1:

I'm not sure I follow here

• An attestation withattestation.data.slot % EPOCH_LENGTH == 0 can at earliest be included in a block at the next slot (assuming MIN_ATTESTATION_INCLUSION_DELAY == 1
• At the next slot state.slot % EPOCH_LENGTH == 1, the epoch start slot of each is that of the attestation, and the epoch of each is that of the attestation. So a.data.justified_epoch == state.justified_epoch

[arnetheduck]

Ok, I replayed this with a bit more detail this time:

• we receive block 131 that contains attestations for block 127
• we start processing the block, so state.slot has advanced to 131 as well
• get_current_epoch(state) returns 2
• get_epoch_start_slot(2) = 128
• 127 < 128, so we're looking at ``previous_justified_epoch` per condition, which at this point is 0

Now, let's teleport back to when the attestation is created:

• we've just run the epoch transition, because 127+1 % 64 == 0
• we've set state.justified_epoch to 1 because.. we just ran a epoch transition that allowed this, assuming a majority vote
• we set attestation.justified_epoch = state.justified_epoch

thus, when creating the attestation, the epoch processing is already done while the condition fails to capture this and uses the previous epoch instead... does this make sense? my original description looks a bit off here in terms of where exactly the off-by-one happens

the proposed fix, to compare using epoch seems to run into the same

[djrtwo]

I see now. thank you

The problem here is that an attester is attesting to the post state of the block. Which when that block is at the end of the epoch includes the epoch transition.

You're right, your proposal is the simplest way to clean it up. Moving the epoch transition to the start of the 0th slot of an epoch cleans this issue up as well, but has some rippling effects in other places. Going to mull it over a bit