Do not check for root in TrieDB and TrieDBMut constructors (#155)

* Do not check for root in `TrieDB` and `TrieDBMut` constructors This leads to one extra storage lookup before we are doing the actual key lookup. This leads to reading the `root` twice when just wanting to lookup one key in the trie. We also return an error on lookup anyway if `root` doesn't exists, so there is really no need to do this twice. * FMT * Update Changelog * Fix benchmarks
paritytech · Apr 8, 2022 · f64e1b0 · f64e1b0
1 parent 8d5b867
commit f64e1b0
Show file tree

Hide file tree

Showing 23 changed files with 88 additions and 107 deletions.
diff --git a/test-support/reference-trie/src/lib.rs b/test-support/reference-trie/src/lib.rs
@@ -928,15 +928,15 @@ where
 	if root_new != root {
 		{
 			let db: &dyn hash_db::HashDB<_, _> = &hashdb;
-			let t = TrieDB::<T>::new(&db, &root_new).unwrap();
+			let t = TrieDB::<T>::new(&db, &root_new);
 			println!("{:?}", t);
 			for a in t.iter().unwrap() {
 				println!("a:{:x?}", a);
 			}
 		}
 		{
 			let db: &dyn hash_db::HashDB<_, _> = &memdb;
-			let t = TrieDB::<T>::new(&db, &root).unwrap();
+			let t = TrieDB::<T>::new(&db, &root);
 			println!("{:?}", t);
 			for a in t.iter().unwrap() {
 				println!("a:{:x?}", a);
@@ -1048,15 +1048,15 @@ pub fn compare_implementations_unordered<T, DB>(
 	if root != root_new {
 		{
 			let db: &dyn hash_db::HashDB<_, _> = &memdb;
-			let t = TrieDB::<T>::new(&db, &root).unwrap();
+			let t = TrieDB::<T>::new(&db, &root);
 			println!("{:?}", t);
 			for a in t.iter().unwrap() {
 				println!("a:{:?}", a);
 			}
 		}
 		{
 			let db: &dyn hash_db::HashDB<_, _> = &hashdb;
-			let t = TrieDB::<T>::new(&db, &root_new).unwrap();
+			let t = TrieDB::<T>::new(&db, &root_new);
 			println!("{:?}", t);
 			for a in t.iter().unwrap() {
 				println!("a:{:?}", a);
@@ -1086,7 +1086,7 @@ pub fn compare_insert_remove<T, DB: hash_db::HashDB<T::Hash, DBValue>>(
 	while a < data.len() {
 		// new triemut every 3 element
 		root = {
-			let mut t = TrieDBMut::<T>::from_existing(&mut memdb, &mut root).unwrap();
+			let mut t = TrieDBMut::<T>::from_existing(&mut memdb, &mut root);
 			for _ in 0..3 {
 				if data[a].0 {
 					// remove
@@ -1107,7 +1107,7 @@ pub fn compare_insert_remove<T, DB: hash_db::HashDB<T::Hash, DBValue>>(
 			*t.root()
 		};
 	}
-	let mut t = TrieDBMut::<T>::from_existing(&mut memdb, &mut root).unwrap();
+	let mut t = TrieDBMut::<T>::from_existing(&mut memdb, &mut root);
 	// we are testing the RefTrie code here so we do not sort or check uniqueness
 	// before.
 	assert_eq!(*t.root(), calc_root::<T, _, _, _>(data2));

diff --git a/test-support/trie-bench/src/lib.rs b/test-support/trie-bench/src/lib.rs
@@ -81,7 +81,7 @@ fn benchmark<L: TrieLayout, S: TrieStream>(
 				}
 			}
 			b.iter(&mut || {
-				let t = TrieDB::<L>::new(&memdb, &root).unwrap();
+				let t = TrieDB::<L>::new(&memdb, &root);
 				for n in t.iter().unwrap() {
 					black_box(n).unwrap();
 				}

diff --git a/trie-db/CHANGELOG.md b/trie-db/CHANGELOG.md
@@ -5,6 +5,14 @@ The format is based on [Keep a Changelog].
 [Keep a Changelog]: http://keepachangelog.com/en/1.0.0/
 
 ## [Unreleased]
+- Do not check for root in `TrieDB` and `TrieDBMut` constructors: [#155](https://github.com/paritytech/trie/pull/155)
+
+  To get back the old behavior you have to add the following code:
+  ```
+  if !db.contains(root, EMPTY_PREFIX) {
+    return Err(InvalidStateRoot(root))
+  }
+  ```
 
 ## [0.23.1] - 2022-02-04
 - Updated `hashbrown` to 0.12. [#150](https://github.com/paritytech/trie/pull/150)

diff --git a/trie-db/src/fatdb.rs b/trie-db/src/fatdb.rs
@@ -38,11 +38,8 @@ where
 	/// Create a new trie with the backing database `db` and empty `root`
 	/// Initialise to the state entailed by the genesis block.
 	/// This guarantees the trie is built correctly.
-	pub fn new(
-		db: &'db dyn HashDBRef<L::Hash, DBValue>,
-		root: &'db TrieHash<L>,
-	) -> Result<Self, TrieHash<L>, CError<L>> {
-		Ok(FatDB { raw: TrieDB::new(db, root)? })
+	pub fn new(db: &'db dyn HashDBRef<L::Hash, DBValue>, root: &'db TrieHash<L>) -> Self {
+		FatDB { raw: TrieDB::new(db, root) }
 	}
 
 	/// Get the backing database.

diff --git a/trie-db/src/fatdbmut.rs b/trie-db/src/fatdbmut.rs
@@ -43,8 +43,8 @@ where
 	pub fn from_existing(
 		db: &'db mut dyn HashDB<L::Hash, DBValue>,
 		root: &'db mut TrieHash<L>,
-	) -> Result<Self, TrieHash<L>, CError<L>> {
-		Ok(FatDBMut { raw: TrieDBMut::from_existing(db, root)? })
+	) -> Self {
+		FatDBMut { raw: TrieDBMut::from_existing(db, root) }
 	}
 
 	/// Get the backing database.

diff --git a/trie-db/src/lib.rs b/trie-db/src/lib.rs
@@ -304,9 +304,8 @@ impl Default for TrieSpec {
 
 /// Trie factory.
 #[derive(Default, Clone)]
-pub struct TrieFactory<L: TrieLayout> {
+pub struct TrieFactory {
 	spec: TrieSpec,
-	layout: L,
 }
 
 /// All different kinds of tries.
@@ -376,30 +375,27 @@ impl<'db, L: TrieLayout> Trie<L> for TrieKinds<'db, L> {
 	}
 }
 
-impl<'db, L> TrieFactory<L>
-where
-	L: TrieLayout + 'db,
-{
+impl TrieFactory {
 	/// Creates new factory.
-	pub fn new(spec: TrieSpec, layout: L) -> Self {
-		TrieFactory { spec, layout }
+	pub fn new(spec: TrieSpec) -> Self {
+		TrieFactory { spec }
 	}
 
 	/// Create new immutable instance of Trie.
-	pub fn readonly(
+	pub fn readonly<'db, L: TrieLayout>(
 		&self,
 		db: &'db dyn HashDBRef<L::Hash, DBValue>,
 		root: &'db TrieHash<L>,
-	) -> Result<TrieKinds<'db, L>, TrieHash<L>, CError<L>> {
+	) -> TrieKinds<'db, L> {
 		match self.spec {
-			TrieSpec::Generic => Ok(TrieKinds::Generic(TrieDB::new(db, root)?)),
-			TrieSpec::Secure => Ok(TrieKinds::Secure(SecTrieDB::new(db, root)?)),
-			TrieSpec::Fat => Ok(TrieKinds::Fat(FatDB::new(db, root)?)),
+			TrieSpec::Generic => TrieKinds::Generic(TrieDB::new(db, root)),
+			TrieSpec::Secure => TrieKinds::Secure(SecTrieDB::new(db, root)),
+			TrieSpec::Fat => TrieKinds::Fat(FatDB::new(db, root)),
 		}
 	}
 
 	/// Create new mutable instance of Trie.
-	pub fn create(
+	pub fn create<'db, L: TrieLayout + 'db>(
 		&self,
 		db: &'db mut dyn HashDB<L::Hash, DBValue>,
 		root: &'db mut TrieHash<L>,
@@ -412,15 +408,15 @@ where
 	}
 
 	/// Create new mutable instance of trie and check for errors.
-	pub fn from_existing(
+	pub fn from_existing<'db, L: TrieLayout + 'db>(
 		&self,
 		db: &'db mut dyn HashDB<L::Hash, DBValue>,
 		root: &'db mut TrieHash<L>,
-	) -> Result<Box<dyn TrieMut<L> + 'db>, TrieHash<L>, CError<L>> {
+	) -> Box<dyn TrieMut<L> + 'db> {
 		match self.spec {
-			TrieSpec::Generic => Ok(Box::new(TrieDBMut::<L>::from_existing(db, root)?)),
-			TrieSpec::Secure => Ok(Box::new(SecTrieDBMut::<L>::from_existing(db, root)?)),
-			TrieSpec::Fat => Ok(Box::new(FatDBMut::<L>::from_existing(db, root)?)),
+			TrieSpec::Generic => Box::new(TrieDBMut::<L>::from_existing(db, root)),
+			TrieSpec::Secure => Box::new(SecTrieDBMut::<L>::from_existing(db, root)),
+			TrieSpec::Fat => Box::new(FatDBMut::<L>::from_existing(db, root)),
 		}
 	}
 

diff --git a/trie-db/src/sectriedb.rs b/trie-db/src/sectriedb.rs
@@ -33,16 +33,12 @@ impl<'db, L> SecTrieDB<'db, L>
 where
 	L: TrieLayout,
 {
-	/// Create a new trie with the backing database `db` and empty `root`
+	/// Create a new trie with the backing database `db` and `root`.
 	///
 	/// Initialise to the state entailed by the genesis block.
 	/// This guarantees the trie is built correctly.
-	/// Returns an error if root does not exist.
-	pub fn new(
-		db: &'db dyn HashDBRef<L::Hash, DBValue>,
-		root: &'db TrieHash<L>,
-	) -> Result<Self, TrieHash<L>, CError<L>> {
-		Ok(SecTrieDB { raw: TrieDB::new(db, root)? })
+	pub fn new(db: &'db dyn HashDBRef<L::Hash, DBValue>, root: &'db TrieHash<L>) -> Self {
+		SecTrieDB { raw: TrieDB::new(db, root) }
 	}
 
 	/// Get a reference to the underlying raw `TrieDB` struct.

diff --git a/trie-db/src/sectriedbmut.rs b/trie-db/src/sectriedbmut.rs
@@ -38,13 +38,11 @@ where
 	}
 
 	/// Create a new trie with the backing database `db` and `root`.
-	///
-	/// Returns an error if root does not exist.
 	pub fn from_existing(
 		db: &'db mut dyn HashDB<L::Hash, DBValue>,
 		root: &'db mut TrieHash<L>,
-	) -> Result<Self, TrieHash<L>, CError<L>> {
-		Ok(SecTrieDBMut { raw: TrieDBMut::from_existing(db, root)? })
+	) -> Self {
+		SecTrieDBMut { raw: TrieDBMut::from_existing(db, root) }
 	}
 
 	/// Get the backing database.

diff --git a/trie-db/src/triedb.rs b/trie-db/src/triedb.rs
@@ -44,7 +44,7 @@ use crate::rstd::{fmt, vec::Vec};
 /// let mut memdb = MemoryDB::<KeccakHasher, HashKey<_>, _>::default();
 /// let mut root = Default::default();
 /// RefTrieDBMut::new(&mut memdb, &mut root).insert(b"foo", b"bar").unwrap();
-/// let t = RefTrieDB::new(&memdb, &root).unwrap();
+/// let t = RefTrieDB::new(&memdb, &root);
 /// assert!(t.contains(b"foo").unwrap());
 /// assert_eq!(t.get(b"foo").unwrap().unwrap(), b"bar".to_vec());
 /// ```
@@ -62,22 +62,11 @@ impl<'db, L> TrieDB<'db, L>
 where
 	L: TrieLayout,
 {
-	/// Create a new trie with the backing database `db` and `root`
-	/// Returns an error if `root` does not exist
-	pub fn new(
-		db: &'db dyn HashDBRef<L::Hash, DBValue>,
-		root: &'db TrieHash<L>,
-	) -> Result<Self, TrieHash<L>, CError<L>> {
-		if !db.contains(root, EMPTY_PREFIX) {
-			Err(Box::new(TrieError::InvalidStateRoot(*root)))
-		} else {
-			Ok(TrieDB { db, root, hash_count: 0 })
-		}
-	}
-
-	/// `new_with_layout`, but do not check root presence, if missing
-	/// this will fail at first node access.
-	pub fn new_unchecked(db: &'db dyn HashDBRef<L::Hash, DBValue>, root: &'db TrieHash<L>) -> Self {
+	/// Create a new trie with the backing database `db` and `root`.
+	///
+	/// This doesn't check if `root` exists in the given `db`. If `root` doesn't exist it will fail
+	/// when trying to lookup any key.
+	pub fn new(db: &'db dyn HashDBRef<L::Hash, DBValue>, root: &'db TrieHash<L>) -> Self {
 		TrieDB { db, root, hash_count: 0 }
 	}
 

diff --git a/trie-db/src/triedbmut.rs b/trie-db/src/triedbmut.rs
@@ -588,26 +588,25 @@ where
 		}
 	}
 
-	/// Create a new trie with the backing database `db` and `root.
-	/// Returns an error if `root` does not exist.
+	/// Create a new trie with the backing database `db` and `root`.
+	///
+	/// This doesn't check if `root` exists in the given `db`. If `root` doesn't exist it will fail
+	/// when trying to lookup any key.
 	pub fn from_existing(
 		db: &'a mut dyn HashDB<L::Hash, DBValue>,
 		root: &'a mut TrieHash<L>,
-	) -> Result<Self, TrieHash<L>, CError<L>> {
-		if !db.contains(root, EMPTY_PREFIX) {
-			return Err(Box::new(TrieError::InvalidStateRoot(*root)))
-		}
-
+	) -> Self {
 		let root_handle = NodeHandle::Hash(*root);
-		Ok(TrieDBMut {
+		TrieDBMut {
 			storage: NodeStorage::empty(),
 			db,
 			root,
 			root_handle,
 			death_row: HashSet::new(),
 			hash_count: 0,
-		})
+		}
 	}
+
 	/// Get the backing database.
 	pub fn db(&self) -> &dyn HashDB<L::Hash, DBValue> {
 		self.db

diff --git a/trie-db/test/benches/bench.rs b/trie-db/test/benches/bench.rs
@@ -464,7 +464,7 @@ fn trie_iteration(c: &mut Criterion) {
 
 	c.bench_function("trie_iteration", move |b: &mut Bencher| {
 		b.iter(|| {
-			let trie = trie_db::TrieDB::<Layout>::new(&mdb, &root).unwrap();
+			let trie = trie_db::TrieDB::<Layout>::new(&mdb, &root);
 			let mut iter = trie_db::TrieDBNodeIterator::new(&trie).unwrap();
 			assert!(iter.all(|result| result.is_ok()));
 		})
@@ -485,7 +485,7 @@ fn trie_proof_verification(c: &mut Criterion) {
 	let mut mdb = memory_db::MemoryDB::<_, HashKey<_>, _>::default();
 	let root = reference_trie::calc_root_build::<Layout, _, _, _, _>(data, &mut mdb);
 
-	let trie = trie_db::TrieDB::<Layout>::new(&mdb, &root).unwrap();
+	let trie = trie_db::TrieDB::<Layout>::new(&mdb, &root);
 	let proof = generate_proof(&trie, keys.iter()).unwrap();
 	let items = keys
 		.into_iter()

diff --git a/trie-db/test/src/fatdb.rs b/trie-db/test/src/fatdb.rs
@@ -24,7 +24,7 @@ fn fatdb_to_trie() {
 		let mut t = RefFatDBMut::new(&mut memdb, &mut root);
 		t.insert(&[0x01u8, 0x23], &[0x01u8, 0x23]).unwrap();
 	}
-	let t = RefFatDB::new(&memdb, &root).unwrap();
+	let t = RefFatDB::new(&memdb, &root);
 	assert_eq!(t.get(&[0x01u8, 0x23]).unwrap().unwrap(), vec![0x01u8, 0x23]);
 	assert_eq!(
 		t.iter().unwrap().map(Result::unwrap).collect::<Vec<_>>(),

diff --git a/trie-db/test/src/fatdbmut.rs b/trie-db/test/src/fatdbmut.rs
@@ -25,7 +25,7 @@ fn fatdbmut_to_trie() {
 		let mut t = RefFatDBMut::new(&mut memdb, &mut root);
 		t.insert(&[0x01u8, 0x23], &[0x01u8, 0x23]).unwrap();
 	}
-	let t = RefTrieDB::new(&memdb, &root).unwrap();
+	let t = RefTrieDB::new(&memdb, &root);
 	assert_eq!(t.get(&RefHasher::hash(&[0x01u8, 0x23])), Ok(Some(vec![0x01u8, 0x23])),);
 }
 

diff --git a/trie-db/test/src/iter_build.rs b/trie-db/test/src/iter_build.rs
@@ -50,7 +50,7 @@ fn test_iter<T: TrieLayout>(data: Vec<(Vec<u8>, Vec<u8>)>) {
 			t.insert(key, value).unwrap();
 		}
 	}
-	let t = TrieDB::<T>::new(&db, &root).unwrap();
+	let t = TrieDB::<T>::new(&db, &root);
 	for (i, kv) in t.iter().unwrap().enumerate() {
 		let (k, v) = kv.unwrap();
 		let key: &[u8] = &data[i].0;