Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: introduce create_note kernel procedure auth #235

Merged
merged 1 commit into from
Sep 13, 2023

Conversation

frisitano
Copy link
Contributor

@frisitano frisitano commented Sep 11, 2023

This PR introduces authentication for the create_note kernel procedure. We modify tests to account for the updated auth pattern and introduce some new tests as well as a few minor comment updates.

closes: #218 #79

Comment on lines +97 to +98
call.0x33e6e544bce56ad3bd235d9806e23910a4f03371e7e0a8549d730d0ee61266d8
drop dropw dropw
Copy link
Contributor Author

@frisitano frisitano Sep 11, 2023

Choose a reason for hiding this comment

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

I started out with this logic defined in a procedure at the top of this source file using the context import:

use.context::account_*

proc.create_note_wrapper
      call.account_*::create_note
      drop dropw dropw
end

This was also defined at the top of the source file in note_2_script_src. This led to a strange bug which I suspect is related to compiler cache collision or some other form of unexpected behaviour which I haven't had time to investigate (I already lost a lot of time battling this). Migrating to the hex specifier of the account procedure fixed things.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think using hex-based calling convention is the right approach here and I've created 0xPolygonMiden/miden-vm#1071 to improve support for this. But it is a little concerning that there was this odd behavior in the first place. Do you think it would be possible to create a minimal reproducible example of this in Miden VM? If we have such an example, I should be able to debug this relatively quickly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure thing, I will try and recreate an example.

Copy link
Contributor Author

@frisitano frisitano Sep 12, 2023

Choose a reason for hiding this comment

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

Here is an example of the failure case for the transaction in which we use different names for the procedures at the top of each note script:

frisitano-create-note-auth...frisitano-assembler-error

This actually compiles and runs but fails an assertion error which I wasn't able to track down. I wouldn't expect a failed assertion as the logic should be the same.

Here is a problem related to procedure name collisions in different scripts when compiled using the same AssemblyContext:

0xPolygonMiden/miden-vm@next...frisitano-assembler-proc-collisions

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When I inline the call operation it works fine so maybe it's something to do with the way that call operations are inlined when invoked via procedures?

This works fine:
frisitano-create-note-auth...frisitano-assembler-bug-2

Copy link
Contributor

@bobbinth bobbinth left a comment

Choose a reason for hiding this comment

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

Looks good! Thank you! I left a note about the odd behavior with the note scripts.

Other than that, this PR should be merged after #236 is merged (which itself is waiting for 0xPolygonMiden/miden-vm#1070 to be merged).

Comment on lines +97 to +98
call.0x33e6e544bce56ad3bd235d9806e23910a4f03371e7e0a8549d730d0ee61266d8
drop dropw dropw
Copy link
Contributor

Choose a reason for hiding this comment

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

I think using hex-based calling convention is the right approach here and I've created 0xPolygonMiden/miden-vm#1071 to improve support for this. But it is a little concerning that there was this odd behavior in the first place. Do you think it would be possible to create a minimal reproducible example of this in Miden VM? If we have such an example, I should be able to debug this relatively quickly.

@frisitano frisitano merged commit ec278bd into main Sep 13, 2023
@frisitano frisitano deleted the frisitano-create-note-auth branch September 13, 2023 05:07
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.

Add authentication to create_note kernel procedure
2 participants