-
Notifications
You must be signed in to change notification settings - Fork 27
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
[3.1 -> main] Add subjective limit to mod_exp host function #742
Merged
Merged
Changes from all commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
737baf5
Add subjective limit to mod_exp host function.
arhag 8cbd077
Update fc submodule.
arhag 65573e6
Merge pull request #739 from eosnetworkfoundation/arhag/GH-361-mod-ex…
arhag 8bdf1d5
Merge branch 'release/3.1.x' into arhag/GH-361-mod-exp-limits-for-main
arhag 6e72e79
Update fc submodule.
arhag File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How was the 2048 bit constraint chosen?
It's normal to have 4096 bit modulus nowadays, especially with root certificates.
If there isn't big performance hit (CPU, RAM) I'd suggest to lift this constraint to 512.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See reasoning in this comment within the code:
mandel/unittests/crypto_primitives_tests.cpp
Lines 415 to 423 in 6514a7d
The 5 ms is somewhat arbitrary. The idea is to not have it be too large of a fraction of the time it takes to deliver a complete block to the next producer to avoid missing blocks. If we assume a typical window of 100 ms time given based on configuration in the producer plugin, then bounding it to 5 ms eats up less than 5% of that time reserved to complete the block, sign it, and then start working on the next one (if not at the end of the round for that producer) or deliver over the network to the next producer in the schedule (if at the end of the round for that producer in which case they typically have more time available as well than 100 ms based on default configurations).
While bumping up to 4096-bit is still well within the typical 30 ms deadline limit, it ends up eating a significant portion of that 100 ms delivery window (around 20% instead of 5%). So we weren't very comfortable pushing it that far just yet.
The proper solution is to remove these arbitrary bit-size limits and just use deadline time as the only practical bound on what calls to
mod_exp
are allowed. Unfortunately, that requires plumbing the deadline timer into the inner loops of the modular exponentiation implementation which is something we intend to do (https://github.com/eosnetworkfoundation/mandel/issues/738) but is not something we are able to do for the 3.1.0 release.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm now going with a more flexible constraint on the bit sizes which should allow for 4096-bit RSA to work.
See https://github.com/eosnetworkfoundation/mandel/issues/747 and #749.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh that's great change!
Btw do you maybe have any metrics how long does it take to perform mod. exp. for 4096 bit modulus with current implementation? Hitting 5ms or more for 4096 bit sounds a bit too much. (Sorry I'm unable to perform the benchmark myself at the moment).
Just for reference, I made dumb benchmark for powm implementation from
eosio.ck
(originally from u-boot) and compiled with clang [64bit -O3 -stdlib=libc++ -std=c++2a].I get ~500us for single mod. exp. operation over 4096 bit modulus & base and on average ~468us per operation running in the loop 1M times (~2100 oper./s). Test vector was taken from Google's project Wycheproof.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On my machine,
mod_exp
with 4096-bit modulus and base, and a 32-bit exponent took less than 200 microseconds.