Chore: update miden-protocol by juan518munoz · Pull Request #2095 · 0xMiden/node

juan518munoz · 2026-05-18T20:51:35Z

No description provided.

igamigo

Mostly looks good, leaving some comments

igamigo · 2026-05-20T18:59:26Z

+            AccountUpdateDetails::Delta(update) => {
+                // Partial updates carry no storage we can inspect here. Forward them as updates and
+                // let the coordinator's actor registry filter to known network accounts.
+                Some(NetworkAccountEffect::Updated(update.clone()))
+            },


Seems like, from an account delta, we can no longer know whether it belongs to a network account. Is this right @PhilippGackstatter? Then this will likely need some refactoring on the NTX builder side because I think we expect to have the account in the builder if we receive a delta. But I think some of this was being refactored anyway (cc @Mirko-von-Leipzig @SantiagoPittella)

Hmm, the block subscription approach will use partial accounts

Seems like, from an account delta, we can no longer know whether it belongs to a network account. Is this right @PhilippGackstatter?

Yes, sounds correct. Previously you could inspect the type on the ID, now you'd need to inspect storage, but the delta doesn't include full storage. So yes, you'd need to decide this at a different point.

Returning to the specific situation of the ntx builder, we can check in our internal storage if the account is a network account, considering that at this point it should already exist in the database because when it was a complete account (the first occurrence) we should have stored it.

igamigo · 2026-05-20T19:14:59Z

+            let response = self
+                .store
+                .clone()
+                .are_network_accounts(tonic::Request::new(proto::account::AccountIdList {
+                    account_ids: vec![tx.account_id().into()],
+                }))
+                .await
+                .map_err(|err| {
+                    Status::internal(format!("network-account classification failed: {err}"))
+                })?;


We probably want to cache new account IDs that belong to network accounts in memory as a future enhancement here

igamigo · 2026-05-20T19:20:34Z

+        .map_err(DatabaseError::from)
+}
+
+/// Returns the subset of `account_ids` whose latest committed state is a network account.


I think for now we can assume that network accounts will either be network accounts forever or not (because right now changing account code and adding/removing components is not supported).

# Conflicts: # crates/rpc/src/tests.rs

Mirko-von-Leipzig

Mostly looks good; I'd like to merge this ASAP but I'm concerned by our Felt::new_unchecked and Word::unchecked usage. Especially when combined with Rng.

This could be addressed in a follow-up though.

Also want to mention that the network account stuff in the ntx context no longer matters - we're taking a different approach where we sync by block instead so most of this will fall away.

Mirko-von-Leipzig · 2026-05-22T12:00:31Z

    let mut rng = ChaCha20Rng::from_seed(rand::random());
    let bridge_seed: [u64; 4] = rng.random();
-    let bridge_seed = Word::from(bridge_seed.map(Felt::new));
+    let bridge_seed = Word::from(bridge_seed.map(Felt::new_unchecked));


This seems.. unsafe? Does word not implement Rng.

cc @PhilippGackstatter

I think this is fine, since you'd just want some seed. We use the same pattern for initializing account seeds in protocol.

But, yeah, it seems like a good idea to implement random sampling for words or use a RandomCoin here, though it also needs to be seeded with a random word...

But then what is unchecked about it, if the constructor is valid? I assumed it would be invalid for some bit sequences that exceed the field modulus

Opened 0xMiden/crypto#1025 in crypto to make this easier.

But then what is unchecked about it, if the constructor is valid?

It's to make callers aware that it effectively truncates u64s that exceed the field modulus.

Mirko-von-Leipzig · 2026-05-22T12:10:48Z

+                // The actor registry only contains accounts the builder has already classified as
+                // network. Wrap the id unconditionally and let the registry lookup filter for us;
+                // unknown accounts simply won't match.
+                let network_account_id = NetworkAccountId::new_trusted(delta.id());


I would call it new_unchecked to align with our other methods.

Addressed in ec52fe5

Mirko-von-Leipzig · 2026-05-22T12:15:30Z

@kkovaacs might be good to TAL at the rocksdb stuff?

PhilippGackstatter

Looks good to me, overall.

Left a few comments, most are optional and noted for future improvements.

The only important one is the note ID / note details commitment switch up. In general, I think it would be worth double-checking that the changes from 0xMiden/protocol#2861 were correctly integrated in crates/store/src/db/* as there are a few note_commitment: NoteId types of occurrences left that will probably be confusing.

I did not review the rocksdb changes as I'm lacking context.

PhilippGackstatter · 2026-05-22T14:27:20Z

    pub fn network_account_id(&self) -> NetworkAccountId {
-        // SAFETY: This is a network account by construction.
-        self.protocol_account_id().try_into().unwrap()
+        // Trusted: constructors only produce this enum for accounts already classified as network
+        // (via the allowlist check above) or for updates that the caller filters through the actor
+        // registry.
+        NetworkAccountId::new_trusted(self.protocol_account_id())


If it makes things easier, we could add the NetworkAccountId wrapper in miden-standards and return it from NetworkAccount::id.

PhilippGackstatter · 2026-05-22T14:28:28Z

+                // lets us reconstruct the Account and look for the standardized
+                // `NetworkAccountNoteAllowlist` slot in storage; that is the new protocol-level
+                // definition of a network account.
+                if let Some(AccountUpdateDetails::Delta(delta)) = account_delta


Nit: I think comments shouldn't mention "changes", i.e. the comment will be a bit weird to read in the future.

Addressed in ec52fe5

PhilippGackstatter · 2026-05-22T14:39:33Z

                .iter()
                .cloned()
-                .filter_map(|note| note.header().map(NoteHeader::to_commitment))
+                .filter_map(|note| note.header().map(|h| h.id().as_word()))


Not necessary for this PR, but to increase type safety, would be useful to change get_block_inputs and get_batch_inputs to accept

unauthenticated_notes: impl Iterator<Item = NoteId> + Send,

instead of the current Word. And also to rename note_commitments to note_ids (for get_batch_inputs).

Addressed in ec52fe5

PhilippGackstatter · 2026-05-22T14:40:34Z

    pub fn output_note_commitments(&self) -> impl Iterator<Item = Word> + '_ {
-        self.inner
-            .output_notes()
-            .iter()
-            .map(miden_protocol::transaction::OutputNote::to_commitment)
+        self.inner.output_notes().iter().map(|n| n.id().as_word())
    }


Would be worth renaming to output_note_ids now.

Addressed in ec52fe5

PhilippGackstatter · 2026-05-22T14:54:22Z

    for raw in &raw_transactions {
        let notes: Vec<NoteHeader> = Deserializable::read_from_bytes(&raw.output_notes)?;
-        all_note_commitments.extend(notes.iter().map(NoteHeader::to_commitment));
+        all_note_commitments.extend(notes.iter().map(|h| h.id().as_word()));


Suggested change

all_note_commitments.extend(notes.iter().map(|h| h.id().as_word()));

all_note_ids.extend(notes.iter().map(NoteHeader::id));

Nit: Ideally this could be written like this.

Addressed in ec52fe5

PhilippGackstatter · 2026-05-22T14:57:18Z

    let mut output_notes_by_id = std::collections::BTreeMap::new();
    for chunk in all_note_commitments.chunks(QueryParamNoteCommitmentLimit::LIMIT) {
        output_notes_by_id.extend(select_note_sync_records(conn, chunk)?);
    }


Might be worth double-checking that note IDs and note commitments are used correctly here now. Looks like select_note_sync_records should now take NoteId as input.

Looking at the unchanged NoteSyncRecord with pub note_id: Word, was/is this correct? If this truly was note ID, why wasn't it NoteId and if it is now, would change it to NoteId.

Addressed in ec52fe5

PhilippGackstatter · 2026-05-22T15:00:47Z

+                .filter_map(|note| {
+                    let key = NoteId::from_raw(note.details_commitment().as_word());
+                    output_notes_by_id.get(&key).cloned()
+                })


This converts a NoteDetailsCommitment into a NoteId which is not a valid conversion. Probably should be:

Suggested change

.filter_map(|note| {

let key = NoteId::from_raw(note.details_commitment().as_word());

output_notes_by_id.get(&key).cloned()

})

.filter_map(|note| {

let key = note.id();

output_notes_by_id.get(&key).cloned()

})

Addressed in ec52fe5

PhilippGackstatter · 2026-05-22T15:04:14Z

    /// A publicly stored account, lives on-chain.
    #[serde(alias = "public")]
+    #[default]
    Public,
    /// A private account, which must be known by interactors.
    #[serde(alias = "private")]
    Private,
 }


Not sure if intentional, so just mentioning that the protocol treats Private as the default.

PhilippGackstatter · 2026-05-22T15:05:05Z

                    note_index,
                    note_id: note.id().as_word(),
-                    note_commitment: note.to_commitment(),
+                    note_commitment: note.id().as_word(),


Suggested change

note_commitment: note.id().as_word(),

note_id: note.id().as_word(),

Renamed the DB columns to match the new API from 0xMiden/protocol#2861:

notes.note_id → notes.details_commitment

notes.note_commitment → notes.note_id

This should make it clearer to understand after the protocol changes. a14e2b1

SantiagoPittella · 2026-05-22T21:19:46Z

+                if let Some(AccountUpdateDetails::Delta(delta)) = account_delta
+                    && delta.is_full_state()
+                    && let Ok(account) = Account::try_from(delta)
+                    && let Ok(network_account) = NetworkAccount::new(account)


Are we checking the storage slot inside NetworkAccount::new?

SantiagoPittella · 2026-05-22T21:45:07Z

Here we are going to panic with the current version. Since the Updated variant is now returned without filtering by network account. A way to handle this is to check if the get_account returned None then we can consider the account as non-network

good catch, fixed in 6cfe7f5

juan518munoz · 2026-05-22T22:23:27Z

heads up, as per d5fcf38 this PR no longer sets git's next as the dependency, but rather 0.15 from crates.io

juan518munoz force-pushed the jmunoz-update-miden-deps branch 3 times, most recently from 13a8bfe to 4ce77a3 Compare May 20, 2026 16:20

juan518munoz added 2 commits May 20, 2026 14:54

chore: update miden-protocol

de0c678

chore: simplify genesis

fbd4960

juan518munoz force-pushed the jmunoz-update-miden-deps branch from 4ce77a3 to fbd4960 Compare May 20, 2026 17:56

juan518munoz marked this pull request as ready for review May 20, 2026 19:09

igamigo reviewed May 20, 2026

View reviewed changes

igamigo requested review from Mirko-von-Leipzig and SantiagoPittella May 20, 2026 19:39

igamigo assigned PhilippGackstatter May 20, 2026

chore: address PR comments

8fbbb39

igamigo unassigned PhilippGackstatter May 21, 2026

juan518munoz requested a review from PhilippGackstatter May 21, 2026 18:04

Merge remote-tracking branch 'origin/next' into jmunoz-update-miden-deps

5004da0

# Conflicts: # crates/rpc/src/tests.rs

Mirko-von-Leipzig approved these changes May 22, 2026

View reviewed changes

PhilippGackstatter reviewed May 22, 2026

View reviewed changes

SantiagoPittella mentioned this pull request May 22, 2026

chore: monitor improvements #2118

Merged

igamigo mentioned this pull request May 22, 2026

chore: block subscription, deactivate actors #2120

Open

juan518munoz added 2 commits May 22, 2026 17:25

chore: address PR comments

ec52fe5

chore: better suit database columns to new protocol

a14e2b1

SantiagoPittella reviewed May 22, 2026

View reviewed changes

chore: change genesis default to private

5ee1c09

juan518munoz force-pushed the jmunoz-update-miden-deps branch from d2aeeab to 5ee1c09 Compare May 22, 2026 21:28

SantiagoPittella requested changes May 22, 2026

View reviewed changes

juan518munoz added 2 commits May 22, 2026 19:15

chore: switch to 0.15 dependency

d5fcf38

fix: prevent panic on non network account retrieval

6cfe7f5

Merge branch 'next' into jmunoz-update-miden-deps

7646269

	all_note_commitments.extend(notes.iter().map(\|h\| h.id().as_word()));
	all_note_ids.extend(notes.iter().map(NoteHeader::id));

	note_commitment: note.id().as_word(),
	note_id: note.id().as_word(),

Conversation

juan518munoz commented May 18, 2026

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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Mirko-von-Leipzig left a comment • 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!

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!

Mirko-von-Leipzig commented May 22, 2026

Uh oh!

PhilippGackstatter left a comment

Choose a reason for hiding this comment

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!

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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Mirko-von-Leipzig left a comment •

edited

Loading

juan518munoz commented May 22, 2026 •

edited

Loading