test(msm): add negative/adversarial tests for MSM constraint satisfaction#392
Conversation
ocdbytes
left a comment
There was a problem hiding this comment.
Can you also add two more test cases here :
- out_y limb corruption (same as written for out_x).
- scalar corruption (replace the scalar but keep output computed from the original scalar). this is to check end to end scalar output binding.
After this will give one more review for the code.
|
Have added both the cases and re-ran the validation commands. New tests:
|
ocdbytes
left a comment
There was a problem hiding this comment.
Also add a test for
- out_inf corruption
- neg2 flip
- multi point MSM (this will catch any error in the process multi point path)
NIT : for tests which dont require much changes and are mostly duplicated logic like out_x and out_y or neg_1 and neg_2. We can have a single test and add it as a case rather than having two different tests with mostly same logic.
| let curve = Secp256r1; | ||
| let gx = curve.generator().0; | ||
| let gy = curve.generator().1; | ||
| let scalar: [u64; 4] = [7, 0, 0, 0]; | ||
| let (ex, ey) = ec_scalar_mul( | ||
| &gx, | ||
| &gy, | ||
| &scalar, | ||
| &curve.curve_a(), | ||
| &curve.field_modulus_p(), | ||
| ); |
There was a problem hiding this comment.
Ig we can de duplicate this among the tests
There was a problem hiding this comment.
Consolidated the duplicated out_x/out_y and neg1/neg2 cases into case-driven tests that share the setup. Please take a look at 73f41e68.
There was a problem hiding this comment.
This logic :
let curve = Secp256r1;
let gx = curve.generator().0;
let gy = curve.generator().1;
let scalar: [u64; 4] = [7, 0, 0, 0];
let (ex, ey) = ec_scalar_mul(
&gx,
&gy,
&scalar,
&curve.curve_a(),
&curve.field_modulus_p(),
);appears too many times which is just a repeating code. Ig this can be delegated to a single function
There was a problem hiding this comment.
Yeah, you're totally right. should've been more DRY in the first place. have extracted the repeated single-point generator setup into a shared helper and updated the affected tests to use it. please take a look.
| fn different_field_element(value: FieldElement) -> FieldElement { | ||
| if value == FieldElement::zero() { | ||
| FieldElement::from(1u64) | ||
| } else { | ||
| FieldElement::zero() | ||
| } | ||
| } |
There was a problem hiding this comment.
we can use a better approach here rather than toggling between 0 and 1
There was a problem hiding this comment.
Replaced different_field_element; the general corruption path now uses corrupt_field_element, and the neg1/neg2 case uses an actual boolean flip. Please take a look.
|
Made the updates:
Also re-ran the validation commands locally. |
|
@PranavPipariya Please address the above comments regarding the code and then will give a final review. |
ocdbytes
left a comment
There was a problem hiding this comment.
Some NITS.
Other than this things look good to me !!
| /// Two-point MSM: corrupting the output must be rejected, exercising the | ||
| /// multi-point accumulation and output-constraining path. | ||
| #[test] | ||
| fn test_two_point_msm_rejects_wrong_output() { |
There was a problem hiding this comment.
this new test it just copy of the test build_single_point_msm_fixture. We can de duplicate this.
There was a problem hiding this comment.
done. have extracted the shared setup into build_default_two_point_msm_fixture. Both test_two_point_msm and test_two_point_msm_rejects_wrong_output use it now. please take a look.
There was a problem hiding this comment.
Pre existing test should also be using the the function for params
There was a problem hiding this comment.
done. the pre-existing positive tests now use secp256r1_generator_params (or secp256r1_generator where curve params aren't needed).
There was a problem hiding this comment.
remove these print statements these were left in previous review
There was a problem hiding this comment.
removed the print statements
| let case = secp256r1_generator_case([7, 0, 0, 0]); | ||
| let fixture = build_generator_single_point_fixture(case); |
There was a problem hiding this comment.
NIT :
The pattern here of calling secp256r1_generator_case and build_generator_single_point_fixture looks redundant use something like this :
fn generator_fixture(scalar: [u64; 4]) -> SinglePointMsmFixture {
build_generator_single_point_fixture(secp256r1_generator_case(scalar))
} so the tests which require their own case internally can call otherwise
fn test_single_point_rejects_wrong_output_inf() {
let fixture = generator_fixture([7, 0, 0, 0]); // ← one call
......
}There was a problem hiding this comment.
Added generator_fixture as mentioned. simple tests now use the one-liner. test_single_point_rejects_off_curve_input and test_single_point_rejects_wrong_scalar_output_pairing keep the two-step form because they read case fields before building the fixture.
Co-authored-by: Arun Jangra <arunjangra1001@gmail.com>
|
@PranavPipariya fix the lint and then we can merge the PR |
|
Thanks for the contribution @PranavPipariya! |
Summary
Adds the negative/adversarial MSM constraint-satisfaction tests proposed in #348.
This PR is test-only and does not change MSM implementation behavior.
Closes #348.
Included tests
s2 = 0forgery attemptneg1sign bitValidation
cargo +nightly-2026-03-04 fmt --all --checkcargo +nightly-2026-03-04 clippy --all-targets --all-features --verbosecargo +nightly-2026-03-04 test -p provekit-bench --test msm_witness_solvingcargo +nightly-2026-03-04 test --no-fail-fast --all-features --verbose --lib --tests --bins