musig2 wrappers over libsecp #31

curiecrypt · 2022-11-12T00:47:23Z

This PR aims to update the library so that the end users do not need to make calls from secp256k1 as mentioned in #30. The following list provides the changes:

Library functions are updated not to take any secp256k1 parameters, including secp256k1_context. Context creation is now handled in library functions.
Defined constants renamed, i.e., SCALAR_BYTES is changed to MUSIG2_SCALAR_BYTES.
A new type (musig2_pubkey) representing the x_only public key of secp256k1 is defined. MuSig2 library does not require full-size public keys; thus, instead of calling secp256k1_xonly_pubkey, users will call musig2_pubkey.
The function musig2_prepare_signer_to_register is created. This function
- serializes signers' public key and batch commitments in compressed format,
- called by musig2_prepare_signer_to_register,
- returns serialized_pk (33-byte) and serialized_comm_list, which is a list of 33-byte commitments with V * NR_MESSAGES elements. So, after initialization, these return values can be directly used to register the signer (Collecting public keys and batch commitments.
Verification of the final signature with secp256k1_schnorrsig_verify is implemented in the library function musig2_verify_musig2.

Note that the documentation and comments are not completed yet.

This closes #30.

iquerejeta · 2022-11-15T11:42:58Z

src/libmusig2.c

@@ -193,46 +196,79 @@ void musig2_context_sig_free(musig2_context_sig *mcs) {
    musig2_context_free(&mcs->mc);
 }

+int musig2_prepare_signer_to_register(musig2_context_sig *mcs, unsigned char *serialized_pubkey, unsigned char serialized_batch_list[][MUSIG2_PUBKEY_BYTES_COMPRESSED]){


I believe this is only used internally. It could be static.

iquerejeta · 2022-11-15T11:52:26Z

src/libmusig2.c

+    if (secp256k1_keypair_pub(mcs->mc.ctx, &temp_pk, &mcs->keypair))
+        secp256k1_ec_pubkey_serialize(mcs->mc.ctx, serialized_pubkey, &ser_size, &temp_pk, SECP256K1_EC_COMPRESSED );
+    else {
+        musig2_context_sig_free(mcs);


Freeing the context within the function might result in a double free, or in using a freed pointer. This should be handled externally, as we might want to continue with the same context if the serialisation failed

iquerejeta · 2022-11-15T11:52:41Z

src/libmusig2.c

+        if (secp256k1_keypair_pub(mcs->mc.ctx, &temp_pk, mcs->comm_list[i]))
+            secp256k1_ec_pubkey_serialize(mcs->mc.ctx, serialized_batch_list[i], &ser_size, &temp_pk, SECP256K1_EC_COMPRESSED );
+        else {
+            musig2_context_sig_free(mcs);


iquerejeta · 2022-11-15T11:59:49Z

src/libmusig2.c

+    if (!musig2_prepare_signer_to_register(mcs, serialized_pubkey, serialized_comm_list)){
+        musig2_context_sig_free(mcs);
        return 0;


It seems that what prepare_signer_to_register really does is serialise the pubkey and batched commitments. What do you think of having a function serialise_shareable_context, that takes as input mcs and the two pointers for the serialised data? In this way we explicitly distinguish both calls

iquerejeta · 2022-11-15T12:32:00Z

src/libmusig2.h

+#define MUSIG2_PUBKEY_BYTES                 32          // XONLY PUBLIC KEY BYTES
+#define MUSIG2_PUBKEY_BYTES_COMPRESSED      33          // COMPRESSED PUBLIC KEY BYTES
+#define MUSIG2_PUBKEY_BYTES_FULL            64          // FULL SIZE PUBLIC KEY BYTES


It's counterintuitive that BYTES_COMPRESSED is larger than BYTES. I would find a different name to the former (maybe something to do with omit sign bit or something of the sort)

iquerejeta

I suggest that the serialisation is separated from the initialisation function.

iquerejeta

These changes look good. Have a look at my comments, and change what you consider, but other than that, happy to see this merged.

iquerejeta · 2022-11-16T15:56:18Z

src/libmusig2.c

+    if (!musig2_key_gen(mcs)) {
+        musig2_context_sig_free(mcs);
        return 0;
+    }

    /* Generate the batch commitments for given signer */
-    if (!musig2_batch_commitment(mcs))
+    if (!musig2_batch_commitment(mcs)) {
        return 0;
+    }



We should be consistent with freeing the context. Both errors are the same (=0), so the function caller will always react the same way for failures. This means that the outcome of both failures needs to be the same wrt freeing the context. Either we free the context in or out of the function, but we cannot do both.

iquerejeta · 2022-11-16T15:59:19Z

src/libmusig2.c

+
+int musig2_verify_musig2(unsigned char *signature, const unsigned char *msg, int msg_len, musig2_aggr_pubkey *aggr_pk){
+    int result;
+    secp256k1_context *ctx = secp256k1_context_create(SECP256K1_CONTEXT_VERIFY);


It could be confusing for someone reading the code in the future to see these two functions: musig2_prepare_verifier and musig2_verify_musig2, and see that in both we create a new context and destroy it. For sake of clarity, I would comment why we do this, and the design decision for not doing it in a single function.

iquerejeta · 2022-11-16T15:59:50Z

src/libmusig2.c


    musig2_context_free(&verifier_context);
 }
+
+int musig2_verify_musig2(unsigned char *signature, const unsigned char *msg, int msg_len, musig2_aggr_pubkey *aggr_pk){


Suggested change

int musig2_verify_musig2(unsigned char *signature, const unsigned char *msg, int msg_len, musig2_aggr_pubkey *aggr_pk){

int musig2_verify(unsigned char *signature, const unsigned char *msg, int msg_len, musig2_aggr_pubkey *aggr_pk){

iquerejeta · 2022-11-16T18:13:18Z

src/libmusig2.h

+#define MUSIG2_SCALAR_BYTES                 32          // SCALAR BYTES
+#define MUSIG2_PARSIG_BYTES                 32          // PARTIAL SIGNATURE BYTES
+#define MUSIG2_AGGR_PUBKEY_BYTES            32          // XONLY PUBLIC KEY BYTES
+#define MUSIG2_PUBKEY_BYTES_COMPRESSED      33          // COMPRESSED PUBLIC KEY BYTES
+#define MUSIG2_PUBKEY_BYTES_FULL            64          // FULL SIZE PUBLIC KEY BYTES
+#define MUSIG2_BYTES                        64          // SCHNORR SIGNATURE BYTES



I'm not convinced by this naming. But this is just a suggestion. You'll be the main maintainer of the lib, so you can make the final call. The following is my reasoning:

MUSIG2_PARSIG_BYTES is a bit confusing, because we define musig2_partial_signature to be two values with a total of 64 bytes. This variable is really used to determine the size of the 'scalar part' of the partial signature.

I think I would remove the parsig bytes, and simply useSCALAR_BYTES and MUSIG2_BYTES for referring to the size of the signatures.

iquerejeta · 2022-11-16T18:19:35Z

Sorry, for the double comments. I'm using the code editor to review the PR and something clearly went wrong 😅

curiecrypt added 6 commits November 12, 2022 03:41

musig2 wrappers over libsecp

664a59b

valgrind errors resolved

3ab6171

prepare signer to register

693da11

style corrected

43778fb

prep register done in init signer

5a679ef

gtests adapted for wrappers

61d63ad

curiecrypt requested a review from iquerejeta November 14, 2022 15:59

musig2 verify function

2a89a08

iquerejeta reviewed Nov 15, 2022

View reviewed changes

iquerejeta requested changes Nov 15, 2022

View reviewed changes

curiecrypt added 3 commits November 15, 2022 18:27

memory freeing cleared from lib functions

a53649d

serialise_shareable_context seperated from init

d73aa29

gtest adapted, const name updated

cdb6567

curiecrypt requested a review from iquerejeta November 15, 2022 16:25

function descriptions

120afd0

curiecrypt self-assigned this Nov 15, 2022

iquerejeta approved these changes Nov 16, 2022

View reviewed changes

review comments

9ea3e5f

curiecrypt merged commit b1d14f6 into main Nov 16, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

musig2 wrappers over libsecp #31

musig2 wrappers over libsecp #31

curiecrypt commented Nov 12, 2022 •

edited

Loading

iquerejeta Nov 15, 2022

iquerejeta Nov 15, 2022

iquerejeta Nov 15, 2022

iquerejeta Nov 15, 2022

iquerejeta Nov 15, 2022

iquerejeta left a comment

iquerejeta left a comment

iquerejeta Nov 16, 2022

iquerejeta Nov 16, 2022

iquerejeta Nov 16, 2022

iquerejeta Nov 16, 2022

iquerejeta commented Nov 16, 2022

	int musig2_verify_musig2(unsigned char signature, const unsigned char msg, int msg_len, musig2_aggr_pubkey *aggr_pk){
	int musig2_verify(unsigned char signature, const unsigned char msg, int msg_len, musig2_aggr_pubkey *aggr_pk){

musig2 wrappers over libsecp #31

musig2 wrappers over libsecp #31

Conversation

curiecrypt commented Nov 12, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iquerejeta left a comment

Choose a reason for hiding this comment

iquerejeta left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iquerejeta commented Nov 16, 2022

curiecrypt commented Nov 12, 2022 •

edited

Loading