Skip to content

Append client version info to graffiti #7558

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 10 commits into
base: unstable
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 10 additions & 5 deletions beacon_node/beacon_chain/src/beacon_chain.rs
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ use crate::events::ServerSentEventHandler;
use crate::execution_payload::{get_execution_payload, NotifyExecutionLayer, PreparePayloadHandle};
use crate::fetch_blobs::EngineGetBlobsOutput;
use crate::fork_choice_signal::{ForkChoiceSignalRx, ForkChoiceSignalTx, ForkChoiceWaitResult};
use crate::graffiti_calculator::GraffitiCalculator;
use crate::graffiti_calculator::{GraffitiCalculator, GraffitiSettings};
use crate::kzg_utils::reconstruct_blobs;
use crate::light_client_finality_update_verification::{
Error as LightClientFinalityUpdateError, VerifiedLightClientFinalityUpdate,
Expand Down Expand Up @@ -4538,7 +4538,7 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
self: &Arc<Self>,
randao_reveal: Signature,
slot: Slot,
validator_graffiti: Option<Graffiti>,
graffiti_settings: GraffitiSettings,
verification: ProduceBlockVerification,
builder_boost_factor: Option<u64>,
block_production_version: BlockProductionVersion,
Expand Down Expand Up @@ -4567,7 +4567,7 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
state_root_opt,
slot,
randao_reveal,
validator_graffiti,
graffiti_settings,
verification,
builder_boost_factor,
block_production_version,
Expand Down Expand Up @@ -5099,7 +5099,7 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
state_root_opt: Option<Hash256>,
produce_at_slot: Slot,
randao_reveal: Signature,
validator_graffiti: Option<Graffiti>,
graffiti_settings: GraffitiSettings,
verification: ProduceBlockVerification,
builder_boost_factor: Option<u64>,
block_production_version: BlockProductionVersion,
Expand All @@ -5110,7 +5110,7 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
let chain = self.clone();
let graffiti = self
.graffiti_calculator
.get_graffiti(validator_graffiti)
.get_graffiti(graffiti_settings)
.await;
let mut partial_beacon_block = self
.task_executor
Expand Down Expand Up @@ -7169,6 +7169,11 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
roots.reverse();
roots
}

// Add a public method to BeaconChain so we can access graffiti_calculator from another crate
pub fn calculate_graffiti(&self) -> &GraffitiCalculator<T> {
&self.graffiti_calculator
}
}

impl<T: BeaconChainTypes> Drop for BeaconChain<T> {
Expand Down
53 changes: 48 additions & 5 deletions beacon_node/beacon_chain/src/graffiti_calculator.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
use crate::BeaconChain;
use crate::BeaconChainTypes;
use eth2::types::GraffitiPolicy;
use execution_layer::{http::ENGINE_GET_CLIENT_VERSION_V1, CommitPrefix, ExecutionLayer};
use logging::crit;
use serde::{Deserialize, Serialize};
Expand Down Expand Up @@ -48,6 +49,22 @@ impl Debug for GraffitiOrigin {
}
}

pub enum GraffitiSettings {
Unspecified,
Specified {
graffiti: Graffiti,
policy: GraffitiPolicy,
},
}

impl GraffitiSettings {
pub fn new(validator_graffiti: Option<Graffiti>, policy: GraffitiPolicy) -> Self {
validator_graffiti
.map(|graffiti| Self::Specified { graffiti, policy })
.unwrap_or(Self::Unspecified)
}
}

pub struct GraffitiCalculator<T: BeaconChainTypes> {
pub beacon_graffiti: GraffitiOrigin,
execution_layer: Option<ExecutionLayer<T::EthSpec>>,
Expand All @@ -73,7 +90,19 @@ impl<T: BeaconChainTypes> GraffitiCalculator<T> {
/// 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 {
pub async fn get_graffiti(&self, graffiti_settings: GraffitiSettings) -> Graffiti {
match graffiti_settings {
GraffitiSettings::Specified { graffiti, policy } => match policy {
GraffitiPolicy::PreserveUserGraffiti => graffiti,
GraffitiPolicy::AppendClientVersions => {
self.calculate_combined_graffiti(Some(graffiti)).await
}
},
GraffitiSettings::Unspecified => self.calculate_combined_graffiti(None).await,
}
}

async fn calculate_combined_graffiti(&self, validator_graffiti: Option<Graffiti>) -> Graffiti {
if let Some(graffiti) = validator_graffiti {
return graffiti;
}
Expand Down Expand Up @@ -221,8 +250,10 @@ async fn engine_version_cache_refresh_service<T: BeaconChainTypes>(

#[cfg(test)]
mod tests {
use crate::graffiti_calculator::GraffitiSettings;
use crate::test_utils::{test_spec, BeaconChainHarness, EphemeralHarnessType};
use crate::ChainConfig;
use eth2::types::GraffitiPolicy;
use execution_layer::test_utils::{DEFAULT_CLIENT_VERSION, DEFAULT_ENGINE_CAPABILITIES};
use execution_layer::EngineCapabilities;
use std::sync::Arc;
Expand Down Expand Up @@ -278,8 +309,12 @@ mod tests {

let version_bytes = std::cmp::min(lighthouse_version::VERSION.len(), GRAFFITI_BYTES_LEN);
// grab the slice of the graffiti that corresponds to the lighthouse version
let graffiti_slice =
&harness.chain.graffiti_calculator.get_graffiti(None).await.0[..version_bytes];
let graffiti_slice = &harness
.chain
.graffiti_calculator
.get_graffiti(GraffitiSettings::Unspecified)
.await
.0[..version_bytes];

// convert graffiti bytes slice to ascii for easy debugging if this test should fail
let graffiti_str =
Expand All @@ -300,7 +335,12 @@ mod tests {
let spec = Arc::new(test_spec::<MinimalEthSpec>());
let harness = get_harness(VALIDATOR_COUNT, spec, None);

let found_graffiti_bytes = harness.chain.graffiti_calculator.get_graffiti(None).await.0;
let found_graffiti_bytes = harness
.chain
.graffiti_calculator
.get_graffiti(GraffitiSettings::Unspecified)
.await
.0;

let mock_commit = DEFAULT_CLIENT_VERSION.commit.clone();
let expected_graffiti_string = format!(
Expand Down Expand Up @@ -349,7 +389,10 @@ mod tests {
let found_graffiti = harness
.chain
.graffiti_calculator
.get_graffiti(Some(Graffiti::from(graffiti_bytes)))
.get_graffiti(GraffitiSettings::new(
Some(Graffiti::from(graffiti_bytes)),
GraffitiPolicy::PreserveUserGraffiti,
))
.await;

assert_eq!(
Expand Down
11 changes: 8 additions & 3 deletions beacon_node/beacon_chain/src/test_utils.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
use crate::blob_verification::GossipVerifiedBlob;
use crate::block_verification_types::{AsBlock, RpcBlock};
use crate::data_column_verification::CustodyDataColumn;
use crate::graffiti_calculator::GraffitiSettings;
use crate::kzg_utils::build_data_column_sidecars;
use crate::observed_operations::ObservationOutcome;
pub use crate::persisted_beacon_chain::PersistedBeaconChain;
Expand All @@ -20,7 +21,7 @@ use crate::{
};
use crate::{get_block_root, BeaconBlockResponseWrapper};
use bls::get_withdrawal_credentials;
use eth2::types::SignedBlockContentsTuple;
use eth2::types::{GraffitiPolicy, SignedBlockContentsTuple};
use execution_layer::test_utils::generate_genesis_header;
use execution_layer::{
auth::JwtKey,
Expand Down Expand Up @@ -942,6 +943,8 @@ where
// BeaconChain errors out with `DuplicateFullyImported`. Vary the graffiti so that we produce
// different blocks each time.
let graffiti = Graffiti::from(self.rng.lock().gen::<[u8; 32]>());
let graffiti_settings =
GraffitiSettings::new(Some(graffiti), GraffitiPolicy::PreserveUserGraffiti);

let randao_reveal = self.sign_randao_reveal(&state, proposer_index, slot);

Expand All @@ -952,7 +955,7 @@ where
None,
slot,
randao_reveal,
Some(graffiti),
graffiti_settings,
ProduceBlockVerification::VerifyRandao,
None,
BlockProductionVersion::FullV2,
Expand Down Expand Up @@ -1001,6 +1004,8 @@ where
// BeaconChain errors out with `DuplicateFullyImported`. Vary the graffiti so that we produce
// different blocks each time.
let graffiti = Graffiti::from(self.rng.lock().gen::<[u8; 32]>());
let graffiti_settings =
GraffitiSettings::new(Some(graffiti), GraffitiPolicy::PreserveUserGraffiti);

let randao_reveal = self.sign_randao_reveal(&state, proposer_index, slot);

Expand All @@ -1013,7 +1018,7 @@ where
None,
slot,
randao_reveal,
Some(graffiti),
graffiti_settings,
ProduceBlockVerification::VerifyRandao,
None,
BlockProductionVersion::FullV2,
Expand Down
12 changes: 9 additions & 3 deletions beacon_node/http_api/src/produce_block.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ use crate::{
ResponseIncludesVersion,
},
};
use beacon_chain::graffiti_calculator::GraffitiSettings;
use beacon_chain::{
BeaconBlockResponseWrapper, BeaconChain, BeaconChainTypes, ProduceBlockVerification,
};
Expand Down Expand Up @@ -61,11 +62,13 @@ pub async fn produce_block_v3<T: BeaconChainTypes>(
query.builder_boost_factor
};

let graffiti_settings = GraffitiSettings::new(query.graffiti, query.graffiti_policy);

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the wrong place to do this. This function will call produce_block_with_verification() which will call produce_block_on_state() which will call the graffiti_calculator.get_graffiti() again. I would suggest you pass this argument down the function calls along with the graffiti. So maybe (inside graffiti_calculator.rs) create some new enum:

enum GraffitiSettings {
    Unspecified,
    Specified {
        graffiti: Graffiti,
        policy: GraffitiPolicy,
    }
}

impl GraffitiSettings {
    fn new(validator_graffiti: Option<Graffiti>, policy: GraffitiPolicy) {
        validator_graffiti.map(|graffiti| Self::Specified {
            graffiti,
            policy,
        })
        .unwrap_or(Self::Unspecified)
    }
}

and pass that down through the functions. Then you'd want to change get_graffiti to:

pub async fn get_graffiti(&self, graffiti_settings: GraffitiSettings) -> Graffiti

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks so much for the comment, made some amendment based on your comment. If you have some time to spare, will appreciate for another look to see if this is on the right track @ethDreamer

let block_response_type = chain
.produce_block_with_verification(
randao_reveal,
slot,
query.graffiti,
graffiti_settings,
randao_verification,
builder_boost_factor,
BlockProductionVersion::V3,
Expand Down Expand Up @@ -141,11 +144,13 @@ pub async fn produce_blinded_block_v2<T: BeaconChainTypes>(
})?;

let randao_verification = get_randao_verification(&query, randao_reveal.is_infinity())?;
let graffiti_settings = GraffitiSettings::new(query.graffiti, query.graffiti_policy);

let block_response_type = chain
.produce_block_with_verification(
randao_reveal,
slot,
query.graffiti,
graffiti_settings,
randao_verification,
None,
BlockProductionVersion::BlindedV2,
Expand All @@ -170,12 +175,13 @@ pub async fn produce_block_v2<T: BeaconChainTypes>(
})?;

let randao_verification = get_randao_verification(&query, randao_reveal.is_infinity())?;
let graffiti_settings = GraffitiSettings::new(query.graffiti, query.graffiti_policy);

let block_response_type = chain
.produce_block_with_verification(
randao_reveal,
slot,
query.graffiti,
graffiti_settings,
randao_verification,
None,
BlockProductionVersion::FullV2,
Expand Down
14 changes: 14 additions & 0 deletions common/eth2/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2177,6 +2177,7 @@ impl BeaconNodeHttpClient {
graffiti: Option<&Graffiti>,
skip_randao_verification: SkipRandaoVerification,
builder_booster_factor: Option<u64>,
graffiti_policy: Option<GraffitiPolicy>,
) -> Result<Url, Error> {
let mut path = self.eth_path(V3)?;

Expand Down Expand Up @@ -2204,6 +2205,11 @@ impl BeaconNodeHttpClient {
.append_pair("builder_boost_factor", &builder_booster_factor.to_string());
}

if let Some(graffiti_policy) = graffiti_policy {
path.query_pairs_mut()
.append_pair("graffiti_policy", &graffiti_policy.to_string());
}

Ok(path)
}

Expand All @@ -2214,13 +2220,15 @@ impl BeaconNodeHttpClient {
randao_reveal: &SignatureBytes,
graffiti: Option<&Graffiti>,
builder_booster_factor: Option<u64>,
graffiti_policy: Option<GraffitiPolicy>,
) -> Result<(JsonProduceBlockV3Response<E>, ProduceBlockV3Metadata), Error> {
self.get_validator_blocks_v3_modular(
slot,
randao_reveal,
graffiti,
SkipRandaoVerification::No,
builder_booster_factor,
graffiti_policy,
)
.await
}
Expand All @@ -2233,6 +2241,7 @@ impl BeaconNodeHttpClient {
graffiti: Option<&Graffiti>,
skip_randao_verification: SkipRandaoVerification,
builder_booster_factor: Option<u64>,
graffiti_policy: Option<GraffitiPolicy>,
) -> Result<(JsonProduceBlockV3Response<E>, ProduceBlockV3Metadata), Error> {
let path = self
.get_validator_blocks_v3_path(
Expand All @@ -2241,6 +2250,7 @@ impl BeaconNodeHttpClient {
graffiti,
skip_randao_verification,
builder_booster_factor,
graffiti_policy,
)
.await?;

Expand Down Expand Up @@ -2283,13 +2293,15 @@ impl BeaconNodeHttpClient {
randao_reveal: &SignatureBytes,
graffiti: Option<&Graffiti>,
builder_booster_factor: Option<u64>,
graffiti_policy: Option<GraffitiPolicy>,
) -> Result<(ProduceBlockV3Response<E>, ProduceBlockV3Metadata), Error> {
self.get_validator_blocks_v3_modular_ssz::<E>(
slot,
randao_reveal,
graffiti,
SkipRandaoVerification::No,
builder_booster_factor,
graffiti_policy,
)
.await
}
Expand All @@ -2302,6 +2314,7 @@ impl BeaconNodeHttpClient {
graffiti: Option<&Graffiti>,
skip_randao_verification: SkipRandaoVerification,
builder_booster_factor: Option<u64>,
graffiti_policy: Option<GraffitiPolicy>,
) -> Result<(ProduceBlockV3Response<E>, ProduceBlockV3Metadata), Error> {
let path = self
.get_validator_blocks_v3_path(
Expand All @@ -2310,6 +2323,7 @@ impl BeaconNodeHttpClient {
graffiti,
skip_randao_verification,
builder_booster_factor,
graffiti_policy,
)
.await?;

Expand Down
20 changes: 19 additions & 1 deletion common/eth2/src/types.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ use serde::{Deserialize, Deserializer, Serialize};
use serde_utils::quoted_u64::Quoted;
use ssz::{Decode, DecodeError};
use ssz_derive::{Decode, Encode};
use std::fmt::{self, Display};
use std::fmt::{self, Display, Formatter};
use std::str::FromStr;
use std::sync::Arc;
use std::time::Duration;
Expand Down Expand Up @@ -751,12 +751,30 @@ pub struct ProposerData {
pub slot: Slot,
}

#[derive(Clone, Copy, Serialize, Deserialize, Default, Debug)]
pub enum GraffitiPolicy {
#[default]
PreserveUserGraffiti,
AppendClientVersions,
}

impl fmt::Display for GraffitiPolicy {
fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result {
match self {
GraffitiPolicy::PreserveUserGraffiti => write!(f, "preserve_user_graffiti"),
GraffitiPolicy::AppendClientVersions => write!(f, "append_client_versions"),
}
}
}

#[derive(Clone, Deserialize)]
pub struct ValidatorBlocksQuery {
pub randao_reveal: SignatureBytes,
pub graffiti: Option<Graffiti>,
pub skip_randao_verification: SkipRandaoVerification,
pub builder_boost_factor: Option<u64>,
#[serde(default)]
pub graffiti_policy: GraffitiPolicy,
}

#[derive(Debug, Clone, Copy, Default, PartialEq, Eq, Deserialize)]
Expand Down
Loading
Loading