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

FPI: rework memory layout #882

Merged
merged 3 commits into from
Sep 23, 2024
Merged

FPI: rework memory layout #882

merged 3 commits into from
Sep 23, 2024

Conversation

Fumuran
Copy link
Contributor

@Fumuran Fumuran commented Sep 17, 2024

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

@Fumuran Fumuran added the no changelog This PR does not require an entry in the `CHANGELOG.md` file label Sep 17, 2024
@Fumuran
Copy link
Contributor Author

Fumuran commented Sep 17, 2024

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 Standardise conventions for comments and naming in Kernel #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).

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! I left a few small comments inline.

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 added a comment about this inline: I think NEW_CODE_ROOT should go into the bookkeeping section.

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.

I think that's fine. I'd probably add get_native_account_id procedure that would return the ID of the native account.

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.

I think 64 is fine. And yes, let's define MAX_NUM_FOREIGN_ACCOUNTS constant.

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.

I would leave this to the next PR. But also, I'm not sure we'll actually need this. I think figuring out whether we already have the account loaded or not may be handled by the host (via the advice provider).

Probably we should slightly update the docs and comments

I think we can leave this to #858.

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?

miden-lib/src/transaction/memory.rs Outdated Show resolved Hide resolved
miden-lib/src/transaction/memory.rs Outdated Show resolved Hide resolved
miden-lib/src/transaction/memory.rs Outdated Show resolved Hide resolved
miden-lib/asm/kernels/transaction/lib/memory.masm Outdated Show resolved Hide resolved
@Fumuran
Copy link
Contributor Author

Fumuran commented Sep 18, 2024

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

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! I left a few more small comments inline. Also, would be good to get a quick look from @phklive and/or @polydez.

In the next PR, we should add checks to make sure that methods that mutate account state or manipulate notes are only accessible if the current account is the native account.

miden-lib/src/transaction/memory.rs Outdated Show resolved Hide resolved
miden-lib/asm/kernels/transaction/lib/memory.masm Outdated Show resolved Hide resolved
miden-lib/asm/kernels/transaction/lib/memory.masm Outdated Show resolved Hide resolved
miden-lib/asm/kernels/transaction/lib/memory.masm Outdated Show resolved Hide resolved
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!

@phklive
Copy link
Contributor

phklive commented Sep 19, 2024

In the next PR, we should add check

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

@bobbinth
Copy link
Contributor

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
Copy link
Contributor

phklive commented Sep 19, 2024

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
Copy link
Contributor

@Fumuran - could you resolve the merge conflict?

@bobbinth bobbinth merged commit 2a09de3 into next Sep 23, 2024
8 checks passed
@bobbinth bobbinth deleted the andrew-fpi-memory-layout branch September 23, 2024 18:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no changelog This PR does not require an entry in the `CHANGELOG.md` file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants