Skip to content
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

Merged
merged 56 commits into from
Oct 15, 2024
Merged

BLAKE2b gadget #1767

merged 56 commits into from
Oct 15, 2024

Conversation

boray
Copy link
Member

@boray boray commented Jul 23, 2024

Closes #1754

Ingredients:

  • BLAKE2b hash function implementation with arbitrary digest length https://www.blake2.net/blake2.pdf
  • divMod64 and addMod64 needed for compression function
  • Gadgets.or()
  • UInt64.or()
  • Add zero and one accessors to UInt8

@boray boray marked this pull request as ready for review July 24, 2024 15:19
boray and others added 2 commits July 28, 2024 15:33
Co-authored-by: Martin Minkov <minkovlmartin@gmail.com>
@boray boray requested a review from Shigoto-dev19 July 28, 2024 18:08
Copy link
Member

@Trivo25 Trivo25 left a 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

src/examples/crypto/blake2b/blake2b.ts Outdated Show resolved Hide resolved
src/lib/provable/crypto/hash.ts Outdated Show resolved Hide resolved
src/lib/provable/gadgets/blake2b.ts Outdated Show resolved Hide resolved
src/lib/provable/gadgets/blake2b.ts Outdated Show resolved Hide resolved
src/lib/provable/gadgets/blake2b.ts Outdated Show resolved Hide resolved
src/lib/provable/gadgets/blake2b.ts Outdated Show resolved Hide resolved
src/lib/provable/gadgets/blake2b.ts Outdated Show resolved Hide resolved
src/lib/provable/gadgets/gadgets.ts Outdated Show resolved Hide resolved
src/lib/provable/test/blake2b.unit-test.ts Show resolved Hide resolved
tests/vk-regression/vk-regression.json Outdated Show resolved Hide resolved
@mitschabaude
Copy link
Collaborator

@boray since this relies on the changes in #1763, you should change the base branch to feature/divmod32-quotientbits-bound to make this easier to review

@mitschabaude mitschabaude changed the base branch from main to feature/divmod32-quotientbits-bound July 30, 2024 09:53
@mitschabaude
Copy link
Collaborator

mitschabaude commented Jul 30, 2024

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 git merge, which reuses the original commits and so doesn't create any fake conflicts

Copy link
Collaborator

@mitschabaude mitschabaude left a 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!

src/lib/provable/gadgets/arithmetic.ts Outdated Show resolved Hide resolved
src/lib/provable/gadgets/arithmetic.ts Outdated Show resolved Hide resolved
src/lib/provable/gadgets/arithmetic.ts Outdated Show resolved Hide resolved
src/lib/provable/gadgets/bitwise.ts Show resolved Hide resolved
Copy link
Member

@querolita querolita left a 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/arithmetic.ts Show resolved Hide resolved
/*
state.t[1] = state.t[1].add(
Provable.if(
state.t[0].lessThan(UInt64.from(state.buflen)),
Copy link
Member

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.

Comment on lines 15 to 18
for (let { digest_length ,preimage, hash } of testVectors()) {
let actual = Gadgets.BLAKE2B.hash(Bytes.fromString(preimage), digest_length);
expect(actual.toHex()).toEqual(hash);
}
Copy link
Collaborator

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!

Copy link
Member Author

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.

Copy link
Collaborator

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

Copy link
Member Author

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.

Copy link
Member Author

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.


type State = {
h: UInt64[];
t: UInt64[];
Copy link
Collaborator

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.

Suggested change
t: UInt64[];
t: [bigint, bigint]

Copy link
Member Author

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 Uint64s instead of an array, but would it improve efficiency?

Copy link
Collaborator

@mitschabaude mitschabaude Oct 2, 2024

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!

Comment on lines 261 to 269
/*
state.t[1] = state.t[1].add(
Provable.if(
state.t[0].lessThan(UInt64.from(state.buflen)),
UInt64.one,
UInt64.zero
)
);
*/
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove

Copy link
Member Author

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.

Copy link
Collaborator

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!

Copy link
Member Author

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?

Copy link
Collaborator

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

Copy link
Member Author

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.

Copy link
Collaborator

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!)

Copy link
Member Author

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.

Copy link
Collaborator

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!

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.
Copy link
Collaborator

@mitschabaude mitschabaude Sep 20, 2024

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

src/lib/provable/test/bitwise.unit-test.ts Outdated Show resolved Hide resolved
tests/vk-regression/vk-regression.json Outdated Show resolved Hide resolved
src/lib/provable/gadgets/blake2b.ts Show resolved Hide resolved
Copy link
Collaborator

@mitschabaude mitschabaude left a 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!

@boray boray requested a review from Trivo25 October 14, 2024 12:01
@boray boray requested a review from querolita October 14, 2024 13:31
`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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: camelCase

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed!

};

/**
* G function
Copy link
Member

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, ..

Copy link
Member Author

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.

* @returns {UInt8[]} digest
*/
function final(state: {
h: UInt64[];
Copy link
Member

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed!

* @param {boolean} last
*/
function compress(
state: {
Copy link
Member

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed!

* @returns {State} updated state
*/
function update(
state: {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same

Copy link
Member Author

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 })(
Copy link
Member

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?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@boray boray merged commit 4fcd62c into v2 Oct 15, 2024
25 checks passed
@boray boray deleted the feature/blake2b branch October 15, 2024 18:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Prototype] Implement Blake Hashing for Mina Consensus Selection in o1js Circuit
5 participants