Schnorrsig API improvements

real-or-random commented at 11:46 am on March 16, 2022: contributor

Should be merged before #995 if we want this.

I suspect the only change here which is debatable on a conceptual level is the renaming. I can drop this of course.

real-or-random force-pushed on Mar 16, 2022

in examples/schnorr.c:111 in 1fb6606316 outdated

 95@@ -96,7 +96,7 @@ int main(void) {
 96      * improve security against side-channel attacks. Signing with a valid
 97      * context, verified keypair and the default nonce function should never
 98      * fail. */
 99-    return_val = secp256k1_schnorrsig_sign(ctx, signature, msg_hash, &keypair, auxiliary_rand);
100+    return_val = secp256k1_schnorrsig_sign32(ctx, signature, msg_hash, &keypair, auxiliary_rand);

real-or-random commented at 11:54 am on March 16, 2022:

If you want to test SECP256K1_DEPRECATED, simply remove the “32” here again and look out for compiler warnings.

real-or-random force-pushed on Mar 16, 2022

michaelfolkson commented at 12:31 pm on March 16, 2022: member

Concept ACK

I read the discussion on the renaming and it seems the only downsides are that the BIP needs to edited and a deprecation process of schnorrsign_sign is needed. Other projects (e.g. DLCs) seem to want to be able sign variable length messages and we’d like to facilitate other uses of this library if it doesn’t present material downsides. I haven’t tested yet but thanks for the guidance comment.

real-or-random commented at 1:07 pm on March 16, 2022: contributor

only downsides are that the BIP needs to edited

Yeah, in fact, this is not even a downside in the sense that the need to edit is caused by the renaming. There was consensus among the authors and contributors of the BIP already that the BIP should be amended to support varlen messages. It just hasn’t been done so far.

michaelfolkson commented at 3:05 pm on March 16, 2022: member

Built successfully on MacOS Big Sur with --enable-dev-mode flag and ran tests.

If you want to test SECP256K1_DEPRECATED, simply remove the “32” here again and look out for compiler warnings.

Followed this instruction and indeed got the compiler warnings:

 0examples/schnorr.c:111:18: warning: 'secp256k1_schnorrsig_sign' is deprecated: Use secp256k1_schnorrsig_sign32 instead [-Wdeprecated-declarations]
 1    return_val = secp256k1_schnorrsig_sign(ctx, signature, msg_hash, &keypair, auxiliary_rand);
 2                 ^
 3./include/secp256k1_schnorrsig.h:136:3: note: 'secp256k1_schnorrsig_sign' has been explicitly marked deprecated here
 4  SECP256K1_DEPRECATED("Use secp256k1_schnorrsig_sign32 instead");
 5  ^
 6./include/secp256k1.h:179:55: note: expanded from macro 'SECP256K1_DEPRECATED'
 7#   define SECP256K1_DEPRECATED(_msg) __attribute__ ((__deprecated__(_msg)))
 8                                                      ^
 91 warning generated.
10  CCLD     schnorr_example

michaelfolkson commented at 3:33 pm on March 16, 2022: member

ACK 735d8beab0590d177e07be7d1b2f1421b7c62aad

Not familiar with this codebase but code looks fine to me. Mainly just renames.

in include/secp256k1.h:817 in 735d8beab0 outdated

816@@ -800,7 +817,7 @@ SECP256K1_API SECP256K1_WARN_UNUSED_RESULT int secp256k1_ec_pubkey_combine(
817  *  implementations optimized for a specific tag can precompute the SHA256 state
818  *  after hashing the tag hashes.
819  *
820- *  Returns 0 if the arguments are invalid and 1 otherwise.
821+ *  Returns: 1 always.

michaelfolkson commented at 3:37 pm on March 16, 2022:

Wasn’t sure why this has changed. Code below hasn’t changed.

real-or-random commented at 3:53 pm on March 16, 2022:

The function really always returns 1: https://github.com/bitcoin-core/secp256k1/blob/master/src/secp256k1.c#L750

I think this was just a copy-and-paste mistake when writing the API docs. Strictly speaking, it’s not even an API change because there simply are no invalid arguments for this function (except those catched by ARG_CHECK but then we’ll die and the function won’t return.)

jonasnick commented at 9:04 pm on March 16, 2022:

“We” are not guaranteed to die in an ARG_CHECK when custom callbacks are used. Then the return value is unspecified in theory but 0 in practice. This is also how we describe the behavior of other functions (example keypair_sec, keypair_pub).

real-or-random commented at 10:37 am on March 18, 2022:

I fixed also the other functions. See the commit message for detailed rationale.

in include/secp256k1.h:179 in 735d8beab0 outdated

174+#if !defined(SECP256K1_BUILD)
175+# if defined(__GNUC__) && SECP256K1_GNUC_PREREQ(4, 5)
176+#  define SECP256K1_DEPRECATED(_msg) __attribute__ ((__deprecated__(_msg)))
177+# elif defined(__has_attribute)
178+#  if __has_attribute(__deprecated__)
179+#   define SECP256K1_DEPRECATED(_msg) __attribute__ ((__deprecated__(_msg)))

jonasnick commented at 9:05 pm on March 16, 2022:

This line seems to be identical to the one in the previous branch. Did you intend to remove the _msg argument here?

real-or-random commented at 10:04 pm on March 16, 2022:

No this is intentional, I just didn’t find a nicer way to write it…

Let me explain. One can’t write this as a single condition

0(defined(__GNUC__) && SECP256K1_GNUC_PREREQ(4, 5)) || (__has_attribute(__deprecated__) && __has_attribute(__deprecated__)

because this is illegal if __has_attribute is really not defined.

The __has_attribute is actually a clang thing (plus very recent GCC) and the clang docs suggest this here:

0#ifndef __has_attribute
1  #define __has_attribute(x) 0
2#endif

Looks nice but I’m somewhat hesitant to define a reserved __ identifier in a public include file. It’s technically UB and in this case it could really lead to strange issues: Say the user includes our file and then checks defined(__has_attribute) in their code… I don’t see an immediate problem with this, but it seems just wrong to do this.

jonasnick commented at 3:11 pm on March 17, 2022:

Ok, makes sense. Fine to leave as is. By the way, it seems that __has_attribute is supported since GCC 5 (https://gcc.gnu.org/gcc-5/changes.html). So if we’d want to, we could remove the first branch.

real-or-random commented at 9:42 pm on March 17, 2022:

Oh indeed, “very recent GCC” is not true. I switched to your suggestion (also added a missing #else case)

jonasnick commented at 9:07 pm on March 16, 2022: contributor

concept ACK

For the record, I probably wouldn’t rename sign at this point because it’s not obviously better. I agree that it’s more logical in terms of the spec, but for a user looking at the include file it’s not immediately clear what the “32” suffix means and potentially causes confused searches for the “normal” signing function. But I’m ok with the change.

Add SECP256K1_DEPRECATED attribute for marking API parts as deprecated 3db0560606

Use SECP256K1_DEPRECATED for existing deprecated API functions fc94a2da44

schnorrsig: Rename schnorrsig_sign to schnorsig_sign32 and deprecate 99e6568fc6

schnorrsig: Adapt example to new API f813bb0df3

real-or-random force-pushed on Mar 17, 2022

real-or-random force-pushed on Mar 18, 2022

docs: Fix return value for functions that don't have invalid inputs

_tagged_sha256 simply cannot have invalid inputs.

The other functions could in some sense have invalid inputs but only in
violation of the type system. For example, a pubkey could be invalid but
invalid objects of type secp256k1_pubkey either can't be obtained
via the API or will be caught by an ARG_CHECK when calling pubkey_load.

This is consistent with similar functions in the public API, e.g.,
_ec_pubkey_negate or _ec_pubkey_serialize.

b8f8b99f0f

real-or-random force-pushed on Mar 18, 2022

jonasnick commented at 1:43 pm on March 18, 2022: contributor

ACK b8f8b99f0fb3a5cd4c6fb1c9c8dfed881839e19e

in include/secp256k1.h:173 in 3db0560606 outdated

168@@ -169,6 +169,17 @@ typedef int (*secp256k1_nonce_function)(
169 #  define SECP256K1_ARG_NONNULL(_x)
170 # endif
171 
172+/** Attribute for marking functions, types, and variables as deprecated */
173+#if !defined(SECP256K1_BUILD) && defined(__has_attribute)

sipa commented at 12:13 pm on March 19, 2022:

IIRC GCC doesn’t have __has_attribute until very recent versions, but does have __attribute__((__deprecated__)) since at least GCC 4.1 (tested with godbolt) and probably earlier. Do we perhaps want this additionally enabled on based on __GNUC__?

real-or-random commented at 3:50 pm on March 19, 2022:

This is what I had earlier but @jonasnick pointed out that already GCC >= 5 offers __has__attribute. Moreover, __attribute__((__deprecated__)) indeed works in >= GCC 4.1 but the optional msg argument works only in GCC >= 4.5 (in fact you get an error if you pass an argument in GCC < 4.5).

(See the resolved discussion above).

sipa commented at 3:52 pm on March 19, 2022:

Oh wow, I thought __has_attribute was only added much later. Looks good then.

sipa commented at 9:51 pm on March 24, 2022: contributor

utACK b8f8b99f0fb3a5cd4c6fb1c9c8dfed881839e19e

real-or-random merged this on Mar 24, 2022

real-or-random closed this on Mar 24, 2022

fanquake referenced this in commit 465d05253a on Mar 30, 2022

fanquake referenced this in commit db00557dcb on Mar 30, 2022

jonasnick cross-referenced this on Mar 30, 2022 from issue Upstream PRs 1064, 1049, 899, 1068, 1072, 1069, 1074, 1026, 1033, 748, 1079, 1088, 1090, 731, 1089, 995, 1094, 1093 by jonasnick

real-or-random referenced this in commit 6c0aecf72b on Apr 1, 2022

fanquake referenced this in commit afb7a6fe06 on Apr 6, 2022

fanquake referenced this in commit 0936101ab4 on Apr 6, 2022

fanquake referenced this in commit 404c53062b on Apr 7, 2022

laanwj cross-referenced this on Apr 8, 2022 from issue Update libsecp256k1 subtree to current master by fanquake

russeree referenced this in commit 5319a2bbaa on Apr 12, 2022

russeree referenced this in commit d14742e4c9 on Apr 12, 2022

russeree referenced this in commit 623d4ff6b2 on Apr 12, 2022

russeree referenced this in commit db0875d0a7 on Apr 12, 2022

russeree referenced this in commit 5842e2351d on Apr 15, 2022

russeree referenced this in commit ac83524b20 on Apr 15, 2022

gwillen referenced this in commit 35d6112a72 on May 25, 2022

patricklodder referenced this in commit 21badcf9d2 on Jul 25, 2022

patricklodder referenced this in commit 03002a9013 on Jul 28, 2022

janus referenced this in commit 3a0652a777 on Aug 4, 2022

janus referenced this in commit 3cc3715b52 on Aug 4, 2022

str4d referenced this in commit 522190d5c3 on Apr 21, 2023

vmta referenced this in commit e1120c94a1 on Jun 4, 2023

theStack referenced this in commit f6867857fb on Jun 11, 2023

theStack referenced this in commit f3644287b1 on Jun 11, 2023

theStack cross-referenced this on Jun 11, 2023 from issue docs: correct `pubkey` param descriptions for `secp256k1_keypair_{xonly_,}pub` by theStack

dderjoel referenced this in commit 15645af9dd on Jun 21, 2023

vmta referenced this in commit 8f03457eed on Jul 1, 2023

Schnorrsig API improvements #1089