chore: improve comments and docs

Author: prestwich

crates/storage/src/cold/traits.rs

@@ -53,6 +53,30 @@ impl BlockData {
 /// The trait is agnostic to how the backend stores or indexes data.
 ///
 /// All methods are async and return futures that are `Send`.
+///
+/// # Implementation Guide
+///
+/// Implementers must ensure:
+///
+/// - **Append-only ordering**: `append_block` must enforce monotonically
+///   increasing block numbers. Attempting to append a block with a number <=
+///   the current latest should return an error.
+///
+/// - **Atomic truncation**: `truncate_above` must remove all data for blocks
+///   N+1 and higher atomically. Partial truncation is not acceptable.
+///
+/// - **Index maintenance**: Hash-based lookups (e.g., header by hash,
+///   transaction by hash) require the implementation to maintain appropriate
+///   indexes. These indexes must be updated during `append_block` and cleaned
+///   during `truncate_above`.
+///
+/// - **Consistent reads**: Read operations should return consistent snapshots.
+///   A read started before a write completes should not see partial data from
+///   that write.
+///
+/// - **Tag resolution**: `HeaderSpecifier::Tag` variants (Latest, Finalized,
+///   Safe, Earliest) must be resolved by the implementation. For simple
+///   backends, Latest/Finalized/Safe may all resolve to the same block.
 pub trait ColdStorage: Send + Sync + 'static {
     // --- Headers ---
 

crates/storage/src/hot/impls/mem.rs

@@ -490,20 +490,27 @@ impl<'a> MemKvCursorMut<'a> {
         }
     }
 
-    /// Get the first key-value pair >= key, returning owned data
+    /// Get the first key-value pair >= key, returning owned data.
+    ///
+    /// Merges queued operations with committed data, giving precedence to queued
+    /// ops for read-your-writes consistency.
     fn get_range_owned(&self, key: &MemStoreKey) -> Option<(MemStoreKey, Bytes)> {
+        // Find the first candidate from both queued ops and committed storage.
         let q = self.queued_ops.range(*key..).next();
         let c = if !self.is_cleared { self.table.range(*key..).next() } else { None };
 
         match (q, c) {
             (None, None) => None,
+
+            // Both sources have candidates - pick the smaller key, preferring
+            // queued ops on ties for read-your-writes consistency.
             (Some((qk, queued)), Some((ck, current))) => {
                 if qk <= ck {
-                    // Queued operation takes precedence
                     match queued {
                         QueuedKvOp::Put { value } => Some((*qk, value.clone())),
                         QueuedKvOp::Delete => {
-                            // Skip deleted entry and look for next
+                            // This key is marked deleted; increment to skip it
+                            // and recurse to find the next valid entry.
                             let mut next_key = *qk;
                             for i in (0..next_key.len()).rev() {
                                 if next_key[i] < u8::MAX {
@@ -519,9 +526,12 @@ impl<'a> MemKvCursorMut<'a> {
                     Some((*ck, current.clone()))
                 }
             }
+
+            // Only queued ops have a candidate.
             (Some((qk, queued)), None) => match queued {
                 QueuedKvOp::Put { value } => Some((*qk, value.clone())),
                 QueuedKvOp::Delete => {
+                    // Increment past the deleted key and recurse.
                     let mut next_key = *qk;
                     for i in (0..next_key.len()).rev() {
                         if next_key[i] < u8::MAX {
@@ -533,14 +543,20 @@ impl<'a> MemKvCursorMut<'a> {
                     self.get_range_owned(&next_key)
                 }
             },
+
+            // Only committed storage has a candidate.
             (None, Some((ck, current))) => Some((*ck, current.clone())),
         }
     }
 
-    /// Get the first key-value pair > key (strictly greater), returning owned data
+    /// Get the first key-value pair > key (strictly greater), returning owned data.
+    ///
+    /// Similar to `get_range_owned` but uses exclusive bounds for cursor
+    /// navigation (read_next).
     fn get_range_exclusive_owned(&self, key: &MemStoreKey) -> Option<(MemStoreKey, Bytes)> {
         use core::ops::Bound;
 
+        // Find candidates strictly greater than the given key.
         let q = self.queued_ops.range((Bound::Excluded(*key), Bound::Unbounded)).next();
         let c = if !self.is_cleared {
             self.table.range((Bound::Excluded(*key), Bound::Unbounded)).next()
@@ -550,31 +566,33 @@ impl<'a> MemKvCursorMut<'a> {
 
         match (q, c) {
             (None, None) => None,
+
+            // Both sources have candidates.
             (Some((qk, queued)), Some((ck, current))) => {
                 if qk <= ck {
-                    // Queued operation takes precedence
                     match queued {
                         QueuedKvOp::Put { value } => Some((*qk, value.clone())),
-                        QueuedKvOp::Delete => {
-                            // This key is deleted, recurse to find the next one
-                            self.get_range_exclusive_owned(qk)
-                        }
+                        // Deleted in queue; skip and recurse.
+                        QueuedKvOp::Delete => self.get_range_exclusive_owned(qk),
                     }
                 } else {
-                    // Check if the current key has a delete queued
+                    // Committed key is smaller, but check if it's been deleted.
                     if let Some(QueuedKvOp::Delete) = self.queued_ops.get(ck) {
                         self.get_range_exclusive_owned(ck)
                     } else {
                         Some((*ck, current.clone()))
                     }
                 }
             }
+
+            // Only queued ops have a candidate.
             (Some((qk, queued)), None) => match queued {
                 QueuedKvOp::Put { value } => Some((*qk, value.clone())),
                 QueuedKvOp::Delete => self.get_range_exclusive_owned(qk),
             },
+
+            // Only committed storage has a candidate; verify not deleted.
             (None, Some((ck, current))) => {
-                // Check if the current key has a delete queued
                 if let Some(QueuedKvOp::Delete) = self.queued_ops.get(ck) {
                     self.get_range_exclusive_owned(ck)
                 } else {
@@ -584,28 +602,39 @@ impl<'a> MemKvCursorMut<'a> {
         }
     }
 
-    /// Get the last key-value pair < key, returning owned data
+    /// Get the last key-value pair < key, returning owned data.
+    ///
+    /// Reverse iteration for cursor navigation (read_prev). Merges queued ops
+    /// with committed data, preferring the larger key (closest to search key).
     fn get_range_reverse_owned(&self, key: &MemStoreKey) -> Option<(MemStoreKey, Bytes)> {
+        // Find candidates strictly less than the given key, scanning backwards.
         let q = self.queued_ops.range(..*key).next_back();
         let c = if !self.is_cleared { self.table.range(..*key).next_back() } else { None };
 
         match (q, c) {
             (None, None) => None,
+
+            // Both sources have candidates - pick the larger key (closest to
+            // search position), preferring queued ops on ties.
             (Some((qk, queued)), Some((ck, current))) => {
                 if qk >= ck {
-                    // Queued operation takes precedence
                     match queued {
                         QueuedKvOp::Put { value } => Some((*qk, value.clone())),
+                        // Deleted; recurse to find the previous valid entry.
                         QueuedKvOp::Delete => self.get_range_reverse_owned(qk),
                     }
                 } else {
                     Some((*ck, current.clone()))
                 }
             }
+
+            // Only queued ops have a candidate.
             (Some((qk, queued)), None) => match queued {
                 QueuedKvOp::Put { value } => Some((*qk, value.clone())),
                 QueuedKvOp::Delete => self.get_range_reverse_owned(qk),
             },
+
+            // Only committed storage has a candidate.
             (None, Some((ck, current))) => Some((*ck, current.clone())),
         }
     }

crates/storage/src/hot/mod.rs

@@ -4,7 +4,7 @@
 //! data. It provides abstractions and implementations for key-value storage
 //! backends.
 //!
-//! ## Serialiazation
+//! ## Serialization
 //!
 //! Hot storage is opinionated with respect to serialization. Each table defines
 //! the key and value types it uses, and these types must implement the

crates/storage/src/hot/model/db_traits.rs

@@ -8,6 +8,13 @@ use reth_db::{BlockNumberList, models::BlockNumberAddress};
 use reth_db_api::models::ShardedKey;
 
 /// Trait for database read operations on standard hot tables.
+///
+/// This is a high-level trait that provides convenient methods for reading
+/// common data types from predefined hot storage tables. It builds upon the
+/// lower-level [`HotKvRead`] trait, which provides raw key-value access.
+///
+/// Users should prefer this trait unless customizations are needed to the
+/// table set.
 pub trait HotDbRead: HotKvRead + sealed::Sealed {
     /// Read a block header by its number.
     fn get_header(&self, number: u64) -> Result<Option<Header>, Self::Error> {
@@ -121,6 +128,14 @@ impl<T> HotDbWrite for T where T: HotKvWrite {}
 /// These tables maintain historical information about accounts and storage
 /// changes, and their contents can be used to reconstruct past states or
 /// roll back changes.
+///
+/// This is a high-level trait that provides convenient methods for reading
+/// common data types from predefined hot storage history tables. It builds
+/// upon the lower-level [`HotDbRead`] trait, which provides raw key-value
+/// access.
+///
+/// Users should prefer this trait unless customizations are needed to the
+/// table set.
 pub trait HotHistoryRead: HotDbRead {
     /// Get the list of block numbers where an account was touched.
     /// Get the list of block numbers where an account was touched.

crates/storage/src/hot/model/revm.rs

@@ -435,7 +435,7 @@ where
     }
 }
 
-#[cfg(test)]
+#[cfg(all(test, feature = "in-mem"))]
 mod tests {
     use super::*;
     use crate::hot::{

crates/storage/src/hot/model/traits.rs

@@ -62,6 +62,10 @@ pub trait HotKv {
 }
 
 /// Trait for hot storage read transactions.
+///
+/// This trait provides read-only access to hot storage tables. It should only
+/// be imported if accessing custom tables, or when implementing new hot storage
+/// backends.
 #[auto_impl::auto_impl(&, Arc, Box)]
 pub trait HotKvRead {
     /// Error type for read operations.

crates/storage/src/hot/ser/traits.rs

@@ -55,10 +55,7 @@ pub trait KeySer: PartialOrd + Ord + Sized + Clone + core::fmt::Debug {
     /// Useful in DB decoding, where the absence of a key is represented by
     /// `None`.
     fn maybe_decode_key(data: Option<&[u8]>) -> Result<Option<Self>, DeserError> {
-        match data {
-            Some(d) => Ok(Some(Self::decode_key(d)?)),
-            None => Ok(None),
-        }
+        data.map(Self::decode_key).transpose()
     }
 }
 
@@ -105,10 +102,7 @@ pub trait ValSer {
     where
         Self: Sized,
     {
-        match data {
-            Some(d) => Ok(Some(Self::decode_value(d)?)),
-            None => Ok(None),
-        }
+        data.map(Self::decode_value).transpose()
     }
 
     /// Deserialize the value from bytes, ensuring all bytes are consumed.