fix: implement hash32_to_scalar function for modular reduction of big-endian hashes - #4
fix: implement hash32_to_scalar function for modular reduction of big-endian hashes#4PastaPastaPasta wants to merge 1 commit into
Conversation
WalkthroughRefactors secure_aggregation by introducing a private helper (hash32_to_scalar) to convert 32-byte hashes into scalars. Replaces duplicated inline conversion logic in two functions with this helper. Hashing workflow, error behavior, and public API remain unchanged. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~7 minutes Possibly related PRs
Poem
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/secure_aggregation.rs (1)
41-43: Consider using fill() for better clarity.The loop that zeros out prefix bytes could be more concise using the
fill()method.- // Zero out any prefix bytes - for byte in &mut repr_bytes[..offset] { - *byte = 0; - } + // Zero out any prefix bytes + repr_bytes[..offset].fill(0);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/secure_aggregation.rs(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/secure_aggregation.rs (6)
examples/verify_secure_example.rs (1)
PublicKey(35-35)src/aggregate_signature.rs (1)
default(55-57)src/signature.rs (1)
default(47-49)src/signature_share.rs (1)
default(15-17)src/impls.rs (1)
default(31-33)tests/utils.rs (1)
default(38-40)
🔇 Additional comments (3)
src/secure_aggregation.rs (3)
9-52: LGTM! Well-designed helper function for hash-to-scalar conversion.The implementation correctly handles the conversion from a 32-byte big-endian hash to a scalar field element with proper modular reduction. The function appropriately:
- Validates the representation size
- Handles endianness conversion (big-endian to little-endian)
- Performs automatic modular reduction via
from_repr- Returns clear error messages
This refactoring successfully eliminates code duplication and centralizes the hash-to-scalar conversion logic.
106-108: LGTM! Clean refactoring to use the new helper function.The replacement of the inline conversion logic with
hash32_to_scalarmaintains the same functionality while improving code maintainability.
312-314: LGTM! Consistent refactoring in legacy mode function.The replacement in
hash_public_keys_with_sorted_modemirrors the change in the standard version, maintaining consistency across the codebase.
| // Ensure we have enough space for the hash | ||
| if repr_bytes.len() < 32 { | ||
| return Err(BlsError::InvalidInputs( | ||
| "Field representation too small for 32-byte hash".to_string(), | ||
| )); | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Consider caching the representation size check.
Since the field representation size is a compile-time constant for a given curve implementation, this check will always pass or fail consistently. Consider making this a debug assertion or documenting why 32 bytes is always safe for the supported curves.
Let me verify the field element sizes for the supported curve implementations:
🏁 Script executed:
#!/bin/bash
# Check the field element representation sizes for supported curves
# Look for curve implementations and their field sizes
# Find all BLS curve implementations
echo "=== Finding BLS curve implementations ==="
fd -e rs | xargs rg -l "impl.*BlsSignatureImpl|impl.*Pairing" | head -20
# Check for field element sizes or scalar representations
echo -e "\n=== Checking field/scalar sizes ==="
rg -A 5 "Scalar.*Repr|PrimeField.*Repr|MODULUS|field.*256|field.*381" --type rust
# Look for any documentation about field sizes
echo -e "\n=== Checking for field size documentation ==="
rg -i "field.*size|scalar.*size|32.*byte|256.*bit|381.*bit" --type rust | head -20Length of output: 3643
Use a compile‐time or debug assertion for the 32-byte hash bound
Instead of a one‐off runtime check, leverage the fact that <PrimeField>::Repr has a fixed size (e.g. 48 bytes for BLS12-381) and enforce the requirement at compile or debug time:
- In
src/secure_aggregation.rs(around lines 29–34), replace:with a debug assertion:// Ensure we have enough space for the hash if repr_bytes.len() < 32 { return Err(BlsError::InvalidInputs( "Field representation too small for 32-byte hash".to_string(), )); }
debug_assert!( repr_bytes.len() >= 32, "Field representation ({}) too small for 32-byte hash", repr_bytes.len() );
- Or, pull the length into a constant and use a compile-time assertion (via
static_assertionsor aconst_assert!macro):const REPR_BYTES: usize = std::mem::size_of::<<<C as Pairing>::PublicKey as Group>::Scalar as PrimeField>::Repr>(); const_assert!(REPR_BYTES >= 32, "Repr must be at least 32 bytes");
- Add a short doc comment (or README entry) stating that all supported curves (currently BLS12-381, etc.) have ≥ 32 bytes of field repr, so this bound is always satisfied.
2 participants
Summary by CodeRabbit
Refactor
Chores
No Public API Changes