[ECDH API change] Support custom hash function

fanatid commented at 8:52 am on November 12, 2015: contributor

Solve #352

fanatid cross-referenced this on Jan 13, 2016 from issue RFC: non-hashed ecdh by axic

fanatid cross-referenced this on Feb 1, 2016 from issue 3.0.0 by wanderer

axic cross-referenced this on Apr 22, 2016 from issue Support custom hash functions in ECDH API by Kagami

fanatid commented at 10:15 am on April 27, 2016: contributor

I think that unsafeECDH that return public key as result will be better than ecdh with custom hash function, so I close this PR.

fanatid closed this on Apr 27, 2016

fanatid deleted the branch on Jan 7, 2017

fanatid cross-referenced this on Mar 10, 2017 from issue ECDH: output compressed point by chfast

fanatid restored the branch on Mar 12, 2017

fanatid reopened this on Mar 12, 2017

fanatid force-pushed on Mar 12, 2017

fanatid commented at 6:47 am on March 12, 2017: contributor

Rebased. @sipa now you have 2 PRs, hash function as argument (this) and raw function (#446).. please select one of them, review and merge :) Thank you!

fanatid force-pushed on Mar 12, 2017

chfast commented at 5:52 pm on March 21, 2017: none

Yes please :) For me it does not matter which variant it will be, but I need access to raw point coordinates (precisely only x).

Actually, this variant is a bit better than mine, because the serialized point is not useful for me directly.

in src/modules/ecdh/tests_impl.h:13 in 750782fef3 outdated

 6@@ -7,6 +7,22 @@
 7 #ifndef _SECP256K1_MODULE_ECDH_TESTS_
 8 #define _SECP256K1_MODULE_ECDH_TESTS_
 9 
10+int ecdh_hash_function_test_fail(unsigned char *output, const unsigned char *x, const unsigned char *y) {
11+    if (1) {
12+        return 0;
13+    }

dcousens commented at 10:41 pm on March 21, 2017:

is this meant to be here?

fanatid commented at 1:58 pm on March 27, 2017:

removed

in include/secp256k1_ecdh.h:10 in 750782fef3 outdated

 6@@ -7,21 +7,41 @@
 7 extern "C" {
 8 # endif
 9 
10+/** A pointer to a function that apply hash function to a point

sipa commented at 11:56 pm on March 21, 2017:

applies

fanatid commented at 1:58 pm on March 27, 2017:

fixed

sipa commented at 11:57 pm on March 21, 2017: contributor

Concept ACK

fanatid force-pushed on Mar 27, 2017

chfast commented at 5:34 pm on August 8, 2017: none

How is this going on?

chfast commented at 4:57 pm on April 5, 2018: none

Ping.

apoelstra commented at 4:59 pm on April 5, 2018: contributor

I’ll rebase and review it.

apoelstra assigned apoelstra on Apr 5, 2018

in include/secp256k1_ecdh.h:18 in f35568b58b outdated

12+ *  Returns: 1 if a point was successfully hashed. 0 will cause ecdh to fail
13+ *  Out:    output:     pointer to an array to be filled by the function
14+ *  In:     x:          pointer to a 32-byte x coordinate
15+ *          y:          pointer to a 32-byte y coordinate
16+ */
17+typedef int (*secp256k1_ecdh_hash_function)(

chfast commented at 9:07 pm on April 5, 2018:

I’m not sure it’s not a bit too much to return bool from hash function. Not very practical to have a hash function that can fail.

apoelstra commented at 1:42 pm on April 6, 2018:

I think it’s reasonable. Imagine a hash function that returns the x coordinate, but only if y is odd (or has Jacobi symbol 1, or whatever the parity is).

apoelstra commented at 10:17 pm on April 5, 2018: contributor

@fanatid I’m not able to rebase your PR it seems. Can you do it? If you want, my version of the commit is at https://github.com/apoelstra/secp256k1/tree/custom-ecdh-hash-function

fanatid force-pushed on Apr 6, 2018

fanatid commented at 8:32 am on April 6, 2018: contributor

@apoelstra rebased

in include/secp256k1_ecdh.h:37 in 1dedb7bdfe outdated

34- *                       secret computed from the point and scalar
35+ *  Out:     output:     pointer to an array to be filled by the function
36  *  In:      pubkey:     a pointer to a secp256k1_pubkey containing an
37  *                       initialized public key
38  *           privkey:    a 32-byte scalar with which to multiply the point
39+ *           hashfp:		 pointer to a hash function. If NULL, secp256k1_ecdh_hash_function_sha256 is used

apoelstra commented at 1:44 pm on April 6, 2018:

awkward spacing

fanatid commented at 2:09 pm on April 6, 2018:

fixed

apoelstra commented at 1:47 pm on April 6, 2018: contributor

ACK aside from spacing nit

fanatid force-pushed on Apr 6, 2018

chfast approved

chfast commented at 3:20 pm on April 9, 2018: none

What more is required to get it merged?

apoelstra commented at 5:01 pm on April 10, 2018: contributor

cc @sipa

sipa commented at 5:05 pm on April 10, 2018: contributor

Hmm, is there no need for a data parameter that gets passed to the hashing function? This way the hash function cannot access any application dependent data, unless through a global (which is not threadsafe)?

chfast commented at 2:26 pm on May 16, 2018: none

Hmm, is there no need for a data parameter that gets passed to the hashing function? This way the hash function cannot access any application dependent data, unless through a global (which is not threadsafe)?

Is this a recommended change in this PR?

apoelstra commented at 2:28 pm on May 16, 2018: contributor

@chfast Yeah, I think so. Our other custom hash functions take an arbitrary data parameter.

chfast commented at 2:37 pm on May 16, 2018: none

I can add it then if @fanatid does not have time.

[ECDH API change] Support custom hash function b00be65056

fanatid force-pushed on May 16, 2018

[ECDH API change] Allow pass arbitrary data to hash function c8fbc3c397

fanatid force-pushed on May 16, 2018

fanatid commented at 10:04 pm on May 16, 2018: contributor

Pointer to arbitrary data added to hash function.

ofek commented at 2:11 am on May 23, 2018: none

Is this ready to be merged?

sipa commented at 7:04 pm on May 31, 2018: contributor

utACK c8fbc3c397b547bc64435a9bffb8f989cd23aba0

apoelstra commented at 5:41 pm on July 4, 2018: contributor

ACK

chfast commented at 2:40 pm on August 9, 2018: none

Can this be merged finally?

chfast commented at 11:59 am on October 16, 2018: none

Can this be merged finally?

Can it?

sipa merged this on Oct 17, 2018

sipa closed this on Oct 17, 2018

sipa referenced this in commit 1086fda4c1 on Oct 17, 2018

fanatid deleted the branch on Oct 17, 2018

fanatid cross-referenced this on Dec 20, 2018 from issue Update secp256k1 (support custom function for ECDH) by fanatid

sipa cross-referenced this on Jan 7, 2020 from issue Review if ECDH is still experimental by ysangkok

[ECDH API change] Support custom hash function #354