-
Notifications
You must be signed in to change notification settings - Fork 11
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
Write hasher multiset check constraints in AirScript #261
base: next
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @Overcastan! I left my comments inline
constraints/miden-vm/hash.air
Outdated
# Set to 1 when selector flags are (1,0,0) on rows which are multiples of 8. This is flag of the | ||
# single permutation, a 2-to-1 hash, or a linear hash of many elements. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I would change the second sentence to "This flag indicates that we are initiating computation of a single permutation, a 2-to-1 hash, or a linear hash of many elements."
constraints/miden-vm/hash.air
Outdated
# Set to 1 when selector flags are (0,0,0) on rows which are 1 less than a multiple of 8. This is | ||
# flag of the instruction that returns the result of the currently running computation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Since it's not exactly an instruction, I would change the second sentence to "This flag indicates that the result of the currently running computation is returned."
constraints/miden-vm/hash.air
Outdated
# Set to 1 when selector flags are (0,0,1) on rows which are 1 less than a multiple of 8. This is | ||
# flag of the instruction that returns the whole hasher state. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I would change the second sentence to "This flag indicates that the entire hasher state is returned."
constraints/miden-vm/hash.air
Outdated
return cycle_row_7 & binary_not(s[0]) & binary_not(s[1]) | ||
|
||
|
||
# Flag f_out_next is set to 1 when either f_hout = 1 or f_sout = 1 in the next row. | ||
fn get_f_out_next(s: vector[3]) -> scalar: | ||
return cycle_row_6 & binary_not(s[0]') & binary_not(s[1]') | ||
return cycle_row_7 & !s[0] & !s[1] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can't actually make this change because these flags use different periodic columns
constraints/miden-vm/hash.air
Outdated
let s_next = [s_i' for s_i in s] | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This variable shouldn't be necessary - we can just pass s'
instead.
constraints/miden-vm/hash.air
Outdated
p0' = p0 * ((get_f_bp(s) + get_f_sout(s)) * v_all + (get_f_mp(s) + get_f_mv(s) + get_f_mu(s)) * | ||
v_leaf + get_f_abp(s) * v_abp + get_f_hout(s) * v_res + 1 - | ||
(get_f_bp(s) + get_f_mp(s) + get_f_mv(s) + get_f_mu(s) + get_f_abp(s) + get_f_out(s))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: almost all of these flags are computed twice, which I would probably avoid. It's not a big deal, though, since it will be optimized by the compiler anyway.
Let's also add a comment with the constraint degree
constraints/miden-vm/hash.air
Outdated
|
||
|
||
# Enforce constraints for the hash chiplet sibling table. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I would change this to: "Enforce constraints for the hash chiplet sibling table, which is used to keep track of sibling nodes used during Merkle root update computations." I would probably even add the next bit from the docs with the 2 rules that need to be enforced.
constraints/miden-vm/hash.air
Outdated
# TODO: Apply multiset check constraints No newline at end of file | ||
# TODO: Apply multiset check constraints | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- The new helper evaluators should go above in the helper evaluator section so that the
hash_chiplet
evaluator is still the last thing in the file as we've done in other places. - We should add calls to the two new evaluators here and remove this TODO
constraints/miden-vm/hash.air
Outdated
enf p1' * ((get_f_mv(s) + get_f_mva(s)) * v_sibling + 1 - (get_f_mv(s) + get_f_mva(s))) = | ||
p1 * ((get_f_mu(s) + get_f_mua(s)) * v_sibling + 1 - (get_f_mu(s) + get_f_mua(s))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's add a comment with the constraint degree
constraints/miden-vm/hash.air
Outdated
# Enforce that computation of the old Merkle root is immediately followed by the computation of | ||
# the new Merkle root. | ||
enf (get_f_bp(s) + get_f_mp(s) + get_f_mv(s) * (1 - p1)) = 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The paren is in the wrong place here. This should be:
enf (get_f_bp(s) + get_f_mp(s) + get_f_mv(s)) * (1 - p1) = 0
Let's also add a comment with the constraint degree.
# Constraints degree: 7 | ||
enf is_unchanged([s[1]]) when !f_out & !f_out_next | ||
enf is_unchanged([s[2]]) when !f_out & !f_out_next |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there is a typo in the docs regarding the degree of these constraints (imho it should be 7, not 9). The f_out
flag has degree 3, not 4, since it is computed using only one periodic column and two selectors (as opposed to f_hout
and f_sout
flags, which use three selectors)
# Constraint degree: 4 | ||
enf f_out * i = 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar comment to the one above: I think the degree of this constraint should be 4, not 5.
constraints/miden-vm/hash.air
Outdated
# TODO: Apply row address constraints: | ||
# 1. Boundary constraint `enf r.first = 1` | ||
# 2. Transition constraint. It requires chiplets module's selector flag s0. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can add this transition constraint, I'm just not sure where we should place chip_s0
chiplet module's selector flag -- to the first (main
) segment, or to the second (aux
).
a42f7bd
to
c8b3431
Compare
Addressing: #200
This PR adds constraints for the hash chiplet bus and hash chiplet sibling table.