Implement IsEqual for SchnorrPublicKey and SchnorrSignature #3

elichai · 2020-03-25T13:16:00Z

So kaspad does use IsEqual on pubkeys and signatures,
mostly in https://github.com/kaspanet/kaspad/blob/master/txscript/sigcache.go#L63 but in other places too.

I also checked against https://godoc.org/github.com/kaspanet/kaspad/ecc to make sure there are no other types that might implement it

Also added more tests for Hash.IsEqual too

secp256k1_test.go

stasatdaglabs · 2020-03-25T13:27:32Z

secp256k1_test.go

+	serializedSig := SerializedSchnorrSignature{}
+	n, err := r.Read(serializedSig[:])
+	if err != nil || n != len(serializedSig) {
+		t.Errorf("Failed generating a random signature '%d' '%s' ", n, err)


Please annotate what n is in the error and remove the whitespace at the end of the line.

I called it n because that's what the API calls it.
how should I call it? bytesRead/byteCount ?

If you meant in the string, then done.

secp256k1_test.go

stasatdaglabs · 2020-03-25T13:28:30Z

secp256k1_test.go

+	serializedSig2 := SerializedSchnorrSignature{}
+	n, err = r.Read(serializedSig2[:])
+	if err != nil || n != len(serializedSig2) {
+		t.Errorf("Failed generating a random signature '%d' '%s' ", n, err)


Please annotate what n is in the error and remove the whitespace at the end of the line.

Same as above

stasatdaglabs · 2020-03-25T13:28:48Z

secp256k1_test.go

+	goodHash := &Hash{}
+	n, err := r.Read(goodHash[:])
+	if err != nil || n != len(goodHash) {
+		t.Errorf("Failed generating a random hash '%d' '%s' ", n, err)


Please annotate what n is in the error and remove the whitespace at the end of the line.

Same as above

secp256k1_test.go

stasatdaglabs · 2020-03-25T13:29:15Z

secp256k1_test.go

+	goodHash2 := &Hash{}
+	n, err = r.Read(goodHash2[:])
+	if err != nil || n != len(goodHash2) {
+		t.Errorf("Failed generating a random hash '%d' '%s' ", n, err)


Please annotate what n is in the error and remove the whitespace at the end of the line.

Same as above

d66ad94 Merge pull request #3 from kaspanet/new-schnorr 2a29b5c Merge remote-tracking branch 'upstream/master' into new-schnorr 3a10696 Merge #849: Convert Sage code to Python 3 (as used by Sage >= 9) 13c88ef Convert Sage code to Python 3 (as used by Sage >= 9) 9e5939d Merge #835: Don't use reserved identifiers memczero and benchmark_verify_t f09320e Revert "Add matching Schnorr implementation " d0a83f7 Merge #839: Prevent arithmetic on NULL pointer if the scratch space is too small 903b16a Merge #840: Return NULL early in context_preallocated_create if flags invalid 1f4dd03 Typedef (u)int128_t only when they're not provided by the compiler 3967d96 Merge #838: Make autotools check for all the used openssl functions 3734b68 Configure echo if openssl tests are enabled ebfa205 Return NULL early in context_preallocated_create if flags invalid 6f54e69 Merge #841: Avoids a potentially shortening size_t to int cast in strauss_wnaf_ 29a299e Run the undefined behaviour sanitizer on Travis 7506e06 Prevent arithmetic on NULL pointer if the scratch space is too small 8893f42 Avoids a potentially shortening size_t to int cast in strauss_wnaf_ e669277 Modify bitcoin_secp.m4's openssl check to call all the functions that we use in the tests/benchmarks. That way linking will fail if those symbols are missing ac05f61 Merge #809: Stop treating ECDH as experimental e6e3d5d travis: add schnorrsig to valgrind and big endian platform test 353dff1 Stop treating ECDH as experimental e89278f Don't use reserved identifiers memczero and benchmark_verify_t c6b6b8f Merge #830: Rip out non-endomorphism code + dependencies c582aba Consistency improvements to the comments 63c6b71 Reorder comments/function around scalar_split_lambda 2edc514 WNAF of lambda_split output has max size 129 4232e5b Rip out non-endomorphism code ebad841 Check correctness of lambda split without -DVERIFY fe7fc1f Make lambda constant accessible 9d2f2b4 Add tests to exercise lambda split near bounds 9aca2f7 Add secp256k1_split_lambda_verify acab934 Detailed comments for secp256k1_scalar_split_lambda 76ed922 Increase precision of g1 and g2 6173839 Switch to our own memcmp function 63150ab Merge #827: Rename testrand functions to have test in name c5257ae Merge #821: travis: Explicitly set --with-valgrind bb1f542 Merge #818: Add static assertion that uint32_t is unsigned int or wider a45c1fa Rename testrand functions to have test in name 5006895 Merge #808: Exhaustive test improvements + exhaustive schnorrsig tests 4eecb4d travis: VALGRIND->RUN_VALGRIND to avoid confusion with WITH_VALGRIND 66a765c travis: Explicitly set --with-valgrind d7838ba Merge #813: Enable configuring Valgrind support 7ceb0b7 Merge #819: Enable -Wundef warning 8b7dcdd Add exhaustive test for extrakeys and schnorrsig 08d7d89 Make pubkey parsing test whether points are in the correct subgroup 87af00b Abstract out challenge computation in schnorrsig 63e1b2a Disable output buffering in tests_exhaustive.c 39f67dd Support splitting exhaustive tests across cores e99b26f Give exhaustive_tests count and seed cmdline inputs 49e6630 refactor: move RNG seeding to testrand b110c10 Change exhaustive test groups so they have a point with X=1 cec7b18 Select exhaustive lambda in function of order 78f6cdf Make the curve B constant a secp256k1_fe d7f39ae Delete gej_is_valid_var: unused outside tests 8bcd78c Make secp256k1_scalar_b32 detect overflow in scalar_low c498366 Move exhaustive tests for recovery to module be31791 Make group order purely compile-time in exhaustive tests e73ff30 Enable -Wundef warning c0041b5 Add static assertion that uint32_t is unsigned int or wider 4ad408f Merge #782: Check if variable=yes instead of if var is set in travis.sh 412bf87 configure: Allow specifying --with[out]-valgrind explicitly 34debf7 Modify .travis.yml to explictly pass no in env vars instead of setting to nothing a0e99fc Merge #814: tests: Initialize random group elements fully 5738e86 tests: Initialize random group elements fully c9939ba Merge #812: travis: run bench_schnorrsig a51f2af travis: run bench_schnorrsig 8ab24e8 Merge #558: Add schnorrsig module which implements BIP-340 compliant signatures f3733c5 Merge #797: Fix Jacobi benchmarks and other benchmark improvements cb5524a Add benchmark for secp256k1_ge_set_gej_var 5c6af60 Make jacobi benchmarks vary inputs d0fdd5f Randomize the Z coordinates in bench_internal c7a3424 Rename bench_internal variables 875d68b Merge #699: Initialize field elements when resulting in infinity 54caf2e Merge #799: Add fallback LE/BE for architectures with known endianness + SHA256 selftest f431b3f valgrind_ctime_test: Add schnorrsig_sign 16ffa9d schnorrsig: Add taproot test case 8dfd53e schnorrsig: Add benchmark for sign and verify 4e43520 schnorrsig: Add BIP-340 compatible signing and verification 7332d2d schnorrsig: Add BIP-340 nonce function 7a703fd schnorrsig: Init empty experimental module eabd9bc Allow initializing tagged sha256 6fcb5b8 extrakeys: Add keypair_xonly_tweak_add 5825446 extrakeys: Add keypair struct with create, pub and pub_xonly f001034 Separate helper functions for pubkey_create and seckey_tweak_add 910d9c2 extrakeys: Add xonly_pubkey_tweak_add & xonly_pubkey_tweak_add_test 176bfb1 Separate helper function for ec_pubkey_tweak_add 4cd2ee4 extrakeys: Add xonly_pubkey with serialize, parse and from_pubkey f49c989 Merge #806: Trivial: Add test logs to gitignore aabf00c Merge #648: Prevent ints from wrapping around in scratch space functions f5adab1 Merge #805: Remove the extremely outdated TODO file. bceefd6 Add test logs to gitignore 1c32519 Remove the extremely outdated TODO file. 47e6618 extrakeys: Init empty experimental module 3e08b02 Make the secp256k1_declassify argument constant 8bc6aef Add SHA256 selftest 670cdd3 Merge #798: Check assumptions on integer implementation at compile time 5e5fb28 Use additional system macros to figure out endianness 7c06899 Compile-time check assumptions on integer types 02b6c87 Add support for (signed) __int128 979961c Merge #787: Use preprocessor macros instead of autoconf to detect endianness 887bd1f Merge #793: Make scalar/field choice depend on C-detected __int128 availability 0dccf98 Use preprocessor macros instead of autoconf to detect endianness b2c8c42 Merge #795: Avoid linking libcrypto in the valgrind ct test. 57d3a3c Avoid linking libcrypto in the valgrind ct test. 79f1f7a Autodetect __int128 availability on the C side 0d7727f Add SECP256K1_FE_STORAGE_CONST_GET to 5x52 field 805082d Merge #696: Run a Travis test on s390x (big endian) 3929536 Test travis s390x (big endian) ef37761 Change travis.sh to check if variables are equal to yes instead of not-empty. Before this, setting `VALGRIND=wat` was considered as true, and to make it evaluate as false you had to unset the variable `VALGRIND=` but not it checks if `VALGRIND=yes` and if it's not `yes` then it's evaluated to false 6034a04 Merge #778: secp256k1_gej_double_nonzero supports infinity f609159 Merge #779: travis: Fix argument quoting for ./configure 9e49a9b travis: Fix argument quoting for ./configure 18d3632 secp256k1_gej_double_nonzero supports infinity 214cb3c Merge #772: Improve constant-timeness on PowerPC 40412b1 Merge #774: tests: Abort if malloc() fails during context cloning tests 2e1b9e0 tests: Abort if malloc() fails during context cloning tests 67a429f Suppress a harmless variable-time optimization by clang in _int_cmov 5b19633 Remove redundant "? 1 : 0" after comparisons in scalar code 3e5cfc5 Merge #741: Remove unnecessary sign variable from wnaf_const 66bb932 Merge #773: Fix some compile problems on weird/old compilers. 1309c03 Fix some compile problems on weird/old compilers. 2309c7d Merge #769: Undef HAVE___INT128 in basic-config.h to fix gen_context compilation 22e578b Undef HAVE___INT128 in basic-config.h to fix gen_context compilation 3f4a5a1 Merge #765: remove dead store in ecdsa_signature_parse_der_lax f00d657 remove dead store in ecdsa_signature_parse_der_lax dbd41db Merge #759: Fix uninitialized variables in ecmult_multi test 2e7fc5b Fix uninitialized variables in ecmult_multi test 2ed54da Merge #755: Recovery signing: add to constant time test, and eliminate non ct operators 2860950 Add tests for the cmov implementations 73596a8 Add ecdsa_sign_recoverable to the ctime tests 2876af4 Split ecdsa_sign logic into a new function and use it from ecdsa_sign and recovery 5e1c885 Merge #754: Fix uninit values passed into cmov f79a7ad Add valgrind uninit check to cmovs output 05d315a Merge #752: autoconf: Use ":" instead of "dnl" as a noop a39c2b0 Fixed UB(arithmetics on uninit values) in cmovs 3a6fd7f Merge #750: Add macOS to the CI 5e8747a autoconf: Use ":" instead of "dnl" as a noop 71757da Explictly pass SECP256K1_BENCH_ITERS to the benchmarks in travis.sh 99bd661 Replace travis_wait with a loop printing "\a" to stdout every minute bc818b1 Bump travis Ubuntu from xenial(16.04) to bionic(18.04) 0c5ff90 Add macOS support to travis b6807d9 Move travis script into a standalone sh file f39f99b Merge #701: Make ec_ arithmetic more consistent and add documentation 37dba32 Remove unnecessary sign variable from wnaf_const 6bb0b77 Fix test_constant_wnaf for -1 and add a test for it. 39198a0 Merge #732: Retry if r is zero during signing 59a8de8 Merge #742: Fix typo in ecmult_const_impl.h 4e28465 Fix typo in ecmult_const_impl.h f862b4c Merge #740: Make recovery/main_impl.h non-executable ffef45c Make recovery/main_impl.h non-executable 2361b37 Merge #735: build: fix OpenSSL EC detection on macOS 3b7d26b build: add SECP_TEST_INCLUDES to bench_verify CPPFLAGS 84b5fc5 build: fix OpenSSL EC detection on macOS 37ed51a Make ecdsa_sig_sign constant-time again after reverting 25e3cfb 93d343b Revert "ecdsa_impl: replace scalar if-checks with VERIFY_CHECKs in ecdsa_sig_sign" 7e3952a Clarify documentation of tweak functions. 89853a0 Make tweak function documentation more consistent. 41fc785 Make ec_privkey functions aliases for ec_seckey_negate, ec_seckey_tweak_add and ec_seckey_mul 22911ee Rename private key to secret key in public API (with the exception of function names) 5a73f14 Mention that value is unspecified for In/Out parameters if the function returns 0 f03df0e Define valid ECDSA keys in the documentation of seckey_verify 5894e1f Return 0 if the given seckey is invalid in privkey_negate, privkey_tweak_add and privkey_tweak_mul 8f814cd Add test for boundary conditions of scalar_set_b32 with respect to overflows 3fec982 Use scalar_set_b32_seckey in ecdsa_sign, pubkey_create and seckey_verify 9ab2cbe Add scalar_set_b32_seckey which does the same as scalar_set_b32 and also returns whether it's a valid secret key 4f27e34 Merge #728: Suppress a harmless variable-time optimization by clang in memczero 0199387 Add test for memczero() 52a0351 Suppress a harmless variable-time optimization by clang in memczero 8f78e20 Merge #722: Context isn't freed in the ECDH benchmark ed1b911 Merge #700: Allow overriding default flags 85b35af Add running benchmarks regularly and under valgrind in travis ca4906b Pass num of iters to benchmarks as variable, and define envvar 02dd5f1 free the ctx at the end of bench_ecdh e9fccd4 Merge #708: Constant-time behaviour test using valgrind memtest. 08fb6c4 Run valgrind_ctime_test in travis 3d23022 Constant-time behaviour test using valgrind memtest. 96d8ccb Merge #710: Eliminate harmless non-constant time operations on secret data. 0585b8b Merge #718: Clarify that a secp256k1_ecdh_hash_function must return 0 or 1 7b50483 Adds a declassify operation to aid constant-time analysis. 34a67c7 Eliminate harmless non-constant time operations on secret data. ca739cb Compile with optimization flag -O2 by default instead of -O3 eb45ef3 Clarify that a secp256k1_ecdh_hash_function must return 0 or 1 83fb1bc Remove -O2 from default CFLAGS because this would override the -O3 flag (see AC_PROG_CC in the Autoconf manual) ecba813 Append instead of Prepend user-CFLAGS to default CFLAGS allowing the user to override default variables 613c34c Remove test in configure.ac because it doesn't have an effect 47a7b83 Clear field elements when writing infinity 61d1ecb Added test with additions resulting in infinity 60f7f2d Don't assume that ALIGNMENT > 1 in tests ada6361 Use ROUND_TO_ALIGN in scratch_create 8ecc6ce Add check preventing rounding to alignment from wrapping around in scratch_alloc 4edaf06 Add check preventing integer multiplication wrapping around in scratch_max_allocation git-subtree-dir: depend/secp256k1 git-subtree-split: d66ad94e2788f960ce2b4bdeaaafbd5cf776a506

26de4dfe Merge #831: Safegcd inverses, drop Jacobi symbols, remove libgmp 24ad04fc Make scalar_inverse{,_var} benchmark scale with SECP256K1_BENCH_ITERS ebc1af70 Optimization: track f,g limb count and pass to new variable-time update_fg_var b306935a Optimization: use formulas instead of lookup tables for cancelling g bits 9164a1b6 Optimization: special-case zero modulus limbs in modinv64 1f233b3f Remove num/gmp support 20448b8d Remove unused Jacobi symbol support 5437e7bd Remove unused scalar_sqr aa9cc521 Improve field/scalar inverse tests 1e0e885c Make field/scalar code use the new modinv modules for inverses 436281af Move secp256k1_fe_inverse{_var} to per-impl files aa404d53 Move secp256k1_scalar_{inverse{_var},is_even} to per-impl files 08d54964 Improve bounds checks in modinv modules 151aac00 Add tests for modinv modules d8a92fcc Add extensive comments on the safegcd algorithm and implementation 8e415acb Add safegcd based modular inverse modules de0a643c Add secp256k1_ctz{32,64}_var functions REVERT: 9fd06254 Merge pull request #5 from bitcoin-core/master REVERT: 143ecc6f Fix multiset benchmarks REVERT: d66ad94e Merge pull request #3 from kaspanet/new-schnorr REVERT: 2a29b5c6 Merge remote-tracking branch 'upstream/master' into new-schnorr REVERT: f09320ed Revert "Add matching Schnorr implementation " REVERT: ee3ab072 Add matching Schnorr implementation REVERT: 087f4bba Fix missing MIT license attribute in oldschnorr impl bitcoin-core/secp256k1#425 REVERT: 054ade68 Add multiset and oldschnorr to travis REVERT: 2b47e2cf Rename schnorr to oldschnorr and add fixes to build system REVERT: 604052f4 [secp256k1] Implement Schnorr signatures REVERT: 1f46fcb2 Add Multiset/ECMH implementation REVERT: 225587b4 Fix UB(violating alignment rules) in multiset tests REVERT: b0e16b52 Fix docs and small code problems in multiset REVERT: 582b1256 Add multiset serialize/parse functions REVERT: d6dd4762 Fix some build configurations for multiset module REVERT: 41145690 Add ECMH multiset module to libsecp256k1 git-subtree-dir: depend/secp256k1 git-subtree-split: 26de4dfeb1f1436dae1fcf17f57bdaa43540f940

Implement IsEqual for SchnorrPublicKey and SchnorrSignature

b24a64f

stasatdaglabs requested changes Mar 25, 2020

View reviewed changes

elichai added 2 commits March 25, 2020 15:39

Add tests for IsEqual

856b5c2

fixed wrong comment, and formatted some error msgs

fd657dd

elichai force-pushed the 2020-03-isequal branch from a35c826 to fd657dd Compare March 25, 2020 13:39

svarogg approved these changes Mar 25, 2020

View reviewed changes

stasatdaglabs approved these changes Mar 25, 2020

View reviewed changes

stasatdaglabs merged commit 0b1b6dd into master Mar 25, 2020

stasatdaglabs deleted the 2020-03-isequal branch March 25, 2020 13:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement IsEqual for SchnorrPublicKey and SchnorrSignature #3

Implement IsEqual for SchnorrPublicKey and SchnorrSignature #3

elichai commented Mar 25, 2020 •

edited

Loading

stasatdaglabs Mar 25, 2020

elichai Mar 25, 2020

elichai Mar 25, 2020

stasatdaglabs Mar 25, 2020

elichai Mar 25, 2020

stasatdaglabs Mar 25, 2020

elichai Mar 25, 2020

stasatdaglabs Mar 25, 2020

elichai Mar 25, 2020

Implement IsEqual for SchnorrPublicKey and SchnorrSignature #3

Implement IsEqual for SchnorrPublicKey and SchnorrSignature #3

Conversation

elichai commented Mar 25, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elichai commented Mar 25, 2020 •

edited

Loading