Introduce Horner evaluation ops #1656
Conversation
|
|
||
|  | ||
|  |
There was a problem hiding this comment.
This file doesn't exist yet right? I still see RCOMBASE.png but no HORNERBASE.png
| #rcomb_base | ||
| push.0 drop |
There was a problem hiding this comment.
I'm assuming you'll update this after we merge #1658?
There was a problem hiding this comment.
I am now working, in a separate PR against this branch, on updating Falcon and it should be done today. I think it is better if we hold off on merging this one until then.
| @@ -68,7 +68,7 @@ impl AuxTraceBuilder { | |||
|
|
|||
| debug_assert_eq!(*t_chip.last().unwrap(), E::ONE); | |||
| // TODO: Fix and re-enable after testing with miden-base | |||
| // debug_assert_eq!(*b_chip.last().unwrap(), E::ONE); | |||
| debug_assert_eq!(*b_chip.last().unwrap(), E::ONE); | |||
There was a problem hiding this comment.
Does this mean miden-base tests now pass? Was the bug only with the RCOMBBASE instruction then?
There was a problem hiding this comment.
All tests passed with the above line uncommented for RCOMBBASE and I uncommented that line to test for the correctness of the implementation for HORNER* ops.
I will put the comment back as I haven't tested with miden-base
docs/src/design/stack/crypto_ops.md
Outdated
| The effect on the rest of the stack is: | ||
| * **No change.** | ||
|
|
||
| > TODO: add detailed constraint descriptions. |
There was a problem hiding this comment.
Are we doing this in this PR?
There was a problem hiding this comment.
#1660
I am thinking this might be implemented directly in AirScript
There was a problem hiding this comment.
Then I would remove this TODO from the docs, as they tend to stick around, and it makes for cleaner docs not to have any in there
Al-Kindi-0
force-pushed
the
al-introduce-horner-eval-ops
branch
from
86cddd7 to
3c6f139
Compare
| alphas, | ||
| row, | ||
| eval_point_ptr, | ||
| [eval_point_0, eval_point_1, ZERO, ZERO], |
There was a problem hiding this comment.
An insight gained from #1664 is that the RCOMBBASE request would fail to match the response by the memory chiplet when the positions 2 and 3 here are not ZERO, ZERO. This happened specifically in the falcon signature code. I didn't investigate more, but my assumption is that memory was reused (probably with the use of procedure locals), and that there are leftover garbage values in those positions.
Hence the fix is to fully read the memory word in the helper registers and include it in the bus message. This learning applies generally too: when we read a word, we must read it entirely and include it all in the bus message, since the "zeroed-out memory" assumption doesn't always hold.
Ideally, we'd add a test also to check that when there are non-zero values in positions 2 & 3, that the bus doesn't fail.
EDIT: Upon closer examination, I don't believe the values in memory from the miden-base are garbage. Since this code is changing with #1661, I will stop investigating; we can see if somehow the problem was fixed when switching to horner base eval.
There was a problem hiding this comment.
Hmmm, thinking more, the larger problem in miden-base seems more to be that the data in the advice provider is wrong, such that the pipe doesn't insert 0s where there should be some.
Regardless, it's probably safer to fully read the word anyway instead of assuming the memory was zero'd in those positions.
There was a problem hiding this comment.
Looks good! Sorry for such a delayed review! I left one comment inline about potentially changing how we describe things in the doc comments - the code itself looks fine.
Also, I think we need to merge the latest next into this branch as the file structure for the auxiliary trace generation has changed a bit.
Al-Kindi-0
force-pushed
the
al-introduce-horner-eval-ops
branch
from
cec1812 to
6fcd075
Compare
Al-Kindi-0
force-pushed
the
al-introduce-horner-eval-ops
branch
from
6fcd075 to
3402f34
Compare
Thank you! I think this is good to merge. |
|
I merged this, but now that I think of it, this probably broke the Falcon signature verification (at least until #1661 is merged). |
|
Indeed! |
Closes #1600