Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 4 commits into from
Apr 8, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 6 additions & 6 deletions test-support/reference-trie/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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
Expand All @@ -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));
Expand Down
2 changes: 1 addition & 1 deletion test-support/trie-bench/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}
Expand Down
8 changes: 8 additions & 0 deletions trie-db/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
7 changes: 2 additions & 5 deletions trie-db/src/fatdb.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
4 changes: 2 additions & 2 deletions trie-db/src/fatdbmut.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
34 changes: 15 additions & 19 deletions trie-db/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TrieFactory is not used by substrate, but I feel like the change is good to have. Worth mentioning in CHANGELOG though.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here I just removed the warning of the unused layout

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean api did change (better new).

/// 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>,
Expand All @@ -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)),
}
}

Expand Down
10 changes: 3 additions & 7 deletions trie-db/src/sectriedb.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
6 changes: 2 additions & 4 deletions trie-db/src/sectriedbmut.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
23 changes: 6 additions & 17 deletions trie-db/src/triedb.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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());
/// ```
Expand All @@ -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 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh we actually have the unchecked version already. I never use it so new being unchecked by default makes sense.
Changelog could simply be new_unchecked replace new.

/// 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 }
}

Expand Down
17 changes: 8 additions & 9 deletions trie-db/src/triedbmut.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
4 changes: 2 additions & 2 deletions trie-db/test/benches/bench.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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()));
})
Expand All @@ -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()
Expand Down
2 changes: 1 addition & 1 deletion trie-db/test/src/fatdb.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<_>>(),
Expand Down
2 changes: 1 addition & 1 deletion trie-db/test/src/fatdbmut.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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])),);
}

Expand Down
2 changes: 1 addition & 1 deletion trie-db/test/src/iter_build.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
Loading