Skip to content

Conversation

CreeptoGengar
Copy link

@CreeptoGengar CreeptoGengar commented Aug 1, 2025

Description

Implement Reduce and ReduceNonZero traits for U384 in Scalar, update tests to cover new cases.

Fixed #1199

Copy link
Contributor

@daxpedda daxpedda left a comment

Choose a reason for hiding this comment

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

This isn't quite right, it should be U352 according to NIST FIPS 186-5

@tarcieri
Copy link
Member

tarcieri commented Aug 2, 2025

The integer size needs to be a multiple of the limb size. 352-bits is 32-bits * 11, however it isn't evenly divisible by 64-bits

@CreeptoGengar
Copy link
Author

@tarcieri @daxpedda

Right, so U352 per FIPS but i checked the crypto-bigint source - no U352 available, only U192, U224, U256, U384, U448, U512, etc. U384 is the closestr we've got and it actually divides evenly into both 32-bit (12 limbs) and 64-bit (6 limbs).

Should U384 be fine for us or am i missing something?

@daxpedda
Copy link
Contributor

daxpedda commented Aug 4, 2025

It is fine, was my bad.

Please refrain from explicit pinging for just a regular answer. Thanks.

type Bytes = [u8; 48];

fn reduce(w: U384) -> Self {
// Convert U384 to U512 for use with barrett_reduce, which expects 512 bits total
Copy link
Member

@tarcieri tarcieri Aug 8, 2025

Choose a reason for hiding this comment

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

Perhaps this should be composed in terms of a Reduce<U512> impl.

If you do that, you can also use u384.concat(&U128::ZERO) to obtain a U512.

Copy link
Contributor

Choose a reason for hiding this comment

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

Would address #1042 as well.

@tarcieri
Copy link
Member

One complication here is I'd like to attempt to migrate to a generic field element implementation (#1311) which wouldn't support bespoke reduce impls like this, and would only support generic zero and nonzero reductions for a modulo-sized integer and a double sized "wide" integer.

I can potentially merge this for now, I'm just saying I can't guarantee it will stick around, and it would be up to the caller to concat zeros to make a U512, then reduce.

@CreeptoGengar
Copy link
Author

One complication here is I'd like to attempt to migrate to a generic field element implementation (#1311) which wouldn't support bespoke reduce impls like this, and would only support generic zero and nonzero reductions for a modulo-sized integer and a double sized "wide" integer.

I can potentially merge this for now, I'm just saying I can't guarantee it will stick around, and it would be up to the caller to concat zeros to make a U512, then reduce.

Makes sense! This won't fit the generic architecture. U384 -> U512 conversion is cleaner.

I'm cool with merging this as-is. When the generic implementation lands, users can just do the zero-padding and use standard reduction I guess

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ReduceNonZero should require at least minimum recommended bits
3 participants