[trie] Unify implementation of DiskTrieIterator and MemTrieIterator #12813

shreyan-gupta · 2025-01-28T08:24:27Z

We had separate implementations for DiskTrieIterator and MemTrieIterator which were originally copy pasted from the same source. This PR unifies the implementation while exposing a simple interface to get nodes and values from trie.

Part of issue #12361

shreyan-gupta · 2025-01-28T08:31:56Z

core/store/src/trie/raw_node.rs

@@ -41,13 +41,6 @@ impl RawTrieNode {
            None => Self::BranchNoValue(children),
        }
    }
-
-    pub fn has_value(&self) -> bool {


No longer needed has_value function. It's not used anywhere

shreyan-gupta · 2025-01-28T08:32:37Z

runtime/runtime/src/state_viewer/mod.rs

@@ -170,14 +171,18 @@ impl TrieViewer {
        let mut values = vec![];
        let query = trie_key_parsers::get_raw_prefix_for_contract_data(account_id, prefix);
        let acc_sep_len = query.len() - prefix.len();
-        let mut iter = state_update.trie().disk_iter()?;
-        iter.remember_visited_nodes(include_proof);


We got rid of remember_visited_nodes in favor of using recorded storage, which makes a lot more sense.

This doesn't seem to be working as expected, I'll take a detailed look.

shreyan-gupta · 2025-01-28T08:34:23Z

core/store/src/trie/ops/iter.rs

+}
+
+// Extension for State Parts processing
+impl<I> TrieIteratorImpl<CryptoHash, ValueHandle, I>


I could only implement this function for DiskTrieIterator as the type of Node and Value is the same, i.e. CryptoHash. It doesn't work for a generic implementation to store TrieTraversalItem.

Ideally we should get rid of this code in favor of using trie recorder :/

shreyan-gupta · 2025-01-28T08:35:25Z

core/store/src/trie/ops/interface.rs

+    fn get_and_record_node(
+        &self,
+        ptr: GenericTrieNodePtr,
+    ) -> Result<GenericTrieNode<GenericTrieNodePtr, GenericValueHandle>, StorageError>;


MemTrie doesn't use the Resultness of this function, but it's needed for DiskTrieIterator, specially given the fact we don't simply want to panic when a node is not found.

shreyan-gupta · 2025-01-28T08:36:13Z

core/store/src/trie/mod.rs

@@ -103,7 +102,7 @@ pub enum KeyLookupMode {
 const TRIE_COSTS: TrieCosts = TrieCosts { byte_of_key: 2, byte_of_value: 1, node_cost: 50 };

 #[derive(Clone, Copy, Hash)]
-pub(crate) enum ValueHandle {
+pub enum ValueHandle {


Needed to make this pub from pub(crate) as disk_iter functions is exposed outside module

codecov · 2025-01-28T12:12:27Z

Codecov Report

Attention: Patch coverage is 95.67901% with 14 lines in your changes missing coverage. Please review.

Project coverage is 70.44%. Comparing base (8097202) to head (608e8fd).
Report is 3 commits behind head on master.

Files with missing lines	Patch %	Lines
core/store/src/trie/ops/iter.rs	97.15%	4 Missing and 3 partials ⚠️
core/store/src/trie/iterator.rs	82.14%	1 Missing and 4 partials ⚠️
core/store/src/trie/mem/iter.rs	95.83%	0 Missing and 1 partial ⚠️
core/store/src/trie/mod.rs	93.33%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #12813      +/-   ##
==========================================
- Coverage   70.47%   70.44%   -0.04%     
==========================================
  Files         847      848       +1     
  Lines      175001   174849     -152     
  Branches   175001   174849     -152     
==========================================
- Hits       123338   123178     -160     
- Misses      46413    46419       +6     
- Partials     5250     5252       +2

Flag	Coverage Δ
backward-compatibility	`0.16% <0.00%> (+<0.01%)`	⬆️
db-migration	`0.16% <0.00%> (+<0.01%)`	⬆️
genesis-check	`1.41% <0.00%> (+<0.01%)`	⬆️
linux	`70.05% <95.67%> (-0.02%)`	⬇️
linux-nightly	`70.07% <95.67%> (-0.05%)`	⬇️
pytests	`1.71% <0.00%> (+<0.01%)`	⬆️
sanity-checks	`1.52% <0.00%> (+<0.01%)`	⬆️
unittests	`70.28% <95.67%> (-0.04%)`	⬇️
upgradability	`0.20% <0.00%> (+<0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Longarithm

Awesome!

Longarithm · 2025-01-28T09:47:23Z

tools/state-viewer/src/commands.rs

@@ -1000,7 +1000,7 @@ pub(crate) fn print_epoch_analysis(

    // The parameters below are required for the next next epoch generation.
    // For `CheckConsistency` mode, they will be overridden in the loop.
-    // For `Backtest` mode, they will stay the same and override information
+    // For `BackTest` mode, they will stay the same and override information


I'd rather add "backtest" to cspell and made these changes in separate PR (because this one is very involved)
Maybe there is some dictionary with engineering words we could just reuse?

Will revert! Apologies!

Longarithm · 2025-01-28T16:30:15Z

tools/state-viewer/src/cli.rs

@@ -644,7 +644,7 @@ pub enum EpochAnalysisMode {
    /// start epoch height.
    /// TODO (#11477): doesn't work for start epoch height <= 544 because of
    /// `EpochOutOfBounds` error.
-    Backtest,
+    BackTest,


I'd rather add "backtest" to cspell and made these changes in separate PR (because this one is very involved). Also, for clap enum here it implicitly changes flag to --back-test which is not desired.

Maybe there is some dictionary with engineering words we could just reuse?

Longarithm · 2025-01-28T16:32:25Z

core/store/src/trie/iterator.rs

-    pub hash: CryptoHash,
-    /// Key of the node if it stores a value.
-    pub key: Option<Vec<u8>>,
+    /// This is used only for state_viewer.


Oh, there is a naming collision... Can we reference it as TrieViewer because it is actually exposed to user, while classic state viewer is dev only?

Independent to this change, confirming, it shouldn't be a problem if we were to change this in the future right? As long as we keep all the relevant nodes in the proof?

Yeah.
Now I think it doesn't record values because they are given anyway in ViewStateResult. Maybe we can add some flag to TrieRecorder to skip recording values.

Otherwise we could ask community whether anyone cares about current exact proof format or its size / notify about this change. I don't think there will be complaints.

Longarithm · 2025-01-28T17:56:34Z

core/store/src/trie/mem/iter.rs

-
-pub struct MemTrieIterator<'a, M: ArenaMemory> {
-    root: Option<MemTrieNodePtr<'a, M>>,
+/// Tiny wrapper around `MemTries` and `Trie` to provide `TrieIteratorStorageInterface` implementation.


Suggested change

/// Tiny wrapper around `MemTries` and `Trie` to provide `TrieIteratorStorageInterface` implementation.

/// Tiny wrapper around `MemTries` and `Trie` to provide `GenericTrieInternalStorage` implementation.

Longarithm · 2025-01-28T18:06:19Z

core/store/src/trie/iterator.rs

@@ -716,39 +343,4 @@ mod tests {
            .collect();
        assert_eq!(got, want);
    }
-
-    #[test]
-    fn test_has_value() {


Can be migrated to TrieIteratorImpl for DiskTrieIteratorInner. Maybe there is no value in keeping it, but there is still a need to get all state KV pairs for state part AFAIU.

This test didn't make too much sense to me. It's basically checking if an internal implementation detail IterStep is of type value, then value should exist, else it shouldn't?

I think this test was originally in place as we had the function has_value() exposed from the iterator which no longer exists, hence the test is also irrelevant.

If we care about this for state parts, the better way of testing this is just doing the actual iteration and checking whether all data that we want exists, which I believe we already do in the state parts tests.

It checked whether two different ways to check value existence match each other. However, your argument makes sense.

shreyan-gupta added 3 commits January 28, 2025 01:53

memtrie iter

c6d84d6

disk iter

f39634b

spell

2f61045

shreyan-gupta commented Jan 28, 2025

View reviewed changes

shreyan-gupta added 2 commits January 28, 2025 14:08

rename

7741101

test fix

34a71fa

shreyan-gupta requested a review from Longarithm January 28, 2025 08:47

shreyan-gupta marked this pull request as ready for review January 28, 2025 08:47

shreyan-gupta requested a review from a team as a code owner January 28, 2025 08:47

shreyan-gupta added 3 commits January 28, 2025 14:19

Merge branch 'master' into shreyan/trie/iter

e9e8022

spellcheck fix

be74ec0

fix test

eaa4882

Longarithm approved these changes Jan 28, 2025

View reviewed changes

shreyan-gupta added 2 commits January 28, 2025 23:47

revert cspell changes

628cfa9

comments

608e8fd

shreyan-gupta added this pull request to the merge queue Jan 28, 2025

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jan 28, 2025

shreyan-gupta added this pull request to the merge queue Jan 28, 2025

Merged via the queue into master with commit 57b9b81 Jan 28, 2025
28 of 29 checks passed

shreyan-gupta deleted the shreyan/trie/iter branch January 28, 2025 19:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[trie] Unify implementation of DiskTrieIterator and MemTrieIterator #12813

[trie] Unify implementation of DiskTrieIterator and MemTrieIterator #12813

shreyan-gupta commented Jan 28, 2025 •

edited

Loading

shreyan-gupta Jan 28, 2025

shreyan-gupta Jan 28, 2025

shreyan-gupta Jan 28, 2025

shreyan-gupta Jan 28, 2025

shreyan-gupta Jan 28, 2025

shreyan-gupta Jan 28, 2025

codecov bot commented Jan 28, 2025 •

edited

Loading

Longarithm left a comment

Longarithm Jan 28, 2025

shreyan-gupta Jan 28, 2025

Longarithm Jan 28, 2025

Longarithm Jan 28, 2025

shreyan-gupta Jan 28, 2025

Longarithm Jan 28, 2025

Longarithm Jan 28, 2025

shreyan-gupta Jan 28, 2025

Longarithm Jan 28, 2025

shreyan-gupta Jan 28, 2025 •

edited

Loading

Longarithm Jan 28, 2025

	/// Tiny wrapper around `MemTries` and `Trie` to provide `TrieIteratorStorageInterface` implementation.
	/// Tiny wrapper around `MemTries` and `Trie` to provide `GenericTrieInternalStorage` implementation.

[trie] Unify implementation of DiskTrieIterator and MemTrieIterator #12813

[trie] Unify implementation of DiskTrieIterator and MemTrieIterator #12813

Conversation

shreyan-gupta commented Jan 28, 2025 • edited Loading

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

codecov bot commented Jan 28, 2025 • edited Loading

Codecov Report

Longarithm 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shreyan-gupta Jan 28, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shreyan-gupta commented Jan 28, 2025 •

edited

Loading

codecov bot commented Jan 28, 2025 •

edited

Loading

shreyan-gupta Jan 28, 2025 •

edited

Loading