diff --git a/sequencer/src/block.rs b/sequencer/src/block.rs index b7724b1e70..033c7253fb 100644 --- a/sequencer/src/block.rs +++ b/sequencer/src/block.rs @@ -2,7 +2,7 @@ mod full_payload; mod namespace_payload; mod uint_bytes; -pub use full_payload::{NsProof, NsTable, NsTableValidationError, Payload}; +pub use full_payload::{NsProof, NsTable, NsTableValidationError, Payload, PayloadByteLen}; #[cfg(test)] mod test; diff --git a/sequencer/src/block/full_payload.rs b/sequencer/src/block/full_payload.rs index d3d8cf201e..61247ec87e 100644 --- a/sequencer/src/block/full_payload.rs +++ b/sequencer/src/block/full_payload.rs @@ -4,7 +4,6 @@ mod payload; pub use ns_proof::NsProof; pub use ns_table::{NsIndex, NsTable, NsTableValidationError}; -pub use payload::Payload; +pub use payload::{Payload, PayloadByteLen}; pub(in crate::block) use ns_table::NsIter; -pub(in crate::block) use payload::PayloadByteLen; diff --git a/sequencer/src/block/full_payload/ns_proof.rs b/sequencer/src/block/full_payload/ns_proof.rs index 9382ba08c6..104ca45f4f 100644 --- a/sequencer/src/block/full_payload/ns_proof.rs +++ b/sequencer/src/block/full_payload/ns_proof.rs @@ -37,8 +37,16 @@ impl NsProof { /// endpoint API at [`endpoints`](crate::api::endpoints), which is a hassle. pub fn new(payload: &Payload, index: &NsIndex, common: &VidCommon) -> Option { let payload_byte_len = payload.byte_len(); - payload_byte_len.is_consistent(common).ok()?; + if !payload_byte_len.is_consistent(common) { + tracing::warn!( + "payload byte len {} inconsistent with common {}", + payload_byte_len, + VidSchemeType::get_payload_byte_len(common) + ); + return None; // error: payload byte len inconsistent with common + } if !payload.ns_table().in_bounds(index) { + tracing::warn!("ns_index {:?} out of bounds", index); return None; // error: index out of bounds } let ns_payload_range = payload.ns_table().ns_range(index, &payload_byte_len); diff --git a/sequencer/src/block/full_payload/ns_table.rs b/sequencer/src/block/full_payload/ns_table.rs index 3254115e45..50ce3489e6 100644 --- a/sequencer/src/block/full_payload/ns_table.rs +++ b/sequencer/src/block/full_payload/ns_table.rs @@ -144,7 +144,9 @@ impl<'de> Deserialize<'de> for NsTable { D: Deserializer<'de>, { let unchecked = NsTable::deserialize(deserializer)?; - unchecked.validate().map_err(de::Error::custom)?; + unchecked + .validate_deserialization_invariants() + .map_err(de::Error::custom)?; Ok(unchecked) } } @@ -219,49 +221,30 @@ impl NsTable { /// 1. Byte length must hold a whole number of entries. /// 2. All namespace IDs and offsets must increase monotonically. Offsets /// must be nonzero. - /// 3. Header consistent with byte length (obsolete after - /// ) - pub fn validate(&self) -> Result<(), NsTableValidationError> { + /// 3. Header consistent with byte length. (Obsolete after + /// .) + /// 4. Final offset must equal `payload_byte_len`. (Obsolete after + /// .) + /// If the namespace table is empty then `payload_byte_len` must be 0. + pub fn validate( + &self, + payload_byte_len: &PayloadByteLen, + ) -> Result<(), NsTableValidationError> { use NsTableValidationError::*; - // Byte length for a table with `x` entries must be exactly - // `x * NsTableBuilder::entry_byte_len() + NsTableBuilder::header_byte_len()` - if self.bytes.len() < NsTableBuilder::header_byte_len() - || (self.bytes.len() - NsTableBuilder::header_byte_len()) - % NsTableBuilder::entry_byte_len() - != 0 - { - return Err(InvalidByteLen); - } + // conditions 1-3 + self.validate_deserialization_invariants()?; - // Header must declare the correct number of namespaces - // - // TODO this check obsolete after - // https://github.com/EspressoSystems/espresso-sequencer/issues/1604 - if self.len().0 != self.read_num_nss() { - return Err(InvalidHeader); - } - - // Namespace IDs and offsets must increase monotonically. Offsets must - // be nonzero. - { - let (mut prev_ns_id, mut prev_offset) = (None, 0); - for (ns_id, offset) in self.iter().map(|i| { - ( - self.read_ns_id_unchecked(&i), - self.read_ns_offset_unchecked(&i), - ) - }) { - if let Some(prev_ns_id) = prev_ns_id { - if ns_id <= prev_ns_id { - return Err(NonIncreasingEntries); - } - } - if offset <= prev_offset { - return Err(NonIncreasingEntries); - } - (prev_ns_id, prev_offset) = (Some(ns_id), offset); + // condition 4 + let len = self.len().0; + if len > 0 { + let final_ns_index = NsIndex(len - 1); + let final_offset = self.read_ns_offset_unchecked(&final_ns_index); + if final_offset != payload_byte_len.as_usize() { + return Err(InvalidFinalOffset); } + } else if payload_byte_len.as_usize() != 0 { + return Err(ExpectNonemptyNsTable); } Ok(()) @@ -306,6 +289,65 @@ impl NsTable { index.0 * (NS_ID_BYTE_LEN + NS_OFFSET_BYTE_LEN) + NUM_NSS_BYTE_LEN + NS_ID_BYTE_LEN; usize_from_bytes::(&self.bytes[start..start + NS_OFFSET_BYTE_LEN]) } + + /// Helper for [`NsTable::validate`], used in our custom [`serde`] + /// implementation. + /// + /// Checks conditions 1-3 of [`NsTable::validate`]. Those conditions can be + /// checked by looking only at the contents of the [`NsTable`]. + fn validate_deserialization_invariants(&self) -> Result<(), NsTableValidationError> { + use NsTableValidationError::*; + + // Byte length for a table with `x` entries must be exactly `x * + // NsTableBuilder::entry_byte_len() + + // NsTableBuilder::header_byte_len()`. + // + // Explanation for the following `if` condition: + // + // The above condition is equivalent to `[byte length] - + // header_byte_len` equals 0 modulo `entry_byte_len`. In order to + // compute `[byte length] - header_byte_len` we must first check that + // `[byte length]` is not exceeded by `header_byte_len` + if self.bytes.len() < NsTableBuilder::header_byte_len() + || (self.bytes.len() - NsTableBuilder::header_byte_len()) + % NsTableBuilder::entry_byte_len() + != 0 + { + return Err(InvalidByteLen); + } + + // Header must declare the correct number of namespaces + // + // TODO this check obsolete after + // https://github.com/EspressoSystems/espresso-sequencer/issues/1604 + if self.len().0 != self.read_num_nss() { + return Err(InvalidHeader); + } + + // Namespace IDs and offsets must increase monotonically. Offsets must + // be nonzero. + { + let (mut prev_ns_id, mut prev_offset) = (None, 0); + for (ns_id, offset) in self.iter().map(|i| { + ( + self.read_ns_id_unchecked(&i), + self.read_ns_offset_unchecked(&i), + ) + }) { + if let Some(prev_ns_id) = prev_ns_id { + if ns_id <= prev_ns_id { + return Err(NonIncreasingEntries); + } + } + if offset <= prev_offset { + return Err(NonIncreasingEntries); + } + (prev_ns_id, prev_offset) = (Some(ns_id), offset); + } + } + + Ok(()) + } } impl EncodeBytes for NsTable { @@ -332,6 +374,8 @@ pub enum NsTableValidationError { InvalidByteLen, NonIncreasingEntries, InvalidHeader, // TODO this variant obsolete after https://github.com/EspressoSystems/espresso-sequencer/issues/1604 + InvalidFinalOffset, // TODO this variant obsolete after https://github.com/EspressoSystems/espresso-sequencer/issues/1604 + ExpectNonemptyNsTable, } pub struct NsTableBuilder { @@ -379,7 +423,7 @@ impl NsTableBuilder { } /// Index for an entry in a ns table. -#[derive(Clone, Debug, Eq, Hash, PartialEq)] +#[derive(Clone, Debug, Display, Eq, Hash, PartialEq)] pub struct NsIndex(usize); bytes_serde_impl!(NsIndex, to_bytes, [u8; NUM_NSS_BYTE_LEN], from_bytes); diff --git a/sequencer/src/block/full_payload/ns_table/test.rs b/sequencer/src/block/full_payload/ns_table/test.rs index 354fb81ba4..df92d25e11 100644 --- a/sequencer/src/block/full_payload/ns_table/test.rs +++ b/sequencer/src/block/full_payload/ns_table/test.rs @@ -3,10 +3,14 @@ use super::{ NUM_NSS_BYTE_LEN, }; use crate::{ - block::uint_bytes::{u32_max_from_byte_len, usize_max_from_byte_len, usize_to_bytes}, - NamespaceId, + block::{ + test::ValidTest, + uint_bytes::{u32_max_from_byte_len, usize_max_from_byte_len, usize_to_bytes}, + }, + NamespaceId, Payload, }; use async_compatibility_layer::logging::{setup_backtrace, setup_logging}; +use hotshot::traits::BlockPayload; use rand::{Rng, RngCore}; use NsTableValidationError::*; @@ -22,7 +26,7 @@ fn random_valid() { } #[test] -fn byte_len() { +fn ns_table_byte_len() { setup_logging(); setup_backtrace(); let mut rng = jf_utils::test_rng(); @@ -57,6 +61,70 @@ fn byte_len() { } } +#[async_std::test] +async fn payload_byte_len() { + setup_logging(); + setup_backtrace(); + let test_case = vec![vec![5, 8, 8], vec![7, 9, 11], vec![10, 5, 8]]; + let mut rng = jf_utils::test_rng(); + let test = ValidTest::from_tx_lengths(test_case, &mut rng); + let mut block = + Payload::from_transactions(test.all_txs(), &Default::default(), &Default::default()) + .await + .unwrap() + .0; + let payload_byte_len = block.byte_len(); + let final_offset = block + .ns_table() + .read_ns_offset_unchecked(&block.ns_table().iter().last().unwrap()); + + // final offset matches payload byte len + block.ns_table().validate(&payload_byte_len).unwrap(); + + // Helper closure fn: modify the final offset of `block`'s namespace table + // by adding `diff` to it. Assert failure. + let mut modify_final_offset = |diff: isize| { + let ns_table_byte_len = block.ns_table().bytes.len(); + let old_final_offset: isize = final_offset.try_into().unwrap(); + let new_final_offset: usize = (old_final_offset + diff).try_into().unwrap(); + + block.ns_table_mut().bytes[ns_table_byte_len - NS_OFFSET_BYTE_LEN..] + .copy_from_slice(&usize_to_bytes::(new_final_offset)); + assert_eq!( + block.ns_table().validate(&payload_byte_len).unwrap_err(), + InvalidFinalOffset + ); + }; + + // final offset exceeds payload byte len + modify_final_offset(1); + + // final offset less than payload byte len + modify_final_offset(-1); + + // zero-length payload + let empty_block = Payload::from_transactions([], &Default::default(), &Default::default()) + .await + .unwrap() + .0; + assert_eq!(empty_block.ns_table().len().0, 0); + assert_eq!( + empty_block.ns_table().bytes, + usize_to_bytes::(0) + ); + empty_block + .ns_table() + .validate(&empty_block.byte_len()) + .unwrap(); + + // empty namespace table with nonempty payload + *block.ns_table_mut() = empty_block.ns_table().clone(); + assert_eq!( + block.ns_table().validate(&payload_byte_len).unwrap_err(), + ExpectNonemptyNsTable + ); +} + #[test] fn monotonic_increase() { setup_logging(); @@ -149,7 +217,7 @@ where fn expect_valid(ns_table: &NsTable) { // `validate` should succeed - ns_table.validate().unwrap(); + ns_table.validate_deserialization_invariants().unwrap(); // serde round-trip should succeed let serde_bytes = bincode::serialize(ns_table).unwrap(); @@ -161,7 +229,10 @@ fn expect_invalid(ns_table: &NsTable, err: NsTableValidationError) { use serde::de::Error; // `validate` should fail - assert_eq!(ns_table.validate().unwrap_err(), err); + assert_eq!( + ns_table.validate_deserialization_invariants().unwrap_err(), + err + ); // serde round-trip should fail // diff --git a/sequencer/src/block/full_payload/payload.rs b/sequencer/src/block/full_payload/payload.rs index 418dc4f42e..b0078ff092 100644 --- a/sequencer/src/block/full_payload/payload.rs +++ b/sequencer/src/block/full_payload/payload.rs @@ -8,6 +8,7 @@ use crate::{ use async_trait::async_trait; use committable::Committable; +use derive_more::Display; use hotshot_query_service::availability::QueryablePayload; use hotshot_types::{ traits::{BlockPayload, EncodeBytes}, @@ -17,7 +18,7 @@ use hotshot_types::{ use jf_vid::VidScheme; use serde::{Deserialize, Serialize}; use sha2::Digest; -use std::{collections::BTreeMap, fmt::Display, sync::Arc}; +use std::{collections::BTreeMap, sync::Arc}; /// Raw payload data for an entire block. /// @@ -253,7 +254,7 @@ impl QueryablePayload for Payload { } } -impl Display for Payload { +impl std::fmt::Display for Payload { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { write!(f, "{self:#?}") } @@ -267,7 +268,8 @@ impl EncodeBytes for Payload { /// Byte length of a block payload, which includes all namespaces but *not* the /// namespace table. -pub(in crate::block) struct PayloadByteLen(usize); +#[derive(Clone, Debug, Display, Eq, Hash, PartialEq)] +pub struct PayloadByteLen(usize); impl PayloadByteLen { /// Extract payload byte length from a [`VidCommon`] and construct a new [`Self`] from it. @@ -276,13 +278,21 @@ impl PayloadByteLen { } /// Is the payload byte length declared in a [`VidCommon`] equal [`Self`]? - pub fn is_consistent(&self, common: &VidCommon) -> Result<(), ()> { + pub fn is_consistent(&self, common: &VidCommon) -> bool { // failure to convert to usize implies that `common` cannot be // consistent with `self`. - let expected = - usize::try_from(VidSchemeType::get_payload_byte_len(common)).map_err(|_| ())?; + let expected = match usize::try_from(VidSchemeType::get_payload_byte_len(common)) { + Ok(n) => n, + Err(_) => { + tracing::warn!( + "VidCommon byte len u32 {} should convert to usize", + VidSchemeType::get_payload_byte_len(common) + ); + return false; + } + }; - (self.0 == expected).then_some(()).ok_or(()) + self.0 == expected } pub(in crate::block::full_payload) fn as_usize(&self) -> usize { @@ -300,3 +310,10 @@ impl hotshot_types::traits::block_contents::TestableBlock for Payload self.len(&self.ns_table) as u64 } } + +#[cfg(any(test, feature = "testing"))] +impl Payload { + pub fn ns_table_mut(&mut self) -> &mut NsTable { + &mut self.ns_table + } +} diff --git a/sequencer/src/block/namespace_payload/tx_proof.rs b/sequencer/src/block/namespace_payload/tx_proof.rs index f3418eaec1..3cab3d0df3 100644 --- a/sequencer/src/block/namespace_payload/tx_proof.rs +++ b/sequencer/src/block/namespace_payload/tx_proof.rs @@ -54,7 +54,14 @@ impl TxProof { common: &VidCommon, ) -> Option<(Transaction, Self)> { let payload_byte_len = payload.byte_len(); - payload_byte_len.is_consistent(common).ok()?; + if !payload_byte_len.is_consistent(common) { + tracing::warn!( + "payload byte len {} inconsistent with common {}", + payload_byte_len, + VidSchemeType::get_payload_byte_len(common) + ); + return None; // error: payload byte len inconsistent with common + } if !payload.ns_table().in_bounds(index.ns()) { tracing::warn!("ns_index {:?} out of bounds", index.ns()); return None; // error: ns index out of bounds diff --git a/sequencer/src/block/test.rs b/sequencer/src/block/test.rs index 993d016847..233dec23d0 100644 --- a/sequencer/src/block/test.rs +++ b/sequencer/src/block/test.rs @@ -165,12 +165,12 @@ async fn enforce_max_block_size() { } // TODO lots of infra here that could be reused in other tests. -struct ValidTest { +pub struct ValidTest { nss: HashMap>, } impl ValidTest { - fn from_tx_lengths(tx_lengths: Vec>, rng: &mut R) -> Self + pub fn from_tx_lengths(tx_lengths: Vec>, rng: &mut R) -> Self where R: RngCore, { @@ -185,7 +185,7 @@ impl ValidTest { Self { nss } } - fn many_from_tx_lengths(test_cases: Vec>>, rng: &mut R) -> Vec + pub fn many_from_tx_lengths(test_cases: Vec>>, rng: &mut R) -> Vec where R: RngCore, { @@ -195,7 +195,7 @@ impl ValidTest { .collect() } - fn all_txs(&self) -> Vec { + pub fn all_txs(&self) -> Vec { self.nss.iter().flat_map(|(_, txs)| txs.clone()).collect() } } diff --git a/sequencer/src/state.rs b/sequencer/src/state.rs index 0c515817e2..d649269d9b 100644 --- a/sequencer/src/state.rs +++ b/sequencer/src/state.rs @@ -1,8 +1,13 @@ use crate::{ - api::data_source::CatchupDataSource, block::NsTableValidationError, catchup::SqlStateCatchup, - chain_config::BlockSize, chain_config::ResolvableChainConfig, eth_signature_key::EthKeyPair, - genesis::UpgradeType, persistence::ChainConfigPersistence, ChainConfig, Header, Leaf, - NodeState, SeqTypes, + api::data_source::CatchupDataSource, + block::{NsTableValidationError, PayloadByteLen}, + catchup::SqlStateCatchup, + chain_config::BlockSize, + chain_config::ResolvableChainConfig, + eth_signature_key::EthKeyPair, + genesis::UpgradeType, + persistence::ChainConfigPersistence, + ChainConfig, Header, Leaf, NodeState, SeqTypes, }; use anyhow::{bail, ensure, Context}; use ark_serialize::{ @@ -334,7 +339,9 @@ pub fn validate_proposal( }); } - proposal.ns_table.validate()?; + proposal + .ns_table + .validate(&PayloadByteLen::from_vid_common(vid_common))?; Ok(()) }