Require "seen" aggregates to be valid

[paulhauner]

Current the spec says that if you have ever seen an aggregate on gossip before then you should reject it.

This means that for every aggregate that is seen on the gossip network, you must store its identifier (hash/root) for one epoch. This is an unbounded cache.

This PR changes the condition such that you only store the identifier for the aggregate if it is valid, meaning that the cache is bound by the number of aggregators in an epoch.

Implications

This change has an impact in the following scenario:

• There are N > 1 aggregators in a committee.
• Each aggregator decides to craft an identical attestation with a bad signature

Without this PR, only the first attestation would propagate on the network. With this change, all N attestations will propagate.

I think this scenario is very low impact (an attestation is <10ms to verify) and also it could also be replicated by without this PR by simply giving each aggregate a different, invalid aggregate signature.

[djrtwo]

Trying to decide if this is considered "backwards compatible" -- or at least enough for our purposes.

Otherwise, need to queue this against v012x instead of dev

[paulhauner]

I think this is backwards compatible and that my previous "implications" section was incorrect, specifically this statement:

With this change, all N attestations will propagate.

All N invalid attestations would not propagate since invalid attestations do not propagate by nature. Instead, if all N attestations were provided to some node it would perform signature verification on all N instead of rejecting N - 1 due to one being seeing previously. Once again, I don't think we should be concerned about this attack vector since it's trivial to force the node to process all the signatures by simply mutating the invalid signature to avoiding the "already seen" check.

Going back to backwards compatibility, I think this change is transparent to other nodes on the network. Lets say there's a node (node) and there's a bad attestation (bad_attn) on the network:

There's two scenarios to consider here; the first time node sees bad_attn and all following times it sees bad_attn:

The first time node sees bad_attn

With or without this PR, node will decide that bad_attn is invalid and reject it.

All following times node sees bad_attn

• Without this PR, node will declare that it's already seen bad_attn and reject it.
• With this PR, node will decide that bad_attn is invalid and reject it.

Either way, the attestation is rejected.

The only potential difference is that perhaps with this PR the node will have a more clear idea about why the attestation is invalid and perhaps ban the peer. That's an internal client thing that's not a concern of the spec, though.

So, even though this change is backwards compatible (according to my reasoning) it didn't make it into v0.11.2. But, it's transparent to the network so I can just include it in Lighthouse without observably violating the protocol. Yay.

[djrtwo]

Thank you!

Queueing in dev to be released on next minor v0.11 or with v0.12 (whichever comes first)