From 0b5b7a54bda23c815c130310a513bdb38251ed12 Mon Sep 17 00:00:00 2001 From: cheme Date: Tue, 14 Feb 2023 15:33:18 +0100 Subject: [PATCH] Inline value should never be recorded. (#184) --- test-support/reference-trie/Cargo.toml | 2 +- test-support/reference-trie/src/lib.rs | 2 +- .../reference-trie/src/substrate_like.rs | 6 +- test-support/trie-bench/Cargo.toml | 2 +- trie-db/CHANGELOG.md | 3 + trie-db/Cargo.toml | 2 +- trie-db/src/lookup.rs | 32 ++--- trie-db/src/proof/verify.rs | 2 +- trie-db/test/Cargo.toml | 2 +- trie-db/test/src/iter_build.rs | 6 +- trie-db/test/src/proof.rs | 2 +- trie-db/test/src/triedb.rs | 109 +++++++++++++++++- trie-db/test/src/triedbmut.rs | 4 +- trie-eip1186/test/Cargo.toml | 2 +- 14 files changed, 131 insertions(+), 45 deletions(-) diff --git a/test-support/reference-trie/Cargo.toml b/test-support/reference-trie/Cargo.toml index 0cd41d88..1042456d 100644 --- a/test-support/reference-trie/Cargo.toml +++ b/test-support/reference-trie/Cargo.toml @@ -10,7 +10,7 @@ edition = "2018" [dependencies] hash-db = { path = "../../hash-db" , version = "0.15.2"} keccak-hasher = { path = "../keccak-hasher", version = "0.15.3" } -trie-db = { path = "../../trie-db", default-features = false, version = "0.25.0" } +trie-db = { path = "../../trie-db", default-features = false, version = "0.25.1" } trie-root = { path = "../../trie-root", default-features = false, version = "0.17.0" } parity-scale-codec = { version = "3.0.0", features = ["derive"] } hashbrown = { version = "0.13.2", default-features = false, features = ["ahash"] } diff --git a/test-support/reference-trie/src/lib.rs b/test-support/reference-trie/src/lib.rs index 2356e104..41925fa2 100644 --- a/test-support/reference-trie/src/lib.rs +++ b/test-support/reference-trie/src/lib.rs @@ -48,7 +48,7 @@ macro_rules! test_layouts { #[test] fn $test() { eprintln!("Running with layout `HashedValueNoExtThreshold`"); - $test_internal::<$crate::HashedValueNoExtThreshold>(); + $test_internal::<$crate::HashedValueNoExtThreshold<1>>(); eprintln!("Running with layout `HashedValueNoExt`"); $test_internal::<$crate::HashedValueNoExt>(); eprintln!("Running with layout `NoExtensionLayout`"); diff --git a/test-support/reference-trie/src/substrate_like.rs b/test-support/reference-trie/src/substrate_like.rs index e9b67489..37cccefc 100644 --- a/test-support/reference-trie/src/substrate_like.rs +++ b/test-support/reference-trie/src/substrate_like.rs @@ -22,7 +22,7 @@ pub struct HashedValueNoExt; /// No extension trie which stores value above a static size /// as external node. -pub struct HashedValueNoExtThreshold; +pub struct HashedValueNoExtThreshold; impl TrieLayout for HashedValueNoExt { const USE_EXTENSION: bool = false; @@ -33,10 +33,10 @@ impl TrieLayout for HashedValueNoExt { type Codec = ReferenceNodeCodecNoExtMeta; } -impl TrieLayout for HashedValueNoExtThreshold { +impl TrieLayout for HashedValueNoExtThreshold { const USE_EXTENSION: bool = false; const ALLOW_EMPTY: bool = false; - const MAX_INLINE_VALUE: Option = Some(1); + const MAX_INLINE_VALUE: Option = Some(C); type Hash = RefHasher; type Codec = ReferenceNodeCodecNoExtMeta; diff --git a/test-support/trie-bench/Cargo.toml b/test-support/trie-bench/Cargo.toml index c934d78d..393f035b 100644 --- a/test-support/trie-bench/Cargo.toml +++ b/test-support/trie-bench/Cargo.toml @@ -13,6 +13,6 @@ trie-standardmap = { path = "../trie-standardmap", version = "0.15.2" } hash-db = { path = "../../hash-db" , version = "0.15.2"} memory-db = { path = "../../memory-db", version = "0.31.0" } trie-root = { path = "../../trie-root", version = "0.17.0" } -trie-db = { path = "../../trie-db", version = "0.25.0" } +trie-db = { path = "../../trie-db", version = "0.25.1" } criterion = "0.4.0" parity-scale-codec = "3.0.0" diff --git a/trie-db/CHANGELOG.md b/trie-db/CHANGELOG.md index 974f09f1..97134e5d 100644 --- a/trie-db/CHANGELOG.md +++ b/trie-db/CHANGELOG.md @@ -6,6 +6,9 @@ The format is based on [Keep a Changelog]. ## [Unreleased] +## [0.25.1] - 2023-02-14 +- Fix recorder behavior: [#184](https://github.com/paritytech/trie/pull/184) + ## [0.25.0] - 2023-02-03 - Updated `hashbrown` to 0.13.2: [#177](https://github.com/paritytech/trie/pull/177) - Iterator refactoring: [#181](https://github.com/paritytech/trie/pull/181) diff --git a/trie-db/Cargo.toml b/trie-db/Cargo.toml index 77b06009..748857d0 100644 --- a/trie-db/Cargo.toml +++ b/trie-db/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "trie-db" -version = "0.25.0" +version = "0.25.1" authors = ["Parity Technologies "] description = "Merkle-Patricia Trie generic over key hasher and node encoding" repository = "https://github.com/paritytech/trie" diff --git a/trie-db/src/lookup.rs b/trie-db/src/lookup.rs index ee321797..5e34055f 100644 --- a/trie-db/src/lookup.rs +++ b/trie-db/src/lookup.rs @@ -167,19 +167,7 @@ where full_key, |v, _, full_key, _, recorder, _| { Ok(match v { - Value::Inline(v) => { - let hash = L::Hash::hash(&v); - - if let Some(recoder) = recorder.as_mut() { - recoder.record(TrieAccess::Value { - hash, - value: v.into(), - full_key, - }); - } - - hash - }, + Value::Inline(v) => L::Hash::hash(&v), Value::Node(hash_bytes) => { if let Some(recoder) = recorder.as_mut() { recoder.record(TrieAccess::Hash { full_key }); @@ -223,17 +211,7 @@ where full_key, cache, |value, _, full_key, _, _, recorder| match value { - ValueOwned::Inline(value, hash) => { - if let Some(recorder) = recorder.as_mut() { - recorder.record(TrieAccess::Value { - hash, - value: value.as_ref().into(), - full_key, - }); - } - - Ok((hash, Some(value.clone()))) - }, + ValueOwned::Inline(value, hash) => Ok((hash, Some(value.clone()))), ValueOwned::Node(hash) => { if let Some(recoder) = recorder.as_mut() { recoder.record(TrieAccess::Hash { full_key }); @@ -317,7 +295,11 @@ where }, Some(CachedValue::Existing { data, hash, .. }) => if let Some(data) = data.upgrade() { - if value_recording_required { + // inline is either when no limit defined or when content + // is less than the limit. + let is_inline = + L::MAX_INLINE_VALUE.map_or(true, |max| max as usize > data.as_ref().len()); + if value_recording_required && !is_inline { // As a value is only raw data, we can directly record it. self.record(|| TrieAccess::Value { hash: *hash, diff --git a/trie-db/src/proof/verify.rs b/trie-db/src/proof/verify.rs index e4dcfe34..fedd0579 100644 --- a/trie-db/src/proof/verify.rs +++ b/trie-db/src/proof/verify.rs @@ -252,7 +252,7 @@ impl<'a, L: TrieLayout> StackEntry<'a, L> { } fn set_value(&mut self, value: &'a [u8]) { - self.value = if L::MAX_INLINE_VALUE.map(|max| value.len() < max as usize).unwrap_or(true) { + self.value = if L::MAX_INLINE_VALUE.map_or(true, |max| max as usize > value.len()) { Some(Value::Inline(value)) } else { let hash = L::Hash::hash(value); diff --git a/trie-db/test/Cargo.toml b/trie-db/test/Cargo.toml index 74a604fc..087b24c4 100644 --- a/trie-db/test/Cargo.toml +++ b/trie-db/test/Cargo.toml @@ -12,7 +12,7 @@ name = "bench" harness = false [dependencies] -trie-db = { path = "..", version = "0.25.0"} +trie-db = { path = "..", version = "0.25.1"} hash-db = { path = "../../hash-db", version = "0.15.2"} memory-db = { path = "../../memory-db", version = "0.31.0" } rand = { version = "0.8", default-features = false, features = ["small_rng"] } diff --git a/trie-db/test/src/iter_build.rs b/trie-db/test/src/iter_build.rs index 2535a2bd..bb4c0d85 100644 --- a/trie-db/test/src/iter_build.rs +++ b/trie-db/test/src/iter_build.rs @@ -62,7 +62,7 @@ fn test_iter(data: Vec<(Vec, Vec)>) { } fn compare_implementations(data: Vec<(Vec, Vec)>) { - test_iter::(data.clone()); + test_iter::>(data.clone()); test_iter::(data.clone()); test_iter::(data.clone()); test_iter::(data.clone()); @@ -71,7 +71,7 @@ fn compare_implementations(data: Vec<(Vec, Vec)>) { } fn compare_implementations_prefixed(data: Vec<(Vec, Vec)>) { - compare_implementations_prefixed_internal::(data.clone()); + compare_implementations_prefixed_internal::>(data.clone()); compare_implementations_prefixed_internal::(data.clone()); compare_implementations_prefixed_internal::(data.clone()); compare_implementations_prefixed_internal::(data.clone()); @@ -82,7 +82,7 @@ fn compare_implementations_prefixed_internal(data: Vec<(Vec, reference_trie::compare_implementations::(data, memdb, hashdb); } fn compare_implementations_h(data: Vec<(Vec, Vec)>) { - compare_implementations_h_internal::(data.clone()); + compare_implementations_h_internal::>(data.clone()); compare_implementations_h_internal::(data.clone()); compare_implementations_h_internal::(data.clone()); compare_implementations_h_internal::(data.clone()); diff --git a/trie-db/test/src/proof.rs b/trie-db/test/src/proof.rs index f84ed8cd..cca2c70e 100644 --- a/trie-db/test/src/proof.rs +++ b/trie-db/test/src/proof.rs @@ -161,7 +161,7 @@ test_layouts!(test_verify_invalid_child_reference, test_verify_invalid_child_ref fn test_verify_invalid_child_reference_internal() { let (root, proof, _) = test_generate_proof::(test_entries(), vec![b"bravo"]); - if T::MAX_INLINE_VALUE.map(|t| t as usize <= b"bravo".len()).unwrap_or(false) { + if T::MAX_INLINE_VALUE.map_or(false, |t| t as usize <= b"bravo".len()) { // node will not be inline: ignore test return } diff --git a/trie-db/test/src/triedb.rs b/trie-db/test/src/triedb.rs index 1b6ee4f3..520db1a5 100644 --- a/trie-db/test/src/triedb.rs +++ b/trie-db/test/src/triedb.rs @@ -17,7 +17,7 @@ use std::ops::Deref; use hash_db::{HashDB, Hasher, EMPTY_PREFIX}; use hex_literal::hex; use memory_db::{HashKey, MemoryDB, PrefixedKey}; -use reference_trie::{test_layouts, TestTrieCache}; +use reference_trie::{test_layouts, HashedValueNoExtThreshold, TestTrieCache}; use trie_db::{ CachedValue, DBValue, Lookup, NibbleSlice, Recorder, Trie, TrieCache, TrieDBBuilder, TrieDBMutBuilder, TrieLayout, TrieMut, @@ -452,7 +452,7 @@ fn test_recorder_with_cache_get_hash_internal() { assert!(cache.get_node(&root).is_some()); // Also the data should be cached. - if T::MAX_INLINE_VALUE.map_or(true, |l| l as usize >= key_value[1].1.len()) { + if T::MAX_INLINE_VALUE.map_or(true, |l| l as usize > key_value[1].1.len()) { assert!(matches!( cache.lookup_value_for_key(&key_value[1].0).unwrap(), CachedValue::Existing { hash, .. } if *hash == T::Hash::hash(&key_value[1].1) @@ -508,13 +508,13 @@ fn test_recorder_with_cache_get_hash_internal() { ); // Check if the values are part of the proof or not, based on the layout. - if T::MAX_INLINE_VALUE.map_or(true, |l| l as usize >= key_value[2].1.len()) { + if T::MAX_INLINE_VALUE.map_or(true, |l| l as usize > key_value[2].1.len()) { assert_eq!(key_value[2].1, trie.get(&key_value[2].0).unwrap().unwrap()); } else { assert!(trie.get(&key_value[2].0).is_err()); } - if T::MAX_INLINE_VALUE.map_or(true, |l| l as usize >= key_value[1].1.len()) { + if T::MAX_INLINE_VALUE.map_or(true, |l| l as usize > key_value[1].1.len()) { assert_eq!(key_value[1].1, trie.get(&key_value[1].0).unwrap().unwrap()); } else { assert!(trie.get(&key_value[1].0).is_err()); @@ -632,3 +632,104 @@ fn test_cache_internal() { } } } + +#[test] +fn test_record_value() { + type Layout = HashedValueNoExtThreshold<33>; + // one root branch and two leaf, one with inline value, the other with node value. + let key_value = vec![(b"A".to_vec(), vec![1; 32]), (b"B".to_vec(), vec![1; 33])]; + + // Add some initial data to the trie + let mut memdb = PrefixedMemoryDB::::default(); + let mut root = Default::default(); + { + let mut t = TrieDBMutBuilder::::new(&mut memdb, &mut root).build(); + for (key, value) in key_value.iter() { + t.insert(key, value).unwrap(); + } + } + + // Value access would record a tow node (branch and leaf with value 32 len inline). + let mut recorder = Recorder::::new(); + let overlay = memdb.clone(); + let new_root = root; + { + let trie = TrieDBBuilder::::new(&overlay, &new_root) + .with_recorder(&mut recorder) + .build(); + + trie.get(key_value[0].0.as_slice()).unwrap(); + } + + let mut partial_db = MemoryDBProof::::default(); + let mut count = 0; + for record in recorder.drain() { + count += 1; + partial_db.insert(EMPTY_PREFIX, &record.data); + } + + assert_eq!(count, 2); + + // Value access on node returns three items: a branch a leaf and a value node + let mut recorder = Recorder::::new(); + let overlay = memdb.clone(); + let new_root = root; + { + let trie = TrieDBBuilder::::new(&overlay, &new_root) + .with_recorder(&mut recorder) + .build(); + + trie.get(key_value[1].0.as_slice()).unwrap(); + } + + let mut partial_db = MemoryDBProof::::default(); + let mut count = 0; + for record in recorder.drain() { + count += 1; + partial_db.insert(EMPTY_PREFIX, &record.data); + } + + assert_eq!(count, 3); + + // Hash access would record two node (branch and leaf with value 32 len inline). + let mut recorder = Recorder::::new(); + let overlay = memdb.clone(); + let new_root = root; + { + let trie = TrieDBBuilder::::new(&overlay, &new_root) + .with_recorder(&mut recorder) + .build(); + + trie.get_hash(key_value[0].0.as_slice()).unwrap(); + } + + let mut partial_db = MemoryDBProof::::default(); + let mut count = 0; + for record in recorder.drain() { + count += 1; + partial_db.insert(EMPTY_PREFIX, &record.data); + } + + assert_eq!(count, 2); + + // Hash access would record two node (branch and leaf with value 32 len inline). + let mut recorder = Recorder::::new(); + let overlay = memdb.clone(); + let new_root = root; + { + let trie = TrieDBBuilder::::new(&overlay, &new_root) + .with_recorder(&mut recorder) + .build(); + + trie.get_hash(key_value[1].0.as_slice()).unwrap(); + } + + let mut partial_db = MemoryDBProof::::default(); + let mut count = 0; + for record in recorder.drain() { + count += 1; + partial_db.insert(EMPTY_PREFIX, &record.data); + } + + assert_eq!(count, 2); +} diff --git a/trie-db/test/src/triedbmut.rs b/trie-db/test/src/triedbmut.rs index cb7c0d28..02316892 100644 --- a/trie-db/test/src/triedbmut.rs +++ b/trie-db/test/src/triedbmut.rs @@ -73,7 +73,7 @@ fn reference_hashed_null_node() -> ::Out { #[test] fn playpen() { env_logger::init(); - playpen_internal::(); + playpen_internal::>(); playpen_internal::(); playpen_internal::(); playpen_internal::(); @@ -543,7 +543,7 @@ fn register_proof_without_value() { use reference_trie::HashedValueNoExtThreshold; use std::{cell::RefCell, collections::HashMap}; - type Layout = HashedValueNoExtThreshold; + type Layout = HashedValueNoExtThreshold<1>; type MemoryDB = memory_db::MemoryDB, DBValue>; let x = [ (b"test1".to_vec(), vec![1; 32]), // inline diff --git a/trie-eip1186/test/Cargo.toml b/trie-eip1186/test/Cargo.toml index 322df775..b252af16 100644 --- a/trie-eip1186/test/Cargo.toml +++ b/trie-eip1186/test/Cargo.toml @@ -9,7 +9,7 @@ edition = "2018" [dependencies] trie-eip1186 = { path = "..", version = "0.3.0"} -trie-db = { path = "../../trie-db", version = "0.25.0"} +trie-db = { path = "../../trie-db", version = "0.25.1"} hash-db = { path = "../../hash-db", version = "0.15.2"} reference-trie = { path = "../../test-support/reference-trie", version = "0.27.0" } memory-db = { path = "../../memory-db", version = "0.31.0" }