From b0604bbcdfa77f6b5cc9fa1f9397c002dcd2beb8 Mon Sep 17 00:00:00 2001 From: tbro <48967308+tbro@users.noreply.github.com> Date: Wed, 2 Oct 2024 10:02:29 -0500 Subject: [PATCH] Validation bugfix (#2089) Adds missing validation steps * validate timestamp (against system time and previous block) * wait for l1_finalized * wait for l1_head * validate blocks are not decrementing * small update to pee-existing tests to avoid hitting these cases * adds TODOs for followups --------- Co-authored-by: tbro Co-authored-by: Jeb Bearer Co-authored-by: Mathis --- .github/workflows/test.yml | 2 +- types/src/v0/impls/header.rs | 3 ++ types/src/v0/impls/l1.rs | 54 ++++++++++++++------------- types/src/v0/impls/state.rs | 71 +++++++++++++++++++++++++++++++++++- 4 files changed, 103 insertions(+), 27 deletions(-) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 58317411e..5087c0c79 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -63,7 +63,7 @@ jobs: - name: Test # Build test binary with `testing` feature, which requires `hotshot_example` config run: cargo nextest run --locked --release --workspace --all-features --verbose --no-fail-fast - timeout-minutes: 5 + timeout-minutes: 20 - name: Install process-compose run: nix profile install nixpkgs#process-compose diff --git a/types/src/v0/impls/header.rs b/types/src/v0/impls/header.rs index a2ad5902d..6315a4dda 100644 --- a/types/src/v0/impls/header.rs +++ b/types/src/v0/impls/header.rs @@ -1374,6 +1374,7 @@ mod test_headers { #[async_std::test] async fn test_validate_proposal_error_cases() { + // TODO add assertion for timestamp validation let genesis = GenesisForTest::default().await; let vid_common = vid_scheme(1).disperse([]).unwrap().common; @@ -1390,6 +1391,8 @@ mod test_headers { validated_state.block_merkle_tree = block_merkle_tree.clone(); *parent_header.block_merkle_tree_root_mut() = block_merkle_tree_root; let mut proposal = parent_header.clone(); + *proposal.timestamp_mut() = OffsetDateTime::now_utc().unix_timestamp() as u64; + *proposal.l1_head_mut() = 5; let ver = StaticVersion::<0, 1>::version(); diff --git a/types/src/v0/impls/l1.rs b/types/src/v0/impls/l1.rs index 436fe1f8e..a49dced56 100644 --- a/types/src/v0/impls/l1.rs +++ b/types/src/v0/impls/l1.rs @@ -79,6 +79,34 @@ impl L1Client { L1Snapshot { head, finalized } } + pub async fn wait_for_block(&self, number: u64) -> L1BlockInfo { + let interval = self.provider.get_interval(); + loop { + let block = match self.provider.get_block(number).await { + Ok(Some(block)) => block, + Ok(None) => { + tracing::info!(number, "no such block"); + sleep(interval).await; + continue; + } + Err(err) => { + tracing::error!(%err, number, "failed to get L1 block"); + sleep(interval).await; + continue; + } + }; + let Some(hash) = block.hash else { + tracing::error!(number, ?block, "L1 block has no hash"); + sleep(interval).await; + continue; + }; + break L1BlockInfo { + number, + hash, + timestamp: block.timestamp, + }; + } + } /// Get information about the given block. /// /// If the desired block number is not finalized yet, this function will block until it becomes @@ -107,31 +135,7 @@ impl L1Client { // The finalized block may have skipped over the block of interest. In this case, our block // is still finalized, since it is before the finalized block. We just need to fetch it. - loop { - let block = match self.provider.get_block(number).await { - Ok(Some(block)) => block, - Ok(None) => { - tracing::error!(number, "no such block"); - sleep(interval).await; - continue; - } - Err(err) => { - tracing::error!(%err, number, "failed to get L1 block"); - sleep(interval).await; - continue; - } - }; - let Some(hash) = block.hash else { - tracing::error!(number, ?block, "L1 block has no hash"); - sleep(interval).await; - continue; - }; - break L1BlockInfo { - number, - hash, - timestamp: block.timestamp, - }; - } + self.wait_for_block(number).await } /// Proxy to `Provider.get_block_number`. diff --git a/types/src/v0/impls/state.rs b/types/src/v0/impls/state.rs index a6ce72db9..5487dfe5e 100644 --- a/types/src/v0/impls/state.rs +++ b/types/src/v0/impls/state.rs @@ -23,6 +23,7 @@ use num_traits::CheckedSub; use serde::{Deserialize, Serialize}; use std::ops::Add; use thiserror::Error; +use time::OffsetDateTime; use vbs::version::Version; use super::{ @@ -94,6 +95,20 @@ pub enum ProposalValidationError { InvalidNsTable { err: NsTableValidationError }, #[error("Some fee amount or their sum total out of range")] SomeFeeAmountOutOfRange, + #[error("Invalid timestamp: proposal={proposal_timestamp}, parent={parent_timestamp}")] + InvalidTimestampNonIncrementing { + proposal_timestamp: u64, + parent_timestamp: u64, + }, + #[error("Invalid timestamp: local:={local_timestamp}, proposal={proposal_timestamp}")] + InvalidTimestampDrift { + proposal_timestamp: u64, + local_timestamp: u64, + }, + #[error("l1_finalized has `None` value")] + L1FinalizedNotFound, + #[error("Invalid timestamp: parent:={parent}, proposal={proposal}")] + NonIncrementingL1Head { parent: u64, proposal: u64 }, } impl StateDelta for Delta {} @@ -306,6 +321,26 @@ pub fn validate_proposal( }); } + // Validate timestamp increasing. + // TODO add test https://github.com/EspressoSystems/espresso-sequencer/issues/2100 + if proposal.timestamp() < parent_header.timestamp() { + return Err(ProposalValidationError::InvalidTimestampNonIncrementing { + proposal_timestamp: proposal.timestamp(), + parent_timestamp: parent_header.timestamp(), + }); + } + + // Validate timestamp hasn't drifted too much from system time. + let system_time: u64 = OffsetDateTime::now_utc().unix_timestamp() as u64; + // TODO 2 seconds of tolerance should be enough for reasonably + // configured nodes, but we should make this configurable. + if proposal.timestamp().abs_diff(system_time) > 2 { + return Err(ProposalValidationError::InvalidTimestampDrift { + proposal_timestamp: proposal.timestamp(), + local_timestamp: system_time, + }); + } + let ValidatedState { block_merkle_tree, fee_merkle_tree, @@ -663,6 +698,40 @@ impl HotShotState for ValidatedState { .resolve() .expect("Chain Config not found in validated state"); + // Validate l1_finalized. + // TODO add test https://github.com/EspressoSystems/espresso-sequencer/issues/2100 + let proposed_finalized = proposed_header.l1_finalized(); + let parent_finalized = parent_leaf.block_header().l1_finalized(); + if proposed_finalized < parent_finalized { + tracing::error!( + "L1 finalized not incrementing. parent: {:?}, proposal: {:?}", + parent_finalized, + proposed_finalized, + ); + } + if let Some(proposed_finalized) = proposed_finalized { + let finalized = instance + .l1_client + .wait_for_finalized_block(proposed_finalized.number()) + .await; + + if finalized != proposed_finalized { + tracing::error!("Invalid proposal: l1_finalized mismatch"); + return Err(BlockError::InvalidBlockHeader); + } + } + // Validate `l1_head`. + // TODO add test https://github.com/EspressoSystems/espresso-sequencer/issues/2100 + if proposed_header.l1_head() < parent_leaf.block_header().l1_head() { + tracing::error!("Invalid proposal: l1_head decreasing"); + return Err(BlockError::InvalidBlockHeader); + } + + let _ = instance + .l1_client + .wait_for_block(proposed_header.l1_head()) + .await; + // validate the proposal if let Err(err) = validate_proposal( &validated_state, @@ -671,7 +740,7 @@ impl HotShotState for ValidatedState { proposed_header, &vid_common, ) { - tracing::error!("invalid proposal: {err:#}"); + tracing::error!("Invalid proposal: {err:#}"); return Err(BlockError::InvalidBlockHeader); }