Add secp256k1_ec_pubkey_negate and secp256k1_ec_privkey_negate
#408
Conversation
|
Concept ACK if you also add a |
|
Sure, I can add a privkey negate. As for the |
65233b0 to
05588d3
Compare
|
Added negate_privkey. That test failure is a randomness test failure, I think it's just a fluke. |
|
change PR title to match new functionality? |
secp256k1_ec_pubkey_negatesecp256k1_ec_pubkey_negate and secp256k1_ec_privkey_negate
|
This is failing a test. |
05588d3 to
4f290a1
Compare
|
kicked travis |
| VERIFY_CHECK(ctx != NULL); | ||
| ARG_CHECK(pubkey != NULL); | ||
|
|
||
| ret = secp256k1_pubkey_load(ctx, &p, pubkey); |
There was a problem hiding this comment.
The pubkey tweak functions zeroize the output on load failure, this code would operate on uninitialized memory.
There was a problem hiding this comment.
for consistency, memset(pubkey, 0, sizeof(*pubkey)); after the load, unless you've got an argument that its bad to clear it completely (in which case we should change it everywhere)
| CHECK(memcmp(&pubkey_tmp, &pubkey, sizeof(pubkey)) != 0); | ||
| CHECK(secp256k1_ec_pubkey_negate(ctx, &pubkey_tmp) == 1); | ||
| CHECK(memcmp(&pubkey_tmp, &pubkey, sizeof(pubkey)) == 0); | ||
|
|
There was a problem hiding this comment.
Test a null argument and test a bad pubkey object.
|
No strong view, but these are multiplicative tweaks with a fixed tweak value of -1. Concept ACK, will ACK with nits fixed. |
|
Fixed nits. |
4f290a1 to
a743364
Compare
|
@apoelstra ping see above: "for consistency, memset(pubkey, 0, sizeof(*pubkey)); after the load, unless you've got an argument that its bad to clear it completely (in which case we should change it everywhere)" |
a743364 to
8e48aa6
Compare
|
Sorry, I forgot to address this. My argument was that the pubkey going into this function is a public key, isn't secret, and there is no need to zeroize it. But this is true even for Added the |
|
in the case of BIP32, a single private key in combination with a master public key exposes all private keys. So, it's possible that a "public" key may indeed be secret. |
exactly. @voisine Nope, unrelated. The issue there is the extended public key. The EC point being described here does not have the properties you're referring to-- it cannot be used to convert extend another private key. |
|
ACK. |
|
utACK |
…y_negate` 8e48aa6 Add `secp256k1_ec_pubkey_negate` and `secp256k1_ec_privkey_negate` (Andrew Poelstra) Tree-SHA512: 28eeca0b04001958ad86b3c802e33a13273514e9e9802d5b358fd577dc95421a2cffb5591716bea10300717f742f0941c465b9df71dbb4c66d174c643887e06f
1ee698f test: refactor: negate signature-s using libsecp256k1 (Sebastian Falbesoner) Pull request description: This small PR gets rid of manual mod-n inversion of the ECDSA signature-s part in unit tests (introduced a long time ago in #5256, triggered by bitcoin-core/secp256k1#69) by using secp256k1 instead. The function wasn't available at that time, but was introduced about three years later, see bitcoin-core/secp256k1#408. Note that as the name suggests, `secp256k1_ec_seckey_negate` is meant to be used for secret keys, but it obviously works in general for scalars modulo the group order. ACKs for top commit: achow101: ACK 1ee698f laanwj: Code review ACK 1ee698f w0xlt: ACK 1ee698f rkrux: tACK 1ee698f Tree-SHA512: dc36ea1572b538d11ae34e1871f310a1cda8083ffb753e93e7ee9d56e91ebd8ec78d35758dfb700254720914b734ef7a071eeef71b6239f19e1e2fb289fb5435
I have an application which uses summed secret keys as a keysplitting mechanism. To use this with BIP32, I would like to negate the public parent key, which would give the following result:
In my case I can change my application to use a difference of keys rather than sum of keys, but this adds another way for the user to get things wrong, especially when used with a more "symmetric" key split where it's unclear which of the two shards ought to be negated.