0xPolygonMiden / miden-base

Core components of the Polygon Miden rollup
MIT License
63 stars 37 forks source link

Fix the stack management for account procedures #783

Open hackaugusto opened 3 weeks ago

hackaugusto commented 3 weeks ago

AFAIU, the current code is organized as follows:

  1. miden lib is used to expose easy to use functions to users, e.g.:

https://github.com/0xPolygonMiden/miden-base/blob/dbe83c90916b9465a850355e6a4ed67f1ec034ff/miden-lib/asm/miden/account.masm#L87-L96

And internally this function manages the stack, such that the user won't lose its state:

https://github.com/0xPolygonMiden/miden-base/blob/dbe83c90916b9465a850355e6a4ed67f1ec034ff/miden-lib/asm/miden/account.masm#L96-L101

  1. the kernel library expects padded stack, since it can't return more than 16 elements, and it must overwrite some of the elements in the stack:

https://github.com/0xPolygonMiden/miden-base/blob/dbe83c90916b9465a850355e6a4ed67f1ec034ff/miden-lib/asm/kernels/transaction/api.masm#L163-L170

The above means the user would exec.set_item and have a nice to use API.

However, for security, set_account_item must authenticate the origin of the call, e.g. to prevent a rogue note from changing the account's code. This means the function can not be execed, and it must be called instead.

Now, because the call can return at most 16 elements in the stack, the code above that was suppose to provide a nice API is unusable, because it is pushing 3 elements to the stack which are never cleaned up:

https://github.com/0xPolygonMiden/miden-base/blob/dbe83c90916b9465a850355e6a4ed67f1ec034ff/miden-lib/asm/miden/account.masm#L97-L102

hackaugusto commented 3 weeks ago

To provide the above interface, one thing that could be done, is to use the same strategy as https://github.com/0xPolygonMiden/miden-base/issues/701, and have the accounts procedures available to dyncall.

The user would initialize the memory with the callback addresses in the beginning of the transaction script, and then would exec the code in miden/account.masm, which would in turn use a dyncall to transition to the authenticated account code. Alternatively, the user would not even need to do the initialization himself if we have a flag in memory and perform lazily initialization from the miden lib.

bobbinth commented 3 weeks ago

I think a couple of things may be conflated here. A few comments:

However, for security, set_account_item must authenticate the origin of the call, e.g. to prevent a rogue note from changing the account's code. This means the function can not be execed, and it must be called instead.

We need to use syscall rather than call to invoke kernel procedures - though, the effect on the stack would be the same in both cases.

Now, because the call can return at most 16 elements in the stack, the code above that was suppose to provide a nice API is unusable, because it is pushing 3 elements to the stack which are never cleaned up:

I'm not sure I follow this part. The referenced code should not leave any extra items on the stack (if it does, there is a bug somewhere). The call sequence looks something like this:

  1. The user runs exec.account::set_item in miden/accounts.masm. This is the procedure which has the user-friendly interface (i.e., the user does not need to worry about padding the stack).
  2. Internally, account::set_item does the stack padding and makes a syscall to set_account_item defined in api.masm.
  3. Then, set_account_item performance procedure authentication and executes account::set_item defined in miden/kernels/tx/account.masm. a. As a side note, putting the kernels modules into the miden library was probably not a good decision - they should be in a separate library which is imported by the api.masm instead of the miden library.

Flow between items 2 and 3 has been the source of quite a bit of confusion, and we should probably refactor it (as is described in #685) - but all of this is hidden from the user because exec.account::set_item from the first step should work like any other function.

hackaugusto commented 3 weeks ago

The user runs exec.account::set_item in miden/accounts.masm. This is the procedure which has the user-friendly interface (i.e., the user does not need to worry about padding the stack).

This only works from inside the account context, as in set_item is not a nice to use function for transaction scripts, for the account code, otherwise the authentication would fail. So there is another call done by the tx script to the code that does set_item. This is the one I'm referring to.

bobbinth commented 3 weeks ago

This only works from inside the account context, as in set_item is not a nice to use function for transaction scripts, for the account code, otherwise the authentication would fail. So there is another call done by the tx script to the code that does set_item. This is the one I'm referring to.

hmmm - account::set_item should only be available from account context. Trying to execute it from transaction or note script should be an error. We list here which kernel procedure are allowed to be called from which context.

bobbinth commented 3 weeks ago

Or I guess you mean we have a procedure on the account which internally invokes account::set_item, and this procedure is called from a transaction script via the call instruction. In this case yes, the caller is responsible for padding the stack and this is also one of the motivations for defining a consistent ABI described in #685.

hackaugusto commented 3 weeks ago

hmmm - account::set_item should only be available from account context. Trying to execute it from transaction or note script should be an error. We list here which kernel procedure are allowed to be called from which context.

Yes. We seem to agree on how it works. We don't seem to agree on how useful it is.

Basically what I'm trying to say is this only works as a nice to use function if you're writing the account code. And there is very little use for that. The vast majority of the code will be note scripts or tx scripts. At least in my eyes, the most common case should be the easiest. So that would be making the so-called nice to use functions usable from the tx script.

Account code is more complicated, we can't really hide the padding of the stack, because the function exposed by the account are called, and they have to handle these conventions anyways. If they have to be aware of the convention when exposing the account's API, it doesn't seem that much extra to assume the author can call the kernel using the same convention.

bobbinth commented 3 weeks ago

Basically what I'm trying to say is this only works as a nice to use function if you're writing the account code. And there is very little use for that. The vast majority of the code will be note scripts or tx scripts.

I am not sure about the last point. Writing account procedures which internally call kernel procedures would be fairly common too. I can't say whether it will be more or less common then writing note/tx scripts - but in my mind they are roughly comparable in importance.

I agree that there isn't much we can do with the procedures exposed by the account interface itself because they must be call-ed. And #685 attempts to define a standard way to call them. But I don't think just because we can't make these procedures as "easy-to-use" as regular procedures, we should make other procedures more complicated.

So, basically, my thinking is: