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

Dynamic kernel procedure invocation #803

Merged
merged 13 commits into from
Sep 13, 2024

Conversation

Fumuran
Copy link
Contributor

@Fumuran Fumuran commented Jul 22, 2024

This PR reworks the way syscall procedures being executed: instead of using syscall for each procedure, now we load the offset of the desired procedure to the stack and then execute syscall.exec_kernel_proc. More details can be found in the corresponding issue: #701.

TODO:

  • Update docs
  • Update CHANGELOG

@Fumuran
Copy link
Contributor Author

Fumuran commented Jul 24, 2024

I am putting forward a proposal to put on hold this task until the Miden VM 0.10 is released.

It is extremely complicated to make this feature work using the "pre-refactoring" version of the assembler. To make it work it's needed to put the hash of the desired procedure to the CodeBlockTable, which could be done by using one of the call, syscall or procref operations. call and procref works similar and can't be used with kernel procedures. The only way to achieve it is to call this procedure with syscall somewhere at the preparation stage and then roll back the stack, but considering that there are 29 procedures in the core overall, it will be a mess. But even if I manage to implement it it still will be needed to rework the implementation to make it compatible with a new version of the VM and assembly.

The other options is to try to change the dependencies of the miden-base so it will work with an updated version of the assembler. This is quite a complicated task, since a lot of breaking changes were made in the new version of the VM. But this is not the only problem: new assembly will require at least a new version of the crypro repo, so this refactoring will be pretty complex. And considering that current version of the VM is not even final for the 0.10 release, I think that it is not appropriate to spend time and effort for this refactoring.

Currently the core functionality for that feature is already implemented, so it shouldn't take too long to finish it after new version of the VM is released.

@bobbinth what do you think?

@Fumuran Fumuran force-pushed the andrew-dynamic-kernel-procedure-invocation branch from 2254f85 to 977ec96 Compare August 7, 2024 19:12
@Fumuran Fumuran force-pushed the andrew-dynamic-kernel-procedure-invocation branch 4 times, most recently from 6642058 to 814e470 Compare August 28, 2024 17:49
@Fumuran Fumuran marked this pull request as ready for review August 28, 2024 18:06
@Fumuran Fumuran added this to the v0.6 milestone Aug 29, 2024
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.

Thank you! Looks good! Not a full review yet, but I left some comments inline.

Also, I'm curious how this affects cycle counts for our transaction benchmarks.

miden-lib/src/transaction/mod.rs Outdated Show resolved Hide resolved
miden-lib/src/transaction/mod.rs Outdated Show resolved Hide resolved
miden-lib/src/transaction/inputs.rs Outdated Show resolved Hide resolved
miden-lib/asm/kernels/transaction/api.masm Show resolved Hide resolved
miden-lib/asm/kernels/transaction/api.masm Outdated Show resolved Hide resolved
miden-lib/asm/miden/account.masm Outdated Show resolved Hide resolved
miden-lib/asm/miden/kernel_invocation.masm Outdated Show resolved Hide resolved
miden-lib/asm/miden/kernel_invocation.masm Outdated Show resolved Hide resolved
@Fumuran Fumuran force-pushed the andrew-dynamic-kernel-procedure-invocation branch 2 times, most recently from 90237a8 to 2ecedfb Compare September 5, 2024 13:28
@Fumuran Fumuran force-pushed the andrew-dynamic-kernel-procedure-invocation branch from 2ecedfb to eabf4da Compare September 5, 2024 13:31
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've added some more comments inline.

objects/src/block/header.rs Show resolved Hide resolved
objects/src/block/header.rs Outdated Show resolved Hide resolved
miden-lib/src/transaction/mod.rs Outdated Show resolved Hide resolved
miden-lib/src/transaction/mod.rs Outdated Show resolved Hide resolved
miden-lib/asm/kernels/transaction/lib/prologue.masm Outdated Show resolved Hide resolved
miden-lib/asm/miden/kernel_invocation.masm Outdated Show resolved Hide resolved
miden-lib/src/transaction/procedures.rs Outdated Show resolved Hide resolved
objects/src/testing/block.rs Show resolved Hide resolved
miden-lib/src/transaction/procedures.rs Outdated Show resolved Hide resolved
miden-lib/src/transaction/procedures.rs Outdated Show resolved Hide resolved
@Fumuran Fumuran force-pushed the andrew-dynamic-kernel-procedure-invocation branch from 15fe435 to 64723f4 Compare September 11, 2024 15:54
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 some more comments inline. The main one is about checking kernel procedure bounds.

miden-lib/src/transaction/mod.rs Outdated Show resolved Hide resolved
miden-lib/src/transaction/mod.rs Outdated Show resolved Hide resolved
miden-lib/asm/kernels/transaction/lib/prologue.masm Outdated Show resolved Hide resolved
miden-lib/src/transaction/inputs.rs Outdated Show resolved Hide resolved
miden-lib/asm/kernels/transaction/api.masm Show resolved Hide resolved
miden-lib/src/transaction/procedures.rs Outdated Show resolved Hide resolved
miden-lib/src/transaction/procedures.rs Outdated Show resolved Hide resolved
miden-lib/src/transaction/procedures.rs Outdated Show resolved Hide resolved
miden-lib/src/transaction/procedures.rs Outdated Show resolved Hide resolved
miden-lib/asm/miden/note.masm Show resolved Hide resolved
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 few small comments inline. Once these are addressed, we can merge.

miden-lib/src/transaction/procedures.rs Outdated Show resolved Hide resolved
miden-lib/src/transaction/procedures.rs Outdated Show resolved Hide resolved
miden-tx/src/errors/tx_kernel_errors.rs Outdated Show resolved Hide resolved
miden-lib/src/transaction/inputs.rs Show resolved Hide resolved
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.

All looks good! Thank you!

@bobbinth bobbinth merged commit bb73449 into next Sep 13, 2024
8 checks passed
@bobbinth bobbinth deleted the andrew-dynamic-kernel-procedure-invocation branch September 13, 2024 15:55
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.

2 participants