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. 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.

# 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

# Conflicts: # crates/store/src/db/sql/mod.rs # crates/store/src/db/sql/utils.rs # crates/store/src/db/tests.rs

Mirko-von-Leipzig

Mostly just style/nit suggestions :) LGTM.

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

Mirko-von-Leipzig · 2024-12-06T08:55:27Z

crates/store/src/db/tests.rs

@@ -471,6 +474,121 @@ fn test_sql_public_account_details() {
    assert_eq!(read_delta, Some(delta2));
 }

+#[test]
+fn test_sql_public_account_details_for_old_block() {


This is more a meta question (probably for @bobbinth) - why do we prefix tests with test?

They're already in a mod tests scope and have #[test]. Sort of feels like Hungarian notation and just stutters when running tests:

tests::test_xxx tests::test_xxy tests::test_xxz

There is no good reason for this, and we are not always being consistent here (most of the time we do use test_ prefix but not always).

I'm totally fine with removing test_ prefixes if someone wants to rename them.

Mirko-von-Leipzig · 2024-12-06T08:57:21Z

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

-/// Select the latest account details by account id from the DB using the given [Connection].
+/// Select the latest account details by account ID from the DB using the given [Connection].


Should we introduce a comment spell-checker into CI? This would also negate the random air-drop-farming PRs we'll get in the future (they love submitting doc fixes).

I have no previous experience with spell-checking on CI. This should work, if it's easy to add new words to a dictionary. And if it won't require disabling it for particular comments.

Me neither; this looks promising? But not really important at this stage :)

I'd agree with this as well (although I don't have prior experience to gauge how effective those tools can be). There have already been quite a few of those types of PRs.

Mirko-von-Leipzig · 2024-12-06T09:01:23Z

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

+    account_ids: &[AccountId],
+    block_number: BlockNumber,
+) -> Result<Vec<Account>> {
+    fn find_account(


Short doc comment here would be good if you keep it.

Mirko-von-Leipzig · 2024-12-06T09:05:50Z

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

+        match find_account(&mut accounts, account_id)?.get_mut().storage.entry(slot) {
+            Entry::Vacant(entry) => {
+                entry.insert(StorageSlot::Value(value));
+            },
+            Entry::Occupied(_) => {
+                return Err(DatabaseError::DataCorrupted(format!(
+                    "Duplicate storage slot: {slot}"
+                )));
+            },
+        }


Could use if <..>.insert(..).is_some() { return Err(..) }. Unclear if its more legible though.

Suggested change

match find_account(&mut accounts, account_id)?.get_mut().storage.entry(slot) {

Entry::Vacant(entry) => {

entry.insert(StorageSlot::Value(value));

},

Entry::Occupied(_) => {

return Err(DatabaseError::DataCorrupted(format!(

"Duplicate storage slot: {slot}"

)));

},

}

let mut account = find_account(&mut accounts, account_id)?;

if account.get_mut().storage.insert(slot, value).is_some() {

return Err(DatabaseError::DataCorrupted(format!(

"Duplicate storage slot: {slot}"

)));

}

Mirko-von-Leipzig · 2024-12-06T09:10:32Z

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

+        find_account(&mut accounts, account_id)?.get_mut().nonce =
+            Some(nonce.try_into().map_err(DatabaseError::DataCorrupted)?);


nit: multi-line chaining from a stand-alone function is sometimes hard to read/grok:

Suggested change

find_account(&mut accounts, account_id)?.get_mut().nonce =

Some(nonce.try_into().map_err(DatabaseError::DataCorrupted)?);

let nonce = row.get::<_, u64>(1)?.try_into().map_err(DatabaseError::DataCorrupted)?;

let mut account = find_account(&mut accounts, account_id)?;

account.get_mut().nonce = Some(nonce);

Mirko-von-Leipzig · 2024-12-06T09:17:18Z

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

+    struct AccountRecord {
+        code: AccountCode,
+        nonce: Option<Felt>,
+        storage: BTreeMap<u8, StorageSlot>,
+        assets: Vec<Asset>,
+    }


Might be worth adding an impl to this to separate it from the record iteration?

impl AccountRecord { // We should start by reading the `nonce` then it doesn't have to be optional? fn new(id: AccountId, nonce: Nonce) -> Self {..} fn push_asset(&mut self, ...) ... }

Mirko-von-Leipzig · 2024-12-06T09:22:50Z

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

+    fn find_account(
+        accounts: &mut BTreeMap<AccountId, AccountRecord>,
+        account_id: AccountId,
+    ) -> Result<OccupiedEntry<AccountId, AccountRecord>> {
+        let Entry::Occupied(found_account) = accounts.entry(account_id) else {
+            return Err(DatabaseError::DataCorrupted(format!(
+                "Account not found in DB: {account_id:x}"
+            )));
+        };
+
+        Ok(found_account)
+    }


Maybe we should new-type accounts? Though I'm bias against free standing functions, so take this with a big pinch of 🧂

#[derive(Default)] struct Accounts(BTreeMap<AccountId, AccountRecord>); impl Accounts { fn insert(...) // or maybe `get_mut_or_err` fn try_get_mut(&mut self, id: &AccountId) -> Result<&mut AccountRecord> {...} }

Mirko-von-Leipzig · 2024-12-06T09:25:49Z

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

+            record.nonce.ok_or(DatabaseError::DataCorrupted(format!(
+                "Missing nonce for account: {account_id:x}"
+            )))?,


If we start by iterating over the nonces the they don't need to be optional.

No, we actually start by getting codes from accounts and crate AccountRecords with only code field filled.

Mirko-von-Leipzig · 2024-12-06T09:26:51Z

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

+                    return Err(DatabaseError::DataCorrupted(format!(
+                        "Missing value for storage slot {expected_slot}, got {slot}"
+                    )));


Is there value in expanding DataCorrupted with variants for each possible corruption? Probably not 🤔

No, I don't think we will process any data corruption cases differently from any other. 😄

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;

igamigo · 2024-12-09T16:06:13Z

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

+pub fn compute_old_account_states(
+    conn: &mut Connection,
+    account_ids: &[AccountId],
+    block_number: BlockNumber,
+) -> Result<Vec<Account>> {


Should this check that account IDs are public only?

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

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

refactor: multiple queries instead of single query with unions

ab3a196

polydez mentioned this pull request Dec 5, 2024

WIP: Enable getting proofs for old account states #553

Closed

6 tasks

polydez added 3 commits December 5, 2024 18:16

Merge branch 'next' into polydez-compute-old-acc-states

f075699

# Conflicts: # crates/store/src/db/sql/mod.rs # crates/store/src/db/sql/utils.rs # crates/store/src/db/tests.rs

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 2 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

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

Mirko-von-Leipzig left a comment

Mirko-von-Leipzig Dec 6, 2024

bobbinth Dec 7, 2024

Mirko-von-Leipzig Dec 6, 2024

polydez Dec 6, 2024

Mirko-von-Leipzig Dec 6, 2024

igamigo Dec 9, 2024 •

edited

Loading

Mirko-von-Leipzig Dec 6, 2024 •

edited

Loading

Mirko-von-Leipzig Dec 6, 2024

Mirko-von-Leipzig Dec 6, 2024

Mirko-von-Leipzig Dec 6, 2024 •

edited

Loading

Mirko-von-Leipzig Dec 6, 2024 •

edited

Loading

Mirko-von-Leipzig Dec 6, 2024

polydez Dec 9, 2024

Mirko-von-Leipzig Dec 6, 2024

polydez Dec 9, 2024 •

edited

Loading

bobbinth commented Dec 7, 2024

igamigo Dec 9, 2024 •

edited

Loading

igamigo left a comment

igamigo Dec 9, 2024

igamigo Dec 9, 2024

igamigo Dec 9, 2024

		/// Select the latest account details by account id from the DB using the given [Connection].
		/// Select the latest account details by account ID from the DB using the given [Connection].

		find_account(&mut accounts, account_id)?.get_mut().nonce =
		Some(nonce.try_into().map_err(DatabaseError::DataCorrupted)?);

-        find_account(&mut accounts, account_id)?.get_mut().nonce =
-            Some(nonce.try_into().map_err(DatabaseError::DataCorrupted)?);
+        let nonce = row.get::<_, u64>(1)?.try_into().map_err(DatabaseError::DataCorrupted)?;
+        let mut account = find_account(&mut accounts, account_id)?;
+        account.get_mut().nonce = Some(nonce);

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

Mirko-von-Leipzig 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

igamigo Dec 9, 2024 • edited Loading

Choose a reason for hiding this comment

Mirko-von-Leipzig 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

Mirko-von-Leipzig Dec 6, 2024 • edited Loading

Choose a reason for hiding this comment

Mirko-von-Leipzig 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

polydez Dec 9, 2024 • edited Loading

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

Choose a reason for hiding this comment

igamigo Dec 9, 2024 •

edited

Loading

Mirko-von-Leipzig Dec 6, 2024 •

edited

Loading

Mirko-von-Leipzig Dec 6, 2024 •

edited

Loading

Mirko-von-Leipzig Dec 6, 2024 •

edited

Loading

polydez Dec 9, 2024 •

edited

Loading

igamigo Dec 9, 2024 •

edited

Loading