Implemented function for computing of old account states based on deltas #563

polydez · 2024-12-04T12:55:33Z

Related to #527

This function (compute_old_account_states) computes state for given accounts for the specified block number based on deltas for accounts stored in the database.

The current solution is based on getting final account states and apply changes from the final states to the given block number in reverse order.

# Conflicts: # crates/store/Cargo.toml

# Conflicts: # Cargo.lock # Cargo.toml # crates/block-producer/Cargo.toml # crates/store/Cargo.toml # crates/store/src/db/tests.rs

…-acc-states # Conflicts: # crates/store/src/db/sql/mod.rs # crates/store/src/db/sql/queries/compute_old_account_states.sql

…rom_row`

…-acc-states # Conflicts: # crates/store/src/db/sql/queries/select_merged_deltas.sql

Mirko-von-Leipzig

Mostly just style/nit suggestions :) LGTM.

crates/store/src/db/sql/utils.rs

crates/store/src/db/tests.rs

crates/store/src/db/sql/mod.rs

bobbinth · 2024-12-07T22:46:26Z

This function (compute_old_account_states) computes state for given accounts for the specified block number based on deltas for accounts stored in the database. Since it takes into account all deltas from the creation of account up to the current block, if account has a lot of changes, it might be more expensive than applying deltas backward from the latest account state to the specified block, so it might be a subject for future improvement.

Are you thinking of this as a temporary solution? Because I agree that computing account states based on deltas from account creation is probably not a good solution.

And if it is a temporary solution - is it actually worth it? Maybe we should go for a longer-terms solution.

In general, as I mentioned in one other comment, let's look at the whole account data storage/retrieval area more holistically. Let's list the use-cases we are trying to solve for and then see what solution would address them well.

igamigo · 2024-12-09T15:28:26Z

crates/rpc/src/server/api.rs

@@ -188,7 +188,7 @@ impl api_server::Api for RpcApi {
        self.block_producer.clone().submit_proven_transaction(request).await
    }

-    /// Returns details for public (public) account by id.
+    /// Returns details for public account by ID.


I think this is a bit misleading. This method still works for non-public accounts, right? It just doesn't return details for them (ie, it just returns the stored hash). There are some other related functions that have the same problem IIRC (like select_accounts_by_ids which get_account_proofs uses, both for private and public accounts).

igamigo

This might have already been discussed (apologies if so), but considering the "usability" of account states decreases as they move further and further from the chain tip, would it make sense (if it is even possible) to retrieve the latest state and subtract deltas instead of building them up from 0?

igamigo · 2024-12-09T15:56:56Z

crates/store/src/db/sql/mod.rs

+                    a.block_num < b.block_num
+            )
+        ORDER BY
+            account_id


Would something like this work? It looks a bit more readable to me, although there's a good possibility it's not as performant (but with indices and fairly large amount of updates on each account, it might?).

SELECT account_id, nonce FROM account_deltas WHERE account_id IN rarray(?1) AND block_num <= ?2 AND block_num = ( SELECT MAX(block_num) FROM account_deltas b WHERE b.account_id = account_deltas.account_id AND b.block_num <= ?2 ) ORDER BY account_id;

crates/store/src/db/sql/mod.rs

igamigo · 2024-12-09T16:09:00Z

crates/store/src/db/sql/mod.rs

+    }
+
+    let mut rows =


nit: Could be nice to have some section separators or just comments to neatly divide each segment of the account retrieval (or maybe just make these their own functions directly)

# Conflicts: # crates/proto/src/generated/account.rs # crates/rpc-proto/proto/account.proto # crates/store/src/db/sql/mod.rs # crates/store/src/db/tests.rs # proto/account.proto

crates/store/src/lib.rs

Mirko-von-Leipzig · 2025-01-10T12:59:53Z

crates/store/src/db/sql/mod.rs

+            asset: Asset,
+            is_remove: bool,
+        ) -> Result<()> {
+            let account = self.try_get_mut(account_id)?;
+
+            if is_remove {
+                account.vault_mut().remove_asset(asset)
+            } else {
+                account.vault_mut().add_asset(asset)
+            }


Sidebar: could be worth creating AssetUpdate which bundles Asset + is_remove, deprecating remove_asset and add_asset in favor of Asset::update(&AssetUpdate)?

I imagine we have quite a few of these if is_remove { ... } implementations around.

crates/store/src/db/sql/mod.rs

Mirko-von-Leipzig · 2025-01-10T13:08:54Z

crates/store/src/db/sql/mod.rs

+    // TODO: What if account was also changed after requested block number? In this case we will
+    //       miss the update (since `accounts` table stores only latest account states,
+    //       the corresponding record in the table will be updated to the state beyond the requested
+    //       block range).


Sorry, vacation brain rot occurred. What is this saying?

If the accounts table only stores the latest state then how/why do we allow querying by a block range?

It's might be confusing, but accounts table does store the latest state of an account. If the account is updated, its state is replaced in the accounts table. I re-read the code now and now I'm thinking that I probably misunderstood the idea of state sync and this behavior is expected:

miden-node/crates/store/src/state.rs

Lines 500 to 501 in b36a0fd

/// - `account_ids`: Include the account's hash if their _last change_ was in the result's block

/// range.

@bobbinth what do you think?

Co-authored-by: Mirko <[email protected]>

polydez · 2025-01-10T14:17:29Z

I had proposed to write old values in state delta updates, but later we decided to remove it, because we can find old values from previous records. But now I'm thinking, it wasn't that bad idea to have old values stored. It would simplify and speed-up such queries significantly: https://github.com/0xPolygonMiden/miden-node/pull/563/files#diff-90615b33cfa131df3c490d264026c75a73772c02c43d3adaa373a632aca40503R306-R324

But in order to implement this, we would also need to implement reverse delta calculation on delta application in miden-base.

@bobbinth, @Mirko-von-Leipzig, what do you think?

crates/store/src/db/sql/mod.rs

igamigo · 2025-01-15T14:42:09Z

crates/store/src/db/sql/mod.rs

+    impl Accounts {
+        fn try_get_mut(&mut self, id: &AccountId) -> Result<&mut Account> {
+            self.0.get_mut(id).ok_or_else(|| {
+                DatabaseError::DataCorrupted(format!("Account not found in DB: {id}"))


Wouldn't DatabaseError::AccountNotFoundInDb fit this description better?

igamigo · 2025-01-15T14:45:33Z

crates/store/src/db/sql/mod.rs

+                if !id.is_public() {
+                    return Err(DatabaseError::AccountNotOnChain(*id));
+                }


When this is used, this should likely be validated at the RPC level so maybe it's not entirely necessary to validate here as well if we assume calls from RPC to the store are to be trusted. Not sure if we still would want to remove this validation since it's very harmless, but just a note.

igamigo · 2025-01-15T14:55:58Z

crates/store/src/db/sql/mod.rs

+        select_latest_states_stmt.query(params![Rc::clone(&account_ids), block_number])?;
+    while let Some(row) = rows.next()? {
+        let account_details = row.get_ref(1)?.as_blob_or_null()?;
+        if let Some(details) = account_details {


Should the else for this if panic or error? Since it would mean there is something wrong in the database since you checked previously that account IDs are public (so details should be there)

bobbinth

Thank you! Looks good. I left a couple of comments inline - but I do wonder if the current approach is going to be very computationally intensive.

Taking a step back, I wonder if we should break this up into a couple of endpoints. What the client is interested in is getting to a state of a public account not too far from the chain tip and there are really 3 parts to it:

Get account header + storage header.
Get account code.
Get account vault.
Get account storage maps.

The first one we can probably just cache in the database for every update of a public account for some number of recent blocks (e.g., 100 - 200). This will make it very fast to retrieve and will probably not result in too much extra storage pressure.

The second one could be handled pretty easily by having a separate table for storing account code keyed by code commitment. This table could be periodically pruned to get rid of obsolete code entries - but I would imaging that'll happen rather rarely.

For the vault, if the vault is small enough, we could return the entire vault. The size of the vault should be pretty easy to track for each of the account updates (it is just the number of assets in the vault). If the vault is big, we'd need to come up with a different strategy - e.g., either sending vault deltas or somehow paginating through assets.

For storage maps, we could try to apply the same strategy as for the vault - i.e., if the map is small enough, just send all key-value pairs, otherwise either send deltas or do some kind of pagination over key-value pairs.

Sending vault/storage map deltas should be pretty simple - and we kind of already do that. How to do pagination is not super clear to me yet.

Overall, before knowing if the above approach makes sense, we'll need to sketch out how would it affect the existing endpoints. I think these endpoints are:

SyncState - maybe we should return account header for all public accounts?
GetAccountStateDelta - would we still need this endpoint?
GetAccountDetails - should we change this to be about getting vault and storage maps (either via deltas or pagination)?
GetAccountProofs - should we modify the map responses to send just key-value pairs if the maps are small enough? Or maybe we should transform this into a more general GetAccountHeader endpoint which could also optionally return account proofs?

bobbinth · 2025-01-17T02:15:45Z

crates/store/src/db/sql/mod.rs

+#[cfg_attr(not(test), expect(dead_code))] // TODO: remove once the function is used
+pub fn compute_old_account_states(
+    conn: &mut Connection,
+    account_ids: &[AccountId],
+    block_number: BlockNumber,
+) -> Result<Vec<Account>> {


Could we move the contents of this function into a separate module? (e.g., under /src/db/sql/account_state.rs).

Also, maybe it would be better to make it work with one account at a time? Is there a significant benefit to trying to process many accounts at the same time?

bobbinth · 2025-01-17T02:32:47Z

crates/store/src/db/sql/mod.rs

+    // *** Query latest states with calculated nonces ***
+
+    // We select states with possible nonce updates in order to (re)construct `Account` with final
+    // nonce, since we're not able to decrease nonce in `Account`.
+    let mut select_latest_states_stmt = transaction.prepare_cached(
+        "
+        SELECT a.account_id, a.details, d.nonce
+        FROM accounts a LEFT JOIN account_deltas d ON
+            a.account_id = d.account_id AND
+            d.block_num = (
+                SELECT MAX(block_num)
+                FROM account_deltas
+                WHERE
+                    block_num <= ?2 AND
+                    account_id = a.account_id
+            )
+        WHERE a.account_id IN rarray(?1)",
+    )?;
+    let mut rows =
+        select_latest_states_stmt.query(params![Rc::clone(&account_ids), block_number])?;
+    while let Some(row) = rows.next()? {
+        let account_details = row.get_ref(1)?.as_blob_or_null()?;
+        let id = read_from_blob_column(row, 0)?;
+        let details = account_details.ok_or_else(|| {
+            DatabaseError::DataCorrupted(format!(
+                "No details for public account {id} in the database"
+            ))
+        })?;
+        let account = Account::read_from_bytes(details)?;
+        let old_nonce = row.get::<_, Option<u64>>(2)?;
+
+        // If there is updated (old) nonce, reconstruct loaded details with the nonce.
+        let account = if let Some(old_nonce) = old_nonce {
+            let (id, vault, storage, code, _nonce) = account.into_parts();
+            let nonce = old_nonce.try_into().map_err(DatabaseError::DataCorrupted)?;
+
+            Account::from_parts(id, vault, storage, code, nonce)
+        } else {
+            account
+        };
+
+        accounts.0.insert(id, account);
+    }


Would it make sense to move this into a separate function to make reading the code easier?

Also, if I understood correctly, we do this so that we could set an older delta in the account, right? If so, this seems like a lot of work for a relatively simple update. Another option could be to enable nonce updates in minde-base. But also, maybe we could just store some amount of historical data for each account to simplify this process.

One example, is that we could store account header + storage headers for all public accounts for the last $n$ blocks (where $n$ is the "look-back period" for which we allow users to query older states).

We could also move code into a separate table where code_commitment is the primary key. Given that code doesn't change too frequently (and there is probably going to be a lot of duplication) we probably don't sacrifice much by doing so.

If we do the above, the main problem we'd need to do is how to update storage maps and asset vault to to some past state - and this actually means that we won't need to deal with full account deltas but only with storage map and asset vault deltas. This, I think, should simplify the task quite a bit.

polydez · 2025-01-27T17:34:36Z

@bobbinth thank you so much for in-deep review! Yes, this idea might help to make things simpler as well, as the new solution might work faster.

Overall, before knowing if the above approach makes sense, we'll need to sketch out how would it affect the existing endpoints. I think these endpoints are:

SyncState - maybe we should return account header for all public accounts?

GetAccountStateDelta - would we still need this endpoint?

GetAccountDetails - should we change this to be about getting vault and storage maps (either via deltas or pagination)?

GetAccountProofs - should we modify the map responses to send just key-value pairs if the maps are small enough? Or maybe we should transform this into a more general GetAccountHeader endpoint which could also optionally return account proofs?

It's unclear for me, which endpoints work good for consumers and which ones should be improved or removed. I was wondering, if @igamigo could give us more context regarding these endpoints and how useful (or not) they are from a client's perspective and what we should improve.

igamigo · 2025-01-27T19:38:28Z

It's unclear for me, which endpoints work good for consumers and which ones should be improved or removed. I was wondering, if @igamigo could give us more context regarding these endpoints and how useful (or not) they are from a client's perspective and what we should improve.

I think in general this is still the case. In terms of whether the endpoints that Bobbin mentions are useful or not, we use all of them except GetAccountStateDelta, as mentioned in the linked comment. As for the others, all of them work fine right now, except for the limitation placed on GetAccountProofs since we cannot get past states.

The way syncing works right now on the client is that we get the response, which includes the AccountSummary for any account. If the account is public, we end up using the GetAccountDetails endpoint. This is probably not future proof because public accounts can obviously get pretty big. I think we always care about the latest state of the public account here, but Bobbin suggested refactoring this to allow for non-current states. I don't think this is necessary now from the client's standpoint, but also he might have some other use case in mind.

So, in terms of what we don't have now but would be nice to have, I guess it boils down to allowing for non-current states for GetAccountProofs and also having a way to (incrementally) get a public account's state because we do want to keep public accounts in sync as well (though maybe this process should be reworked entirely). If any of this is not clear enough feel free to let me know.

polydez added 20 commits November 26, 2024 09:54

feat: prepare DB structure for new delta format

25a82ff

chore: remove unused dependencies

ea8898c

fix: delta merging bugs

5c0f4d8

fix: remove obsolete check in test

209ee7b

fix: update directory name to correspond common format

97389f2

feat: compute old account states by deltas in DB

1dfa146

refactor: rename DB tables

419f871

refactor: add WITHOUT ROWID to tables where ROWID is unnecessary

3b10b3c

refactor: don't require allocations and clones for bulk_insert

4b5cfd8

fix: get rid of unsafe in NonFungibleAsset construction from Word

8552b12

Merge branch 'refs/heads/next' into polydez-refactor-deltas-in-db

560d00d

# Conflicts: # crates/store/Cargo.toml

refactor: extract utils to separate module inside sql

9b83153

refactor: rename account_nonce_updates table to account_deltas

8945b7f

docs: improve function description for select_account_state_delta

9ea1f17

Merge branch 'next' into polydez-refactor-deltas-in-db

45ce1b6

# Conflicts: # Cargo.lock # Cargo.toml # crates/block-producer/Cargo.toml # crates/store/Cargo.toml # crates/store/src/db/tests.rs

fix: compilation errors

98fc38d

Merge branch 'polydez-refactor-deltas-in-db' into polydez-compute-old…

c153a52

…-acc-states # Conflicts: # crates/store/src/db/sql/mod.rs # crates/store/src/db/sql/queries/compute_old_account_states.sql

fix: table names

3203526

fix: get rid of unsafe

8099914

tests: implement test, fix bugs

9a54663

polydez added the no changelog label Dec 4, 2024

polydez requested review from bobbinth and Mirko-von-Leipzig December 4, 2024 12:55

polydez added 7 commits December 4, 2024 18:00

refactor: rename account_hash_update_from_row to `account_summary_f…

aa6d939

…rom_row`

refactor: rename account_hash_update_from_row to `account_summary_f…

bb22838

…rom_row`

refactor: get rid of u32_to_value

8fd5bb1

refactor: remove unnecessary indexes

42267e5

refactor: multiple inserts instead of bulk_insert

431eb5a

refactor: multiple queries instead of single query with unions

dbffa9a

Merge branch 'polydez-refactor-deltas-in-db' into polydez-compute-old…

a4f4ed3

…-acc-states # Conflicts: # crates/store/src/db/sql/queries/select_merged_deltas.sql

polydez added 2 commits December 6, 2024 10:50

docs: fix example test for insert_sql macro

064aaf3

formatting: apply rustfmt

8b5cacb

Mirko-von-Leipzig approved these changes Dec 6, 2024

View reviewed changes

Mirko-von-Leipzig mentioned this pull request Dec 9, 2024

chore: remove test_ prefix stutter #568

Merged

refactor: address review comments

c2e387e

igamigo reviewed Dec 9, 2024

View reviewed changes

igamigo mentioned this pull request Dec 9, 2024

Enable getting a proof for non-current account states #527

Open

polydez added 7 commits December 26, 2024 14:32

Merge branch 'refs/heads/next' into polydez-compute-old-acc-states

9e15f5b

# Conflicts: # crates/proto/src/generated/account.rs # crates/rpc-proto/proto/account.proto # crates/store/src/db/sql/mod.rs # crates/store/src/db/tests.rs # proto/account.proto

refactor: update to the latest next

76c68b4

refactor: slightly improve SQL queries

0dba96e

refactor: apply assets in backward order

44390d4

refactor: apply storage in backward order, get rid of AccountRecord

69c744b

refactor: improve code

2753588

refactor: address review comments

b36a0fd

polydez requested review from Mirko-von-Leipzig and igamigo January 10, 2025 11:37

docs: fix

b6c33b3

Mirko-von-Leipzig reviewed Jan 10, 2025

View reviewed changes

Update crates/store/src/db/sql/mod.rs

ac28428

Co-authored-by: Mirko <[email protected]>

refactor: address review comments

7f809f4

Mirko-von-Leipzig reviewed Jan 13, 2025

View reviewed changes

crates/store/src/db/sql/mod.rs Show resolved Hide resolved

crates/store/src/db/sql/mod.rs Outdated Show resolved Hide resolved

refactor: use transaction for atomicity

275b028

igamigo reviewed Jan 15, 2025

View reviewed changes

refactor: address review comments

577415e

bobbinth reviewed Jan 17, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implemented function for computing of old account states based on deltas #563

Implemented function for computing of old account states based on deltas #563

polydez commented Dec 4, 2024 •

edited

Loading

Mirko-von-Leipzig left a comment

bobbinth commented Dec 7, 2024

igamigo Dec 9, 2024 •

edited

Loading

igamigo left a comment

igamigo Dec 9, 2024

igamigo Dec 9, 2024

Mirko-von-Leipzig Jan 10, 2025 •

edited

Loading

Mirko-von-Leipzig Jan 10, 2025

polydez Jan 10, 2025

polydez commented Jan 10, 2025

igamigo Jan 15, 2025

igamigo Jan 15, 2025

igamigo Jan 15, 2025

bobbinth left a comment

bobbinth Jan 17, 2025

bobbinth Jan 17, 2025

polydez commented Jan 27, 2025 •

edited

Loading

igamigo commented Jan 27, 2025

	/// - `account_ids`: Include the account's hash if their _last change_ was in the result's block
	/// range.

Implemented function for computing of old account states based on deltas #563

Are you sure you want to change the base?

Implemented function for computing of old account states based on deltas #563

Conversation

polydez commented Dec 4, 2024 • edited Loading

Mirko-von-Leipzig left a comment

Choose a reason for hiding this comment

bobbinth commented Dec 7, 2024

igamigo Dec 9, 2024 • edited Loading

Choose a reason for hiding this comment

igamigo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mirko-von-Leipzig Jan 10, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

polydez commented Jan 10, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bobbinth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

polydez commented Jan 27, 2025 • edited Loading

igamigo commented Jan 27, 2025

polydez commented Dec 4, 2024 •

edited

Loading

igamigo Dec 9, 2024 •

edited

Loading

Mirko-von-Leipzig Jan 10, 2025 •

edited

Loading

polydez commented Jan 27, 2025 •

edited

Loading