-
Notifications
You must be signed in to change notification settings - Fork 72
fix: Enforce input length to match the modulus #1905
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -133,8 +133,10 @@ pub struct RegisteredUser { | |
impl light_hasher::DataHasher for RegisteredUser { | ||
fn hash<H: light_hasher::Hasher>(&self) -> std::result::Result<[u8; 32], HasherError> { | ||
let truncated_user_pubkey = hash_to_bn254_field_size_be(&self.user_pubkey.to_bytes()); | ||
let mut data = [0u8; 32]; | ||
data[1..].copy_from_slice(&self.data); | ||
|
||
H::hashv(&[truncated_user_pubkey.as_slice(), self.data.as_slice()]) | ||
H::hashv(&[truncated_user_pubkey.as_slice(), data.as_slice()]) | ||
Comment on lines
+136
to
+139
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. are you sure this is necessary? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The But There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. you are right got it |
||
} | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -1,4 +1,6 @@ | ||||||||||||||||||||||||
use light_hasher::{bigint::bigint_to_be_bytes_array, Hasher, Poseidon}; | ||||||||||||||||||||||||
use light_hasher::{ | ||||||||||||||||||||||||
bigint::bigint_to_be_bytes_array, to_byte_array::ToByteArray, Hasher, Poseidon, | ||||||||||||||||||||||||
}; | ||||||||||||||||||||||||
use light_merkle_tree_reference::indexed::IndexedMerkleTree; | ||||||||||||||||||||||||
use num_bigint::ToBigUint; | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
|
@@ -38,16 +40,16 @@ pub fn functional_non_inclusion_test() { | |||||||||||||||||||||||
assert_eq!( | ||||||||||||||||||||||||
leaf_0, | ||||||||||||||||||||||||
Poseidon::hashv(&[ | ||||||||||||||||||||||||
&0_u32.to_biguint().unwrap().to_bytes_be(), | ||||||||||||||||||||||||
&30_u32.to_biguint().unwrap().to_bytes_be() | ||||||||||||||||||||||||
&0_u32.to_byte_array().unwrap(), | ||||||||||||||||||||||||
&30_u32.to_byte_array().unwrap(), | ||||||||||||||||||||||||
]) | ||||||||||||||||||||||||
.unwrap() | ||||||||||||||||||||||||
); | ||||||||||||||||||||||||
assert_eq!( | ||||||||||||||||||||||||
Comment on lines
41
to
47
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 Verification agent 🧩 Analysis chainVerify leaf hashing arity vs. inline comment (“H(0, 1, 30)” vs. two-field hash calls) The inline comments say the leaf is H(value, next_index, next_value), but the assertions hash only two inputs. If the leaf really includes next_index, this test may be under-specifying the expected hash. If the design intentionally moved to two inputs, please update the comment to avoid confusion. Run this script to find Poseidon::hashv call sites and check how many fields are hashed for leaves: Also applies to: 49-55 🏁 Script executed: #!/bin/bash
set -euo pipefail
# Show Poseidon::hashv usage with a bit of context
rg -n "Poseidon::hashv\\(&\\[" -A 6 -B 2
# Inspect leaf construction sites in the indexed merkle tree library to confirm arity
rg -n "leaf\\(|leaf_hash|hash_leaf|Hash\\(value, next_index, next_value\\)|H\\(" -S -A 6 -B 2 Length of output: 304588 Update inline comments to match two-input leaf hash The test’s comments still describe a three-input hash ( • File: program-tests/merkle-tree/tests/indexed.rs Suggested diff: - // leaf index: 0 = H(0, 1, 30) // Hash(value, next_index, next_value)
+ // leaf index: 0 = H(0, 30) // Hash(value, next_value)
- // leaf index: 1 = H(30, 0, 0)
+ // leaf index: 1 = H(30, 0) 📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents
|
||||||||||||||||||||||||
leaf_1, | ||||||||||||||||||||||||
Poseidon::hashv(&[ | ||||||||||||||||||||||||
&30_u32.to_biguint().unwrap().to_bytes_be(), | ||||||||||||||||||||||||
&0_u32.to_biguint().unwrap().to_bytes_be() | ||||||||||||||||||||||||
&30_u32.to_byte_array().unwrap(), | ||||||||||||||||||||||||
&0_u32.to_byte_array().unwrap(), | ||||||||||||||||||||||||
]) | ||||||||||||||||||||||||
.unwrap() | ||||||||||||||||||||||||
); | ||||||||||||||||||||||||
|
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.
💡 Verification agent
🧩 Analysis chain
Quick repo-wide audit to catch non-32-byte Poseidon inputs.
To reduce the chance of a missed callsite (especially in tests/fixtures), run the search below and adjust any remaining .to_be_bytes() or variable-length to_bytes_be() inputs to 32-byte encodings.
🏁 Script executed:
Length of output: 75413
Enforce 32-byte encodings for all Poseidon::hashv inputs
Several hash callsites still pass variable-length byte slices. Please update the following to use fixed 32-byte arrays (via
to_byte_array()
orbigint_to_be_bytes_array::<32>()
):• prover/client/tests/batch_update.rs:36
• program-tests/merkle-tree/tests/indexed.rs:40–43 & 48–51
After making these changes, rerun the repo-wide audit to confirm no direct
.to_be_bytes()
or variable-length.to_bytes_be()
slices are passed toPoseidon::hashv
.🤖 Prompt for AI Agents