[Merged by Bors] - Further remove EE redundancy by paulhauner · Pull Request #3324 · sigp/lighthouse

paulhauner · 2022-07-12T01:52:24Z

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 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.
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

beacon_node/execution_layer/src/lib.rs

realbigsean · 2022-07-12T19:47:12Z

beacon_node/execution_layer/src/engines.rs

-            )
-        }
-    }
+            Err(EngineApiError::IsSyncing) => {


Seems like we've lost the WARN Execution engine syncing log, is this intentional?

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

warn!(

log,

"Head is optimistic";

"info" => "chain not fully verified, \

block and attestation production disabled until execution engine syncs",

"execution_block_hash" => ?hash,

);

Thanks for noticing and raising it.

realbigsean · 2022-07-12T19:59:29Z

beacon_node/execution_layer/src/lib.rs

+/// 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"


Can remove the comments about fallback here

beacon_node/execution_layer/src/lib.rs

ethDreamer · 2022-07-12T20:06:34Z

beacon_node/execution_layer/src/engines.rs

-                        );
-                    }
+    pub async fn upcheck(&self) {
+        let state = match self.api.upcheck().await {


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?

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.

beacon_node/execution_layer/src/engines.rs

Co-authored-by: realbigsean <[email protected]>

paulhauner · 2022-07-13T02:53:17Z

All comments addressed, thank you @ethDreamer and @realbigsean!

realbigsean · 2022-07-13T20:31:24Z

bors r+

@divagant-martian

## 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

bors · 2022-07-13T23:00:00Z

Pull request successfully merged into unstable.

Build succeeded:

paulhauner added 3 commits July 12, 2022 11:24

Remove broadcast and first_success

fb6291b

Spawn upcheck tasks

b1a4ef8

Always log, simplify watchdog task

6cc14c5

paulhauner added the work-in-progress PR is a work-in-progress label Jul 12, 2022

paulhauner changed the title ~~Tidy ee redundancy~~ Further remove EE redundancy Jul 12, 2022

paulhauner mentioned this pull request Jul 12, 2022

Don't mark EL as offline if non-essential API call fails #3176

Closed

divagant-martian marked this pull request as ready for review July 12, 2022 03:34

divagant-martian marked this pull request as draft July 12, 2022 04:18

Fix test

249799e

paulhauner added ready-for-review The code is ready for review and removed work-in-progress PR is a work-in-progress labels Jul 12, 2022

paulhauner marked this pull request as ready for review July 12, 2022 05:51

paulhauner requested a review from ethDreamer July 12, 2022 05:51

realbigsean requested changes Jul 12, 2022

View reviewed changes

ethDreamer reviewed Jul 12, 2022

View reviewed changes

beacon_node/execution_layer/src/engines.rs Show resolved Hide resolved

ethDreamer reviewed Jul 12, 2022

View reviewed changes

beacon_node/execution_layer/src/engines.rs Show resolved Hide resolved

realbigsean added waiting-on-author The reviewer has suggested changes and awaits thier implementation. and removed ready-for-review The code is ready for review labels Jul 12, 2022

paulhauner and others added 3 commits July 13, 2022 10:18

Apply suggestions from code review

5255380

Co-authored-by: realbigsean <[email protected]>

Use explicit types

eb285d5

Fix comments

89ef7e8

paulhauner added ready-for-review The code is ready for review and removed waiting-on-author The reviewer has suggested changes and awaits thier implementation. labels Jul 13, 2022

realbigsean approved these changes Jul 13, 2022

View reviewed changes

ethDreamer approved these changes Jul 13, 2022

View reviewed changes

realbigsean added ready-for-merge This PR is ready to merge. and removed ready-for-review The code is ready for review labels Jul 13, 2022

bors bot changed the title ~~Further remove EE redundancy~~ [Merged by Bors] - Further remove EE redundancy Jul 13, 2022

bors bot closed this Jul 13, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Merged by Bors] - Further remove EE redundancy#3324

[Merged by Bors] - Further remove EE redundancy#3324
paulhauner wants to merge 7 commits intosigp:unstablefrom
paulhauner:tidy-ee-redundancy

paulhauner commented Jul 12, 2022 •

edited

Loading

Uh oh!

Uh oh!

realbigsean Jul 12, 2022

Uh oh!

paulhauner Jul 13, 2022

Uh oh!

realbigsean Jul 12, 2022

Uh oh!

Uh oh!

ethDreamer Jul 12, 2022 •

edited

Loading

Uh oh!

paulhauner Jul 13, 2022

Uh oh!

Uh oh!

Uh oh!

paulhauner commented Jul 13, 2022

Uh oh!

realbigsean commented Jul 13, 2022

Uh oh!

bors bot commented Jul 13, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	warn!(
	log,
	"Head is optimistic";
	"info" => "chain not fully verified, \
	block and attestation production disabled until execution engine syncs",
	"execution_block_hash" => ?hash,
	);

Conversation

paulhauner commented Jul 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Issue Addressed

Proposed Changes

Additional Info

Uh oh!

Uh oh!

realbigsean Jul 12, 2022

Choose a reason for hiding this comment

Uh oh!

paulhauner Jul 13, 2022

Choose a reason for hiding this comment

Uh oh!

realbigsean Jul 12, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ethDreamer Jul 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

paulhauner Jul 13, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

paulhauner commented Jul 13, 2022

Uh oh!

realbigsean commented Jul 13, 2022

Uh oh!

bors bot commented Jul 13, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

paulhauner commented Jul 12, 2022 •

edited

Loading

ethDreamer Jul 12, 2022 •

edited

Loading