From 77eabc5401223d7de06a55204d69e68a92e0a54d Mon Sep 17 00:00:00 2001 From: Michael Sproul Date: Wed, 26 Oct 2022 06:50:04 +0000 Subject: [PATCH] Revert "Optimise HTTP validator lookups" (#3658) ## Issue Addressed This reverts commit ca9dc8e0947a0ec83f31830aaabc1ffbd3c14c9c (PR #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 #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 #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. --- .../beacon_chain/src/canonical_head.rs | 6 +++--- beacon_node/http_api/src/lib.rs | 21 ++----------------- consensus/types/src/beacon_state.rs | 15 ------------- 3 files changed, 5 insertions(+), 37 deletions(-) diff --git a/beacon_node/beacon_chain/src/canonical_head.rs b/beacon_node/beacon_chain/src/canonical_head.rs index 53e0fbaac..c9bd6db0e 100644 --- a/beacon_node/beacon_chain/src/canonical_head.rs +++ b/beacon_node/beacon_chain/src/canonical_head.rs @@ -654,11 +654,11 @@ impl BeaconChain { }) }) .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) })?; diff --git a/beacon_node/http_api/src/lib.rs b/beacon_node/http_api/src/lib.rs index 51e97c893..5b4fa5816 100644 --- a/beacon_node/http_api/src/lib.rs +++ b/beacon_node/http_api/src/lib.rs @@ -668,10 +668,9 @@ pub fn serve( "Invalid validator ID".to_string(), )) })) - .and(log_filter.clone()) .and(warp::path::end()) .and_then( - |state_id: StateId, chain: Arc>, validator_id: ValidatorId, log| { + |state_id: StateId, chain: Arc>, validator_id: ValidatorId| { blocking_json_task(move || { let (data, execution_optimistic) = state_id .map_state_and_execution_optimistic( @@ -679,23 +678,7 @@ pub fn serve( |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), }; diff --git a/consensus/types/src/beacon_state.rs b/consensus/types/src/beacon_state.rs index 46a431d07..a5d00cdf2 100644 --- a/consensus/types/src/beacon_state.rs +++ b/consensus/types/src/beacon_state.rs @@ -447,21 +447,6 @@ impl BeaconState { 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, 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())