Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bridge pcurves into the main elliptic curve arithmetic #4143

Merged
merged 2 commits into from
Aug 10, 2024

Conversation

randombit
Copy link
Owner

@randombit randombit commented Jun 22, 2024

This results in roughly a 2x-3x speedup for all elliptic curve algorithms, when using a supported curve.

@randombit randombit added this to the Botan 3.6.0 milestone Jun 22, 2024
@randombit randombit requested a review from reneme June 22, 2024 11:29
@randombit randombit changed the base branch from jack/new-ec-types to master June 24, 2024 11:26
@reneme
Copy link
Collaborator

reneme commented Jun 25, 2024

Without a full picture, yet: I think it would make sense to allow disabling the BN-based ECC implementation at compile time.

@randombit
Copy link
Owner Author

I think it would make sense to allow disabling the BN-based ECC implementation at compile time.

In principle yes I agree. However it's greatly complicated by the fact that we have large swaths of public API which expose various BigInt based ec functionality (EC_Group::multiply_mod_order, etc). This is one of those things that I think is best deferred to Botan 4 - once all of these various APIs are removed we have a much better shot at being able to do this cleanly.

@reneme
Copy link
Collaborator

reneme commented Jun 25, 2024

To be able to get an idea of the big picture, I extended the diagram I started at some point. Feedback appreciated. :)

image
excalidraw.com

Edit: updated with the added structures from #4203.

Note that excalidraw isn't a shared collaboration space. To publish any modifications, one has to re-create a new snapshot via "save as... > shareable link"

src/lib/pubkey/ec_group/ec_inner_data.cpp Outdated Show resolved Hide resolved
src/lib/pubkey/ec_group/ec_inner_data.h Outdated Show resolved Hide resolved
src/lib/pubkey/ec_group/ec_inner_pc.cpp Outdated Show resolved Hide resolved
src/lib/pubkey/ec_group/ec_inner_pc.cpp Outdated Show resolved Hide resolved
src/lib/pubkey/ec_group/ec_inner_pc.cpp Outdated Show resolved Hide resolved
src/lib/pubkey/ec_group/ec_inner_pc.cpp Outdated Show resolved Hide resolved
src/lib/pubkey/ec_group/info.txt Outdated Show resolved Hide resolved
src/lib/pubkey/ec_group/ec_inner_data.cpp Outdated Show resolved Hide resolved
src/lib/pubkey/ec_group/ec_inner_data.cpp Outdated Show resolved Hide resolved
@randombit randombit force-pushed the jack/bridge-pcurves-and-ec branch 4 times, most recently from 45b7e82 to f5c09c6 Compare July 12, 2024 01:41
@coveralls
Copy link

coveralls commented Jul 12, 2024

Coverage Status

coverage: 91.273% (-0.03%) from 91.298%
when pulling 1fca425 on jack/bridge-pcurves-and-ec
into 6a88af1 on master.

@randombit
Copy link
Owner Author

Comparing ECDSA vs ECDSA-pcurves (with RFC 6979 disabled) it appears the overhead of the various abstraction layers is betwee 2K and 8K cycles, which is not horrible in this context.

@randombit randombit force-pushed the jack/bridge-pcurves-and-ec branch 5 times, most recently from 51c9ee0 to ace1146 Compare July 20, 2024 10:22
@randombit randombit force-pushed the jack/bridge-pcurves-and-ec branch 2 times, most recently from 40f0073 to a6a4dd2 Compare July 26, 2024 09:25
@randombit
Copy link
Owner Author

@reneme Ready for another look

Copy link
Collaborator

@reneme reneme left a comment

Choose a reason for hiding this comment

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

Some minor documentation inconsistencies, a suggestion for more explicit memory management and some C++ Klugscheißerei.

src/lib/pubkey/ec_group/ec_inner_data.cpp Outdated Show resolved Hide resolved
src/lib/pubkey/ec_group/ec_inner_data.cpp Show resolved Hide resolved
src/lib/pubkey/ec_group/ec_inner_data.cpp Show resolved Hide resolved
src/lib/pubkey/ec_group/ec_inner_pc.cpp Outdated Show resolved Hide resolved
Comment on lines +134 to +140
const size_t fe_bytes = this->field_element_bytes();
BOTAN_ARG_CHECK(bytes.size() == fe_bytes, "Invalid output size");
copy_mem(bytes, m_x_bytes);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: copy_mem does that size check internally, same in some methods that follow. Obviously, with a less clear error message.
Please bear with me if I did point that out before and you decided against it. 😓

Copy link
Owner Author

Choose a reason for hiding this comment

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

Intentional for the error message - this function is (at least through several layers) reachable by user code, and while they could probably figure it out just from a generic error message it seemed nicer to be a bit more explicit. TBH I think of copy_mems check as more of a fallback for cases we forget.

src/lib/pubkey/ec_group/ec_inner_pc.cpp Outdated Show resolved Hide resolved
@randombit randombit force-pushed the jack/bridge-pcurves-and-ec branch 3 times, most recently from fcdeba9 to f76300f Compare August 10, 2024 00:22
randombit and others added 2 commits August 9, 2024 20:34
Now any uses of the standard (public) interfaces for elliptic curve
arithmetic automatically use the faster implementation, whenever
a supported group is used.

Co-authored-by: René Meusel <github@renemeusel.de>
@randombit randombit merged commit a27f5a2 into master Aug 10, 2024
40 checks passed
@randombit randombit deleted the jack/bridge-pcurves-and-ec branch August 10, 2024 04:49
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.

3 participants