feat: enforce namespace table validity at consensus (#1607)

* new method NsTable::validate, use it in state::validate_proposal

* serde deserialize check NsTable::validate via ugly boilerplate serde-rs/serde#1220 (comment)

* add test for invalid byte lengths

* rename and tidy doc

* Payload::from_transactions use BTreeMap instead of HashMap

* NsIter do not skip duplicate entries

* NsTable::validate enforce increasing entries, with tests

* NsTable::validate enforce correct header, with tests

* NsTable::validate disallow 0 namespace id, offset

* tweak doc

* clippy pacification game with old rust version

* more clippy pacification (someone plz update nix)

* debug CI

* debug-ci: revert BTreeMap to HashMap

* debug ci: remove serde validation, restore BTreeMap

* restore NsTable serde validation

* NsTable::validate tests also check serde round trip

* NsTable do not derive Default, fix test_append_and_collect_garbage

* restore ns_table.validate()

* NamespaceId do not derive Default, Copy, do not impl Namespace, Namespaced, serde deserialize disallow zero

* tidy use statements

* allow zero NamespaceId (changed my mind)

* fix doc NsTable::validate

* nice job, idiot

* clarify doc on serde derivation for NsTable

* clarify doc for NsTable::len

* fix: byte length check in NsTable::validate

* address #1607 (comment)

* make NamespaceId Copy again

* fix again: byte length check in NsTable::validate

* address #1607 (comment)

builder/src/non_permissioned.rs

@@ -242,8 +242,7 @@ mod test {
             PersistenceOptions,
         },
         state::FeeAccount,
-        transaction::Transaction,
-        Payload,
+        NamespaceId, Payload, Transaction,
     };
     use std::time::Duration;
     use surf_disco::Client;
@@ -403,7 +402,7 @@ mod test {
             }
         }
 
-        let txn = Transaction::new(Default::default(), vec![1, 2, 3]);
+        let txn = Transaction::new(NamespaceId::from(1), vec![1, 2, 3]);
         match builder_client
             .post::<()>("txn_submit/submit")
             .body_json(&txn)

builder/src/permissioned.rs

@@ -65,20 +65,22 @@ use hotshot_builder_core::{
 use hotshot_state_prover;
 use jf_merkle_tree::{namespaced_merkle_tree::NamespacedMerkleTreeScheme, MerkleTreeScheme};
 use jf_signature::bls_over_bn254::VerKey;
-use sequencer::{catchup::mock::MockStateCatchup, eth_signature_key::EthKeyPair, ChainConfig};
 use sequencer::{
-    catchup::StatePeers,
+    catchup::{mock::MockStateCatchup, StatePeers},
     context::{Consensus, SequencerContext},
+    eth_signature_key::EthKeyPair,
     genesis::L1Finalized,
     l1_client::L1Client,
     network,
+    network::libp2p::split_off_peer_id,
     persistence::SequencerPersistence,
     state::FeeAccount,
     state::ValidatedState,
+    state_signature::StakeTableCommitmentType,
     state_signature::{static_stake_table_commitment, StateSigner},
-    Genesis, L1Params, NetworkParams, Node, NodeState, Payload, PrivKey, PubKey, SeqTypes,
+    ChainConfig, Genesis, L1Params, NetworkParams, Node, NodeState, Payload, PrivKey, PubKey,
+    SeqTypes,
 };
-use sequencer::{network::libp2p::split_off_peer_id, state_signature::StakeTableCommitmentType};
 use std::{alloc::System, any, fmt::Debug, mem};
 use std::{marker::PhantomData, net::IpAddr};
 use std::{net::Ipv4Addr, thread::Builder};
@@ -568,8 +570,7 @@ mod test {
     };
     use sequencer::{
         persistence::no_storage::{self, NoStorage},
-        transaction::Transaction,
-        Payload,
+        NamespaceId, Payload, Transaction,
     };
     use std::time::Duration;
     use surf_disco::Client;
@@ -715,7 +716,7 @@ mod test {
             }
         }
 
-        let txn = Transaction::new(Default::default(), vec![1, 2, 3]);
+        let txn = Transaction::new(NamespaceId::from(1), vec![1, 2, 3]);
         match builder_client
             .post::<()>("txn_submit/submit")
             .body_json(&txn)

sequencer/src/api.rs

@@ -4,7 +4,7 @@ use crate::{
     persistence::{ChainConfigPersistence, SequencerPersistence},
     state::{BlockMerkleTree, FeeAccountProof},
     state_signature::StateSigner,
-    ChainConfig, Node, NodeState, PubKey, SeqTypes, SequencerContext, Transaction,
+    ChainConfig, NamespaceId, Node, NodeState, PubKey, SeqTypes, SequencerContext, Transaction,
 };
 use anyhow::{bail, Context};
 use async_once_cell::Lazy;
@@ -583,7 +583,7 @@ pub mod test_helpers {
         setup_logging();
         setup_backtrace();
 
-        let txn = Transaction::new(Default::default(), vec![1, 2, 3, 4]);
+        let txn = Transaction::new(NamespaceId::from(1), vec![1, 2, 3, 4]);
 
         let port = pick_unused_port().expect("No ports free");
 

sequencer/src/block.rs

@@ -2,7 +2,7 @@ mod full_payload;
 mod namespace_payload;
 mod uint_bytes;
 
-pub use full_payload::{NsProof, NsTable, Payload};
+pub use full_payload::{NsProof, NsTable, NsTableValidationError, Payload};
 
 #[cfg(test)]
 mod test;

sequencer/src/block/full_payload.rs

@@ -3,7 +3,7 @@ mod ns_table;
 mod payload;
 
 pub use ns_proof::NsProof;
-pub use ns_table::{NsIndex, NsTable};
+pub use ns_table::{NsIndex, NsTable, NsTableValidationError};
 pub use payload::Payload;
 
 pub(in crate::block) use ns_table::NsIter;

sequencer/src/block/full_payload/ns_table.rs

@@ -16,9 +16,11 @@ use crate::{
     NamespaceId,
 };
 use committable::{Commitment, Committable, RawCommitmentBuilder};
+use derive_more::Display;
 use hotshot_types::traits::EncodeBytes;
-use serde::{Deserialize, Deserializer, Serialize, Serializer};
-use std::{collections::HashSet, sync::Arc};
+use serde::{de, Deserialize, Deserializer, Serialize, Serializer};
+use std::{ops::Range, sync::Arc};
+use thiserror::Error;
 
 /// Byte lengths for the different items that could appear in a namespace table.
 const NUM_NSS_BYTE_LEN: usize = 4;
@@ -116,28 +118,70 @@ const NS_ID_BYTE_LEN: usize = 4;
 /// TODO prefer [`NsTable`] to be a newtype like this
 /// ```ignore
 /// #[repr(transparent)]
-/// #[derive(Clone, Debug, Default, Deserialize, Eq, Hash, PartialEq, Serialize)]
+/// #[derive(Clone, Debug, Deserialize, Eq, Hash, PartialEq, Serialize)]
 /// #[serde(transparent)]
 /// pub struct NsTable(#[serde(with = "base64_bytes")] Vec<u8>);
 /// ```
 /// but we need to maintain serialization compatibility.
 /// <https://github.com/EspressoSystems/espresso-sequencer/issues/1575>
-#[derive(Clone, Debug, Default, Deserialize, Eq, Hash, PartialEq, Serialize)]
+#[derive(Clone, Debug, Deserialize, Eq, Hash, PartialEq, Serialize)]
+// Boilerplate: `#[serde(remote = "Self")]` needed to check invariants on
+// deserialization. See
+// https://github.com/serde-rs/serde/issues/1220#issuecomment-382589140
+#[serde(remote = "Self")]
 pub struct NsTable {
     #[serde(with = "base64_bytes")]
     bytes: Vec<u8>,
 }
 
+// Boilerplate: `#[serde(remote = "Self")]` allows invariant checking on
+// deserialization via re-implementation of `Deserialize` in terms of default
+// derivation. See
+// https://github.com/serde-rs/serde/issues/1220#issuecomment-382589140
+impl<'de> Deserialize<'de> for NsTable {
+    fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
+    where
+        D: Deserializer<'de>,
+    {
+        let unchecked = NsTable::deserialize(deserializer)?;
+        unchecked.validate().map_err(de::Error::custom)?;
+        Ok(unchecked)
+    }
+}
+
+// Boilerplate: use of `#[serde(remote = "Self")]` must include a trivial
+// `Serialize` impl. See
+// https://github.com/serde-rs/serde/issues/1220#issuecomment-382589140
+impl Serialize for NsTable {
+    fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error>
+    where
+        S: Serializer,
+    {
+        NsTable::serialize(self, serializer)
+    }
+}
+
 impl NsTable {
     /// Search the namespace table for the ns_index belonging to `ns_id`.
     pub fn find_ns_id(&self, ns_id: &NamespaceId) -> Option<NsIndex> {
         self.iter()
             .find(|index| self.read_ns_id_unchecked(index) == *ns_id)
     }
 
+    /// Number of entries in the namespace table.
+    ///
+    /// Defined as the maximum number of entries that could fit in the namespace
+    /// table, ignoring what's declared in the table header.
+    pub fn len(&self) -> NumNss {
+        NumNss(
+            self.bytes.len().saturating_sub(NUM_NSS_BYTE_LEN)
+                / NS_ID_BYTE_LEN.saturating_add(NS_OFFSET_BYTE_LEN),
+        )
+    }
+
     /// Iterator over all unique namespaces in the namespace table.
     pub fn iter(&self) -> impl Iterator<Item = NsIndex> + '_ {
-        NsIter::new(self)
+        NsIter::new(&self.len())
     }
 
     /// Read the namespace id from the `index`th entry from the namespace table.
@@ -166,17 +210,61 @@ impl NsTable {
 
     /// Does the `index`th entry exist in the namespace table?
     pub fn in_bounds(&self, index: &NsIndex) -> bool {
-        // The number of entries in the namespace table, including all duplicate
-        // namespace IDs.
-        let num_nss_with_duplicates = std::cmp::min(
-            // Number of namespaces declared in the ns table
-            self.read_num_nss(),
-            // Max number of entries that could fit in the namespace table
-            self.bytes.len().saturating_sub(NUM_NSS_BYTE_LEN)
-                / NS_ID_BYTE_LEN.saturating_add(NS_OFFSET_BYTE_LEN),
-        );
+        self.len().in_bounds(index)
+    }
+
+    /// Are the bytes of this [`NsTable`] uncorrupted?
+    ///
+    /// # Checks
+    /// 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
+    ///    <https://github.com/EspressoSystems/espresso-sequencer/issues/1604>)
+    pub fn validate(&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);
+        }
 
-        index.0 < num_nss_with_duplicates
+        // 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(())
     }
 
     // CRATE-VISIBLE HELPERS START HERE
@@ -188,31 +276,32 @@ impl NsTable {
         index: &NsIndex,
         payload_byte_len: &PayloadByteLen,
     ) -> NsPayloadRange {
-        let end = self.read_ns_offset(index).min(payload_byte_len.as_usize());
+        let end = self
+            .read_ns_offset_unchecked(index)
+            .min(payload_byte_len.as_usize());
         let start = if index.0 == 0 {
             0
         } else {
-            self.read_ns_offset(&NsIndex(index.0 - 1))
+            self.read_ns_offset_unchecked(&NsIndex(index.0 - 1))
         }
         .min(end);
         NsPayloadRange::new(start, end)
     }
 
     // PRIVATE HELPERS START HERE
 
-    /// Read the number of namespaces declared in the namespace table. This
-    /// quantity might exceed the number of entries that could fit in the
-    /// namespace table.
+    /// Read the number of namespaces declared in the namespace table. THIS
+    /// QUANTITY IS NEVER USED. Instead use [`NsTable::len`].
     ///
-    /// For a correct count of the number of unique namespaces in this
-    /// namespace table use `iter().count()`.
+    /// TODO Delete this method after
+    /// <https://github.com/EspressoSystems/espresso-sequencer/issues/1604>
     fn read_num_nss(&self) -> usize {
         let num_nss_byte_len = NUM_NSS_BYTE_LEN.min(self.bytes.len());
         usize_from_bytes::<NUM_NSS_BYTE_LEN>(&self.bytes[..num_nss_byte_len])
     }
 
     /// Read the namespace offset from the `index`th entry from the namespace table.
-    fn read_ns_offset(&self, index: &NsIndex) -> usize {
+    fn read_ns_offset_unchecked(&self, index: &NsIndex) -> usize {
         let start =
             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])
@@ -237,6 +326,14 @@ impl Committable for NsTable {
     }
 }
 
+/// Return type for [`NsTable::validate`].
+#[derive(Error, Debug, Display, Eq, PartialEq)]
+pub enum NsTableValidationError {
+    InvalidByteLen,
+    NonIncreasingEntries,
+    InvalidHeader, // TODO this variant obsolete after https://github.com/EspressoSystems/espresso-sequencer/issues/1604
+}
+
 pub struct NsTableBuilder {
     bytes: Vec<u8>,
     num_entries: usize,
@@ -270,18 +367,13 @@ impl NsTableBuilder {
         NsTable { bytes }
     }
 
-    /// Byte length of a namespace table with zero entries.
-    ///
-    /// Currently this quantity equals the byte length of the ns table header.
-    pub const fn fixed_overhead_byte_len() -> usize {
+    /// Byte length of a namespace table header.
+    pub const fn header_byte_len() -> usize {
         NUM_NSS_BYTE_LEN
     }
 
-    /// Byte length added to a namespace table by a new entry.
-    ///
-    /// Currently this quantity equals the byte length of a single ns table
-    /// entry.
-    pub const fn ns_overhead_byte_len() -> usize {
+    /// Byte length of a single namespace table entry.
+    pub const fn entry_byte_len() -> usize {
         NS_ID_BYTE_LEN + NS_OFFSET_BYTE_LEN
     }
 }
@@ -300,38 +392,32 @@ impl NsIndex {
     }
 }
 
-/// Return type for [`Payload::ns_iter`].
-pub(in crate::block) struct NsIter<'a> {
-    cur_index: usize,
-    repeat_nss: HashSet<NamespaceId>,
-    ns_table: &'a NsTable,
+/// Number of entries in a namespace table.
+pub struct NumNss(usize);
+
+impl NumNss {
+    pub fn in_bounds(&self, index: &NsIndex) -> bool {
+        index.0 < self.0
+    }
 }
 
-impl<'a> NsIter<'a> {
-    pub fn new(ns_table: &'a NsTable) -> Self {
-        Self {
-            cur_index: 0,
-            repeat_nss: HashSet::new(),
-            ns_table,
-        }
+/// Return type for [`Payload::ns_iter`].
+pub(in crate::block) struct NsIter(Range<usize>);
+
+impl NsIter {
+    pub fn new(num_nss: &NumNss) -> Self {
+        Self(0..num_nss.0)
     }
 }
 
-impl<'a> Iterator for NsIter<'a> {
+// Simple `impl Iterator` delegates to `Range`.
+impl Iterator for NsIter {
     type Item = NsIndex;
 
     fn next(&mut self) -> Option<Self::Item> {
-        loop {
-            let candidate_result = NsIndex(self.cur_index);
-            let ns_id = self.ns_table.read_ns_id(&candidate_result)?;
-            self.cur_index += 1;
-
-            // skip duplicate namespace IDs
-            if !self.repeat_nss.insert(ns_id) {
-                continue;
-            }
-
-            break Some(candidate_result);
-        }
+        self.0.next().map(NsIndex)
     }
 }
+
+#[cfg(test)]
+mod test;