Remove duplicate checks for gossiped aggregates

[michaelsproul]

Issue Addressed

Implement the removal of the duplicate check on aggregate attestations and sync committee contributions, as described in ethereum/consensus-specs#2225 and ethereum/consensus-specs#2183.

Proposed Changes

• Remove the observed_attestations and observed_sync_contributions caches from the BeaconChain, and all associated code.
• Invert the tests for duplicate aggregates and sync contributions so that they are accepted rather than rejected. This required some hacking with the test generator methods like make_sync_contributions so that they could produce multiple aggregates that were identical except for the aggregator index.

Additional Info

I've considered the impact of this change on the op pool. We already filter out duplicate attestations here:

lighthouse/beacon_node/operation_pool/src/lib.rs

Lines 193 to 195 in 2dc6163

	} else if *existing_attestation == attestation {
	aggregated = true;
	}

Similarly for sync contributions, we only ever keep one per subnet in the pool, and we keep the one of the highest quality.

Hence the op pool should be unaffected by this change.

[michaelsproul]

I'd like some feedback from @AgeManning on the networking impact of this change, as he's the original author of ethereum/consensus-specs#2183.

I'd also like to see how this performs on real networks and whether it leads to substantially more aggregates being processed. An alternative to fully processing the duplicates would be to continue to propagate them at the gossip layer, but keep the caches so we can avoid processing them.

[paulhauner]

Mmmm red.. Looks good, I just have one little comment about a test.

[paulhauner]

Suggested change

	(contributions.len() == count).then(|| contributions)
	(contributions.len() >= count).then(|| contributions)

This obtains the "at least" logic, however it wont obtain exactly count.

[michaelsproul]

I clarified the comment instead in this commit 09de7e2

[michaelsproul]

Testing on Prater now.

[michaelsproul]

As expected we are (successfully) verifying a lot more aggregates, seemingly 2x more:

The average time per aggregate has also roughly doubled from 3ms to 6ms, because we now have to do signature verification for all of them:

Bandwidth seems constant:

So this change seems to mostly be a net negative in its current state 😬 I'm going to try keeping the cache and using it to skip the attestation signature verification. We'll still forward the attestations on gossip as per the spec, but should have processing times more similar to what we have now. We'll only have to check the aggregator's signatures:

lighthouse/beacon_node/beacon_chain/src/attestation_verification.rs

Lines 1134 to 1149 in 09de7e2

	signed_aggregate_selection_proof_signature_set(
	|validator_index| pubkey_cache.get(validator_index).map(Cow::Borrowed),
	signed_aggregate,
	&fork,
	chain.genesis_validators_root,
	&chain.spec,
	)
	.map_err(BeaconChainError::SignatureSetError)?,
	signed_aggregate_signature_set(
	|validator_index| pubkey_cache.get(validator_index).map(Cow::Borrowed),
	signed_aggregate,
	&fork,
	chain.genesis_validators_root,
	&chain.spec,
	)
	.map_err(BeaconChainError::SignatureSetError)?,

[michaelsproul]

The signature skipping approach doesn't seem to save much in the way of verification time. This is an alternative patch that I've been running on Prater: michaelsproul@13ccbc1

It doesn't meaningfully affect the average processing time per aggregate compared to this branch. Still a ~2x slowdown from ~3ms to ~6ms (sometimes 5ms). However, the attestation batch signature verification time seems to be slightly better with the alternative approach:

The switch to this branch is at 10:45, and the switch to the alternative is at 16:10.

---

Given this I'm not 100% sure how to proceed. Unless this brings some tangible benefit to networking I'm tempted to say we should leave it as-is. When you have some time @AgeManning could we go over how to measure the impact on extra IWANT messages mentioned in your original issue ethereum/consensus-specs#2183? 🙏

[AgeManning]

Yep sounds good. Still awaiting some PRs for extended metrics.
Will loop back in when its ready

[michaelsproul]

Current resolution decided out-of-band:

• Move forward with the "alternative" approach of verifying as few signatures as possible, and
• Add a runtime flag --ignore-duplicate-aggregates to retain the previous behaviour (so we have something up our sleeve to turn it off).

[michaelsproul]

Closing now that the spec has reverted this change.