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

Set Miller-Rabin rounds based on bitsize #58

Closed
wants to merge 1 commit into from
Closed

Set Miller-Rabin rounds based on bitsize #58

wants to merge 1 commit into from

Conversation

@adamantike
Copy link
Contributor

@adamantike adamantike commented Mar 28, 2016

Number of rounds now depends on bitsize, according to FIPS 186-4. It runs for (minimum + 1) rounds.

@coveralls
Copy link

@coveralls coveralls commented Mar 28, 2016

Coverage Status

Coverage decreased (-0.6%) to 90.292% when pulling 29f7b0a on adamantike:miller-rabin-rounds into e5839ee on sybrenstuvel:master.

Loading

@sybrenstuvel
Copy link
Owner

@sybrenstuvel sybrenstuvel commented Mar 29, 2016

Please refactor so that determining the number of rounds needed is in its own function, and add unittests.

It's also quite a lot of code for something that's essentially:

if bitsize >= 1536:
    return 3
if bitsize >= 1024:
    return 4
if bitsize >= 512:
    return 7
return 10

Loading

@coveralls
Copy link

@coveralls coveralls commented Apr 15, 2016

Coverage Status

Coverage increased (+0.1%) to 91.462% when pulling 85275a9 on adamantike:miller-rabin-rounds into 96eaa5e on sybrenstuvel:master.

Loading

@coveralls
Copy link

@coveralls coveralls commented Apr 15, 2016

Coverage Status

Coverage increased (+0.1%) to 91.462% when pulling 85275a9 on adamantike:miller-rabin-rounds into 96eaa5e on sybrenstuvel:master.

Loading

@adamantike
Copy link
Contributor Author

@adamantike adamantike commented Apr 15, 2016

That was my first approach, but I kind of over-thought it by trying not to add new if ...: return... blocks when adding new cases in the future. Actually, being k = 3 the current minimum, there won't be a lot new cases :P

Loading

@sybrenstuvel
Copy link
Owner

@sybrenstuvel sybrenstuvel commented Apr 15, 2016

Great! Merged in 38a7255

Loading

@adamantike adamantike deleted the miller-rabin-rounds branch Apr 15, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants