Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 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
14 changes: 4 additions & 10 deletions beacon_node/beacon_chain/src/beacon_chain.rs
Original file line number Diff line number Diff line change
Expand Up @@ -144,11 +144,6 @@ pub const FORK_CHOICE_DB_KEY: Hash256 = Hash256::ZERO;
/// Defines how old a block can be before it's no longer a candidate for the early attester cache.
const EARLY_ATTESTER_CACHE_HISTORIC_SLOTS: u64 = 4;

/// Defines a distance between the head block slot and the current slot.
///
/// If the head block is older than this value, don't bother preparing beacon proposers.
const PREPARE_PROPOSER_HISTORIC_EPOCHS: u64 = 4;

/// If the head is more than `MAX_PER_SLOT_FORK_CHOICE_DISTANCE` slots behind the wall-clock slot, DO NOT
/// run the per-slot tasks (primarily fork choice).
///
Expand Down Expand Up @@ -4848,7 +4843,7 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
let proposer_index = if let Some(proposer) = cached_proposer {
proposer.index as u64
} else {
if head_epoch + 2 < proposal_epoch {
if head_epoch + self.config.sync_tolerance_epochs < proposal_epoch {
warn!(
self.log,
"Skipping proposer preparation";
Expand Down Expand Up @@ -6079,19 +6074,18 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
// Use a blocking task since blocking the core executor on the canonical head read lock can
// block the core tokio executor.
let chain = self.clone();
let tolerance_slots = self.config.sync_tolerance_epochs * T::EthSpec::slots_per_epoch();
let maybe_prep_data = self
.spawn_blocking_handle(
move || {
let cached_head = chain.canonical_head.cached_head();

// Don't bother with proposer prep if the head is more than
// `PREPARE_PROPOSER_HISTORIC_EPOCHS` prior to the current slot.
// `sync_tolerance_epochs` prior to the current slot.
//
// This prevents the routine from running during sync.
let head_slot = cached_head.head_slot();
if head_slot + T::EthSpec::slots_per_epoch() * PREPARE_PROPOSER_HISTORIC_EPOCHS
< current_slot
{
if head_slot + tolerance_slots < current_slot {
debug!(
chain.log,
"Head too old for proposer prep";
Expand Down
7 changes: 7 additions & 0 deletions beacon_node/beacon_chain/src/chain_config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,9 @@ pub const DEFAULT_PREPARE_PAYLOAD_LOOKAHEAD_FACTOR: u32 = 3;
/// Fraction of a slot lookahead for fork choice in the state advance timer (500ms on mainnet).
pub const FORK_CHOICE_LOOKAHEAD_FACTOR: u32 = 24;

/// Default sync tolerance epochs.
pub const DEFAULT_SYNC_TOLERANCE_EPOCHS: u64 = 16;
Copy link
Member Author

Choose a reason for hiding this comment

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

I haven't really given enough thoughts to this, so this PR changes the following

  • PREPARE_PROPOSER_HISTORIC_EPOCHS from 4 to sync_tolerance_epochs
  • get_pre_payload_attributes epochs from 2 to sync_tolerance_epochs
  • DEFAULT_SYNC_TOLERANCE_EPOCHS for http block production from 8 to sync_tolerance_epochs

sync_tolerance_epochs is now defaulted to 16.

We discussed about DEFAULT_SYNC_TOLERANCE_EPOCHS a while ago but I forgot why it was 8.

@AgeManning had a comment here #6880 (comment)

Copy link
Member

Choose a reason for hiding this comment

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

I think lets change this back to 8.

Now that we have a config flag for it, we have the option of getting people to set it in adverse chain conditions (sparse chain with legitimately very few blocks).

I'm worried that setting it to 16 might have contributed to the plethora of shitty sidechains we saw on Holesky. Whatever value we set here is essentially a bound on "we will build a shitty side chain during sync/stuckness of up to this length". I'd even be tempted to set the default lower, maybe 2 (as Age suggested originally) or 4.

Copy link
Member

Choose a reason for hiding this comment

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

As agreed on the call today, I've lowered the default to 2 in this commit: c7105d1.

In case of a sparse chain and liveness issues, we can advise validators to bump up the sync tolerance temporarily.


#[derive(Debug, PartialEq, Eq, Clone, Deserialize, Serialize)]
pub struct ChainConfig {
/// Maximum number of slots to skip when importing an attestation.
Expand Down Expand Up @@ -94,6 +97,9 @@ pub struct ChainConfig {
/// The delay in milliseconds applied by the node between sending each blob or data column batch.
/// This doesn't apply if the node is the block proposer.
pub blob_publication_batch_interval: Duration,
/// The max distance between the head block and the current slot at which Lighthouse will
/// consider itself synced and still serve validator-related requests.
pub sync_tolerance_epochs: u64,
}

impl Default for ChainConfig {
Expand Down Expand Up @@ -129,6 +135,7 @@ impl Default for ChainConfig {
enable_sampling: false,
blob_publication_batches: 4,
blob_publication_batch_interval: Duration::from_millis(300),
sync_tolerance_epochs: DEFAULT_SYNC_TOLERANCE_EPOCHS,
}
}
}
Expand Down
15 changes: 2 additions & 13 deletions beacon_node/http_api/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -107,13 +107,6 @@ use warp_utils::{query::multi_key_query, reject::convert_rejection, uor::Unifyin

const API_PREFIX: &str = "eth";

/// If the node is within this many epochs from the head, we declare it to be synced regardless of
/// the network sync state.
///
/// This helps prevent attacks where nodes can convince us that we're syncing some non-existent
/// finalized head.
const DEFAULT_SYNC_TOLERANCE_EPOCHS: u64 = 8;

/// A custom type which allows for both unsecured and TLS-enabled HTTP servers.
type HttpServer = (SocketAddr, Pin<Box<dyn Future<Output = ()> + Send>>);

Expand Down Expand Up @@ -157,7 +150,6 @@ pub struct Config {
pub duplicate_block_status_code: StatusCode,
pub enable_light_client_server: bool,
pub target_peers: usize,
pub sync_tolerance_epochs: Option<u64>,
}

impl Default for Config {
Expand All @@ -174,7 +166,6 @@ impl Default for Config {
duplicate_block_status_code: StatusCode::ACCEPTED,
enable_light_client_server: true,
target_peers: 100,
sync_tolerance_epochs: None,
}
}
}
Expand Down Expand Up @@ -475,10 +466,8 @@ pub fn serve<T: BeaconChainTypes>(
)
})?;

let sync_tolerance_epochs = config
.sync_tolerance_epochs
.unwrap_or(DEFAULT_SYNC_TOLERANCE_EPOCHS);
let tolerance = sync_tolerance_epochs * T::EthSpec::slots_per_epoch();
let tolerance =
chain.config.sync_tolerance_epochs * T::EthSpec::slots_per_epoch();

if head_slot + tolerance >= current_slot {
Ok(())
Expand Down
11 changes: 7 additions & 4 deletions beacon_node/src/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ use beacon_chain::graffiti_calculator::GraffitiOrigin;
use beacon_chain::TrustedSetup;
use clap::{parser::ValueSource, ArgMatches, Id};
use clap_utils::flags::DISABLE_MALLOC_TUNING_FLAG;
use clap_utils::{parse_flag, parse_optional, parse_required};
use clap_utils::{parse_flag, parse_required};
use client::{ClientConfig, ClientGenesis};
use directory::{DEFAULT_BEACON_NODE_DIR, DEFAULT_NETWORK_DIR, DEFAULT_ROOT_DIR};
use environment::RuntimeContext;
Expand Down Expand Up @@ -177,9 +177,6 @@ pub fn get_config<E: EthSpec>(

client_config.http_api.enable_light_client_server =
!cli_args.get_flag("disable-light-client-server");

client_config.http_api.sync_tolerance_epochs =
parse_optional(cli_args, "sync-tolerance-epochs")?;
}

if cli_args.get_flag("light-client-server") {
Expand All @@ -194,6 +191,12 @@ pub fn get_config<E: EthSpec>(
client_config.chain.enable_light_client_server = false;
}

if let Some(sync_tolerance_epochs) =
clap_utils::parse_optional(cli_args, "sync-tolerance-epochs")?
{
client_config.chain.sync_tolerance_epochs = sync_tolerance_epochs;
}

if let Some(cache_size) = clap_utils::parse_optional(cli_args, "shuffling-cache-size")? {
client_config.chain.shuffling_cache_size = cache_size;
}
Expand Down
17 changes: 15 additions & 2 deletions lighthouse/tests/beacon_node.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
use crate::exec::{CommandLineTestExec, CompletedTest};
use beacon_node::beacon_chain::chain_config::{
DisallowedReOrgOffsets, DEFAULT_RE_ORG_CUTOFF_DENOMINATOR, DEFAULT_RE_ORG_HEAD_THRESHOLD,
DEFAULT_RE_ORG_MAX_EPOCHS_SINCE_FINALIZATION,
DEFAULT_RE_ORG_MAX_EPOCHS_SINCE_FINALIZATION, DEFAULT_SYNC_TOLERANCE_EPOCHS,
};
use beacon_node::{
beacon_chain::graffiti_calculator::GraffitiOrigin,
Expand Down Expand Up @@ -2586,7 +2586,20 @@ fn sync_tolerance_epochs() {
.flag("sync-tolerance-epochs", Some("0"))
.run_with_zero_port()
.with_config(|config| {
assert_eq!(config.http_api.sync_tolerance_epochs, Some(0));
assert_eq!(config.chain.sync_tolerance_epochs, 0);
});
}

#[test]
fn sync_tolerance_epochs_default() {
CommandLineTest::new()
.flag("http", None)
.run_with_zero_port()
.with_config(|config| {
assert_eq!(
config.chain.sync_tolerance_epochs,
DEFAULT_SYNC_TOLERANCE_EPOCHS
);
});
}

Expand Down