Implemented writing on-chain accounts details on block applying #294

polydez · 2024-03-30T12:05:58Z

This is the last PR of this series. Here is details of on-chain accounts is written to database.

…num`

… conversions

# Conflicts: # .gitignore # Cargo.toml

hackaugusto

partial review, since this is built in another pr.

hackaugusto · 2024-04-03T16:16:25Z

block-producer/src/batch_builder/batch.rs

-            .map(|(account_id, account_states)| (*account_id, account_states.final_state))
+    pub fn updated_accounts(
+        &self
+    ) -> impl Iterator<Item = (AccountId, Option<AccountDetails>, Digest)> + '_ {


Can you add a struct type for the return tuple? It should make the code a bit easier to read.

hackaugusto · 2024-04-03T16:19:35Z

block-producer/src/state_view/mod.rs

+            block.updated_accounts.iter().map(|(account_id, _, _)| *account_id).collect();
+        let produced_nullifiers = block.produced_nullifiers.clone();
+
+        self.store.apply_block(block).await?;


I assume that store.apply_block encodes the Block into a protobuf message. Meaning it doesn't need ownership of Block. If that is the case, I think we should change it to receive a reference, and then here we don't need the copies.

@hackaugusto we can't do it simple way because the block needs be disassembled inside apply_block:

miden-node/block-producer/src/store/mod.rs

Lines 137 to 140 in 8f0935a

block: Some(block.header.into()),

accounts: convert(block.updated_accounts),

nullifiers: convert(block.produced_nullifiers),

notes: convert(block.created_notes),

The linked code is creating new objects. At first sight it seems none of them require ownership.

Done, thanks for noticing!

proto/proto/responses.proto

hackaugusto · 2024-04-03T16:25:16Z

store/src/db/sql.rs

+        if let Some(details) = details {
+            match details {
+                AccountDetails::Full(account) => {


nit: this could be just:

match details { Some(AccountDetails::Full(account)) => { ... }, Some(AccountDetails::Delta(delta)) => { ... }, None => (), }

Not a huge improvement, but this makes the indentation shallower which IMO is a bit easier to read.

store/src/db/sql.rs

hackaugusto · 2024-04-03T16:32:28Z

store/src/server/api.rs

@@ -190,11 +221,15 @@ impl api_server::Api for StoreApi {
            .accounts
            .into_iter()
            .map(|account_update| {
+                let account_details: Option<AccountDetails> =
+                    <Option<AccountDetails>>::read_from_bytes(&account_update.details)
+                        .map_err(|err| Status::invalid_argument(err.to_string()))?;


This should have validation:

if the account is on-chain, there must be details

if the account is off-chain, there must NOT be any details

# Conflicts: # Cargo.lock # block-producer/src/block_builder/prover/tests.rs # block-producer/src/block_builder/tests.rs # block-producer/src/test_utils/account.rs # store/src/db/tests.rs

chore: update `miden-base` dependency to fixed version refactor: extract `apply_delta` function fix: separate error invariant for missed details in store fix: make account details optional refactor: introduce `UpdatedAccount` struct fix: avoid cloning of block data feat: simple details validation in store feat: rewrite `sql::upsert_accounts` to simplified work with details and update test fix: compilation errors feat: use serialized account details feat: writing account details on block applying

# Conflicts: # Cargo.lock # proto/src/domain/accounts.rs # store/src/db/mod.rs # store/src/db/sql.rs # store/src/db/tests.rs # store/src/errors.rs # store/src/server/api.rs # store/src/state.rs

bobbinth

Thank you! Looks good! I left some comments inline.

The main open question is how to verify that the account details included with a public account are valid. The most problematic part of this is to verify account delta for existing accounts. Ideally, this should be happening in the RPC so that a proven transaction with malformed account delta would not make it into the block producer. But I'm not seeing an efficient way of doing this. So, the current approach is fine, but let's create an issue to figure out how to validate account delta more holistically.

Cargo.toml

store/Cargo.toml

proto/src/domain/accounts.rs

store/src/server/api.rs

store/src/db/mod.rs

store/src/db/sql.rs

# Conflicts: # rpc/README.md # store/README.md # store/src/db/mod.rs # store/src/db/sql.rs # store/src/db/tests.rs # store/src/server/api.rs # store/src/state.rs

bobbinth

Looks good! Thank you! I left a couple of very small nits inline. After these, we can merge.

bobbinth · 2024-04-09T09:11:48Z

rpc/README.md

@@ -111,7 +110,7 @@ contains excessive notes and nullifiers, client can make additional filtering of
 - `chain_tip`: `uint32` – number of the latest block in the chain.
 - `block_header`: `BlockHeader` – block header of the block with the first note matching the specified criteria.
 - `mmr_delta`: `MmrDelta` – data needed to update the partial MMR from `block_num + 1` to `block_header.block_num`.
- `accounts`: `[AccountHashUpdate]` – a list of account hashes updated after `block_num + 1` but not after `block_header.block_num`.
+- `accounts`: `[AccountSummary]` – a list of account hashes updated after `block_num + 1` but not after `block_header.block_num`.


nit: I would probably change the description to something like: "account summaries for accounts updated after ...".

bobbinth · 2024-04-09T09:12:04Z

store/README.md

@@ -157,7 +156,7 @@ contains excessive notes and nullifiers, client can make additional filtering of
 - `chain_tip`: `uint32` – number of the latest block in the chain.
 - `block_header`: `BlockHeader` – block header of the block with the first note matching the specified criteria.
 - `mmr_delta`: `MmrDelta` – data needed to update the partial MMR from `block_num + 1` to `block_header.block_num`.
- `accounts`: `[AccountHashUpdate]` – a list of account hashes updated after `block_num + 1` but not after `block_header.block_num`.
+- `accounts`: `[AccountSummary]` – a list of account hashes updated after `block_num + 1` but not after `block_header.block_num`.


Same nit as above.

* feat: add `account_details` table to the DB * refactor: rename `block_number` column in nullifiers table to `block_num` * refactor: use `BETWEEN` in interval comparison checks * feat: implement account details protobuf messages, domain objects and conversions * feat: (WIP) implement account details support * feat: (WIP) implement account details support * feat: (WIP) implement account details support * feat: (WIP) implement account details support * fix: db creation * docs: remove TODO * refactor: apply formatting * feat: implement endpoint for getting public account details * tests: add test for storage * feat: add rpc endpoint for getting account details * refactor: keep only domain object changes * fix: compilation errors * fix: use note tag conversion from `u64` * refactor: remove account details protobuf messages * fix: remove unused error invariants * refactor: introduce `UpdatedAccount` struct * fix: rollback details conversion * fix: compilation error * feat: account details in store * refactor: add constraint name for foreign key * refactor: small code improvement Co-authored-by: Augusto Hack <[email protected]> * feat: account id validation * refactor: rename `get_account_details` to `select_*` * feat: return serialized account details * feat: add requirement of account id to be public in RPC * fix: remove error message used in different PR * fix: union account details with account and process them together * docs: remove `GetAccountDetails` from README.md * fix: remove unused error invariants * fix: use `Account` instead of `AccountDetails` in store * wip * feat: implement `GetAccountDetails` endpoint * docs: document `GetAccountDetails` endpoint * refactor: simplify code, make account details optional * fix: clippy warning * fix: address review comments * fix: update code to the latest miden-base * refactor: little code improvement * fix: remove error message used in different PR * fix: compilation errors chore: update `miden-base` dependency to fixed version refactor: extract `apply_delta` function fix: separate error invariant for missed details in store fix: make account details optional refactor: introduce `UpdatedAccount` struct fix: avoid cloning of block data feat: simple details validation in store feat: rewrite `sql::upsert_accounts` to simplified work with details and update test fix: compilation errors feat: use serialized account details feat: writing account details on block applying * fix: compilation errors, update test * refactor: rename protobuf messages * docs: update endpoint in README.md * tests: get rid of miden-mock dependency * tests: get rid of winter-rand-utils dependency * refactor: rename `AccountDetailsUpdate` to `AccountUpdateDetails` * feat: check for account hash for new on-chain accounts * formatting: run rustfmt * docs: address review comments --------- Co-authored-by: Augusto Hack <[email protected]>

polydez added 18 commits March 25, 2024 18:23

feat: add account_details table to the DB

b6c529f

refactor: rename block_number column in nullifiers table to `block_…

ed04d5a

…num`

refactor: use BETWEEN in interval comparison checks

4cc4ba8

feat: implement account details protobuf messages, domain objects and…

8f0935a

… conversions

feat: (WIP) implement account details support

9f5c832

feat: (WIP) implement account details support

58dc518

feat: (WIP) implement account details support

ef7a1f7

feat: (WIP) implement account details support

bb31753

fix: db creation

ac81502

docs: remove TODO

ab9c457

refactor: apply formatting

31cfd19

feat: implement endpoint for getting public account details

6560444

Merge branch 'next' into polydez-onchain-accounts

bd7ac74

tests: add test for storage

be2be66

feat: add rpc endpoint for getting account details

fbcfd6b

refactor: keep only domain object changes

28d44fb

Merge branch 'next' into polydez-onchain-accounts

d344c57

# Conflicts: # .gitignore # Cargo.toml

fix: compilation errors

618e24e

polydez requested review from hackaugusto and bobbinth March 30, 2024 12:05

polydez marked this pull request as ready for review March 30, 2024 12:06

polydez added 2 commits April 1, 2024 20:49

fix: use note tag conversion from u64

bd47398

refactor: remove account details protobuf messages

42b0c22

polydez force-pushed the polydez-onchain-accounts-apply branch from 06d453a to 63be641 Compare April 3, 2024 12:01

fix: remove unused error invariants

bb6e18e

polydez force-pushed the polydez-onchain-accounts-apply branch 2 times, most recently from 1d6d3a4 to c0110c7 Compare April 3, 2024 15:17

hackaugusto reviewed Apr 3, 2024

View reviewed changes

bobbinth changed the base branch from polydez-onchain-accounts to polydez-onchain-accounts-store April 4, 2024 00:51

bobbinth changed the base branch from polydez-onchain-accounts-store to polydez-onchain-accounts April 4, 2024 00:51

polydez force-pushed the polydez-onchain-accounts-apply branch from aa86a10 to 324bca3 Compare April 5, 2024 17:32

polydez added 4 commits April 6, 2024 11:30

fix: address review comments

da0deed

fix: update code to the latest miden-base

fb7d577

Merge branch 'next' into polydez-onchain-accounts-store

1e3e713

# Conflicts: # Cargo.lock # block-producer/src/block_builder/prover/tests.rs # block-producer/src/block_builder/tests.rs # block-producer/src/test_utils/account.rs # store/src/db/tests.rs

refactor: little code improvement

955be92

polydez force-pushed the polydez-onchain-accounts-apply branch from 324bca3 to d338bda Compare April 6, 2024 07:34

fix: remove error message used in different PR

dc2a8bc

polydez force-pushed the polydez-onchain-accounts-apply branch from d338bda to 7f09974 Compare April 6, 2024 07:44

Base automatically changed from polydez-onchain-accounts-store to next April 6, 2024 07:46

polydez added 3 commits April 6, 2024 12:47

Merge branch 'next' into polydez-onchain-accounts-apply

22c89bb

# Conflicts: # Cargo.lock # proto/src/domain/accounts.rs # store/src/db/mod.rs # store/src/db/sql.rs # store/src/db/tests.rs # store/src/errors.rs # store/src/server/api.rs # store/src/state.rs

fix: compilation errors, update test

328042b

polydez force-pushed the polydez-onchain-accounts-apply branch from 7f09974 to 328042b Compare April 6, 2024 08:12

bobbinth requested changes Apr 6, 2024

View reviewed changes

polydez added 6 commits April 8, 2024 11:15

refactor: rename protobuf messages

65caccc

docs: update endpoint in README.md

6b5c313

tests: get rid of miden-mock dependency

895b982

tests: get rid of winter-rand-utils dependency

6aa4819

refactor: rename AccountDetailsUpdate to AccountUpdateDetails

d69af1d

feat: check for account hash for new on-chain accounts

bd4355c

polydez requested a review from bobbinth April 8, 2024 06:53

polydez linked an issue Apr 8, 2024 that may be closed by this pull request

Implement support for on-chain accounts #142

Closed

polydez added 2 commits April 9, 2024 11:12

Merge branch 'next' into polydez-onchain-accounts-apply

4af5946

# Conflicts: # rpc/README.md # store/README.md # store/src/db/mod.rs # store/src/db/sql.rs # store/src/db/tests.rs # store/src/server/api.rs # store/src/state.rs

formatting: run rustfmt

652905a

bobbinth approved these changes Apr 9, 2024

View reviewed changes

docs: address review comments

609aaef

polydez merged commit f5a7bd9 into next Apr 9, 2024
5 checks passed

polydez deleted the polydez-onchain-accounts-apply branch April 9, 2024 09:37

mFragaBA mentioned this pull request Apr 11, 2024

feat: Add account details endpoint 0xPolygonMiden/miden-client#248

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implemented writing on-chain accounts details on block applying #294

Implemented writing on-chain accounts details on block applying #294

polydez commented Mar 30, 2024 •

edited

Loading

hackaugusto left a comment

hackaugusto Apr 3, 2024

hackaugusto Apr 3, 2024

polydez Apr 4, 2024

hackaugusto Apr 4, 2024

polydez Apr 5, 2024

hackaugusto Apr 3, 2024

hackaugusto Apr 3, 2024

bobbinth left a comment

bobbinth left a comment

bobbinth Apr 9, 2024

bobbinth Apr 9, 2024

	block: Some(block.header.into()),
	accounts: convert(block.updated_accounts),
	nullifiers: convert(block.produced_nullifiers),
	notes: convert(block.created_notes),

Implemented writing on-chain accounts details on block applying #294

Implemented writing on-chain accounts details on block applying #294

Conversation

polydez commented Mar 30, 2024 • edited Loading

hackaugusto 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

bobbinth left a 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 Mar 30, 2024 •

edited

Loading