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

feat: Set transaction expiry block delta #897

Merged
merged 12 commits into from
Sep 30, 2024
Merged

feat: Set transaction expiry block delta #897

merged 12 commits into from
Sep 30, 2024

Conversation

igamigo
Copy link
Collaborator

@igamigo igamigo commented Sep 25, 2024

Closes #354
Remaining work is testing that ExecutedTransaction and ProvenTransaction get the expected block expiration number, and addressing the questions I left in the comments.

@igamigo igamigo changed the title feat: Set transaction recency conditions feat: Set transaction expiry block delta Sep 25, 2024
miden-lib/asm/kernels/transaction/lib/tx.masm Outdated Show resolved Hide resolved
Comment on lines 335 to 340
exec.get_absolute_expiration_block movdn.8

exec.::std::sys::truncate_stack
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I wasn't sure why I was getting an address overflow error after executing (since some lines above, half of the stack is dropped anyway), but adding this fixed it so I was unblocked.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I also wasn't sure what ordering we would prefer on the stack so I pushed it near the end until I had confirmation of what would be better.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the stack order you have upon return is fine (i.e., [OUTPUT_NOTES_COMMITMENT, FINAL_ACCOUNT_HASH, tx_expiration_block_num]) - but let's update docs on line 253 above to reflect this.

As for truncating the stack, the reason why it happens is that get_absolute_expiration_block pushes an item onto the stack which already has 16 items. So, at the end, we end with 17 items, which results in an overflow. (the swapdw dropw dropw reduces stack depth to 16, because stack depth cannot drop below 16).

However, I would probably not use std::sys::truncate_stack here. Instead, the following would work better (i.e., would take much fewer cycles):

swapdw
exec.get_absolute_expiration_block
swap drop swapdw

Separately, I'm actually wondering if we need swapdw dropw dropw on line 331. Could you try removing these and see if things still work? If it doesn't work, we enter this procedure with a dirty stack and we should probably understand why. In this case, let's create an issue for this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Seems like it does not work without the swapdw dropw dropw.

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's create an issue to investigate this in the future.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Created #902

@igamigo igamigo requested review from bobbinth, Fumuran and phklive and removed request for bobbinth September 25, 2024 19: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 good! Thank you! I left some comments inline. Most are pretty minor, and once they are addressed, we can merge.

miden-lib/asm/kernels/transaction/lib/epilogue.masm Outdated Show resolved Hide resolved
Comment on lines 335 to 340
exec.get_absolute_expiration_block movdn.8

exec.::std::sys::truncate_stack
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the stack order you have upon return is fine (i.e., [OUTPUT_NOTES_COMMITMENT, FINAL_ACCOUNT_HASH, tx_expiration_block_num]) - but let's update docs on line 253 above to reflect this.

As for truncating the stack, the reason why it happens is that get_absolute_expiration_block pushes an item onto the stack which already has 16 items. So, at the end, we end with 17 items, which results in an overflow. (the swapdw dropw dropw reduces stack depth to 16, because stack depth cannot drop below 16).

However, I would probably not use std::sys::truncate_stack here. Instead, the following would work better (i.e., would take much fewer cycles):

swapdw
exec.get_absolute_expiration_block
swap drop swapdw

Separately, I'm actually wondering if we need swapdw dropw dropw on line 331. Could you try removing these and see if things still work? If it doesn't work, we enter this procedure with a dirty stack and we should probably understand why. In this case, let's create an issue for this.

miden-lib/asm/kernels/transaction/lib/memory.masm Outdated Show resolved Hide resolved
miden-lib/asm/miden/kernel_proc_offsets.masm Outdated Show resolved Hide resolved
miden-lib/src/transaction/mod.rs Outdated Show resolved Hide resolved
miden-lib/src/transaction/mod.rs Outdated Show resolved Hide resolved
miden-lib/src/transaction/mod.rs Outdated Show resolved Hide resolved
@igamigo igamigo marked this pull request as ready for review September 27, 2024 16:31
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 couple comments inline. The main one is about not returning the delta from the "set delta" procedures and instead adding a procedure to the kernel for getting the delta at any point in time.

miden-lib/asm/kernels/transaction/lib/prologue.masm Outdated Show resolved Hide resolved
miden-lib/asm/kernels/transaction/api.masm Outdated Show resolved Hide resolved
miden-lib/asm/kernels/transaction/lib/tx.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 some more comments inline. Also, seems like the tests are now failing?

miden-lib/asm/kernels/transaction/api.masm Outdated Show resolved Hide resolved
miden-lib/asm/kernels/transaction/lib/tx.masm Outdated Show resolved Hide resolved
miden-lib/asm/kernels/transaction/lib/tx.masm Outdated Show resolved Hide resolved
miden-lib/asm/kernels/transaction/lib/tx.masm Outdated Show resolved Hide resolved
miden-lib/asm/kernels/transaction/lib/tx.masm Outdated Show resolved Hide resolved
@igamigo
Copy link
Collaborator Author

igamigo commented Sep 29, 2024

Also, seems like the tests are now failing?

Tests related to transactions now started failing with verification errors: TransactionVerificationFailed(VerifierError(InconsistentOodConstraintEvaluations)). This is related to the line I added on the prologue for initializing the transaction expiration number because if I comment that out, tests pass again (but not sure what the error means).

@bobbinth
Copy link
Contributor

Tests related to transactions now started failing with verification errors: TransactionVerificationFailed(VerifierError(InconsistentOodConstraintEvaluations)). This is related to the line I added on the prologue for initializing the transaction expiration number because if I comment that out, tests pass again (but not sure what the error means).

This error usually means that there may be some kind of a mismatch in public inputs between proof generation and verification. Most likely this is happening because we put an extra value on the output stack and during verification we expect that value to be $0$ (but now it is u32::MAX). If that's the case, commenting out new lines in the epilogue should also get rid of this error.

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 couple of very minor nits inline. After these, we can merge.

Also, let's make sure we have PRs on the node/client needed to make sure this change doesn't break them.

miden-lib/src/transaction/mod.rs Outdated Show resolved Hide resolved
@@ -160,8 +161,13 @@ impl TransactionKernel {
.expect("Invalid stack input")
}

pub fn build_output_stack(final_acct_hash: Digest, output_notes_hash: Digest) -> StackOutputs {
pub fn build_output_stack(
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's add doc comments to this function.

miden-lib/src/transaction/mod.rs Outdated Show resolved Hide resolved
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.

2 participants