Forks longer than two epochs cause invalid blocks to be produced

[michaelsproul]

Description

In #820 we disabled the signature check on attestations when drawing them from the op pool, and this saved us a bunch of CPU cycles during block production. However, it also introduced a bug whereby if there are two chains that stay forked for longer than 2 epochs, then their committee shufflings will diverge and the newest attestations from each side will not be valid to include in the opposing chain (their signatures will appear invalid, because for the same slot and index, the two sides will have different committees).

The example I stumbled on was: 4 epochs of blocks in common, with the last common block at slot 32, start of epoch 4 (note: SLOTS_PER_EPOCH = 8). Then 2 epochs of forking, with one fork ending with a block at slot 48 and the other with a block at slot 49. The block produced on the 2nd fork at slot 49 was invalid, because it included an attestation from the 1st fork from slot 48, and by this point, the committee shuffling for the 2nd fork at slot 48 was completely different to the committee shuffling for the 1st fork at slot 48.

Steps to resolve

I checked that re-enabling signature checks fixes the problem (it does), but this seems like a heavy-handed solution. I think we can do better by checking that the attestation's head block descends from a common ancestor of the state that we're producing the block upon, although doing this efficiently might be a bit annoying, because we'd have to load an extra state...

[paulhauner]

So there's this check:

// `state` is the BeaconState of the block being produced, it should already be loaded.

let target_a = state.get_block_root(state.current_epoch().start_slot());
let target_b = state.get_block_root(state.previous_epoch().start_slot());

if attn.data.target.root == target_a || attn.data.target.root == target_b {
  // Accept.
} else {
  // Reject.
}

This would ensure that we never include any blocks with a distinct shuffling, however it would unnecessarily restrict blocks that might have forked in state.current_epoch() - 3 and still provide some reward to the block proposer for having a "correct" attn.data.source.

This would be a bit of hot-fix that moves us from a glaring liveness issue to a profit maximization one (with potential delayed finality).

[michaelsproul]

I don't quite follow what you mean about a fork from state.current_epoch() - 3, I think rejecting attestations from such long-ranged forks is exactly what we want to do (because their signatures will appear invalid).

I think the problem might be that it's too strict for short-ranged forks. If a fork occurs on the first slot of an epoch, then the attestations from both sides will be unnecessarily excluded. Maybe that's OK as a hotfix?

[paulhauner]

Oh yep, sorry I should have said state.current_epoch() - 2. My suggestion filters out those forks unnecessarily.

[michaelsproul]

I'm starting work on this