secp256k1
secp256k1 copied to clipboard
bitcoin-core
Add x-only ECDH support to ecdh module
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.
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.
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
@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.
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.)
@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.
@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 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!
@real-or-random Ping?
Yep, I'll take care of that!
Will do today or tomorrow!
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. :)
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 https://github.com/bitcoin/bitcoin/pull/28983
@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.
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?
@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.
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 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: https://github.com/bitcoin/bitcoin/pull/28983/commits/27928dba41bb0186bfe82e6668c0f1024f056772 (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.
@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.
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.
@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.
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?
@Sjors rust-secp256k1 appears to have ellswift bindings, but I don't know anything about Rust myself.
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.