Skip to content
Open
Show file tree
Hide file tree
Changes from 12 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
2 changes: 1 addition & 1 deletion Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -290,7 +290,7 @@ debug = true
[patch.crates-io]
# FIXME(eip-7688): EIP-7688 development patches
ssz_types = { git = "https://github.com/sigp/ssz_types", rev = "8955d22edc7633f26fe3b5ac933facb5121d717a" }
milhouse = { git = "https://github.com/sigp/milhouse", rev = "d67aabd2a7c405b3c16f4aaff159b137dc5b5e38" }
milhouse = { git = "https://github.com/sigp/milhouse", rev = "e7c611a7d768cf303b02797d9d2000f9b5e2bb41" }
ethereum_ssz = { git = "https://github.com/sigp/ethereum_ssz", rev = "974f78f421fbf70cba4e8f84e0fa28c23ad54f34" }
ethereum_ssz_derive = { git = "https://github.com/sigp/ethereum_ssz", rev = "974f78f421fbf70cba4e8f84e0fa28c23ad54f34" }
tree_hash = { git = "https://github.com/sigp/tree_hash", rev = "e5ea1875edddf12438506c3751cbb313f9c52d08" }
Expand Down
4 changes: 1 addition & 3 deletions beacon_node/beacon_chain/src/chain_config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -61,8 +61,6 @@ pub struct ChainConfig {
pub paranoid_block_proposal: bool,
/// Optionally set timeout for calls to checkpoint sync endpoint.
pub checkpoint_sync_url_timeout: u64,
/// The offset before the start of a proposal slot at which payload attributes should be sent.
///
/// Low values are useful for execution engines which don't improve their payload after the
/// first call, and high values are useful for ensuring the EL is given ample notice.
pub prepare_payload_lookahead: Duration,
Expand Down Expand Up @@ -159,7 +157,7 @@ impl Default for ChainConfig {
always_reset_payload_statuses: false,
paranoid_block_proposal: false,
checkpoint_sync_url_timeout: 60,
prepare_payload_lookahead: Duration::from_secs(4),
prepare_payload_lookahead: Duration::from_secs(4),
// This value isn't actually read except in tests.
optimistic_finalized_sync: true,
shuffling_cache_size: crate::shuffling_cache::DEFAULT_CACHE_SIZE,
Expand Down
15 changes: 15 additions & 0 deletions beacon_node/client/src/builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -452,6 +452,21 @@ where
})?
.ok_or("Finalized block missing from remote, it returned 404")?;
let block_root = block.canonical_root();
// If --wss-checkpoint was provided, verify the downloaded block root matches it
// before initializing the chain from the checkpoint state.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The checkpoint state also has to be validated. i.e., have to verify that the block header's state_root matches the state's root, and that both of them are referring to the same slot / block.

Given that you only add new logic here, I wonder if this is a problem related to the PR, or already there before.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Yeah thats checked, this predates the PR. The state and block are fetched independently a few lines above with no cross-check between them already there. My change only adds the root-vs---wss-checkpoint comparison after that, I didn’t touch the fetch logic itself. It's definitely worth fixing, but it’s a separate, existing gap, not something I introduced. Happy to pick it up as a follow-up, or fold it into this PR if you’d rather bundle it.

if let Some(wss_checkpoint) = config.chain.weak_subjectivity_checkpoint {
if block_root != wss_checkpoint.root {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Hmm, given that --wss-checkpoint was already used before, is this compatible with its earlier use?
e.g., if the user passes a checkpoint that's slightly older than the freshest one, would it still work?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Also, in PR description --checkpoint-sync-url is mentioned, is that still applicable?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

No Im still using --wss-checkpoint as the source of the root, that part’s unchanged. The gap is in how I compare it: so my check does flat equality against the freshest downloaded block, so it would reject a checkpoint that’s slightly older than the tip, something the existing verify_weak_subjectivity_checkpoint already tolerates by walking backward through block roots when the checkpoint’s epoch is older than the current finalized one. I’ll rework the comparison to match that existing backward-walk behavior instead of a flat equality check.

return Err(format!(
"Checkpoint sync block root mismatch. Expected (from --wss-checkpoint): {:?}, \
Downloaded: {:?}. The checkpoint provider may be serving a different chain.",
wss_checkpoint.root, block_root
));
}
info!(
block_root = ?block_root,
"Verified checkpoint block root matches --wss-checkpoint"
);
}

debug!("Downloaded finalized block");

Expand Down
1 change: 1 addition & 0 deletions beacon_node/src/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -533,6 +533,7 @@ pub fn get_config<E: EthSpec>(
clap_utils::parse_required(cli_args, "genesis-state-url-timeout")
.map(Duration::from_secs)?;


let genesis_state_url_opt =
clap_utils::parse_optional::<String>(cli_args, "genesis-state-url")?;
let checkpoint_sync_url_opt =
Expand Down
27 changes: 27 additions & 0 deletions consensus/types/src/beacon_state_summary/beacon_state_part.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
use serde::{Deserialize, Serialize};
use ssz_derive::{Decode, Encode};
use ssz_types::ProgressiveVariableList;
use tree_hash_derive::TreeHash;

use crate::core::Hash256;

/// One fetchable slice of state data, returned by `beacon_state_parts_by_range`: the chunk's
/// index, its raw bytes, and a Merkle proof (`branch`) tying it to an already-verified
/// [`super::BeaconStateSnapshot`] summary root. Different chunks can be fetched from different,
/// mutually untrusting peers and verified independently.
#[cfg_attr(feature = "arbitrary", derive(arbitrary::Arbitrary))]
#[derive(Debug, Clone, PartialEq, Serialize, Deserialize, Encode, Decode, TreeHash)]
#[serde(deny_unknown_fields)]
pub struct BeaconStatePart {
#[serde(with = "serde_utils::quoted_u64")]
pub chunk_index: u64,
pub data: ProgressiveVariableList<u8>,
pub branch: ProgressiveVariableList<Hash256>,
}

#[cfg(test)]
mod tests {
use super::*;

ssz_and_tree_hash_tests!(BeaconStatePart);
}
25 changes: 25 additions & 0 deletions consensus/types/src/beacon_state_summary/beacon_state_snapshot.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
use serde::{Deserialize, Serialize};
use ssz_derive::{Decode, Encode};
use ssz_types::ProgressiveVariableList;
use tree_hash_derive::TreeHash;

use crate::{beacon_state_summary::BeaconStateSummary, core::EthSpec, core::Hash256};

/// Response to a `beacon_state_summary` request: a [`BeaconStateSummary`] plus a Merkle proof
/// (`state_branch`) tying its root to the already-trusted block root the requester started from.
#[cfg_attr(feature = "arbitrary", derive(arbitrary::Arbitrary))]
#[derive(Debug, Clone, PartialEq, Serialize, Deserialize, Encode, Decode, TreeHash)]
#[serde(bound = "E: EthSpec", deny_unknown_fields)]
#[cfg_attr(feature = "arbitrary", arbitrary(bound = "E: EthSpec"))]
pub struct BeaconStateSnapshot<E: EthSpec> {
pub summary: BeaconStateSummary<E>,
pub state_branch: ProgressiveVariableList<Hash256>,
}

#[cfg(test)]
mod tests {
use super::*;
use crate::MinimalEthSpec;

ssz_and_tree_hash_tests!(BeaconStateSnapshot<MinimalEthSpec>);
}
Loading