Add x-only ECDH support to ecdh module #1198

sipa · 2023-01-27T18:32:46Z

Built on top of #1118, this adds API calls to the ecdh module to support x-only ECDH. When pubkey decompression is included in the ECDH benchmark, it's roughly 10% faster to do x-only ECDH.

The default hash function included uses SHA256(shared_x_coordinate) as output function.

real-or-random · 2023-01-28T09:33:58Z

Tried to fix the MSVC issues... let's see what CI says.

See commit messages. I verified locally that static linking doesn't break and just outputs a warning.

hebasto · 2023-01-28T09:46:28Z

Tried to fix the MSVC issues... let's see what CI says.

See commit messages. I verified locally that static linking doesn't break and just outputs a warning.

FWIW, the last commit fixes this branch build with MSVC when using CMake-based build system as well: https://cirrus-ci.com/build/6082649656131584

sipa · 2023-01-28T17:29:10Z

@real-or-random Thanks for the fixes. I've removed my earlier attempt from the commit history. If this works, I'll extract the non-ECDH-related parts from this PR and submit them separately.

sipa · 2023-01-28T17:39:19Z

include/secp256k1.h

+# ifdef SECP256K1_BUILD
+#  ifdef DLL_EXPORT
+#   define SECP256K1_API            __declspec (dllexport)
+#   define SECP256K1_API_VAR extern __declspec (dllexport)
 #  endif


What if defined(SECP256K1_BUILD) && !defined(DLL_EXPORT), then SECP256K1_API{,_VAR} will be undefined?

This is handled in #ifndef block in line 163. Maybe the same logic can be written in a more readable structure, feel free to reorganize if you see a better method.

real-or-random · 2023-01-28T19:20:04Z

We should probably move my two build/ci commits in a separate commit. At least the build commit fixes a real build issue that we simply haven't encountered so far, because none of our "user" binaries (bench+examples) have used variables exported from the variable so far (but just functions).

(I can take care of that, and I still want to rethink one detail here.)

sipa · 2023-01-28T19:52:25Z

@real-or-random Yeah, I think both of your commits should be submitted as a separate PR. Perhaps the benchmark change here should also (as ECDH benchmarking without including pubkey decoding is misleading), so I was planning to do those 3. It won't be before tonight though, so if you want to, go ahead.

sipa · 2023-01-29T03:07:12Z

@real-or-random Actually since both of the commits are yours, and perhaps you want to make some changes, perhaps you should open a PR for them?

I'll rebase this and/or open the ECDH benchmark change as a follow-up then.

real-or-random · 2023-01-30T10:10:59Z

@real-or-random Actually since both of the commits are yours, and perhaps you want to make some changes, perhaps you should open a PR for them?

Yep, I'll take care of that!

sipa · 2023-02-06T15:07:36Z

@real-or-random Ping?

Yep, I'll take care of that!

real-or-random · 2023-02-06T15:54:00Z

@real-or-random Ping?

Yep, I'll take care of that!

Will do today or tomorrow!

…from DLLs e433034 ci: Shutdown wineserver whenever CI script exits (Tim Ruffing) 9a5a611 build: Suppress stupid MSVC linker warning (Tim Ruffing) 739c53b examples: Extend sig examples by call that uses static context (Tim Ruffing) 914276e build: Add SECP256K1_API_VAR to fix importing variables from DLLs (Tim Ruffing) Pull request description: ... and more Windows fixes, please see the individual commits. The fixed issues were discovered in #1198. ACKs for top commit: sipa: utACK e433034 hebasto: ACK e433034, tested on Windows using [CMake](#1113) (which means that the 3rd commit is reviewed only, but not tested). FWIW, `LNK4217` warnings have been indeed observed. Tree-SHA512: ce7845b106190cdc517988c30aaf2cc9f1d6da22904dfc5cb6bf4ee05f063929dc8b3038479e703b6cebac79d1c21d0c84560344d2478cb1c1740087383f40e3

sipa · 2023-09-19T16:58:00Z

@real-or-random

Will do today or tomorrow!

Never mind, rebased.

real-or-random · 2023-09-20T17:18:59Z

Concept ACK

Not entirely sure if there's demand for this, but we have it in the code base, and we have an ECDH module already, so I think it's pragmatic to expose this.

But this should hash both public keys into the shared secret. :)

Sjors · 2024-01-05T11:41:28Z

Concept ACK.

Stratum v2 uses x-only ECDH (but not ElligatorSwift), so this would be quite useful.

I'll try if I can get this PR to work with bitcoin/bitcoin#28983

sipa · 2024-01-05T13:00:17Z

@Sjors This reminds me to pick this up again. We agreed that the interface needed a rewrite to use the safer approach the ellswift module uses (with a default hasher that includes the two pubkeys). I'm sure that whatever Sv2 uses can be adapted to fit in that model.

Sjors · 2024-01-05T13:38:26Z

When naively performing ECDH with x-only keys, by just converting it to a compressed key that starts with 0x02, are the two sides supposed to get the same result?

ECDH(Alice_priv, Bob_pub_x) == ECDH(Bob_priv, Alice_pub_x)

Or only if Alice_priv and Bob_priv are chosen such that their public key has even Y, i.e. the compressed key starts with 0x02? Or is that not guaranteed at all?

sipa · 2024-01-05T14:27:30Z

@Sjors That will work. In other words, negating the private key of one of the sides (or of both sides) won't change the resulting shared X coordinate. It will change the Y coordinate of the resulting shared point, but in x-only ECDH you normally ignore that Y coordinate.

Sjors · 2024-01-05T14:48:49Z

Mmm, but when using secp256k1_ecdh it does matter, doesn't it? Because the default ecdh_hash_function_sha256 uses the y-coordinate (version = (y32[31] & 0x01) | 0x02;).

Is that function based on any RFC or existing standard?

sipa · 2024-01-05T14:50:19Z

@Sjors Not that I know of. What does Sv2 use?

Sjors · 2024-01-05T15:01:15Z

@sipa the spec is ambiguous on the ECDH details: https://github.com/stratum-mining/sv2-spec/blob/main/04-Protocol-Security.md#4311-ec-point-encoding-remarks

The Template Provider PR above uses ecdh_hash_function_sha256: bitcoin/bitcoin@27928db (link to commit since I'm about to refactor this)

When you call that with a private key that has an odd corresponding public key, then both sides of the connection will generate a different output for CKey::ECDH(). That in turns breaks the cypher.

sipa · 2024-01-05T15:07:35Z

@Sjors Heh, it's not specified how the shared point is serialized? That's worth addressing in the spec if possible (... or if it can still be changed, use ellswift?).

In any case, yes, if the full compressed point serialization of the shared point is used as secret, then you need to make sure the private key matches the (full) public key. That means it's not actually x-only ECDH, and this PR (or its envisioned successor) won't be usable, as it doesn't even compute the resulting shared Y coordinate.

Sjors · 2024-01-05T15:43:47Z

I think the spec can still be modified / clarified. It should probably only use the x-component of the shared point. That seems more consistent with the use of x-only public keys in the rest of the sv2 spec.

sipa · 2024-01-05T17:43:15Z

@Sjors Is it an option to use ellswift negotiation instead? It's effectively a drop-in replacement for ECDH, except the public keys are 64 bytes instead of 32. Depending on how exactly Noise is being used to encode messages boundaries, that may actually be sufficient to make Sv2 have a pseudorandom bytestream.

Sjors · 2024-01-06T10:55:38Z

On the Bitcoin Core side that would be trivial to add support for. The Stratum Reference Implementation is written in Rust, is there already a library for that? Or I guess it would use libsecp indirectly anyway?

sipa · 2024-01-07T14:27:03Z

@Sjors rust-secp256k1 appears to have ellswift bindings, but I don't know anything about Rust myself.

real-or-random · 2024-02-06T22:55:12Z

include/secp256k1_ecdh.h

+ *           data:       arbitrary data pointer that is passed through to hashfp
+ *                       (can be NULL for secp256k1_ecdh_xonly_hash_function_sha256).
+ *
+ * The function is constant time in seckey. It is not constant time in xpubkey, hashfp, or the output.


I truly hope that it's constant time in the output (if hashfp is constant time).

Sjors · 2024-02-07T08:19:32Z

There's pretty good progress in Stratum v2 on switching to EllSwift for both encoding the key on the wire and performing ECDH, so we probably won't need this.

sipa mentioned this pull request Jan 27, 2023

x-only ECDH support #994

Open

sipa force-pushed the 202301_ecdh_xonly branch from e18ff7c to 090b20b Compare January 27, 2023 21:56

sipa force-pushed the 202301_ecdh_xonly branch from e5479c1 to e7e86fe Compare January 28, 2023 17:28

sipa commented Jan 28, 2023

View reviewed changes

real-or-random mentioned this pull request Feb 6, 2023

build: Add SECP256K1_API_VAR to fix importing variables from DLLs #1209

Merged

sipa mentioned this pull request May 8, 2023

ECDH from compressed points with "free" sqrt #363

Closed

real-or-random mentioned this pull request May 11, 2023

x-only ECDH without sqrt #262

Open

real-or-random added feature performance labels May 11, 2023

sipa added 4 commits September 19, 2023 12:41

Add x-only ECDH functionality to ecdh module

b66e0d1

Update ECDH benchmarks to include pubkey decode and add x-only

b62b7b7

Add X-only ECDH unit tests

0e8602c

Add x-only ECDH ctime tests

a732222

sipa force-pushed the 202301_ecdh_xonly branch from e7e86fe to a732222 Compare September 19, 2023 16:57

jonasnick added this to the 0.4.1 or 0.5.0 milestone Sep 20, 2023

real-or-random removed this from the 0.4.1 or 0.5.0 milestone Nov 20, 2023

Sjors mentioned this pull request Jan 9, 2024

ECDH is underspecified and not x-only stratum-mining/sv2-spec#65

Open

real-or-random reviewed Feb 6, 2024

View reviewed changes

real-or-random mentioned this pull request Feb 6, 2024

RecPedPop: use ECDH between hostkeys to drop the enckeys round BlockstreamResearch/bip-frost-dkg#15

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add x-only ECDH support to ecdh module #1198

Add x-only ECDH support to ecdh module #1198

sipa commented Jan 27, 2023

real-or-random commented Jan 28, 2023 •

edited

Loading

hebasto commented Jan 28, 2023 •

edited

Loading

sipa commented Jan 28, 2023

sipa Jan 28, 2023

real-or-random Jan 28, 2023

real-or-random commented Jan 28, 2023 •

edited

Loading

sipa commented Jan 28, 2023

sipa commented Jan 29, 2023

real-or-random commented Jan 30, 2023

sipa commented Feb 6, 2023

real-or-random commented Feb 6, 2023

sipa commented Sep 19, 2023

real-or-random commented Sep 20, 2023

Sjors commented Jan 5, 2024

sipa commented Jan 5, 2024

Sjors commented Jan 5, 2024

sipa commented Jan 5, 2024 •

edited

Loading

Sjors commented Jan 5, 2024 •

edited

Loading

sipa commented Jan 5, 2024

Sjors commented Jan 5, 2024

sipa commented Jan 5, 2024

Sjors commented Jan 5, 2024 •

edited

Loading

sipa commented Jan 5, 2024

Sjors commented Jan 6, 2024

sipa commented Jan 7, 2024

real-or-random Feb 6, 2024 •

edited

Loading

Sjors commented Feb 7, 2024

Add x-only ECDH support to ecdh module #1198

Are you sure you want to change the base?

Add x-only ECDH support to ecdh module #1198

Conversation

sipa commented Jan 27, 2023

real-or-random commented Jan 28, 2023 • edited Loading

hebasto commented Jan 28, 2023 • edited Loading

sipa commented Jan 28, 2023

sipa Jan 28, 2023

Choose a reason for hiding this comment

real-or-random Jan 28, 2023

Choose a reason for hiding this comment

real-or-random commented Jan 28, 2023 • edited Loading

sipa commented Jan 28, 2023

sipa commented Jan 29, 2023

real-or-random commented Jan 30, 2023

sipa commented Feb 6, 2023

real-or-random commented Feb 6, 2023

sipa commented Sep 19, 2023

real-or-random commented Sep 20, 2023

Sjors commented Jan 5, 2024

sipa commented Jan 5, 2024

Sjors commented Jan 5, 2024

sipa commented Jan 5, 2024 • edited Loading

Sjors commented Jan 5, 2024 • edited Loading

sipa commented Jan 5, 2024

Sjors commented Jan 5, 2024

sipa commented Jan 5, 2024

Sjors commented Jan 5, 2024 • edited Loading

sipa commented Jan 5, 2024

Sjors commented Jan 6, 2024

sipa commented Jan 7, 2024

real-or-random Feb 6, 2024 • edited Loading

Choose a reason for hiding this comment

Sjors commented Feb 7, 2024

real-or-random commented Jan 28, 2023 •

edited

Loading

hebasto commented Jan 28, 2023 •

edited

Loading

real-or-random commented Jan 28, 2023 •

edited

Loading

sipa commented Jan 5, 2024 •

edited

Loading

Sjors commented Jan 5, 2024 •

edited

Loading

Sjors commented Jan 5, 2024 •

edited

Loading

real-or-random Feb 6, 2024 •

edited

Loading