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.

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

Signature Subtraction function #67

Merged
merged 5 commits into from
May 16, 2023
Merged

Signature Subtraction function #67

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
79f6ef6
subtraction function which potentially returns two possible results
yeastplume May 4, 2023
b022f36
final arg checks + removal of debug printf output
yeastplume May 9, 2023
8069aac
revert modifications to main test fike
yeastplume May 9, 2023
45cfe0a
tweaks based on feedback
yeastplume May 10, 2023
a03783f
add case to handle possibility where two provided sigs result in subt…
yeastplume May 11, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 19 additions & 0 deletions include/secp256k1_aggsig.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,25 @@ SECP256K1_API int secp256k1_aggsig_partial_sign(
size_t index
) SECP256K1_ARG_NONNULL(1) SECP256K1_ARG_NONNULL(2) SECP256K1_ARG_NONNULL(3) SECP256K1_ARG_NONNULL(4) SECP256K1_ARG_NONNULL(5) SECP256K1_WARN_UNUSED_RESULT;

/** Subtract a partial signature from a completed signature, resulting in another partial signature
*
* Returns: 0 on failure, 1 if there is only one possible result, 2 if there is a second possiblity due
* to two possiblities for the chosen y value of the original public nonce
* Args: ctx: an existing context object, initialized for verifying (cannot be NULL)
* Out: result: the (partial) signature resulting from subtracting partial from sig64
* result_alt: alternative possible signature resulting from subtracting partial from sig64
* In: sig64: a completed signature
* partial64: the signature (partial) to subtract from sig64
*/

SECP256K1_API int secp256k1_aggsig_subtract_partial_signature(
const secp256k1_context* ctx,
unsigned char *result,
unsigned char *result_alt,
const unsigned char *sig64,
const unsigned char *partial64
) SECP256K1_ARG_NONNULL(1) SECP256K1_ARG_NONNULL(2) SECP256K1_ARG_NONNULL(3) SECP256K1_ARG_NONNULL(4) SECP256K1_ARG_NONNULL(5) SECP256K1_WARN_UNUSED_RESULT;


/** Aggregate multiple signature parts into a single aggregated signature
*
Expand Down
110 changes: 110 additions & 0 deletions src/modules/aggsig/main_impl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -346,6 +346,116 @@ int secp256k1_aggsig_partial_sign(const secp256k1_context* ctx, secp256k1_aggsig
return 1;
}

int secp256k1_aggsig_subtract_partial_signature(
const secp256k1_context* ctx,
unsigned char* result,
unsigned char* result_alt,
const unsigned char *sig64,
const unsigned char *partial64
) {
secp256k1_scalar tmp, tmp2;
secp256k1_fe noncesum_fe;
secp256k1_ge noncesum_ge;
secp256k1_gej noncesum_gej;
secp256k1_ge noncesum_ge_neg;
secp256k1_gej noncesum_gej_neg;
secp256k1_fe noncepartial_fe;
secp256k1_ge noncepartial_ge;
secp256k1_ge noncepartial_ge_neg;
secp256k1_gej nonceresult_gej;
secp256k1_gej nonceresult_gej_neg;
secp256k1_ge final;
int overflow;
int neg_version_has_quad = 0;
int pos_version_has_quad = 0;

VERIFY_CHECK(ctx != NULL);
ARG_CHECK(result != NULL);
ARG_CHECK(result_alt != NULL);
ARG_CHECK(sig64 != NULL);
ARG_CHECK(partial64 != NULL);
(void) ctx;

/* Scalar portion */
secp256k1_scalar_set_b32(&tmp, sig64 + 32, &overflow);
if (overflow) {
secp256k1_scalar_clear(&tmp);
return 0;
}
secp256k1_scalar_set_b32(&tmp2, partial64 + 32, &overflow);
if (overflow) {
secp256k1_scalar_clear(&tmp);
secp256k1_scalar_clear(&tmp2);
return 0;
}
secp256k1_scalar_negate(&tmp2, &tmp2);
secp256k1_scalar_add(&tmp, &tmp, &tmp2);

secp256k1_scalar_get_b32(result + 32, &tmp);
secp256k1_scalar_get_b32(result_alt + 32, &tmp);
secp256k1_scalar_clear(&tmp);
secp256k1_scalar_clear(&tmp2);

/* nonce portion
* Note that we are unable to determine with 100% certainty
* what nonce was originally chosen due to only the x coordinate
* being stored. We can sometimes determine which was correct, but
* may have to return a second possibility.
*/

/* Parse nonce sum total and negated version (R, -R) */
if (!secp256k1_fe_set_b32(&noncesum_fe, sig64)) {
return 0;
}
secp256k1_ge_set_xquad(&noncesum_ge, &noncesum_fe);
Copy link

@tromp tromp May 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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);

Copy link

Choose a reason for hiding this comment

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

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...

secp256k1_gej_set_ge(&noncesum_gej, &noncesum_ge);
secp256k1_ge_neg(&noncesum_ge_neg, &noncesum_ge);
secp256k1_gej_set_ge(&noncesum_gej_neg, &noncesum_ge_neg);

/* Parse provided partial-sig nonce and negate */
if (!secp256k1_fe_set_b32(&noncepartial_fe, partial64)) {
return 0;
}
secp256k1_ge_set_xquad(&noncepartial_ge, &noncepartial_fe);
Copy link

Choose a reason for hiding this comment

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

Similarly for these 2.

Copy link
Member Author

Choose a reason for hiding this comment

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

should be in the same file as above

secp256k1_ge_neg(&noncepartial_ge_neg, &noncepartial_ge);

/* Try positive (Rr = R-Rs) */
secp256k1_gej_add_ge(&nonceresult_gej, &noncesum_gej, &noncepartial_ge_neg);

/* Now try neg (Rr = -R-Rs) */
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;
Copy link

Choose a reason for hiding this comment

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

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);

Copy link
Member Author

Choose a reason for hiding this comment

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

tweaked

}
if (secp256k1_gej_has_quad_y_var(&nonceresult_gej_neg)) {
neg_version_has_quad = 1;
}
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);
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);
return 1;
} else {
/* if both, we need to return both possibilities */
Copy link

Choose a reason for hiding this comment

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

Huh, this is either both or neither.

Copy link

Choose a reason for hiding this comment

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

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

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;
}

return 0;
Copy link

Choose a reason for hiding this comment

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

This looks unreachacble.

Copy link
Member Author

Choose a reason for hiding this comment

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

it is, removed

}

int secp256k1_aggsig_combine_signatures(const secp256k1_context* ctx, secp256k1_aggsig_context* aggctx, unsigned char *sig64, const secp256k1_aggsig_partial_signature *partial, size_t n_sigs) {
size_t i;
secp256k1_scalar s;
Expand Down
37 changes: 36 additions & 1 deletion src/modules/aggsig/tests_impl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,10 @@ void test_aggsig_api(void) {
unsigned char seckeys[5][32];
secp256k1_pubkey pubkeys[5];
secp256k1_aggsig_partial_signature partials[5];
unsigned char sub_result[64];
unsigned char sub_result_alt[64];
int sub_return_val;
int sub_check;
secp256k1_aggsig_context *aggctx;
unsigned char seed[32] = { 1, 2, 3, 4, 0 };
unsigned char sig[64];
Expand Down Expand Up @@ -134,6 +138,7 @@ void test_aggsig_api(void) {
CHECK(!secp256k1_aggsig_verify(vrfy, scratch, sig, msg, pubkeys, 0));
CHECK(secp256k1_aggsig_verify(vrfy, scratch, sig, msg, pubkeys, 5));
CHECK(ecount == 15);

CHECK(!secp256k1_aggsig_verify(none, scratch, sig, msg, pubkeys, 5));
CHECK(ecount == 16);

Expand Down Expand Up @@ -264,7 +269,37 @@ void test_aggsig_api(void) {
msg[2]=3;
CHECK(!secp256k1_aggsig_verify_single(vrfy, combined_sig, msg, NULL, &combiner_sum_2, NULL, NULL, 0));
CHECK(!secp256k1_aggsig_verify_single(vrfy, combined_sig, msg, NULL, &combiner_sum_2, &combiner_sum_2, NULL, 0));
}

/* Test subtracting partial sig from the completed sig (as used in early payment proofs)*/
sub_return_val = secp256k1_aggsig_subtract_partial_signature(none, sub_result, sub_result_alt, combined_sig, sigs[0]);
CHECK(sub_return_val);

/* If there's only one possible result, it should be the only one that needs to be tested */
if (sub_return_val == 1) {
for (j = 0; j < 64; j++){
CHECK(sub_result[j] == sigs[1][j]);
}
}

/* if there are two possible results, both potentially need to be checked */
if (sub_return_val == 2) {
sub_check = 1;
for (j = 0; j < 64; j++){
if (sub_result[j] != sigs[1][j]) {
sub_check = 0;
}
}
if (!sub_check) {
sub_check = 1;
for (j = 0; j < 64; j++){
if (sub_result_alt[j] != sigs[1][j]) {
sub_check = 0;
}
}
}
CHECK(sub_check);
}
}
Copy link

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link

Choose a reason for hiding this comment

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

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?!

Copy link
Member Author

Choose a reason for hiding this comment

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

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?

Copy link

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

/*** End aggsig for Grin exchange test ***/

/* cleanup */
Expand Down