fix: resolve bug in modulo operations during hash_public_keys, add unit test with real data from mainnet - #3
Conversation
…it test with real data from mainnet
WalkthroughThe PR updates documentation imports and examples, fixes endianness when constructing scalars in secure aggregation, and adds a large-scale verification test. It also removes an example from serialization docs and applies minor formatting changes. No public API behavior changes except the corrected byte order in scalar hashing. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Signature
participant SecureAggregation
participant Pairing
Caller->>Signature: verify_secure(message, pubkeys)
Signature->>SecureAggregation: hash_public_keys_with_sorted(...)
SecureAggregation->>SecureAggregation: hash -> 32 bytes -> reverse -> Scalar(s)
SecureAggregation-->>Signature: coefficients, sorted pubkeys
Signature->>Pairing: aggregate verify with coefficients
Pairing-->>Signature: result
Signature-->>Caller: verification outcome
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15–20 minutes Possibly related PRs
Suggested reviewers
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: 2
🔭 Outside diff range comments (1)
src/secure_aggregation.rs (1)
61-96: Ensure deterministic modular reduction usingfrom_bytes_wideinstead offrom_repr
from_reprrequires a canonical (reduced) input and will returnNoneif the 32-byte hash is ≥ the field modulus (≈50% of the time). To match C++’sbn_mod_basic(which always reduces), switch to an explicit reduction viafrom_bytes_wide.Locations to update:
src/secure_aggregation.rs(lines 61–96)src/secret_key_share.rs(around lines 88–90)src/helpers.rs(around lines 96–113)Suggested diff in
secure_aggregation.rs:- // Convert hash → big-endian repr → reverse → from_repr (no reduction) - let mut repr = <…>::Repr::default(); - // copy `hash` into `repr_bytes` and reverse - let scalar = <<C as Pairing>::PublicKey as Group>::Scalar - ::from_repr(repr) - .into_option()? + // Deterministically reduce hash mod p using from_bytes_wide + let mut wide = [0u8; 64]; + // Place big-endian hash into the high bytes of a little-endian 64-byte array + for (i, &b) in hash.iter().enumerate() { + wide[31 - i] = b; + } + let scalar = <<C as Pairing>::PublicKey as Group>::Scalar + ::from_bytes_wide(&wide);Apply the same pattern in the other files to ensure every
from_reprpath becomes an explicit reduction. This guarantees parity with C++’sbn_mod_basic.
🧹 Nitpick comments (1)
tests/secure_aggregation_test.rs (1)
143-235: Great real-world coverage; a couple of nits and a small allocation improvement
- Pre-allocate public_keys with capacity to avoid reallocations.
- The comment about choosing Bls12381G2Impl could be confusing (it mentions G1 pubkeys and G2 signatures). Consider clarifying that you are using the variant where pubkeys are 48 bytes and signatures are 96 bytes, and that this corresponds to the selected Impl in this crate.
Apply this diff inside the test:
- // Test data from production system with 57 signers - // Using Bls12381G2Impl because public keys are 48 bytes (G1) and signature is 96 bytes (G2) + // Test data from production system with 57 signers. + // Using the variant where public keys are 48 bytes (G1) and the signature is 96 bytes (G2). + // This corresponds to Bls12381G2Impl in this crate. @@ - // Parse public keys - let mut public_keys = Vec::new(); + // Parse public keys + let mut public_keys = Vec::with_capacity(keys_hex.len());
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/aggregate_signature.rs(1 hunks)src/secure_aggregation.rs(5 hunks)src/serialization.rs(1 hunks)src/signature.rs(2 hunks)tests/secure_aggregation_test.rs(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
tests/secure_aggregation_test.rs (4)
src/secure_aggregation.rs (1)
PublicKey(517-517)examples/verify_secure_example.rs (1)
PublicKey(35-35)src/aggregate_signature.rs (2)
try_from(126-147)try_from(163-165)src/signature.rs (5)
try_from(123-125)try_from(287-319)from_bytes_with_mode(231-253)to_bytes_with_mode(209-214)verify_secure(177-197)
🔇 Additional comments (10)
src/serialization.rs (1)
24-24: Minor whitespace cleanup looks goodNo functional changes; enum semantics and helpers remain intact.
src/secure_aggregation.rs (3)
118-123: Signature bound reformatting is fineFormatting-only change for H’s bounds. No functional impact.
181-186: Signature bound reformatting is fineSame as above; no behavior changes.
365-367: Closure style change is finePurely stylistic; behavior unchanged.
src/aggregate_signature.rs (1)
15-16: Doc import path update is correctThe example now uses blsful::Bls12381G1Impl, consistent with the other doc updates. No functional changes.
src/signature.rs (4)
13-14: Doc example import path updated correctlyMatches the new canonical path used across examples.
16-16: Doc cleanupRemoving the stray doc spacer is fine.
19-19: Doc cleanupAnother harmless doc whitespace tweak.
88-89: Minor formatting tweak after debug_assertNo behavior change; fine to keep.
tests/secure_aggregation_test.rs (1)
10-10: Importing Modern directly is fineBrings the enum variant into scope cleanly; improves readability in the test.
| // Reverse bytes since from_repr expects little-endian but hash is big-endian | ||
| repr_bytes.reverse(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Endianness fix is correct; consider extracting into a helper to avoid duplication
Reversing the bytes to feed little-endian into from_repr addresses the BE/LE mismatch. The same conversion logic appears below in the legacy-aware path; extract to a single helper to reduce duplication and future drift.
Apply this diff to use a helper here:
- // Create a field element representation
- let mut repr =
- <<<C as Pairing>::PublicKey as Group>::Scalar as PrimeField>::Repr::default();
- let repr_bytes = repr.as_mut();
-
- // Copy the hash into the representation
- // For BLS12-381, the scalar field is ~255 bits, stored in 32 bytes
- // The hash is 32 bytes, so it fits directly
- if repr_bytes.len() >= 32 {
- // Copy hash to the least significant bytes (big-endian interpretation)
- let offset = repr_bytes.len() - 32;
- repr_bytes[offset..].copy_from_slice(&hash);
- // Zero out any higher bytes
- for byte in &mut repr_bytes[..offset] {
- *byte = 0;
- }
- // Reverse bytes since from_repr expects little-endian but hash is big-endian
- repr_bytes.reverse();
- } else {
- // This shouldn't happen for BLS12-381, but handle it gracefully
- return Err(BlsError::InvalidInputs(
- "Field representation too small".to_string(),
- ));
- }
-
- // Create scalar from representation - this automatically reduces modulo field order
- let scalar = <<C as Pairing>::PublicKey as Group>::Scalar::from_repr(repr)
- .into_option()
- .ok_or_else(|| {
- BlsError::InvalidInputs("Failed to create scalar from hash".to_string())
- })?;
+ // Convert hash -> scalar (handles endianness and reduction semantics)
+ let scalar = hash32_to_scalar::<C>(hash)?;Add this helper near the top of the module (outside of the changed range):
#[inline]
fn hash32_to_scalar<C: BlsSignatureImpl>(
hash: [u8; 32],
) -> BlsResult<<<C as Pairing>::PublicKey as Group>::Scalar> {
// Build little-endian repr from big-endian hash
let mut repr =
<<<C as Pairing>::PublicKey as Group>::Scalar as PrimeField>::Repr::default();
let repr_bytes = repr.as_mut();
if repr_bytes.len() < 32 {
return Err(BlsError::InvalidInputs(
"Field representation too small".to_string(),
));
}
let offset = repr_bytes.len() - 32;
repr_bytes[offset..].copy_from_slice(&hash);
for byte in &mut repr_bytes[..offset] {
*byte = 0;
}
repr_bytes.reverse();
// Note: from_repr must accept canonical encodings. If it rejects >r values, we must reduce.
let ct = <<C as Pairing>::PublicKey as Group>::Scalar::from_repr(repr);
ct.into_option()
.ok_or_else(|| BlsError::InvalidInputs("Failed to create scalar from hash".to_string()))
}Note: If the underlying field’s from_repr rejects values ≥ modulus (common in ff-based fields), this helper may still need an explicit reduction path to fully match “bn_mod_basic”. If that’s the case, we can switch to a reduction-based approach (e.g., using from_bytes_wide with proper LE placement) in a follow-up.
🤖 Prompt for AI Agents
In src/secure_aggregation.rs around lines 81-82, the byte-reversal logic for
converting a big-endian 32-byte hash into the scalar representation is
duplicated; extract it into a single helper function placed near the top of the
module (outside the changed range) named hash32_to_scalar that is generic over
C: BlsSignatureImpl and returns BlsResult<<<C as Pairing>::PublicKey as
Group>::Scalar>. Implement the helper to allocate the scalar Repr, ensure
repr.as_mut() is large enough, copy the 32 hash bytes into the last 32 bytes,
zero the prefix bytes, reverse the whole repr to little-endian, call from_repr
and convert the Option into an Err with a descriptive BlsError on failure; then
replace the inline reversal-and-from_repr code at this location (and the
legacy-aware path) with a call to hash32_to_scalar::<C>(hash).
| // Reverse bytes since from_repr expects little-endian but hash is big-endian | ||
| repr_bytes.reverse(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Duplicate endianness fix; reuse a common helper
This mirrors the BE→LE correction above. Recommend calling a shared helper to prevent subtle drift between code paths.
Apply this diff to use the same helper here:
- if repr_bytes.len() >= 32 {
- let offset = repr_bytes.len() - 32;
- repr_bytes[offset..].copy_from_slice(&hash);
- for byte in &mut repr_bytes[..offset] {
- *byte = 0;
- }
- // Reverse bytes since from_repr expects little-endian but hash is big-endian
- repr_bytes.reverse();
- } else {
- return Err(BlsError::InvalidInputs(
- "Field representation too small".to_string(),
- ));
- }
-
- let scalar = <<C as Pairing>::PublicKey as Group>::Scalar::from_repr(repr)
- .into_option()
- .ok_or_else(|| {
- BlsError::InvalidInputs("Failed to create scalar from hash".to_string())
- })?;
+ let scalar = hash32_to_scalar::<C>(hash)?;Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/secure_aggregation.rs around lines 311-312, replace the manual
repr_bytes.reverse() endianness flip with the shared BE→LE helper used elsewhere
to avoid duplication and drift; call the same helper function (the one
previously used in this file — e.g., convert_be_to_le_bytes or be_to_le_bytes)
instead of reversing in-place, adjust the call/assignment to match the helper's
signature (mutate or return a new Vec<[u8]> as appropriate), and remove the
manual reverse so both code paths use the identical conversion routine.
2 participants
Summary by CodeRabbit