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

Port fixes from bitcoin #9

Merged
merged 5 commits into from
Jul 16, 2024
Merged

Port fixes from bitcoin #9

merged 5 commits into from
Jul 16, 2024

Conversation

Naviabheeman
Copy link
Contributor

Port:

  • #1307 Bugfix: mark outputs as early clobber in scalar x86_64 asm
  • #1303 ct: Use volatile trick in scalar_cond_negate
  • #1344 group: remove unneeded normalize_weak in secp256k1_ge_is_valid_var
  • #1358 tests: refactor: remove duplicate function random_field_element_test
  • #1446 field: Remove x86_64 asm

Additionally:

  • Upgrade CI to ubuntu22
  • Fix compiler warnings in schnorr tests

Mark more assembly outputs as early clobber #1307
Use more volatile in temporarties #1303
save normalize_weak calls in secp256k1_gej_eq_x_var #1344
remove random_field_element_test and use random_fe_test instead
upgrade CI to ubuntu22
@Naviabheeman Naviabheeman requested a review from azuchi July 4, 2024 06:30
Copy link
Contributor

@azuchi azuchi left a comment

Choose a reason for hiding this comment

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

src/tests.c Outdated
@@ -96,7 +96,7 @@ void random_group_element_test(secp256k1_ge *ge) {
void random_group_element_jacobian_test(secp256k1_gej *gej, const secp256k1_ge *ge) {
secp256k1_fe z2, z3;
do {
random_field_element_test(&gej->z);
random_fe_test(&gej->z);
Copy link
Contributor

Choose a reason for hiding this comment

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

In #1358, the following multiple lines are summarized:

  do {
        random_field_element_test(&gej->z);
        if (!secp256k1_fe_is_zero(&gej->z)) {
            break;
        }
    } while(1);

Copy link
Contributor Author

@Naviabheeman Naviabheeman Jul 12, 2024

Choose a reason for hiding this comment

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

Yes, in our library I only removed the redundant random_field_element_test and used random_fe_test. Summarising the loop with random_fe_non_zero_test was not my initial intent as I wanted to make minimal change. I'll port it since it is causing confusion.

src/tests.c Outdated
@@ -1930,7 +1921,7 @@ void test_ge(void) {
if (i == 0) {
/* The point at infinity does not have a meaningful z inverse. Any should do. */
do {
random_field_element_test(&zs[i]);
random_fe_test(&zs[i]);
Copy link
Contributor

Choose a reason for hiding this comment

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

In #1356, the while loop itself has also disappeared.

src/tests.c Outdated
@@ -1942,7 +1933,7 @@ void test_ge(void) {

/* Generate random zf, and zfi2 = 1/zf^2, zfi3 = 1/zf^3 */
do {
random_field_element_test(&zf);
random_fe_test(&zf);
Copy link
Contributor

Choose a reason for hiding this comment

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

In #1356, the while loop itself has also disappeared.

src/group_impl.h Show resolved Hide resolved
remove secp256k1_fe_normalize_weak from secp256k1_ge_is_valid_var
port random_fe_non_zero_test and its usage
@Naviabheeman Naviabheeman requested a review from azuchi July 15, 2024 13:51
@azuchi azuchi merged commit ab8bcb7 into chaintope:master Jul 16, 2024
1 check passed
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.

2 participants