[Merged by Bors] - Initial work to remove engines fallback from the execution_layer crate
#3257
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
divagant-martian
force-pushed
the
remove-engines-fallback
branch
from
fa6ff6a to
9adeca3
Compare
divagant-martian
commented
Jun 9, 2022
| } | ||
| } | ||
| Err(e) => { | ||
| let i = 0usize; |
There was a problem hiding this comment.
This looks horrible, but I don't want to touch the EngineError::Api (which contains the id) to keep the changes minimal
divagant-martian
changed the title
execution_layer crateexecution_layer crate
divagant-martian
changed the title
execution_layer crateexecution_layer crate
pawanjay176
approved these changes
Jun 10, 2022
paulhauner
requested changes
Jun 22, 2022
There was a problem hiding this comment.
Looks good, I only have a couple of really minor suggestions.
We'll have to come back and clean this up, but I appreciate that this will unblocks #3094 and I think this PR is a good way to do that :)
paulhauner
approved these changes
Jun 22, 2022
|
bors r+ |
bors bot
pushed a commit
that referenced
this pull request
Jun 22, 2022
…ate (#3257) ## Issue Addressed Part of #3160 ## Proposed Changes Use only the first url given in the execution engine, if more than one is provided log it. This change only moves having multiple engines to one. The amount of code cleanup that can and should be done forward is not small and would interfere with ongoing PRs. I'm keeping the changes intentionally very very minimal. ## Additional Info Future works: - In [ `EngineError` ](https://github.com/sigp/lighthouse/blob/9c429d0764ed91cf56efb8a47a35a556b54a86a4/beacon_node/execution_layer/src/engines.rs#L173-L177) the id is not needed since it now has no meaning. - the [ `first_success_without_retry` ](https://github.com/sigp/lighthouse/blob/9c429d0764ed91cf56efb8a47a35a556b54a86a4/beacon_node/execution_layer/src/engines.rs#L348-L351) function can return a single error. - the [`first_success`](https://github.com/sigp/lighthouse/blob/9c429d0764ed91cf56efb8a47a35a556b54a86a4/beacon_node/execution_layer/src/engines.rs#L324) function can return a single error. - After the redundancy is removed for the builders, we can probably make the [ `EngineErrors` ](https://github.com/sigp/lighthouse/blob/9c429d0764ed91cf56efb8a47a35a556b54a86a4/beacon_node/execution_layer/src/lib.rs#L69) carry a single error. - Merge the [`Engines`](https://github.com/sigp/lighthouse/blob/9c429d0764ed91cf56efb8a47a35a556b54a86a4/beacon_node/execution_layer/src/engines.rs#L161-L165) struct and [`Engine` ](https://github.com/sigp/lighthouse/blob/9c429d0764ed91cf56efb8a47a35a556b54a86a4/beacon_node/execution_layer/src/engines.rs#L62-L67) - Fix the associated configurations and cli params. Not sure if both are done in #3214 In general I think those changes can be done incrementally and in individual pull requests.
bors
bot
changed the title
execution_layer crateexecution_layer crate
This was referenced Jun 23, 2022
bors bot
pushed a commit
that referenced
this pull request
Jul 4, 2022
## Issue Addressed Part of #3118, continuation of #3257 ## Proposed Changes - the [ `first_success_without_retry` ](https://github.com/sigp/lighthouse/blob/9c429d0764ed91cf56efb8a47a35a556b54a86a4/beacon_node/execution_layer/src/engines.rs#L348-L351) function returns a single error. - the [`first_success`](https://github.com/sigp/lighthouse/blob/9c429d0764ed91cf56efb8a47a35a556b54a86a4/beacon_node/execution_layer/src/engines.rs#L324) function returns a single error. - [ `EngineErrors` ](https://github.com/sigp/lighthouse/blob/9c429d0764ed91cf56efb8a47a35a556b54a86a4/beacon_node/execution_layer/src/lib.rs#L69) carries a single error. - [`EngineError`](https://github.com/sigp/lighthouse/blob/9c429d0764ed91cf56efb8a47a35a556b54a86a4/beacon_node/execution_layer/src/engines.rs#L173-L177) now does not need to carry an Id - [`process_multiple_payload_statuses`](https://github.com/sigp/lighthouse/blob/9c429d0764ed91cf56efb8a47a35a556b54a86a4/beacon_node/execution_layer/src/payload_status.rs#L46-L50) now doesn't need to receive an iterator of statuses and weight in different errors ## Additional Info This is built on top of #3294
bors bot
pushed a commit
that referenced
this pull request
Jul 11, 2022
…#3284) ## Issue Addressed Part of #3118, continuation of #3257 and #3283 ## Proposed Changes - Merge the [`Engines`](https://github.com/sigp/lighthouse/blob/9c429d0764ed91cf56efb8a47a35a556b54a86a4/beacon_node/execution_layer/src/engines.rs#L161-L165) struct and [`Engine` ](https://github.com/sigp/lighthouse/blob/9c429d0764ed91cf56efb8a47a35a556b54a86a4/beacon_node/execution_layer/src/engines.rs#L62-L67) - Remove unnecessary generics ## Additional Info There is more cleanup to do that will come in subsequent PRs
bors bot
pushed a commit
that referenced
this pull request
Jul 13, 2022
## 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Issue Addressed
Part of #3118
Proposed Changes
Use only the first url given in the execution engine, if more than one is provided log it.
This change only moves having multiple engines to one. The amount of code cleanup that can and should be done forward is not small and would interfere with ongoing PRs. I'm keeping the changes intentionally very very minimal.
Additional Info
Future works:
EngineErrorthe id is not needed since it now has no meaning.first_success_without_retryfunction can return a single error.first_successfunction can return a single error.EngineErrorscarry a single error.Enginesstruct andEngineIn general I think those changes can be done incrementally and in individual pull requests.