Merge attestation verification logic

[vbuterin]

Also rename slashable attestation to indexed attestation to reflect its broader functionality in phase 1.

[hwwhww]

I like this reorg! 👍

IMHO the name StandaloneAttestation is still kind of confusing and hard to link to "standalone-verifiable". Even VerifiableAttestation is more understandable than StandaloneAttestation.

[vbuterin]

I still like StandaloneAttestation; technically both are verifiable, one is just verifiable with less auxiliary info than the other 😄

Let's argue about it in the next call?

[hwwhww]

To confirm with @vbuterin:

1. I assumed that before this conversion, the committee size of this Attestation.custody_bitfield is the committee of the given crosslink committee - but actually, the size of Attestation.custody_bitfield is not restricted in the dev branch.
2. And after this conversion, in verify_standalone_attestation, it expects that the committee
   size of StandaloneAttestation.custody_bitfield is len(StandaloneAttestation.validator_indices)?

The benefit of having len(Attestation.custody_bitfield) == len(Attestation.aggregation_bitfield) == (len(crosslink_committee) + 7) // 8 is that we can simply merge the bitfields with or operator when aggregating
many Attestations. If the size of len(Attestation.custody_bitfield) != (len(crosslink_committee) + 7) // 8, it seems it would add additional overhead to generate the new custody_bitfield?

[vbuterin]

I assumed that before this conversion, the committee size of this Attestation.custody_bitfield is the committee of the given crosslink committee - but actually, the size of Attestation.custody_bitfield is not restricted in the dev branch.

An attestation committee will not exceed 4096 already because that's the "all ETH is validating" threshold. In any case, having the same threshold in both regular attestations and standalone attestations seems necessary for slashing to be possible in any case.

it expects that the committee
size of StandaloneAttestation.custody_bitfield is len(StandaloneAttestation.validator_indices)?

You are right that the proposed change does increase strictness because the current approach requires the custody bitfield to be any quantity of zero bytes. I would argue again that the current approach is wrong and the new approach is correct.

[JustinDrake]

Brainstorming names:

• IndexedAttestation (suggesting that attester indices are provided)
• ExternalAttestation, ExogenousAttestation, ForkAttestation (suggesting the attestation can come from an external fork)

[djrtwo]

This custody_bitfield here is incorrect. We need to truncate the bitfield to be the ceildiv8of the size of the participants (validator_indices) and if we sort the validator_indices, the newly constructed custody_bitfield needs to reflect this sort order.

[djrtwo]

Otherwise it:

• fails verify_bitfield because the length of validator_indices can easily be less than the length of the original committee
• even if all validators participated the custody_bitfield, it is then in the incorrect order wrt validator_indices

[vbuterin]

The best I've thought of so far is to modify IndexedAttestation so that it has bit_0_validator_indices and bit_1_validator_indices, and get rid of the custody bitfield.

[djrtwo]

That will make IndexedAttestation inside of AttesterSlashing almost 2x in size.
Did you check out the fix I put in place?

[vbuterin]

That will make IndexedAttestation inside of AttesterSlashing almost 2x in size.

Why would it? The total size of the two lists would still equal the size of the original list.

[vbuterin]

I don't like the current proposed fix; adding two whole new functions in a PR that's supposed to be simplifying things feels very not-worth-it.

[djrtwo]

whoops, missed that you wanted to get rid of the original validator indices list.
Making the change, thanks!