Skip to content

fix: use generic security level in aux precompute instead of hardcoded SecurityLevel128 - #187

Open
Mandalorian7773 wants to merge 1 commit into
LFDT-Lockness:cggmp24/mfrom
Mandalorian7773:fix/thread-generic-security-level-in-tests
Open

fix: use generic security level in aux precompute instead of hardcoded SecurityLevel128#187
Mandalorian7773 wants to merge 1 commit into
LFDT-Lockness:cggmp24/mfrom
Mandalorian7773:fix/thread-generic-security-level-in-tests

Conversation

@Mandalorian7773

@Mandalorian7773 Mandalorian7773 commented Apr 28, 2026

Copy link
Copy Markdown

Closes #185

Problem

get_aux_inner in tests/src/lib.rs calls precompute_multiexp_table with a hardcoded SecurityLevel128, ignoring the generic L parameter already in scope on the function signature.

This causes all secp384r1 tests to fail because that curve uses SecurityLevel192 (via the CurveParams trait impl at line 509), creating a type mismatch during aux info precomputation. The bug was silent because CI runs cargo test --release, which compiles out all debug_assert! calls — so no assertion fired to catch it.

Fix

1. One-line bug fix (tests/src/lib.rs):

-  .precompute_multiexp_table::<cggmp24::security_level::SecurityLevel128>()
+  .precompute_multiexp_table::<L>()

2. Enable debug assertions in release test builds (Cargo.toml):

Added [profile.release.package.*] overrides so debug_assert! calls inside workspace library code fire during cargo test --release, as CI does. Third-party dependencies are unaffected.

[profile.release.package.cggmp24]
debug-assertions = true

[profile.release.package.cggmp24-keygen]
debug-assertions = true

[profile.release.package.key-share]
debug-assertions = true

[profile.release.package.paillier-zk]
debug-assertions = true

This ensures existing debug_assert! guards in paillier-zk, key-share, cggmp24-keygen, and cggmp24/src/signing.rs will catch invariant violations in future CI runs.

Impact

  • Fixes all secp384r1 signing tests
  • No behavioral change for secp256k1, secp256r1, or stark
  • Prevents silent regressions of this class in CI going forward

Copilot AI review requested due to automatic review settings April 28, 2026 11:14

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes aux-info multiexponentiation precomputation in tests so it respects the generic security level L (instead of always using SecurityLevel128), resolving type/parameter mismatches for curves like secp384r1 that require SecurityLevel192.

Changes:

  • Replace hardcoded SecurityLevel128 with the generic L in get_aux_inner when calling precompute_multiexp_table.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@Mandalorian7773

Copy link
Copy Markdown
Author

Hi @survived , this PR fixes the secp384r1 failures.
All tests pass locally, would love your review.

@Mandalorian7773
Mandalorian7773 force-pushed the fix/thread-generic-security-level-in-tests branch from 4e36c7f to bda8464 Compare April 28, 2026 15:14
Comment thread Cargo.toml Outdated

# Enable debug assertions for workspace packages even in release builds, so that
# debug_assert! calls in library code fire during `cargo test --release` (as used
# in CI). Third-party dependencies are NOT affected by these overrides.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick: it's evident that crates not listed here are not affected. I'd rather have a comment like « the binaries using the listed crates are not affected since these options only affect the builds in the context of this workspace », except I phrased it terribly. This fact was not evident to me

@maurges

maurges commented Apr 29, 2026

Copy link
Copy Markdown
Contributor

Looks good, but your patch fails the cargo fmt check

@Mandalorian7773
Mandalorian7773 force-pushed the fix/thread-generic-security-level-in-tests branch from bda8464 to 3781d6d Compare April 29, 2026 16:08
@Mandalorian7773

Copy link
Copy Markdown
Author

Fixed @maurges

…sertions in test builds

Signed-off-by: Aditya Jagrani <adityajagrani8@gmail.com>
@Mandalorian7773
Mandalorian7773 force-pushed the fix/thread-generic-security-level-in-tests branch from 3781d6d to 6b93ab9 Compare May 1, 2026 09:33

@maurges maurges left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good otherwise

Comment thread Cargo.toml
Comment on lines +58 to +61
# Enable debug assertions for workspace packages in release builds so that
# debug_assert! calls in library code fire during `cargo test --release`.
# Note: these overrides only apply within this workspace — binaries that
# depend on these crates externally are not affected.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Replace with:

Enable debug assertions in the release builds since we have to run the tests in release mode, because they are too slow

A comment should rarely be an explaination, it should state intent and reason.

And remove the lines between sections, they make them seem unrelated

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Backlog

Development

Successfully merging this pull request may close these issues.

secp384r1 signing tests fail due to hardcoded SecurityLevel128 in aux precompute

4 participants