feat: state boundaries on KV pairs #8995

Longarithm · 2023-05-02T12:03:21Z

First step towards #8984. Here I want to guarantee that state part boundaries always correspond to some key-value pair except two corner cases:

part_id = 0 -> trie key is empty which is lower than all keys
part_id = num_parts -> trie key is [16] which is larger than all keys
This guarantees that parts cover all nodes in state.

It solves an inconvenience on the way to #8898. It is useful to assume that boundaries are keys, because it allows to restore all keys in part by making trivial range query to flat storage. Otherwise you need a hack to convert one last nibble to byte. This is also necessary if we switch to AVL or other tree some day - AVL should not know about trie nodes, and interface should be defined in terms of state key-value pairs.

Some auxiliary work:

more documentation for state_parts
removing visit_nodes_for_size_range_old as it was needed for backwards compatibility for version deprecated long ago

@nikurt note that after this, existing state parts become incompatible with newly generated ones.

Testing

Testing is a pain because current testset is not well organised. I'm adding two tests specifically for new behaviour:

boundary_is_state_key - checks that state boundary is a key for sampling small trie. Doesn't pass without this change.
single_path_trie - small sanity check that keys are evenly distributed among state parts.

Also testing revealed that run_test_parts_not_huge doesn't check anything, see Zulip thread. I refactored test in such way that we separately check proof size and whole part size. Manually checked that two parts doesn't fit in memory limit for that test.

nikurt · 2023-05-04T12:41:55Z

existing state parts become incompatible with newly generated ones.

It breaks backwards compatibility, but state sync is currently not working for testnet and mainnet. Sounds good to me to proceed without explicitly taking care of backwards compatibility.

nikurt · 2023-05-04T12:37:38Z

core/primitives/src/challenge.rs

+/// state part boundaries and storing state items for state part range.
+pub enum PartialState {
+    /// State represented by the trie nodes.
+    Nodes(Vec<StateItem>),


Nodes is confusing, because it contains both Trie Nodes and Values referenced by those nodes.

How about TrieItems?
I really want to resolve this confusion in the whole codebase and call these entities "items" instead of "nodes" or "nodes-or-values"

TrieItem sounds good.

Took another look and renamed with TrieValue. Because here we store only DB values, and item is more like KV pair in my mind.

nikurt · 2023-05-04T12:58:55Z

core/store/src/trie/state_parts.rs

-        } else {
+        if *memory_skipped + node_size <= memory_threshold {
+            *memory_skipped += node_size;
+        } else if node.node.has_value() {


Don't we need to extend key_nibbles in case the current node is a Leaf, which is done at line 137?
But IIUC, this code will return at line 130 and not reach line 137.

I don't think we ever should actually hit the line 137 and that whole clause of match. Because if we got to the Leaf, then we know that this is THE node, so line 127 should return false and we should return Ok(false) with incomplete key.
But, I also don't think that completeness of the key maters. What maters is that it is a key prefix that defines the same set of vertices as a full key -- only the Leaf node. Slice of key in the Leaf node is kinda redundant suffix in terms of the trie structure.
My intuition here is that we only use this key_nibbles to iterate trie -- not to retrieve nodes (needs checking), and we iterate trie based on function that works with prefixes (again, needs checking, I'm talking about seek_nibble_slice)
In slightly different words, I think we only working in Trie based on key comparison as in <=, >=, not equality. All of these inequalities will yield the same value if we switch full leaf key for that truncated leaf key, but only if we are only working with this Trie elements. That is true because we constructed such a key prefix that is long enough to include at least one different character from every other Trie element.

It is right that key_nibbles determines node uniquely. However, now I explicitly want key_nibbles to correspond to existing state key in the end. So it was a bug and I fixed it - for a Leaf, I extend key with remaining nibbles and then return.

Also it makes logic more consistent in the sense that key_nibbles always stores all information found during traversing trie.

nikurt · 2023-05-04T13:01:08Z

core/store/src/trie/state_parts.rs

            return Ok(false);
        }
+
        match &node.node {
            TrieNode::Empty => Ok(false),


What are Empty nodes?

This is a single cornercase when Trie is completely empty. The comment looks misleading though, I'll look at it later.

posvyatokum · 2023-05-08T10:51:34Z

core/store/src/trie/state_parts.rs

-        } else {
+        if *memory_skipped + node_size <= memory_threshold {
+            *memory_skipped += node_size;
+        } else if node.node.has_value() {
            return Ok(false);
        }


Are we concerned with the integrity of the memory_skipped? Because I think we skip some nodes without values that still have non zero size, but don't record it in memory_skipped, because there is no other else.
This only happens when memory_skipped is already over threshold, so it doesn't matter for this function, but maybe memory_skipped is used outside somewhere?

Fortunately it is not used outside, but you are right, fixed.

Subj. If during iteration over range `[path_begin, path_end)` you ended up in a Value, it is still necessary to check that current key (key_nibbles) doesn't exceed `path_end`. This is done already when we descend into internal node, and we need to treat Value in the same way. Fortunately there is no visible impact, as we use it to eventually write the whole state, so we just write some values more than once. But it messes with refcounts during testing and leads to refcount leaks. ## Testing Adding `test_visit_interval` specifically for that case. It doesn't pass without fix. `visit_nodes_interval` obviously needs more tests, but at least #8995 should pass tests after this.

Longarithm · 2023-05-11T13:02:11Z

Re-requesting review. Addressed existing comments and added more tests. Now PR has more changes so I also added more description to it.

nikurt · 2023-05-15T14:55:20Z

core/primitives/src/challenge.rs

+/// TODO (#8984): consider supporting format containing trie values only for
+/// state part boundaries and storing state items for state part range.
+pub enum PartialState {
+    /// State represented by the set of unique trie values.


Maybe mention explicitly that this includes Nodes and Values.

nikurt · 2023-05-15T14:57:05Z

core/store/src/trie/state_parts.rs

+                // This line should be unreachable if we descended into current node.
+                // TODO (#8997): test this case properly by simulating trie data corruption.
+                Err(StorageError::StorageInconsistentState(format!(
+                    "Skipped all children of node {node:?} while finding memory \


Suggested change

"Skipped all children of node {node:?} while finding memory \

"Skipped all children of node {node:?} while searching for memory \

Longarithm self-assigned this May 2, 2023

Longarithm added the A-storage Area: storage and databases label May 2, 2023

require kv pairs

7378a87

Longarithm force-pushed the key-in-parts branch from 137ec9a to 7378a87 Compare May 4, 2023 10:44

Merge branch 'master' into key-in-parts

222bc47

Longarithm requested review from nikurt, pugachAG and posvyatokum May 4, 2023 10:44

Longarithm marked this pull request as ready for review May 4, 2023 10:45

Longarithm requested a review from a team as a code owner May 4, 2023 10:45

Longarithm added 5 commits May 4, 2023 15:15

enum

d3023eb

fix usages

344a404

fix more usages

3d5a661

warn fixes

7f78011

comment

e32573a

nikurt reviewed May 4, 2023

View reviewed changes

posvyatokum reviewed May 8, 2023

View reviewed changes

Merge branch 'master' into key-in-parts

7f25c62

Longarithm mentioned this pull request May 10, 2023

fix: visit_nodes_interval should exclude path_end #9043

Merged

more state part tests

362f011

Longarithm force-pushed the key-in-parts branch from 82d404a to 362f011 Compare May 11, 2023 12:04

Longarithm added 4 commits May 11, 2023 16:06

Merge branch 'master' into key-in-parts

5b67a6b

remove println

cba8cd2

trie values

b885848

docs

d81b97a

Longarithm requested a review from posvyatokum May 11, 2023 13:02

Longarithm requested a review from nikurt May 11, 2023 13:02

Longarithm added 5 commits May 11, 2023 17:22

comment

c38e8bf

sanity

d5058d9

Merge branch 'master' into key-in-parts

9d55935

minor fix

be6ad78

Merge branch 'master' into key-in-parts

2001001

nikurt approved these changes May 15, 2023

View reviewed changes

apply suggestions

6f41af8

Longarithm added the S-automerge label May 15, 2023

Merge branch 'master' into key-in-parts

40c2472

near-bulldozer bot merged commit f3bc243 into near:master May 15, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: state boundaries on KV pairs #8995

feat: state boundaries on KV pairs #8995

Longarithm commented May 2, 2023 •

edited

Loading

nikurt commented May 4, 2023

nikurt May 4, 2023

Longarithm May 4, 2023

nikurt May 4, 2023

Longarithm May 11, 2023

nikurt May 4, 2023 •

edited

Loading

posvyatokum May 8, 2023

Longarithm May 11, 2023

nikurt May 4, 2023

Longarithm May 11, 2023 •

edited

Loading

posvyatokum May 8, 2023

Longarithm May 11, 2023

Longarithm commented May 11, 2023 •

edited

Loading

nikurt May 15, 2023

nikurt May 15, 2023

	"Skipped all children of node {node:?} while finding memory \
	"Skipped all children of node {node:?} while searching for memory \

feat: state boundaries on KV pairs #8995

feat: state boundaries on KV pairs #8995

Conversation

Longarithm commented May 2, 2023 • edited Loading

Testing

nikurt commented May 4, 2023

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

nikurt May 4, 2023 • 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

Longarithm May 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Longarithm commented May 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Longarithm commented May 2, 2023 •

edited

Loading

nikurt May 4, 2023 •

edited

Loading

Longarithm May 11, 2023 •

edited

Loading

Longarithm commented May 11, 2023 •

edited

Loading