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

Rework tests to prevent root context calls and remove kernel workarounds #851

Closed
phklive opened this issue Sep 2, 2024 · 3 comments
Closed
Assignees
Labels
enhancement New feature or request
Milestone

Comments

@phklive
Copy link
Contributor

phklive commented Sep 2, 2024

Feature description

The testing framework in Miden-base executes code directly from the kernel root context. Which should not be possible during "normal" execution of the virtual machine in the context of the Miden rollup.

This method makes some of the checks fail and forces us to use workarounds in the kernel code. One example is the following code:

# get procedure info (PROC_ELEMENTS, storage_offset) from memory stored at index
exec.get_procedure_info
# In production:
# => [PROC_ELEMENTS, storage_offset, PROC_ROOT]
# During testing:
# => [255, PROC_ROOT]
# TODO: Fix VM caller == [0,0,0,0] bug and remove this check
# check if get_procedure_info returned 255
dup push.255 neq
# => [is_255, index, PROC_ROOT]
# verify that PROC_ROOT matches returned procedure information
if.true
# verify that PROC_ROOT exists in memory at index
movup.4 movdn.8 assert_eqw.err=ERR_PROC_NOT_PART_OF_ACCOUNT_CODE
# => [storage_offset]
else
# drop PROC_ROOT
movdn.4 dropw
end

# TODO: Fix VM caller == [0,0,0,0] bug and remove this check
# check if index == 255
dup push.255 neq
# => [is_255, index]
# load procedure information from memory
if.true
# check that index < number of procedures contained in the account code
dup exec.memory::get_num_account_procedures lt assert.err=ERR_PROC_INDEX_OUT_OF_BOUNDS
# => [index]
# get procedure section ptr
push.2 mul exec.memory::get_account_procedures_section_offset add dup push.1 add
# => [proc_ptr, offset_ptr]
# load procedure information from memory
mem_load swap padw movup.4 mem_loadw
# => [PROC_ROOT, storage_offset]
end

// mock account method for testing from root context
// TODO: figure out if we can get rid of this
if proc_root == Digest::default() {
return Ok(255);
}

These pieces of code check if the caller is equal to [0,0,0,0] which would only happen if the call is being made directly from the kernel context and not from a user context, which should be the case in a general circumstances.

We need to remove this workaround by:

  1. Reworking how we test code in Miden-base, without making calls from root context
  2. Making sure that the caller is not equal to [0,0,0,0] the call having been made from a valid user context
  3. Remove the conditional code from account.masm here and here and account_procs.rs

Why is this feature needed?

We need to make sure that we remove workarounds as soon as possible to keep the cleanest codebase possible.

@igamigo
Copy link
Collaborator

igamigo commented Sep 5, 2024

I think I got some of this working but not sure if there is any alternative: Basically I'm exposing more calls through the account interface and calling instead of execing. And then of course assembling/running the code or transaction with the new account code. Does that sound OK or is there a better approach? Here's a rough draft, most tests pass except 2 or 3 for which the interface is not correctly set up I think.

@bobbinth
Copy link
Contributor

bobbinth commented Sep 6, 2024

I think this approach works (assuming we can make all tests work like that).

@igamigo
Copy link
Collaborator

igamigo commented Sep 11, 2024

Closed by #857

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Status: Done
Development

No branches or pull requests

3 participants