-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Add unit tests for ecp_mod_p192k1() #7424
Add unit tests for ecp_mod_p192k1() #7424
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good and functionality works as intended. Minor remarks, which should not warrant a rework, considering we will be refactoring those methods in the coming weeks.
I will approve is second reviewer is happy with the state as is.
tests/suites/test_suite_ecp.function
Outdated
size_t limbs = limbs_N; | ||
size_t bytes = limbs * sizeof(mbedtls_mpi_uint); | ||
|
||
TEST_EQUAL(limbs_X, 2 * limbs); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should that be TEST_LE_U
given that the issue requirements are:
Only cases where the input is non-negative and at most twice as long as the modulus.
tests/suites/test_suite_ecp.function
Outdated
TEST_EQUAL(mbedtls_test_read_mpi(&N, input_N), 0); | ||
TEST_EQUAL(mbedtls_test_read_mpi(&res, result), 0); | ||
|
||
limbs_X = X.n; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we are using old mpi's I see no reason for intermediate assignments to limbs_X
, limbs_N
, as they can be directly be acccessed by X.n
, N.n
Signed-off-by: Gabor Mezei <[email protected]>
Signed-off-by: Gabor Mezei <[email protected]>
Signed-off-by: Gabor Mezei <[email protected]>
Signed-off-by: Gabor Mezei <[email protected]>
ea8a2ed
to
0a11ee6
Compare
Rebased |
Signed-off-by: Gabor Mezei <[email protected]>
Signed-off-by: Gabor Mezei <[email protected]>
Signed-off-by: Gabor Mezei <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good ( provided it passes internal CI )
The failure in the OpenCI appears to be caused by other issues
[2023-04-13T12:21:44.102Z] stdout:
[2023-04-13T12:21:44.102Z] stderr: fatal: unable to access 'https://github.com/Mbed-TLS/mbedtls.git/': Failed to connect to github.com port 443: Connection timed out
[2023-04-13T12:21:44.102Z]
[2023-04-13T12:21:44.102Z] at org.jenkinsci.plugins.gitclient.CliGitAPIImpl.launchCommandIn(CliGitAPIImpl.java:2734)
Signed-off-by: Gabor Mezei <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Just a reminder: all PRs related to this quarter's bignum work should have the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Description
The task is to add unit tests for ecp_mod_p192k1(). This function reduces the input modulo secp192k1_p.
Resolve #7256
Gatekeeper checklist