secp256k1: Test consistency cleanup and rework. #2887
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
davecgh
added
the
test coverage
davecgh
commented
Feb 28, 2022
davecgh
force-pushed
the
secp256k1_test_cleanup
branch
3 times, most recently
from
fab661f
to
70d58a0
Compare
rstaudt2
approved these changes
Mar 4, 2022
davecgh
force-pushed
the
secp256k1_test_cleanup
branch
from
70d58a0
to
2f0b47e
Compare
dajohi
approved these changes
Mar 5, 2022
davecgh
force-pushed
the
secp256k1_test_cleanup
branch
from
2f0b47e
to
65bc85a
Compare
JoeGruffins
approved these changes
Mar 10, 2022
There was a problem hiding this comment.
$ go test -bench=.
goos: linux
goarch: amd64
pkg: github.com/decred/dcrd/dcrec/secp256k1/v4
cpu: AMD Ryzen 9 3900XT 12-Core Processor
BenchmarkAddNonConst-24 3585380 306.6 ns/op 0 B/op 0 allocs/op
BenchmarkAddNonConstNotZOne-24 1452766 796.9 ns/op 0 B/op 0 allocs/op
BenchmarkScalarBaseMultNonConst-24 58899 17878 ns/op 0 B/op 0 allocs/op
BenchmarkScalarMultNonConst-24 12162 98178 ns/op 720 B/op 11 allocs/op
BenchmarkNAF-24 17839897 63.77 ns/op 0 B/op 0 allocs/op
BenchmarkPubKeyDecompress-24 127308 9277 ns/op 0 B/op 0 allocs/op
BenchmarkParsePubKeyCompressed-24 126291 9406 ns/op 80 B/op 1 allocs/op
BenchmarkParsePubKeyUncompressed-24 3375386 361.2 ns/op 80 B/op 1 allocs/op
BenchmarkScalarBaseMultAdaptor-24 40498 28251 ns/op 224 B/op 5 allocs/op
BenchmarkScalarBaseMultLargeAdaptor-24 37504 30458 ns/op 400 B/op 8 allocs/op
BenchmarkScalarMultAdaptor-24 10488 110865 ns/op 1009 B/op 18 allocs/op
BenchmarkFieldNormalize-24 60825061 16.67 ns/op 0 B/op 0 allocs/op
BenchmarkFieldSqrt-24 130118 8961 ns/op 0 B/op 0 allocs/op
BenchmarkBigSqrt-24 49902 22461 ns/op 1017 B/op 91 allocs/op
BenchmarkFieldIsGtOrEqPrimeMinusOrder-24 189939106 6.072 ns/op 0 B/op 0 allocs/op
BenchmarkBigIsGtOrEqPrimeMinusOrder-24 9318628 146.0 ns/op 96 B/op 2 allocs/op
BenchmarkBigIntModN-24 3418603 376.6 ns/op 72 B/op 2 allocs/op
BenchmarkModNScalar-24 107776508 10.58 ns/op 0 B/op 0 allocs/op
BenchmarkBigIntZero-24 485790676 2.102 ns/op 0 B/op 0 allocs/op
BenchmarkModNScalarZero-24 1000000000 0.4864 ns/op 0 B/op 0 allocs/op
BenchmarkBigIntIsZero-24 1000000000 0.2550 ns/op 0 B/op 0 allocs/op
BenchmarkModNScalarIsZero-24 1000000000 0.4916 ns/op 0 B/op 0 allocs/op
BenchmarkBigIntEquals-24 214368002 5.438 ns/op 0 B/op 0 allocs/op
BenchmarkModNScalarEquals-24 450851756 2.337 ns/op 0 B/op 0 allocs/op
BenchmarkBigIntAddModN-24 2934122 428.4 ns/op 128 B/op 2 allocs/op
BenchmarkModNScalarAdd-24 89115290 13.18 ns/op 0 B/op 0 allocs/op
BenchmarkBigIntMulModN-24 1959603 619.4 ns/op 176 B/op 2 allocs/op
BenchmarkModNScalarMul-24 5727213 189.8 ns/op 0 B/op 0 allocs/op
BenchmarkBigIntSquareModN-24 1926706 618.9 ns/op 176 B/op 2 allocs/op
BenchmarkModNScalarSquare-24 5744864 193.1 ns/op 0 B/op 0 allocs/op
BenchmarkBigIntNegateModN-24 3064765 390.0 ns/op 72 B/op 2 allocs/op
BenchmarkModNScalarNegate-24 197936019 5.791 ns/op 0 B/op 0 allocs/op
BenchmarkBigIntInverseModN-24 1000000 1260 ns/op 576 B/op 12 allocs/op
BenchmarkModNScalarInverse-24 1000000 1008 ns/op 472 B/op 12 allocs/op
BenchmarkBigIntIsOverHalfOrder-24 254648517 4.333 ns/op 0 B/op 0 allocs/op
BenchmarkModNScalarIsOverHalfOrder-24 242337598 4.852 ns/op 0 B/op 0 allocs/op
PASS
ok github.com/decred/dcrd/dcrec/secp256k1/v4 54.926s
65bc85a
to
7fff833
Compare
This makes several of the benchmarks more consistent in terms of resetting the timer and reporting memory allocations as well as moves the ones that only apply when accessing the package via the standard library elliptic curve adaptor code to a new file named ellipticadaptor_bench_test.go. It also converts the ecdsa signature verification benchmark to use bytes directly instead of standard lib big ints and remove the no longer necessary hexToBigInt helper.
In order to simplify and consolidate the test logic that ensures test points in Jacobian coordinates are valid group elements, this modifies the test helper function that determines if a given Jacobian point is on the secp256k1 curve to also check for the point at infinity, renames it to isValidJacobianPoint, and updates all callers accordingly.
In order to simplify and consolidate the test logic that ensures test points in affine coordinates are valid group elements, this introduces a new test helper function that determines if a given affine point is on the secp256k1 curve or is the point at infinity and updates all callers accordingly.
This cleans up the tests for performing affine addition to make them more consistent with modern practices in the code and also moves them to the ellipticadaptor_test.go file since they only apply when accessing the package via the standard library elliptic curve adaptor code. The following is a high level overview of the changes: - Renames TestAddAffine to TestAddAffineAdaptor to make it clear it is testing via the adaptor - Adds names to each individual test for easier identification - Makes the individual test definitions consistent with the rest of the code
This cleans up the tests for performing affine point doubling to make them more consistent with modern practices in the code and also moves them to the ellipticadaptor_test.go file since they only apply when accessing the package via the standard library elliptic curve adaptor code. The following is a high level overview of the changes: - Renames TestDoubleAffine to TestDoubleAffineAdaptor to make it clear it is testing via the adaptor - Adds names to each individual test for easier identification - Makes the individual test definitions consistent with the rest of the code
This cleans up the test for key generation to make it more consistent with modern practices in the code and also moves it to the privkey_test.go file since the associated function is defined in privkey.go. The following is a high level overview of the changes: - Rename TestKeyGeneration to TestGeneratePrivateKey to match the func name - Remove the curve-based indirection since it only works with secp256k1
This cleans up the tests for performing affine scalar point multiplication to make them more consistent with modern practices in the code and also moves them to the ellipticadaptor_test.go file since they only apply when accessing the package via the standard library elliptic curve adaptor code. The following is a high level overview of the changes: - Renames TestScalarMult to TestScalarMultAdaptor to make it clear it is testing via the adaptor - Adds names to each individual test for easier identification - Makes the individual test definitions consistent with the rest of the code - Adds some additional edge cases
This cleans up the tests for performing affine scalar base multiplication to make them more consistent with modern practices in the code. The following is a high level overview of the changes: - Adds names to each individual test for easier identification - Makes the individual test definitions consistent with the rest of the code - Adds some additional edge cases
This cleans up the test for performing affine scalar base multiplication with random scalars to make it more consistent with modern practices in the code and also moves it to the ellipticadaptor_test.go file since it only applies when accessing the package via the standard library elliptic curve adaptor code. The following is a high level overview of the changes: - Adds a randBytes convenience func to acquire a specified number of random bytes from a given rng - Renames TestBaseMultVerify to TestScalarBaseMultAdaptorRandom to make it clear it is testing via the adaptor and based on random values - Uses a new seed for each invocation to ensure new random values are tested each run as opposed to always testing the same random values generated by the same seed
This cleans up the test for key generation to make it more consistent with modern practices in the code and adds a couple more test cases.
This cleans up the tests for performing Jacobian addition to make them more consistent with modern practices in the code. The following is a high level overview of the changes: - Adds names to each individual test for easier identification - Makes the individual test definitions consistent with the rest of the code
This cleans up the tests for performing Jacobian point doubling to make them more consistent with modern practices in the code. The following is a high level overview of the changes: - Adds names to each individual test for easier identification - Makes the individual test definitions consistent with the rest of the code
This adds a new test helper for generating random mod n scalars without also generating a companion big integer and updates the tests to make use of it. It also marks the combined version as a test helper while here. The new helper will be useful for future tests.
This adds tests for scalar base multiplication for points projected in Jacobian coordinates.
This reworks the tests that deal with scalar point multiplication with points projected into Jacobian coordinates for randomly-generated scalars and points to make them more consistent with modern practices in the code as well as to expand the testing methodology to include additional assurances. It also updates the tests to ensure new random values are tested each run as opposed to the existing tests which always test the same values since they use the same random seed. Specifically: - The points are no longer converted to affine with each iteration which ensures z values other than 1 are tested - Each iteration now ensures calculating the negative version of the point and adding it results in the point at infinity - The check to ensure the same final point was calculated is now done outside of the loop to speed up the test
davecgh
force-pushed
the
secp256k1_test_cleanup
branch
from
7fff833
to
4a6438a
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This requires #2886.This cleans up a bunch of tests and benchmarks to make them more consistent with modern practices in the code and also moves the ones that only apply when accessing the package via the standard library elliptic curve adaptor code to files dedicated to that purpose. It also adds several new tests for various edge cases and ensures the randomized testing uses new seeds on each invocation to ensure new random values are tested each run.
It is split up into a series of commits to help ease the review process.
The following is a high level overview of the changes:
ellipticadaptor_test.goellipticadaptor_test.goprivkey_test.goellipticadaptor_test.goellipticadaptor_test.gorandBytesconvenience func to acquire a specified number of random bytes from a given rng