trie-db: Fetch the closest merkle value #199

lexnv · 2023-08-21T13:35:04Z

This PR adds support for fetching the merkle value directly from the trie-db.

Adds get_closest_merkle_value on the Trie trait
TrieDb iterates over the nibbles of the provided key
- At each iteration DB is queried for the containing node
- last value encountered on the path from the root to the key is returned
During trie traversal, the internal recorder is updated with trie actions
- This should make other trie queries to the key more efficient
Test with HashedValueNoExtThreshold<1> as trie layout and value update at given key

We could build on this PR to have support for recorded Merkel Value in the cache.

Would love to get your thoughts on this @cheme @arkpar 🙏
(For reference @tomaka to ensure we don't break anything wrt to compatibilities between smoldot and substrate)

Part of paritytech/substrate#14550.

// @paritytech/subxt-team

Signed-off-by: Alexandru Vasile <[email protected]>

trie-db/src/lookup.rs

arkpar · 2023-08-22T15:50:17Z

trie-db/test/src/triedb.rs

@@ -644,6 +644,80 @@ fn test_recorder_with_cache_get_hash_internal<T: TrieLayout>() {
 	}
 }

+#[test]
+fn test_merkle_value() {


The test should also check that the functions works correctly for keys that aren't acuatlly in the tree.
E.g. for a tree with nodes ["AAA"] it should return a closest descendant for the key "AA" but not for "AB". For the tree with nodes ["AAAA", "AABA] closes descendant for the key "A" should be the branch node, etc.

Also there's no need to make changes to the test trie after it is created really.

Also, you don't need to

Argh, the suspense :D

arkpar · 2023-08-22T15:59:03Z

After reading the spec more carefully, I think the logic here requires some changes. We need to return the descendant of the provided key, not just the lower bound. Basically this means traversing the tree as usual, but for Leaf, Extension and NibbledBranch nodes if you run out of key, just check if the current node key slice extends the remainder.

if slice.starts_with(&partial) {
  return Ok(Some(hash)))
else
  return Ok(None)
}

Co-authored-by: Arkadiy Paronyan <[email protected]>

Signed-off-by: Alexandru Vasile <[email protected]>

cheme

@lexnv , thanks for the work, I got a few question on corner cases, notably inline node (I am not 100% sure, depends of how the spec is understood).

trie-db/src/lookup.rs

trie-db/test/src/triedb.rs

trie-db/src/lookup.rs

Signed-off-by: Alexandru Vasile <[email protected]>

trie-db/src/lookup.rs

Signed-off-by: Alexandru Vasile <[email protected]>

cheme

Looks good.
Worth remembering that if at some point this get used in runtime (especially by a host function), not simply rpc, it would be worth fuzzing a bit.

bkchr

Left mainly some nitpicks. However, the return value is wrong. The merkle value is either the node data or the hash, depending on if this is an inline node or not.

trie-db/src/fatdb.rs

trie-db/src/lib.rs

trie-db/src/sectriedb.rs

trie-db/src/lookup.rs

bkchr · 2023-09-05T23:06:26Z

trie-db/src/lookup.rs

+			let mut node = if let Some(cache) = &mut cache {
+				let node = cache.get_or_insert_node(hash, &mut || get_owned_node(depth))?;
+
+				self.record(|| TrieAccess::NodeOwned { hash, node_owned: node });
+
+				node
+			} else {
+				_owned_node = get_owned_node(depth)?;
+
+				self.record(|| TrieAccess::EncodedNode {
+					hash,
+					encoded_node: node_data.as_slice().into(),
+				});
+
+				&_owned_node
+			};


Now we always need to create an owned node, which is a little bit shitty. But fine for now I think.

bkchr · 2023-09-05T23:07:49Z

trie-db/src/lookup.rs

+		mut self,
+		nibble_key: NibbleSlice,
+		full_key: &[u8],
+	) -> Result<Option<TrieHash<L>>, TrieHash<L>, CError<L>> {


The return type is wrong.

enum MerkleValue { /// Is returned if `v.len() < MAX_INLINE_VALUE`. Node(Vec<u8>), Hash(H), }

I did propose to use hash for inline node (generally I would prefer if inline node is something users don't need to be aware off). But I did not double check the rpc proposal.

Maybe trie crate api could return this enum, but the rpc return hash of the data value for inline node (this way we can avoid some hashing if this api get use by runtime in the future).

But the spec is speaking about the merkle value and not the hash. The point being that if the data is smaller, we save bandwidth etc.

(this way we can avoid some hashing if this api get use by runtime in the future)

This doesn't add any extra hashing. If you need hashing for the runtime, we can just do it where we use it. There would be no extra hashing involved as we currently also hash the inline node manually.

That makes sense! I've added the MerkleValue enum to be explicit about what the trie-db returns.

Then in substrate's RPC layer we could further hash(MerkleValue::Node(..)) such that we return a consistent view of nodes and not care about inline ones. And if this function gets ever called from the runtime we save a Layout:hash() call 🙏

Then in substrate's RPC layer we could further hash(MerkleValue::Node(..)) such that we return a consistent view of nodes and not care about inline ones.

This still goes against the size argument I laid out above. I opened paritytech/json-rpc-interface-spec#88 to be sure that we use the correct value in the RPC.

Signed-off-by: Alexandru Vasile <[email protected]>

trie-db/src/lib.rs

Co-authored-by: Bastian Köcher <[email protected]>

Signed-off-by: Alexandru Vasile <[email protected]>

…1153) This PR adds support for fetching the closest merkle value of some key. Builds on top of - paritytech/trie#199 Migrates paritytech/substrate#14818 to the monorepo. Closes: paritytech/substrate#14550 Closes: #1506 // @paritytech/subxt-team --------- Signed-off-by: Alexandru Vasile <[email protected]> Co-authored-by: Sebastian Kunert <[email protected]>

…aritytech#1153) This PR adds support for fetching the closest merkle value of some key. Builds on top of - paritytech/trie#199 Migrates paritytech/substrate#14818 to the monorepo. Closes: paritytech/substrate#14550 Closes: paritytech#1506 // @paritytech/subxt-team --------- Signed-off-by: Alexandru Vasile <[email protected]> Co-authored-by: Sebastian Kunert <[email protected]>

lexnv added 3 commits August 18, 2023 17:33

trie-db: Add get_closest_merkle_value to Trie trait

bf44483

Signed-off-by: Alexandru Vasile <[email protected]>

trie-db: Extract the merkle value

c2e2ecd

Signed-off-by: Alexandru Vasile <[email protected]>

trie-db/test: Check merkle value on update key

a93733f

Signed-off-by: Alexandru Vasile <[email protected]>

lexnv self-assigned this Aug 21, 2023

lexnv requested review from cheme and arkpar August 21, 2023 13:37

arkpar reviewed Aug 22, 2023

View reviewed changes

trie-db/src/lookup.rs Outdated Show resolved Hide resolved

arkpar reviewed Aug 22, 2023

View reviewed changes

trie-db/src/lookup.rs Outdated Show resolved Hide resolved

arkpar reviewed Aug 22, 2023

View reviewed changes

trie-db/src/lookup.rs Outdated Show resolved Hide resolved

arkpar reviewed Aug 22, 2023

View reviewed changes

arkpar requested a review from bkchr August 22, 2023 15:59

lexnv and others added 7 commits August 23, 2023 12:10

Update trie-db/src/lookup.rs

546fdb5

Co-authored-by: Arkadiy Paronyan <[email protected]>

Update trie-db/src/lookup.rs

adfd6f1

Co-authored-by: Arkadiy Paronyan <[email protected]>

trie-db: Rename look_up_merkle_without_cache function

b7db016

Signed-off-by: Alexandru Vasile <[email protected]>

trie-db/tests: Check closest descendant of partial keys

fa10f70

Signed-off-by: Alexandru Vasile <[email protected]>

trie-db: Adjust lookups for partial keys

5998874

Signed-off-by: Alexandru Vasile <[email protected]>

trie-db/tests: Check non-existent key and branch nodes

fb365bb

Signed-off-by: Alexandru Vasile <[email protected]>

trie-db: Ensure recording of NonExisting for leaves

77f19e2

Signed-off-by: Alexandru Vasile <[email protected]>

lexnv mentioned this pull request Aug 23, 2023

chainHead: Add support for storage closest merkle descendant paritytech/substrate#14818

Open

lexnv added 3 commits August 23, 2023 20:48

trie-db: Ensure the merkle descedent hash is returned

7e2ae36

Signed-off-by: Alexandru Vasile <[email protected]>

trie-db/tests: Extend tests with branch nodes and single key db

40d700c

Signed-off-by: Alexandru Vasile <[email protected]>

trie-db/tests: Check trie modification and merkle propagation

1124af8

Signed-off-by: Alexandru Vasile <[email protected]>

lexnv mentioned this pull request Aug 25, 2023

chainHead: Add support for storage closest merkle descendant #14818 paritytech/polkadot-sdk#1153

Merged

cheme reviewed Aug 28, 2023

View reviewed changes

lexnv added 5 commits August 28, 2023 19:30

trie-db/tests: Use PrefixedKey instead of HashKey

bc2d977

Signed-off-by: Alexandru Vasile <[email protected]>

trie-db/tests: Test extra keys for test_merkle_value

b46eecd

Signed-off-by: Alexandru Vasile <[email protected]>

trie-db: Return the extension node hash

f714033

Signed-off-by: Alexandru Vasile <[email protected]>

trie-db: Use starts_with method instead of common prefix

82127bb

Signed-off-by: Alexandru Vasile <[email protected]>

trie-db: Return no merkle value on empty node

1b5fcae

Signed-off-by: Alexandru Vasile <[email protected]>

lexnv added 4 commits August 28, 2023 20:48

trie-db/tests: Ensure inline nodes

28bb9fa

Signed-off-by: Alexandru Vasile <[email protected]>

trie-db/tests: Check empty trie with empty keys

5de55d0

Signed-off-by: Alexandru Vasile <[email protected]>

trie-db/tests: Add extra keys to check

d6c8a55

Signed-off-by: Alexandru Vasile <[email protected]>

trie-db: Use starts_with for extension nodes

e2ffe4f

Signed-off-by: Alexandru Vasile <[email protected]>

arkpar reviewed Aug 28, 2023

View reviewed changes

trie-db/src/lookup.rs Outdated Show resolved Hide resolved

bkchr reviewed Aug 29, 2023

View reviewed changes

trie-db/src/lookup.rs Outdated Show resolved Hide resolved

trie-db/src/lookup.rs Outdated Show resolved Hide resolved

lexnv added 6 commits August 29, 2023 13:41

trie-db: Rename merkle lookups to lookup_first_descendant

21b7307

Signed-off-by: Alexandru Vasile <[email protected]>

trie-db: Return inline hashes properly

5c6ea20

Signed-off-by: Alexandru Vasile <[email protected]>

trie-db: Implement starts_with_slice for NibbleVec

1a6fa44

Signed-off-by: Alexandru Vasile <[email protected]>

trie-db: Use cache for finding first descendent hash

db496a7

Signed-off-by: Alexandru Vasile <[email protected]>

trie-db: Introduce caching for descedent node access

770a5f5

Signed-off-by: Alexandru Vasile <[email protected]>

trie-db: Use rstd::vec::Vec

9fb3bb3

Signed-off-by: Alexandru Vasile <[email protected]>

cheme approved these changes Aug 29, 2023

View reviewed changes

arkpar approved these changes Aug 30, 2023

View reviewed changes

bkchr reviewed Sep 5, 2023

View reviewed changes

lexnv added 4 commits September 6, 2023 10:05

trie-db: Forward merkle value for fatdb and sectriedb

5d29a75

Signed-off-by: Alexandru Vasile <[email protected]>

trie-db: Rename get_closest_merkle_value to lookup_first_descendant

bc32ac8

Signed-off-by: Alexandru Vasile <[email protected]>

trie-db: Introduce MerkleValue to return inline nodes and hashes

d538fe3

Signed-off-by: Alexandru Vasile <[email protected]>

trie-db: Remove inner function for merkle value lookups

5c05937

Signed-off-by: Alexandru Vasile <[email protected]>

bkchr approved these changes Sep 6, 2023

View reviewed changes

trie-db/src/lib.rs Outdated Show resolved Hide resolved

trie-db/src/lib.rs Outdated Show resolved Hide resolved

lexnv and others added 3 commits September 11, 2023 12:54

Update trie-db/src/lib.rs

b8a589f

Co-authored-by: Bastian Köcher <[email protected]>

Update trie-db/src/lib.rs

d9af8ce

Co-authored-by: Bastian Köcher <[email protected]>

Apply fmt

1fa98e5

Signed-off-by: Alexandru Vasile <[email protected]>

lexnv merged commit 08a2305 into master Sep 11, 2023
3 checks passed

lexnv mentioned this pull request Sep 12, 2023

chore: Release trie-db 0.28.0 #200

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

trie-db: Fetch the closest merkle value #199

trie-db: Fetch the closest merkle value #199

lexnv commented Aug 21, 2023 •

edited

Loading

arkpar Aug 22, 2023 •

edited

Loading

jsdw Aug 22, 2023

arkpar commented Aug 22, 2023

cheme left a comment

cheme left a comment

bkchr left a comment

bkchr Sep 5, 2023

bkchr Sep 5, 2023

cheme Sep 6, 2023

cheme Sep 6, 2023

bkchr Sep 6, 2023

lexnv Sep 6, 2023

bkchr Sep 6, 2023

trie-db: Fetch the closest merkle value #199

trie-db: Fetch the closest merkle value #199

Conversation

lexnv commented Aug 21, 2023 • edited Loading

arkpar Aug 22, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arkpar commented Aug 22, 2023

cheme left a comment

Choose a reason for hiding this comment

cheme left a comment

Choose a reason for hiding this comment

bkchr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lexnv commented Aug 21, 2023 •

edited

Loading

arkpar Aug 22, 2023 •

edited

Loading