Rename msg32 to msghash32 in ecdsa_sign/verify and add explanation #854

pull jonasnick wants to merge 2 commits into bitcoin-core:master from jonasnick:msghash changing 4 files +70 −63

jonasnick commented at 3:56 PM on December 3, 2020: contributor

This fixes #307 if there's nothing else that's confusing.

in include/secp256k1.h:461 in cec7cfd9b1 outdated

 457 | + *                      must make sure to apply a cryptographic hash function to
 458 | + *                      the message by itself and not accept an msghash32 value
 459 | + *                      directly. Otherwise, it would be easy to create a
 460 | + *                      "valid" signature without knowledge of the secret key.
 461 | + *                      See also https://bitcoin.stackexchange.com/a/81116/35586
 462 | + *                      for more background on this topic. (cannot be NULL)

real-or-random commented at 8:53 PM on December 3, 2020:

maybe move the "cannot be NULL" up again.

Otherwise it's "See also https://bitcoin.stackexchange.com/a/81116/35586 for more background on this topic. (cannot be NULL)", which confused me. We could also move the explanation to the body alternatively.

jonasnick commented at 2:17 PM on December 4, 2020:

Fixed

real-or-random commented at 8:58 PM on December 3, 2020: contributor

We should do the same for the functions in the recovery module.

One more thing that I noticed is that extrakeys module says "tweak32" whereas the other functions have just "tweak". I think "tweak32" is better. The number suffixes are really neat.

I think the only exception where we don't use them is "seckey". (Maybe because this thing is so common that people should not get it wrong.)
Rename msg32 to msghash32 in ecdsa_sign/verify and add explanation f587f04e35
Rename tweak to tweak32 in public API 6e85d675aa
jonasnick force-pushed on Dec 4, 2020
jonasnick commented at 2:18 PM on December 4, 2020: contributor

Good points. Renamed msg32 also in recovery module and added commit that renames tweak to tweak32.
real-or-random commented at 4:38 PM on December 4, 2020: contributor

ACK 6e85d675aaf9dc17842096f9cbf8cfab216c9331 I inspected the diff
apoelstra approved
apoelstra commented at 12:34 AM on December 5, 2020: contributor

ack 6e85d675aaf9dc17842096f9cbf8cfab216c9331
gmaxwell commented at 7:50 PM on December 7, 2020: contributor

looks fine to me.
real-or-random merged this on Dec 7, 2020
real-or-random closed this on Dec 7, 2020
Fabcien referenced this in commit b9aaa07a18 on Apr 8, 2021
deadalnix referenced this in commit 156ca47cad on Apr 9, 2021

Contributors

Linked (view graph)

#307 Check argument names for confusability.