Should passing invalid recid to sig parsing call the callback? #751

1. 
   elichai commented at 3:24 pm on May 4, 2020: contributor

   Hi, Currently if you pass a recid that is recid < 0 || recid > 3 into secp256k1_ecdsa_recoverable_signature_parse_compact it will fail on ARG_CHECK which will call secp256k1_callback_call which will by default abort the program. https://github.com/bitcoin-core/secp256k1/blob/master/src/modules/recovery/main_impl.h#L46

   Is this the right thing to do? AFAIK this is the only place in our API where a parsing function can abort your program on invalid input. This means that whoever uses the API must check the recid before passing it to the parsing function.

   I think this makes sense on opaque structs which we require to go through the parsing functions first and not manually initialize them because that will be “library UB”(sometimes abort, others unexpected results, see #668 #701) but I think it makes less sense on an int used as the input to a parsing function.

2. real-or-random added this to the milestone initial release (1.0.0-rc.1) on May 4, 2020
3. 
   real-or-random commented at 5:23 pm on May 4, 2020: contributor

   Yeah, this somehow depends on what “parsing” is. The parse function is supposed to parse the signature (as in “read a byte array”), whereas it assumes that recid is an int already. But that’s weird because the normal usage of recovery is that the recid is transferred along with the signature. You may even consider it part of the signature, and if you look at the caller in Core, that’s pretty much what it’s doing: https://github.com/bitcoin/bitcoin/blob/99813a9745fe10a58bedd7a4cb721faf14f907a4/src/pubkey.cpp#L187

   I’m not sure to be honest. I think the ideal API would define a serialized recoverable signature to be a 65-byte string and simply take care of parsing the first byte. Maybe we can simply add a function that does this but also the keep the current function as it is?

   If people don’t think that’s a good idea, I think you’re right. Then the pragmatic thing is to return 0. (And we could do this change without breaking the API, we’re just defining undefined cases.)

Contributors
elichai real-or-random

Milestone
stable release (1.0.0-rc.1)