Reduce frequency of polling unknown validators to avoid overwhelming the Beacon Node

[jimmygchen]

Issue Addressed

Addresses #4388.

A large number of unknown validators on a VC is known to overwhelm the beacon node because the VC triggers a retrieval for each validator per slot. This PR reduces the frequency to one query per unknown validator each epoch.

Proposed Changes

1. Poll for all validator indices on startup (unchanged)
2. If any validator is unknown, register to poll again in the 32 slots (1 epoch) instead of the next slot.
3. Avoid polling on the first 1st slot of epoch.

For more details and rationale, see comment here.

[pawanjay176]

Nice, LGTM. Just a minor question

[jimmygchen]

I'll do a bit of manual testing next week before we merge this!

[jimmygchen]

@chong-he found an issue during testing and it looks like the VC re-queries the BN during the first slot of the epoch, I'll look into this.

[jimmygchen]

Upon investigating the logs, I think it is working as intended, and the poll in the first slot of epoch is skipped. Will wait for CK to confirm.

However, I think there's likelihood that this could happen due to the async nature of this function, in the scenario where BN returns a response really late - basically if iterating through all the validators takes close to 12 seconds, then we could be querying in a new slot, which could potentially be first slot of a new epoch.

This is because of the calculation outside the for loop:

lighthouse/validator_client/src/duties_service.rs

Lines 483 to 502 in ce914d1

	let current_slot_opt = duties_service.slot_clock.now();
	let next_poll_slot_opt = current_slot_opt.map(|slot| slot.saturating_add(E::slots_per_epoch()));
	let is_first_slot_of_epoch = if let Some(current_slot) = current_slot_opt {
	let current_epoch_first_slot = current_slot
	.epoch(E::slots_per_epoch())
	.start_slot(E::slots_per_epoch());
	current_slot == current_epoch_first_slot
	} else {
	false
	};
	for pubkey in all_pubkeys {
	// This is on its own line to avoid some weirdness with locks and if statements.
	let is_known = duties_service
	.validator_store
	.initialized_validators()
	.read()
	.get_index(&pubkey)
	.is_some();

For more accuracy, I'll move the calculation into the for loop given we await on each validator query and the slot status could be inaccurate if we have a large list or slow BN.

[chong-he]

@chong-he found an issue during testing and it looks like the VC re-queries the BN during the first slot of the epoch, I'll look into this.

Apologies! I was looking at the wrong slot time.

The VC does not re-query on the first slot in the next epoch (even though we start the VC in the first slot). It waits until the 12s has lapsed and only re-query the BN about the status (i.e., query at the second slot of an epoch). The VC also only query once per epoch, which is a great improvement.

All good upon testing, this PR is good to go

[michaelsproul]

Looks good. Nice improvement

I think we should include this in 5.2

[michaelsproul]

sorry looks like cargo fmt is failing because I messed up the indentation in my suggestion

[jimmygchen]

My bad, should've checked myself :P thanks!

[michaelsproul]

@Mergifyio queue

[mergify]

queue

✅ The pull request has been merged automatically

The pull request has been merged automatically at 52e3112