feat: optimize `GetAccountProof` for small accounts by drahnr · Pull Request #1214 · 0xMiden/node

drahnr · 2025-09-09T18:10:29Z

Part 2 of #1185 - reworks the GetAccountProof query with a new type.

The changeset focuses on providing account data / vault entries in the response if they are less than 100 (might be too conservative, for both). @SantiagoPittella

drahnr · 2025-09-11T14:55:31Z

crates/store/src/state.rs


-        let account_ids: Vec<AccountId> =
-            account_requests.iter().map(|req| req.account_id).collect();
+        let AccountProofRequest { account_id, block_num: _, details } = account_request;


The block_num will be handled once the relevant data is stored #1185 , which is part of step 3, the next PR.

drahnr · 2025-09-11T18:03:28Z

Hold off for a sec, will push the correct changeset in a few ..

bobbinth · 2025-09-13T06:49:47Z

@drahnr - could you rebase this from the latest next? Seems like some of the changes from #1211 are not here yet.

crates/store/src/state.rs

crates/proto/src/domain/account.rs

bobbinth · 2025-09-29T00:10:19Z

@drahnr - could you merge the latest next into this branch. I think currently it contains some changes that are already in next.

Co-authored-by: Mirko <48352201+Mirko-von-Leipzig@users.noreply.github.com>

bobbinth

Looks good! Thank you! Not a full review yet, but I left some questions/comments inline (mostly about protobuf files).

proto/proto/store/rpc.proto

crates/store/src/state.rs

igamigo

Looks good to me. Left some comments but overall functionality is OK for what we currently use it in the client

crates/proto/src/domain/account.rs

igamigo · 2025-10-02T19:36:33Z

crates/proto/src/domain/account.rs

+        if vault.assets().nth(Self::MAX_RETURN_ENTRIES).is_some() {
+            Self::too_many()
+        } else {
+            Self {
+                too_many_assets: false,
+                assets: Vec::from_iter(vault.assets()),
+            }


This iterates over the vault's assets twice, would be nice to do it in one go but not sure there's a nice way to do that either

We unfortunately don't get access to the underlying data structure, but only in form of an iterator

I think you are one miden-base commit behind 0xMiden/protocol@b6f679b which merged 0xMiden/protocol#1939. If you can update to that, you could use num_assets.

crates/store/src/state.rs

igamigo · 2025-10-02T21:43:34Z

crates/store/src/state.rs

            }
+
+            // Only include unknown account code blobs, which is equal to a account code digest
+            // mismatch. If `None` was requested, don't return any.


I think this differs from the previous approach where we did respond with the account code if it was None, right? Because it signaled that the user did not know the code.

Is the idea that if we want the code we send a dummy commitment now? I think that works but also I don't think there's valid usecases for sending None here and not expecting the code back AFAICT.

The interpretation here changed, can you take a look @bobbinth

I think this was covered in another comment, but yes, this is slightly different. Now, the client can do the following:

If the client doesn't know the code send 0x0000... as commitment. This would guarantee that the current code is returned.

If the client has the code, send its commitment, and then the code would be returned only if there is a mismatch.

There is now also the 3rd option that the client doesn't currently use: send None to indicate that the client is not interested in the code.

proto/proto/store/rpc.proto

PhilippGackstatter

Looks good. Take this as a light approval - I'm missing some context around the client's need with regards to this API. I mainly looked at the usages of the account-related APIs.

crates/store/src/state.rs

proto/proto/store/rpc.proto

PhilippGackstatter · 2025-10-06T14:58:39Z

crates/store/src/state.rs

+            for StorageMapRequest { slot_index, slot_data } in storage_requests {
+                let Some(StorageSlot::Map(storage_map)) =
+                    details.storage().slots().get(slot_index as usize)
+                else {
+                    return Err(AccountError::StorageSlotNotMap(slot_index).into());
+                };
+                let details = AccountStorageMapDetails::new(slot_index, slot_data, storage_map);
+                storage_map_details.push(details);
            }


Question out of curiosity: Why does this not return a merkle proof that proves inclusion of the entry in the storage map? I would have thought that the client needed this, but it seems it doesn't.

I am not fully in the loop how the client uses it CC @igamigo might be able to help here if this is OK, I'll double check tomorrow

As I see it, the client currently doesn't use the account storage map details, but only the account proofs. So the usage would remain the same.
Do we need the proofs for FPI calls? I'd assume so? CC @bobbinth

I don't necessarily want to block the PR any longer

There are 3 ways to fulfill these requests and currently we are supporting two of them. The requests types are:

We request all entries of the storage map: if the number of entries is less than some threshold $n$ (currently, I think $n = 1000$ but maybe it should be smaller), then we return all entries and the client can reconstruct the SMT locally.

We request specific set of key-value pairs. Here we have two options:
a. The total number of key-value pairs in the SMT is less than $n$, in which case we return all key-value pairs.
b. The total number of key-value pairs in the SMT is greater than $n$, in which case we'd need to return Merkle paths for each returned key-value pair (and we can also apply path compaction here).

Case 2b is currently not implemented. Also, looking at the code, I'm not sure the case 2a handled correctly. Specifically, AccountStorageMapDetails::from_slot_data() for now should return all entries if the storage size under a given threshold and return too_many_entries otherwise.

The reason for the above is that if we don't return all entries (and don't return Merkle paths in case of partial entries), the client would have no way to reconstruct the underling SMT - and so the returned data would not be usable.

I think we should do the following:

Open a small PR to fix handling of case 2a.

Create a follow-up issue to implement handling of case 2b.

PhilippGackstatter · 2025-10-06T15:07:08Z

crates/proto/src/domain/account.rs

+        if vault.assets().nth(Self::MAX_RETURN_ENTRIES).is_some() {
+            Self::too_many()
+        } else {
+            Self {
+                too_many_assets: false,
+                assets: Vec::from_iter(vault.assets()),
+            }


I think you are one miden-base commit behind 0xMiden/protocol@b6f679b which merged 0xMiden/protocol#1939. If you can update to that, you could use num_assets.

drahnr · 2025-10-07T16:05:05Z

Merging, I'll address any outstanding issues as a follow-up

bobbinth

Looks good! Thank you! I left a few comments inline. The main one is about fixing how we return storage map data.

proto/proto/store/rpc.proto

bobbinth · 2025-10-07T18:41:00Z

crates/store/src/state.rs

            }
+
+            // Only include unknown account code blobs, which is equal to a account code digest
+            // mismatch. If `None` was requested, don't return any.


I think this was covered in another comment, but yes, this is slightly different. Now, the client can do the following:

If the client doesn't know the code send 0x0000... as commitment. This would guarantee that the current code is returned.

If the client has the code, send its commitment, and then the code would be returned only if there is a mismatch.

There is now also the 3rd option that the client doesn't currently use: send None to indicate that the client is not interested in the code.

bobbinth · 2025-10-07T18:58:24Z

crates/store/src/state.rs

+            for StorageMapRequest { slot_index, slot_data } in storage_requests {
+                let Some(StorageSlot::Map(storage_map)) =
+                    details.storage().slots().get(slot_index as usize)
+                else {
+                    return Err(AccountError::StorageSlotNotMap(slot_index).into());
+                };
+                let details = AccountStorageMapDetails::new(slot_index, slot_data, storage_map);
+                storage_map_details.push(details);
            }


There are 3 ways to fulfill these requests and currently we are supporting two of them. The requests types are:

We request all entries of the storage map: if the number of entries is less than some threshold $n$ (currently, I think $n = 1000$ but maybe it should be smaller), then we return all entries and the client can reconstruct the SMT locally.

We request specific set of key-value pairs. Here we have two options:
a. The total number of key-value pairs in the SMT is less than $n$, in which case we return all key-value pairs.
b. The total number of key-value pairs in the SMT is greater than $n$, in which case we'd need to return Merkle paths for each returned key-value pair (and we can also apply path compaction here).

Case 2b is currently not implemented. Also, looking at the code, I'm not sure the case 2a handled correctly. Specifically, AccountStorageMapDetails::from_slot_data() for now should return all entries if the storage size under a given threshold and return too_many_entries otherwise.

The reason for the above is that if we don't return all entries (and don't return Merkle paths in case of partial entries), the client would have no way to reconstruct the underling SMT - and so the returned data would not be usable.

I think we should do the following:

Open a small PR to fix handling of case 2a.

Create a follow-up issue to implement handling of case 2b.

drahnr force-pushed the bernhard-1185-acounts-part-2 branch from 323f051 to c8d0953 Compare September 10, 2025 13:45

bobbinth mentioned this pull request Sep 11, 2025

Change GetAccountProofs to a single GetAccountProof, cleanup #1211

Merged

drahnr added a commit that referenced this pull request Sep 11, 2025

migrate some changes from #1214

e2e3a9c

drahnr force-pushed the bernhard-1185-acounts-part-2 branch from e003c3b to bce5a37 Compare September 11, 2025 14:48

drahnr changed the base branch from next to bernhard-1185-acounts-part-1 September 11, 2025 14:49

drahnr commented Sep 11, 2025

View reviewed changes

drahnr changed the title ~~more sophisticated account state query~~ feat: optimize GetAccountProof for small accounts Sep 11, 2025

drahnr marked this pull request as ready for review September 11, 2025 17:55

drahnr force-pushed the bernhard-1185-acounts-part-2 branch from 00637fa to 2b77b09 Compare September 11, 2025 17:58

drahnr marked this pull request as draft September 11, 2025 18:03

drahnr marked this pull request as ready for review September 11, 2025 21:36

Base automatically changed from bernhard-1185-acounts-part-1 to next September 12, 2025 11:09

drahnr force-pushed the bernhard-1185-acounts-part-2 branch from 2a88587 to 5486809 Compare September 14, 2025 20:15

This comment was marked as outdated.

Sign in to view

cursor bot reviewed Sep 15, 2025

View reviewed changes

crates/store/src/state.rs Outdated Show resolved Hide resolved

drahnr requested review from Mirko-von-Leipzig and bobbinth September 18, 2025 08:28

Mirko-von-Leipzig reviewed Sep 18, 2025

View reviewed changes

crates/store/src/state.rs Outdated Show resolved Hide resolved

drahnr force-pushed the bernhard-1185-acounts-part-2 branch from 974b43d to a800530 Compare September 19, 2025 12:17

Mirko-von-Leipzig reviewed Sep 22, 2025

View reviewed changes

PhilippGackstatter mentioned this pull request Sep 25, 2025

feat: Add StorageMap::{num_entries, num_leaves} 0xMiden/protocol#1935

Merged

drahnr and others added 6 commits October 1, 2025 17:01

more accounts

5de0aed

format + drop *Response suffix

3b1da97

protobuf comments

e14aeef

regenerate

d4ee9a5

Update crates/store/src/state.rs

d1bb537

Co-authored-by: Mirko <48352201+Mirko-von-Leipzig@users.noreply.github.com>

cleanup

e6a80dd

changed API

eaac935

drahnr force-pushed the bernhard-1185-acounts-part-2 branch from 49f50c9 to eaac935 Compare October 1, 2025 15:47

bobbinth reviewed Oct 2, 2025

View reviewed changes

drahnr added 5 commits October 2, 2025 12:00

proposed extension

c7f4ab4

clippy

13352b3

comment

5f873d7

z

74003a6

address PR review feedback

3230501

drahnr force-pushed the bernhard-1185-acounts-part-2 branch from 5e2cbaf to 3230501 Compare October 2, 2025 14:29

clippy

9c10e85

drahnr requested review from Mirko-von-Leipzig, PhilippGackstatter, bobbinth and igamigo October 2, 2025 15:06

drahnr mentioned this pull request Oct 2, 2025

feat: use a partial SMT proof instead of a list of merkle proofs #1158

Closed

igamigo approved these changes Oct 2, 2025

View reviewed changes

drahnr added 2 commits October 6, 2025 11:15

review comments addressed

58ad95b

clippy

b0a0457

drahnr force-pushed the bernhard-1185-acounts-part-2 branch from 09c319f to b0a0457 Compare October 6, 2025 12:02

PhilippGackstatter approved these changes Oct 6, 2025

View reviewed changes

drahnr added 5 commits October 6, 2025 22:06

account

8bc852f

comment gen

af49296

naming

dde7aad

comment emphasis

9641138

misplaced comment

ba66538

drahnr merged commit 40529f1 into next Oct 7, 2025
6 checks passed

drahnr deleted the bernhard-1185-acounts-part-2 branch October 7, 2025 16:05

bobbinth reviewed Oct 7, 2025

View reviewed changes

drahnr mentioned this pull request Oct 7, 2025

fix: simplify logic in store.rpc for partial queries #1277

Merged

Conversation

drahnr commented Sep 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

drahnr commented Sep 11, 2025

Uh oh!

bobbinth commented Sep 13, 2025

Uh oh!

This comment was marked as outdated.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bobbinth commented Sep 29, 2025

Uh oh!

bobbinth left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

igamigo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

PhilippGackstatter left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

drahnr Oct 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

drahnr commented Oct 7, 2025

Uh oh!

Uh oh!

bobbinth left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

drahnr commented Sep 9, 2025 •

edited

Loading

drahnr Oct 6, 2025 •

edited

Loading