[Randomness part 4] Update math/rand usage in crypto module and improve randomness tests #4111

tarakby · 2023-03-28T19:40:39Z

( part of this PR is a result of splitting #4052 into several PRs)

prepare for Go1.20 update by removing deprecated math/rand functions Seed and Read.
improve randomness tests in (crypto/random, utils/rand and fvm/environment):
- fix test bugs in utils/rand is extremely rare cases.
- consolidate the statistic test tools in under crypto library - once the new crypto module is tagged, flow-go test would use the newly exported functions.
- improve coverage of integer modulo-type random functions to cover small and large modulos.
- improve the sample size to avoid bias of distribution and speed up tests when modulo is small.
- side change: update math/rand usage in /integration and /insecure

…est tools

github-actions · 2023-03-28T19:43:47Z

FVM Benchstat comparison

This branch with compared with the base branch onflow:master commit fc5b515

The command (for i in {1..7}; do go test ./fvm ./engine/execution/computation --bench . --tags relic -shuffle=on --benchmem --run ^$; done) was used.

Collapsed results for better readability

	old.txt	new.txt
	time/op		delta
ComputeBlock/16/cols/128/txes-2	6.43s ± 0%	6.34s ± 0%	~	(p=1.000 n=1+1)

	us/tx		delta
ComputeBlock/16/cols/128/txes-2	3.14k ± 0%	3.09k ± 0%	~	(p=1.000 n=1+1)

	alloc/op		delta
ComputeBlock/16/cols/128/txes-2	1.21GB ± 0%	1.21GB ± 0%	~	(p=1.000 n=1+1)

	allocs/op		delta
ComputeBlock/16/cols/128/txes-2	17.4M ± 0%	17.4M ± 0%	~	(p=1.000 n=1+1)

tarakby · 2023-03-28T19:44:12Z

crypto/random/rand_utils.go

+	"gonum.org/v1/gonum/stat"
+)
+
+// BasicDistributionTest is a test function to run a basic statistic test on `randf` output.


Randomness test tools consolidated here so that all flow-go tests can use them.
Redundant code will be deleted once the new crypto version is tagged.

tarakby · 2023-03-28T19:45:04Z

fvm/environment/unsafe_random_generator_test.go

+// TODO: these functions are copied from flow-go/crypto/rand
+// Once the new flow-go/crypto/ module version is tagged, flow-go would upgrade
+// to the new version and import these functions
+func BasicDistributionTest(t *testing.T, n uint64, classWidth uint64, randf func() (uint64, error)) {


redundant code till the new crypto tag is ready

tarakby · 2023-03-28T19:45:30Z

utils/rand/rand_test.go

+// TODO: these functions are copied from flow-go/crypto/rand
+// Once the new flow-go/crypto/ module version is tagged, flow-go would upgrade
+// to the new version and import these functions
+func BasicDistributionTest(t *testing.T, n uint64, classWidth uint64, randf func() (uint64, error)) {


redundant code till the new crypto tag is ready

codecov-commenter · 2023-03-28T19:48:42Z

Codecov Report

Merging #4111 (c24395a) into master (814e98d) will decrease coverage by 2.33%.
The diff coverage is 48.18%.

@@            Coverage Diff             @@
##           master    #4111      +/-   ##
==========================================
- Coverage   53.51%   51.18%   -2.33%     
==========================================
  Files         833      673     -160     
  Lines       77896    60340   -17556     
==========================================
- Hits        41687    30886   -10801     
+ Misses      32874    26971    -5903     
+ Partials     3335     2483     -852

Flag	Coverage Δ
unittests	`51.18% <48.18%> (-2.33%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
cmd/bootstrap/cmd/keys.go	`53.60% <0.00%> (ø)`
cmd/scaffold.go	`14.75% <0.00%> (-0.34%)`	⬇️
cmd/util/ledger/reporters/account_reporter.go	`0.00% <0.00%> (ø)`
...nsensus/hotstuff/committees/consensus_committee.go	`69.72% <ø> (ø)`
fvm/derived/derived_block_data.go	`33.33% <0.00%> (-0.96%)`	⬇️
fvm/derived/table_invalidator.go	`100.00% <ø> (ø)`
fvm/environment/account_key_updater.go	`19.48% <0.00%> (ø)`
fvm/environment/facade_env.go	`87.11% <0.00%> (ø)`
fvm/errors/accounts.go	`0.00% <0.00%> (ø)`
fvm/errors/codes.go	`100.00% <ø> (ø)`
... and 43 more

... and 166 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

crypto/hash/hash_test.go

pattyshack · 2023-03-29T17:01:49Z

crypto/random/rand.go

@@ -149,11 +149,7 @@ func (p *genericPRG) Shuffle(n int, swap func(i, j int)) error {
 	if n < 0 {
 		return fmt.Errorf("population size cannot be negative")
 	}
-	for i := n - 1; i > 0; i-- {


the algorithms in utils/rand/rand.go is basically identical to this. the only difference is the random source. can you refactor the code to share implementation?

btw, Permutation / SubPermutation should also just call Shuffle/Sample

maybe undo changes to this file in this PR (since it's unrelated to test updates), and make the changes in a separate PR

Permutation / SubPermutation should also just call Shuffle/Sample

This is true, though it is slightly less efficient: in order to use Shuffle, Permutation needs to initialize an array firts, and then shuffle it. The current implementation of Permutation initializes and permutes at the same time (it's an optimized variant of Fisher-Yates).

can you refactor the code to share implementation?

You are right that the core idea is the same. I did explore the idea of combining both implementations but I didn't find an elegant way to do that, given there are minor differences between the packages (returned errors, thread safety, integer types). For now I only combined the test tools, but we can discuss ways to combine the rest.

peterargue

looks good. a few nits

crypto/bls12381_utils_test.go

crypto/bls_test.go

crypto/bls_thresholdsign_test.go

crypto/ecdsa_test.go

tarakby added 6 commits March 27, 2023 17:06

remove math/rand in insecure and integration module

7704e97

minor fix and remove non-needed check

f37aaca

update math/rand usage to prepare for go1.20

a7bec70

refactor and improve uintN test with small and big N cases - export t…

12cb135

…est tools

fix possible test errors in rare cases - refactor test functions

79a6024

minor code reuse

efe5500

tarakby commented Mar 28, 2023

View reviewed changes

tarakby marked this pull request as ready for review March 28, 2023 19:53

tarakby requested review from gomisha, yhassanzadeh13, ramtinms, janezpodhostnik and pattyshack as code owners March 28, 2023 19:53

tarakby changed the title ~~[Crypto] Update math/rand usage in crypto module and improve randomness tests~~ [Randomness part 4] Update math/rand usage in crypto module and improve randomness tests Mar 28, 2023

pattyshack approved these changes Mar 29, 2023

View reviewed changes

peterargue approved these changes Mar 29, 2023

View reviewed changes

tarakby and others added 2 commits March 29, 2023 19:01

add error check in rand.Read to diversify benchark measures

2897627

Merge branch 'master' into tarak/randomness-part4-crypto

c24395a

tarakby merged commit 72a1e42 into master Mar 31, 2023

tarakby deleted the tarak/randomness-part4-crypto branch March 31, 2023 16:56

tarakby mentioned this pull request Apr 22, 2023

[randomness final part] Update math/rand usage #4052

Merged

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Randomness part 4] Update math/rand usage in crypto module and improve randomness tests #4111

[Randomness part 4] Update math/rand usage in crypto module and improve randomness tests #4111

tarakby commented Mar 28, 2023 •

edited

Loading

github-actions bot commented Mar 28, 2023 •

edited

Loading

tarakby Mar 28, 2023

tarakby Mar 28, 2023

tarakby Mar 28, 2023

codecov-commenter commented Mar 28, 2023 •

edited

Loading

pattyshack Mar 29, 2023

tarakby Mar 31, 2023

peterargue left a comment

[Randomness part 4] Update math/rand usage in crypto module and improve randomness tests #4111

[Randomness part 4] Update math/rand usage in crypto module and improve randomness tests #4111

Conversation

tarakby commented Mar 28, 2023 • edited Loading

github-actions bot commented Mar 28, 2023 • edited Loading

FVM Benchstat comparison

tarakby Mar 28, 2023

Choose a reason for hiding this comment

tarakby Mar 28, 2023

Choose a reason for hiding this comment

tarakby Mar 28, 2023

Choose a reason for hiding this comment

codecov-commenter commented Mar 28, 2023 • edited Loading

Codecov Report

pattyshack Mar 29, 2023

Choose a reason for hiding this comment

tarakby Mar 31, 2023

Choose a reason for hiding this comment

peterargue left a comment

Choose a reason for hiding this comment

tarakby commented Mar 28, 2023 •

edited

Loading

github-actions bot commented Mar 28, 2023 •

edited

Loading

codecov-commenter commented Mar 28, 2023 •

edited

Loading