Use `AccountTree` type wrapper by PhilippGackstatter · Pull Request #783 · 0xMiden/node

PhilippGackstatter · 2025-04-04T00:57:09Z

Companion PR to 0xMiden/protocol#1254.

Mirko-von-Leipzig

Some very minor suggestions, and missing a changelog entry, but otherwise this API looks much cleaner!

proto/proto/responses.proto

crates/proto/src/domain/block.rs

crates/block-producer/src/test_utils/block.rs

PhilippGackstatter · 2025-04-04T15:03:44Z

crates/block-producer/src/test_utils/store.rs

-                .insert(update.account_id().into(), update.final_state_commitment().into());
+                .insert(update.account_id(), update.final_state_commitment())
+                .expect("TODO: what should we do with this error?");


@Mirko-von-Leipzig What should we do with this one? This is test code, so panicking might be fine? Otherwise I'll have to add a new variant to StoreError that is only used in tests.

panic is fine 👍

igamigo · 2025-04-04T19:20:55Z

After this comment, I wanted to run some tests pointing the client to this branch (and the corresponding branch from miden-base) and found that the node logs some errors before any external interaction:

2025-04-04T19:17:32.120194Z  INFO store.rpc/ApplyBlock:store.server.apply_block:apply_block:update_in_memory_structs: miden-store: crates/store/src/state.rs:285: new rpc.service: "store.rpc", rpc.method: "ApplyBlock"
2025-04-04T19:17:32.121424Z  INFO store.rpc/ApplyBlock:store.server.apply_block:apply_block:update_in_memory_structs: miden-store: crates/store/src/state.rs:285: close, time.busy: 1.20ms, time.idle: 24.8µs rpc.service: "store.rpc", rpc.method: "ApplyBlock"
2025-04-04T19:17:32.121457Z ERROR store.rpc/ApplyBlock:store.server.apply_block:apply_block: miden-store: crates/store/src/state.rs:231: error: invalid block error rpc.service: "store.rpc", rpc.method: "ApplyBlock"
2025-04-04T19:17:32.121488Z  INFO store.rpc/ApplyBlock:store.server.apply_block:apply_block: miden-store: crates/store/src/state.rs:231: close, time.busy: 1.46ms, time.idle: 956µs rpc.service: "store.rpc", rpc.method: "ApplyBlock"
2025-04-04T19:17:32.121517Z ERROR store.rpc/ApplyBlock:store.server.apply_block: miden-store: crates/store/src/server/api.rs:297: error: status: InvalidArgument, message: "invalid block error", details: [], metadata: MetadataMap { headers: {} } rpc.service: "store.rpc", rpc.method: "ApplyBlock"
2025-04-04T19:17:32.121553Z  INFO store.rpc/ApplyBlock:store.server.apply_block: miden-store: crates/store/src/server/api.rs:297: close, time.busy: 2.38ms, time.idle: 955µs rpc.service: "store.rpc", rpc.method: "ApplyBlock"
2025-04-04T19:17:32.121612Z ERROR store.rpc/ApplyBlock: tower_http::trace::on_failure: /Users/igamigo/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/tower-http-0.6.2/src/trace/on_failure.rs:93: response failed, classification: Code: 3, latency: 3 ms rpc.service: "store.rpc", rpc.method: "ApplyBlock"
2025-04-04T19:17:32.121685Z  INFO store.rpc/ApplyBlock: miden_node_utils::tracing::grpc: crates/utils/src/tracing/grpc.rs:36: close, time.busy: 2.64ms, time.idle: 1.03ms rpc.service: "store.rpc", rpc.method: "ApplyBlock"
2025-04-04T19:17:32.122029Z ERROR block_builder.build_block:block_builder.commit_block:store.client.apply_block: miden-block-producer: crates/block-producer/src/store/mod.rs:232: error: gRPC client error
2025-04-04T19:17:32.122088Z  INFO block_builder.build_block:block_builder.commit_block:store.client.apply_block: miden-block-producer: crates/block-producer/src/store/mod.rs:232: close, time.busy: 286µs, time.idle: 4.74ms
2025-04-04T19:17:32.122123Z ERROR block_builder.build_block:block_builder.commit_block: miden-block-producer: crates/block-producer/src/block_builder/mod.rs:214: error: failed to apply block to store
2025-04-04T19:17:32.122152Z  INFO block_builder.build_block:block_builder.commit_block: miden-block-producer: crates/block-producer/src/block_builder/mod.rs:214: close, time.busy: 350µs, time.idle: 4.77ms

Haven't looked too much into it yet, but running the node on next does not seem to reproduce this

crates/proto/src/generated/responses.rs

PhilippGackstatter · 2025-04-15T16:39:02Z

After this comment, I wanted to run some tests pointing the client to this branch

The issue was that I forgot to update SimpleSmt usage in genesis building. I changed it, and now the node seems to startup and run correctly.

I'll try to run the client integration tests tomorrow to see if any problems occur.

PhilippGackstatter · 2025-04-17T18:21:46Z

I ran the client integration tests, pointing the dependencies at the corresponding branch in miden-base, but I think due to the changes in the gRPC definitions a few tests are failing. So I think this PR also needs a companion PR in miden-client, cc @igamigo.

igamigo · 2025-04-17T18:35:52Z

Yeah, changes shouldn't be too bad, I'll work on a PR soon.

bobbinth

Looks good! Thank you!

Since 0xMiden/protocol#1254 we can now update the Cargo.toml references and merges this PR. @Mirko-von-Leipzig could you help with this? (@PhilippGackstatter is out today).

After this, I believe @igamigo will create a PR in miden-client to make sure integration tests are working against the latest next here.

igamigo · 2025-04-18T12:42:47Z

crates/block-producer/src/block_builder/mod.rs

+        let num_block_created_notes = self.batches().num_created_notes();
        span.set_attribute(
            "block.output_notes.count",
            u32::try_from(num_block_created_notes)
                .expect("should have less than u32::MAX output notes"),
        );

-        let num_batch_created_notes = self
-            .batches()
-            .as_slice()
-            .iter()
-            .fold(0, |acc, batch| acc + batch.output_notes().len());
+        let num_batch_created_notes = self.batches().num_created_notes();


Is this right? Seems like there is no difference between num_block_created_notes and num_batch_created_notes and so num_erased_notes will always be 0

Ah yeah nice catch. It looks like the block variant is incorrect now.

ProposedBlock::output_note_batches should be used there it seems, though the naming is a bit confusing :)

I'll create an issue.

Nice catch!

igamigo · 2025-04-18T15:44:03Z

FYI integration tests work on the client: 0xMiden/miden-client#859

PhilippGackstatter added 2 commits April 3, 2025 16:12

feat: Use AccountTree

8ef31ad

chore: Adapt to miden-base updates

27e3d43

PhilippGackstatter requested a review from Mirko-von-Leipzig April 4, 2025 00:57

Mirko-von-Leipzig added no changelog This PR does not require an entry in the `CHANGELOG.md` file and removed no changelog This PR does not require an entry in the `CHANGELOG.md` file labels Apr 4, 2025

Mirko-von-Leipzig approved these changes Apr 4, 2025

View reviewed changes

proto/proto/responses.proto Show resolved Hide resolved

crates/proto/src/domain/block.rs Outdated Show resolved Hide resolved

crates/block-producer/src/test_utils/block.rs Outdated Show resolved Hide resolved

crates/block-producer/src/test_utils/block.rs Outdated Show resolved Hide resolved

PhilippGackstatter commented Apr 4, 2025

View reviewed changes

PhilippGackstatter added 3 commits April 4, 2025 08:05

chore: Add changelog

24c7814

chore: Do not panic in account witness deserialization

4d9bb72

chore: Use new ordered batches/tx apis

4464e69

PhilippGackstatter mentioned this pull request Apr 4, 2025

Update to use TransactionHeader #781

Merged

igamigo reviewed Apr 4, 2025

View reviewed changes

crates/proto/src/generated/responses.rs Outdated Show resolved Hide resolved

PhilippGackstatter mentioned this pull request Apr 4, 2025

Add AccountTree and PartialAccountTree 0xMiden/protocol#1254

Merged

PhilippGackstatter added 3 commits April 15, 2025 18:24

feat: Use AccountTree in genesis, update account witness

9af948d

chore: Bump miden-base commit

231a96c

fix: Clippy lint

ef273e4

chore: Bump miden-base commit

cf56adb

PhilippGackstatter requested a review from bobbinth April 15, 2025 17:03

PhilippGackstatter added 3 commits April 16, 2025 18:32

chore: Bump miden-base commit

4fd6dbd

chore: Bump miden-base commit

060969f

Merge remote-tracking branch 'origin/next' into pgackst-account-tree

0b781bb

bobbinth approved these changes Apr 18, 2025

View reviewed changes

Use next@base

bdd89d7

Mirko-von-Leipzig merged commit a864933 into next Apr 18, 2025
6 checks passed

Mirko-von-Leipzig deleted the pgackst-account-tree branch April 18, 2025 08:59

igamigo reviewed Apr 18, 2025

View reviewed changes

Mirko-von-Leipzig mentioned this pull request Apr 18, 2025

OTel stats are incorrect for block output notes #793

Closed

SantiagoPittella pushed a commit that referenced this pull request May 16, 2025

feat: use AccountTree type wrapper (#783)

c14a650

Conversation

PhilippGackstatter commented Apr 4, 2025

Uh oh!

Mirko-von-Leipzig left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

PhilippGackstatter Apr 4, 2025

Choose a reason for hiding this comment

Uh oh!

Mirko-von-Leipzig Apr 5, 2025

Choose a reason for hiding this comment

Uh oh!

igamigo commented Apr 4, 2025

Uh oh!

Uh oh!

PhilippGackstatter commented Apr 15, 2025

Uh oh!

PhilippGackstatter commented Apr 17, 2025

Uh oh!

igamigo commented Apr 17, 2025

Uh oh!

bobbinth left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

igamigo Apr 18, 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!

Mirko-von-Leipzig Apr 18, 2025

Choose a reason for hiding this comment

Uh oh!

bobbinth Apr 18, 2025

Choose a reason for hiding this comment

Uh oh!

igamigo commented Apr 18, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

igamigo Apr 18, 2025 •

edited

Loading