Encode Execution Engine Client Version In Graffiti #5290

ethDreamer · 2024-02-25T06:36:46Z

Issue Addressed

Encode Execution Engine Client Version In Graffiti #5284

Additional Info

All logic related to the choice of graffiti in the beacon node has been moved into:

beacon_node/beacon_chain/src/graffiti_calculator.rs

The graffiti field in the BeaconChain object has been replaced by an instance of GraffitiCalculator. The correct graffiti is obtained by calling the following method on chain.graffiti_calculator:

    /// Returns the appropriate graffiti to use for block production, prioritizing
    /// sources in the following order:
    /// 1. Graffiti specified by the validator client.
    /// 2. Graffiti specified by the user via beacon node CLI options.
    /// 3. The EL & CL client version string, applicable when the EL supports version specification.
    /// 4. The default lighthouse version string, used if the EL lacks version specification support.
    pub async fn get_graffiti(&self, validator_graffiti: Option<Graffiti>) -> Graffiti

macladson

Looks good! Just had a few comments:

beacon_node/execution_layer/src/engine_api.rs

beacon_node/beacon_chain/src/graffiti_calculator.rs

beacon_node/client/src/builder.rs

beacon_node/execution_layer/src/lib.rs

beacon_node/execution_layer/src/engine_api.rs

beacon_node/beacon_chain/src/graffiti_calculator.rs

macladson

LGTM!

beacon_node/beacon_chain/src/graffiti_calculator.rs

pawanjay176 · 2024-03-26T13:55:06Z

beacon_node/beacon_chain/src/graffiti_calculator.rs

+    }
+}
+
+pub fn start_engine_version_cache_refresh_service<T: BeaconChainTypes>(


I'm not sure why we need to maintain a service to keep updating the version.
Why not instead do it once on startup, then do it in BeaconChain::prepare_beacon_proposer. Updating something that doesn't change repeatedly seems quite wasteful to me

We could do it there, but then it would happen right in window between notifying the EL that there's a proposal & actually proposing the block, which is a critical time. The whole reason I wrote it this way to avoid extra calls to the EL within that critical time.

Presumably we could have a service that only caches this result when we detect that we are connected to a validator which will have a proposal within the next epoch, but that code would surely be more complex that this service.

I don't think that making a single call to an EL that returns a pre set value should be very intensive, but your point is fair. Better to not cram more calls than necessary just before a proposal.

beacon_node/beacon_chain/Cargo.toml

dapplion · 2024-04-03T02:00:24Z

beacon_node/beacon_chain/src/graffiti_calculator.rs

+    }
+
+    // this service should run 3/8 of the way through the epoch
+    let epoch_delay = (epoch_duration * 3) / 8;


Why at 3/8?

There are other scheduled tasks that take place at certain times in the epoch so I just picked a time that wouldn't overlap with them

…on_graffiti

pawanjay176

Tested this out with geth and it works as expected.
LGTM.

realbigsean · 2024-04-22T19:35:54Z

@mergify queue

mergify · 2024-04-22T19:36:10Z

queue

🛑 The pull request has been removed from the queue `default`

The queue conditions cannot be satisfied due to failing checks.

You can take a look at Queue: Embarked in merge queue check runs for more details.

In case of a failure due to a flaky test, you should first retrigger the CI.
Then, re-embark the pull request into the merge queue by posting the comment
@mergifyio refresh on the pull request.

realbigsean · 2024-04-22T21:04:00Z

@mergify requeue

mergify · 2024-04-22T21:04:12Z

requeue

✅ This pull request will be re-embarked automatically

The followup queue command will be automatically executed to re-embark the pull request

mergify · 2024-04-22T21:04:13Z

queue

🛑 The pull request has been removed from the queue `default`

The queue conditions cannot be satisfied due to failing checks.

You can take a look at Queue: Embarked in merge queue check runs for more details.

In case of a failure due to a flaky test, you should first retrigger the CI.
Then, re-embark the pull request into the merge queue by posting the comment
@mergifyio refresh on the pull request.

realbigsean · 2024-04-22T22:12:47Z

@mergify requeue

mergify · 2024-04-22T22:12:55Z

requeue

✅ This pull request will be re-embarked automatically

The followup queue command will be automatically executed to re-embark the pull request

mergify · 2024-04-22T22:12:56Z

queue

🛑 The pull request has been removed from the queue `default`

The queue conditions cannot be satisfied due to failing checks.

You can take a look at Queue: Embarked in merge queue check runs for more details.

In case of a failure due to a flaky test, you should first retrigger the CI.
Then, re-embark the pull request into the merge queue by posting the comment
@mergifyio refresh on the pull request.

# Conflicts: # beacon_node/beacon_chain/Cargo.toml

jimmygchen · 2024-04-24T05:03:49Z

@mergify queue

mergify · 2024-04-24T05:04:05Z

queue

✅ The pull request has been merged automatically

The pull request has been merged automatically at 4a48d7b

ethDreamer changed the title ~~El client version graffiti~~ Encode Execution Engine Client Version In Graffiti Feb 25, 2024

ethDreamer added the ready-for-review The code is ready for review label Feb 25, 2024

macladson reviewed Mar 7, 2024

View reviewed changes

macladson reviewed Mar 8, 2024

View reviewed changes

beacon_node/beacon_chain/src/graffiti_calculator.rs Show resolved Hide resolved

macladson approved these changes Mar 8, 2024

View reviewed changes

ethDreamer added the v5.2.0 Q2 2024 label Mar 8, 2024

zilm13 mentioned this pull request Mar 20, 2024

Add clients information to user's graffiti when producing a block Consensys/teku#8107

Merged

2 tasks

ethDreamer added 6 commits March 25, 2024 10:25

Add engine_clientVersionV1 structs

3d9dda2

Implement engine_clientVersionV1

875e550

Update to latest spec changes

3a791b4

Implement GraffitiCalculator Service

7d91a9f

Added Unit Tests for GraffitiCalculator

4f9210e

Address Mac's Comments

f85eeda

ethDreamer force-pushed the el_client_version_graffiti branch from e43db19 to f85eeda Compare March 25, 2024 15:31

pawanjay176 reviewed Mar 26, 2024

View reviewed changes

dapplion reviewed Apr 3, 2024

View reviewed changes

beacon_node/beacon_chain/Cargo.toml Outdated Show resolved Hide resolved

dapplion reviewed Apr 3, 2024

View reviewed changes

ethDreamer added 2 commits April 16, 2024 11:06

Remove need to use clap in beacon chain

f858eca

Merge remote-tracking branch 'upstream/unstable' into el_client_versi…

be55163

…on_graffiti

pawanjay176 approved these changes Apr 22, 2024

View reviewed changes

mergify bot added a commit that referenced this pull request Apr 22, 2024

Merge of #5290

dae2f87

mergify bot mentioned this pull request Apr 22, 2024

merge queue: embarking unstable (ad7f0e0) and #5290 together #5621

Closed

5 tasks

mergify bot added a commit that referenced this pull request Apr 22, 2024

Merge of #5290

98431d6

mergify bot mentioned this pull request Apr 22, 2024

merge queue: embarking unstable (ad7f0e0) and #5290 together #5622

Closed

5 tasks

mergify bot added a commit that referenced this pull request Apr 22, 2024

Merge of #5290

6839def

mergify bot mentioned this pull request Apr 22, 2024

merge queue: embarking unstable (ad7f0e0) and #5290 together #5623

Closed

5 tasks

realbigsean added ready-for-merge This PR is ready to merge. and removed ready-for-review The code is ready for review labels Apr 22, 2024

Merge branch 'unstable' into el_client_version_graffiti

a2f374e

# Conflicts: # beacon_node/beacon_chain/Cargo.toml

mergify bot added a commit that referenced this pull request Apr 24, 2024

Merge of #5290

92787fc

This was referenced Apr 24, 2024

merge queue: embarking unstable (6196289) and [#5635 + #5641 + #5620 + #5290] together #5642

Closed

Yaml rust2 #5635

Merged

Bump jobserver and fix non-portable builds #5641

Merged

Drop lookup type trait for a simple arg #5620

Merged

mergify bot merged commit 4a48d7b into sigp:unstable Apr 24, 2024
27 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Encode Execution Engine Client Version In Graffiti #5290

Encode Execution Engine Client Version In Graffiti #5290

ethDreamer commented Feb 25, 2024

macladson left a comment

macladson left a comment

pawanjay176 Mar 26, 2024

ethDreamer Mar 28, 2024

pawanjay176 Apr 19, 2024

dapplion Apr 3, 2024

ethDreamer Apr 3, 2024

pawanjay176 left a comment

realbigsean commented Apr 22, 2024

mergify bot commented Apr 22, 2024 •

edited

Loading

realbigsean commented Apr 22, 2024

mergify bot commented Apr 22, 2024

mergify bot commented Apr 22, 2024 •

edited

Loading

realbigsean commented Apr 22, 2024

mergify bot commented Apr 22, 2024

mergify bot commented Apr 22, 2024 •

edited

Loading

jimmygchen commented Apr 24, 2024

mergify bot commented Apr 24, 2024 •

edited

Loading

Encode Execution Engine Client Version In Graffiti #5290

Encode Execution Engine Client Version In Graffiti #5290

Conversation

ethDreamer commented Feb 25, 2024

Issue Addressed

Additional Info

macladson left a comment

Choose a reason for hiding this comment

macladson left a comment

Choose a reason for hiding this comment

pawanjay176 Mar 26, 2024

Choose a reason for hiding this comment

ethDreamer Mar 28, 2024

Choose a reason for hiding this comment

pawanjay176 Apr 19, 2024

Choose a reason for hiding this comment

dapplion Apr 3, 2024

Choose a reason for hiding this comment

ethDreamer Apr 3, 2024

Choose a reason for hiding this comment

pawanjay176 left a comment

Choose a reason for hiding this comment

realbigsean commented Apr 22, 2024

mergify bot commented Apr 22, 2024 • edited Loading

🛑 The pull request has been removed from the queue default

realbigsean commented Apr 22, 2024

mergify bot commented Apr 22, 2024

✅ This pull request will be re-embarked automatically

mergify bot commented Apr 22, 2024 • edited Loading

🛑 The pull request has been removed from the queue default

realbigsean commented Apr 22, 2024

mergify bot commented Apr 22, 2024

✅ This pull request will be re-embarked automatically

mergify bot commented Apr 22, 2024 • edited Loading

🛑 The pull request has been removed from the queue default

jimmygchen commented Apr 24, 2024

mergify bot commented Apr 24, 2024 • edited Loading

✅ The pull request has been merged automatically

mergify bot commented Apr 22, 2024 •

edited

Loading

🛑 The pull request has been removed from the queue `default`

mergify bot commented Apr 22, 2024 •

edited

Loading

🛑 The pull request has been removed from the queue `default`

mergify bot commented Apr 22, 2024 •

edited

Loading

🛑 The pull request has been removed from the queue `default`

mergify bot commented Apr 24, 2024 •

edited

Loading