add previous and current crosslinks

[djrtwo]

Somewhat deep change to address some fundamental bugs in how we handle crosslinks in state. Addresses the issues raised in #855 but through a simplified mechanism. Previous and current crosslinks are kept in state to ensure can always check attestation.previous_crosslink validity for attestations from previous and current epoch.

Changes:

• remove latest_crosslinks
• add current_crosslinks and previous_crosslinks -- this ensures that if an attestation is from previous or current epoch, the requisite previous_crosslink data is available in state to validate
• add previous_crosslink_root to Crosslink and strictly enforce that crosslinks must form a chain to prevent cases in which data is crosslinked and then overwritten.
• ensure that crosslink rewards are only given to attestations that did or could have formed a crosslink
• remove filter condition in process_crosslinks that checks for validity of previous_crosslink. This is unnecessary due to it already being checked in process_crosslink during block processing.
• add some tests for process_crosslinks

[djrtwo]

cc: @mkalinin

[mkalinin]

Looks great to me 👍

Works even better. Implemented and tested it in our simulator.
There are a few tiny things more that could be dropped out. Also, I think this PR should update other parts of the spec that rely on latest_crosslinks.

[vbuterin]

There is one edge case that worries me. What if, during epoch N+1, a crosslink created from attestations during epoch N is confirmed (during epoch N itself, not enough of the attestations were included, but during epoch N+1 some extra attestations get included bringing it above 2/3) and a crosslink created from attestations during epoch N+1 is confirmed? It seems like both would trigger and so the latest crosslink record would be overwritten twice? Are we sure there aren't weird edge cases here where some part of a shard chain somehow slips through without being part of a crosslink?

Also, do we have a hard invariant that for a crosslink to be overwritten, the latest crosslink record of the attestations making the new crosslink need to have the existing crosslink as the previous crosslink specified in the attestation?

[mkalinin]

Are we sure there aren't weird edge cases here where some part of a shard chain somehow slips through without being part of a crosslink?

There is an invariant in a block processing part. Attestations created in previous epoch must have a crosslink pointing to previous_crosslinks[shard], the same is for current epoch attestations and current_crosslinks list.
If any of current_crosslinks was overwritten twice during epoch transition this invariant wouldn't be broken, as attestations created in the next epoch would use current_crosslinks as a source for their previous crosslink argument, whatever value current_crosslinks[shard] would held.

Also, do we have a hard invariant that for a crosslink to be overwritten, the latest crosslink record of the attestations making the new crosslink need to have the existing crosslink as the previous crosslink specified in the attestation?

This PR removes previous crosslink verification from epoch transition. This verification happens only when attestation is being added to the state (during block processing).

[djrtwo]

It seems like both would trigger and so the latest crosslink record would be overwritten twice?

Yes, this can happen. It seems that this being able to happen is fundamental if we let both the prev and current epoch get crosslinked rather only attempting to crosslink from the previous epoch.

If this does happen, then it would be the case (before the epoch transition) that previous_crosslinks[shard] == current_crosslinks[shard]. In this case, both the previous and current epoch attestations for a shard have the same source crosslink so they would either have been (1) attempting to crosslink the same range of data on the shard or (2) the current attestations would be crosslinking from the same source but potentially a bit more data than the previous attestations. The second write of the crosslink in this scenario would strictly be crosslinking the same or more amount of data from the shard chain so not gaps.

If it were the case that the crosslinked data from the previous attestations is different than the crosslinked data from the current attestations, then the data vote from "current epoch attestations" would win within the context of the single state transition and externally nothing would be conflicting.

[vbuterin]

OK, I guess I'm ok with double-overwriting...

But the other weird edge case is that in epoch N, a crosslink A -> B is included, then in epoch N+1 the previous latest crosslink is still A, so A -> C could get included. This would require 1/3 slashing within the committee, but it potentially could happen. So then the invariant that the latest crosslink never gets "reverted", only "updated", gets violated.

I would recommend we consider just adding a "latest crosslink hash" as part of the crosslink object, and add a check that the crosslink is updated only if its latest crosslink hash matches. It would make analysis cleaner and resolve all of these weird edge cases.

[djrtwo]

Added the enforcement and prevents the corner case you described.

In the case that A->C attestation exists, I am still giving crosslink rewards atm. Most likely in this scenario, the A->C attestation was not malicious but just got messed up due to latency in processing. A bit torn on whether "best root" should be rewarded in this case. I think so.

This also does remove some optionality. In the case that A->B and A->C both come in during the same epoch, only A->B would be updated and A->C would be dropped. If we process the slots in reverse order A->C would be updated and A->B would be dropped. This is a bit indirect and strange so didn't add it in without first discussing

TODO: update validator guide once we are happy with this functionality.

[djrtwo]

This is ready for review

[JustinDrake]

A couple questions:

1. Now that get_winning_root considers the pending attestations in a single epoch (as opposed to both the previous and current epochs), can we replace range(get_epoch_start_slot(previous_epoch), get_epoch_start_slot(next_epoch)) by range(get_epoch_start_slot(current_epoch), get_epoch_start_slot(next_epoch))?
2. Especially if the answer to the above is "yes", can process_crosslinks and get_crosslink_deltas be merged?

[djrtwo]

process_crosslinks attempts to build crosslinks from attestations from both the previous and current epoch. It builds from the current "eagerly"

get_crosslink_deltas only ranges across previous epoch because we only reward for things once they have been given an entire epoch to be included.

[djrtwo]

A follow up, I think it is cleaner to keep the rewards section separate within epoch processing.

[djrtwo]

I'd like to get this merged asap.
looking for a review @JustinDrake @hwwhww

[hwwhww]

It now returns winning_root, previous_crosslink_root, participants. Alternatively, we can reuse Crosslink by creating Crosslink(epoch=FAR_FUTURE_EPOCH, crosslink_data_root=winning_root, previous_crosslink_root=previous_crosslink_root). So we can change it function to:

def get_crosslink_candidate_and_participants(state: BeaconState, slot: Slot, shard: Shard) -> Tuple[Crosslink, List[ValidatorIndex]]:

[JustinDrake]

That's what I did 👍

[hwwhww]

ditto, alternatively:

    crosslink_candidates = [Crosslink(epoch=FAR_FUTURE_EPOCH, crosslink_data_root=a.data.crosslink_data_root, previous_crosslink_root=a.data.previous_crosslink_root) for a in valid_attestations]

[JustinDrake]

Refactor idea:

• Replace current_crosslinks and previous_crosslinks with current_crosslink_roots and previous_crosslink_roots.
• Add shard to Crosslink
• Replace AttestationData.shard, AttestationData. previous_crosslink , AttestationData. crosslink_data_root by a Crosslink.

Possibly for another PR once this gets merged :)

[hwwhww]

@JustinDrake I added next_epoch back to process_crosslinks to avoiding mysterious + 1 in code. 😃