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
78 changes: 71 additions & 7 deletions crates/blockchain/fork_choice.rs
Original file line number Diff line number Diff line change
Expand Up @@ -99,11 +99,15 @@ pub async fn apply_fork_choice(
let head_res = store.get_block_header_by_hash(head_hash)?;

if !safe_hash.is_zero() {
check_order(&safe_res, &head_res)?;
check_order(&safe_res, &head_res, error::ForkChoiceElement::Safe)?;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Let's import ForkChoiceElement here

}

if !finalized_hash.is_zero() && !safe_hash.is_zero() {
check_order(&finalized_res, &safe_res)?;
check_order(
&finalized_res,
&safe_res,
error::ForkChoiceElement::Finalized,
)?;
}

let Some(head) = head_res else {
Expand Down Expand Up @@ -261,15 +265,17 @@ pub async fn apply_fork_choice(
}

// Checks that block 1 is prior to block 2 and that if the second is present, the first one is too.
// `missing` names the element `block_1` was looked up as, so an absent block is reported as the
// element the caller actually asked for. Hardcoding one element here mislabels every other call
// site, which sends whoever reads the log looking at the wrong forkchoice field.
fn check_order(
block_1: &Option<BlockHeader>,
block_2: &Option<BlockHeader>,
missing: error::ForkChoiceElement,
) -> Result<(), InvalidForkChoice> {
// We don't need to perform the check if the hashes are null
match (block_1, block_2) {
(None, Some(_)) => Err(InvalidForkChoice::ElementNotFound(
error::ForkChoiceElement::Finalized,
)),
(None, Some(_)) => Err(InvalidForkChoice::ElementNotFound(missing)),
(Some(b1), Some(b2)) => {
if b1.number > b2.number {
Err(InvalidForkChoice::Unordered)
Expand Down Expand Up @@ -501,8 +507,12 @@ async fn reorg_apply_deep(
.ok_or(InvalidForkChoice::StateNotReachable)?;
let to_block = pivot_number.saturating_add(1);
if edge < to_block {
// Pivot is above the cache edge ; `apply_fork_choice` should have
// succeeded as a shallow reorg. Bail.
// The overlay range [pivot+1, edge] is empty: no committed block sits above the
// pivot, so no overlay can make the head's state readable again. We only reach here
// after the shallow path already failed with an unreachable head state, so that
// state is genuinely absent locally — never committed, and its in-memory layers
// dropped — rather than merely shadowed by newer commits. Nothing local recovers
// it, so report it and let the caller sync.
warn!(
edge,
to_block, "deep-reorg path entered but pivot is above cache edge"
Expand Down Expand Up @@ -766,3 +776,57 @@ fn map_chain_error_for_fcu(err: ChainError, last_valid_hash: H256) -> InvalidFor
| ChainError::UnknownPayload => InvalidForkChoice::StateNotReachable,
}
}

#[cfg(test)]
mod tests {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Should we put this in the tests directory?

use super::*;

fn header(number: u64) -> BlockHeader {
BlockHeader {
number,
..Default::default()
}
}

#[test]
fn missing_element_is_reported_as_the_element_looked_up() {
let present = Some(header(10));

let err = check_order(&None, &present, error::ForkChoiceElement::Safe).unwrap_err();
assert!(matches!(
err,
InvalidForkChoice::ElementNotFound(error::ForkChoiceElement::Safe)
));

let err = check_order(&None, &present, error::ForkChoiceElement::Finalized).unwrap_err();
assert!(matches!(
err,
InvalidForkChoice::ElementNotFound(error::ForkChoiceElement::Finalized)
));
}

#[test]
fn known_blocks_in_the_wrong_order_are_unordered_not_missing() {
let err = check_order(
&Some(header(20)),
&Some(header(10)),
error::ForkChoiceElement::Safe,
)
.unwrap_err();
assert!(matches!(err, InvalidForkChoice::Unordered));
}

#[test]
fn known_blocks_in_order_pass() {
for (lower, upper) in [(10, 20), (10, 10)] {
assert!(
check_order(
&Some(header(lower)),
&Some(header(upper)),
error::ForkChoiceElement::Safe,
)
.is_ok()
);
}
}
}
70 changes: 51 additions & 19 deletions crates/networking/p2p/sync.rs
Original file line number Diff line number Diff line change
Expand Up @@ -296,34 +296,59 @@ impl Syncer {
)
.await;
}
METRICS.enable().await;
// We validate that we have the folders that are being used empty, as we currently assume
// they are. If they are not empty we empty the folder
delete_leaves_folder(&self.datadir);
let sync_cycle_result = snap_sync::sync_cycle_snap(
&mut self.peers,
self.blockchain.clone(),
&self.snap_enabled,
sync_head,
store,
&self.datadir,
&self.diagnostics,
)
.await;
METRICS.disable().await;
sync_cycle_result
self.run_snap_cycle(sync_head, store).await
} else {
full::sync_cycle_full(
let result = full::sync_cycle_full(
&mut self.peers,
self.blockchain.clone(),
self.cancel_token.clone(),
sync_head,
store,
store.clone(),
&self.diagnostics,
)
.await
.await;
if !matches!(result, Err(SyncError::StateUnrecoverable)) {
return result;
}
// Full sync walked all the way to genesis without finding a single block whose
// post-state we can still read, so it has no base to execute from. This happens
// when the layered store drops the state of every canonical block we hold —
// typically after a deep reorg unwinds past the retained window, leaving the
// canonical chain and the state history pointing at different branches.
//
// Full sync cannot dig itself out: it needs a stateful parent and there is none,
// so every later cycle repeats the same walk and pauses again while the chain
// moves on — the node goes quiet indefinitely. Snap sync is the only in-protocol
// way to obtain state we do not have, so escalate rather than stall until an
// operator notices and runs `ethrex removedb`.
warn!(
%sync_head,
"Full sync has no reachable state to resume from; escalating to snap sync"

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Suggested change
"Full sync has no reachable state to resume from; escalating to snap sync"
"Full sync has no reachable state to resume from; switching to snap sync"

);
self.snap_enabled.store(true, Ordering::Relaxed);
self.run_snap_cycle(sync_head, store).await
Comment on lines +352 to +353

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Should we check here if the sync-mode was set to snap-sync/default? If the sync-mode is full-sync, switching to snap-sync here can result in lost data.

}
}

/// Runs one snap-sync cycle, enabling snap metrics for its duration.
async fn run_snap_cycle(&mut self, sync_head: H256, store: Store) -> Result<(), SyncError> {
METRICS.enable().await;
// We validate that we have the folders that are being used empty, as we currently assume
// they are. If they are not empty we empty the folder
delete_leaves_folder(&self.datadir);
let sync_cycle_result = snap_sync::sync_cycle_snap(
&mut self.peers,
self.blockchain.clone(),
&self.snap_enabled,
sync_head,
store,
&self.datadir,
&self.diagnostics,
)
.await;
METRICS.disable().await;
sync_cycle_result
}
}

/// Number of attempts to fetch the sync head's header for the snap-vs-full pre-check.
Expand Down Expand Up @@ -401,6 +426,8 @@ pub enum SyncError {
CorruptDB,
#[error("Failed to fetch latest canonical block, unable to sync")]
NoLatestCanonical,
#[error("No block with a reachable post-state to resume full sync from, down to genesis")]
StateUnrecoverable,
#[error("Range received is invalid")]
InvalidRangeReceived,
#[error("Failed to fetch block number for head {0}")]
Expand Down Expand Up @@ -496,6 +523,11 @@ impl SyncError {
| SyncError::BlockNumber(_)
| SyncError::NoBlocks
| SyncError::NoBlockHeaders => true,
// `sync_cycle` escalates this to snap sync before it can reach the
// classifier, so reaching here means the escalation itself failed to run.
// Retry rather than exit: killing the process does not restore the missing
// state, and the restart path refuses to boot without it.
SyncError::StateUnrecoverable => true,
// PeerHandler handled above by delegation
SyncError::PeerHandler(_) => unreachable!(),
}
Expand Down
12 changes: 8 additions & 4 deletions crates/networking/p2p/sync/full.rs
Original file line number Diff line number Diff line change
Expand Up @@ -406,12 +406,16 @@ pub async fn sync_cycle_full(
local_head,
"Full sync cannot resume: post-state for block {resume_parent_number} is absent \
(pruned from the layered store, or never executed). The consensus sync head does \
not reconcile to a block whose state we retain; pausing until a reconcilable \
forkchoice head arrives. If this persists with no state above genesis, the datadir \
needs a fresh resync (ethrex removedb)."
not reconcile to a block whose state we retain, so there is nothing to execute \
on top of; handing over to snap sync to rebuild state from a recent pivot."
);
store.clear_fullsync_headers().await?;
return Ok(());
// Returning Ok here would pause the cycle and wait for a forkchoice head that
// reconciles to state we retain. When the walk has already bottomed out at
// genesis no such head exists — the state is gone, not merely unreferenced —
// so the wait never ends and the node stops following the chain entirely.
// Report it so the caller can escalate to snap sync.
Comment on lines +413 to +417

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Suggested change
// Returning Ok here would pause the cycle and wait for a forkchoice head that
// reconciles to state we retain. When the walk has already bottomed out at
// genesis no such head exists — the state is gone, not merely unreferenced —
// so the wait never ends and the node stops following the chain entirely.
// Report it so the caller can escalate to snap sync.
// Returning Ok here would pause the cycle.
// Report it so the caller can switch to snap sync.

return Err(SyncError::StateUnrecoverable);
}
// If we are resuming at or below the canonical head, the canonical chain extends
// past the executed-state head: an FCU canonicalized blocks before their state
Expand Down
14 changes: 13 additions & 1 deletion crates/networking/rpc/engine/fork_choice.rs
Original file line number Diff line number Diff line change
Expand Up @@ -382,10 +382,22 @@ async fn handle_forkchoice(
syncer.sync_to_head(fork_choice_state.head_block_hash);
ForkChoiceResponse::from(PayloadStatus::syncing())
}
InvalidForkChoice::Disconnected(_, _) | InvalidForkChoice::ElementNotFound(_) => {
InvalidForkChoice::Disconnected(_, _) => {
warn!("Invalid fork choice state. Reason: {:?}", forkchoice_error);
return Err(RpcErr::InvalidForkChoiceState(forkchoice_error.to_string()));
}
InvalidForkChoice::ElementNotFound(_) => {
// A safe/finalized block we simply do not have yet is missing data, not an
// inconsistent forkchoice: -38002 is for elements we DO hold but that are
// ordered wrongly (`Unordered`) or sit on a disjoint branch (`Disconnected`).
// Reporting the hard error here wedges a node that has fallen behind, because
// unlike `Syncing`/`StateNotReachable` this arm never starts a sync: the very
// blocks we are missing are never fetched, so every later FCU fails
// identically and the node stays stuck for as long as the CL keeps asking.
warn!("Fork choice element not found, syncing. Reason: {forkchoice_error:?}");
syncer.sync_to_head(fork_choice_state.head_block_hash);
ForkChoiceResponse::from(PayloadStatus::syncing())
}
InvalidForkChoice::TooDeepReorg { .. } => {
warn!("Rejecting fork choice update. Reason: {forkchoice_error}");
return Err(RpcErr::TooDeepReorg(forkchoice_error.to_string()));
Expand Down
Loading