[Merged by Bors] - Further remove EE redundancy #3324
Conversation
paulhauner
added
ready-for-review
| ) | ||
| } | ||
| } | ||
| Err(EngineApiError::IsSyncing) => { |
There was a problem hiding this comment.
Seems like we've lost the WARN Execution engine syncing log, is this intentional?
There was a problem hiding this comment.
Yep, that's right! We'll get a warning whenever the head is optimistic so I figured this one is effectively a duplicate.
lighthouse/beacon_node/client/src/notifier.rs
Lines 263 to 269 in a390695
Thanks for noticing and raising it.
| @@ -132,7 +132,7 @@ pub struct Config { | |||
| pub default_datadir: PathBuf, | |||
| } | |||
|
|
|||
| /// Provides access to one or more execution engines and provides a neat interface for consumption | |||
| /// Provides access to one execution engine and provides a neat interface for consumption | |||
| /// by the `BeaconChain`. | |||
| /// | |||
| /// When there is more than one execution node specified, the others will be used in a "fallback" | |||
There was a problem hiding this comment.
Can remove the comments about fallback here
| ); | ||
| } | ||
| pub async fn upcheck(&self) { | ||
| let state = match self.api.upcheck().await { |
There was a problem hiding this comment.
nothing wrong here specifically but I noticed this comment in this upcheck() function. I want to make sure this hasn't fallen off the radar? Perhaps verification of the network and chain ids should go outside the upcheck function since it doesn't really need to be done over and over?
There was a problem hiding this comment.
Ah yeah, I also had a think about this during the PR. I'm not sure the chain-id checks are necessary, they're a bit of a hangover from "eth1". It was to prevent, say, a mainnet node from unwittingly building it's caches from Goerli.
The "exchange transition config" endpoint should achieve the same thing before the merge, then after the merge it should become obvious very quickly if the nodes are on the wrong chain.
I'm tempted to just leave the TODO there for now, we can pick it up again when we do a git grep TODO at some point in the future.
realbigsean
added
waiting-on-author
Co-authored-by: realbigsean <seananderson33@GMAIL.com>
|
All comments addressed, thank you @ethDreamer and @realbigsean! |
paulhauner
added
ready-for-review
|
bors r+ |
## Issue Addressed Resolves #3176 ## Proposed Changes Continues from PRs by @divagant-martian to gradually remove EL redundancy (see #3284, #3257). This PR achieves: - Removes the `broadcast` and `first_success` methods. The functional impact is that every request to the EE will always be tried immediately, regardless of the cached `EngineState` (this resolves #3176). Previously we would check the engine state before issuing requests, this doesn't make sense in a single-EE world; there's only one EE so we might as well try it for every request. - Runs the upcheck/watchdog routine once per slot rather than thrice. When we had multiple EEs frequent polling was useful to try and detect when the primary EE had come back online and we could switch to it. That's not as relevant now. - Always creates logs in the `Engines::upcheck` function. Previously we would mute some logs since they could get really noisy when one EE was down but others were functioning fine. Now we only have one EE and are upcheck-ing it less, it makes sense to always produce logs. This PR purposefully does not achieve: - Updating all occurances of "engines" to "engine". I'm trying to keep the diff small and manageable. We can come back for this. ## Additional Info NA
realbigsean
added
ready-for-merge
bors
bot
changed the title
Issue Addressed
Resolves #3176
Proposed Changes
Continues from PRs by @divagant-martian to gradually remove EL redundancy (see #3284, #3257).
This PR achieves:
broadcastandfirst_successmethods. The functional impact is that every request to the EE will always be tried immediately, regardless of the cachedEngineState(this resolves Don't mark EL as offline if non-essential API call fails #3176). Previously we would check the engine state before issuing requests, this doesn't make sense in a single-EE world; there's only one EE so we might as well try it for every request.Engines::upcheckfunction. Previously we would mute some logs since they could get really noisy when one EE was down but others were functioning fine. Now we only have one EE and are upcheck-ing it less, it makes sense to always produce logs.This PR purposefully does not achieve:
Additional Info
NA