Skip to content

Conversation

@czurnieden
Copy link
Contributor

Removed, not only deprecated, removed support for 8-bit (MP_8BIT).
Deprecating was not really possible besides a simple

#else
/* pragma message() not supported by several compilers (in mostly older but still used versions) */
#  ifdef _MSC_VER
#    pragma message("8-bit (MP_8BIT) support is deprecated and will be dropped completely in the next version.")
#  else
#    warning "8-bit (MP_8BIT) support is deprecated and will be dropped completely in the next version."
#  endif
#endif

at the top of tommath.h.

I went through it with a fine comb but still might have missed some, please check.
If that is OK with you: there is also need for an "Issue" to announce it a bit more official and an entry in the Wiki. Both can only be done after the merge, of course, but should not be forgotten.

Copy link
Member

@minad minad left a comment

Choose a reason for hiding this comment

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

I agree with the general sentiment here. 8 bit needs way too many special cases.

I think the comments mention 7 bits at some places too. We should probably also remove MP_31BIT which is special. But if we do that we should probably discuss the pros and cons a bit.

If the algorithms work at different bit lenghts, we can trust them a bit more and it is ensured that no bit length dependent constants sneaked in. Then there is also the case with microcontrollers. Is ltm used there with MP_8BIT? If we focus on more powerful platforms, why keep MP_16BIT? Only keep 64 and 32?

@minad minad self-requested a review September 9, 2019 08:03
@minad
Copy link
Member

minad commented Sep 9, 2019

Hmm, what I would prefer even more than removing support is, if we could get rid of the MP_8BIT special casing and all the algorithms would just work for each size. But this might be fantasy ;)

@sjaeckel
Copy link
Member

sjaeckel commented Sep 9, 2019

I'd say we deprecate it for 1.2.0 as proposed by a warning sign only and then merge this after 1.2.0 is released.

@czurnieden
Copy link
Contributor Author

We should probably also remove MP_31BIT which is special.

Not much of a problem, more or less only with the comba multiplier.
But we can deprecate it and if nobody complains…

Hmm, what I would prefer even more than removing support is, if we could get rid of the MP_8BIT special casing and all the algorithms would just work for each size.

Yes, we could do it. A lot of work but doable. But we get into one large problem here: the Lucas-Selfridge algorithm does not work with 8-bit, that's why the prime-test uses Frobenius-Underwood in that case. The BPSW algorithm is well analysed (and tested) with Lucas-Selfridge but not with Frobenius-Underwood. The intersection is quite large (that's why it is an alternative in this PR instead of using both. I even thought about removing F-U completely) but I'm very conservative in regards to cryptographic code and always prefer the well tested.

Also: yes, we can use the fixed/min width types from stdint.h but then you have the compiler's bigint with LTM, another bigint, on top. All of that needs to get to squeezed into the little memory 8-bit processors have together with the program using LTM and most likely other programs, too.

But as said in another PR: I try to get my hands on hardware and ask in the forums. Forums mightwill be faster but I probably need to do some…uhm…shmoozing first, please allow for a couple of days in that case.

If we focus on more powerful platforms, why keep MP_16BIT?

A lot of the 16-bit MCUs (and old CPUs) have enough memory (Flash and RAM) to run a stripped down LTM; some can even run a full version of LTM. Support of 16-bit is also no problem in contrast to 8-bit which is a real pain in the behind (mp_digit has a size of just 7 bits! One hundred and twenty-seven!).
And if you grep through LTM for MP_16BIT you won't find a lot of preprocessor ifs, just the Montgomery setup which can be refactored if you insist (and implement it, too ;-) ).

I'd say we deprecate it for 1.2.0 as proposed by a warning sign only and then merge this after 1.2.0 is released.

Yepp, sounds good to me.
Done in #351

@czurnieden
Copy link
Contributor Author

@minad flagging does not cause an email to me (or I misconfigured something here), it is quicker if you just shout atping me if you want me to do something quickly. Thanks!

@sjaeckel
Copy link
Member

lulz sorry @czurnieden I force-pushed over your rebase :)

@czurnieden
Copy link
Contributor Author

lulz sorry @czurnieden I force-pushed over your rebase :)

Your mean that I could have got me another coffee instead of fiddling with the merge conflicts?
*grrr*
;-)

@sjaeckel
Copy link
Member

Your mean that I could have got me another coffee instead of fiddling with the merge conflicts?

yep :-)

@sjaeckel sjaeckel merged commit 800ec1e into libtom:develop Oct 19, 2019
@sjaeckel sjaeckel removed the finished label Oct 20, 2019
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