feat: Add StorageIndex newtype #2428

varun-doshi · 2024-12-02T14:20:15Z

Short description of what this PR does.

Sets a new Type StorageIndex for the unique index for the trie nodes

A longer description, include motivation and intent if possible. Detail any caveats or follow-up steps still required.

Ref #2332
Adds a new explicit type StorageIndex to store the unique index for the trie nodes in the database.

Consider motivating any new dependencies.

kkovaacs

Looks promising, thanks!

There are a few more places where the new StorageIndex type should be used though (which would probably also warrant moving it to the common crate).

In particular pathfinder_storage::connection::trie has a few methods that are returning an Option<u64> -- these are in fact storage indices, so the return type should probably be an Option<StorageIndex> instead. Also, MerkleTree::new() should probably take a StorageIndex as well, and InternalNode::Unresolved should contain a StorageIndex, too.

crates/merkle-tree/src/merkle_node.rs

varun-doshi · 2024-12-04T10:16:25Z

Resolved comments

And did linting on some files

kkovaacs · 2024-12-04T14:10:32Z

Actually, that linting might make this PR pretty much impossible to review (too many unrelated changes). Please note that we're using nightly rustfmt for formatting. Please check our justfile for the proper command to format the code.

varun-doshi · 2024-12-04T14:21:31Z

Actually, that linting might make this PR pretty much impossible to review (too many unrelated changes). Please note that we're using nightly rustfmt for formatting. Please check our justfile for the proper command to format the code.

Ahh apologies for the wrong linting...it should be fixed now

sistemd · 2024-12-06T12:23:04Z

crates/common/src/lib.rs

@@ -27,6 +27,7 @@ pub mod prelude;
 pub mod receipt;
 pub mod signature;
 pub mod state_update;
+pub mod storage_index;


I think this fits better into the merkle-tree crate since that's the only place where it's relevant.

storage_index would also be used in pathfinder-storage

If i move storage_index to the pathfinder-merkle-tree crate and then try to import this in the pathfinder-storage crate ,it creates a circular dependency issue

In that case can you maybe move the type to the storage crate and please rename it to MerkleStorageIndex or TrieStorageIndex?

These storage indices are not really defined by the protocol, they have nothing to do with starknet per se; they're an implementation detail of our storage mechanism. In common we're trying to only keep the most core types for the protocol.

Understood

Moved to storage crate and renamed to TrieStorageIndex

kkovaacs

Generally, this PR looks really promising to me. Now that we have the basics right I think we should make even more changes in connection::trie. In general, we should be wary of u64 popping up in function signatures anywhere.

I'm not sure this list is exhaustive, but the general idea is that wherever we're actually having a storage index we should use TrieStorageIndex instead of u64:

RootIndexUpdate::Updated should now contain a TrieStorageIndex
TrieUpdate::nodes_removed should be a Vec<TrieStorageIndex>
NodeRef::TrieStorageIndex should contain a TrieStorageIndex
StoredNode u64 ids are also storage indices
Transaction::trie_node() and Transaction::trie_node_hash() are also taking storage indices as their index argument

kkovaacs · 2024-12-10T09:37:50Z

crates/merkle-tree/src/contract.rs

@@ -66,7 +67,7 @@ impl<'tx> ContractsStorageTree<'tx> {
            block: Some(block),
            contract,
        };
-        let tree = MerkleTree::new(root);
+        let tree = MerkleTree::new(TrieStorageIndex::new(root));


Looks like tx.contract_root_index() is still returning a trie storage index as an u64. It should be modified to return a Result<Option<TrieStorageIndex>> like class_root_index() and storage_root_index().

varun-doshi · 2024-12-13T09:15:10Z

@kkovaacs friendly ping for review

kkovaacs · 2024-12-13T15:09:29Z

@varun-doshi Could you please check (and fix) the test failures It seems there are a few tests that are failing with the latest state.

varun-doshi · 2024-12-13T16:05:38Z

@varun-doshi Could you please check (and fix) the test failures It seems there are a few tests that are failing with the latest state.

Fixed the issues caused by the StorageIndex changes.

However, I ran into some protobuf issues on my local test. I then pull upstream main to a new branch and tested it there as well and it threw the same issue. Can you confirm if thats a failing test on main or its on my side?

vbar · 2024-12-16T10:03:15Z

However, I ran into some protobuf issues on my local test. I then pull upstream main to a new branch and tested it there as well and it threw the same issue. Can you confirm if thats a failing test on main or its on my side?

Well, if you mean #2446 , that should be fixed in main now (by #2447 ) - do tests work for you when you merge that?

varun-doshi · 2024-12-16T19:57:27Z

However, I ran into some protobuf issues on my local test. I then pull upstream main to a new branch and tested it there as well and it threw the same issue. Can you confirm if thats a failing test on main or its on my side?

Well, if you mean #2446 , that should be fixed in main now (by #2447 ) - do tests work for you when you merge that?

All good now

kkovaacs

Another step forward, nice! Now I hope we're getting really close to using TrieStorageIndex everywhere we're using storage indices.

kkovaacs · 2024-12-17T10:06:40Z

crates/merkle-tree/src/contract.rs

@@ -173,7 +174,7 @@ impl<'tx> StorageCommitmentTree<'tx> {
            block: Some(block),
        };

-        let tree = MerkleTree::new(root);
+        let tree = MerkleTree::new(TrieStorageIndex::new(root.get()));


Do we need the TrieStorageIndex conversion here? As far as I've seen tx.storage_root_index() is now returning an Option<TrieStorageIndex> now so this seems to be redundant.

kkovaacs · 2024-12-17T10:07:26Z

crates/merkle-tree/src/tree.rs

-        let root = Some(Rc::new(RefCell::new(InternalNode::Unresolved(root))));
+    pub fn new(root: TrieStorageIndex) -> Self {
+        let root = Some(Rc::new(RefCell::new(InternalNode::Unresolved(
+            TrieStorageIndex::new(root.get()),


This seems to be a redundant conversion here. root is already a TrieStorageIndex.

kkovaacs · 2024-12-17T10:10:54Z

crates/merkle-tree/src/tree.rs

@@ -589,12 +603,12 @@ impl<H: FeltHash, const HEIGHT: usize> MerkleTree<H, HEIGHT> {
                        height += 1;

                        let left = storage
-                            .hash(left)
+                            .hash(left.get())


Can't we change the pathfinder_merkle_tree::storage::Storage trait to use TrieStorageIndex as arguments to get() and hash()? That seems to be another place where we're still having trie storage indices passed as u64. That would also allow us removing many of these conversions in this file.

kkovaacs · 2024-12-17T10:12:50Z

crates/rpc/src/method/get_storage_proof.rs

+        tx,
+        block_number,
+        contract_addresses,
+        storage_root_idx.get(),


This is also suspicious: StorageCommitmentTree::get_proofs() is taking a root index as an u64 here -- we should avoid that.

kkovaacs · 2024-12-17T10:13:23Z

crates/rpc/src/pathfinder/methods/get_proof.rs

+            &tx,
+            header.number,
+            input.class_hash,
+            class_root_idx.get(),


Same here: root index passed as an u64.

kkovaacs · 2024-12-17T10:16:48Z

crates/storage/src/connection/trie.rs

@@ -594,7 +614,7 @@ impl Transaction<'_> {
            .prepare_cached(&format!("SELECT hash FROM {table} WHERE idx = ?"))
            .context("Creating get statement")?;

-        stmt.query_row(params![&index], |row| row.get_felt(0))
+        stmt.query_row(params![&index.get()], |row| row.get_felt(0))


Implementing pathfinder_storage::params::ToSql would make it possible to use a TrieStorageIndex as a parameter directly. Please check the to_sql_int!() macro to implement ToSql.

kkovaacs · 2024-12-17T10:18:34Z

crates/storage/src/lib.rs

@@ -7,7 +7,7 @@ mod prelude;

 mod bloom;
 pub use bloom::BLOCK_RANGE_LEN;
-mod connection;
+pub mod connection;


Can we avoid making the connection module public? We already expose all the contents via pub use connection::* from the crate root so this seems redundant?

varun-doshi added 2 commits December 2, 2024 19:41

replaced storage_index from u64 to proper type

f583e8b

fmt

c2fc124

varun-doshi requested a review from a team as a code owner December 2, 2024 14:20

kkovaacs reviewed Dec 3, 2024

View reviewed changes

crates/merkle-tree/src/merkle_node.rs Outdated Show resolved Hide resolved

fixed comments and lint

d127455

fix: fixed lint

dd4dbf6

varun-doshi requested a review from kkovaacs December 4, 2024 14:22

sistemd reviewed Dec 6, 2024

View reviewed changes

renamed to TrieStorageIndex and moved to Storage crate

20e82d9

varun-doshi requested a review from sistemd December 10, 2024 07:57

kkovaacs reviewed Dec 10, 2024

View reviewed changes

varun-doshi added 2 commits December 11, 2024 20:33

convert u64 index types to TrieStorageIndex

a4152a9

merge:main

c20bfac

varun-doshi requested a review from kkovaacs December 11, 2024 15:09

fmt

596af2e

vbar mentioned this pull request Dec 13, 2024

Add StorageIndex newtype #2332

Open

updated to pass tests

d818c5a

kkovaacs reviewed Dec 17, 2024

View reviewed changes

resolved comments

5b6d3ae

varun-doshi requested a review from kkovaacs December 22, 2024 06:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add StorageIndex newtype #2428

feat: Add StorageIndex newtype #2428

varun-doshi commented Dec 2, 2024 •

edited

Loading

kkovaacs left a comment

varun-doshi commented Dec 4, 2024

kkovaacs commented Dec 4, 2024

varun-doshi commented Dec 4, 2024

sistemd Dec 6, 2024 •

edited

Loading

varun-doshi Dec 7, 2024

varun-doshi Dec 7, 2024

sistemd Dec 9, 2024

varun-doshi Dec 10, 2024

kkovaacs left a comment

kkovaacs Dec 10, 2024

varun-doshi commented Dec 13, 2024

kkovaacs commented Dec 13, 2024

varun-doshi commented Dec 13, 2024 •

edited

Loading

vbar commented Dec 16, 2024

varun-doshi commented Dec 16, 2024

kkovaacs left a comment

kkovaacs Dec 17, 2024

kkovaacs Dec 17, 2024

kkovaacs Dec 17, 2024

kkovaacs Dec 17, 2024

kkovaacs Dec 17, 2024

kkovaacs Dec 17, 2024

kkovaacs Dec 17, 2024

feat: Add StorageIndex newtype #2428

Are you sure you want to change the base?

feat: Add StorageIndex newtype #2428

Conversation

varun-doshi commented Dec 2, 2024 • edited Loading

Short description of what this PR does.

A longer description, include motivation and intent if possible. Detail any caveats or follow-up steps still required.

Consider motivating any new dependencies.

kkovaacs left a comment

Choose a reason for hiding this comment

varun-doshi commented Dec 4, 2024

kkovaacs commented Dec 4, 2024

varun-doshi commented Dec 4, 2024

sistemd Dec 6, 2024 • 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

kkovaacs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

varun-doshi commented Dec 13, 2024

kkovaacs commented Dec 13, 2024

varun-doshi commented Dec 13, 2024 • edited Loading

vbar commented Dec 16, 2024

varun-doshi commented Dec 16, 2024

kkovaacs 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

varun-doshi commented Dec 2, 2024 •

edited

Loading

sistemd Dec 6, 2024 •

edited

Loading

varun-doshi commented Dec 13, 2024 •

edited

Loading