Make the public API docs more consistent and explicit

elichai commented at 9:53 am on July 30, 2020: contributor

I went over the public API and added missing explanations on when a pointer can be null and when it cannot, and added some missing checks for null ctx and null pubkey pointers.

Open questions IMHO:

Can secp256k1_context_create return NULL? right now it could return null if you replaced the callbacks at compile time to ones that do return(unlike the default ones which never return).
Related to the first, should we document that the callbacks should never return? (in the tests we use returning callbacks but we can violate our own API) right now we say the following:

After this callback returns, anything may happen, including crashing.

Is this enough to document answer no for the first question and just saying that if the callback returned then you violated the API so secp256k1_context_create can return NULL even though it is promised not to? Right now we AFAICT we never check if it returns null

Another nit I’m not sure about is wording (does nothing if NULL)/(ignored if NULL)/(can be NULL)

More missing docs:

Documenting the data argument to the default nonce functions

in include/secp256k1.h:232 in 298b850c13 outdated

228@@ -229,7 +229,7 @@ SECP256K1_API secp256k1_context* secp256k1_context_clone(
229  *  be used instead.
230  *
231  *  Args:   ctx: an existing context to destroy, constructed using
232- *               secp256k1_context_create or secp256k1_context_clone
233+ *               secp256k1_context_create or secp256k1_context_clone (does nothing if NULL)

real-or-random commented at 10:48 am on July 30, 2020:

I don’t think that we want to guarantee this. As I understand it, we don’t define any behavior if you pass NULL, so it’s UB (we’re just nice currently and do nothing :))

in include/secp256k1.h:271 in 298b850c13 outdated

267@@ -268,7 +268,7 @@ SECP256K1_API void secp256k1_context_destroy(
268  *  In:   fun:  a pointer to a function to call when an illegal argument is
269  *              passed to the API, taking a message and an opaque pointer.
270  *              (NULL restores the default handler.)
271- *        data: the opaque pointer to pass to fun above.
272+ *        data: the opaque pointer to pass to fun above (can be NULL for default handler)

real-or-random commented at 10:50 am on July 30, 2020:

I think we could say “must be NULL” for default handler because the default handler does not expect data. (Again, nothing bad happens if you don’t obey.)

in include/secp256k1.h:321 in 298b850c13 outdated

317@@ -318,8 +318,8 @@ SECP256K1_API SECP256K1_WARN_UNUSED_RESULT secp256k1_scratch_space* secp256k1_sc
318 /** Destroy a secp256k1 scratch space.
319  *
320  *  The pointer may not be used afterwards.
321- *  Args:       ctx: a secp256k1 context object.
322- *          scratch: space to destroy
323+ *  Args:       ctx: a secp256k1 context object (cannot be NULL)

real-or-random commented at 10:51 am on July 30, 2020:

Well NULL is not a secp256k1, not sure if we need to say this everywhere.

But apparently we say this in some places, so we could make it consistent. But personally I’d prefer to omit it consistently.

elichai commented at 10:58 am on July 30, 2020:

yeah I started by omitting it when it’s obvious that it cannot be null, but then other places didn’t omit so I settled on being explicit everywhere

in include/secp256k1.h:322 in 298b850c13 outdated

317@@ -318,8 +318,8 @@ SECP256K1_API SECP256K1_WARN_UNUSED_RESULT secp256k1_scratch_space* secp256k1_sc
318 /** Destroy a secp256k1 scratch space.
319  *
320  *  The pointer may not be used afterwards.
321- *  Args:       ctx: a secp256k1 context object.
322- *          scratch: space to destroy
323+ *  Args:       ctx: a secp256k1 context object (cannot be NULL)
324+ *          scratch: space to destroy (does nothing if NULL)

real-or-random commented at 10:52 am on July 30, 2020:

same as with the context, not sure why we would want to guarantee this.

real-or-random cross-referenced this on Sep 11, 2020 from issue API/docs: Return value if ARG_CHECK fires by real-or-random

tnawanna approved

sipa commented at 8:56 pm on September 18, 2020: contributor

I think this is overkill, and in some cases makes the public description of the API commit to promises that we may not want to keep long term.

My preference would be to state once that unless explicitly indicated, pointer arguments cannot be NULL. What should be documented is whether or not an argument is intentionally optional (as in: it still does something useful if the specified argument is NULL). Optional arguments that are missing are passed as NULL. Others are passed as non-NULL pointers to objects of the specified type.

real-or-random cross-referenced this on Oct 28, 2020 from issue Return NULL early in context_preallocated_create if flags invalid by real-or-random

jonasnick commented at 10:50 pm on November 4, 2020: contributor

Agree with @sipa, just documenting which arguments are optional is better. As far as wording goes, it’d make sense to state what happens if the argument is NULL. If applicable, something like Ignored if NULL (without parantheses preferably).

Can secp256k1_context_create return NULL? Related to the first, should we document that the callbacks should never return?

Even if we mention that illegal callbacks should never return, context_create can still return NULL. That happens for example if malloc fails and the error callback is called. This seems to be already mentioned in the docs:

When this [error] callback is triggered, the API function called is guaranteed not to cause a crash, though its return value and output arguments are undefined.

elichai force-pushed on Jan 23, 2021

in include/secp256k1.h:429 in e34258e166 outdated

401- *  In:   input:    a pointer to the signature to be parsed
402- *        inputlen: the length of the array pointed to be input
403+ *  Args: ctx:      a secp256k1 context object.
404+ *  Out:  sig:      a pointer to a signature object.
405+ *  In:   input:    a pointer to the signature to be parsed.
406+ *        inputlen: the length of the array pointed to be input.

jonasnick commented at 4:17 pm on January 25, 2021:

Adding periods for every arguments seems a bit unnecessary. So far I’ve tried to follow the rule to add a period only if it follows an actual sentence (obj + verb).

elichai commented at 7:23 pm on January 25, 2021:

I mostly wanted consistency and thought that will be the less controversial way, but I’ll remove that change

elichai commented at 9:52 am on July 4, 2021:

I removed all the changes that simply added periods

in include/secp256k1_extrakeys.h:76 in e34258e166 outdated

77- *          pk_parity: pointer to an integer that will be set to 1 if the point
78- *                     encoded by xonly_pubkey is the negation of the pubkey and
79- *                     set to 0 otherwise. (can be NULL)
80- *  In:        pubkey: pointer to a public key that is converted (cannot be NULL)
81+ *                     converted public key.
82+ *          pk_parity: Optional(if NULL): pointer to an integer that

jonasnick commented at 4:21 pm on January 25, 2021:

How about Ignored if NULL. Otherwise, pointer ....

elichai commented at 9:51 am on July 4, 2021:

That’s better, Thanks!

in include/secp256k1_schnorrsig.h:79 in e34258e166 outdated

81  *         ndata: pointer to arbitrary data used by the nonce generation
82- *                function (can be NULL). If it is non-NULL and
83- *                secp256k1_nonce_function_bip340 is used, then ndata must be a
84- *                pointer to 32-byte auxiliary randomness as per BIP-340.
85+ *                function for secp256k1_nonce_function_bip340 it can either be NULL or
86+ *                non-NULL and then ndata must be a pointer to 32-byte auxiliary randomness as per BIP-340.

jonasnick commented at 4:26 pm on January 25, 2021:

How about

pointer to arbitrary data used by the nonce generation function. secp256k1_nonce_function_bip340 ignores ndata if it is NULL, otherwise it must be a pointer to 32-byte auxiliary randomness as per BIP-340.

Also I think we should try to keep the line width <= 100 chars.

elichai commented at 9:51 am on July 4, 2021:

That’s better, Thanks!

jonasnick commented at 4:27 pm on January 25, 2021: contributor

Really good to get rid of the “(cannot be NULL)”. There’s one more of those in secp256k1_ecdsa_signature_normalize (see sigin argument) and more in contrib/lax_der_privatekey_parsing.h.

jonasnick cross-referenced this on Feb 5, 2021 from issue Add ECDSA adaptor signatures module by jesseposner

real-or-random commented at 10:38 am on February 19, 2021: contributor

Maybe it’s will be a good idea to write the requirements on the context flags in more structured way. At the moment we say for example “initialized for verification”.

I believe it makes things easier for the user if we’d say “created with flag SECP256K1_CONTEXT_SIGN” or similar, and if this somehow stands out such that it can be parsed easily (be the human eye).

jonasnick cross-referenced this on Apr 28, 2021 from issue add `secp256k1_ec_pubkey_cmp` method by apoelstra

jonasnick cross-referenced this on Apr 28, 2021 from issue secp256k1.h: clarify that by default arguments must be != NULL by jonasnick

real-or-random referenced this in commit 69394879b6 on May 7, 2021

jonasnick commented at 9:35 pm on June 23, 2021: contributor

Needs rebase

jonasnick cross-referenced this on Jun 23, 2021 from issue schnorrsig API overhaul by jonasnick

in include/secp256k1.h:755 in 55ad72a784 outdated

755- *                      (cannot be NULL)
756- *  In:     ins:        pointer to array of pointers to public keys (cannot be NULL)
757- *          n:          the number of public keys to add together (must be at least 1)
758+ *  Args:   ctx:        pointer to a context object.
759+ *  Out:    out:        pointer to a public key object for placing the resulting public key.
760+ *                     .

ariard commented at 10:10 pm on June 24, 2021:

nit: i believe this dot is redundant

in include/secp256k1.h:248 in 55ad72a784 outdated

230@@ -229,7 +231,7 @@ SECP256K1_API secp256k1_context* secp256k1_context_clone(
231  *  be used instead.
232  *
233  *  Args:   ctx: an existing context to destroy, constructed using
234- *               secp256k1_context_create or secp256k1_context_clone
235+ *               secp256k1_context_create or secp256k1_context_clone.
236  */
237 SECP256K1_API void secp256k1_context_destroy(

ariard commented at 10:22 pm on June 24, 2021:

Does secp256k1_context_destroy’s ctx deserves its own SECP256K1_ARG_NONNULL annotation?

edit: Have a look on secp256k1_ecdsa_signature_normalize’s second arg, missing too? re-edit: secp256k1_context_randomize’s second arg?

elichai commented at 8:56 am on July 4, 2021:

Good question if it shouldn’t be null, right now it will work if it’s not but we don’t promise that so I think it should have.

As for the rest: secp256k1_ecdsa_signature_normalize 2nd argument can be NULL if you just want to check for normalization without normalizing. secp256k1_context_randomize you can pass NULL as the 2nd argument to reset to the initial state.

jonasnick commented at 9:26 pm on July 5, 2021:

I don’t think it’s worth changing the API of context_destroy and preallocated_destroy given that it is fine to give NULL pointers to free().

ariard commented at 12:02 pm on July 6, 2021:

Good point, though i don’t know how “if ptr NULL, no operations is performed” guarantee holds beyond standardized *nix platforms.

jonasnick commented at 12:43 pm on July 6, 2021:

Probably best to mention this behavior in the doc (“Ignored if NULL” or something).

elichai commented at 1:31 pm on July 6, 2021:

@jonasnick See @real-or-random’s comment at: #783 (review)

jonasnick commented at 1:59 pm on July 6, 2021:

If we don’t mention this, we will have the exact same discussion again at some point. But fine either way imo.

jonasnick commented at 2:01 pm on July 6, 2021:

@ariard that behavior is part of the C standard (http://port70.net/~nsz/c/c89/c89-draft.html#4.10.3.2)

elichai commented at 4:19 pm on July 6, 2021:

Yeah, I wanted to make that promise too, but not too important (as long as it’s explicitly either NON_NULL or allowed to be null, I prefer not to leave it in a limbo)

ariard commented at 0:30 am on July 7, 2021:

@ariard that behavior is part of the C standard (http://port70.net/~nsz/c/c89/c89-draft.html#4.10.3.2)

Ah right, so “allowed to be null”/“Ignored if NULL” recalling C standard?

real-or-random commented at 9:44 am on July 7, 2021:

@ariard Not sure if we’re talking past each other. Sure the current free(NULL) implementation is fine. I was just pointing out that adding the promise that secp256k1_context_destroy(NULL) is a no-op will be an API change and I don’t see a reason to make this change.

jonasnick commented at 6:51 pm on July 8, 2021:

I still think it would be better to remove the SECP256K1_ARG_NONNULL because it is a backwards-incompatible API change.

jonasnick commented at 1:30 pm on August 9, 2021:

Just to clarify, it’s an API change since before 0881633d we didn’t force that pointer args must not be NULL if not mentioned otherwise. But here we never mentioned “cannot be NULL” or similar.

jonasnick commented at 12:45 pm on September 15, 2021:

@real-or-random pointed out that while there wasn’t a “cannot be NULL”, an existing context was required. So I think it’s ok as is now.

in include/secp256k1_preallocated.h:120 in 55ad72a784 outdated

114@@ -115,7 +115,7 @@ SECP256K1_API secp256k1_context* secp256k1_context_preallocated_clone(
115  *
116  *  Args:   ctx: an existing context to destroy, constructed using
117  *               secp256k1_context_preallocated_create or
118- *               secp256k1_context_preallocated_clone (cannot be NULL)
119+ *               secp256k1_context_preallocated_clone.
120  */
121 SECP256K1_API void secp256k1_context_preallocated_destroy(

ariard commented at 10:37 pm on June 24, 2021:

Same, missing SECP256K1_ARG_NONNULL on secp256k1_context_preallocated_destroy’s first arg ?

elichai commented at 9:50 am on July 4, 2021:

Added, Thanks!

in include/secp256k1_recovery.h:65 in 55ad72a784 outdated

64+ *  Out:  output64: a pointer to a 64-byte array of the compact signature.
65+ *        recid:    Optional(if NULL): a pointer to an integer to hold the recovery id.
66+ *  In:   sig:      a pointer to an initialized signature object.
67  */
68 SECP256K1_API int secp256k1_ecdsa_recoverable_signature_serialize_compact(
69     const secp256k1_context* ctx,

ariard commented at 10:38 pm on June 24, 2021:

Well maybe here you need to drop the annotation for third arg?

elichai commented at 8:57 am on July 4, 2021:

Good find! Thanks

elichai commented at 9:50 am on July 4, 2021:

I was wrong for writing recid is optional, so I removed that change. Thanks!

ariard commented at 10:42 pm on June 24, 2021: none

Otherwise good to me, let me know if I’m completely astray w.r.t to SECP256K1_ARG_NONNULL

real-or-random cross-referenced this on Jul 2, 2021 from issue tests_exhaustive: check the result of secp256k1_ecdsa_sign by niooss-ledger

real-or-random cross-referenced this on Jul 3, 2021 from issue Policy for SECP256K1_WARN_UNUSED_RESULT by real-or-random

elichai force-pushed on Jul 4, 2021

Improve consistency for NULL arguments in the public interface f4edfc7581

Add missing null check for ctx and input keys in the public API adec5a1638

elichai force-pushed on Jul 4, 2021

elichai commented at 9:52 am on July 4, 2021: contributor

Rebased and fixed comments

jonasnick commented at 9:27 pm on July 5, 2021: contributor

ACK mod nit

ariard commented at 12:03 pm on July 6, 2021: none

ACK adec5a16

Sorry for my annotation call around context_destroy/preallocated_destroy if you think it’s redundant, feel free to drop it.

jonasnick commented at 12:45 pm on September 15, 2021: contributor

ACK adec5a16383f1704d80d7c767b2a65d9221cee08

real-or-random commented at 2:23 pm on September 15, 2021: contributor

@real-or-random pointed out that while there wasn’t a “cannot be NULL”, an existing context was required. So I think it’s ok as is now.

I agree.

real-or-random merged this on Sep 15, 2021

real-or-random closed this on Sep 15, 2021

elichai deleted the branch on Sep 15, 2021

jonasnick cross-referenced this on Sep 15, 2021 from issue Upstream PRs 969, 956, 783, 976 by jonasnick

fanquake referenced this in commit ff458a7b78 on Sep 29, 2021

real-or-random referenced this in commit 7812feb896 on Oct 15, 2021

fanquake referenced this in commit 8f5cd5e893 on Oct 20, 2021

sipa referenced this in commit f727914d7e on Oct 28, 2021

sipa referenced this in commit 440f7ec80e on Oct 31, 2021

dhruv referenced this in commit 395e1155b9 on Nov 2, 2021

dhruv referenced this in commit 184e1fac17 on Nov 2, 2021

sipa referenced this in commit d057eae556 on Dec 2, 2021

fanquake referenced this in commit c4a1e09a8c on Dec 3, 2021

sipa referenced this in commit 86dbc4d075 on Dec 15, 2021

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

janus referenced this in commit 879a9a27b9 on Jul 10, 2022

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

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

backpacker69 referenced this in commit 77186f4a04 on Jan 18, 2023

str4d referenced this in commit 6de4698bf9 on Apr 21, 2023

vmta referenced this in commit e1120c94a1 on Jun 4, 2023

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

Make the public API docs more consistent and explicit #783