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 part 3] Remove math/rand from integration and insecure modules #4106

Closed
wants to merge 2 commits into from

Conversation

tarakby
Copy link
Contributor

@tarakby tarakby commented Mar 27, 2023

( result of splitting #4052 into several PRs)

  • prepare for Go1.20 update by removing deprecated math/rand functions Seed and Read.
  • minor clean ups

@codecov-commenter
Copy link

codecov-commenter commented Mar 27, 2023

Codecov Report

Merging #4106 (36e488f) into master (814e98d) will decrease coverage by 2.65%.
The diff coverage is 50.36%.

@@            Coverage Diff             @@
##           master    #4106      +/-   ##
==========================================
- Coverage   53.51%   50.87%   -2.65%     
==========================================
  Files         833      600     -233     
  Lines       77896    55921   -21975     
==========================================
- Hits        41687    28450   -13237     
+ Misses      32874    25104    -7770     
+ Partials     3335     2367     -968     
Flag Coverage Δ
unittests 50.87% <50.36%> (-2.65%) ⬇️

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 29 more

... and 235 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.

@tarakby tarakby marked this pull request as ready for review March 28, 2023 05:35
@tarakby tarakby changed the title Remove math/rand from integration and insecure modules [Randomness part 3] Remove math/rand from integration and insecure modules Mar 28, 2023
@@ -168,8 +168,6 @@ func (s *DKGSuite) runTest(goodNodes int, emulatorProblems bool) {

// shuffle the signatures and indices before constructing the group
// signature (since it only uses the first half signatures)
seed := time.Now().UnixNano()
Copy link
Contributor

Choose a reason for hiding this comment

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

did you also want to update the import, or just remove the seed?

Copy link
Contributor Author

@tarakby tarakby Mar 29, 2023

Choose a reason for hiding this comment

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

time and rand are still used that's why the imports were not changed. Did I get your comment right?

@@ -298,8 +298,6 @@ func TestWithWhiteboard(t *testing.T) {

// shuffle the signatures and indices before constructing the group
// signature (since it only uses the first half signatures)
seed := time.Now().UnixNano()
Copy link
Contributor

Choose a reason for hiding this comment

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

same here

integration/tests/consensus/inclusion_test.go Outdated Show resolved Hide resolved
@tarakby tarakby requested a review from peterargue March 31, 2023 01:06
@tarakby
Copy link
Contributor Author

tarakby commented Apr 3, 2023

Closing as changes were incorporated in another PR.
fabbd32 will also be ported.

@tarakby tarakby closed this Apr 3, 2023
@tarakby tarakby deleted the tarak/randomness-part3-integ-insecure branch April 21, 2023 03:08
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.

3 participants