Update secp, build fixes, add M1 and universal2 support

[jgriffiths]

No description provided.

[mdance]

I'm running on:

Darwin 21.4.0 Darwin Kernel Version 21.4.0: Fri Mar 18 00:45:05 PDT 2022; root:xnu-8020.101.4~15/RELEASE_X86_64 x86_64

From my lightning clone, I went into external/libwally-core and checked out this branch, when I attempt a poetry run make, it is now erroring with:

cc hsmd/hsmd.c
wiregen hsmd/hsmd_wiregen.c
cc hsmd/hsmd_wiregen.c
cc hsmd/libhsmd.c
hsmd/libhsmd.c:638:7: error: 'secp256k1_schnorrsig_sign' is deprecated: Use secp256k1_schnorrsig_sign32 instead [-Werror,-Wdeprecated-declarations]
if (!secp256k1_schnorrsig_sign(secp256k1_ctx, sig.u8,
^
external/libwally-core/src/secp256k1/include/secp256k1_schnorrsig.h:136:3: note: 'secp256k1_schnorrsig_sign' has been explicitly marked deprecated here
SECP256K1_DEPRECATED("Use secp256k1_schnorrsig_sign32 instead");
^
external/libwally-core/src/secp256k1/include/secp256k1.h:175:54: note: expanded from macro 'SECP256K1_DEPRECATED'

define SECP256K1_DEPRECATED(_msg) attribute ((deprecated(_msg)))

                                                 ^

hsmd/libhsmd.c:641:18: error: too many arguments to function call, expected 5, have 6
NULL, NULL)) {

[jgriffiths]

Hi @mdance

That is a c-lightning issue with the updated secp that comes with wally, and will need to be corrected there. Once I have merged the build fixes in #321, I plan to make a release at which point c-lightning can update their wally version and move their direct secp calls to the non deprecated interface.

In the meantime if you are building with the above PR, you can simply rename secp256k1_schnorrsig_sign to secp256k1_schnorrsig_sign32 throughout your c-lightning source, and remove the final NULL argument, that should get you going again.

cc: @cdecker

[mdance]

Yeah I've been working my way through the deprecations with the attached patch so far, am running into this now, not quite sure what to do, according to external/libwally-core/src/secp256k1/src/modules/schnorrsig/main_impl.h +219 it looks like its missing the msglen parameter?

int secp256k1_schnorrsig_verify(const secp256k1_context* ctx, const unsigned char *sig64, const unsigned char *msg, size_t msglen, const secp256k1_xonly_pubkey *pubkey) {
    secp256k1_scalar s;
    secp256k1_scalar e;
    secp256k1_gej rj;
    secp256k1_ge pk;
    secp256k1_gej pkj;
    secp256k1_fe rx;
    secp256k1_ge r;
    unsigned char buf[32];
    int overflow;

    VERIFY_CHECK(ctx != NULL);
    ARG_CHECK(sig64 != NULL);
    ARG_CHECK(msg != NULL || msglen == 0);
    ARG_CHECK(pubkey != NULL);

The failing code is:

    /* Now we sanity-check! */
    sighash_from_merkle(messagename, fieldname, merkle, &sighash);
    if (secp256k1_schnorrsig_verify(secp256k1_ctx, sig->u8,
                                    sighash.u.u8, &key->pubkey) != 1)
            fatal("HSM gave bad signature %s for pubkey %s",
                  type_to_string(tmpctx, struct bip340sig, sig),
                  type_to_string(tmpctx, struct point32, key));

cc lightningd/offer.c
lightningd/offer.c:80:32: error: too few arguments to function call, expected 5, have 4
sighash.u.u8, &key->pubkey) != 1)
^
external/libwally-core/src/secp256k1/include/secp256k1_schnorrsig.h:170:48: note: 'secp256k1_schnorrsig_verify' declared here
SECP256K1_API SECP256K1_WARN_UNUSED_RESULT int secp256k1_schnorrsig_verify(
^
1 error generated.
make: *** [lightningd/offer.o] Error 1

deprecations-patch.txt

[mdance]

I think I've managed to get it working lol obviously I am way out of my depth here so I've forked the project, and am going to put up a PR, would someone be able to code review it? Probably will get ditched, but mays well go through the motions hehe

[jgriffiths]

@mdance, I suggest you raise an issue on c-lightning, linking to this PR and issue #320, along with your PR (which you should prefix as WIP:).

"Tracking issue to update wally and libsecp256k1 to next release" or so should do it, so that the changes required aren't lost (and you may get some review, wally doesn't expose the new schnorr sig functions yet, which is why c-lightning is calling them directly).

[jgriffiths]

@mdance, I think you need sizeof(sighash) after sighash.u.u8, to make the above correct.

[mdance]

I have updated https://github.com/ElementsProject/lightning/pull/5172/files with the sizeof change

[jgriffiths]

I have updated https://github.com/ElementsProject/lightning/pull/5172/files with the sizeof change

Hi @mdance you may want to update your PR to use the latest changes from here which include universal2 support.

[rustyrussell]

Waiting for this to merge so I can release rc3....

[mdance]

@jgriffiths @rustyrussell thinks your sizeof suggestion is incorrect at ElementsProject/lightning#5172 (comment) should I revert that commit out?

I will try to compile with the latest changes as well

[jgriffiths]

@jgriffiths @rustyrussell thinks your sizeof suggestion is incorrect at ElementsProject/lightning#5172 (comment) should I revert that commit out?

@mdance my master version of c-lightning has this:

        if (secp256k1_schnorrsig_verify(secp256k1_ctx, sig->u8,
                                        sighash.u.u8, &key->pubkey) != 1)

So for this error:

cc lightningd/offer.c
lightningd/offer.c:80:32: error: too few arguments to function call, expected 5, have 4
sighash.u.u8, &key->pubkey) != 1)

My suggestion was:

        if (secp256k1_schnorrsig_verify(secp256k1_ctx, sig->u8,
                                        sighash.u.u8, sizeof(sighash.u.u8), &key->pubkey) != 1)

sizeof(sighash) is also correct here but as Rusty points out sighash.u.u8 is probably more portable.

[jkauffman1]

utack 59652b5