fix: Remove caller ID workaround for root contexts #857

igamigo · 2024-09-06T21:03:56Z

Closes #851
Also refactors tests to check against VM error codes. This resulted in some tests being slightly modified to account for things that were out of place (eg, some create_notes did not have parameters up to date and the process.is_err() would make the tests pass anyway).
Also adds some missing kernel errors to the kernel errors array.

igamigo · 2024-09-06T21:05:19Z

miden-tx/src/tests/kernel_tests/test_faucet.rs

+    // ERR_FUNGIBLE_ASSET_MISMATCH
+    assert_execution_errored(process, 0x00020039);


I'm not entirely sure all these error codes are correct and in line of what was being returned originally. I mean to double check this soon

I confirmed this by going back and checking the expected error codes for these tests

igamigo · 2024-09-06T21:06:56Z

miden-tx/src/tests/kernel_tests/test_account.rs


            # double check that on storage slot is indeed the new map
            push.{item_index}
-            exec.account::get_item
+            call.account::get_map_item 
        end
        ",
        item_index = storage_item.index,


This test is the only failing one. Not sure what it is, seems to be related to the stack state after set_map_item

This was fixed, but I'm not entirely sure how the fix is valid. Something in the interplay between get_item and set_map_item is likely not correct

get_item() reads the word in a given slot, regardless of the underlying type of the slot. Currently, we have two types of slots: simple values and maps. When the type of a slat is map, its value is actually the commitment to the map (i.e., its root).

So, if we update the map via set_map_item and then do get_item for the same storage slot, we'll get the root of the updated storage map back.

Let me know if this clarifies it.

set_map_item returns [OLD_VALUE, NEW_ROOT] hence in this test OLD_VALUE is asserted for but NEW_ROOT is not as of now.

We should add the following assertion:

assert_eq!( new_storage_map.root(), RpoDigest::from(process.get_stack_word(2)), "The returned updated value doesn't match the stored updated value", );

get_item() reads the word in a given slot, regardless of the underlying type of the slot. Currently, we have two types of slots: simple values and maps. When the type of a slat is map, its value is actually the commitment to the map (i.e., its root).

So, if we update the map via set_map_item and then do get_item for the same storage slot, we'll get the root of the updated storage map back.

Let me know if this clarifies it.

I get the difference, but this is how I had to define get_item on the mock account code for it to work:

export.get_item exec.account::get_item movup.8 drop movup.8 drop movup.8 drop end

set_map_item returns [OLD_VALUE, NEW_ROOT] hence in this test OLD_VALUE is asserted for but NEW_ROOT is not as of now.

It seems that ::miden::account::set_map_item returns [OLD_MAP_ROOT, OLD_MAP_VALUE]

I get the difference, but this is how I had to define get_item on the mock account code for it to work:

export.get_item exec.account::get_item movup.8 drop movup.8 drop movup.8 drop end

This is related to #685. Basically, we expect the above procedure to be call-ed and so it must leave exactly 16 items on the stack. But when we call exec.account::get_item it pushes 3 items onto the stack and so we need to drop them before returning from the call.

As discussed in #685, we should probably adapt an ABI that makes these things explicit. One general solution to this is that at the end of an account interface procedure, we should always call std::sys::truncate_stack. This is a little bit more expensive (in this case would take 28 cycles) - but probably not a big price to pay for a solution that would work every time.

igamigo · 2024-09-06T21:10:08Z

This is also possibly a breaking change (under testing at least), but I'm not sure we need to add a changelog line for this

igamigo · 2024-09-06T21:49:20Z

Some tests were failing with incorrect errors (missing note types in note-creation scripts, etc.). Changing this PR to draft until all are fixed.

phklive · 2024-09-07T14:08:04Z

Looking into the PR, I think this is the right solution, though previously when trying this method we hit limitations with the assembler erroring out on PhantomCalls did you face similar issues?

miden-lib/asm/kernels/transaction/lib/prologue.masm

bobbinth

Thank you! Looks good! I left a few comments/questions inline.

miden-lib/asm/kernels/transaction/lib/prologue.masm

miden-tx/src/tests/kernel_tests/test_prologue.rs

miden-tx/src/tests/kernel_tests/test_account.rs

bobbinth · 2024-09-09T23:52:02Z

miden-tx/src/tests/kernel_tests/test_account.rs


            # double check that on storage slot is indeed the new map
            push.{item_index}
-            exec.account::get_item
+            call.account::get_map_item 
        end
        ",
        item_index = storage_item.index,


get_item() reads the word in a given slot, regardless of the underlying type of the slot. Currently, we have two types of slots: simple values and maps. When the type of a slat is map, its value is actually the commitment to the map (i.e., its root).

So, if we update the map via set_map_item and then do get_item for the same storage slot, we'll get the root of the updated storage map back.

Let me know if this clarifies it.

miden-tx/src/tests/kernel_tests/test_account.rs

miden-tx/src/tests/kernel_tests/test_faucet.rs

phklive

Great work!

Love to see the workaround be removed.

A few proposed changes.

phklive · 2024-09-10T10:03:54Z

miden-lib/asm/kernels/transaction/lib/account.masm

@@ -565,24 +558,11 @@ export.authenticate_procedure

    # get procedure info (PROC_ELEMENTS, storage_offset) from memory stored at index
    exec.get_procedure_info
-    # In production:
    # => [PROC_ELEMENTS, storage_offset, PROC_ROOT]


I would change the return naming here from PROC_ELEMENTS to PROC_ROOT and in the above comment.

phklive · 2024-09-10T10:05:36Z

miden-lib/asm/kernels/transaction/lib/prologue.masm

@@ -368,6 +368,7 @@ proc.validate_new_account
            # empty SMT)
            exec.constants::get_empty_smt_root assert_eqw.err=ERR_PROLOGUE_NEW_NON_FUNGIBLE_FAUCET_INVALID_RESERVED_SLOT
            # => []
+            push.1001 drop                              # TODO: remove line, see miden-vm/#1122


Why do we need to add this here specifically? Did we have an error with decorators previously?

Yeah, it's what Bobbin mentioned yesterday (and related to the error mentioned in the comment). Essentially for asserts with errors the MAST root would be the same and thus the error returned by the VM could be unrelated to the actual error happening in the execution. I do not know the inner workings but adding this push and drop fixes it and we can assert for the actual error in the test.

phklive · 2024-09-10T10:11:04Z

miden-tx/src/tests/kernel_tests/test_account.rs


            # double check that on storage slot is indeed the new map
            push.{item_index}
-            exec.account::get_item
+            call.account::get_map_item 
        end
        ",
        item_index = storage_item.index,


set_map_item returns [OLD_VALUE, NEW_ROOT] hence in this test OLD_VALUE is asserted for but NEW_ROOT is not as of now.

We should add the following assertion:

assert_eq!( new_storage_map.root(), RpoDigest::from(process.get_stack_word(2)), "The returned updated value doesn't match the stored updated value", );

phklive · 2024-09-10T10:13:45Z

miden-tx/src/tests/kernel_tests/test_account.rs

-fn test_authenticate_procedure() {
-    let tx_context = TransactionContextBuilder::with_standard_account(ONE).build();
-    let account = tx_context.tx_inputs().account();
+fn test_procedure_in_advice_provider() {


I would keep the naming of this test test_authenticate_procedure because authenticate_procedure is a kernel proc.

And it is the one that we are trying to test the logic of here.

I changed it back, but there's a subtle distinction I think: Even though authenticate_procedure is the actually called procedure, it errors immediately where the push_procedure event is emitted instead of the check for comparing roots and authenticating against the account code.

export.authenticate_procedure # load procedure index emit.ACCOUNT_PUSH_PROCEDURE_INDEX_EVENT adv_push.1 <- Execution fails here # => [index, PROC_ROOT] # get procedure info (PROC_ROOT, storage_offset) from memory stored at index exec.get_procedure_info # => [PROC_ROOT, storage_offset, PROC_ROOT] # 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] end

If I'm not wrong the account code could contain the procedure, but if the procedure is not in the advice provider, the execution would fail anyway (with the same error it throws now) and the test would pass anyway, which is why I thought the other name was a tiny bit more appropriate. In any case the positive cases do test the whole proc for valid cases.

miden-tx/src/tests/kernel_tests/test_account.rs

bobbinth

Looks good! Thank you! I left some small comments inline.

miden-lib/asm/kernels/transaction/lib/prologue.masm

miden-tx/src/lib.rs

bobbinth · 2024-09-11T08:44:06Z

miden-tx/src/tests/kernel_tests/test_account.rs


            # double check that on storage slot is indeed the new map
            push.{item_index}
-            exec.account::get_item
+            call.account::get_map_item 
        end
        ",
        item_index = storage_item.index,


I get the difference, but this is how I had to define get_item on the mock account code for it to work:

export.get_item exec.account::get_item movup.8 drop movup.8 drop movup.8 drop end

This is related to #685. Basically, we expect the above procedure to be call-ed and so it must leave exactly 16 items on the stack. But when we call exec.account::get_item it pushes 3 items onto the stack and so we need to drop them before returning from the call.

As discussed in #685, we should probably adapt an ABI that makes these things explicit. One general solution to this is that at the end of an account interface procedure, we should always call std::sys::truncate_stack. This is a little bit more expensive (in this case would take 28 cycles) - but probably not a big price to pay for a solution that would work every time.

bobbinth

All looks good! Thank you!

igamigo commented Sep 6, 2024

View reviewed changes

igamigo added the no changelog This PR does not require an entry in the `CHANGELOG.md` file label Sep 6, 2024

igamigo marked this pull request as draft September 6, 2024 21:49

igamigo commented Sep 9, 2024

View reviewed changes

miden-lib/asm/kernels/transaction/lib/prologue.masm Show resolved Hide resolved

igamigo requested a review from phklive September 9, 2024 02:41

igamigo marked this pull request as ready for review September 9, 2024 02:41

bobbinth reviewed Sep 10, 2024

View reviewed changes

phklive reviewed Sep 10, 2024

View reviewed changes

bobbinth approved these changes Sep 11, 2024

View reviewed changes

igamigo force-pushed the igamigo-wallet branch 3 times, most recently from 45e16cf to 5db5c7c Compare September 11, 2024 15:52

Base automatically changed from igamigo-wallet to next September 11, 2024 16:08

bobbinth approved these changes Sep 11, 2024

View reviewed changes

test: Fix tests, assert on error codes, remove root context calls

ce66581

igamigo force-pushed the igamigo-root-context branch from 88d9f44 to ce66581 Compare September 11, 2024 23:23

igamigo merged commit afdabb7 into next Sep 11, 2024
8 checks passed

igamigo deleted the igamigo-root-context branch September 11, 2024 23:39

igamigo mentioned this pull request Sep 11, 2024

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

Closed

phklive mentioned this pull request Sep 13, 2024

Offset faucet procedures by 1 to enable correct authentication and reserved slot access #864

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Remove caller ID workaround for root contexts #857

fix: Remove caller ID workaround for root contexts #857

igamigo commented Sep 6, 2024 •

edited

Loading

igamigo Sep 6, 2024

igamigo Sep 11, 2024

igamigo Sep 6, 2024

igamigo Sep 6, 2024 •

edited

Loading

bobbinth Sep 9, 2024

phklive Sep 10, 2024

igamigo Sep 10, 2024

bobbinth Sep 11, 2024

igamigo commented Sep 6, 2024 •

edited

Loading

igamigo commented Sep 6, 2024 •

edited

Loading

phklive commented Sep 7, 2024

bobbinth left a comment

bobbinth Sep 9, 2024

phklive left a comment

phklive Sep 10, 2024

igamigo Sep 10, 2024

phklive Sep 10, 2024

igamigo Sep 10, 2024

phklive Sep 10, 2024

phklive Sep 10, 2024

igamigo Sep 10, 2024 •

edited

Loading

bobbinth left a comment

bobbinth Sep 11, 2024

bobbinth left a comment

		// ERR_FUNGIBLE_ASSET_MISMATCH
		assert_execution_errored(process, 0x00020039);

fix: Remove caller ID workaround for root contexts #857

fix: Remove caller ID workaround for root contexts #857

Conversation

igamigo commented Sep 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

igamigo Sep 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

igamigo commented Sep 6, 2024 • edited Loading

igamigo commented Sep 6, 2024 • edited Loading

phklive commented Sep 7, 2024

bobbinth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phklive left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

igamigo Sep 10, 2024 • edited Loading

Choose a reason for hiding this comment

bobbinth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bobbinth left a comment

Choose a reason for hiding this comment

igamigo commented Sep 6, 2024 •

edited

Loading

igamigo Sep 6, 2024 •

edited

Loading

igamigo commented Sep 6, 2024 •

edited

Loading

igamigo commented Sep 6, 2024 •

edited

Loading

igamigo Sep 10, 2024 •

edited

Loading