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 13 commits into
base: unstable
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 7 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
5 changes: 5 additions & 0 deletions beacon_node/beacon_chain/src/beacon_chain.rs
Original file line number Diff line number Diff line change
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
16 changes: 14 additions & 2 deletions beacon_node/http_api/src/produce_block.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,9 @@ use crate::{
use beacon_chain::{
BeaconBlockResponseWrapper, BeaconChain, BeaconChainTypes, ProduceBlockVerification,
};
use eth2::types::{self as api_types, ProduceBlockV3Metadata, SkipRandaoVerification};
use eth2::types::{
self as api_types, GraffitiPolicy, ProduceBlockV3Metadata, SkipRandaoVerification,
};
use ssz::Encode;
use std::sync::Arc;
use types::{payload::BlockProductionVersion, *};
Expand Down Expand Up @@ -61,11 +63,21 @@ pub async fn produce_block_v3<T: BeaconChainTypes>(
query.builder_boost_factor
};

let final_graffiti = match query.graffiti_policy {
GraffitiPolicy::PreserveUserGraffiti => query.graffiti,
GraffitiPolicy::AppendClientVersions => Some(
chain
.calculate_graffiti()
.get_graffiti(query.graffiti)
.await,
),
};

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,
final_graffiti,
randao_verification,
builder_boost_factor,
BlockProductionVersion::V3,
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
10 changes: 10 additions & 0 deletions validator_client/src/cli.rs
Original file line number Diff line number Diff line change
Expand Up @@ -145,11 +145,21 @@ pub struct ValidatorClient {
#[clap(
long,
value_name = "GRAFFITI",
conflicts_with = "graffiti_client",
help = "Specify your custom graffiti to be included in blocks.",
display_order = 0
)]
pub graffiti: Option<String>,

#[clap(
long,
value_name = "GRAFFITI",
requires = "graffiti",
help = "When used, client version info will be automatically appended to user custom graffiti.",
display_order = 0
)]
pub graffiti_client: bool,

#[clap(
long,
value_name = "GRAFFITI-FILE",
Expand Down
11 changes: 10 additions & 1 deletion validator_client/src/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ use directory::{
get_network_dir, DEFAULT_HARDCODED_NETWORK, DEFAULT_ROOT_DIR, DEFAULT_SECRET_DIR,
DEFAULT_VALIDATOR_DIR,
};
use eth2::types::Graffiti;
use eth2::types::{Graffiti, GraffitiPolicy};
use graffiti_file::GraffitiFile;
use initialized_validators::Config as InitializedValidatorsConfig;
use lighthouse_validator_store::Config as ValidatorStoreConfig;
Expand Down Expand Up @@ -55,6 +55,8 @@ pub struct Config {
pub graffiti: Option<Graffiti>,
/// Graffiti file to load per validator graffitis.
pub graffiti_file: Option<GraffitiFile>,
/// GraffitiPolicy to append client version info
pub graffiti_policy: Option<GraffitiPolicy>,
/// Configuration for the HTTP REST API.
pub http_api: validator_http_api::Config,
/// Configuration for the HTTP REST API.
Expand Down Expand Up @@ -117,6 +119,7 @@ impl Default for Config {
long_timeouts_multiplier: 1,
graffiti: None,
graffiti_file: None,
graffiti_policy: None,
http_api: <_>::default(),
http_metrics: <_>::default(),
beacon_node_fallback: <_>::default(),
Expand Down Expand Up @@ -231,6 +234,12 @@ impl Config {
}
}

config.graffiti_policy = if validator_client_config.graffiti_client {
Some(GraffitiPolicy::AppendClientVersions)
} else {
Some(GraffitiPolicy::PreserveUserGraffiti)
};

if let Some(input_fee_recipient) = validator_client_config.suggested_fee_recipient {
config.validator_store.fee_recipient = Some(input_fee_recipient);
}
Expand Down
13 changes: 13 additions & 0 deletions validator_client/validator_services/src/block_service.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
use beacon_node_fallback::{ApiTopic, BeaconNodeFallback, Error as FallbackError, Errors};
use bls::SignatureBytes;
use eth2::types::GraffitiPolicy;
use eth2::{BeaconNodeHttpClient, StatusCode};
use graffiti_file::{determine_graffiti, GraffitiFile};
use logging::crit;
Expand Down Expand Up @@ -50,6 +51,7 @@ pub struct BlockServiceBuilder<S, T> {
chain_spec: Option<Arc<ChainSpec>>,
graffiti: Option<Graffiti>,
graffiti_file: Option<GraffitiFile>,
graffiti_policy: Option<GraffitiPolicy>,
}

impl<S: ValidatorStore, T: SlotClock + 'static> BlockServiceBuilder<S, T> {
Expand All @@ -63,6 +65,7 @@ impl<S: ValidatorStore, T: SlotClock + 'static> BlockServiceBuilder<S, T> {
chain_spec: None,
graffiti: None,
graffiti_file: None,
graffiti_policy: None,
}
}

Expand Down Expand Up @@ -106,6 +109,11 @@ impl<S: ValidatorStore, T: SlotClock + 'static> BlockServiceBuilder<S, T> {
self
}

pub fn graffiti_policy(mut self, graffiti_policy: Option<GraffitiPolicy>) -> Self {
self.graffiti_policy = graffiti_policy;
self
}

pub fn build(self) -> Result<BlockService<S, T>, String> {
Ok(BlockService {
inner: Arc::new(Inner {
Expand All @@ -127,6 +135,7 @@ impl<S: ValidatorStore, T: SlotClock + 'static> BlockServiceBuilder<S, T> {
proposer_nodes: self.proposer_nodes,
graffiti: self.graffiti,
graffiti_file: self.graffiti_file,
graffiti_policy: self.graffiti_policy,
}),
})
}
Expand Down Expand Up @@ -193,6 +202,7 @@ pub struct Inner<S, T> {
chain_spec: Arc<ChainSpec>,
graffiti: Option<Graffiti>,
graffiti_file: Option<GraffitiFile>,
graffiti_policy: Option<GraffitiPolicy>,
}

/// Attempts to produce attestations for any block producer(s) at the start of the epoch.
Expand Down Expand Up @@ -460,6 +470,7 @@ impl<S: ValidatorStore + 'static, T: SlotClock + 'static> BlockService<S, T> {
graffiti,
proposer_index,
builder_boost_factor,
self_ref.graffiti_policy,
)
.await
.map_err(|e| {
Expand Down Expand Up @@ -523,13 +534,15 @@ impl<S: ValidatorStore + 'static, T: SlotClock + 'static> BlockService<S, T> {
graffiti: Option<Graffiti>,
proposer_index: Option<u64>,
builder_boost_factor: Option<u64>,
graffiti_policy: Option<GraffitiPolicy>,
) -> Result<UnsignedBlock<S::E>, BlockError> {
let (block_response, _) = beacon_node
.get_validator_blocks_v3::<S::E>(
slot,
randao_reveal_ref,
graffiti.as_ref(),
builder_boost_factor,
graffiti_policy,
)
.await
.map_err(|e| {
Expand Down
Loading