feat: Namespace table enforce final offset equals payload length

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;

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;

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<NsProof> {
         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);

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)
     }
 }
@@ -221,46 +223,24 @@ impl NsTable {
     ///    must be nonzero.
     /// 3. Header consistent with byte length (obsolete after
     ///    <https://github.com/EspressoSystems/espresso-sequencer/issues/1604>)
-    pub fn validate(&self) -> Result<(), NsTableValidationError> {
+    /// 4. Final offset must equal `payload_byte_len` (obsolete after
+    ///    <https://github.com/EspressoSystems/espresso-sequencer/issues/1604>)
+    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);
-        }
-
-        // 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);
-        }
+        // conditions 1-3
+        self.validate_deserialization_invariants()?;
 
-        // 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);
             }
         }
 
@@ -306,6 +286,57 @@ impl NsTable {
             index.0 * (NS_ID_BYTE_LEN + NS_OFFSET_BYTE_LEN) + NUM_NSS_BYTE_LEN + NS_ID_BYTE_LEN;
         usize_from_bytes::<NS_OFFSET_BYTE_LEN>(&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()`
+        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 +363,7 @@ 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
 }
 
 pub struct NsTableBuilder {
@@ -379,7 +411,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);
 

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,48 @@ 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::<NS_OFFSET_BYTE_LEN>(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);
+}
+
 #[test]
 fn monotonic_increase() {
     setup_logging();
@@ -149,7 +195,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 +207,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
     //

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<SeqTypes> 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<SeqTypes> 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
+    }
+}

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