This repository has been archived by the owner on Jul 5, 2024. It is now read-only.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Implement
ADDMOD
#564Implement
ADDMOD
#564Changes from 6 commits
1ca4427
5ad56ea
92f45d9
a70aaba
fe85380
46994df
b4cee20
13cd65a
4ff67ef
e6ee7e5
85d8db3
ed61005
f67abb0
7e5a3f6
7c24153
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 overflow bit should come first
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.
Ok, bad news, all tests passes both for
[d, n, a_reduced_plus_b, a_reduced_plus_b_overflow]
and[d, n, a_reduced_plus_b_overflow, a_reduced_plus_b]
🧐
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.
oh... We need to figure out why! 🕵️♀️
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 have a test case missing!
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.
But the missing test is not related...
@davidnevadoc, the
MulAdd512WordGadgets
is ok with the following witnessMulAddWords512Gadget([1,10,12,0], Some(2))
could you check it? Maybe I'm wrong somewhere but I am not able to find it
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 I found the reason why this works when passing swapped
d
ande
in the assign function ofMulAddWords512Gadget
.On one hand,
MulAddWords512Gadget
only assigns to its internal witness which arecarry_0
,carry_1
andcarry_2
. And those values are calculated froma, b, c, d, e
. If I remember correctly, for ADDMOD we only needed 257 bit arithmetic, but we're usingMulAddWords512Gadget
for convenience. This means that the possible values we will encounter for the carry witnesses is more limited (I think they are always 0 or 1). And it seems that swappingd
ande
(using values from ADDMOD), the calculated carry witnesses stay the same! Or at least that's the case with the tests.I'm not sure if there's a case where swapping
d
ande
in the assign function forMulAddWords512Gadget
could change the carry witnesses.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 see what @davidnevadoc says, @ed255 !
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've tried to find some combination of inputs that calculates in different carry witnesses when swapping
d
ande
, but I didn't find any. It's a bit tricky because at this point the inputs are derived from other calculations. So even after trying I'm not convinced that such inputs don't exist.Anyway, I think we can merge this PR even if this is not yet resolved!
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 reason for this is what @ed255 said plus the fact that we are using
saturating_sub
inassign
.Turns out that in this example the correct values for the 3 carry are
0
. Whend
ande
are swapped, the computation for these carries would result in an underflow and a panic. Instead of this, the use ofsaturating_sub
gives a0
(the correct value by chance).The constraints pass because the rest of the cells (
d
ande
included) are placed correctly.