Fix hardcoded scalar size in Straus NAF computation#57
Fix hardcoded scalar size in Straus NAF computation#57sridhar-panigrahi wants to merge 2 commits intoLFDT-Lockness:mfrom
Conversation
The `add_scalar` method in `NafMatrix` hardcodes `x_u64[0..4]` when reading scalar bytes, which only works for 32-byte scalars. This panics for curves with different scalar sizes like P-384 or P-521. Use the actual scalar byte length instead. Signed-off-by: Shridhar Panigrahi <sridharpanigrahi2006@gmail.com>
|
@survived , please let me know your thoughts on this ! |
|
Hi @sridhar-panigrahi. What you propose looks like a reasonable change, but we cannot test it since there's no curve with scalar_len != 32 Bringing curves with larger scalar sizes is something we would like to do, but it's a broader topic than just updating NAF computations: we need to add an actual curve to the crate, update all the tests, see what gets broken (probably something else besides multiscalar multiplication), and otherwise carefully check all code to make sure it makes no assumptions on scalar size. |
|
Hi @survived, thanks for the detailed feedback! I completely understand — supporting larger scalar sizes is a broader effort that goes well beyond just this NAF fix. I did a quick scan and noticed there are similar assumptions in other places too (e.g., Would you be open to merging this as a small incremental fix for now? It doesn't break anything for the current 32-byte curves, and it removes one assumption that would otherwise panic for larger scalars. I'd also be happy to help with the broader work — adding a curve with a larger scalar size, auditing the rest of the codebase for similar hardcoded assumptions, and updating the tests. If you'd prefer that as a single comprehensive PR instead, I can work on that too. Just let me know what approach you'd prefer! |
The `doc_auto_cfg` feature was merged into `doc_cfg` in Rust 1.92.0 (see rust-lang/rust#138907), causing nightly rustdoc builds to fail with E0557. Remove it from the feature list. Signed-off-by: Shridhar Panigrahi <sridharpanigrahi2006@gmail.com>
|
I would prefer to defer merging this PR until we start adding curves with larger scalar sizes. I want the merged code to be tested against various scalar sizes to make sure everything works as expected. Given that PR concerns one line, we should be able to merge it without merge conflicts once we start working on it. |
|
I added #58 issue to track this project |
|
Thanks @survived, that makes total sense — deferring until it can be properly tested is the right call. I'll keep an eye on #58. Happy to pick up work on it when you're ready to start adding larger scalar curves — whether that's adding the curve implementation, auditing other hardcoded assumptions, or updating tests. Just let me know! |
This is the first curve with a larger scalar size (48 bytes instead of
32) to be added to the library. It exercises the full scalar pipeline
with a non-256-bit field and validates that no hardcoded scalar-size
assumptions remain.
What's included:
- p384 crate dependency and `secp384r1` feature flag wired through
generic-ec-curves and generic-ec
- CurveName, FromUniformBytes (L=64 per RFC 9380), BytesModOrder,
Reduce<48>, and NoInvalidPoints implementations
- New scalar_from_{be,le}_bytes_mod_order_reducing_48 utility functions
for Horner-method reduction using 48-byte chunks
- All existing tests instantiated for Secp384r1: scalar ops, point ops,
serde, multiscalar (Straus), coordinates, dlog_eq ZKPs, Reduce<48>
- Benchmarks updated for the new curve
All 180 tests pass. Zero breakage — the dynamic scalar-size handling
(including the Straus NAF fix from LFDT-Lockness#57) generalizes cleanly to 384-bit
scalars.
Relates to LFDT-Lockness#58
Signed-off-by: Shridhar Panigrahi <sridharpanigrahi2006@gmail.com>
This is the first curve with a larger scalar size (48 bytes instead of
32) to be added to the library. It exercises the full scalar pipeline
with a non-256-bit field and validates that no hardcoded scalar-size
assumptions remain.
What's included:
- p384 crate dependency and `secp384r1` feature flag wired through
generic-ec-curves and generic-ec
- CurveName, FromUniformBytes (L=64 per RFC 9380), BytesModOrder,
Reduce<48>, and NoInvalidPoints implementations
- New scalar_from_{be,le}_bytes_mod_order_reducing_48 utility functions
for Horner-method reduction using 48-byte chunks
- All existing tests instantiated for Secp384r1: scalar ops, point ops,
serde, multiscalar (Straus), coordinates, dlog_eq ZKPs, Reduce<48>
- Benchmarks updated for the new curve
All 180 tests pass. Zero breakage — the dynamic scalar-size handling
(including the Straus NAF fix from LFDT-Lockness#57) generalizes cleanly to 384-bit
scalars.
Relates to LFDT-Lockness#58
Signed-off-by: Shridhar Panigrahi <sridharpanigrahi2006@gmail.com>
|
Done in #59 |
2 participants
The
add_scalarmethod inNafMatrixhardcodesx_u64[0..4]when reading scalar bytes, which only works for 32-byte scalars. This panics for curves with different scalar sizes like P-384 or P-521.Use the actual scalar byte length instead.