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

[3.1 -> main] Add subjective limit to mod_exp host function #742

Merged
merged 5 commits into from
Aug 3, 2022

Conversation

arhag
Copy link
Member

@arhag arhag commented Aug 3, 2022

Brings #739 to main.

Resolves #361.

@arhag arhag requested a review from larryk85 August 3, 2022 04:55
@arhag arhag merged commit 195face into main Aug 3, 2022
@arhag arhag deleted the arhag/GH-361-mod-exp-limits-for-main branch August 3, 2022 05:55
@@ -158,6 +158,14 @@ namespace eosio { namespace chain { namespace webassembly {
span<const char> exp,
span<const char> modulus,
span<char> out) const {
if (context.control.is_producing_block()) {
static constexpr int byte_size_limit = 256; // Allow up to 2048-bit values for base, exp, and modulus.
Copy link

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.

Copy link
Member Author

@arhag arhag Aug 4, 2022

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:

// Given the need to respect the deadline timer and the current limitation that the deadline timer is not plumbed into the
// inner loops of the implementation of mod_exp (which currently exists in the gmp shared library), only a small enough duration for
// mod_exp can be tolerated to avoid going over the deadline timer by too much. A good threshold for small may be less than 5 ms.
// Then based on benchmarks within the test_modular_arithmetic test within fc, it seems safe to limit the bit size to 2048 bits.
// This test case verifies that the subjective bit size limit for mod_exp is properly enforced within libchain.
// To allow mod_exp to be more useful, the limits on bit size need to be removed and the deadline timer plumbing into the implementation
// needs to occur. When that happens, this test case can be removed.

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.

Copy link
Member Author

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.

Copy link

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.

Copy link
Member Author

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.

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.

Add subjective limit for mod_exp
3 participants