doc: clarify API doc of secp256k1_ecdsa_recover return value

[theStack]

This PR aims to fix #1718. I thought it's nice to provide an usage example, but not sure if we want actual code snippets in the API header (it's also incomplete, as the declarations are missing), so an alternative might be to drop it, or add a "(see example)" reference in the future in case #1714 gets in.

[jonasnick]

I think it'd be better if the docs pointed out that this is not typical usage. In typical usage you only care whether the signature is valid for the recovered public key. So you just need run ecdsa_recover and do not need to call ecdsa_verify.

Maybe there are cases where this is necessary (e.g., you have to hand over the signature and public key to a verify that doesn't support ecdsa_recover, like the Bitcoin consensus protocol).

[theStack]

Makes sense. My assumption when writing this was that most users would very likely want to enforce lower-S in order to avoid malleability, but that was more based on gut feeling than actual research (seems BOLT11 now explicitly allows low- and high-S signatures if pubkey recovery is used: lightning/bolts#1284).

[jonasnick]

"correct ECDSA signature in a mathematical sense" sounds a bit imprecise. How about the following wording, which avoids that term entirely?

Successful public key recovery guarantees that the signature, when normalized,
passes `secp256k1_ecdsa_verify`. Thus, explicit verification is not necessary.

A recoverable signature converted to a normal signature may not pass
`secp256k1_ecdsa_verify` if it is not normalized. If a normalized signature is
required, call `secp256k1_ecdsa_signature_normalize` after
`secp256k1_ecdsa_recover`.

[theStack]

Took that suggestion, and set you as commit author accordingly. As tiny-nit, it's slightly unfortunate that "normal" and "normalized" sound so similar, but not sure how to avoid it (maybe it's not a big deal).

[real-or-random]

Took that suggestion, and set you as commit author accordingly. As tiny-nit, it's slightly unfortunate that "normal" and "normalized" sound so similar, but not sure how to avoid it (maybe it's not a big deal).

Stumbled upon this, too. One fix is s/normal/non-recoverable. Another one is to spell out the types explicitly:

Here's an improved version. This uses a middle way. It says "non-recoverable" but mentions secp256k1_ecdsa_recoverable_signature_convert so the reader can look up the exact types.

Successful public key recovery guarantees that the signature, after normalization,
passes `secp256k1_ecdsa_verify`. Thus, explicit verification is not necessary.

However, a non-recoverable signature converted from a recoverable signature
(using `secp256k1_ecdsa_recoverable_signature_convert`) is not guaranteed
to be normalized and thus not guaranteed to pass `secp256k1_ecdsa_verify`
(even if it passes `secp256k1_ecdsa_recover`). If a normalized signature is
required, call `secp256k1_ecdsa_signature_normalize` after `secp256k1_ecdsa_recoverable_signature_convert`.

[theStack]

SGTM, taken.

[jonasnick]

"a non-recoverable signature [...] (even if it passes secp256k1_ecdsa_recover)" may be a bit confusing because a non-recoverable signature cannot pass secp256k1_ecdsa_recover.

How about this:

However, a recoverable signature that successfully passes `secp256k1_ecdsa_recover`,
when converted to a non-recoverable signature (using
`secp256k1_ecdsa_recoverable_signature_convert`), is not guaranteed to be
normalized and thus not guaranteed to pass `secp256k1_ecdsa_verify`.

[real-or-random]

"a non-recoverable signature [...] (even if it passes secp256k1_ecdsa_recover)" may be a bit confusing because a non-recoverable signature cannot pass secp256k1_ecdsa_recover.

Good point.

[theStack]

Updated again with the suggestion.

[real-or-random]

utACK 131f871

[jonasnick]

ACK 7321bdf

Thanks @theStack