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

Tighter verification bounds in mul/sqr #167

Closed

Conversation

peterdettman
Copy link
Contributor

I reviewed the VERIFY_BITS bounds-checking for 32bit and 64bit mul/sqr implementations, finding various places where stricter bounds can be specified (I found no cases of over-strictness).

@sipa
Copy link
Contributor

sipa commented Dec 17, 2014

So, the numbers I came up for the verification are to the extent possible, "mechanical", meaning that you only need to look at previous bounds of input variables, and the operations done on them, to obtain the new ones. Those are very likely not the smallest possible ones, but their only purpose is for review/testing to assure that no overflows happen. I'd like to keep the numbers that way, as they are easy to verify locally ("pick a random piece of a lines of code, and verify that - assuming former bounds are correct - the newer ones are correct to").

Is this what you're doing too, or actually trying to find the strictest bounds possible?

@peterdettman
Copy link
Contributor Author

I did strict calculations where they were already present. There are at least a couple of larger corrections which should at least be made (relating to products of the top limb(s)), which kind of led me to review the rest. Most of the rest is reasoned like e.g. a quantity is 63 bits because it is the sum of 5 30x30-bit products, which actually "leaves room for" 3 more 60-bit values, the carry-in accounts for only one of them, so this extra add fits too... this sort of reasoning. It depends on more information than just the previous VERIFY_BITS statement, so probably this is what you'd rather not have?.

@gmaxwell gmaxwell added this to the initial release milestone Aug 31, 2015
@real-or-random
Copy link
Contributor

This is ages old, and I agree with @sipa that there's no point for this in the current code. (And 6 years later we're more confident in the current code. :) There's #811 and #815 of course, but these are separate efforts.

Closing this. Feel free to object.

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.

4 participants