Recover from our error handling Snafu ;)

Switched back to good old fashioned Strings.

src/lib.rs

@@ -254,7 +254,7 @@ impl<NR> Branch<NR> {
             .iter()
             .for_each(|word| hasher.update(word.to_le_bytes()));
 
-        hasher.finalize().into()
+        NodeHash::new(hasher.finalize().into())
     }
 }
 
@@ -403,12 +403,12 @@ pub struct Leaf<V> {
 }
 
 impl<V: AsRef<[u8]>> Leaf<V> {
+    #[inline]
     pub fn hash_leaf(&self) -> NodeHash {
         let mut hasher = Sha256::new();
         hasher.update(self.key_hash.to_bytes());
         hasher.update(self.value.as_ref());
-        let hash: NodeHash = hasher.finalize().into();
-        hash
+        NodeHash::new(hasher.finalize().into())
     }
 }
 
@@ -468,14 +468,14 @@ impl<'a, Db: DatabaseSet<V>, V: Clone + AsRef<[u8]>> Transaction<SnapshotBuilder
                 self.data_store
                     .db
                     .set(*hash, Node::Branch(branch))
-                    .map_err(|e| e.into())
+                    .map_err(|e| format!("Error writing branch {hash} to database: {e}"))
             };
 
         let store_modified_leaf = &mut |hash: &NodeHash, leaf: &Leaf<V>| {
             self.data_store
                 .db
                 .set(*hash, Node::Leaf(leaf.clone()))
-                .map_err(|e| e.into())
+                .map_err(|e| format!("Error writing leaf {hash} to database: {e}"))
         };
 
         let root_hash = self.calc_root_hash_inner(store_modified_branch, store_modified_leaf)?;
@@ -552,7 +552,7 @@ impl<S: Store<V>, V: AsRef<[u8]>> Transaction<S, V> {
             }
             NodeRef::Stored(stored_idx) => {
                 let hash = data_store
-                    .get_unvisted_hash(*stored_idx)
+                    .get_unvisited_hash(*stored_idx)
                     .copied()
                     .map_err(|e| e.into())?;
                 Ok(hash)

src/stored.rs

@@ -1,9 +1,8 @@
+pub mod memory_db;
 pub mod merkle;
 
-use std::cell::RefCell;
-use std::hash::Hash;
-
-use alloc::{collections::BTreeMap, fmt::Debug, string::String};
+use alloc::{fmt::Debug, string::String};
+use core::{fmt::Display, hash::Hash};
 
 use crate::{Branch, Leaf};
 
@@ -14,16 +13,16 @@ pub trait Store<V> {
 
     /// Must return a hash of a node that has not been visited.
     /// May return a hash of a node that has already been visited.
-    fn get_unvisted_hash(&self, hash_idx: Idx) -> Result<&NodeHash, Self::Error>;
+    fn get_unvisited_hash(&self, hash_idx: Idx) -> Result<&NodeHash, Self::Error>;
 
     fn get_node(&self, hash_idx: Idx) -> Result<Node<&Branch<Idx>, &Leaf<V>>, Self::Error>;
 }
 
 impl<V, S: Store<V>> Store<V> for &S {
     type Error = S::Error;
 
-    fn get_unvisted_hash(&self, hash_idx: Idx) -> Result<&NodeHash, Self::Error> {
-        (**self).get_unvisted_hash(hash_idx)
+    fn get_unvisited_hash(&self, hash_idx: Idx) -> Result<&NodeHash, Self::Error> {
+        (**self).get_unvisited_hash(hash_idx)
     }
 
     fn get_node(&self, hash_idx: Idx) -> Result<Node<&Branch<Idx>, &Leaf<V>>, Self::Error> {
@@ -32,7 +31,7 @@ impl<V, S: Store<V>> Store<V> for &S {
 }
 
 pub trait DatabaseGet<V> {
-    type GetError: Into<String> + Debug;
+    type GetError: Display;
 
     fn get(&self, hash: &NodeHash) -> Result<Node<Branch<NodeHash>, Leaf<V>>, Self::GetError>;
 }
@@ -46,7 +45,7 @@ impl<V, D: DatabaseGet<V>> DatabaseGet<V> for &D {
 }
 
 pub trait DatabaseSet<V>: DatabaseGet<V> {
-    type SetError: Into<String> + Debug;
+    type SetError: Display;
 
     fn set(
         &self,
@@ -73,12 +72,20 @@ pub enum Node<B, L> {
     Leaf(L),
 }
 
-#[derive(Clone, PartialEq, Eq, PartialOrd, Ord, Hash, Debug)]
+#[derive(Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash, Debug)]
 pub struct NodeHash {
-    bytes: [u8; 32],
+    pub bytes: [u8; 32],
+}
+
+impl NodeHash {
+    #[inline]
+    pub fn new(bytes: [u8; 32]) -> Self {
+        Self { bytes }
+    }
 }
 
 impl AsRef<[u8]> for NodeHash {
+    #[inline]
     fn as_ref(&self) -> &[u8] {
         &self.bytes
     }

src/stored/merkle.rs

@@ -6,7 +6,10 @@ use bumpalo::Bump;
 
 use crate::{Branch, Leaf, NodeRef, TrieRoot};
 
-use super::{DatabaseGet, Error, Idx, Node, NodeHash, Store};
+use super::{DatabaseGet, Idx, Node, NodeHash, Store};
+
+type Error = String;
+type Result<T, E = Error> = core::result::Result<T, E>;
 
 /// A snapshot of the merkle trie
 ///
@@ -23,7 +26,7 @@ pub struct Snapshot<V> {
 }
 
 impl<V: AsRef<[u8]>> Snapshot<V> {
-    pub fn root_node_idx(&self) -> Result<TrieRoot<Idx>, String> {
+    pub fn root_node_idx(&self) -> Result<TrieRoot<Idx>> {
         // Revist this once https://github.com/rust-lang/rust/issues/37854 is stable
         match (
             self.branches.deref(),
@@ -49,15 +52,15 @@ impl<V: AsRef<[u8]>> Snapshot<V> {
         }
     }
 
-    pub fn trie_root(&self) -> Result<TrieRoot<NodeRef<V>>, String> {
+    pub fn trie_root(&self) -> Result<TrieRoot<NodeRef<V>>> {
         match self.root_node_idx()? {
             TrieRoot::Node(idx) => Ok(TrieRoot::Node(NodeRef::Stored(idx))),
             TrieRoot::Empty => Ok(TrieRoot::Empty),
         }
     }
 
     /// Always check that the snapshot is of the merkle tree you expect.
-    pub fn calc_root_hash(&self) -> Result<TrieRoot<NodeHash>, String> {
+    pub fn calc_root_hash(&self) -> Result<TrieRoot<NodeHash>> {
         match self.root_node_idx()? {
             TrieRoot::Node(idx) => Ok(TrieRoot::Node(self.calc_root_hash_inner(idx)?)),
             TrieRoot::Empty => Ok(TrieRoot::Empty),
@@ -67,7 +70,7 @@ impl<V: AsRef<[u8]>> Snapshot<V> {
     // TODO fix possible stack overflow
     // I dislike using an explicit mutable stack.
     // I have an idea for abusing async for high performance segmented stacks
-    fn calc_root_hash_inner(&self, node: Idx) -> Result<NodeHash, String> {
+    fn calc_root_hash_inner(&self, node: Idx) -> Result<NodeHash> {
         match self.get_node(node) {
             Ok(Node::Branch(branch)) => {
                 let left = self.calc_root_hash_inner(branch.left)?;
@@ -77,7 +80,7 @@ impl<V: AsRef<[u8]>> Snapshot<V> {
             }
             Ok(Node::Leaf(leaf)) => Ok(leaf.hash_leaf()),
             Err(_) => self
-                .get_unvisted_hash(node)
+                .get_unvisited_hash(node)
                 .copied()
                 .map_err(|_| format!("Invalid snapshot: node {} not found", node)),
         }
@@ -87,13 +90,22 @@ impl<V: AsRef<[u8]>> Snapshot<V> {
 impl<V: AsRef<[u8]>> Store<V> for Snapshot<V> {
     type Error = Error;
 
-    fn get_unvisted_hash(&self, idx: Idx) -> Result<&NodeHash, Self::Error> {
+    fn get_unvisited_hash(&self, idx: Idx) -> Result<&NodeHash> {
         let idx = idx as usize - self.branches.len() - self.leaves.len();
 
-        self.unvisited_nodes.get(idx).ok_or(Error::NodeNotFound)
+        self.unvisited_nodes.get(idx).ok_or_else(|| {
+            format!(
+                "Invalid snapshot: no unvisited node at index {}\n\
+                Snapshot has {} branches, {} leaves, and {} unvisited nodes",
+                idx,
+                self.branches.len(),
+                self.leaves.len(),
+                self.unvisited_nodes.len(),
+            )
+        })
     }
 
-    fn get_node(&self, idx: Idx) -> Result<Node<&Branch<Idx>, &Leaf<V>>, Self::Error> {
+    fn get_node(&self, idx: Idx) -> Result<Node<&Branch<Idx>, &Leaf<V>>> {
         let idx = idx as usize;
         let leaf_offset = self.branches.len();
         let unvisited_offset = leaf_offset + self.leaves.len();
@@ -103,7 +115,14 @@ impl<V: AsRef<[u8]>> Store<V> for Snapshot<V> {
         } else if idx < unvisited_offset {
             Ok(Node::Leaf(&self.leaves[idx - leaf_offset]))
         } else {
-            Err(Error::NodeNotFound)
+            Err(format!(
+                "Invalid snapshot: no visited node at index {}\n\
+                Snapshot has {} branches, {} leaves, and {} unvisited nodes",
+                idx,
+                self.branches.len(),
+                self.leaves.len(),
+                self.unvisited_nodes.len(),
+            ))
         }
     }
 }
@@ -122,14 +141,21 @@ type NodeHashMaybeNode<'a, V> = (NodeHash, Option<Node<&'a Branch<Idx>, &'a Leaf
 impl<'a, Db: DatabaseGet<V>, V: Clone> Store<V> for SnapshotBuilder<'a, Db, V> {
     type Error = Error;
 
-    fn get_unvisted_hash(&self, hash_idx: Idx) -> Result<&NodeHash, Self::Error> {
+    fn get_unvisited_hash(&self, hash_idx: Idx) -> Result<&NodeHash, Self::Error> {
         let hash_idx = hash_idx as usize;
 
         self.nodes
             .borrow()
             .get(hash_idx)
             .map(|(hash, _)| hash)
-            .ok_or(Error::NodeNotFound)
+            .ok_or_else(|| {
+                format!(
+                    "Invalid snapshot: no unvisited node at index {}\n\
+                    SnapshotBuilder has {} nodes",
+                    hash_idx,
+                    self.nodes.borrow().len()
+                )
+            })
     }
 
     fn get_node(&self, hash_idx: Idx) -> Result<Node<&Branch<Idx>, &Leaf<V>>, Self::Error> {
@@ -141,7 +167,12 @@ impl<'a, Db: DatabaseGet<V>, V: Clone> Store<V> for SnapshotBuilder<'a, Db, V> {
             .get(hash_idx)
             .map(|(hash, o_node)| (hash, *o_node))
         else {
-            return Err(Error::NodeNotFound);
+            return Err(format!(
+                "Invalid snapshot: no node at index {}\n\
+                SnapshotBuilder has {} nodes",
+                hash_idx,
+                self.nodes.borrow().len()
+            ));
         };
 
         if let Some(node) = o_node {
@@ -231,14 +262,14 @@ impl<'a, Db, V> SnapshotBuilder<'a, Db, V> {
             Option<NodeHash>,
             Option<NodeHash>,
         ),
-        Error,
+        String,
     >
     where
         Db: DatabaseGet<V>,
     {
-        let Ok(node) = db.get(hash) else {
-            return Err(Error::NodeNotFound);
-        };
+        let node = db
+            .get(hash)
+            .map_err(|e| format!("Error getting {hash} from database: `{e}`"))?;
 
         Ok(match node {
             Node::Branch(Branch {

tests/build_store_modify.rs

@@ -3,10 +3,8 @@ mod utils;
 use proptest::prelude::*;
 use std::collections::HashMap;
 
-use sha2::{Digest, Sha256};
-
 use kairos_trie::{
-    stored::{merkle::SnapshotBuilder, MemoryDb, NodeHash},
+    stored::{memory_db::MemoryDb, merkle::SnapshotBuilder},
     KeyHash, Transaction, TrieRoot,
 };
 

tests/modified_only.rs

@@ -4,8 +4,8 @@ use std::collections::HashMap;
 use sha2::{Digest, Sha256};
 
 use kairos_trie::{
-    stored::{merkle::SnapshotBuilder, MemoryDb},
-    KeyHash, Transaction, TrieRoot,
+    stored::{memory_db::MemoryDb, merkle::SnapshotBuilder},
+    KeyHash, Transaction,
 };
 
 fn sha256_hash(data: &[u8]) -> [u8; 32] {

tests/utils/mod.rs

@@ -3,7 +3,7 @@ use std::collections::HashMap;
 use kairos_trie::{
     stored::{
         merkle::{Snapshot, SnapshotBuilder},
-        DatabaseSet, MemoryDb, NodeHash,
+        DatabaseSet, NodeHash,
     },
     KeyHash, Transaction, TrieRoot,
 };