Inline value should never be recorded. (#184)

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"] }

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`");

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<const C: u32>;
 
 impl TrieLayout for HashedValueNoExt {
 	const USE_EXTENSION: bool = false;
@@ -33,10 +33,10 @@ impl TrieLayout for HashedValueNoExt {
 	type Codec = ReferenceNodeCodecNoExtMeta<RefHasher>;
 }
 
-impl TrieLayout for HashedValueNoExtThreshold {
+impl<const C: u32> TrieLayout for HashedValueNoExtThreshold<C> {
 	const USE_EXTENSION: bool = false;
 	const ALLOW_EMPTY: bool = false;
-	const MAX_INLINE_VALUE: Option<u32> = Some(1);
+	const MAX_INLINE_VALUE: Option<u32> = Some(C);
 
 	type Hash = RefHasher;
 	type Codec = ReferenceNodeCodecNoExtMeta<RefHasher>;

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"

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)

trie-db/Cargo.toml

@@ -1,6 +1,6 @@
 [package]
 name = "trie-db"
-version = "0.25.0"
+version = "0.25.1"
 authors = ["Parity Technologies <admin@parity.io>"]
 description = "Merkle-Patricia Trie generic over key hasher and node encoding"
 repository = "https://github.com/paritytech/trie"

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,

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);

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"] }

trie-db/test/src/iter_build.rs

@@ -62,7 +62,7 @@ fn test_iter<T: TrieLayout>(data: Vec<(Vec<u8>, Vec<u8>)>) {
 }
 
 fn compare_implementations(data: Vec<(Vec<u8>, Vec<u8>)>) {
-	test_iter::<HashedValueNoExtThreshold>(data.clone());
+	test_iter::<HashedValueNoExtThreshold<1>>(data.clone());
 	test_iter::<HashedValueNoExt>(data.clone());
 	test_iter::<ExtensionLayout>(data.clone());
 	test_iter::<NoExtensionLayout>(data.clone());
@@ -71,7 +71,7 @@ fn compare_implementations(data: Vec<(Vec<u8>, Vec<u8>)>) {
 }
 
 fn compare_implementations_prefixed(data: Vec<(Vec<u8>, Vec<u8>)>) {
-	compare_implementations_prefixed_internal::<HashedValueNoExtThreshold>(data.clone());
+	compare_implementations_prefixed_internal::<HashedValueNoExtThreshold<1>>(data.clone());
 	compare_implementations_prefixed_internal::<HashedValueNoExt>(data.clone());
 	compare_implementations_prefixed_internal::<NoExtensionLayout>(data.clone());
 	compare_implementations_prefixed_internal::<ExtensionLayout>(data.clone());
@@ -82,7 +82,7 @@ fn compare_implementations_prefixed_internal<T: TrieLayout>(data: Vec<(Vec<u8>,
 	reference_trie::compare_implementations::<T, _>(data, memdb, hashdb);
 }
 fn compare_implementations_h(data: Vec<(Vec<u8>, Vec<u8>)>) {
-	compare_implementations_h_internal::<HashedValueNoExtThreshold>(data.clone());
+	compare_implementations_h_internal::<HashedValueNoExtThreshold<1>>(data.clone());
 	compare_implementations_h_internal::<HashedValueNoExt>(data.clone());
 	compare_implementations_h_internal::<NoExtensionLayout>(data.clone());
 	compare_implementations_h_internal::<ExtensionLayout>(data.clone());

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<T: TrieLayout>() {
 	let (root, proof, _) = test_generate_proof::<T>(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
 	}

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<T: TrieLayout>() {
 	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<T: TrieLayout>() {
 			);
 
 			// 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<T: TrieLayout>() {
 		}
 	}
 }
+
+#[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::<Layout>::default();
+	let mut root = Default::default();
+	{
+		let mut t = TrieDBMutBuilder::<Layout>::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::<Layout>::new();
+	let overlay = memdb.clone();
+	let new_root = root;
+	{
+		let trie = TrieDBBuilder::<Layout>::new(&overlay, &new_root)
+			.with_recorder(&mut recorder)
+			.build();
+
+		trie.get(key_value[0].0.as_slice()).unwrap();
+	}
+
+	let mut partial_db = MemoryDBProof::<Layout>::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::<Layout>::new();
+	let overlay = memdb.clone();
+	let new_root = root;
+	{
+		let trie = TrieDBBuilder::<Layout>::new(&overlay, &new_root)
+			.with_recorder(&mut recorder)
+			.build();
+
+		trie.get(key_value[1].0.as_slice()).unwrap();
+	}
+
+	let mut partial_db = MemoryDBProof::<Layout>::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::<Layout>::new();
+	let overlay = memdb.clone();
+	let new_root = root;
+	{
+		let trie = TrieDBBuilder::<Layout>::new(&overlay, &new_root)
+			.with_recorder(&mut recorder)
+			.build();
+
+		trie.get_hash(key_value[0].0.as_slice()).unwrap();
+	}
+
+	let mut partial_db = MemoryDBProof::<Layout>::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::<Layout>::new();
+	let overlay = memdb.clone();
+	let new_root = root;
+	{
+		let trie = TrieDBBuilder::<Layout>::new(&overlay, &new_root)
+			.with_recorder(&mut recorder)
+			.build();
+
+		trie.get_hash(key_value[1].0.as_slice()).unwrap();
+	}
+
+	let mut partial_db = MemoryDBProof::<Layout>::default();
+	let mut count = 0;
+	for record in recorder.drain() {
+		count += 1;
+		partial_db.insert(EMPTY_PREFIX, &record.data);
+	}
+
+	assert_eq!(count, 2);
+}

trie-db/test/src/triedbmut.rs

@@ -73,7 +73,7 @@ fn reference_hashed_null_node<T: TrieLayout>() -> <T::Hash as Hasher>::Out {
 #[test]
 fn playpen() {
 	env_logger::init();
-	playpen_internal::<HashedValueNoExtThreshold>();
+	playpen_internal::<HashedValueNoExtThreshold<1>>();
 	playpen_internal::<HashedValueNoExt>();
 	playpen_internal::<NoExtensionLayout>();
 	playpen_internal::<ExtensionLayout>();
@@ -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<RefHasher, PrefixedKey<RefHasher>, DBValue>;
 	let x = [
 		(b"test1".to_vec(), vec![1; 32]), // inline

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