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.

Sign up for GitHub

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

Jump to bottom

Signature Subtraction function #67

Merged

yeastplume merged 5 commits into mimblewimble:master from yeastplume:sig_neg

May 16, 2023

Member

yeastplume commented May 4, 2023

WIP - Due to ambiguity around nonce y-coordinates, the current version returns two separate possible signatures in some cases (both of which much be tried for the purposes of payment proofs.

Debugging output left in for the moment, to run this test only you may want to comment or def out everything in src/tests.c other than run_aggsig_tests()


          subtraction function which potentially returns two possible results

79f6ef6

Member Author

yeastplume commented May 4, 2023

relevant keybase conversation starts here: keybase://chat/grincoin.teams.node_dev#general/8880


          final arg checks + removal of debug printf output

b022f36

yeastplume changed the title ~~[WIP] Signature Subtraction function~~ Signature Subtraction function


          revert modifications to main test fike

8069aac

Member Author

yeastplume commented May 9, 2023

This is ready for review now. Note that I opted to have a single function that returns whether the caller needs to check the second value to verify. This works well with the rust side, which can just return an tuple containing a result and an optional second possible result only if needed.

yeastplume requested review from phyro, tromp and quentinlesceller

May 9, 2023 14:24

yeastplume mentioned this pull request

Aggsig signature subtraction function, rust side mimblewimble/rust-secp256k1-zkp#81

Merged

tromp reviewed

View reviewed changes

src/modules/aggsig/main_impl.h

+                  if (!secp256k1_fe_set_b32(&noncesum_fe, sig64)) {
+                      return 0;
+                  }
+                  secp256k1_ge_set_xquad(&noncesum_ge, &noncesum_fe);

tromp May 9, 2023 •

edited

I don't know what all these 4 calls do. Could you add comments for each? I wish I could just click on the name and be taken to their definition, but sadly I don't have such an editor.

Member Author

yeastplume May 10, 2023

I don't have an editor set up for ansi-C code traversal either, but core secp256k1 functions are all documented in src/field.h for field elements functions and src/group.h for group elements.

secp256k1-zkp/src/field.h

Line 74 in 8d1f5bb

static int secp256k1_fe_set_b32(secp256k1_fe *r, const unsigned char *a);

secp256k1-zkp/src/group.h

Line 50 in 8d1f5bb

static int secp256k1_ge_set_xquad(secp256k1_ge *r, const secp256k1_fe *x);

tromp May 10, 2023

I mean you should add comments in the style of "/* Parse nonce sum total and negated version (R, -R) */" for every reader's benefit, so it's clear why you need to call that specific function...

tromp reviewed

View reviewed changes

src/modules/aggsig/main_impl.h

+                  if (!secp256k1_fe_set_b32(&noncepartial_fe, partial64)) {
+                      return 0;
+                  }
+                  secp256k1_ge_set_xquad(&noncepartial_ge, &noncepartial_fe);

tromp May 9, 2023

Similarly for these 2.

Member Author

yeastplume May 10, 2023

should be in the same file as above

tromp reviewed

View reviewed changes

src/modules/aggsig/main_impl.h Outdated

+                  secp256k1_gej_add_ge(&nonceresult_gej_neg, &noncesum_gej_neg, &noncepartial_ge_neg);
+                  if (secp256k1_gej_has_quad_y_var(&nonceresult_gej)) {
+                      pos_version_has_quad = 1;

tromp May 9, 2023

These *_has_quad variables should just be booleans that you assign to, e.g.

pos_version_has_quad = secp256k1_gej_has_quad_y_var(&nonceresult_gej);

Member Author

yeastplume May 10, 2023

tweaked

tromp reviewed

View reviewed changes

src/modules/aggsig/main_impl.h Outdated

+                      return 2;
+                  }
+                  return 0;

tromp May 9, 2023

This looks unreachacble.

Member Author

yeastplume May 10, 2023

it is, removed

tromp reviewed

View reviewed changes

src/modules/aggsig/main_impl.h Outdated

+                      secp256k1_fe_get_b32(result, &final.x);
+                      return 1;
+                  } else {
+                      /* if both, we need to return both possibilities */

tromp May 9, 2023

Huh, this is either both or neither.

tromp May 10, 2023

Still don't see this addressed. You should only return 2 when you tested both *_has_quad...

tromp reviewed

View reviewed changes

src/modules/aggsig/tests_impl.h Outdated

+                          }
+                          CHECK(sub_check);
+                      }
+                   }

tromp May 9, 2023

Missing test for neither, i.e. 0 results.

Member Author

yeastplume May 10, 2023

It doesn't happen due to (I think) the manner in which nonces are flipped during the partial sign operation, to demonstrate here's a modded version and the output from tests. Our tests run 20 invocations of this function, and I've run this test hundreds of times now.

    if (pos_version_has_quad && !neg_version_has_quad) {
        secp256k1_ge_set_gej(&final, &nonceresult_gej);
        secp256k1_fe_normalize_var(&final.x);
        secp256k1_fe_get_b32(result, &final.x);
        printf("POS HAS QUAD, NEG DOES NOT\n");
        return 1;
    } else if (!pos_version_has_quad && neg_version_has_quad) {
        secp256k1_ge_set_gej(&final, &nonceresult_gej_neg);
        secp256k1_fe_normalize_var(&final.x);
        secp256k1_fe_get_b32(result, &final.x);
        printf("NEG HAS QUAD, POS DOES NOT\n");
        return 1;
    } else if (!pos_version_has_quad && !neg_version_has_quad) {
        printf("!!!!!!!!!!!!!THIS DOESN'T HAPPEN!!!!!!!!!!!!!!!\n");
    } else {
        printf("BOTH HAVE QUAD\n");
        /* if both, we need to return both possibilities */
        secp256k1_ge_set_gej(&final, &nonceresult_gej);
        secp256k1_fe_normalize_var(&final.x);
        secp256k1_fe_get_b32(result, &final.x);

        secp256k1_ge_set_gej(&final, &nonceresult_gej_neg);
        secp256k1_fe_normalize_var(&final.x);
        secp256k1_fe_get_b32(result_alt, &final.x);
        return 2;
    }

And the output:

test count = 64
random seed = 290bdc25ab4fd42b4475f13ee818f9e5
START INVOCATION
BOTH HAVE QUAD
END INVOCATION

START INVOCATION
BOTH HAVE QUAD
END INVOCATION

START INVOCATION
POS HAS QUAD, NEG DOES NOT
END INVOCATION

START INVOCATION
BOTH HAVE QUAD
END INVOCATION

START INVOCATION
POS HAS QUAD, NEG DOES NOT
END INVOCATION

START INVOCATION
BOTH HAVE QUAD
END INVOCATION

START INVOCATION
NEG HAS QUAD, POS DOES NOT
END INVOCATION

START INVOCATION
POS HAS QUAD, NEG DOES NOT
END INVOCATION

START INVOCATION
POS HAS QUAD, NEG DOES NOT
END INVOCATION

START INVOCATION
POS HAS QUAD, NEG DOES NOT
END INVOCATION

START INVOCATION
BOTH HAVE QUAD
END INVOCATION

START INVOCATION
NEG HAS QUAD, POS DOES NOT
END INVOCATION

START INVOCATION
BOTH HAVE QUAD
END INVOCATION

START INVOCATION
NEG HAS QUAD, POS DOES NOT
END INVOCATION

START INVOCATION
BOTH HAVE QUAD
END INVOCATION

START INVOCATION
BOTH HAVE QUAD
END INVOCATION

START INVOCATION
BOTH HAVE QUAD
END INVOCATION

START INVOCATION
BOTH HAVE QUAD
END INVOCATION

START INVOCATION
NEG HAS QUAD, POS DOES NOT
END INVOCATION

START INVOCATION
BOTH HAVE QUAD
END INVOCATION

tromp May 10, 2023

It can't happen if you use QR partial nonces. But I think if you subtract a random nonce Rb from a random nonce Rb it should be possible for both Ra-Rb and -Ra-Rb to be non-QR?!

Member Author

yeastplume May 10, 2023

Ahh.. okay I think this is where we're not on the same page. I'm assuming that we're only every only going to be subtracting our signatures, which always have QR nonces. You're thinking more general case, signatures that won't be produced by us, is that correct?

Assuming we're only dealing with 32 byte x-values with implicit Y based on convention (in our case where Y is a QR).. this method isn't really 'compatible' with other conventions, but you're suggesting covering those cases anyhow by returning 0 instead?

tromp May 10, 2023

I thought "Returns: 0 on failure" could also be interpreted as saying 0 of the subtraction outcomes are valid QR. So yes, I thought this should apply to arbitrarily constructed signatures.

Member Author

yeastplume May 11, 2023

Okay, that's fine. all the existing functions return 0 on failure, but that can mean an argument error (like providing null for an intended out value) or various other things.

I've disambiguated this function at least to provide a 0 on an argument error as before, and return -1 if it's been provided two signatures resulting in no possible QR result. This still won't happen to callers who've only used this lib to generate signatures, but at least the case is covered.

everything else addressed as of last push, I think.

tromp requested changes

View reviewed changes

tromp left a comment

See inline comments.

yeastplume added 2 commits

May 10, 2023 11:03


          tweaks based on feedback

45cfe0a


          add case to handle possibility where two provided sigs result in subt…

a03783f

…raction with no possible x value with corresponding y QR

tromp approved these changes

View reviewed changes

tromp left a comment

Ok; good to go.

yeastplume merged commit b247e1e into mimblewimble:master

yeastplume mentioned this pull request

core wrapper function for subtract signature function mimblewimble/grin#3748

Merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment