Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[ECDH API change] Support custom hash function #354

Merged
merged 2 commits into from
Oct 17, 2018
Merged

[ECDH API change] Support custom hash function #354

merged 2 commits into from
Oct 17, 2018

Conversation

fanatid
Copy link
Contributor

@fanatid fanatid commented Nov 12, 2015

Solve #352

@fanatid fanatid mentioned this pull request Jan 13, 2016
Closed
@fanatid fanatid mentioned this pull request Feb 1, 2016
Merged
@axic axic mentioned this pull request Apr 22, 2016
Closed
@fanatid
Copy link
Contributor Author

fanatid commented Apr 27, 2016

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 fanatid closed this Apr 27, 2016
@fanatid fanatid deleted the custom-ecdh-hash-function branch January 7, 2017 11:05
@fanatid fanatid mentioned this pull request Mar 10, 2017
Closed
@fanatid fanatid restored the custom-ecdh-hash-function branch March 12, 2017 06:18
@fanatid fanatid reopened this Mar 12, 2017
@fanatid fanatid force-pushed the custom-ecdh-hash-function branch from 124cf05 to d78377a Compare March 12, 2017 06:45
@fanatid
Copy link
Contributor Author

fanatid commented Mar 12, 2017 •
edited
Loading

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 fanatid force-pushed the custom-ecdh-hash-function branch from d78377a to 750782f Compare March 12, 2017 07:14
@chfast
Copy link

chfast commented Mar 21, 2017

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.

dcousens
dcousens reviewed Mar 21, 2017
View reviewed changes
src/modules/ecdh/tests_impl.h Outdated
int ecdh_hash_function_test_fail(unsigned char *output, const unsigned char *x, const unsigned char *y) {
if (1) {
return 0;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this meant to be here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed

sipa
sipa reviewed Mar 21, 2017
View reviewed changes
Copy link
Contributor

@sipa sipa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Concept ACK

include/secp256k1_ecdh.h Outdated
@@ -7,21 +7,41 @@
extern "C" {
# endif

/** A pointer to a function that apply hash function to a point
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

applies

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

@fanatid fanatid force-pushed the custom-ecdh-hash-function branch from 750782f to f35568b Compare March 27, 2017 13:57
@chfast
Copy link

chfast commented Aug 8, 2017

How is this going on?

@chfast
Copy link

chfast commented Apr 5, 2018

Ping.

@apoelstra
Copy link
Contributor

I'll rebase and review it.

@apoelstra apoelstra self-assigned this Apr 5, 2018
@apoelstra
Copy link
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 fanatid force-pushed the custom-ecdh-hash-function branch 3 times, most recently from ab09836 to 1dedb7b Compare April 6, 2018 08:05
@fanatid
Copy link
Contributor Author

fanatid commented Apr 6, 2018

@apoelstra rebased

chfast
chfast reviewed Apr 6, 2018
View reviewed changes
include/secp256k1_ecdh.h Outdated
* In: x: pointer to a 32-byte x coordinate
* y: pointer to a 32-byte y coordinate
*/
typedef int (*secp256k1_ecdh_hash_function)(
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
apoelstra reviewed Apr 6, 2018
View reviewed changes
include/secp256k1_ecdh.h Outdated
* In: pubkey: a pointer to a secp256k1_pubkey containing an
* initialized public key
* privkey: a 32-byte scalar with which to multiply the point
* hashfp: pointer to a hash function. If NULL, secp256k1_ecdh_hash_function_sha256 is used
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

awkward spacing

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

@apoelstra
Copy link
Contributor

ACK aside from spacing nit

@fanatid fanatid force-pushed the custom-ecdh-hash-function branch from 1dedb7b to 59b4894 Compare April 6, 2018 14:09
@chfast
Copy link

chfast commented Apr 9, 2018

What more is required to get it merged?

@apoelstra
Copy link
Contributor

cc @sipa

@sipa
Copy link
Contributor

sipa commented Apr 10, 2018

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
Copy link

chfast commented May 16, 2018

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
Copy link
Contributor

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

@chfast
Copy link

chfast commented May 16, 2018

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

@fanatid fanatid force-pushed the custom-ecdh-hash-function branch from 59b4894 to e96318e Compare May 16, 2018 21:13
@fanatid fanatid force-pushed the custom-ecdh-hash-function branch from e96318e to c8fbc3c Compare May 16, 2018 21:39
@fanatid
Copy link
Contributor Author

fanatid commented May 16, 2018

Pointer to arbitrary data added to hash function.

@ofek
Copy link

ofek commented May 23, 2018

Is this ready to be merged?

@sipa
Copy link
Contributor

sipa commented May 31, 2018

utACK c8fbc3c

@apoelstra
Copy link
Contributor

ACK

@chfast
Copy link

chfast commented Aug 9, 2018

Can this be merged finally?

@chfast
Copy link

chfast commented Oct 16, 2018

Can this be merged finally?

Can it?

@sipa sipa merged commit c8fbc3c into bitcoin-core:master Oct 17, 2018
sipa added a commit that referenced this pull request Oct 17, 2018
@sipa
Merge #354: [ECDH API change] Support custom hash function
1086fda 
c8fbc3c [ECDH API change] Allow pass arbitrary data to hash function (Kirill Fomichev)
b00be65 [ECDH API change] Support custom hash function (Kirill Fomichev)

Pull request description:

  Solve #352

Tree-SHA512: f5985874d03e976cdb3d59036af7720636ad1488da40fd3bd7881b1fb71b05036a952013d519baa84c4ce4b558bdef25c4ce76b384b297e4d0aece9e37e78a01
@fanatid fanatid deleted the custom-ecdh-hash-function branch October 17, 2018 19:15
@fanatid fanatid mentioned this pull request Dec 20, 2018
Merged
@sipa sipa mentioned this pull request Jan 7, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dcousens dcousens dcousens left review comments

@sipa sipa sipa left review comments

@apoelstra apoelstra apoelstra left review comments

@chfast chfast chfast approved these changes

Assignees

@apoelstra apoelstra

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@fanatid @chfast @apoelstra @sipa @ofek @dcousens