Skip to content

Commit

Permalink
Revert "Optimise HTTP validator lookups" (sigp#3658)
Browse files Browse the repository at this point in the history
## Issue Addressed

This reverts commit ca9dc8e (PR sigp#3559) with some modifications.

## Proposed Changes

Unfortunately that PR introduced a performance regression in fork choice. The optimisation _intended_ to build the exit and pubkey caches on the head state _only if_ they were not already built. However, due to the head state always being cloned without these caches, we ended up building them every time the head changed, leading to a ~70ms+ penalty on mainnet.

https://github.com/sigp/lighthouse/blob/fcfd02aeec435203269b03865e3ccc23e5f51e6d/beacon_node/beacon_chain/src/canonical_head.rs#L633-L636

I believe this is a severe enough regression to justify immediately releasing v3.2.1 with this change.

## Additional Info

I didn't fully revert sigp#3559, because there were some unrelated deletions of dead code in that PR which I figured we may as well keep.

An alternative would be to clone the extra caches, but this likely still imposes some cost, so in the interest of applying a conservative fix quickly, I think reversion is the best approach. The optimisation from sigp#3559 was not even optimising a particularly significant path, it was mostly for VCs running larger numbers of inactive keys. We can re-do it in the `tree-states` world where cache clones are cheap.
  • Loading branch information
michaelsproul authored and Woodpile37 committed Jan 6, 2024
1 parent c7bd47a commit b8bba87
Show file tree
Hide file tree
Showing 3 changed files with 5 additions and 37 deletions.
6 changes: 3 additions & 3 deletions beacon_node/beacon_chain/src/canonical_head.rs
Original file line number Diff line number Diff line change
Expand Up @@ -654,11 +654,11 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
})
})
.and_then(|mut snapshot| {
// Regardless of where we got the state from, attempt to build all the
// caches except the tree hash cache.
// Regardless of where we got the state from, attempt to build the committee
// caches.
snapshot
.beacon_state
.build_all_caches(&self.spec)
.build_all_committee_caches(&self.spec)
.map_err(Into::into)
.map(|()| snapshot)
})?;
Expand Down
21 changes: 2 additions & 19 deletions beacon_node/http_api/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -668,34 +668,17 @@ pub fn serve<T: BeaconChainTypes>(
"Invalid validator ID".to_string(),
))
}))
.and(log_filter.clone())
.and(warp::path::end())
.and_then(
|state_id: StateId, chain: Arc<BeaconChain<T>>, validator_id: ValidatorId, log| {
|state_id: StateId, chain: Arc<BeaconChain<T>>, validator_id: ValidatorId| {
blocking_json_task(move || {
let (data, execution_optimistic) = state_id
.map_state_and_execution_optimistic(
&chain,
|state, execution_optimistic| {
let index_opt = match &validator_id {
ValidatorId::PublicKey(pubkey) => {
// Fast path: use the pubkey cache which is probably
// initialised at the head.
match state.get_validator_index_read_only(pubkey) {
Ok(result) => result,
Err(e) => {
// Slow path, fall back to iteration.
debug!(
log,
"Validator look-up cache miss";
"reason" => ?e,
);
state
.validators()
.iter()
.position(|v| v.pubkey == *pubkey)
}
}
state.validators().iter().position(|v| v.pubkey == *pubkey)
}
ValidatorId::Index(index) => Some(*index as usize),
};
Expand Down
15 changes: 0 additions & 15 deletions consensus/types/src/beacon_state.rs
Original file line number Diff line number Diff line change
Expand Up @@ -448,21 +448,6 @@ impl<T: EthSpec> BeaconState<T> {
Ok(self.pubkey_cache().get(pubkey))
}

/// Immutable variant of `get_validator_index` which errors if the cache is not up to date.
pub fn get_validator_index_read_only(
&self,
pubkey: &PublicKeyBytes,
) -> Result<Option<usize>, Error> {
let pubkey_cache = self.pubkey_cache();
if pubkey_cache.len() != self.validators().len() {
return Err(Error::PubkeyCacheIncomplete {
cache_len: pubkey_cache.len(),
registry_len: self.validators().len(),
});
}
Ok(pubkey_cache.get(pubkey))
}

/// The epoch corresponding to `self.slot()`.
pub fn current_epoch(&self) -> Epoch {
self.slot().epoch(T::slots_per_epoch())
Expand Down

0 comments on commit b8bba87

Please sign in to comment.