BIP324: ElligatorSwift integrations #27479

pull sipa wants to merge 9 commits into bitcoin:master from sipa:202304_bip324_ellswift changing 90 files +4198 −1176
  1. sipa commented at 10:17 pm on April 17, 2023: member

    This replaces #23432 and part of #23561.

    This PR introduces all of the ElligatorSwift-related changes (libsecp256k1 updates, generation, decoding, ECDH, tests, fuzzing, benchmarks) needed for BIP324.

    ElligatorSwift is a special 64-byte encoding format for public keys introduced in libsecp256k1 in https://github.com/bitcoin-core/secp256k1/pull/1129. It has the property that every 64-byte array is a valid encoding for some public key, and every key has approximately $2^{256}$ encodings. Furthermore, it is possible to efficiently generate a uniformly random encoding for a given public key or private key. This is used for the key exchange phase in BIP324, to achieve a byte stream that is entirely pseudorandom, even before the shared encryption key is established.

  2. DrahtBot commented at 10:17 pm on April 17, 2023: contributor

    The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK instagibbs, theStack, achow101

    If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #27973 (util: Safer MakeByteSpan with ByteSpanCast by MarcoFalke)
    • #27927 (util: Allow std::byte and char Span serialization by MarcoFalke)
    • #27827 ([Silent Payments]: Base functionality by josibake)

    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.

  3. sipa force-pushed on Apr 17, 2023
  4. sipa commented at 10:34 pm on April 17, 2023: member

    I’m in the process of taking over the BIP324 PRs from @dhruv.

    Note for reviewers:

    • Previously, the EllSwift encoding/decoding/creation was in #23432, while the ECDH logic was in #23561. I’ve opted here to merge all EllSwift-related functionality into 1 PR, as the integration is now tighter at the libsecp256k1 too compared to when the PRs were first opened (libsecp256k1 has functions for directly performing ECDH on the EllSwift-encoded keys).
    • EllSwiftPubKey is now a proper class that exposes a few utility functions, and only read-only access to the encoding.
    • Introduction of FastRandomContext::fillrand to fill Span<std::byte> with randomness.
    • Extended UCharCast to support std::byte* -> unsigned char* conversion, which removes the need for lots of reinterpret_casts.
  5. sipa force-pushed on Apr 18, 2023
  6. sipa force-pushed on Apr 18, 2023
  7. DrahtBot added the label CI failed on Apr 19, 2023
  8. in src/pubkey.h:311 in 2ecd571c95 outdated 
    303+public:
304+    /** Construct a new ellswift public key from a given serialization. */
305+    EllSwiftPubKey(const std::array<std::byte, ELLSWIFT_ENCODED_SIZE>& ellswift) :
306+        m_pubkey(ellswift) {}
307+
308+    /** Decode to normal key (for debugging purposes). */
    instagibbs commented at 1:36 pm on April 19, 2023:
    normal compressed key
    sipa commented at 3:09 pm on April 20, 2023:
    Done.
  9. in src/key.h:167 in 2ecd571c95 outdated 
    161@@ -156,6 +162,21 @@ class CKey
162 
163     //! Load private key and check that public key matches.
164     bool Load(const CPrivKey& privkey, const CPubKey& vchPubKey, bool fSkipCheck);
165+
166+    /** Create an ellswift-encoded public key for this key, with specified entropy.
167+     *  This must be a valid private key. Ent32 must be 32 bytes. */
    instagibbs commented at 1:40 pm on April 19, 2023:
    Took me too long to figure out what “ent” referred to, just putting the full word “entropy” in the comment would help. If the entropy is not unpredictable this would result in a not uniformly random(?) series of bytes in the resulting pubkey?
    sipa commented at 2:08 pm on April 19, 2023:

    Actually, no, the “create ellswift encoding” algorithm in libsecp256k1 uses an internal RNG which is always at least seeded with the private key; the ent32 argument here is just auxiliary randomness that gets fed to that RNG.

    The output of the encoding is in fact indistinguishable from uniformly random to anyone who doesn’t know the private key already, even without additional entropy. Adding actual randomness is a more belt-and-suspenders step to make sure the result is actually close to uniformly random, rather than just indistinguishable from random.

    This deserves some extra comments, will add soon.

    sipa commented at 3:10 pm on April 20, 2023:
    Added some comments. Please have a look.
    instagibbs commented at 3:12 pm on April 20, 2023:
    significantly clearer, thanks
  10. in src/key.cpp:359 in 2ecd571c95 outdated 
    354+
355+static int BIP324ECDHHash(unsigned char* output, const unsigned char* x32, const unsigned char* ours, const unsigned char* theirs, void* data)
356+{
357+    bool initiating = *(bool*)data;
358+    HashWriter writer{HASHER_BIP324_ECDH};
359+    writer << Span{initiating ? ours : theirs, 64};
    instagibbs commented at 1:48 pm on April 19, 2023:
    s/64/EllSwiftPubKey::size()/
    sipa commented at 3:10 pm on April 20, 2023:
    Done.
  11. in src/key.cpp:360 in 2ecd571c95 outdated 
    355+static int BIP324ECDHHash(unsigned char* output, const unsigned char* x32, const unsigned char* ours, const unsigned char* theirs, void* data)
356+{
357+    bool initiating = *(bool*)data;
358+    HashWriter writer{HASHER_BIP324_ECDH};
359+    writer << Span{initiating ? ours : theirs, 64};
360+    writer << Span{initiating ? theirs : ours, 64};
    instagibbs commented at 1:48 pm on April 19, 2023:
    s/64/EllSwiftPubKey::size()/
    sipa commented at 3:10 pm on April 20, 2023:
    Done.
  12. sipa force-pushed on Apr 19, 2023
  13. DrahtBot removed the label CI failed on Apr 19, 2023
  14. in src/pubkey.h:298 in fd04a2baec outdated 
    290@@ -291,6 +291,30 @@ class XOnlyPubKey
291     SERIALIZE_METHODS(XOnlyPubKey, obj) { READWRITE(obj.m_keydata); }
292 };
293 
294+/** An ElligatorSwift-encoded public key. */
295+struct EllSwiftPubKey
296+{
297+public:
298+    static constexpr size_t ELLSWIFT_ENCODED_SIZE = 64;
    theStack commented at 0:19 am on April 20, 2023:
    tiny nit: could be put in the “private:” section, since there is a static constexpr size() function returning this value below anyway.
    sipa commented at 3:10 pm on April 20, 2023:
    Done. Also renamed to just SIZE.
  15. in src/bench/ellswift.cpp:20 in e1ddcb416a outdated 
    17+    key.MakeNewKey(true);
18+
19+    uint256 entropy = GetRandHash();
20+
21+    bench.batch(1).unit("pubkey").run([&] {
22+        auto ret = key.EllSwiftCreate(AsBytes(Span{entropy}));
    theStack commented at 0:43 am on April 20, 2023:
    There’s a theoretical possibility that this call crashes due to failed assertion assert(fValid), if the generated key from the previous round (=first 32 bytes of ellswift encoded pubkey) was not valid. Due to the practical low probably for this to ever happen (if my math isn’t off, on average every 2^(127,65) randomly generated key is invalid), I guess it’s not worth it to add any checks here, especially since it’s only a benchmark?
    sipa commented at 3:11 pm on April 20, 2023:
    Your math is correct. And indeed, it’s such a low probability event that it’s not worth adding code to deal with it (even outside of a benchmark, I’d say). Still, I’ve added an assert to make it clear to the reader this is always supposed to be the case.
  16. sipa force-pushed on Apr 20, 2023
  17. theStack commented at 2:14 pm on April 22, 2023: contributor
    Code-review ACK 818acac23295d526d9f85f0218949c817f5df964 (starting from commit fd04a2baec77b97cfb36ec488cec58811ea1ad45; didn’t review any of the secp256k1 changes introduced in https://github.com/bitcoin-core/secp256k1/pull/1129)
  18. in src/test/fuzz/key.cpp:370 in 7acde73936 outdated 
    353+    ent32.resize(32);
354+    auto k2_ellswift = k2.EllSwiftCreate(ent32);
355+
356+    bool initiating = fdp.ConsumeBool();
357+    auto ecdh_secret_1 = k1.ComputeBIP324ECDHSecret(k2_ellswift, k1_ellswift, initiating);
358+    auto ecdh_secret_2 = k2.ComputeBIP324ECDHSecret(k1_ellswift, k2_ellswift, !initiating);
    instagibbs commented at 6:03 pm on May 4, 2023:
    throwing it out there: could also have the initiating match erroneously, and assert that the secret should not match unless both privkeys/ent32s match?
    sipa commented at 9:12 pm on May 14, 2023:

    I’ve just added a test for this on the libsecp256k1 side: https://github.com/bitcoin-core/secp256k1/pull/1129/files#diff-d7678006ab19bb19a4a8dfb5f497cab0127e33730791c6e0597db203751322d9R365

    I may do something similar here when I update again.

    sipa commented at 3:44 pm on June 21, 2023:
    I’ve added more tests like this on the Bitcoin Core side.
  19. in src/pubkey.h:302 in 818acac232 outdated 
    298@@ -299,6 +299,9 @@ struct EllSwiftPubKey
299     std::array<std::byte, SIZE> m_pubkey;
300 
301 public:
302+    /** Construct an all 0x00 public key. Note that that is a valid pubkey. */
    instagibbs commented at 6:13 pm on May 4, 2023:
    where is this used?
    sipa commented at 3:44 pm on June 21, 2023:
    Apparently, not. Removed it.
  20. instagibbs commented at 6:15 pm on May 4, 2023: member
    Looking good, have not reviewed the secp PRs yet
  21. DrahtBot added the label Needs rebase on May 8, 2023
  22. sipa force-pushed on May 12, 2023
  23. DrahtBot removed the label Needs rebase on May 12, 2023
  24. fanquake commented at 8:49 am on May 12, 2023: member
    cc @stratospher
  25. Squashed 'src/secp256k1/' changes from 4258c54f4e..705ce7ed8c 
    705ce7ed8c Merge bitcoin-core/secp256k1#1129: ElligatorSwift + integrated x-only DH
0702ecb061 Merge bitcoin-core/secp256k1#1338: Drop no longer needed `#include "../include/secp256k1.h"`
90e360acc2 Add doc/ellswift.md with ElligatorSwift explanation
4f091847c2 Add ellswift testing to CI
1bcea8c57f Add benchmarks for ellswift module
2d1d41acf8 Add ctime tests for ellswift module
df633cdeba Add _prefix and _bip324 ellswift_xdh hash functions
9695deb351 Add tests for ellswift module
c47917bbd6 Add ellswift module implementing ElligatorSwift
79e5b2a8b8 Add functions to test if X coordinate is valid
a597a5a9ce Add benchmark for key generation
30574f22ea Merge bitcoin-core/secp256k1#1349: Normalize ge produced from secp256k1_pubkey_load
45c5ca7675 Merge bitcoin-core/secp256k1#1350: scalar: introduce and use `secp256k1_{read,write}_be64` helpers
f1652528be Normalize ge produced from secp256k1_pubkey_load
7067ee54b4 tests: add tests for `secp256k1_{read,write}_be64`
740528caad scalar: use newly introduced `secp256k1_{read,write}_be64` helpers (4x64 impl.)
67214f5f7d Merge bitcoin-core/secp256k1#1339: scalar: refactor: use `secp256k1_{read,write}_be32` helpers
cb1a59275c Merge bitcoin-core/secp256k1#1341: docs: correct `pubkey` param descriptions for `secp256k1_keypair_{xonly_,}pub`
f3644287b1 docs: correct `pubkey` param descriptions for `secp256k1_keypair_{xonly_,}pub`
887183e7de scalar: use `secp256k1_{read,write}_be32` helpers (4x64 impl.)
52b84238de scalar: use `secp256k1_{read,write}_be32` helpers (8x32 impl.)
e449af6872 Drop no longer needed `#include "../include/secp256k1.h"`
60556c9f49 Merge bitcoin-core/secp256k1#1337: ci: Fix error D8037 in `cl.exe` (attempt 2)
db29bf220c ci: Remove quirk that runs dummy command after wineserver
c7db4942b3 ci: Fix error D8037 in `cl.exe`
7dae115861 Revert "ci: Move wine prefix to /tmp to avoid error D8037 in cl.exe"
bf29f8d0a6 Merge bitcoin-core/secp256k1#1334: fix input range comment for `secp256k1_fe_add_int`
605e07e365 fix input range comment for `secp256k1_fe_add_int`
debf3e5c08 Merge bitcoin-core/secp256k1#1330: refactor: take use of `secp256k1_scalar_{zero,one}` constants
d75dc59b58 Merge bitcoin-core/secp256k1#1333: test: Warn if both `VERIFY` and `COVERAGE` are defined
ade5b36701 tests: add checks for scalar constants `secp256k1_scalar_{zero,one}`
e83801f5db test: Warn if both `VERIFY` and `COVERAGE` are defined
654246c635 refactor: take use of `secp256k1_scalar_{zero,one}` constants
908e02d596 Merge bitcoin-core/secp256k1#1328: build: Bump MSVC warning level up to W3
1549db0ca5 build: Level up MSVC warnings
20a5da5fb1 Merge bitcoin-core/secp256k1#1310: Refine release process
ad84603297 release process: clarify change log updates
6348bc7eee release process: fix process for maintenance release
79fa50b082 release process: mention targeted release schedule
165206789b release process: add sanity checks
09df0bfb23 Merge bitcoin-core/secp256k1#1327: ci: Move wine prefix to /tmp to avoid error D8037 in cl.exe
27504d5c94 ci: Move wine prefix to /tmp to avoid error D8037 in cl.exe
d373a7215b Merge bitcoin-core/secp256k1#1316: Do not invoke fe_is_zero on failed set_b32_limit
6433175ffe Do not invoke fe_is_zero on failed set_b32_limit
5f7903c73c Merge bitcoin-core/secp256k1#1318: build: Enable -DVERIFY for precomputation binaries
e9e4526a4e Merge bitcoin-core/secp256k1#1317: Make fe_cmov take max of magnitudes
5768b50229 build: Enable -DVERIFY for precomputation binaries
31b4bbee1e Make fe_cmov take max of magnitudes
83186db34a Merge bitcoin-core/secp256k1#1314: release cleanup: bump version after 0.3.2
95448ef2f8 release cleanup: bump version after 0.3.2
acf5c55ae6 Merge bitcoin-core/secp256k1#1312: release: Prepare for 0.3.2
d490ca2046 release: Prepare for 0.3.2
3e3d125b83 Merge bitcoin-core/secp256k1#1309: changelog: Catch up
e8295d07ab Merge bitcoin-core/secp256k1#1311: Revert "Remove unused scratch space from API"
697e1ccf4a changelog: Catch up
3ad1027a40 Revert "Remove unused scratch space from API"
76b43f3443 changelog: Add entry for #1303
7d4f86d242 Merge bitcoin-core/secp256k1#1307: Mark more assembly outputs as early clobber
b54a0672ef Merge bitcoin-core/secp256k1#1304: build: Rename arm to arm32 and check if it's really supported
c6bb29b303 build: Rename `64bit` to `x86_64`
8c9ae37a5a Add release note
03246457a8 autotools: Add `SECP_ARM32_ASM_CHECK` macro
ed4ba238e2 cmake: Add `check_arm32_assembly` function
350b4bd6e6 Mark stack variables as early clobber for technical correctness
0c729ba70d Bugfix: mark outputs as early clobber in scalar x86_64 asm
3353d3c753 Merge bitcoin-core/secp256k1#1207: Split fe_set_b32 into reducing and normalizing variants
5b32602295 Split fe_set_b32 into reducing and normalizing variants
006ddc1f42 Merge bitcoin-core/secp256k1#1306: build: Make tests work with external default callbacks
1907f0f166 build: Make tests work with external default callbacks
fb3a806365 Merge bitcoin-core/secp256k1#1133: schnorrsig: Add test vectors for variable-length messages
cd54ac7c1c schnorrsig: Improve docs of schnorrsig_sign_custom
28687b0312 schnorrsig: Add BIP340 varlen test vectors
97a98bed1e schnorrsig: Refactor test vector code to allow varlen messages
ab5a917128 Merge bitcoin-core/secp256k1#1303: ct: Use more volatile
9eb6934f69 Merge bitcoin-core/secp256k1#1305: Remove unused scratch space from API
073d98a076 Merge bitcoin-core/secp256k1#1292: refactor: Make 64-bit shift explicit
17fa21733a ct: Be cautious and use volatile trick in more "conditional" paths
5fb336f9ce ct: Use volatile trick in scalar_cond_negate
712e7f8722 Remove unused scratch space from API
54d34b6c24 Merge bitcoin-core/secp256k1#1300: Avoid normalize conditional on VERIFY
c63ec88ebf Merge bitcoin-core/secp256k1#1066: Abstract out and merge all the magnitude/normalized logic
7fc642fa25 Simplify secp256k1_fe_{impl_,}verify
4e176ad5b9 Abstract out verify logic for fe_is_square_var
4371f98346 Abstract out verify logic for fe_add_int
89e324c6b9 Abstract out verify logic for fe_half
283cd80ab4 Abstract out verify logic for fe_get_bounds
d5aa2f0358 Abstract out verify logic for fe_inv{,_var}
3167646072 Abstract out verify logic for fe_from_storage
76d31e5047 Abstract out verify logic for fe_to_storage
1e6894bdd7 Abstract out verify logic for fe_cmov
be82bd8e03 Improve comments/checks for fe_sqrt
6ab35082ef Abstract out verify logic for fe_sqr
4c25f6efbd Abstract out verify logic for fe_mul
e179e651cb Abstract out verify logic for fe_add
7e7ad7ff57 Abstract out verify logic for fe_mul_int
65d82a3445 Abstract out verify logic for fe_negate
144670893e Abstract out verify logic for fe_get_b32
f7a7666aeb Abstract out verify logic for fe_set_b32
ce4d2093e8 Abstract out verify logic for fe_cmp_var
7d7d43c6dd Improve comments/check for fe_equal{,_var}
c5e788d672 Abstract out verify logic for fe_is_odd
d3f3fe8616 Abstract out verify logic for fe_is_zero
c701d9a471 Abstract out verify logic for fe_clear
19a2bfeeea Abstract out verify logic for fe_set_int
864f9db491 Abstract out verify logic for fe_normalizes_to_zero{,_var}
6c31371120 Abstract out verify logic for fe_normalize_var
e28b51f522 Abstract out verify logic for fe_normalize_weak
b6b6f9cb97 Abstract out verify logic for fe_normalize
7fa5195559 Bugfix: correct SECP256K1_FE_CONST mag/norm fields
e5cf4bf3ff build: Rename `arm` to `arm32`
b29566c51b Merge magnitude/normalized fields, move/improve comments
97c63b9039 Avoid normalize conditional on VERIFY
341cc19726 Merge bitcoin-core/secp256k1#1299: Infinity handling: ecmult_const(infinity) works, and group verification
bbc834467c Avoid secp256k1_ge_set_gej_zinv with uninitialized z
0a2e0b2ae4 Make secp256k1_{fe,ge,gej}_verify work as no-op if non-VERIFY
f20266722a Add invariant checking to group elements
a18821d5b1 Always initialize output coordinates in secp256k1_ge_set_gej
3086cb90ac Expose secp256k1_fe_verify to other modules
a0e696fd4d Make secp256k1_ecmult_const handle infinity
24c768ae09 Merge bitcoin-core/secp256k1#1301: Avoid using bench_verify_data as bench_sign_data; merge them
2e65f1fdbc Avoid using bench_verify_data as bench_sign_data; merge them
1cf15ebd94 Merge bitcoin-core/secp256k1#1296: docs: complete interface description for `secp256k1_schnorrsig_sign_custom`
149c41cee1 docs: complete interface description for `secp256k1_schnorrsig_sign_custom`
f30c74866b Merge bitcoin-core/secp256k1#1270: cmake: Fix library ABI versioning
d1e48e5474 refactor: Make 64-bit shift explicit
b2e29e43d0 ci: Treat all compiler warnings as errors in "Windows (VS 2022)" task
3c81838856 Merge bitcoin-core/secp256k1#1289: cmake: Use full signature of `add_test()` command
755629bc03 cmake: Use full signature of `add_test()` command
bef448f9af cmake: Fix library ABI versioning
4b0f711d46 Merge bitcoin-core/secp256k1#1277: autotools: Clean up after adding Wycheproof
222ecaf661 Merge bitcoin-core/secp256k1#1284: cmake: Some improvements using `PROJECT_IS_TOP_LEVEL` variable
71f746c057 cmake: Include `include` directory for subtree builds
024a409484 Merge bitcoin-core/secp256k1#1240: cmake: Improve and document compiler flag checks
a8d059f76c cmake, doc: Document compiler flags
6ece1507cb cmake, refactor: Rename `try_add_compile_option` to `try_append_cflags`
19516ed3e9 cmake: Use `add_compile_options()` in `try_add_compile_option()`
4b84f4bf0f Merge bitcoin-core/secp256k1#1239: cmake: Bugfix and other improvements after bumping CMake up to 3.13
596b336ff6 Merge bitcoin-core/secp256k1#1234: cmake: Add dev-mode
6b7e5b717d Merge bitcoin-core/secp256k1#1275: build: Fix C4005 "macro redefinition" MSVC warnings in examples
1c89536718 Merge bitcoin-core/secp256k1#1286: tests: remove extra semicolon in macro
c4062d6b5d debug: move helper for printing buffers into util.h
7e977b3c50 autotools: Take VPATH builds into account when generating testvectors
2418d3260a autotools: Create src/wycheproof dir before creating file in it
8764034ed5 autotools: Make all "pregenerated" targets .PHONY
e1b9ce8811 autotools: Use same conventions for all pregenerated files
3858bad2c6 tests: remove extra semicolon in macro
1f33bb2b1c Merge bitcoin-core/secp256k1#1205: field: Improve docs +tests of secp256k1_fe_set_b32
162da73e9a tests: Add debug helper for printing buffers
e9fd3dff76 field: Improve docs and tests of secp256k1_fe_set_b32
f6bef03c0a Merge bitcoin-core/secp256k1#1283: Get rid of secp256k1_fe_const_b
5431b9decd cmake: Make `SECP256K1_INSTALL` default depend on `PROJECT_IS_TOP_LEVEL`
5ec1333d4f Merge bitcoin-core/secp256k1#1285: bench: Make sys/time.h a system include
68b16a1662 bench: Make sys/time.h a system include
162608cc98 cmake: Emulate `PROJECT_IS_TOP_LEVEL` for CMake<3.21
69e1ec0331 Get rid of secp256k1_fe_const_b
ce5ba9e24d gitignore: Add CMakeUserPresets.json
0a446a312f cmake: Add dev-mode CMake preset
a6f4bcf6e1 Merge bitcoin-core/secp256k1#1231: Move `SECP256K1_INLINE` macro definition out from `include/secp256k1.h`
a273d74b2e cmake: Improve version comparison
6a58b483ef cmake: Use `if(... IN_LIST ...)` command
2445808c02 cmake: Use dedicated `GENERATOR_IS_MULTI_CONFIG` property
9f8703ef17 cmake: Use dedicated `CMAKE_HOST_APPLE` variable
8c2017035a cmake: Use recommended `add_compile_definitions` command
04d4cc071a cmake: Add `DESCRIPTION` and `HOMEPAGE_URL` options to `project` command
8a8b6536ef cmake: Use `SameMinorVersion` compatibility mode
5b0444a3b5 Merge bitcoin-core/secp256k1#1263: cmake: Make installation optional
47ac3d63cd cmake: Make installation optional
2e035af251 Merge bitcoin-core/secp256k1#1273: build: Make `SECP_VALGRIND_CHECK` preserve `CPPFLAGS`
5be353d658 Merge bitcoin-core/secp256k1#1279: tests: lint wycheproof's python script
08f4b1632d autotools: Move code around to tidy Makefile
04bf3f6778 Merge bitcoin-core/secp256k1#1230: Build: allow static or shared but not both
9ce9984f32 Merge bitcoin-core/secp256k1#1265: Remove bits argument from secp256k1_wnaf_const{_xonly}
566faa17d3 Merge bitcoin-core/secp256k1#1267: doc: clarify process for patch releases
ef49a11d29 build: allow static or shared but not both
35ada3b954 tests: lint wycheproof's python script
529b54d922 autotools: Move Wycheproof header from EXTRA_DIST to noinst_HEADERS
dc0657c762 build: Fix C4005 "macro redefinition" MSVC warnings in examples
1ecb94ebe9 build: Make `SECP_VALGRIND_CHECK` preserve `CPPFLAGS`
1b6fb5593c doc: clarify process for patch releases
a575339c02 Remove bits argument from secp256k1_wnaf_const (always 256)
36b0adf1b9 build: remove warning until it's reproducible
8e142ca410 Move `SECP256K1_INLINE` macro definition out from `include/secp256k1.h`
77445898a5 Remove `SECP256K1_INLINE` usage from examples
ca92a35d01 field: Simplify code in secp256k1_fe_set_b32
d93f62e369 field: Verify field element even after secp256k1_fe_set_b32 fails

git-subtree-dir: src/secp256k1
git-subtree-split: 705ce7ed8c1557a31e1bfc99be06082c5098d9f5
    901336eee7
  26. Update src/secp256k1 subtree to version with ElligatorSwift support a143a12d44
  27. sipa force-pushed on Jun 21, 2023
  28. sipa commented at 3:45 pm on June 21, 2023: member
    Rebased, updated to use the now-merged bitcoin-core/secp256k1#1129, and addressed some comments.
  29. in src/bench/bip324_ecdh.cpp:35 in ff58a4a4a3 outdated 
    30+        EllSwiftPubKey our_ellswift(our_ellswift_data);
31+        EllSwiftPubKey their_ellswift(their_ellswift_data);
32+
33+        auto ret = key.ComputeBIP324ECDHSecret(their_ellswift, our_ellswift, true);
34+
35+        // Copy 8 bytes from the resulting shared secret into middle of the private key.
    instagibbs commented at 7:54 pm on June 21, 2023:
    is there an importance to the 8/12 bytes chosen here?
    sipa commented at 9:00 pm on June 21, 2023:

    Not really, the reasoning is:

    • The copying is done so that not all inputs are always the same.
    • The copying into the middle is so that for ellswift encodings both the left and right 32 bytes are affected. For the private key, the place doesn’t matter.
    • The 8/12/12 split gives a bit more entropy to the ellswift encodings as they are bigger. Overall it gives 64 bits to the private keys, and 48 bits to each of the four 256-bit components of the ellswift encodings.
    sipa commented at 6:26 pm on June 23, 2023:
    I’ve changed it to 8/8/16, and added rationale why in a comment.
  30. instagibbs approved
  31. instagibbs commented at 8:16 pm on June 21, 2023: member

    code review ACK ff58a4a4a3930ecb913eee8a42df2756b916b858

    ran fuzzing targets for a bit as sanity check, no issues

    I’m assuming CI is doing some sensible checks on the subtree update, confirmed [705ce7ed8c](https://github.com/bitcoin/bitcoin/pull/27479/commits/901336eee751de088465e313dd8b500dfaf462b2) points to a commit in secp master with the secp PR merged.

  32. DrahtBot requested review from theStack on Jun 21, 2023
  33. in src/key.cpp:363 in 015dbbc7f8 outdated 
    356+    bool success = secp256k1_ellswift_xdh(secp256k1_context_sign,
357+                                          UCharCast(output.data()),
358+                                          UCharCast(initiating ? our_ellswift.data() : their_ellswift.data()),
359+                                          UCharCast(initiating ? their_ellswift.data() : our_ellswift.data()),
360+                                          keydata.data(),
361+                                          initiating ? 0 : 1,
    achow101 commented at 8:22 pm on June 21, 2023:

    In bea8a4f737540ef72e795bcc4ca60b249e8c57ca “Add ElligatorSwift key creation and ECDH logic”

    How come these need to be swapped depending on initiating? secp256k1_ellswift_xdh’s documentation does not indicate that the order of pubkeys matters, just that we correctly indicate which is ours.

    sipa commented at 8:32 pm on June 21, 2023:

    It actually does (https://github.com/bitcoin-core/secp256k1/blob/705ce7ed8c1557a31e1bfc99be06082c5098d9f5/include/secp256k1_ellswift.h#L173):

    0 *           party:     boolean indicating which party we are: zero if we are
1 *                      party A, non-zero if we are party B. seckey32 must be
2 *                      the private key corresponding to that party's ell_?64.
3 *                      This correspondence is not checked.
    sipa commented at 6:27 pm on June 23, 2023:
    I’ve added a comment.
    achow101 commented at 8:38 pm on June 26, 2023:
    To clarify, the order matters because the ECDH is more than just plain ECDH - it hashes the ECDH secret with the pubkeys, and so ordering in the hashing serialization matters. BIP 324 specifies the preimage of this hash as initiator pub || receiver pub || ecdh secret, so we do this swapping so that we are computing this order correctly.
  34. achow101 commented at 9:15 pm on June 21, 2023: member

    Code Review ACK ff58a4a4a3930ecb913eee8a42df2756b916b858

    Ran the fuzz tests for a while and didn’t see any errors. The code itself looks good and is small and self contained. Also verified that the updated subtree matches.

  35. theStack commented at 9:39 pm on June 21, 2023: contributor
    Considering that the ellswift module is built by default in libsecp256k1, commit 665e60e5dbecb802b14253dfcc691d91750332a3 could be dropped? (Explicit enabling for other built-by-default modules have been removed in commit 3bfca788b0dae879bfc745cc52c2cb6edc49fd70 / PR #26691).
  36. DrahtBot requested review from theStack on Jun 21, 2023
  37. in src/span.h:277 in ff58a4a4a3 outdated 
    273@@ -274,6 +274,7 @@ Span<std::byte> MakeWritableByteSpan(V&& v) noexcept
274 // Helper functions to safely cast to unsigned char pointers.
275 inline unsigned char* UCharCast(char* c) { return (unsigned char*)c; }
276 inline unsigned char* UCharCast(unsigned char* c) { return c; }
277+inline unsigned char* UCharCast(std::byte* c) { return (unsigned char*)c; }
    maflcko commented at 6:13 am on June 22, 2023:

    style-nit: Obviously doesn’t matter here, but could use reinterpret_cast to guard against accidentally removing constness. (And for symmetry with UCharCast(const std::byte*))

    0inline unsigned char* UCharCast(std::byte* c) { return reinterpret_cast<unsigned char*>(c); }
  38. in src/bench/ellswift.cpp:10 in 55ce092324 outdated 
     5+#include <bench/bench.h>
 6+
 7+#include <key.h>
 8+#include <random.h>
 9+
10+#include <array>
    theStack commented at 12:35 pm on June 22, 2023:

    nit: unneeded include (but it’s missing in src/bench/bip324_ecdh.cpp)

    sipa commented at 6:26 pm on June 23, 2023:
    Fixed both.
  39. theStack approved
  40. theStack commented at 12:36 pm on June 22, 2023: contributor
    Code-review ACK ff58a4a4a3930ecb913eee8a42df2756b916b858 (https://github.com/bitcoin/bitcoin/pull/27479#pullrequestreview-1491952027 is not a blocker and can be done as a follow-up as well)
  41. Enable ellswift module in libsecp256k1 42239f8390
  42. Add ElligatorSwift key creation and ECDH logic 
    Co-authored-by: Dhruv Mehta <856960+dhruv@users.noreply.github.com>
    eff72a0dff
  43. Unit test for ellswift creation/decoding roundtrip 
    Co-authored-by: Pieter Wuille <bitcoin-dev@wuille.net>
    aae432a764
  44. Fuzz test for CKey->EllSwift->CPubKey creation/decoding 
    Co-authored-by: Pieter Wuille <bitcoin-dev@wuille.net>
    c3ac9f5cf4
  45. Fuzz test for Ellswift ECDH 
    Co-authored-by: Pieter Wuille <bitcoin-dev@wuille.net>
    2e5a8a437c
  46. Bench tests for CKey->EllSwift 
    Co-Authored-By: Pieter Wuille <bitcoin-dev@wuille.net>
    42d759f239
  47. Bench test for EllSwift ECDH 
    Co-authored-by: Dhruv Mehta <856960+dhruv@users.noreply.github.com>
    3168b08043
  48. in src/bench/ellswift.cpp:20 in ff58a4a4a3 outdated 
    17+    key.MakeNewKey(true);
18+
19+    uint256 entropy = GetRandHash();
20+
21+    bench.batch(1).unit("pubkey").run([&] {
22+        auto ret = key.EllSwiftCreate(AsBytes(Span{entropy}));
    maflcko commented at 1:37 pm on June 22, 2023: 
    0        auto ret = key.EllSwiftCreate(MakeByteSpan(entropy));

    style nit: Can use the alias which does the same.

  49. in src/test/key_tests.cpp:354 in ff58a4a4a3 outdated 
    349+    for (const auto& secret : {strSecret1, strSecret2, strSecret1C, strSecret2C}) {
350+        CKey key = DecodeSecret(secret);
351+        BOOST_CHECK(key.IsValid());
352+
353+        uint256 ent32 = InsecureRand256();
354+        auto ellswift = key.EllSwiftCreate(AsBytes(Span{ent32}));
    maflcko commented at 1:39 pm on June 22, 2023: 
    0        auto ellswift = key.EllSwiftCreate(MakeByteSpan(ent32));
  50. sipa force-pushed on Jun 23, 2023
  51. sipa commented at 6:27 pm on June 23, 2023: member
    @theStack I’ve dropped the --enable-module-ellswift, but I believe the MSVC build change remains necessary (as it isn’t influenced by the ./configure defaults).
  53. DrahtBot requested review from achow101 on Jun 23, 2023
  54. DrahtBot requested review from theStack on Jun 23, 2023
  55. theStack approved
  56. theStack commented at 9:43 pm on June 23, 2023: contributor
    re-ACK 3168b08043546cd248a81563e21ff096019f1521
  57. maflcko commented at 8:42 am on June 26, 2023: member
    left some style nits, feel free to ignore, unless you retouch for other reasons.
  58. achow101 commented at 8:48 pm on June 26, 2023: member
    ACK 3168b08043546cd248a81563e21ff096019f1521
  59. DrahtBot removed review request from achow101 on Jun 26, 2023
  60. achow101 merged this on Jun 26, 2023
  61. achow101 closed this on Jun 26, 2023
  62. sidhujag referenced this in commit 86a3a6bd8a on Jun 27, 2023
  63. achow101 referenced this in commit 626d346469 on Jun 28, 2023
  64. sidhujag referenced this in commit b54fe90f36 on Jun 30, 2023
  65. fanquake referenced this in commit 8c7e735456 on Sep 5, 2023
  66. Frank-GER referenced this in commit 66f3ee38a0 on Sep 8, 2023
  67. kwvg referenced this in commit c77cab2cf2 on Sep 9, 2023
  68. kwvg referenced this in commit 6effc626cc on Sep 9, 2023
  69. kwvg referenced this in commit e9bbd48308 on Sep 24, 2023
  70. kwvg referenced this in commit 5a608b0586 on Sep 28, 2023
  71. kwvg referenced this in commit 3cab540a2a on Sep 28, 2023
  72. ogabrielides referenced this in commit d40ab43171 on Nov 15, 2023
  73. UdjinM6 referenced this in commit a29538887c on Nov 15, 2023
  74. UdjinM6 referenced this in commit 1a122a043a on Nov 16, 2023
  75. UdjinM6 referenced this in commit 2fbbe12ff2 on Nov 20, 2023
  76. PastaPastaPasta referenced this in commit 78a9f1e55b on Nov 21, 2023
  77. PastaPastaPasta referenced this in commit 39ce20cddc on Nov 21, 2023
  78. bitcoin locked this on Jun 25, 2024


sipa DrahtBot instagibbs theStack fanquake achow101 maflcko

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-06-26 13:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me