Depends on #25361 for some constants, and on https://github.com/bitcoin-core/secp256k1/pull/1129 for ellswift integrated XDH but can be reviewed independently. Only the last 5 commits belong to this PR.
This PR adds xonly ECDH and HKDF key derivation code for BIP324. Unit, bench and fuzz tests are included.
The dependency tree for BIP324 PRs is here.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| Concept ACK | naumenkogs, PastaPastaPasta, theStack |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
<!--174a7506f384e20aa4161008e828411d-->
Reviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
Concept ACK. Moving forward with BIP324 is a good idea, and this code seems to not have any negative/unexpected side-effects.
341 | + } 342 | + 343 | + ECDHSecret secret; 344 | + secret.resize(ECDH_SECRET_SIZE); 345 | + 346 | + if (secp256k1_ecdh(secp256k1_context_sign, secret.data(), &pubkey_internal,
The case of secp256k1_ecdh returning 0 corresponds to invalid input. That won't happen here; I think you can assert instead and avoid the std::optional in the return type.
Done - still needed a boolean return value due to the other failure case above.
304 | @@ -305,4 +305,20 @@ FUZZ_TARGET_INIT(key, initialize_key) 305 | assert(key == loaded_key); 306 | } 307 | } 308 | + 309 | + { 310 | + // Test ECDH computation with another key
You could do a much stronger test: take two private keys as input fuzz data, compute the corresponding public keys, and compute the ecdh secrets from (sec1,pub2) and (sec2,pub1), and check that they match.
It's cryptographic data (hashes and EC operations), so this is very unlikely to actually uncover any bugs unless they're very apparent, but this way the fuzz test actually verifies the full behavior we desire from these functions.
Done!
21 | + std::array<uint8_t, BIP324_KEY_LEN> responder_V; // k2b 22 | + std::array<uint8_t, BIP324_KEY_LEN> session_id; 23 | + 24 | + // Takes ownership of the ECDH secret and wipes the ephemeral secret once 25 | + // the derivation is complete. 26 | + explicit BIP324Keys(ECDHSecret&& ecdh_secret, const Span<uint8_t>& initiator_hdata, const Span<uint8_t>& responder_hdata);
Nit: pass Spans by value.
Done.
0 | @@ -0,0 +1,36 @@ 1 | +// Copyright (c) 2021 The Bitcoin Core developers
I'm not sure this belongs in util, it seems pretty specific to Bitcoin.
I moved this into net.{h|cpp} as that's where it will be used. I didn't particularly want to increase the length of those files, so please let me know if the right thing to do is to have it in src/bip324.{h|cpp}
10 | + 11 | +#include <array> 12 | + 13 | +constexpr size_t BIP324_KEY_LEN = 32; 14 | + 15 | +class BIP324Keys
Not sure if a class for this is desirable in the first place. These keys shouldn't be long-lived in any case (they're only used to initialize the stream ciphers, and then destroyed). That can just as well be done with a simple function call?
Done.
400 | @@ -399,6 +401,38 @@ static CAddress GetBindAddress(SOCKET sock) 401 | return addr_bind; 402 | } 403 | 404 | +bool derive_bip324_keys(ECDHSecret&& ecdh_secret, const Span<uint8_t> initiator_hdata, const Span<uint8_t> responder_hdata, BIP324Keys& derived_keys)
Why snake case for function name derive_bip324_keys here?
Oh oops. I don't know why I did that. Updated.
405 | +{ 406 | + if (ecdh_secret.size() != ECDH_SECRET_SIZE) { 407 | + return false; 408 | + } 409 | + 410 | + std::string salt{"bitcoin_v2_shared_secret"};
The BIP currently refers to BitcoinSharedSecret. I guess to ack this, i'd have to see a new bip :)
:) I am working on re-structuring it. It's close. Will keep you updated here.
Addressed comments; Rebased with master to incorporate #23413. I am still investigating CI failures which seem like a timeout on Cirrus CI but re-running hasn't helped (other recent open PRs seem to be seeing this error as well). Meanwhile, ready for further review.
Rebased against master. Ready for further review.
0 | @@ -0,0 +1,30 @@ 1 | +// Copyright (c) 2021 The Bitcoin Core developers
nit: if you rebase this at some point, might as well update this
Done.
concept ACK / light utACK. I did a high level review, and everything makes sense to me.
I do have 1 tiny nit
Rebased. Ready for further review.
Rebased. git range-diff e0680bbce ea94adb 2a2cbfc1e
Ready for further review.
Rebased. Ready for further review.
Updated to use xonly ECDH as per changes being made to BIP324 before publication. Ready for further review.
1956 | @@ -1957,7 +1957,7 @@ LIBS_TEMP="$LIBS" 1957 | unset LIBS 1958 | LIBS="$LIBS_TEMP" 1959 | 1960 | -ac_configure_args="${ac_configure_args} --disable-shared --with-pic --enable-benchmark=no --enable-module-recovery --enable-module-schnorrsig" 1961 | +ac_configure_args="${ac_configure_args} --disable-shared --with-pic --enable-benchmark=no --enable-module-recovery --enable-module-schnorrsig --enable-module-ecdh -enable-experimental"
You can drop -enable-experimental. It should no-longer be required for --enable-module-ecdh, and note the missing - in any case.
Thanks. Done.
362 | + 363 | + hasher.Write(xonly_ecdh.data(), xonly_ecdh.size()); 364 | + 365 | + secret.resize(ECDH_SECRET_SIZE); 366 | + hasher.Finalize(secret.data()); 367 | + memory_cleanse(xonly_ecdh.data(), ECDH_SECRET_SIZE);
This is unnecessary, as it's already using secure_allocator, which wipes memory on deallocation.
That said, using a vector with secure_allocator is overkill here; you can just use an array (which doesn't need allocation) - and then you do need the cleanse call.
Done - simpler with array + cleanse.
925 | + static const std::string strSecret2C = "L3Hq7a8FEQwJkW1M2GNKDW28546Vp5miewcCzSqUD9kCAXrJdS3g"; 926 | + static const std::string initiator_hdata = "2deb41da6887640dda029ae41c9c9958881d0bb8e28f6bb9039ee9b7bb11091d62f4cbe65cc418df7aefd738f4d3e926c66365b4d38eefd0a883be64112f4495"; 927 | + static const std::string responder_hdata = "4c469c70ba242ae0fc98d4eff6258cf19ecab96611c9c736356a4cf11d66edfa4d2970e56744a6d071861a4cbe2730eb7733a38b166e3df73450ef37112dd32f"; 928 | + 929 | + auto initiator_hdata_vec = ParseHex(initiator_hdata); 930 | + Span<uint8_t> initiator_hdata_span{initiator_hdata_vec.data(), initiator_hdata_vec.size()};
These spans are unnecessary. You can just pass initiator_hdata_vec etc. whereever initiator_hdata_span is used (they get converted automatically).
Done.
975 | + BOOST_CHECK_EQUAL(0, memcmp(initiator_keys.responder_V.data(), responder_keys.responder_V.data(), initiator_keys.responder_V.size())); 976 | + BOOST_CHECK_EQUAL("94e374b822bc293b4eb3475aed2457e1b2fc9691df9ffb02afc462c36d513783", HexStr(Span{initiator_keys.responder_V})); 977 | + 978 | + BOOST_CHECK_EQUAL(BIP324_KEY_LEN, initiator_keys.session_id.size()); 979 | + BOOST_CHECK_EQUAL(initiator_keys.session_id.size(), responder_keys.session_id.size()); 980 | + BOOST_CHECK_EQUAL(0, memcmp(initiator_keys.session_id.data(), responder_keys.session_id.data(), initiator_keys.session_id.size()));
You can use BOOST_CHECK(initiator_keys.session_id == responder_keys.session_id) instead here.
Done.
24 | @@ -22,6 +25,14 @@ 25 | */ 26 | typedef std::vector<unsigned char, secure_allocator<unsigned char> > CPrivKey; 27 | 28 | +constexpr static size_t ECDH_SECRET_SIZE = CSHA256::OUTPUT_SIZE; 29 | + 30 | +// SHA256("secp256k1_ellsq_xonly_ecdh") proven with a unit test (xonly_ecdh_tag) 31 | +const static auto BIP324_ECDH_TAG = ParseHex("4dbebba5394890bf7370e9ca2ca6112941b974c56950e5c4fef7f00a2bbf3e55");
You can also use const CHashWriter HASHER_BIP324_ECDH = TaggedHash("secp256k1_ellsq_xonly_ecdh");, and then do the rest of the hashing using (HashWriter(HASHER_BIP324_ECDH) << [rest of stuff] << [to hash]).GetSHA256(). That precomputes the entire midstate after the tag.
Super nice change - much cleaner. Thanks for showing me that. Done.
339 | +{ 340 | + memcpy(output, x32, 32); 341 | + return 1; 342 | +} 343 | + 344 | +bool CKey::ComputeBIP324ECDHSecret(const CPubKey& pubkey, const Span<uint8_t> initiator_hdata, const Span<uint8_t> responder_hdata,
You can make the arguments here take Span<const uint8_t> - or even Span<const std::byte>. I don't think this function needs to be able to mutate the passed in hdata.
Done.
349 | +{ 350 | + CKey initiator_key = DecodeSecret(strSecret1); 351 | + CKey responder_key = DecodeSecret(strSecret2C); 352 | + 353 | + ECDHSecret initiator_secret, responder_secret; 354 | + auto initiator_hdata_vec = ParseHex("2deb41da6887640dda029ae41c9c9958881d0bb8e28f6bb9039ee9b7bb11091d62f4cbe65cc418df7aefd738f4d3e96c66365b4d38eefd0a883be64112f4495");
to match the 64 bytes encoding in commit c9db50b.
auto initiator_hdata_vec = ParseHex("2deb41da6887640dda029ae41c9c9958881d0bb8e28f6bb9039ee9b7bb11091d62f4cbe65cc418df7aefd738f4d3e926c66365b4d38eefd0a883be64112f4495");
Thanks! Done.
Concept ACK
nit: Considering that ECDHSecret always has a fixed size, do we really need to use a std::vector? An alternative would be to use std::array and remove the .resize() calls. The following diff on the top-commit seems to work fine:
diff --git a/src/key.cpp b/src/key.cpp
index aab8bbda1..791c4c878 100644
--- a/src/key.cpp
+++ b/src/key.cpp
@@ -357,7 +357,6 @@ bool CKey::ComputeBIP324ECDHSecret(const CPubKey& pubkey, const Span<const uint8
hasher << initiator_hdata << responder_hdata << xonly_ecdh;
auto secret_uint256 = hasher.GetSHA256();
- secret.resize(ECDH_SECRET_SIZE);
memcpy(secret.data(), &secret_uint256, CSHA256::OUTPUT_SIZE);
memory_cleanse(xonly_ecdh, ECDH_SECRET_SIZE);
return true;
diff --git a/src/key.h b/src/key.h
index 0fdaef1eb..60f6cce3f 100644
--- a/src/key.h
+++ b/src/key.h
@@ -29,7 +29,7 @@ constexpr static size_t ECDH_SECRET_SIZE = CSHA256::OUTPUT_SIZE;
const CHashWriter HASHER_BIP324_ECDH = TaggedHash("secp256k1_ellsq_xonly_ecdh");
// Used to represent a ECDH secret (ECDH_SECRET_SIZE bytes)
-using ECDHSecret = std::vector<uint8_t, secure_allocator<uint8_t>>;
+using ECDHSecret = std::array<uint8_t, ECDH_SECRET_SIZE>;
/** An encapsulated private key. */
class CKey
diff --git a/src/test/fuzz/net.cpp b/src/test/fuzz/net.cpp
index 09dac0f17..2ca07b7d7 100644
--- a/src/test/fuzz/net.cpp
+++ b/src/test/fuzz/net.cpp
@@ -89,7 +89,6 @@ FUZZ_TARGET_INIT(bip324, initialize_chainparams)
FuzzedDataProvider fuzzed_data_provider{buffer.data(), buffer.size()};
ECDHSecret ecdh_secret;
- ecdh_secret.resize(ECDH_SECRET_SIZE);
auto ecdh_secret_bytes = fuzzed_data_provider.ConsumeBytes<uint8_t>(ECDH_SECRET_SIZE);
ecdh_secret_bytes.resize(ECDH_SECRET_SIZE);
Not sure though what security implications this has without the secure_allocator being involved.
This push:
std::array instead of a secure vector.Ready for further review.
349 | + return false; 350 | + } 351 | + 352 | + uint8_t xonly_ecdh[ECDH_SECRET_SIZE]; 353 | + assert(secp256k1_ecdh(secp256k1_context_sign, xonly_ecdh, &pubkey_internal, 354 | + keydata.data(), bip324_ecdh_hash, nullptr));
According to our guidelines, assertions shouldn't have side-effects (see https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#general-c).
if (!secp256k1_ecdh(secp256k1_context_sign, xonly_ecdh, &pubkey_internal,
keydata.data(), bip324_ecdh_hash, nullptr)) {
assert(false);
}
Thanks for teaching me this. Done.
165 | @@ -156,6 +166,10 @@ class CKey 166 | 167 | //! Load private key and check that public key matches. 168 | bool Load(const CPrivKey& privkey, const CPubKey& vchPubKey, bool fSkipCheck); 169 | + 170 | + // Returns false if an invalid public key is provided 171 | + bool ComputeBIP324ECDHSecret(const CPubKey& pubkey, const Span<const uint8_t> initiator_hdata, const Span<const uint8_t> responder_hdata, 172 | + ECDHSecret& secret) const;
nit: An alternative, probably more modern interface would be to return std::optional<ECDHSecret> instead of storing the result in an out-parameter.
// Returns std::nullopt if an invalid public key is provided
std::optional<ECDHSecret> ComputeBIP324ECDHSecret(const CPubKey& pubkey, const Span<const uint8_t> initiator_hdata, const Span<const uint8_t> responder_hdata) const;
Done.
358 | + Span<uint8_t> responder_hdata{responder_hdata_vec.data(), responder_hdata_vec.size()}; 359 | + 360 | + BOOST_CHECK(initiator_key.ComputeBIP324ECDHSecret(responder_key.GetPubKey(), initiator_hdata, responder_hdata, initiator_secret)); 361 | + BOOST_CHECK(responder_key.ComputeBIP324ECDHSecret(initiator_key.GetPubKey(), initiator_hdata, responder_hdata, responder_secret)); 362 | + BOOST_CHECK_EQUAL(initiator_secret.size(), ECDH_SECRET_SIZE); 363 | + BOOST_CHECK_EQUAL(responder_secret.size(), ECDH_SECRET_SIZE);
nit: These checks obviously don't hurt, but now that ECDHSecret uses std::array and has it's size fixed at compile-time, they are not that useful anymore.
Done.
359 | + 360 | + BOOST_CHECK(initiator_key.ComputeBIP324ECDHSecret(responder_key.GetPubKey(), initiator_hdata, responder_hdata, initiator_secret)); 361 | + BOOST_CHECK(responder_key.ComputeBIP324ECDHSecret(initiator_key.GetPubKey(), initiator_hdata, responder_hdata, responder_secret)); 362 | + BOOST_CHECK_EQUAL(initiator_secret.size(), ECDH_SECRET_SIZE); 363 | + BOOST_CHECK_EQUAL(responder_secret.size(), ECDH_SECRET_SIZE); 364 | + BOOST_CHECK_EQUAL(0, memcmp(initiator_secret.data(), responder_secret.data(), ECDH_SECRET_SIZE));
nit: could simply use operator== of std::array here:
BOOST_CHECK(initiator_secret == responder_secret);
Done.
340 | + ECDHSecret ecdh_secret_1, ecdh_secret_2; 341 | + assert(k1.ComputeBIP324ECDHSecret(k2_pubkey, k1_ellsq, k2_ellsq, ecdh_secret_1)); 342 | + assert(k2.ComputeBIP324ECDHSecret(k1_pubkey, k1_ellsq, k2_ellsq, ecdh_secret_2)); 343 | + assert(ecdh_secret_1.size() == ECDH_SECRET_SIZE); 344 | + assert(ecdh_secret_2.size() == ECDH_SECRET_SIZE); 345 | + assert(memcmp(ecdh_secret_1.data(), ecdh_secret_2.data(), ECDH_SECRET_SIZE) == 0);
nit:
assert(ecdh_secret_1 == ecdh_secret_2);
Done.
439 | @@ -438,6 +440,42 @@ static CAddress GetBindAddress(const Sock& sock) 440 | return addr_bind; 441 | } 442 | 443 | +bool DeriveBIP324Keys(ECDHSecret&& ecdh_secret, const Span<uint8_t> initiator_hdata, const Span<uint8_t> responder_hdata, BIP324Keys& derived_keys) 444 | +{ 445 | + if (ecdh_secret.size() != ECDH_SECRET_SIZE) { 446 | + return false; 447 | + } 448 | +
Since ECDHSecret has a fixed size (std::array<uint8_t, ECDH_SECRET_SIZE>), this is dead code now and can be removed.
Done. This also let me optimize the interface to the function as it always succeeds now.
942 | + BOOST_CHECK(initiator_key.ComputeBIP324ECDHSecret(responder_pubkey, initiator_hdata_vec, responder_hdata_vec, initiator_secret)); 943 | + BOOST_CHECK(responder_key.ComputeBIP324ECDHSecret(initiator_pubkey, initiator_hdata_vec, responder_hdata_vec, responder_secret)); 944 | + 945 | + BOOST_CHECK_EQUAL(ECDH_SECRET_SIZE, initiator_secret.size()); 946 | + BOOST_CHECK_EQUAL(ECDH_SECRET_SIZE, responder_secret.size()); 947 | + BOOST_CHECK_EQUAL(0, memcmp(initiator_secret.data(), responder_secret.data(), ECDH_SECRET_SIZE));
nit:
BOOST_CHECK(initiator_secret == responder_secret);
Done.
Latest push:
Ready for further review.
Latest push:
Ready for further review. I don't yet know what's happening with the vs2022 tests on CI, but I will address that once https://github.com/bitcoin-core/secp256k1/pull/1129 is merged.
26 | + return; 27 | + } 28 | + 29 | + std::array<unsigned char, 32> rnd32; 30 | + GetRandBytes(rnd32); 31 | + secp256k1_ellswift_encode(ctx, ell64, &pubkey_internal, rnd32.data());
The new ellswift API has a function secp256k1_ellswift_create which goes directly from private key + randomness to ellswift encoding, bypassing the need for encoding the public key in the traditional form.
Much nicer. Thank you for the reminder. Done.
Addressed comments. Ready for further review.
Added rekeying salt to key derivation. Ready for further review.
358 | + BOOST_CHECK(initiator_secret.has_value()); 359 | + auto responder_secret = responder_key.ComputeBIP324ECDHSecret(MakeByteSpan(initiator_ellswift), MakeByteSpan(responder_ellswift), false); 360 | + BOOST_CHECK(responder_secret.has_value()); 361 | + BOOST_CHECK(initiator_secret.value() == responder_secret.value()); 362 | + BOOST_CHECK_EQUAL("6cbb9cd446f28623979b16affc4b57c4af95db832cb1a4ff8bc41e3192a33bd8", HexStr(initiator_secret.value())); 363 | + BOOST_CHECK_EQUAL("6cbb9cd446f28623979b16affc4b57c4af95db832cb1a4ff8bc41e3192a33bd8", HexStr(responder_secret.value()));
in 789d8bb: this would be the ECDH secret - ae1b0c44c3c38aa5206899c0928ca51f637e3ec05771b4a6c0662b46b76049d8
Done
27 | @@ -22,6 +28,12 @@ 28 | */ 29 | typedef std::vector<unsigned char, secure_allocator<unsigned char> > CPrivKey; 30 | 31 | +constexpr static size_t ECDH_SECRET_SIZE = CSHA256::OUTPUT_SIZE; 32 | +const auto HASHER_BIP324_ECDH = TaggedHash("secp256k1_ellsq_xonly_ecdh");
in 789d8bb8: s/secp256k1_ellsq_xonly_ecdh/secp256k1_ellswift_xonly_ecdh
in 789d8bb: would it be useful to keep the TaggedHash() function call similar to other calls in the codebase?
const HashWriter HASHER_BIP324_ECDH{TaggedHash("secp256k1_ellswift_xonly_ecdh")};
Done
928 | + BOOST_CHECK(initiator_secret.has_value()); 929 | + auto responder_secret = responder_key.ComputeBIP324ECDHSecret(MakeByteSpan(initiator_ellswift), MakeByteSpan(responder_ellswift), false); 930 | + BOOST_CHECK(responder_secret.has_value()); 931 | + BOOST_CHECK(initiator_secret.value() == responder_secret.value()); 932 | + BOOST_CHECK_EQUAL("6cbb9cd446f28623979b16affc4b57c4af95db832cb1a4ff8bc41e3192a33bd8", HexStr(initiator_secret.value())); 933 | + BOOST_CHECK_EQUAL("6cbb9cd446f28623979b16affc4b57c4af95db832cb1a4ff8bc41e3192a33bd8", HexStr(responder_secret.value()));
in 4220009b: ECDH secret is ae1b0c44c3c38aa5206899c0928ca51f637e3ec05771b4a6c0662b46b76049d8
Done
941 | + BOOST_CHECK_EQUAL("0000000000000000000000000000000000000000000000000000000000000000", HexStr(initiator_secret.value())); 942 | + BOOST_CHECK_EQUAL("0000000000000000000000000000000000000000000000000000000000000000", HexStr(responder_secret.value())); 943 | + 944 | + BOOST_CHECK_EQUAL(BIP324_KEY_LEN, initiator_keys.initiator_L.size()); 945 | + BOOST_CHECK(initiator_keys.initiator_L == responder_keys.initiator_L); 946 | + BOOST_CHECK_EQUAL("75fa71b052982a72ecb4a0e2a73697857121e4fca2f06e791a5d3c3383d4bb16", HexStr(initiator_keys.initiator_L));
in 4220009: 98b1a948d70374db8078475fd2b573789989a57d5394ecc229a3f2ec336c4d18
Done
945 | + BOOST_CHECK(initiator_keys.initiator_L == responder_keys.initiator_L); 946 | + BOOST_CHECK_EQUAL("75fa71b052982a72ecb4a0e2a73697857121e4fca2f06e791a5d3c3383d4bb16", HexStr(initiator_keys.initiator_L)); 947 | + 948 | + BOOST_CHECK_EQUAL(BIP324_KEY_LEN, initiator_keys.initiator_P.size()); 949 | + BOOST_CHECK(initiator_keys.initiator_P == responder_keys.initiator_P); 950 | + BOOST_CHECK_EQUAL("bc6617bd1473017b1b40e927c0330b74d1bdd30594a1daa49d89c831f03775cd", HexStr(initiator_keys.initiator_P));
in 4220009: 95bdf50958c46ad24c7646cd7bf7579ffafb2f032d2c5356fc8341e198d0bb51
Done
949 | + BOOST_CHECK(initiator_keys.initiator_P == responder_keys.initiator_P); 950 | + BOOST_CHECK_EQUAL("bc6617bd1473017b1b40e927c0330b74d1bdd30594a1daa49d89c831f03775cd", HexStr(initiator_keys.initiator_P)); 951 | + 952 | + BOOST_CHECK_EQUAL(BIP324_KEY_LEN, initiator_keys.responder_L.size()); 953 | + BOOST_CHECK(initiator_keys.responder_L == responder_keys.responder_L); 954 | + BOOST_CHECK_EQUAL("f31764965c22b792a5ac3f35573b01db27a8d479677c53c43d71bbc2141e8886", HexStr(initiator_keys.responder_L));
in 4220009: 0dec3e671918898ce5472b161ddfcc6f765bf459e8cdeb86825fa704a58546e5
Done
953 | + BOOST_CHECK(initiator_keys.responder_L == responder_keys.responder_L); 954 | + BOOST_CHECK_EQUAL("f31764965c22b792a5ac3f35573b01db27a8d479677c53c43d71bbc2141e8886", HexStr(initiator_keys.responder_L)); 955 | + 956 | + BOOST_CHECK_EQUAL(BIP324_KEY_LEN, initiator_keys.responder_P.size()); 957 | + BOOST_CHECK(initiator_keys.responder_P == responder_keys.responder_P); 958 | + BOOST_CHECK_EQUAL("82c8d144f1dc7b86993c248db020aecfaed37a5c215bea5a7f1bf4399a85ecca", HexStr(initiator_keys.responder_P));
in 4220009: af8b74244dd43c5921e2449a125d669f6d82a23250fa040eafc3ba2373067de5
Done
957 | + BOOST_CHECK(initiator_keys.responder_P == responder_keys.responder_P); 958 | + BOOST_CHECK_EQUAL("82c8d144f1dc7b86993c248db020aecfaed37a5c215bea5a7f1bf4399a85ecca", HexStr(initiator_keys.responder_P)); 959 | + 960 | + BOOST_CHECK_EQUAL(BIP324_KEY_LEN, initiator_keys.session_id.size()); 961 | + BOOST_CHECK(initiator_keys.session_id == responder_keys.session_id); 962 | + BOOST_CHECK_EQUAL("e6b4d42bc1fbce790cf7b419d0128c7caf319b491acfd9fd874f29ca3843fe40", HexStr(initiator_keys.session_id));
in 4220009: ae605e623a8f710f4f0c99454d74cdfb8861cad1f8f6dd2eb86390d615efab01
Done
961 | + BOOST_CHECK(initiator_keys.session_id == responder_keys.session_id); 962 | + BOOST_CHECK_EQUAL("e6b4d42bc1fbce790cf7b419d0128c7caf319b491acfd9fd874f29ca3843fe40", HexStr(initiator_keys.session_id)); 963 | + 964 | + BOOST_CHECK_EQUAL(BIP324_KEY_LEN, initiator_keys.rekey_salt.size()); 965 | + BOOST_CHECK(initiator_keys.rekey_salt == responder_keys.rekey_salt); 966 | + BOOST_CHECK_EQUAL("060d26411cdedabda638b1a807d0b68cbbb326385881409fcd2f5705091c5842", HexStr(initiator_keys.rekey_salt));
in 4220009: e46620b5e10052931401606ccbb4c810c4f9afa9db948e3acfbeaa65b0cc8199
Done
965 | + BOOST_CHECK(initiator_keys.rekey_salt == responder_keys.rekey_salt); 966 | + BOOST_CHECK_EQUAL("060d26411cdedabda638b1a807d0b68cbbb326385881409fcd2f5705091c5842", HexStr(initiator_keys.rekey_salt)); 967 | + 968 | + BOOST_CHECK_EQUAL(BIP324_GARBAGE_TERMINATOR_LEN, initiator_keys.garbage_terminator.size()); 969 | + BOOST_CHECK(initiator_keys.garbage_terminator == responder_keys.garbage_terminator); 970 | + BOOST_CHECK_EQUAL("118878010356d064", HexStr(Span{initiator_keys.garbage_terminator}));
in 4220009: 73d7412ff3b42a01
Done.
Addressed comments from @stratospher and a clang-tidy issue. Ready for further review.
961 | + BOOST_CHECK(initiator_keys.rekey_salt == responder_keys.rekey_salt); 962 | + BOOST_CHECK_EQUAL("e46620b5e10052931401606ccbb4c810c4f9afa9db948e3acfbeaa65b0cc8199", HexStr(initiator_keys.rekey_salt)); 963 | + 964 | + BOOST_CHECK_EQUAL(BIP324_GARBAGE_TERMINATOR_LEN, initiator_keys.garbage_terminator.size()); 965 | + BOOST_CHECK(initiator_keys.garbage_terminator == responder_keys.garbage_terminator); 966 | + BOOST_CHECK_EQUAL("73d7412ff3b42a01", HexStr(Span{initiator_keys.garbage_terminator}));
f5d4f36: Span can be removed.
Fixed.
343 | + 344 | +std::optional<ECDHSecret> CKey::ComputeBIP324ECDHSecret(const Span<const std::byte> their_ellswift, 345 | + const Span<const std::byte> our_ellswift, 346 | + bool initiating) const 347 | +{ 348 | + uint8_t xonly_ecdh[ECDH_SECRET_SIZE];
ab46788: uint8_t can be made std::byte.
@stratospher Upon further thought after our conversation, I think unsigned char makes sense here as that's exactly what all the functions it is used with require. So I'm going with that when I update.
Latest push:
Windows tests failing - but should get resolved once secp/PR 1129 is merged.
Latest push:
Rebased.
Latest push:
Bringing in upstream rebase (#26153)
Latest push:
Rebased
Rebased
Rebased. Brought in upstream changes.
Rebased.
Rebased. Brought in changes from https://github.com/bitcoin-core/secp256k1/pull/1129.
8034c67a48 Add doc/ellswift.md with ElligatorSwift explanation
e90aa4e62e Add ellswift testing to CI
131faedd8a Add ElligatorSwift ctime tests
198a04c058 Add tests for ElligatorSwift
9984bfe476 Add ElligatorSwift benchmarks
f053da3ab7 Add ellswift module implementing ElligatorSwift
76c64be237 Add functions to test if X coordinate is valid
aff948fca2 Add benchmark for key generation
5ed9314d6d Add exhaustive tests for ecmult_const_xonly
b69fe88d5e Add x-only ecmult_const version for x=n/d
427bc3cdcf Merge bitcoin-core/secp256k1#1236: Update comment for secp256k1_modinv32_inv256
647f0a5cb1 Update comment for secp256k1_modinv32_inv256
5658209459 Merge bitcoin-core/secp256k1#1228: release cleanup: bump version after 0.3.0
28e63f7ea7 release cleanup: bump version after 0.3.0
git-subtree-dir: src/secp256k1
git-subtree-split: 8034c67a48dc1334bc74ee4ba239111a23d9789e
Rebased.
<!--cf906140f33d8803c4a75a2196329ecb-->
🐙 This pull request conflicts with the target branch and needs rebase.