[ECDH API change] Support custom hash function #354

pull fanatid wants to merge 2 commits into bitcoin-core:master from fanatid:custom-ecdh-hash-function changing 5 files +101 −35
  1. fanatid commented at 8:52 AM on November 12, 2015: contributor

    Solve #352

  2. fanatid cross-referenced this on Jan 13, 2016 from issue RFC: non-hashed ecdh by axic
  3. fanatid cross-referenced this on Feb 1, 2016 from issue 3.0.0 by wanderer
  4. axic cross-referenced this on Apr 22, 2016 from issue Support custom hash functions in ECDH API by Kagami
  5. 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.

  6. fanatid closed this on Apr 27, 2016
  7. fanatid deleted the branch on Jan 7, 2017
  8. fanatid cross-referenced this on Mar 10, 2017 from issue ECDH: output compressed point by chfast
  9. fanatid restored the branch on Mar 12, 2017
  10. fanatid reopened this on Mar 12, 2017
  11. fanatid force-pushed on Mar 12, 2017
  12. 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!

  13. fanatid force-pushed on Mar 12, 2017
  14. 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.

  15. 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

  16. 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

  17. sipa commented at 11:57 PM on March 21, 2017: contributor

    Concept ACK

  18. fanatid force-pushed on Mar 27, 2017
  19. chfast commented at 5:34 PM on August 8, 2017: none

    How is this going on?

  20. chfast commented at 4:57 PM on April 5, 2018: none

    Ping.

  21. apoelstra commented at 4:59 PM on April 5, 2018: contributor

    I'll rebase and review it.

  22. apoelstra assigned apoelstra on Apr 5, 2018
  23. 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).

  24. 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

  25. fanatid force-pushed on Apr 6, 2018
  26. fanatid force-pushed on Apr 6, 2018
  27. fanatid force-pushed on Apr 6, 2018
  28. fanatid commented at 8:32 AM on April 6, 2018: contributor

    @apoelstra rebased

  29. 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

  30. apoelstra commented at 1:47 PM on April 6, 2018: contributor

    ACK aside from spacing nit

  31. fanatid force-pushed on Apr 6, 2018
  32. chfast approved
  33. chfast commented at 3:20 PM on April 9, 2018: none

    What more is required to get it merged?

  34. apoelstra commented at 5:01 PM on April 10, 2018: contributor

    cc @sipa

  35. 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)?

  36. 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?

  37. 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.

  38. chfast commented at 2:37 PM on May 16, 2018: none

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

  39. [ECDH API change] Support custom hash function b00be65056
  40. fanatid force-pushed on May 16, 2018
  41. [ECDH API change] Allow pass arbitrary data to hash function c8fbc3c397
  42. fanatid force-pushed on May 16, 2018
  43. fanatid commented at 10:04 PM on May 16, 2018: contributor

    Pointer to arbitrary data added to hash function.

  44. ofek commented at 2:11 AM on May 23, 2018: none

    Is this ready to be merged?

  45. sipa commented at 7:04 PM on May 31, 2018: contributor

    utACK c8fbc3c397b547bc64435a9bffb8f989cd23aba0

  46. apoelstra commented at 5:41 PM on July 4, 2018: contributor

    ACK

  47. chfast commented at 2:40 PM on August 9, 2018: none

    Can this be merged finally?

  48. chfast commented at 11:59 AM on October 16, 2018: none

    Can this be merged finally?

    Can it?

  49. sipa merged this on Oct 17, 2018
  50. sipa closed this on Oct 17, 2018
  51. sipa referenced this in commit 1086fda4c1 on Oct 17, 2018
  52. fanatid deleted the branch on Oct 17, 2018
  53. fanatid cross-referenced this on Dec 20, 2018 from issue Update secp256k1 (support custom function for ECDH) by fanatid
  54. sipa cross-referenced this on Jan 7, 2020 from issue Review if ECDH is still experimental by ysangkok
Contributors
fanatidchfastdcousenssipaapoelstraofek
Linked (view graph)
#352 Support custom hash functions in ECDH API#446 ECDH: output compressed point#665 Review if ECDH is still experimental
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin-core/secp256k1. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-04-27 05:15 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me