[frontend] Add Blake2s circuit by fkondej · Pull Request #757 · binius-zk/binius64

fkondej · 2025-08-22T00:51:51Z

No description provided.

linear · 2025-08-22T00:51:53Z

ENG2-205 Blake2s (and Blake3) Circuit

fkondej · 2025-08-22T00:52:07Z

[frontend] Add Blake2s circuit #757 👈 (View in Graphite)
main

How to use the Graphite Merge Queue

Add the label merge-ready to this PR to add it to the merge queue.

You must have a Graphite account in order to use the merge queue. Sign up using this link.

_{An organization admin has enabled the Graphite Merge Queue in this repository.} _{Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.}

This stack of pull requests is managed by Graphite. Learn more about stacking.

jadnohra · 2025-08-26T16:20:17Z

There is no support for keyed MAC mode (Blake2s-MAC). I suppose this is OK? I would check with Jim.

jadnohra

Added comments, please fix @fkondej

onesk · 2025-08-28T21:26:38Z

+//! - **Variable output length**: Fixed at 256 bits (32 bytes).
+//!
+//! These exclusions are appropriate for a ZK circuit focused on hash verification
+//! rather than general-purpose hashing. For details, see RFC_7693_COMPLIANCE_REPORT.md.


Referencing a Markdown document not in the repo.

onesk · 2025-08-28T21:35:07Z

+		// Initialize hash state with Blake2s-256 parameters
+		// h[0] = IV[0] ^ 0x01010020 (param block: digest_length=32, fanout=1, depth=1)
+		let init_state = [
+			builder.add_constant(Word((IV[0] ^ 0x01010020) as u64)),


IMO this is a perfect candidate for add_constant_64

should I extract it as a function in this PR?

onesk · 2025-08-28T21:41:37Z

+							builder.shl(byte_wire, (byte_idx * 8) as u32)
+						};
+
+						word = builder.bor(word, shifted);


I am almost sure this can be a XOR instead (the bit ranges are disjoint). XOR is allegedly cheaper.

onesk · 2025-08-28T21:44:32Z

+
+			// Finalization flag: apply mask to set appropriately
+			let flag_value = builder.add_constant(Word(0xFFFFFFFF));
+			let last_flag = builder.band(is_final_block, flag_value);


Using select here is more future proof. The same thing applies to all band usages above. The rewritten form should be something like:

builder.select(is_final_block, flag_value, zero);

onesk · 2025-08-28T21:48:41Z

+			let counter_if_final = builder.band(is_final_block, length);
+			let counter_if_not_final = builder.band(builder.bnot(is_final_block), block_counter);
+			let t_lo = builder.bxor(counter_if_final, counter_if_not_final);
+			let t_hi = zero;


This implicitly assumes less than 4 GiB hashed data, which is reasonable for a circuit, but needs to be mentioned in the comments.

onesk · 2025-08-28T21:51:56Z

+					let global_byte_idx = byte_offset + byte_idx;
+					if global_byte_idx < max_bytes {
+						// Get the byte wire (or zero if beyond message length)
+						let byte_wire = if global_byte_idx < message.len() {


There should be a similar constraint but against length via icmp_ult, as RFC7693 requires the m array to be zeroed at the end of the final block, and this is currently not enforced. If someone tries to hash data with nonzero bytes in message beyond length they'll be in for a surprise.

inb4: I know that populate_message zeroes this bytes out; we are talking a potential soundness bug here.

onesk

Message handling needs to be improved, otherwise this seems to follow RFC7693 to the very letter.

graphite-app · 2025-08-29T15:17:25Z

+						let byte_wire = if global_byte_idx < message.len() {
+							message[global_byte_idx]
+						} else {
+							builder.add_constant(Word(0))
+						};


The condition global_byte_idx < message.len() will always evaluate to true because message.len() equals max_bytes (from line 296 where the message vector is created with exactly max_bytes elements). This means the else branch is unreachable dead code.

The condition should check against max_bytes instead:

let byte_wire = if global_byte_idx < max_bytes { message[global_byte_idx] } else { builder.add_constant(Word(0)) };

This would properly handle the case where padding with zeros is needed beyond the message array bounds.

Suggested change

let byte_wire = if global_byte_idx < message.len() {

message[global_byte_idx]

} else {

builder.add_constant(Word(0))

};

let byte_wire = if global_byte_idx < max_bytes {

message[global_byte_idx]

} else {

builder.add_constant(Word(0))

};

Spotted by Diamond

Is this helpful? React 👍 or 👎 to let us know.

graphite-app · 2025-08-29T15:17:27Z

+            cargo run --release --example blake2s -- --message-len 64\n\
+            \n\
+            Test with maximum message length:\n\
+            cargo run --release --example blake2s -- --max-message-len 256 --message-len 256",


The parameter name in the help text (--max-message-len) doesn't match the actual parameter defined in the Params struct (--max-bytes). This inconsistency will cause confusion for users trying to follow the examples. Consider updating the help text to use --max-bytes to match the actual parameter name:

cargo run --release --example blake2s -- --max-bytes 256 --message-len 256

Suggested change

cargo run --release --example blake2s -- --max-message-len 256 --message-len 256",

cargo run --release --example blake2s -- --max-bytes 256 --message-len 256",

Spotted by Diamond

Is this helpful? React 👍 or 👎 to let us know.

graphite-app · 2025-09-01T15:48:34Z

+				"606beeec8ccdc32c8c8c3c18afc8ff8a3f42fb3bdbde4d823d3d3e2323232323"
+			),


The expected hash value for 'The quick brown fox jumps over the lazy dog' is incorrect in this test vector. The value should be:

"606beeec743ccbeff6cbcdf5d5302aa855c256c29b88c8ed331ea1a6bf3c8812"

This matches the correct value already defined in PATTERN_VECTORS in test_vectors.rs. Using inconsistent test vectors could lead to confusing test failures.

Suggested change

"606beeec8ccdc32c8c8c3c18afc8ff8a3f42fb3bdbde4d823d3d3e2323232323"

),

"606beeec743ccbeff6cbcdf5d5302aa855c256c29b88c8ed331ea1a6bf3c8812"

),

Spotted by Diamond

Is this helpful? React 👍 or 👎 to let us know.

graphite-app · 2025-09-01T16:52:20Z

+            cargo run --release --example blake2s -- --message-len 64\n\
+            \n\
+            Test with maximum message length:\n\
+            cargo run --release --example blake2s -- --max-message-len 256 --message-len 256",


There's an inconsistency between the argument name defined in the Params struct (--max-bytes on line 21) and what's referenced in the help text examples (--max-message-len on line 114). This will cause confusion when users try to follow the examples from the help text.

To fix this, please update line 114 to use --max-bytes instead of --max-message-len to match the actual parameter name defined in the code.

cargo run --release --example blake2s -- --max-bytes 256 --message-len 256

Suggested change

cargo run --release --example blake2s -- --max-message-len 256 --message-len 256",

cargo run --release --example blake2s -- --max-bytes 256 --message-len 256",

Spotted by Diamond

Is this helpful? React 👍 or 👎 to let us know.

lockedloop · 2025-09-01T17:40:26Z

Blake2s Benchmark Parameters:
  Circuit capacity: 131072 bytes (2048 blocks × 64 bytes/block)
  Message length: 131072 bytes (using full capacity)
  Note: Circuit size is dynamic based on max_bytes parameter
=======================================

Benchmarking blake2s_witness_generation/bytes_131072_arm64_neon_aes: Collecting 100 samples in estimablake2s_witness_generation/bytes_131072_arm64_neon_aes
                        time:   [17.968 ms 18.028 ms 18.094 ms]
                        thrpt:  [6.9083 MiB/s 6.9336 MiB/s 6.9567 MiB/s]
                 change:
                        time:   [−2.6608% −2.1187% −1.5337%] (p = 0.00 < 0.05)
                        thrpt:  [+1.5576% +2.1645% +2.7336%]
                        Performance has improved.
Found 5 outliers among 100 measurements (5.00%)
  4 (4.00%) high mild
  1 (1.00%) high severe

Benchmarking blake2s_proof_generation/bytes_131072_arm64_neon_aes: Collecting 100 samples in estimateblake2s_proof_generation/bytes_131072_arm64_neon_aes
                        time:   [452.20 ms 493.01 ms 536.00 ms]
                        thrpt:  [238.81 KiB/s 259.63 KiB/s 283.06 KiB/s]
                 change:
                        time:   [−17.175% −5.3693% +10.153%] (p = 0.43 > 0.05)
                        thrpt:  [−9.2176% +5.6740% +20.737%]
                        No change in performance detected.
Found 4 outliers among 100 measurements (4.00%)
  4 (4.00%) high mild

Benchmarking blake2s_proof_verification/bytes_131072_arm64_neon_aes: Warming up for 3.0000 s
Warning: Unable to complete 100 samples in 5.0s. You may wish to increase target time to 11.9s, or reduce sample count to 40.
Benchmarking blake2s_proof_verification/bytes_131072_arm64_neon_aes: Collecting 100 samples in estimablake2s_proof_verification/bytes_131072_arm64_neon_aes
                        time:   [118.89 ms 119.31 ms 119.76 ms]
                        thrpt:  [1.0437 MiB/s 1.0477 MiB/s 1.0514 MiB/s]
                 change:
                        time:   [−3.9458% −3.4996% −3.0288%] (p = 0.00 < 0.05)
                        thrpt:  [+3.1234% +3.6265% +4.1079%]
                        Performance has improved.
Found 1 outliers among 100 measurements (1.00%)
  1 (1.00%) high mild



Blake2s proof size for 131072 bytes message: 407792 bytes

witness gen + proving ~0.5sec on my mac, stwo prover on my mac is ~1.3s for the same thing

graphite-app · 2025-09-01T17:42:46Z

+		let bench_name = format!("bytes_{}_{}", max_bytes, feature_suffix);
+		group.bench_with_input(BenchmarkId::from_parameter(&bench_name), &max_bytes, |b, _| {
+			b.iter(|| {
+				let mut prover_transcript = ProverTranscript::new(StdChallenger::default());


The Pseudo-Random Testing rule requires using StdRng::seed_from_u64 for reproducible tests, but this benchmark code uses StdChallenger::default() which likely uses non-deterministic randomness. For benchmarks, the rule allows using thread RNG rand::rng(), but the current code should be updated to explicitly use rand::rng() or a seeded RNG for consistency with the testing guidelines.

Spotted by Diamond (based on custom rule: Monbijou Testing Patterns)

Is this helpful? React 👍 or 👎 to let us know.

graphite-app · 2025-09-01T17:53:33Z

+		let reference = hasher.finalize();
+
+		// Only test if our expected matches reference (to avoid bad test vectors)
+		if reference.as_slice() == vector.expected {


This test logic may inadvertently mask implementation bugs by only running when the reference implementation matches the expected vector. If there's a discrepancy between the reference implementation and the test vector, the test silently skips instead of failing.

Consider restructuring to first assert that the reference matches the expected value:

assert_eq!( reference.as_slice(), vector.expected, "Reference implementation doesn't match expected test vector for {}", vector.description ); // Then proceed with circuit verification

This approach would properly validate both the reference implementation and the circuit against known test vectors.

Suggested change

if reference.as_slice() == vector.expected {

assert_eq!(

reference.as_slice(),

vector.expected,

"Reference implementation doesn't match expected test vector for {}",

vector.description

);

Spotted by Diamond

Is this helpful? React 👍 or 👎 to let us know.

graphite-app · 2025-09-02T10:01:34Z

Merge activity

Sep 2, 10:01 AM UTC: onesk added this pull request to the Graphite merge queue.
Sep 2, 10:01 AM UTC: CI is running for this pull request on a draft pull request (#914) due to your merge queue CI optimization settings.
Sep 2, 10:09 AM UTC: Merged by the Graphite merge queue via draft PR: #914.

fkondej force-pushed the fkondej/eng2-205-frontend-blake2s branch 2 times, most recently from 36a5314 to 1b3cc02 Compare August 22, 2025 07:55

fkondej marked this pull request as ready for review August 22, 2025 15:34

graphite-app bot reviewed Aug 22, 2025

View reviewed changes