Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 45 commits into from
Jul 14, 2023
Merged

Conversation

tarakby
Copy link
Contributor

@tarakby tarakby commented Mar 16, 2023

This PR is the last of multiple small PRs updating the usage of math/rand in all the repo, for two purposes:

  • move on from using math/rand in production code (seed length is too short, randomness quality is low)
  • even though math/rand can be used in test files, the functions Seed and Read are deprecated in Go1.20 and should not be used as the repo prepares to upgrade the Go version.

Previous related PRs include:

  1. [randomness part 1] New utils/rand package for non-deterministic true randomness #4062
  2. [randomness part 2] improve usafeRandom and update math/rand in FVM #4067
  3. [Randomness part 3] Remove math/rand from integration and insecure modules #4106
  4. [Randomness part 4] Update math/rand usage in crypto module and improve randomness tests #4111
  5. [Randmomness part 5] update math/rand usage in ledger #4112
  6. [randomness part 6] update math/rand usage in /consenus and /engine #4258
  7. Side change: Clean up DKG simulation in bootstrap #4259
  8. [randomness part 8] update math/rand usage in /cmd/bootstrap #4362

This final PR of the list has the following changes:

  • update Identifier and IdentifierList random functions to use utils/rand (backed by secure crypto/rand) instead of relying on math/rand. The random functions are Sample, Shuffle, SamplePct.
  • update calls of all random functions above (with new returned error)
  • replace math/rand PRG in DefaultLibp2pBackoffConnectorFactory by a secure PRG (cc @yhassanzadeh13)
  • remove 1.20 deprecated math/rand functions Seed and Read from test files (this means some tests are now deterministic! which is temporary till Go1.20 is used), and use crypto/rand in some tests.
  • add a new make target that checks math/rand is not imported in production code, to avoid the package is re-introduced in the future.

@github-actions
Copy link
Contributor

github-actions bot commented Mar 16, 2023

FVM Benchstat comparison

This branch with compared with the base branch onflow:master commit 117fcc0

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.txtnew.txt
time/opdelta
pkg:github.com/onflow/flow-go/fvm goos:linux goarch:amd64
RuntimeTransaction/reference_tx-239.5ms ± 3%39.8ms ± 2%~(p=0.628 n=7+6)
RuntimeTransaction/convert_int_to_string-243.8ms ±12%42.2ms ± 5%~(p=0.535 n=7+7)
RuntimeTransaction/convert_int_to_string_and_concatenate_it-244.4ms ± 2%43.2ms ± 4%~(p=0.177 n=5+6)
RuntimeTransaction/get_signer_address-240.6ms ± 4%41.2ms ± 3%~(p=0.240 n=6+6)
RuntimeTransaction/get_public_account-243.9ms ± 2%44.1ms ±12%~(p=0.731 n=6+7)
RuntimeTransaction/get_account_and_get_balance-2311ms ± 5%312ms ± 0%~(p=0.268 n=7+5)
RuntimeTransaction/get_account_and_get_available_balance-2304ms ± 1%304ms ± 1%~(p=0.731 n=6+7)
RuntimeTransaction/get_account_and_get_storage_capacity-2270ms ± 1%270ms ± 2%~(p=0.731 n=6+7)
RuntimeTransaction/get_signer_vault-248.7ms ± 4%47.9ms ± 5%~(p=0.259 n=7+7)
RuntimeTransaction/get_signer_receiver-260.0ms ± 7%60.2ms ± 2%~(p=0.836 n=7+6)
RuntimeTransaction/transfer_tokens-2228ms ± 3%225ms ± 4%~(p=0.097 n=7+7)
RuntimeTransaction/load_and_save_empty_string_on_signers_address-247.8ms ± 6%47.8ms ± 4%~(p=1.000 n=7+7)
RuntimeTransaction/load_and_save_long_string_on_signers_address-296.3ms ± 4%98.0ms ± 2%~(p=0.073 n=7+6)
RuntimeTransaction/create_new_account-2906ms ± 1%905ms ± 1%~(p=0.805 n=7+7)
RuntimeTransaction/call_empty_contract_function-243.6ms ± 5%43.6ms ± 3%~(p=0.628 n=7+6)
RuntimeTransaction/emit_event-257.8ms ± 6%57.7ms ± 4%~(p=1.000 n=7+7)
RuntimeTransaction/borrow_array_from_storage-2152ms ± 2%151ms ± 2%~(p=0.318 n=7+7)
RuntimeTransaction/copy_array_from_storage-2155ms ± 3%153ms ± 2%~(p=0.318 n=7+7)
RuntimeNFTBatchTransfer-2133ms ± 6%133ms ± 2%~(p=0.620 n=7+7)
pkg:github.com/onflow/flow-go/engine/execution/computation goos:linux goarch:amd64
ComputeBlock/16/cols/128/txes/1/max-concurrency-25.10s ± 0%5.10s ± 1%~(p=1.000 n=6+7)
ComputeBlock/16/cols/128/txes/2/max-concurrency-25.41s ± 1%5.41s ± 1%~(p=0.710 n=7+7)
pkg:github.com/onflow/flow-go/fvm goos:linux goarch:amd64
RuntimeTransaction/get_account_and_get_storage_used-246.7ms ± 4%45.3ms ± 3%−3.11%(p=0.026 n=7+7)
 
computationdelta
pkg:github.com/onflow/flow-go/fvm goos:linux goarch:amd64
RuntimeTransaction/reference_tx-2202 ± 0%202 ± 0%~(all equal)
RuntimeTransaction/convert_int_to_string-2402 ± 0%402 ± 0%~(all equal)
RuntimeTransaction/convert_int_to_string_and_concatenate_it-2502 ± 0%502 ± 0%~(all equal)
RuntimeTransaction/get_signer_address-2302 ± 0%302 ± 0%~(all equal)
RuntimeTransaction/get_public_account-2402 ± 0%402 ± 0%~(all equal)
RuntimeTransaction/get_account_and_get_balance-21.00k ± 0%1.00k ± 0%~(all equal)
RuntimeTransaction/get_account_and_get_available_balance-23.10k ± 0%3.10k ± 0%~(all equal)
RuntimeTransaction/get_account_and_get_storage_used-2402 ± 0%402 ± 0%~(all equal)
RuntimeTransaction/get_account_and_get_storage_capacity-21.70k ± 0%1.70k ± 0%~(all equal)
RuntimeTransaction/get_signer_vault-2402 ± 0%402 ± 0%~(all equal)
RuntimeTransaction/get_signer_receiver-2602 ± 0%602 ± 0%~(all equal)
RuntimeTransaction/transfer_tokens-23.50k ± 0%3.50k ± 0%~(all equal)
RuntimeTransaction/load_and_save_empty_string_on_signers_address-2602 ± 0%602 ± 0%~(all equal)
RuntimeTransaction/load_and_save_long_string_on_signers_address-2602 ± 0%602 ± 0%~(all equal)
RuntimeTransaction/create_new_account-2202 ± 0%202 ± 0%~(all equal)
RuntimeTransaction/call_empty_contract_function-2402 ± 0%402 ± 0%~(all equal)
RuntimeTransaction/emit_event-2602 ± 0%602 ± 0%~(all equal)
RuntimeTransaction/borrow_array_from_storage-22.60k ± 0%2.60k ± 0%~(all equal)
RuntimeTransaction/copy_array_from_storage-22.60k ± 0%2.60k ± 0%~(all equal)
 
interactionsdelta
pkg:github.com/onflow/flow-go/fvm goos:linux goarch:amd64
RuntimeTransaction/reference_tx-238.2k ± 0%38.2k ± 0%~(p=0.056 n=7+6)
RuntimeTransaction/convert_int_to_string-238.2k ± 0%38.2k ± 0%~(p=0.268 n=6+7)
RuntimeTransaction/convert_int_to_string_and_concatenate_it-238.2k ± 0%38.2k ± 0%~(p=0.971 n=7+7)
RuntimeTransaction/get_signer_address-238.2k ± 0%38.2k ± 0%~(p=0.161 n=7+6)
RuntimeTransaction/get_public_account-238.2k ± 0%38.2k ± 0%~(p=0.826 n=7+7)
RuntimeTransaction/get_account_and_get_balance-246.4k ± 0%46.4k ± 0%~(all equal)
RuntimeTransaction/get_account_and_get_available_balance-238.1k ± 0%38.1k ± 0%~(p=0.195 n=7+7)
RuntimeTransaction/get_account_and_get_storage_used-238.2k ± 0%38.2k ± 0%~(p=1.000 n=7+7)
RuntimeTransaction/get_account_and_get_storage_capacity-238.1k ± 0%38.1k ± 0%~(p=0.212 n=7+7)
RuntimeTransaction/get_signer_vault-238.2k ± 0%38.2k ± 0%~(p=0.988 n=7+7)
RuntimeTransaction/get_signer_receiver-238.2k ± 0%38.2k ± 0%~(p=0.997 n=7+7)
RuntimeTransaction/transfer_tokens-238.1k ± 0%38.1k ± 0%~(p=0.636 n=7+5)
RuntimeTransaction/load_and_save_empty_string_on_signers_address-238.3k ± 0%38.3k ± 0%~(p=0.425 n=7+7)
RuntimeTransaction/load_and_save_long_string_on_signers_address-242.1k ± 0%42.1k ± 0%~(p=0.074 n=7+7)
RuntimeTransaction/create_new_account-284.6k ± 0%84.6k ± 0%~(p=0.567 n=6+6)
RuntimeTransaction/call_empty_contract_function-238.4k ± 0%38.4k ± 0%~(p=0.330 n=7+7)
RuntimeTransaction/emit_event-238.4k ± 0%38.4k ± 0%~(p=0.368 n=7+7)
RuntimeTransaction/borrow_array_from_storage-243.4k ± 0%43.4k ± 0%~(p=0.091 n=6+5)
RuntimeTransaction/copy_array_from_storage-243.4k ± 0%43.4k ± 0%~(p=0.293 n=7+7)
 
alloc/opdelta
pkg:github.com/onflow/flow-go/fvm goos:linux goarch:amd64
RuntimeTransaction/reference_tx-235.5MB ± 2%35.8MB ± 4%~(p=0.456 n=7+7)
RuntimeTransaction/convert_int_to_string-236.1MB ± 1%36.3MB ± 5%~(p=0.902 n=7+7)
RuntimeTransaction/convert_int_to_string_and_concatenate_it-237.4MB ± 5%36.8MB ± 3%~(p=0.318 n=7+7)
RuntimeTransaction/get_signer_address-236.2MB ± 2%36.1MB ± 3%~(p=0.805 n=7+7)
RuntimeTransaction/get_public_account-237.2MB ± 2%36.4MB ± 2%~(p=0.101 n=7+6)
RuntimeTransaction/get_account_and_get_balance-2122MB ± 1%123MB ± 2%~(p=0.628 n=6+7)
RuntimeTransaction/get_account_and_get_available_balance-2110MB ± 3%110MB ± 2%~(p=0.902 n=7+7)
RuntimeTransaction/get_account_and_get_storage_capacity-2106MB ± 2%107MB ± 3%~(p=0.383 n=7+7)
RuntimeTransaction/get_signer_vault-237.7MB ± 4%38.3MB ± 3%~(p=0.209 n=7+7)
RuntimeTransaction/get_signer_receiver-241.6MB ± 8%41.3MB ± 4%~(p=0.535 n=7+7)
RuntimeTransaction/transfer_tokens-282.1MB ± 5%81.2MB ± 1%~(p=0.343 n=7+5)
RuntimeTransaction/load_and_save_empty_string_on_signers_address-237.3MB ± 6%37.2MB ± 3%~(p=0.902 n=7+7)
RuntimeTransaction/load_and_save_long_string_on_signers_address-254.8MB ± 7%53.8MB ± 9%~(p=0.620 n=7+7)
RuntimeTransaction/create_new_account-2186MB ± 3%185MB ± 3%~(p=1.000 n=7+7)
RuntimeTransaction/call_empty_contract_function-236.8MB ± 3%37.0MB ± 3%~(p=0.710 n=7+7)
RuntimeTransaction/emit_event-241.1MB ± 7%40.6MB ± 3%~(p=0.620 n=7+7)
RuntimeTransaction/borrow_array_from_storage-271.1MB ± 3%71.1MB ± 3%~(p=0.836 n=6+7)
RuntimeTransaction/copy_array_from_storage-283.2MB ± 1%84.2MB ± 3%~(p=0.165 n=7+7)
RuntimeNFTBatchTransfer-254.7MB ± 6%55.3MB ± 6%~(p=0.535 n=7+7)
pkg:github.com/onflow/flow-go/engine/execution/computation goos:linux goarch:amd64
ComputeBlock/16/cols/128/txes/1/max-concurrency-21.24GB ± 1%1.24GB ± 1%~(p=0.128 n=7+7)
ComputeBlock/16/cols/128/txes/2/max-concurrency-21.29GB ± 1%1.29GB ± 1%~(p=0.836 n=6+7)
pkg:github.com/onflow/flow-go/fvm goos:linux goarch:amd64
RuntimeTransaction/get_account_and_get_storage_used-238.3MB ± 2%37.3MB ± 5%−2.64%(p=0.026 n=7+7)
 
allocs/opdelta
pkg:github.com/onflow/flow-go/fvm goos:linux goarch:amd64
RuntimeTransaction/reference_tx-292.4k ± 0%92.4k ± 0%~(p=0.274 n=7+7)
RuntimeTransaction/convert_int_to_string-2105k ± 0%105k ± 0%~(p=0.148 n=6+7)
RuntimeTransaction/convert_int_to_string_and_concatenate_it-2116k ± 0%116k ± 0%~(p=0.052 n=7+7)
RuntimeTransaction/get_signer_address-296.5k ± 0%96.5k ± 0%~(p=0.087 n=6+6)
RuntimeTransaction/get_public_account-2121k ± 0%121k ± 0%~(p=0.597 n=7+7)
RuntimeTransaction/get_account_and_get_balance-21.33M ± 0%1.33M ± 0%~(p=0.318 n=7+7)
RuntimeTransaction/get_account_and_get_available_balance-21.29M ± 0%1.29M ± 0%~(p=0.435 n=7+7)
RuntimeTransaction/get_account_and_get_storage_capacity-21.16M ± 0%1.16M ± 0%~(p=0.945 n=7+6)
RuntimeTransaction/get_signer_vault-2137k ± 0%137k ± 0%~(p=0.736 n=7+7)
RuntimeTransaction/get_signer_receiver-2223k ± 0%223k ± 0%~(p=0.731 n=7+6)
RuntimeTransaction/transfer_tokens-2863k ± 0%863k ± 0%~(p=0.383 n=7+6)
RuntimeTransaction/load_and_save_empty_string_on_signers_address-2143k ± 0%143k ± 0%~(p=0.350 n=6+7)
RuntimeTransaction/load_and_save_long_string_on_signers_address-2226k ± 0%226k ± 0%~(p=0.535 n=7+7)
RuntimeTransaction/create_new_account-22.36M ± 0%2.36M ± 0%~(p=0.456 n=7+7)
RuntimeTransaction/call_empty_contract_function-2110k ± 0%110k ± 0%~(p=0.139 n=7+7)
RuntimeTransaction/emit_event-2151k ± 0%151k ± 0%~(p=0.620 n=7+7)
RuntimeTransaction/borrow_array_from_storage-2372k ± 0%372k ± 0%~(p=0.628 n=7+6)
RuntimeTransaction/copy_array_from_storage-2304k ± 0%304k ± 0%~(p=0.512 n=7+7)
pkg:github.com/onflow/flow-go/engine/execution/computation goos:linux goarch:amd64
ComputeBlock/16/cols/128/txes/2/max-concurrency-218.4M ± 0%18.4M ± 0%~(p=0.710 n=7+7)
pkg:github.com/onflow/flow-go/fvm goos:linux goarch:amd64
RuntimeTransaction/get_account_and_get_storage_used-2132k ± 0%132k ± 0%−0.01%(p=0.023 n=7+7)
pkg:github.com/onflow/flow-go/engine/execution/computation goos:linux goarch:amd64
ComputeBlock/16/cols/128/txes/1/max-concurrency-217.8M ± 0%17.8M ± 0%−0.03%(p=0.001 n=7+7)
pkg:github.com/onflow/flow-go/fvm goos:linux goarch:amd64
RuntimeNFTBatchTransfer-2282k ± 0%281k ± 0%−0.28%(p=0.022 n=6+7)
 
us/txdelta
pkg:github.com/onflow/flow-go/engine/execution/computation goos:linux goarch:amd64
ComputeBlock/16/cols/128/txes/1/max-concurrency-22.49k ± 0%2.49k ± 1%~(p=1.000 n=6+7)
ComputeBlock/16/cols/128/txes/2/max-concurrency-22.64k ± 1%2.64k ± 1%~(p=0.685 n=7+7)
 

@codecov-commenter
Copy link

codecov-commenter commented Mar 16, 2023

Codecov Report

Merging #4052 (8e21991) into master (2528190) will decrease coverage by 1.86%.
The diff coverage is 46.64%.

@@            Coverage Diff             @@
##           master    #4052      +/-   ##
==========================================
- Coverage   56.25%   54.40%   -1.86%     
==========================================
  Files         653      914     +261     
  Lines       64699    85199   +20500     
==========================================
+ Hits        36396    46351    +9955     
- Misses      25362    35263    +9901     
- Partials     2941     3585     +644     
Flag Coverage Δ
unittests 54.40% <46.64%> (-1.86%) ⬇️

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

Impacted Files Coverage Δ
cmd/scaffold.go 14.83% <0.00%> (+0.06%) ⬆️
engine/access/rpc/backend/backend_transactions.go 46.07% <0.00%> (-0.08%) ⬇️
model/flow/identifierList.go 22.36% <0.00%> (+0.84%) ⬆️
model/verification/chunkDataPackRequest.go 43.75% <0.00%> (-6.25%) ⬇️
module/metrics/herocache.go 0.00% <0.00%> (ø)
module/metrics/network.go 0.00% <0.00%> (ø)
module/metrics/noop.go 0.00% <0.00%> (ø)
network/p2p/inspector/internal/cache/cache.go 64.70% <ø> (ø)
network/p2p/middleware/middleware.go 1.71% <0.00%> (ø)
network/p2p/p2pbuilder/libp2pNodeBuilder.go 0.00% <0.00%> (ø)
... and 26 more

... and 245 files with indirect coverage changes

@tarakby tarakby changed the title WIP [WIP] Update randomness usage Mar 16, 2023
@tarakby tarakby changed the title [WIP] Update randomness usage Update randomness usage Mar 16, 2023
@tarakby tarakby marked this pull request as ready for review March 16, 2023 19:21
cmd/bootstrap/cmd/clusters.go Outdated Show resolved Hide resolved
engine/execution/computation/query/executor.go Outdated Show resolved Hide resolved
storage/badger/cleaner.go Show resolved Hide resolved
model/flow/identity.go Outdated Show resolved Hide resolved
network/p2p/connection/peerManager.go Outdated Show resolved Hide resolved
Copy link
Contributor

@peterargue peterargue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good

@tarakby
Copy link
Contributor Author

tarakby commented Jun 22, 2023

@peterargue I'm thinking of adding a Makefile target that checks math/rand is not used in any production code and that is required to pass for PR merging. While this current PR removes all the package usage, I'm confident it will make its way back to the repo in the next months if we don't enforce it somehow 😄 What do you think?

[Update]: I added this check.

@tarakby tarakby force-pushed the tarak/go1.20-rand-prep branch from bac7d61 to 14df98f Compare June 22, 2023 20:06
@tarakby tarakby force-pushed the tarak/go1.20-rand-prep branch from b09f12b to e3447dc Compare June 23, 2023 23:59
Copy link
Contributor

@yhassanzadeh13 yhassanzadeh13 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the refactoring. I reviewed mostly the relevant parts to my domain.

module/mempool/herocache/backdata/heropool/pool.go Outdated Show resolved Hide resolved
module/mempool/stdmap/eject.go Outdated Show resolved Hide resolved
network/p2p/connection/connector_factory.go Show resolved Hide resolved
Copy link
Contributor

@yhassanzadeh13 yhassanzadeh13 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great; thanks for refactoring.

@tarakby tarakby merged commit 72b4db9 into master Jul 14, 2023
@tarakby tarakby deleted the tarak/go1.20-rand-prep branch July 14, 2023 20:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants