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

Update instructions and constants according to u32 instructions refactoring #292

Merged
merged 2 commits into from
Nov 3, 2023

Conversation

Fumuran
Copy link
Contributor

@Fumuran Fumuran commented Oct 29, 2023

This small PR updates u32 instructions according to the new refactoring. To make the code work after updating the instructions it is necessary to update MAST roots of the procedures used in the call invocations, as well as constants containing MAST roots of this procedures.

Due to the fact that some roots are stored as constants, it turns out to be quite difficult to automatically regenerate them when running tests, so for now, new hashes are hardcoded, just as before.

Also now it becomes clear the reason why errors appear during the actions described in this issue.

This PR should be merged after the main refactoring PR.

@Fumuran Fumuran requested a review from bobbinth October 29, 2023 04:25
@Fumuran Fumuran force-pushed the andrew-refactor-u32-instructions branch from 4d1cb7d to fe5f2c0 Compare November 1, 2023 08:48
@bobbinth bobbinth force-pushed the andrew-refactor-u32-instructions branch from fe5f2c0 to 48844df Compare November 2, 2023 01:01
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!

Let's create an issue for computing MAST roots used in testing at compile time. We can probably use something similar to what we did here for decreasing PoW requirements in testing context.

@bobbinth
Copy link
Contributor

bobbinth commented Nov 2, 2023

The CI is failing here because account seeds seem to no longer result in valid account IDs. This is likely because MAST roots of procedures changed. @hackaugusto - could you help with figuring out how to generate new valid seeds?

@bobbinth
Copy link
Contributor

bobbinth commented Nov 2, 2023

@Overcastan - one question: why did the MAT roots for procedures change? Most checked instructions should have been replaced with the exact same sequences of operations as would have been generated by the assembler. Is the reason that shl/shr instructions generate slightly different sequences of operations?

@Fumuran
Copy link
Contributor Author

Fumuran commented Nov 2, 2023

Yes, unfortunately it turned out to be impossible to emulate exactly the checked variants of shl, shr, rotl and rotr instructions, since we need to validate the 2^imm value, what can be performed only inside the instruction compilation. This is why I leaved the parameter validation even in the unchecked versions, but anyway the overall set of instructions changed. For example:

u32checked_shr.30 -> validate_param() + [Push(2^30), U32Assert2, U32Div, Drop]
checked u32shr.30 emulation -> u32assert u32shr.30 -> [Pad, U32Assert2, Drop] + validate_param() + [Push(2^30), U32Div, Drop]

@Fumuran
Copy link
Contributor Author

Fumuran commented Nov 2, 2023

Looks like making u32sh* and u32rot* instructions be always checked will be better overall, so I'll change them ASAP. After that change this PR could become much more simple (I hope), because used in this PR checked shift and rotate operations will essentially be unchanged, which should save us from changing MAST roots and seeds.

@hackaugusto
Copy link
Contributor

hackaugusto commented Nov 2, 2023

The CI is failing here because account seeds seem to no longer result in valid account IDs. This is likely because MAST roots of procedures changed. @hackaugusto - could you help with figuring out how to generate new valid seeds?

I wonder if an easier approach would be to just query the MAST roots from the assembler?

If we can't do that, my suggestion would to:

  • use the build.rs to generate the files, and add it to source control.

    • the goal here is to make the files accessible for the LSP, similar to what is done with the protobuf messages on the miden-node
  • to implement the above, I would do something like this:

    • add in the build.rs an assembler, to parse all the masm files in the library
    • query the constants from said assembler, and generate the source code with a format!, something like:
    /// === GENERATED SOURCE CODE ===
    /// for additional info check `build.rs`
    pub const ACCOUNT_PROCEDURE_INCR_NONCE_MAST_ROOT: &str = "{}";
    pub const ACCOUNT_PROCEDURE_SET_CODE_MAST_ROOT: &str = "{}";
    pub const ACCOUNT_PROCEDURE_SET_ITEM_MAST_ROOT: &str = "{}";
    • save the above template into a file into something like generated/constants.rs which are re-exported at the top-level
    • and use said constants in the test code

One issue with the above, is that changes to the miden-vm can't be detected, so I think querying from the assembler is more reliable.

@bobbinth
Copy link
Contributor

bobbinth commented Nov 3, 2023

main branch currently doesn't compile - so, I'll roll this back to 7dae4d8 and merge. This will mean that 5 or so tests will be failing - we'll fix them in a separate PR.

@bobbinth bobbinth force-pushed the andrew-refactor-u32-instructions branch from 3f65702 to 7dae4d8 Compare November 3, 2023 17:58
@bobbinth bobbinth force-pushed the andrew-refactor-u32-instructions branch from 7dae4d8 to aea11c0 Compare November 3, 2023 18:00
@bobbinth
Copy link
Contributor

bobbinth commented Nov 3, 2023

I wonder if an easier approach would be to just query the MAST roots from the assembler?

I think getting correct MAST roots is a part of the issue. The other part is that once MAST roots change, we also need to update account seeds (and mock data). So, we need to come up with a process of how to handle such updates end-to-end. I'll create an issue for this.

@bobbinth bobbinth merged commit c29dc30 into main Nov 3, 2023
@bobbinth bobbinth deleted the andrew-refactor-u32-instructions branch November 3, 2023 18:07
@bobbinth bobbinth mentioned this pull request Nov 3, 2023
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