-
Notifications
You must be signed in to change notification settings - Fork 135
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
Conversation
Co-authored-by: Martin Minkov <minkovlmartin@gmail.com>
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.
Great job! I left some initial comments after a first pass
I changed the base branch, which reveals that you didn't merge that branch but added it as a single squashed commit. I highly recommend to never do that! it means we have the same changes in two different commits, which git can't resolve and instead will mark them as conflicts. (see "This branch has conflicts that must be resolved" on this PR now) the preferred way to use git at o1Labs is to basically always join branches using |
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.
a few quick comments on the gadgets additions!
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.
Leaving two more comments before acceptance. These are related to the actual constraints being created behind the scenes.
src/lib/provable/gadgets/blake2b.ts
Outdated
/* | ||
state.t[1] = state.t[1].add( | ||
Provable.if( | ||
state.t[0].lessThan(UInt64.from(state.buflen)), |
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.
If this needs to go in the circuit, then the same lines of the update
part should also be written in circuit manner. In any case, would it make sense to have a final constrain checking t[1]*2^64+t[0]=total_length
? That, together with the modular additions gives you a correct decomposition.
Now, going back to what @mitschabaude said about the state.t[0]
being a variable, one cannot decide the design of the circuit depending on the value of a witness cell. The circuit must have the same "universal shape", regardless of the concrete values of the variables.
So putting all the above together, it's not a matter of whether that logic needs to go into the circuit or not. It's about not being able to decide upon the concrete value a variable takes. Thus, I believe this part should be rewritten, perhaps with the use of Provable.if
(but I might be missing the right o1js know-how, so maybe ask another team member for further insights).
At least in Kimchi, one would create a circuit that is already "bifurcated". Meaning, all the constraints hold and they are the same, no matter what concrete values are passed to the prover. So we play with multiplications to "select" one branch or another, but both sides should eventually evaluate to 0 to satisfy the constraint system.
for (let { digest_length ,preimage, hash } of testVectors()) { | ||
let actual = Gadgets.BLAKE2B.hash(Bytes.fromString(preimage), digest_length); | ||
expect(actual.toHex()).toEqual(hash); | ||
} |
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.
these tests should run in a circuit!
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 added in-circuit tests in the last commit.
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 see it, but there were two misunderstandings:
- I didn't mean to run them in a ZkProgram with proving, but just in a circuit with checking of constraints. You can use
runAndCheckSync()
for that like in other test files - I meant run the same test vectors in a circuit, not a small number of random inputs like you did
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.
Got it. I added a test that uses equivalentProvable()
to run on test vectors, which revealed a mismatch in the 10k-byte preimage. I’m investigating the why it's happening.
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.
sorry, it's a false alarm. It works with 10k-byte preimage too.
src/lib/provable/gadgets/blake2b.ts
Outdated
|
||
type State = { | ||
h: UInt64[]; | ||
t: UInt64[]; |
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.
as far as I can tell, t
never contains variables, just constants that change depending on the input size (which is also constant)
therefore, it's better to change the type to bigint or number, to avoid confusion with variables (see other comments where we were confused):
Also, nicer to use a fixed-size array since it's just two elements.
t: UInt64[]; | |
t: [bigint, bigint] |
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.
no, t
is set to [Uint64.zero, Uint64.zero]
at initialisation. t[0]
is incremented by buflen
during every update()
and final()
. However, t[1] is only used when input size is larger than 2^64 bytes which is unlikely. We can brake up the t
into two Uint64
s instead of an array, but would it improve efficiency?
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.
buflen
is not a variable though, it only depends on the input length
so I still claim you can and should use bigints for it. initialize them to [0n, 0n]
.
We can brake up the t into two Uint64s instead of an array, but would it improve efficiency?
the suggestion to change the type has nothing to do with efficiency, just with making the type more precise. but my main point was to change the UIn64 to bigint!
src/lib/provable/gadgets/blake2b.ts
Outdated
/* | ||
state.t[1] = state.t[1].add( | ||
Provable.if( | ||
state.t[0].lessThan(UInt64.from(state.buflen)), | ||
UInt64.one, | ||
UInt64.zero | ||
) | ||
); | ||
*/ |
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.
remove
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 was confused because the non-provable version of this part didn’t raise any errors. But, I believe this logic needs to be provable.
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 believe it doesn't!
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 gadget compiles with both. Is there a way to test our beliefs?
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 there's a way to find out, make 't' consist of bigints, if you still can make everything work without issues then t never interacts with a variable, and so stays constant, and so the logic isn't in-circuit
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 did that and everything worked but I worry the proof generation would fail when the case t[1] is changed reached.
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.
then you need to make sure that this case is tested (should be ensured anyway!)
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.
To test this case, we need to hash an input larger than 2^64 bytes which is not possible for now. Please share if you have an idea to test.
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 right. Then that case doesn't matter :D
But again, I went through the code a while ago and concluded that t0 / t1 at different steps of the circuit only depend on the constant input size.
So they are inherently constant. A constant is just a wrapper around a bigint, and your "provable logic" applied to a constant just produces another constant (= another bigint), and is not proved at all. So it's all the same when you do those operations in plain JS. You're just making it more explicit and easier to see that they are on constants.
It's fine!
src/lib/provable/gadgets/gadgets.ts
Outdated
divMod64, | ||
|
||
/** | ||
* Addition modulo 2^64. The operation adds two {@link Field} elements in the range [0, 2^128] and returns the result modulo 2^64. |
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.
first of all, all ranges are always of the form [a, b)
i.e. open on the upper end
second, 128 bits is wrong. it uses a 1-bit range check on the quotient. for that to work, the addition result has to be at most 65 bits
also, you say further below that "the gadget assumes both inputs to be in the range [0, 2^64)". that will definitely work and is the intended use case, so stick with that here in the description 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.
all my comments are addressed as far as I can see. great job @boray!
src/lib/provable/gadgets/blake2b.ts
Outdated
`data byte length must be in the range [0, 2**128), got ${data.length}` | ||
); | ||
const state = initialize(digestLength); | ||
const updated_state = update(state, Bytes.from(data).bytes); |
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: camelCase
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!
src/lib/provable/gadgets/blake2b.ts
Outdated
}; | ||
|
||
/** | ||
* G function |
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.
Since this isn't a publicly exposed function, I think we can remove this doc comment or describe what the function is doing, typescript already gives us the expected types, hence we don't need .., @param {UInt64[]} v, ..
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 removed this doc comment instead of describing the function’s behaviour, as a better description exists in the BLAK2B RFC referenced in the first line.
src/lib/provable/gadgets/blake2b.ts
Outdated
* @returns {UInt8[]} digest | ||
*/ | ||
function final(state: { | ||
h: UInt64[]; |
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!
src/lib/provable/gadgets/blake2b.ts
Outdated
* @param {boolean} last | ||
*/ | ||
function compress( | ||
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: You define a State
type above, might as well use 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.
fixed!
src/lib/provable/gadgets/blake2b.ts
Outdated
* @returns {State} updated state | ||
*/ | ||
function update( | ||
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.
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!
@@ -160,82 +170,6 @@ await equivalentAsync({ from: [maybeField], to: field }, { runs })( | |||
return proof.publicOutput; | |||
} | |||
); | |||
await equivalentAsync({ from: [maybeField], to: field }, { runs })( |
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.
why are we getting rid of all these unit tests?
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.
Closes #1754
Ingredients:
divMod64
andaddMod64
needed for compression functionGadgets.or()
UInt64.or()