Skip to content

k256+p256: impl ff::Field trait for FieldElement types#498

Merged
tarcieri merged 1 commit into
masterfrom
k256+p256/impl-field-trait-for-fieldelement
Dec 23, 2021
Merged

k256+p256: impl ff::Field trait for FieldElement types#498
tarcieri merged 1 commit into
masterfrom
k256+p256/impl-field-trait-for-fieldelement

Conversation

@tarcieri

Copy link
Copy Markdown
Member

Allows for writing code which is generic over field elements, such as the optimized SWU implementation added in RustCrypto/traits#854

Allows for writing code which is generic over field elements, such as
the optimized SWU implementation added in RustCrypto/traits#854
@tarcieri tarcieri changed the title k256+p256: impl ff::Field trait for FieldElement k256+p256: impl ff::Field trait for FieldElement types Dec 23, 2021
@tarcieri

Copy link
Copy Markdown
Member Author

One thing I'm a bit worried about in regard to generic field arithmetic is lazy normalization used by k256. It feels like a potentially leaky abstraction.

@@ -1,5 +1,7 @@
//! Field arithmetic modulo p = 2^256 - 2^32 - 2^9 - 2^8 - 2^7 - 2^6 - 2^4 - 1

#![allow(clippy::assign_op_pattern, clippy::op_ref)]

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Left all of the current arithmetic borrowing as-is in order to not affect performance as part of this PR, but it's probably worth benchmarking removing these in a followup and seeing if it affects performance

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.

Yeah I can fix in a follow up PR. I did this crypto-bigint

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

It's really easy to fix automatically with cargo clippy --fix, but I just want to make sure it doesn't cause a performance regression

@@ -1,5 +1,7 @@
//! Affine points

#![allow(clippy::op_ref)]

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.

Could probably fix this in a subsequent PR

@mikelodder7

Copy link
Copy Markdown
Contributor

One thing I'm a bit worried about in regard to generic field arithmetic is lazy normalization used by k256. It feels like a potentially leaky abstraction.

How much performance do we loose if normalization is applied with each call? We could add a benchmark for it

@tarcieri

Copy link
Copy Markdown
Member Author

How much performance do we loose if normalization is applied with each call? We could add a benchmark for it

I think it could potentially be a lot for the core::ops::* traits.

@mikelodder7

Copy link
Copy Markdown
Contributor

How much performance do we loose if normalization is applied with each call? We could add a benchmark for it

I think it could potentially be a lot for the core::ops::* traits.

Hmm well not sure how to avoid the leaky then

@tarcieri

Copy link
Copy Markdown
Member Author

Given that FieldElement is an internal type, I think this is less worrisome than leaking these details outside the crate, but I would expect cases where a generic implementation may not do what you expect with k256's FieldElement type.

Going to go ahead and merge this, but I'd love to get some additional feedback if anyone has anything to add in perpetuity.

@tarcieri tarcieri merged commit f60fc7f into master Dec 23, 2021
@tarcieri tarcieri deleted the k256+p256/impl-field-trait-for-fieldelement branch December 23, 2021 18:30
@tarcieri tarcieri mentioned this pull request Jan 17, 2022
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.

2 participants