search

0xPolygonMiden / miden-base

Core components of the Polygon Miden rollup
MIT License
68 stars 41 forks source link

FPI: rework memory layout #882

Closed Fumuran closed 4 days ago

Fumuran commented 1 week ago

This PR reworks memory layout to create an account data section, described in the FPI issue: #847

Fumuran commented 1 week ago

During the implementation I had a couple of proposals and questions:

  1. Probably we should update the changelog after all the changes in the miden-base will be addressed.
  2. I slightly changed the proposed account data layout:
    • I added the NEW_CODE_ROOT in addition to CODE_ROOT since it was a part of the previous layout and is used by other components.
    • I removed the current_account_id variable from the bookkeeping section, I'm not sure that we need to store it there: we can get the account data using the current_account_data_offset and then get the ID from this data itself. It makes it more simple to keep the bookkeeping values updated and allows us to remove the associated procedures. This is a tradeoff, so I'm not sure what is the best solution.
  3. I wonder what is the limit on the number of the foreign accounts which we can load? For now I set it to 64 accounts, but that is just my "feeling".
    • Probably we should add a constant like MAX_ACCOUNT_NUMBER to track whether we can load another account.
  4. I think we should also add the number of already loaded accounts to the bookkeeping section: that will help us to iterate over the loaded accounts to check whether the new one was already loaded, and also will help to get the next available offset for the new account.
  5. Probably we should slightly update the docs and comments:
    • We use a notion of "core" account data, meaning its first four words (id and nonce, vault root, storage root, code root) but I didn't find any docs describing this notion (only an indirect description). Probably we should explain this somewhere.
    • I think this could be the part of the #858: we use both root and commitment notions referencing the account data. Probably we should choose and stick to only one option.
  6. This one is not directly connected to this PR, but we should add additional test to the test_prologue.rs to check the values stored in the kernel section (I forgot to do that in the Dynamic kernel procedure invocation PR).
Fumuran commented 1 week ago

This one is not directly connected to this PR, but we should add additional test to the test_prologue.rs to check the values stored in the kernel section (I forgot to do that in the Dynamic kernel procedure invocation PR).

Yes, we should do that but in a different PR. Should we create an issue for this?

I created an issue: #884

phklive commented 1 week ago

In the next PR, we should add check

Sure, will finish my tasks at hand and look into it during my EOD.

bobbinth commented 1 week ago

In the next PR, we should add check

Sure, will finish my tasks at hand and look into it during my EOD.

I think @Fumuran will be working on this.

phklive commented 1 week ago

In the next PR, we should add check

Sure, will finish my tasks at hand and look into it during my EOD.

I think @Fumuran will be working on this.

No I meant, I will review this PR by EOD, as you requested in the first part of your sentence.

bobbinth commented 1 week ago

@Fumuran - could you resolve the merge conflict?