Skip to content

fix: create monitor increment note via the account context#2344

Open
SantiagoPittella wants to merge 1 commit into
nextfrom
santiagopittella-fix-monitor-note-creation
Open

fix: create monitor increment note via the account context#2344
SantiagoPittella wants to merge 1 commit into
nextfrom
santiagopittella-fix-monitor-note-creation

Conversation

@SantiagoPittella

Copy link
Copy Markdown
Collaborator

Summary

The network monitor's counter flow was broken on next: every increment transaction failed during execution with:

failed to execute transaction kernel program:
  x error during processing of event 'miden::protocol::account::push_procedure_index'
  `-> account procedure with procedure root 0x… is not in the account procedure index map

The monitor's increment transaction script was written to mirror miden-standards 0.15's build_send_notes_script, which created output notes by executing output_note::create directly from the transaction-script context. After migrating to 0.16, the kernel makes note creation account-context only.

I changed it to create the note through the standard note_creator::create_note account procedure via call (switching into the account context)

Changelog

[[entry]]
scope       = "network-monitor"
impact      = "fixed"
description = "Fixed network monitor increment transactions failing under the 0.16 transaction kernel by creating notes through the account context"

@SantiagoPittella

Copy link
Copy Markdown
Collaborator Author

@Mirko-von-Leipzig we may want to consider releasing this as alpha.2 or something since the monitor doesn't work without this

@igamigo igamigo left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! A couple of notes:

  • Since we are creating a component already for increasing the counter, that same component could also create the note. This has a couple of small benefits: "atomicity" of storage increase with note creation (right now you could create a transaction that creates the note but does nto increase the counter or viceversa) and I think it may make the code just a tiny bit easier to follow since the transaction script would effectively be one call, and you don't need to explicitly add the basic wallet or note creator component. Obviously these are minor so feel free to disregard, but it might make it a bit more correct/idiomatic
  • Not sure what the plans are for #1827 but it might have caught this. With everything getting much more stable soon, addressing it might be diminishing returns but could still be interesting to tackle

let account = AccountBuilder::new(init_seed)
.account_type(AccountType::Public)
.with_auth_component(auth_component)
.with_component(BasicWallet)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I think we could replace this with a NoteCreator component (no real benefit though AFAICT, just reduces the scope of the account)

@Mirko-von-Leipzig Mirko-von-Leipzig left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! LGTM modulo the comments by @igamigo

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants