feat(rpc): storage proofs for snos and v0.8.0 #292

cchudant · 2024-10-01T16:27:47Z

Pull Request type

Please add the labels corresponding to the type of changes your PR introduces:

Feature

What is the current behavior?

Resolves: #NA

What is the new behavior?

Does this introduce a breaking change?

Yes (deleted some unused rocksdb columns - old database won't open.)

Other information

HermanObst · 2024-10-29T18:48:08Z

@antiyro @cchudant hey there! I started reviewing this PR, but I would really appreciate a description that summarize the changes I need to expect in the PR.

crates/client/db/src/bonsai_db.rs

crates/client/rpc/src/lib.rs

crates/client/rpc/src/versions/v0_8_0/get_storage_proof.rs

cchudant · 2024-11-06T12:48:04Z

@HermanObst @shamsasari I don't 100% know where to start really, but the basic idea is this:
Bonsai-trie has a trie-log and uses snapshots to get the historical state of the global tries. The way it's handled today in bonsai-trie bothers me quite a lot, and this is the stop-gap solution that uses what's already there in bonsai-trie, with some drawbacks:

we can't get a storage proof for a block that is earlier than the block the node restarted at (due to using rocksdb snapshots)
the trie logs are not compact at all and handled weirdly in bonsai-trie
i had to remove a few features temporarily in bonsai-trie to make all of this work

The snapshot arc thing is a thing that stores a rocksdb Snapshot in an Arc, so that it can be stored in the MadaraBackend. Otherwise, it has an annoying lifetime. See rust-rocksdb/rust-rocksdb#937 and rust-rocksdb/rust-rocksdb#936

That's the hardest part, getting a MerkleTree out of the trie log. Otherwise, generating the merkle proof is as simple as calling getStorageProof which is implemented in my new tree traversal Iterator struct from madara-alliance/bonsai-trie#36

I started reworking the whole trie-log idea in bonsai-trie from scratch in a way that would fix all of the drawbacks & not use rocksdb snapshots. That won't allow us to get rid of our rocksdb fork though, since I will use ArcSnapshot for other things.

Feel free to ask more questions though, this pr is a weird one.

shamsasari

Thanks for the comment, it was very useful. Something of it needs to be in the code as well.

Cargo.toml

crates/client/db/src/rocksdb_snapshot.rs

crates/client/db/src/snapshots.rs

shamsasari · 2024-11-21T18:14:39Z

crates/client/db/src/bonsai_db.rs

        // NB: we don't need old value as the trie log is not used :)
        // this actually speeds up things quite a lot


This comment isn't relevant anymore?

shamsasari · 2024-11-21T18:23:42Z

crates/client/db/src/bonsai_db.rs

+        _batch: Option<&mut Self::Batch>,
+    ) -> Result<Option<ByteVec>, Self::DatabaseError> {
+        self.changed.insert(to_changed_key(key), Some(value.into()));
+        Ok(None)


self.changed.insert gives you the old value. Why is it not returned to the caller?

shamsasari · 2024-11-21T18:23:53Z

crates/client/db/src/bonsai_db.rs

+        key: &DatabaseKey,
+        _batch: Option<&mut Self::Batch>,
+    ) -> Result<Option<ByteVec>, Self::DatabaseError> {
+        self.changed.insert(to_changed_key(key), None);


shamsasari · 2024-11-21T18:25:01Z

crates/client/db/src/bonsai_db.rs

+    }
+
+    fn write_batch(&mut self, _batch: Self::Batch) -> Result<(), Self::DatabaseError> {
+        // TODO: this is not really used yet, this whole abstraction does not really make sense anyway, this needs to be modified


Use unreachable! then?

shamsasari · 2024-11-21T18:37:44Z

crates/client/db/src/snapshots.rs

+        let mut snapshots = self.snapshots.write().expect("Poisoned lock");
+
+        if let btree_map::Entry::Vacant(entry) = snapshots.entry(id) {
+            tracing::debug!("Making snap at {id:?}");


snap -> snapshot

shamsasari · 2024-11-21T18:52:26Z

crates/node/src/cli/db.rs

@@ -13,4 +13,21 @@ pub struct DbParams {
    /// Restore the database at startup from the latest backup version. Use it with `--backup-dir <PATH>`
    #[clap(env = "MADARA_RESTORE_FROM_LATEST_BACKUP", long)]
    pub restore_from_latest_backup: bool,
+
+    /// Maximal number of trie logs saved.


shamsasari · 2024-11-21T18:53:18Z

crates/node/src/cli/db.rs

@@ -13,4 +13,21 @@ pub struct DbParams {
    /// Restore the database at startup from the latest backup version. Use it with `--backup-dir <PATH>`
    #[clap(env = "MADARA_RESTORE_FROM_LATEST_BACKUP", long)]
    pub restore_from_latest_backup: bool,
+
+    /// Maximal number of trie logs saved.


Will users know what trie logs are?

shamsasari · 2024-11-21T18:54:12Z

crates/node/src/cli/db.rs

+    #[clap(env = "MADARA_DB_MAX_SAVED_TRIE_LOGS", long, default_value_t = 0)]
+    pub db_max_saved_trie_logs: usize,
+
+    /// How many of the latest snapshots are saved, older ones are discarded.


snapshots of what? This is not explained.

shamsasari · 2024-11-21T18:54:32Z

crates/node/src/cli/db.rs

+
+    /// How many of the latest snapshots are saved, older ones are discarded.
+    /// Higher values cause more database space usage, while lower values prevent the efficient reverting and histoical access for
+    /// the global state trie at older older blocks.


older repeated

shamsasari · 2024-11-21T18:55:33Z

crates/node/src/cli/db.rs

+    pub db_max_saved_snapshots: usize,
+
+    /// A database snapshot is created every `db_snapshot_interval` commits.
+    /// See `db_max_saved_snapshots` to understand what snapshots are used for.


See --db-max-saved-snapshots

crates/client/db/src/bonsai_db.rs

Trantorian1 · 2024-11-25T08:22:40Z

crates/client/db/src/snapshots.rs

+pub struct Snapshots {
+    db: Arc<DB>,
+    snapshots: RwLock<BTreeMap<BasicId, SnapshotRef>>,
+    max_saved_snapshots: Option<usize>,


If I'm not mistaken we keep a certain amount of snaphots in RAM to be able to fetch historical values much faster than with a simple iterative lookup from the tip of the chain. These are lost on node restart though.

crates/client/db/src/lib.rs

crates/client/db/src/snapshots.rs

crates/client/rpc/src/versions/user/v0_8_0/methods/read/get_storage_proof.rs

cchudant · 2024-11-25T16:21:48Z

I have modified upstream bonsai-trie and this branch to apply the trie-logs backwards. This is important because it allows us to serve proofs for blocks that are older than the current oldest snapshot

cchudant · 2024-11-25T16:29:12Z

i also fixed the review comments

Trantorian1

Seems good, great work 👍

HermanObst

This is looking great. Good job team!
Im just requiring some documentation for others to have a faster understanding of the code. The rest looks ok!

I would also would like some test on the class trie, but we can left that to other PR

HermanObst · 2024-10-29T18:54:21Z

crates/client/block_import/src/pre_validate.rs

@@ -365,7 +365,7 @@ fn compute_merkle_root<H: StarkHash + Send + Sync>(values: &[Felt]) -> Felt {
    let config = bonsai_trie::BonsaiStorageConfig::default();
    let bonsai_db = bonsai_trie::databases::HashMapDb::<bonsai_trie::id::BasicId>::default();
    let mut bonsai_storage =
-        bonsai_trie::BonsaiStorage::<_, _, H>::new(bonsai_db, config).expect("Failed to create bonsai storage");
+        bonsai_trie::BonsaiStorage::<_, _, H>::new(bonsai_db, config, /* max tree height */ 64);


Im interested about this tree (specifically about the height). What is for?

https://docs.starknet.io/architecture-and-concepts/network-architecture/block-structure/#transactions_events_receipts_commitments

in-memory merkle tree within every block
max tree height is 64 since the indices are u64s.
will add a comment about that

HermanObst · 2024-11-26T12:38:03Z

crates/client/db/src/bonsai_db.rs

        if let Some(batch) = batch {
            batch.put_cf(&handle, key.as_slice(), value);
        } else {
            self.db.put_cf_opt(&handle, key.as_slice(), value, &self.write_opt)?;
        }
-        Ok(None)
+        Ok(old_value.map(Into::into))


Is this used for something later?

what do you mean?

no it's used now, the old value will be added to the trie log

HermanObst · 2024-11-26T13:01:51Z

crates/client/db/src/bonsai_db.rs

+pub struct BonsaiTransaction {
+    snapshot: SnapshotRef,
+    changed: BTreeMap<(u8, ByteVec), Option<ByteVec>>,
+    column_mapping: DatabaseKeyMapping,
+}


Could you add documentation for this struct? It would be helpful to clarify:

Field Descriptions: For example, what does the changed key represent? (I assume u8 represents the column and ByteVec is the database key?) And why is the value optional?

Usage Examples: Providing examples of how this struct is intended to be used would help others understand its purpose and functionality.

In general, adding more documentation would greatly enhance the code's readability and make it easier for others to understand and work with efficiently :)

well, most of this file needs to be redone anyway, this abstraction is not that good.
will add some comments, but this is a hack for now

HermanObst · 2024-11-26T20:14:05Z

crates/client/db/src/lib.rs

@@ -249,6 +246,19 @@ impl DatabaseExt for DB {
    }
 }

+#[derive(Debug)]


Link to references/documentation?

HermanObst · 2024-11-26T20:32:12Z

crates/client/db/src/snapshots.rs

+
+/// This struct holds the snapshots. To avoid holding the lock the entire time the snapshot is used, it's behind
+/// an Arc. Getting a snapshot only holds the lock for the time of cloning the Arc.
+pub struct Snapshots {


Can we add some docs on how the Snapshots relates to being able to retrieve storage proofs ?
And also slightly document the fields of the struct?

mmh, i'm having a hard time figuring out what needs more documentation here

HermanObst · 2024-11-27T12:09:47Z

crates/client/rpc/src/versions/user/v0_8_0/methods/read/get_storage_proof.rs

+            .chain(iter::once(contract_addresses.len()))
+            .chain(contracts_storage_keys.iter().map(|v| v.storage_keys.len())),
+    );
+    if proof_keys > starknet.storage_proof_config.max_keys {


Is there some limit to this?

HermanObst · 2024-11-27T12:13:14Z

crates/client/rpc/src/versions/user/v0_8_0/methods/read/get_storage_proof.rs

+    let n_tries = saturating_sum(
+        iter::once(!class_hashes.is_empty() as usize)
+            .chain(iter::once(!contract_addresses.is_empty() as usize))
+            .chain(contracts_storage_keys.iter().map(|keys| (!keys.storage_keys.is_empty() as usize))),
+    );


In SNOS, the default usage needs to retrieve proofs for all the tries.
Is there a limitation here?

HermanObst · 2024-11-27T12:14:11Z

crates/client/rpc/src/versions/user/v0_8_0/methods/read/get_storage_proof.rs

+    if n_tries > starknet.storage_proof_config.max_tries {
+        return Err(StarknetRpcApiError::ProofLimitExceeded {
+            kind: StorageProofLimit::MaxUsedTries,
+            limit: starknet.storage_proof_config.max_tries,


Is there a limit to this?
Usually several contract storage tries are queried

yes, for snos you would just put something very big
these limits are just limits that you put for eg. public rpcs so that they don't get DoSed by having to handle huge requests

HermanObst · 2024-11-27T12:30:11Z

crates/node/src/cli/db.rs

+    /// This is the number of blocks for which you can get storage proofs using the storage proof endpoints.
+    /// Blocks older than this limit will not be stored for retrieving historical merkle trie state. By default,
+    /// the value 0 means that no historical merkle trie state access is allowed.
+    #[clap(env = "MADARA_DB_MAX_SAVED_TRIE_LOGS", long, default_value_t = 0)]
+    pub db_max_saved_trie_logs: usize,


What is the maximum number?

there's no maximum number

HermanObst · 2024-11-27T12:49:12Z

crates/node/src/cli/rpc.rs

+    /// Limit how many tries can be used within a single storage proof rpc request. Default: 5.
+    /// The global class trie and global contract tries count each as one, and every contract whose
+    /// storage is queried count as one each.


Is there a max value to this?

cchudant · 2024-11-28T11:56:25Z

@HermanObst I am not really sure what you have in mind for the documentation you are asking.
I have made some modifications, but:

Do you think the documentation in RpcParams and DbParams (which is user-facing) is enough, or should I copy paste all of that all over the codebase everytime one of those fields pop up again? I would assume people read the user-facing doc before jumping into the code, or if they have questions about these args they would just look at how they're defined and find the user-facing RpcParams/DbParams struct pretty easily. I also think that these should have a dedicated page in the user-facing documentation, when we have that.
the snapshots struct in particular looks very self explanatory and I struggle to find any comment that I could add that would explain it better than looking at its code directly. I may be biased as it's my code and I understand it pretty well, but what do you not understand here?

feat(rpc): storage proofs for v0.8.0

e71cce5

antiyro assigned cchudant Oct 2, 2024

antiyro added the enhancement New feature or request label Oct 2, 2024

cchudant added 8 commits October 23, 2024 10:01

historical

52a9207

merged with main

15c2610

fix, and add trie log args to cli

3d73f24

starting to make an e2e test

d1361a0

e2e test

62fc546

Merge remote-tracking branch 'origin/main' into storage-proofs

27ccc75

fix: tests

f45114e

fix: fmt, logs & bonsai-trie path

2d980ef

cchudant marked this pull request as ready for review October 28, 2024 15:06

fix: changelog

8404e2d

antiyro requested a review from HermanObst October 28, 2024 17:36

shamsasari reviewed Oct 30, 2024

View reviewed changes

cchudant added 2 commits November 6, 2024 10:36

fix: review

acf3c67

Merge remote-tracking branch 'origin/main' into storage-proofs

b0a72df

shamsasari reviewed Nov 8, 2024

View reviewed changes

cchudant mentioned this pull request Nov 20, 2024

dev docs: add documentation to the rocksdb configuration #392

Open

fix review comments

4838e60

cchudant mentioned this pull request Nov 20, 2024

Trie log fixes for madara madara-alliance/bonsai-trie#37

Merged

cchudant added 4 commits November 21, 2024 12:29

fix: change bonsai-trie fork

fdfb0f3

Merge remote-tracking branch 'origin/main' into storage-proofs

4881a02

fix: snapshot arc comments

788d504

Merge branch 'main' into storage-proofs

cb50e6b

shamsasari suggested changes Nov 21, 2024

View reviewed changes

Trantorian1 requested changes Nov 25, 2024

View reviewed changes

cchudant added 2 commits November 25, 2024 16:22

fix: apply trie log backwards

63d63b5

Merge remote-tracking branch 'origin/main' into storage-proofs

d56a12c

cchudant requested review from Trantorian1 and shamsasari November 25, 2024 16:29

Trantorian1 approved these changes Nov 25, 2024

View reviewed changes

fix: db-max-saved-snapshots

b31dc10

HermanObst requested changes Nov 27, 2024

View reviewed changes

fix: docs

8ddea25

Merge branch 'main' into storage-proofs

a2caeba

		// NB: we don't need old value as the trie log is not used :)
		// this actually speeds up things quite a lot

feat(rpc): storage proofs for snos and v0.8.0 #292

Are you sure you want to change the base?

feat(rpc): storage proofs for snos and v0.8.0 #292

Conversation

cchudant commented Oct 1, 2024

Pull Request type

What is the current behavior?

What is the new behavior?

Does this introduce a breaking change?

Other information

HermanObst commented Oct 29, 2024

cchudant commented Nov 6, 2024

shamsasari left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shamsasari Nov 21, 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

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

cchudant commented Nov 25, 2024

cchudant commented Nov 25, 2024

Trantorian1 left a comment

Choose a reason for hiding this comment

HermanObst 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

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

Choose a reason for hiding this comment

cchudant commented Nov 28, 2024

shamsasari left a comment •

edited

Loading

shamsasari Nov 21, 2024 •

edited

Loading