-
Notifications
You must be signed in to change notification settings - Fork 174
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
Update Falcon verification to use horner_eval_base
#1661
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.
LGTM! Great job 👏
Left a few nits
stdlib/tests/crypto/falcon.rs
Outdated
#[test] | ||
fn test_falcon512_probabilistic_product_failure() { | ||
// Create a polynomial pi that is not equal to h * s2. | ||
// create a polynomial pi that is not equal to h * s2. |
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 random_coefficients
use rng - can we either fix a seed, or generate those coefficients deterministically?
adv_push.2 | ||
dup.1 dup.1 ext2inv | ||
push.0.0 | ||
loc_storew.4 |
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: can we add the state of the stack after this block?
@@ -218,182 +207,83 @@ export.load_h_s2_and_product.4 | |||
push.M u32lt assert | |||
push.M u32lt assert | |||
|
|||
horner_eval_base |
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.
can we also add some comments for the state of the stack in this loop? It's quite hard to follow
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 ran the miden-base
tests on top of this branch, and I'm getting some NotU32
errors (for example, with the scripts::p2id::prove_consume_multiple_notes
test).
Would be good to make sure the bus works fine too after we fix those (and #1664 might be useful if there's a bug there)
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.
My bad, miden-base
reimplements the dsa::falcon_sign
method which was updated in this PR. After updating it, all tests pass.
I also confirmed that the bus is fixed with this PR, which confirms the findings of #1664 that rcombbase was the problem.
cec1812
to
6fcd075
Compare
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.
Looks good! Thank you! I left a few comments inline - most are pretty small.
One other thing, before merging this PR we'd need to update |
6fcd075
to
3402f34
Compare
Indeed, we have now changed how and what advice is provided for the signature. |
66cfab3
to
4c3f3a4
Compare
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.
Looks good! Thank you! I left some more comments inline - these are mostly doc-related.
Also, would you mind making a parallel PR in miden-base
?
Sure thing, will open one soon |
a037d70
to
f330f6c
Compare
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.
Looks good! Thank you! I left one small comment inline. After it is addressed, let's merge.
processor/src/host/dsa.rs
Outdated
/// 1. The challenge point at which we evaluate the three aforementioned polynomials to check the | ||
/// product relationship. |
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'm not sure the word "aforementioned" is correct here. Also, I would specify that the challenge point is 2 field 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.
Indeed, this was from the previous order of the bullet points. Changed it now and updated the comment.
7e72ef6
to
1b82170
Compare
Signed-off-by: Al-Kindi-0 <82364884+Al-Kindi-0@users.noreply.github.com>
1b82170
to
0891f41
Compare
As the title says. The current cycle count is around
57500
cycles.