chore: add accounts creation for network monitor by SantiagoPittella · Pull Request #1276 · 0xMiden/node

SantiagoPittella · 2025-10-07T21:38:14Z

part of #1190

Added the account creations and deployment in this first version.
I'm working on another PR in the periodic value increments, and i'm leaving authentication as the last part.

bin/network-monitor/src/cli/commands.rs

igamigo

Leaving some comments after the offline discussion

igamigo · 2025-10-08T23:23:41Z

bin/network-monitor/src/assets/increment_counter.masm

+use.external_contract::counter_contract
+
+begin
+    call.counter_contract::increment


If this is used as a transaction script on #1280 (should it be moved to that PR?), it will fail every time because the get_sender call only applies at the moment of note processing (and in general, any getter under active_note)

I removed the sender validations temporarily, since it was complicating things i wanted to first have a simpler version working and then add it after.
Since this change made the deployment to be easier, I merge #1280 here and now we have the complete workflow. I will now work on sending the increment counter requests and display it in the frontend.

I would maybe put the authentication logic into counter_contract::increment since we want the counter contract to decide which notes to accept (rather than for notes to decide which accounts can consume them). To do this, we'd need to store the "owner" account ID in the storage slot together with the counter as well, and then before we increment, check that owner_id == note.sender.

I added it here, but fixed some small things in #1295 where it is actually tested.

igamigo · 2025-10-08T23:25:29Z

bin/network-monitor/src/assets/counter_program.masm

+    push.0
+    # => [0]
+
+    exec.account::get_item
+    # => [count]


nit: Some of these could be grouped for better readability:

Suggested change

push.0

# => [0]

exec.account::get_item

# => [count]

push.0 exec.account::get_item

# => [count]

* chore: add accounts creation command * wip: deploy accounts * chore: deploy counter account

bin/network-monitor/src/deploy/account.rs

bin/network-monitor/src/config.rs

bin/network-monitor/src/deploy/account.rs

…-monitor

sergerad · 2025-10-14T18:37:05Z

bin/network-monitor/src/monitor/tasks.rs

                let prover_url_clone = prover_url.clone();
                let name_clone = name.clone();
+                // SAFETY: we matched Some above
+                let proof_type = proof_type.unwrap();


nit: would do .expect("proof type is not none") instead of safety comment

…-monitor

bobbinth

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

bin/network-monitor/src/assets/counter_program.masm

bin/network-monitor/src/deploy/account.rs

bin/network-monitor/src/config.rs

bin/network-monitor/src/deploy/account.rs

bin/network-monitor/README.md

SantiagoPittella · 2025-11-04T18:46:00Z

I would maybe put the authentication logic into counter_contract::increment since we want the counter contract to decide which notes to accept (rather than for notes to decide which accounts can consume them). To do this, we'd need to store the "owner" account ID in the storage slot together with the counter as well, and then before we increment, check that owner_id == note.sender.

Yes. I did exactly that in the first implementation that I did of the procedures, but ATM I'm using the increment to deploy the network account too, so it was incompatible due to the account ID check failing (the sender was the network account itself and not the wallet)

…D validation

SantiagoPittella · 2025-11-04T19:34:49Z

I replaced the deployment, adding authentication and a call to get_count instead of the counter increase, so the nonce is increased but we dont need to go through the account ID match verification for deployment.

igamigo · 2025-11-05T00:00:22Z

bin/network-monitor/src/deploy/counter.rs

+    let account_code = AccountComponent::compile(
+        script,
+        TransactionKernel::assembler(),
+        vec![StorageSlot::empty_value(), StorageSlot::Value(owner_word)],
+    )?
+    .with_supports_all_types();
+
+    const INCR_NONCE_AUTH_CODE: &str = r"
+        use.miden::account
+        use.std::sys
+
+        export.auth__basic
+            exec.account::incr_nonce
+            drop
+        end
+    ";
+
+    let incr_nonce_auth =
+        AccountComponent::compile(INCR_NONCE_AUTH_CODE, TransactionKernel::assembler(), vec![])
+            .context("failed to compile increment nonce auth component")?
+            .with_supports_all_types();


I wonder if IncrNonce component from the mock tools from miden-testing can be used here instead (I think they are pretty much equivalent).

Yes! It is equivalent. I;m replacing it since we already use miden testing.

igamigo · 2025-11-05T00:01:10Z

bin/network-monitor/src/deploy/counter.rs

+        use.miden::account
+        use.std::sys
+
+        export.auth__basic


nit: I'm not 100% sure, but I believe having two underscores is not required anymore (ie, auth_basic would work)

It was removed in the replacement with miden testing

igamigo · 2025-11-05T00:03:17Z

bin/network-monitor/src/deploy/counter.rs

+
+    // Compile the account code
+    let owner_felts: [Felt; 2] = owner_account_id.into();
+    let owner_word = Word::from([owner_felts[0], owner_felts[1], Felt::new(0), Felt::new(0)]);


Based on how these will be used, would it be better to arrange them differently?

Suggested change

let owner_word = Word::from([owner_felts[0], owner_felts[1], Felt::new(0), Felt::new(0)]);

let owner_word = Word::from([Felt::ZERO, Felt::ZERO, owner_felts[1], owner_felts[0]]);

Should it like this? It the prefix first and then the suffix

Suggested change

let owner_word = Word::from([owner_felts[0], owner_felts[1], Felt::new(0), Felt::new(0)]);

let owner_word = Word::from([Felt::new(0), Felt::new(0), owner_felts[0], owner_felts[1]]);

Oh no, you're right, i see why.

igamigo · 2025-11-05T00:54:59Z

bin/network-monitor/src/deploy/counter.rs

+        .with_auth_component(incr_nonce_auth)
+        .build()?;
+
+    Ok(counter_program)


nit: "program" is a bit misleading, I've seen people name this "counter contract" or "counter account"

igamigo · 2025-11-05T01:24:10Z

bin/network-monitor/src/assets/counter_program.masm

+    # Ensure the note sender matches the authorized wallet stored in slot 1.
+    push.0 exec.input_note::get_sender
+    # => [sender_prefix, sender_suffix]
+    push.1 exec.account::get_item
+    # => [sender_prefix, sender_suffix, owner_prefix, owner_suffix, 0, 0]
+    movup.3 drop
+    movup.2 drop
+    # => [sender_prefix, sender_suffix, owner_prefix, owner_suffix]
+    exec.account_id::is_equal
+    assert


I think the ordering on the stack comments may not be correct here. Now that I see how the account is set up, I also think what I commented offline was not fully correct either. If the ordering of the owner word is as in this other comment, I believe you should be able to do something like:

push.1 exec.account::get_item # => [owner_prefix, owner_suffix, 0, 0] exec.active_note::get_sender # => [sender_prefix, sender_suffix, owner_prefix, owner_suffix, 0, 0] exec.account_id::is_equal # => [are_equal, 0, 0] assert drop drop

(but maybe this is not fully correct either, I have not run it)

igamigo · 2025-11-05T01:28:27Z

bin/network-monitor/src/deploy/mod.rs

+    if wallet_exists && counter_exists {
+        tracing::info!("Account files already exist, skipping account creation");
+        return Ok(());
+    }


I think this shouldn't matter too much outside of testing (where one may want to reset the state of at least one of the accounts), but if somehow only one of these were false, both accounts would be created again. Maybe this was intended but mentioning it just in case.

If only the wallet does not exists, the counter needs to be created again (since it is coupled to a wallet). The other way (wallet exists but not the counter) might be possible and should work

Despite that it should work to deploy a new counter with an existing account, we probably want to use a new wallet to keep some consistency between accounts, wdyt? I can comment it in the check

Ah yes, thanks for the explanation. Makes sense! I think always re-creating both should be fine.

igamigo · 2025-11-05T01:29:52Z

bin/network-monitor/README.md

+1. Checks for existing account files on startup
+2. Creates new accounts if files don't exist
+3. Deploys accounts to the network via RPC
+4. Uses the specified file paths (default: `wallet_account.bin` and `counter_program.bin`)


nit: When using AccountFile, we've used the .mac suffix for files. Also, I'd mention that the files are saved in the paths:

Suggested change

4. Uses the specified file paths (default: `wallet_account.bin` and `counter_program.bin`)

4. Saves the wallet and counter contract account in the specified file paths (default: `wallet_account.mac` and `counter_program.mac`)

igamigo · 2025-11-05T01:36:49Z

bin/network-monitor/src/config.rs

I was trying to test this locally and saw that the default port does not match with the node's default (altohugh in the env file it seems to be correct). Should it be changed here as well?

Suggested change

const DEFAULT_RPC_URL: &str = "http://localhost:57291";

…-monitor

bobbinth

Looks good! Thank you! I left a few more small comments inline, and I think there are some comments from @igamigo that still need to be addressed.

Also, we should rebase this from the latest next now that it is updated to v0.12.0 miden-base.

bin/network-monitor/src/config.rs

bin/network-monitor/src/deploy/wallet.rs

…-monitor

igamigo

LGTM! Left a few comments. Let me know if they are not clear. Overall though, they are mostly optional and so this should be good to merge.

One more thing: it would be great to test this somehow to make sure we catch functional drifts without having to manually run the monitor (similar to the stress test). This would include either mocking the node or running an integration test, so it could be left for a different issue.

igamigo · 2025-11-06T19:51:39Z

bin/network-monitor/src/deploy/mod.rs

+        .map_err(|e| anyhow::anyhow!("Failed to assemble library: {e}"))
+}
+
+// MONITOR DATA STORE


nit: I'd probably move this to its own mod

Since it is not too large and only used in this same file, I think that leaving it here is fine. Maybe we can consider to move it if we start to use it somewhere else, wdyt?

Yeah, sounds good. Not a strong opinion bjut it felt like the executor side of things could be refactored into its own submodule, but I agree it's probably premature

igamigo · 2025-11-06T19:53:43Z

bin/network-monitor/src/deploy/mod.rs

+/// Deploy accounts to the network.
+///
+/// This function creates both a wallet account and a counter program account,
+/// then saves them to the specified files.
+#[instrument(target = COMPONENT, name = "deploy-accounts", skip_all, ret(level = "debug"))]
+pub async fn deploy_accounts(
+    wallet_account: &Account,
+    counter_account: &Account,
+    rpc_url: &Url,
+) -> Result<()> {


I think part of the doc comment is outdated. Also, really only the counter account gets deployed to the network, right? So maybe it could be named deploy_counter_contract

It was already changed with deploy_counter_account in the follow up PR https://github.com/0xMiden/miden-node/pull/1295/files already

igamigo · 2025-11-06T19:56:17Z

bin/network-monitor/src/assets/deploy_counter.masm

+# Transaction script used during deployment to bump the counter contract nonce
+# without changing the counter value.


I think transaction scripts are not needed anymore for this. I'd try removing it and executing without a script.

Awesome, tested it and works. I remvoed the deploy tx script.

igamigo · 2025-11-06T20:00:48Z

bin/network-monitor/src/deploy/mod.rs

+        let account = self.wallet_account.lock().await;
+
+        let account = if account_id == account.id() {
+            account.to_owned()
+        } else {
+            self.counter_account.lock().await.to_owned()
+        };
+
+        let mut map_witness = None;
+        for slot in account.storage().slots() {
+            if let StorageSlot::Map(map) = slot {
+                if map.root() == map_root {
+                    map_witness = Some(map.open(&map_key));
+                }
+            }
+        }
+
+        if let Some(map_witness) = map_witness {
+            Ok(map_witness)
+        } else {
+            Err(DataStoreError::Other {
+                error_msg: "account storage does not contain the expected root".into(),
+                source: None,
+            })
+        }


In get_transaction_inputs you are instantiating the partial accounts with PartialAccount::from. If you include all the account data (I think you'd need to use PartialAccount::new), I think you should be able to remove the implementations of these methods. I would test removing them. We can also keep them, but it's more maintenance overhead + more code to test.

Removing the get_storage_map_witness worked when removing while using PartialAccount::new, that was not the case for get_vault_asset_witness.

Interesting, I wonder why that was? I don't think assets are involved in these flows.

I'll do further investigation after merging this PR 👍🏼

SantiagoPittella force-pushed the santiagopittella-deploy-test-account-network-monitor branch 2 times, most recently from 1cb7884 to d11a4ea Compare October 7, 2025 22:11

SantiagoPittella requested review from bobbinth, igamigo and sergerad October 7, 2025 22:14

sergerad reviewed Oct 7, 2025

View reviewed changes

bin/network-monitor/src/cli/commands.rs Show resolved Hide resolved

igamigo reviewed Oct 8, 2025

View reviewed changes

SantiagoPittella added 3 commits October 9, 2025 17:33

chore: add accounts creation command

ae90381

review: remove increment_counter, reduce counter_program

0d91054

review: remove create accounts command

0040d14

SantiagoPittella force-pushed the santiagopittella-deploy-test-account-network-monitor branch from e1f2703 to 0040d14 Compare October 9, 2025 20:33

SantiagoPittella and others added 2 commits October 9, 2025 18:13

chore: deploy accounts

20e22c4

* chore: add accounts creation command * wip: deploy accounts * chore: deploy counter account

chore: remove deploy-account command from readme

9b60886

SantiagoPittella changed the title ~~chore: add accounts creation command~~ chore: add accounts creation for network monitor Oct 9, 2025

SantiagoPittella requested review from igamigo and sergerad October 13, 2025 13:15

sergerad reviewed Oct 13, 2025

View reviewed changes

bin/network-monitor/src/deploy/account.rs Outdated Show resolved Hide resolved

sergerad reviewed Oct 13, 2025

View reviewed changes

bin/network-monitor/src/config.rs Outdated Show resolved Hide resolved

SantiagoPittella added 2 commits October 13, 2025 17:15

review: use PathBuf instead of str

98395e3

chore: remove authorized account from script

8178c6b

SantiagoPittella requested a review from sergerad October 13, 2025 20:29

sergerad reviewed Oct 13, 2025

View reviewed changes

bin/network-monitor/src/deploy/account.rs Outdated Show resolved Hide resolved

review: remove ? and return

7a9069e

SantiagoPittella requested a review from sergerad October 14, 2025 13:04

SantiagoPittella added 4 commits October 14, 2025 10:05

Merge branch 'next' into santiagopittella-deploy-test-account-network…

d57ba45

…-monitor

chore: update with latest next

b40eed6

chore: save account after deploy, add instrumentation to deployment

df40db9

fix: dont panic if remote prover is not available

58697a5

sergerad approved these changes Oct 14, 2025

View reviewed changes

sergerad reviewed Oct 14, 2025

View reviewed changes

Merge branch 'next' into santiagopittella-deploy-test-account-network…

7d4eb12

…-monitor

bobbinth reviewed Nov 4, 2025

View reviewed changes

SantiagoPittella added 6 commits November 4, 2025 15:10

review: add zeros to stack comment, use dropw

92f3da5

review: remove Box::pin

d7489f5

review: merge accounts into mod, add section separators

30e8b0d

Review: use filepath as suffix for config vars

42e32a2

review: replace 'will be saved' with 'are located' in docs

1f369d2

review: move files save into specific modules

1750869

chore: deploy incrementing nonce and calling get_count, add account I…

3c3f334

…D validation

igamigo reviewed Nov 5, 2025

View reviewed changes

SantiagoPittella added 5 commits November 5, 2025 12:13

review: use auth component from miden testing

6429e51

review: rename to counter_account

ca8dbb1

review: update account ID check in procedure

be5d4bd

review: replace .bin with .mac

7e9d494

review: use proper default URL

4f02292

SantiagoPittella requested review from bobbinth and igamigo November 5, 2025 16:19

Merge branch 'next' into santiagopittella-deploy-test-account-network…

182eefb

…-monitor

bobbinth approved these changes Nov 6, 2025

View reviewed changes

bin/network-monitor/src/config.rs Outdated Show resolved Hide resolved

bin/network-monitor/src/config.rs Outdated Show resolved Hide resolved

bin/network-monitor/src/deploy/wallet.rs Show resolved Hide resolved

SantiagoPittella added 4 commits November 6, 2025 09:14

wip: update to latest miden-base

f123533

Merge branch 'next' into santiagopittella-deploy-test-account-network…

245201d

…-monitor

chore: remove unused transaction store

a1396bb

review: update filepath docs, instrument create_wallet_account

c1768c3

igamigo approved these changes Nov 6, 2025

View reviewed changes

SantiagoPittella added 2 commits November 6, 2025 17:17

review: remove tx script to deploy

8bee4e3

review: use PartialAccount::new, remove get_vault_asset_witness impl

b4bc111

SantiagoPittella merged commit 6a8dea2 into next Nov 6, 2025
6 checks passed

SantiagoPittella deleted the santiagopittella-deploy-test-account-network-monitor branch November 6, 2025 21:06

	let owner_word = Word::from([owner_felts[0], owner_felts[1], Felt::new(0), Felt::new(0)]);
	let owner_word = Word::from([Felt::ZERO, Felt::ZERO, owner_felts[1], owner_felts[0]]);

	let owner_word = Word::from([owner_felts[0], owner_felts[1], Felt::new(0), Felt::new(0)]);
	let owner_word = Word::from([Felt::new(0), Felt::new(0), owner_felts[0], owner_felts[1]]);

	4. Uses the specified file paths (default: `wallet_account.bin` and `counter_program.bin`)
	4. Saves the wallet and counter contract account in the specified file paths (default: `wallet_account.mac` and `counter_program.mac`)

		# Transaction script used during deployment to bump the counter contract nonce
		# without changing the counter value.

Conversation

SantiagoPittella commented Oct 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

igamigo left a comment

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!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bobbinth left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

SantiagoPittella commented Nov 4, 2025

Uh oh!

SantiagoPittella commented Nov 4, 2025

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!

bobbinth left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

igamigo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

SantiagoPittella commented Oct 7, 2025 •

edited

Loading