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

Introduce transaction epilogue #65

Merged
merged 1 commit into from
Apr 3, 2023
Merged

Introduce transaction epilogue #65

merged 1 commit into from
Apr 3, 2023

Conversation

frisitano
Copy link
Contributor

@frisitano frisitano commented Mar 22, 2023

This PR implements the transaction kernel epilogue. The scope of this PR includes the following:

  • Introduction of make watch command that allows cargo watch to monitor the assembly directory miden-lib/asm and rebuild the miden library when a file change is saved. This dramatically improves developer experience when working with the debugger making quick iterative changes.
  • Introduction of masm transaction epilogue. The final check to assert the invariant of assets between start and end of the transaction is still outstanding and is waiting on the completion of SMT implementation.
  • Introduction of an assembly utils module for shared code. This has been useful and I suggest we create a memory module that we can use to handle all memory related activities. I think this should also allow us to centralise the constants we use as most (maybe all?) of them are related to memory.
  • Introduction of rust based memory module that specifies the memory layout for the miden kernel.
  • Refactor of the test suite and common modules.
  • Change max assets per note to 256
  • Introduction of ExecutedTransaction object which holds all information relating to a transaction.
  • Introduction of transaction/utils.rs which holds common functionality for producing commitments and inputs / outputs for a transaction.
  • Introduction of Metadata to the Note object.

@frisitano frisitano marked this pull request as ready for review March 23, 2023 10:12
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 great! Thank you! I left some questions and comments inline.

objects/src/notes/vault.rs Outdated Show resolved Hide resolved
objects/src/transaction/utils.rs Outdated Show resolved Hide resolved
objects/src/transaction/utils.rs Outdated Show resolved Hide resolved
objects/src/transaction/executed_tx.rs Show resolved Hide resolved
objects/src/notes/mod.rs Show resolved Hide resolved
miden-lib/asm/sat/utils.masm Outdated Show resolved Hide resolved
miden-lib/asm/sat/epilogue.masm Outdated Show resolved Hide resolved
miden-lib/asm/sat/epilogue.masm Outdated Show resolved Hide resolved
miden-lib/asm/sat/epilogue.masm Outdated Show resolved Hide resolved
miden-lib/asm/sat/epilogue.masm Outdated Show resolved Hide resolved
@frisitano frisitano force-pushed the frisitano-epilogue branch 2 times, most recently from c55fa05 to de7e34b Compare March 24, 2023 17:24
miden-lib/tests/test_epilogue.rs Outdated Show resolved Hide resolved
miden-lib/tests/common/mod.rs Show resolved Hide resolved
miden-lib/src/memory.rs Outdated Show resolved Hide resolved
miden-lib/src/memory.rs Outdated Show resolved Hide resolved
miden-lib/asm/sat/prologue.masm Outdated Show resolved Hide resolved
miden-lib/src/memory.rs Outdated Show resolved Hide resolved
miden-lib/asm/sat/utils.masm Outdated Show resolved Hide resolved
miden-lib/asm/sat/utils.masm Outdated Show resolved Hide resolved
miden-lib/asm/sat/epilogue.masm Outdated Show resolved Hide resolved
miden-lib/asm/sat/epilogue.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! I left a few more comments inline. A couple of other things:

  1. This should probably be updated to work with the latest next branch of Miden VM.
  2. In the future, once things stabilize a bit more, we should write an article in the docs to document memory layout with diagrams and in-depth explanations. Let's create an issue for this.

@frisitano frisitano force-pushed the frisitano-epilogue branch 2 times, most recently from 7995be8 to 48a2121 Compare April 3, 2023 07:49
@frisitano frisitano requested a review from bobbinth April 3, 2023 07:57
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 one comment about the nonce - once it is addressed, we should be good to merge.

Comment on lines 123 to 128
export.incr_account_nonce
padw push.ACCT_ID_AND_NONCE_PTR mem_loadw
movup.4 u32checked_add
push.ACCT_ID_AND_NONCE_PTR mem_storew dropw
end
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking we'd check only the input value (not the nonce itself). So, it would look like:

export.incr_account_nonce
    u32assert.1
    padw push.ACCT_ID_AND_NONCE_PTR mem_loadw
    movup.4 add
    push.ACCT_ID_AND_NONCE_PTR mem_storew dropw
end

I'm not sure we should restrict the nonce to be 32 bits.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

got it, makes sense!

@frisitano frisitano merged commit 48720d1 into main Apr 3, 2023
@frisitano frisitano deleted the frisitano-epilogue branch April 3, 2023 09:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants