Restrict sync committee messages to optimistic blocks

[paulhauner]

Description

The optimistic sync spec declares:

An optimistic validator MUST NOT participate in sync committees (i.e., sign across the DOMAIN_SYNC_COMMITTEE, DOMAIN_SYNC_COMMITTEE_SELECTION_PROOF or DOMAIN_CONTRIBUTION_AND_PROOF domains).

We need to ensure that a Lighthouse BN/VC pair will not sign sync committee messages that reference an optimistic beacon_block_root.

Steps to resolve

The first change is to return an error in BeaconChain::get_aggregated_sync_committee_contribution whenever the sync_contribution being return references an optimistic beacon_block_root (see #3140 to see how this was done for attestations).

The second, more complicated change is to stop the VC from producing a SyncCommitteeMessage whenever that block references an optimistic head:

lighthouse/validator_client/src/sync_committee_service.rs

Lines 233 to 241 in bac7c3f

	match self
	.validator_store
	.produce_sync_committee_signature(
	slot,
	beacon_block_root,
	duty.validator_index,
	&duty.pubkey,
	)
	.await

We can't just refuse to accept the already-signed SyncCommitteeMessage at the BN HTTP API for two reasons:

• The message is dangerous and should never be signed, we need to prevent it being signed before it is submitted to the BN.
• We don't want to restrict already signed messages, since we assume that the VC must have good advice from some other BN that the beacon_block_root is valid (e.g. when using multiple BNs).

I can see two options, both are blocked on #3070:

1. Pass the execution_optimistic values around with the sync duties in the VC
   • Pros: doesn't require extra calls to the BN.
   • Cons: it's hard to ensure the execution_optimistic are up-to-date without frequent polling.
2. Call /eth/v1/beacon/headers before producing the SyncCommitteeMessage and checking the execution_optimistic value before signing
   • Pros: always gets the latest info.
   • Cons: requires an extra call.

I'm leaning towards (2), since it's the easiest to implement and gives us the most up-to-date info. The call to beacon/headers should be nice and fast, so I don't think it's too much of a bottleneck.

[paulhauner]

@michaelsproul I know you've done a lot of work on sync committees, so I'd be interested to know if what I've written makes sense and if you have a better suggestion? 🙏 ☺️

[michaelsproul]

Looks good, and I favour option (2) as well. I don't even think we need an extra request because I think we could read the optimistic status when we fetch the block root here:

lighthouse/validator_client/src/sync_committee_service.rs

Lines 177 to 187 in db0beb5

	// Fetch block root for `SyncCommitteeContribution`.
	let block_root = self
	.beacon_nodes
	.first_success(RequireSynced::Yes, |beacon_node| async move {
	beacon_node.get_beacon_blocks_root(BlockId::Head).await
	})
	.await
	.map_err(|e| e.to_string())?
	.ok_or_else(|| format!("No block root found for slot {}", slot))?
	.data
	.root;

[macladson]

Implemented in #3191