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.
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
BLAKE2b gadget #1767
BLAKE2b gadget #1767
Changes from 19 commits
d357853
cb93722
31abb6e
2846396
9dd79aa
24c743a
044cbe1
81bc5a9
58a07a0
72877d7
10836c9
00cdb75
5db6974
63b939c
e6317f1
95a71d3
90daf62
1938405
5e2b28a
a86a106
c38118e
5bba739
e9c5a29
555070b
d071e7b
9b32c4d
70169e3
acd32bb
ec5b6a7
b741027
a34d8c2
6ba2968
d841a51
d999c09
e5bd574
1c5f3bd
7f650b1
e545799
368ae88
cb80ccf
20a0eac
cceeef5
4304c0f
567e2ac
7125718
4d897aa
5e67f23
6e469c1
3fe8f42
2dbf186
b972e7a
cd102fc
ab34d29
4e83d46
fc3aa3e
1da0ed0
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.
What's the reasoning behind
nBits
being 128 in this case? (Instead of a larger or smaller value perhaps). Is it to prevent the quotient from being larger than 64 bits upon reduction?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.
yeah if both inputs are 64 bits then the result is 65 bits, that would cause an efficient (boolean) check for the quotient
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.
so the question: can't we assume that the inputs are 64 bits and therefore use nBits=65?
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.
yes, it should be 65. It used to be 128 at some point. I adapted the changes made in #1763 and forgot to update nBits.
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.
same
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.
fixed!
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 you explain in what situations this conditional would succeed? I am asking because it seems like the counter
t[0]
gets updated using a normaladd
, whereas thet[1]
uses a modular addition instead. Are you thinking of aUInt64
overflow? If so, maybe it's better to useaddMod64
here as well.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.
btw @boray this is a bug. you can't use a
Bool
as condition, it's an object, always truthy!I highly recommend to do out-of-circuit logic on plain JS values, like bigint to avoid mistakes like this, i.e. here I would use
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.
ah but wait, is this supposed to be circuit code and
state.t[0]
is a variable? then you fundamentally can't use a JS if condition anywayThere 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.
state.t[0]
is a variable but I am not sure if that logic should be in-circuit or not. I added the in-circuit version as a comment.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 implemented this part with reference to the snippet below from the BLAKE2B RFC.
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: state type here too
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.
fixed!