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

Fix bugs in mpint/tommath_ext_features #116

Merged
merged 15 commits into from
Sep 5, 2023
Merged

Fix bugs in mpint/tommath_ext_features #116

merged 15 commits into from
Sep 5, 2023

Conversation

maths644311798
Copy link
Contributor

What problem does this PR solve?

Issue Number: Fixed #107

Possible side effects?

  • Performance: Improve Performance

  • Backward compatibility: No side effects

  • Security: Secure

@anakinxc anakinxc requested a review from zheyang0825 August 11, 2023 03:20
@zheyang0825
Copy link

LGTM

@Jamie-Cui Jamie-Cui requested a review from usafchn August 17, 2023 08:28
@maths644311798
Copy link
Contributor Author

maths644311798 commented Aug 19, 2023

I request new reviews.
First, I figure out one more problem.

change MPINT_ENFORCE_OK(mp_mod_d(&q, small_prime_prod, &mod));
to MPINT_ENFORCE_OK(mp_mod_d(&q, small_prime_prod, &original_m));

Second, I can not explain why adding mp_prime_is_prime(p, 1, &res) helps pass the unittest steadily. But we need this to pass the unittest. I have tested some values of t of mp_prime_is_prime. It seems that a big t can not guarantee the generated p is a prime. But a small t can. I believe this is due to the inner structure of libtommath. But I don’t want to read all details of libtommath. I have tried to move

MPINT_ENFORCE_OK(mp_prime_is_prime(p, 1, &res));
if (!res) {
continue;
}

to the place above

MPINT_ENFORCE_OK(mp_prime_is_prime(p, t, &res));

But this does not work. I guess it may be due to the random source of libtommath.

Also note that pocklington criterion is deterministic: If q is prime and p passes pocklington criterion, then p must be prime. So we only need to do Lucas, Rabin, etc prime tests on q.

decrease the test times
@maths644311798
Copy link
Contributor Author

The failure of channel_brpc_blackbox_test in the unittest should not be a consequence of this pull request.

@anakinxc
Copy link
Collaborator

The failure of channel_brpc_blackbox_test in the unittest should not be a consequence of this pull request.

I have restarted the unittest

yacl/math/mpint/tommath_ext_features.cc Show resolved Hide resolved
yacl/math/mpint/tommath_ext_features.cc Outdated Show resolved Hide resolved
yacl/math/mpint/tommath_ext_features.cc Outdated Show resolved Hide resolved
yacl/math/mpint/tommath_ext_features.cc Outdated Show resolved Hide resolved
Copy link
Member

@usafchn usafchn left a comment

Choose a reason for hiding this comment

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

great job

@usafchn usafchn merged commit 34f812a into secretflow:main Sep 5, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Sep 5, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Problems in yacl/math/mpint
4 participants