Add Semaphore over ECDSA by jadnohra · Pull Request #911 · binius-zk/binius64

jadnohra · 2025-09-02T09:10:11Z

Add Semaphore ECDSA Circuit Implementation

This PR implements a Semaphore anonymous group membership proof system using ECDSA key derivation on secp256k1.

Circuit Hierarchy

SemaphoreProofECDSA
├─ IN: message (Vec) - Public message being signed
├─ IN: scope (Vec) - Public scope/context for nullifier
├─ IN: merkle_root ([u8; 32]) - Public Merkle tree root
├─ OUT: nullifier ([u8; 32]) - Public nullifier (prevents double-spending)
├─ AUX: secret_scalar ([u8; 32]) - Private ECDSA key (witness)
├─ AUX: merkle_siblings (Vec<[u8; 32]>) - Merkle proof path (witness)
├─ AUX: leaf_index (usize) - Position in tree (witness)
└─ Internal Components:
├─ Secp256k1ScalarMult: secret_scalar → (pubkey_x, pubkey_y)
├─ Keccak256: (pubkey_x || pubkey_y) → commitment
├─ Keccak256: (scope || secret_scalar) → nullifier
└─ MerkleProof: (commitment, siblings, index) → root_verification

Example Usage

The semaphore_ecdsa example demonstrates the circuit with configurable parameters:

Circuit Parameters

--tree-height <N> (default: 2) - Merkle tree height, determines max group size (2^N members)
--message-len-bytes <N> (default: 32) - Maximum message length in bytes
--scope-len-bytes <N> (default: 24) - Maximum scope length in bytes

Instance Parameters

--group-size <N> (default: 4) - Number of group members to create
--prover-index <N> (default: 1) - Index of member generating the proof (0-based)
--message <STRING> (default: "I vote YES on proposal [field_buffer] FieldSlice and FieldSliceMut to own single elements #42") - Message to include in proof
--scope <STRING> (default: "dao_vote_2024_q1") - Scope for nullifier generation

Example Commands

# Basic usage with defaults
cargo run --example semaphore_ecdsa --release

# Large group with custom message
cargo run --example semaphore_ecdsa --release -- \
  --tree-height 16 \
  --group-size 100 \
  --prover-index 42 \
  --message "Approve budget proposal" \
  --scope "dao_2024_q1_vote"

# Benchmark mode (runs circuit N times)
cargo run --example semaphore_ecdsa --release -- --bench 10

Scaling

Circuit complexity scales with tree height but is independent of message size:

Tree Height	Total Constraints	Scaling Factor
1	~15,000	Baseline
2	~18,000	+2,931/level
4	~24,000	+2,931/level
8	~36,000	+2,931/level

Tree height adds ~2,931 AND constraints per level (Merkle proof verification)
Scope size has minimal effect (only affects Keccak padding at boundary crossings)
Message size has zero constraint impact (public input only, not processed)
MUL constraints remain constant (~12,000 from secp256k1 operations)

jadnohra · 2025-09-02T09:10:36Z

Add Semaphore over ECDSA #911 👈 (View in Graphite)
main

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

graphite-app · 2025-09-02T09:11:40Z

+        let nullifier: [u8; 32] = hasher.finalize().into();
+
+        // Populate SHA-256 circuit
+        self.hasher.populate_len_bytes(witness, scope.len() + 32);


There appears to be a length mismatch in the SHA-256 circuit population. The circuit is initialized with total_len = scope_len_bytes + 32 (lines 243-244), but when populating the witness, scope.len() + 32 is used (line 279).

These values can differ because scope_len_bytes represents the padded length used for circuit allocation, while scope.len() is the actual data length. The message vector is padded to wire boundaries (scope_wires = (scope_len_bytes + 7) / 8), creating scope_wires * 8 bytes of space.

To ensure consistency, consider using the same length value in both places:

// Either use scope_len_bytes in both places self.hasher.populate_len_bytes(witness, scope_len_bytes + 32); // Or adjust the circuit initialization to use the actual length let len_bytes = builder.add_witness(); // Make length dynamic

This inconsistency could lead to constraint verification failures or incorrect hash computations.

Suggested change

self.hasher.populate_len_bytes(witness, scope.len() + 32);

self.hasher.populate_len_bytes(witness, scope_len_bytes + 32);

Spotted by Diamond

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

graphite-app · 2025-09-02T10:02:22Z

+        let nullifier: [u8; 32] = hasher.finalize().into();
+
+        // Populate Keccak circuit
+        self.hasher.populate_len_bytes(witness, scope.len() + 32);


Length Inconsistency in Keccak Circuit Initialization

There's a potential bug in the length parameter passed to populate_len_bytes(). The circuit is initialized with total_len = scope_len_bytes + 32 (lines 239-241), but when populating the witness, scope.len() + 32 is used instead.

When the actual scope is shorter than scope_len_bytes, this creates a length mismatch that will cause constraint verification to fail. To maintain consistency:

// Change this: self.hasher.populate_len_bytes(witness, scope.len() + 32); // To this: self.hasher.populate_len_bytes(witness, self.scope_len_bytes + 32);

This ensures the length parameter matches what was used during circuit initialization.

Suggested change

self.hasher.populate_len_bytes(witness, scope.len() + 32);

self.hasher.populate_len_bytes(witness, self.scope_len_bytes + 32);

Spotted by Diamond

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

graphite-app · 2025-09-03T09:10:18Z

+        // Create Keccak circuit with fixed 64-byte length like IdentityCommitmentKeccak
+        let len_bytes = builder.add_constant_64(64);
+        let hasher = Keccak::new(builder, len_bytes, nullifier, message.clone());


Wire conflict bug: The Keccak circuit is created with message.clone() as the message parameter, but these message wires are created as add_witness() wires in the NullifierGeneratorKeccak constructor. However, the Keccak::new() constructor expects to own and manage these message wires internally. This creates a conflict where the same wires are being managed by both the parent circuit and the Keccak circuit, leading to potential witness population conflicts and constraint violations. The fix is to let Keccak::new() create its own message wires internally, or use a different approach that doesn't share wire ownership.

Spotted by Diamond

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

graphite-app · 2025-09-03T10:24:36Z

+[[example]]
+name = "semaphore"
+path = "examples/semaphore.rs"


The example configuration in Cargo.toml has a mismatch between the declared name and path. The entry specifies path = "examples/semaphore.rs", but the actual file added is examples/semaphore_ecdsa.rs. To ensure the example can be properly built and run, the path should be updated to match the actual file:

[[example]] name = "semaphore" path = "examples/semaphore_ecdsa.rs"

Alternatively, the file could be renamed to match the declared path.

Suggested change

[[example]]

name = "semaphore"

path = "examples/semaphore.rs"

[[example]]

name = "semaphore"

path = "examples/semaphore_ecdsa.rs"

Spotted by Diamond

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

graphite-app · 2025-09-03T11:13:13Z

+
+        // This does NOT match what the example produces: [82, 27, 68, 224, 77, 37, 233, 239, ...]
+        // So there's a discrepancy between frontend crate vs examples crate
+    }


The identity commitment calculation differs between the frontend and examples crates, producing inconsistent hash values. This discrepancy should be resolved to ensure deterministic behavior across the codebase. Consider standardizing the commitment generation logic in a shared utility function that both crates can reference, or at minimum documenting the expected behavior and why the implementations differ.

Spotted by Diamond

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

graphite-app · 2025-09-03T11:27:46Z

+		assert!(!message.is_empty(), "Keccak message wires cannot be empty");
+		assert!(max_len_bytes > 0, "Keccak max message length must be > 0, got {} bytes from {} wires", max_len_bytes, message.len());
+


There appears to be confusion in the message length validation logic. The assertion on line 57 checks max_len_bytes > 0, but this value is calculated as message.len() << 3 on line 52. Since line 56 already asserts !message.is_empty(), the second assertion becomes redundant.

Additionally, the error message is misleading - it refers to "bytes" when max_len_bytes actually represents bits (since it's calculated as message.len() * 8).

Consider simplifying to a single assertion or clarifying the error message to accurately reflect that max_len_bytes represents the bit length, not byte length.

Spotted by Diamond

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

jimpo · 2025-09-03T20:13:52Z

@@ -0,0 +1,19 @@
+{


Don't commit local settings

graphite-app · 2025-09-05T08:14:03Z

+    public_key: Secp256k1Affine,
+    commitment: [Wire; KECCAK_DIGEST_LIMBS],
+    commitment_message_wires: Vec<Wire>,
+    hasher: Keccak,


The test identity generation uses predictable patterns like [((i + 42) as u8); 32] which creates weak ECDSA keys (arrays filled with the same byte value). While this is acceptable for testing, it's worth noting that in production code, proper cryptographically secure random generation should be used. Consider adding a comment indicating this is test-only code or using a more realistic key derivation approach even in tests to better simulate real-world conditions.

Spotted by Diamond

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

graphite-app · 2025-09-05T08:21:35Z

+		// Let's see if this matches what the frontend crate produces
+	}


The Common rule requires that commented out code must be flagged with an exception when it's clearly marked with a reason why that particular piece of commented code is included. The comment // Let's see if this matches what the frontend crate produces appears to be commented out code or a TODO without a clear explanation of why it's included in the codebase. This should either be removed or have a clear explanation of why it's being kept.

Suggested change

// Let's see if this matches what the frontend crate produces

}

}

Spotted by Diamond (based on custom rule: Irreducible Rust and Cargo)

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

lockedloop · 2025-09-05T11:05:53Z

I find this interface confusing:

cargo run --example semaphore_ecdsa --release -- --bench 10

i'd rather have a dedicated bench with criterion etc; no point in re-runnign the example like this IMO

jadnohra · 2025-09-05T11:13:01Z

agree, do you want to create one the remove this code? this can go into the category of benchmarking work.

graphite-app · 2025-09-07T19:50:36Z

Merge activity

Sep 7, 7:50 PM UTC: This pull request can not be added to the Graphite merge queue. Please try rebasing and resubmitting to merge when ready.
Sep 7, 7:50 PM UTC: Graphite disabled "merge when ready" on this PR due to: a merge conflict with the target branch; resolve the conflict and try again..

onesk · 2025-09-18T08:17:31Z

+		// Build tree bottom-up
+		while level.len() > 1 {
+			let mut next_level = Vec::new();
+			for i in (0..level.len()).step_by(2) {


nit: level.chunks(2) seems more natural here

onesk · 2025-09-18T08:22:29Z

+			siblings.push(sibling);
+
+			// Compute next level
+			let mut next_level = Vec::new();


this replicates the logic from root; wouldn't it be simpler to compute internal nodes in the constructor and cache them? It only requires twice the memory of the leaf hashes.

onesk · 2025-09-18T10:16:07Z

+		}
+	}
+
+	#[test]


these aren't really tests, they always pass and don't assert anything; they also pollute the test binary stdout

onesk · 2025-09-18T11:11:24Z

+		};
+
+		let generator = Secp256k1Affine::generator(builder);
+		let public_key = scalar_mul_naive(builder, curve, SCALAR_BITS, &scalar_biguint, generator);


Use scalar_mul - it leverages secp256k1 endomorphism and is thus twice more efficient

onesk · 2025-09-18T11:17:05Z

+		let mut index = leaf_index;
+
+		for sibling in &siblings {
+			let is_even = builder.bnot(builder.band(index, builder.add_constant_64(1)));


nit: instead of threading index value through the circuit, you could've just shifted the bit in question to the MSB

onesk · 2025-09-18T11:18:03Z

+
+			// Constrain message wires to match multiplexer output
+			for i in 0..SCALAR_LIMBS {
+				builder.assert_eq(format!("merkle_message_left[{}]", i), message_wires[i], left[i]);


why constrain instead of just using left and right wires as-is?

onesk · 2025-09-18T11:20:56Z

+					if global_byte_idx >= scope_len_bytes {
+						let byte_val = builder.shr(wire, (byte_offset * BITS_PER_BYTE) as u32);
+						let byte_masked = builder.band(byte_val, builder.add_constant_64(0xFF));
+						builder.assert_eq("scope_padding_zero", byte_masked, zero);


nit: this could've been a single AND constraint, with one of 0xFF..0xFFFFFF_FFFFFFFFFF masks. See how Keccak / Blake2s / etc. handle this. But given that this is done only once per nullifier that doesn't have much effect on circuit size.

onesk · 2025-09-18T11:22:03Z

+
+		// Create witness wires for Keccak message
+		let total_message_wires = scope_wires + SCALAR_LIMBS;
+		let nullifier_message_wires: Vec<Wire> = (0..total_message_wires)


nit: this impl assumes that scope is always padded to the nearest multiple of 64 bits in size, whereas reference impl can work with scopes of arbitrary length

onesk · 2025-09-18T11:38:17Z

+				for byte_offset in 0..BYTES_PER_WIRE {
+					let global_byte_idx = wire_start_byte + byte_offset;
+					if global_byte_idx >= message_len_bytes {
+						let byte_val = builder.shr(wire, (byte_offset * BITS_PER_BYTE) as u32);


nit: may be useful to reuse this logic between scope & message computations

onesk · 2025-09-18T11:40:38Z

+		);
+
+		Self {
+			message,


How exactly is message used? It does not feed anywhere? Is there any attempt to replicate "dummy square" functionality of the original Semaphore?

Indeed the message is not used anywhere, just passed forward. I will look at the 'dummy square'

Discussed with Ben and Jim. The way this works in binius64 is to 'observe' the message in the transcript. So in our case, we should not even have the message in the circuit/witness. I will remove it.

indeed. solution: completely remove it from the circuit, and observe into the transcript

paulcadman · 2025-09-18T16:12:24Z

+struct NullifierGeneratorECDSA {
+	scope: Vec<Wire>,
+	secret_scalar_wires: [Wire; SCALAR_LIMBS],
+	#[allow(dead_code)]


Is identity_secret_scalar still dead code?

paulcadman · 2025-09-18T16:33:56Z

+		let scalar_biguint = BigUint {
+			limbs: secret_scalar.to_vec(),
+		};
+


Should scalar_biguint be constrained to be non-zero? There is an assert! in compute_public_key_coords in populate_witness.

graphite-app bot reviewed Sep 2, 2025

View reviewed changes

jadnohra force-pushed the 09-02-semaphore branch from ecc58f8 to 81b8963 Compare September 2, 2025 10:01

graphite-app bot reviewed Sep 2, 2025

View reviewed changes

jadnohra force-pushed the 09-02-semaphore branch from 81b8963 to d922cb9 Compare September 3, 2025 09:07