Add `secp256k1_ec_pubkey_negate` and `secp256k1_ec_privkey_negate` #408

pull apoelstra wants to merge 1 commits into bitcoin-core:master from apoelstra:negate-pubkey changing 3 files +72 −3

apoelstra commented at 4:12 PM on July 28, 2016: contributor
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:
- one of the key shards is the negative parent key, the other the child key
- the "summed key" is then the BIP32 tweak
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.
sipa commented at 6:39 PM on August 24, 2016: contributor

Concept ACK if you also add a secp256k1_ec_privkey_negate. Also, what about using the secp256k1_ec_*_tweak_negate name, for consistency with _tweak_add and _tweak_mul ?
apoelstra commented at 10:29 PM on August 24, 2016: contributor

Sure, I can add a privkey negate.

As for the tweak_ names, I had thought that the "tweak" was the extra parameter, not the original element -- you are multiplying by a tweak, or adding a tweak.
apoelstra force-pushed on Aug 26, 2016
apoelstra commented at 6:35 PM on August 26, 2016: contributor

Added negate_privkey.

That test failure is a randomness test failure, I think it's just a fluke.
instagibbs commented at 7:51 PM on August 26, 2016: none

change PR title to match new functionality?
apoelstra renamed this:
~~Add `secp256k1_ec_pubkey_negate`~~
Add `secp256k1_ec_pubkey_negate` and `secp256k1_ec_privkey_negate`
on Aug 26, 2016
instagibbs commented at 3:43 PM on November 2, 2016: none

This is failing a test.
apoelstra force-pushed on Nov 2, 2016
apoelstra commented at 3:46 PM on November 2, 2016: contributor

kicked travis
in src/secp256k1.c:None in 4f290a193c outdated
```
 440 | +    int ret = 0;
 441 | +    secp256k1_ge p;
 442 | +    VERIFY_CHECK(ctx != NULL);
 443 | +    ARG_CHECK(pubkey != NULL);
 444 | +
 445 | +    ret = secp256k1_pubkey_load(ctx, &p, pubkey);
```
gmaxwell commented at 10:14 PM on December 16, 2016:

The pubkey tweak functions zeroize the output on load failure, this code would operate on uninitialized memory.

gmaxwell commented at 3:09 AM on December 17, 2016:

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)

in src/tests.c:None in 4f290a193c outdated

3463 | +    memcpy(&pubkey_tmp, &pubkey, sizeof(pubkey));
3464 | +    CHECK(secp256k1_ec_pubkey_negate(ctx, &pubkey_tmp) == 1);
3465 | +    CHECK(memcmp(&pubkey_tmp, &pubkey, sizeof(pubkey)) != 0);
3466 | +    CHECK(secp256k1_ec_pubkey_negate(ctx, &pubkey_tmp) == 1);
3467 | +    CHECK(memcmp(&pubkey_tmp, &pubkey, sizeof(pubkey)) == 0);
3468 | +

gmaxwell commented at 10:15 PM on December 16, 2016:

Test a null argument and test a bad pubkey object.

gmaxwell commented at 10:16 PM on December 16, 2016: contributor

No strong view, but these are multiplicative tweaks with a fixed tweak value of -1.

Concept ACK, will ACK with nits fixed.
apoelstra commented at 10:43 PM on December 16, 2016: contributor

Fixed nits.
apoelstra force-pushed on Dec 16, 2016
gmaxwell commented at 12:35 AM on December 20, 2016: contributor

@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)"
Add `secp256k1_ec_pubkey_negate` and `secp256k1_ec_privkey_negate` 8e48aa60dc
apoelstra force-pushed on Dec 20, 2016
apoelstra commented at 12:41 AM on December 20, 2016: contributor

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 pubkey_tweak_add, so I guess the purpose of the zeroing is to turn any invalid pubkey into a "standard invalid pubkey", which seems like a sane functional behaviour.

Added the memset.
voisine commented at 12:46 AM on December 20, 2016: contributor

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.
gmaxwell commented at 12:49 AM on December 20, 2016: contributor

so I guess the purpose of the zeroing is to turn any invalid pubkey into a "standard invalid pubkey",

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.
gmaxwell commented at 12:51 AM on December 20, 2016: contributor

ACK.
dcousens cross-referenced this on Mar 21, 2017 from issue secp256k1_ec_privkey_tweak_negate by dcousens
sipa commented at 12:54 AM on March 22, 2017: contributor

utACK
sipa merged this on Mar 22, 2017
sipa closed this on Mar 22, 2017
sipa referenced this in commit 119949232a on Mar 22, 2017
apoelstra deleted the branch on Jun 19, 2017
elichai cross-referenced this on Mar 22, 2020 from issue Implement go bindings to secp256k1 by elichai

Contributors

Linked (view graph)

#450 secp256k1_ec_privkey_tweak_negate