Fix infinity loop in secp256k1_schnorr_sign #416

secp256k1_schnorr_sign stop the loop only if secp256k1_schnorr_sig_sign return non-zero. But if secret key is zero secp256k1_schnorr_sig_sign will always return zero and secp256k1_schnorr_sign never stops.

utACK. Note this also catches the case when the user passes the field order as secret key (which will also be interpreted as zero). I think this is a reasonably likely footgun to run into, especially because in many cases applications can accept secret keys from untrusted sources and not worry that anything will break.

Also note that secp256k1_ecdsa_sign has the same behaviour as this fix.

@apoelstra I don't sure, but shouldn't we also check that secret key is less than field order?

Updated. I also changed conditions order for nonce, because with overflow first makes check faster (overflow already exists, while for zero we should call function)

ping :)

Needs rebase. Note that I plan to remove the Schnorr code in this repository, as it shouldn't be used for production use in its current form (and both API and signature compatibility will break before it's brough back).

Rebased. @sipa does it means that secp256k1 bindings (like secp256k1-node) should not use Schnorr right now?

Nobody should use Schnorr as implemented now. It is an experiment, and subject to change at any time (as the configure script will warn you about if you try to enable it). The reason I want to remove it from the codebase is because I keep hearing about people who want to use it for production, so this is apparently not clear.

Thank you for explanation.

No longer needed.