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 8] update math/rand usage in /cmd/bootstrap #4362

Merged
merged 8 commits into from
May 31, 2023

Conversation

tarakby
Copy link
Contributor

@tarakby tarakby commented May 17, 2023

This is part of the work started in #4052.

Context is to update 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 isn't suited for production)
  • 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.

This PR updated /cmd/bootstrap:

  • replace deterministic clustering (based on math/rand) by non-deterministic clustering (based on crypto/rand).
  • remove the bootstrap seed flag that used to seed the deterministic process
  • the cluster assignment only outputs clusters satisfying the internal/partner nodes requirement (the later check is only a sanity check) - clarify the deterministic properties of the algorithm.
  • Epoch's random source is set non-deterministically based on crypto/rand in :
    • execution state generation
    • root result generation
  • clean up tests

@codecov-commenter
Copy link

codecov-commenter commented May 17, 2023

Codecov Report

Merging #4362 (86bc599) into master (b427320) will decrease coverage by 0.12%.
The diff coverage is 55.76%.

@@            Coverage Diff             @@
##           master    #4362      +/-   ##
==========================================
- Coverage   53.75%   53.63%   -0.12%     
==========================================
  Files         871      882      +11     
  Lines       80769    82048    +1279     
==========================================
+ Hits        43420    44010     +590     
- Misses      33915    34553     +638     
- Partials     3434     3485      +51     
Flag Coverage Δ
unittests 53.63% <55.76%> (-0.12%) ⬇️

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

Impacted Files Coverage Δ
cmd/bootstrap/cmd/rootblock.go 90.54% <ø> (+3.79%) ⬆️
cmd/bootstrap/utils/file.go 0.00% <0.00%> (ø)
cmd/execution_builder.go 0.00% <0.00%> (ø)
cmd/scaffold.go 14.50% <0.00%> (-0.20%) ⬇️
...ck-executed-height/cmd/rollback_executed_height.go 15.69% <0.00%> (ø)
cmd/verification_builder.go 0.00% <0.00%> (ø)
consensus/config.go 0.00% <0.00%> (ø)
consensus/hotstuff/pacemaker/timeout/controller.go 47.14% <ø> (-1.47%) ⬇️
...s/hotstuff/timeoutaggregator/timeout_aggregator.go 70.31% <ø> (ø)
...sus/hotstuff/timeoutcollector/timeout_collector.go 100.00% <ø> (ø)
... and 67 more

... and 11 files with indirect coverage changes

@tarakby tarakby marked this pull request as ready for review May 19, 2023 22:17
@tarakby tarakby requested a review from zhangchiqing as a code owner May 19, 2023 22:17
// and only depends on the number of clusters and nodes.
// However, the list of nodes in each cluster if non-deterministic and produces
// different lists on each function call.
func constructClusterAssignment(participants flow.IdentityList) (flow.AssignmentList, flow.ClusterList) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

background discussion about this change happened in another PR: #4052 (comment)

@tarakby tarakby requested review from jordanschalm and AlexHentschel and removed request for zhangchiqing, jordanschalm and AlexHentschel May 19, 2023 22:35
@tarakby tarakby changed the title [randomness part 8] update math/rand usage in /cmd/bootstrap WIP [randomness part 8] update math/rand usage in /cmd/bootstrap May 19, 2023
@tarakby tarakby force-pushed the tarak/8-bootstrap-seed branch from 208a099 to bdb5330 Compare May 19, 2023 23:48
@tarakby tarakby changed the title WIP [randomness part 8] update math/rand usage in /cmd/bootstrap [randomness part 8] update math/rand usage in /cmd/bootstrap May 23, 2023
Copy link
Member

@jordanschalm jordanschalm left a comment

Choose a reason for hiding this comment

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

I think we can further cleanup the cluster constraint checking, by removing the now-extraneous checkClusterConstraint. Otherwise looks good!

cmd/bootstrap/cmd/constraints.go Outdated Show resolved Hide resolved
cmd/bootstrap/cmd/constraints.go Outdated Show resolved Hide resolved
cmd/bootstrap/cmd/constraints.go Outdated Show resolved Hide resolved
@tarakby
Copy link
Contributor Author

tarakby commented May 31, 2023

bors merge

@bors bors bot merged commit 3c25e4e into master May 31, 2023
@bors bors bot deleted the tarak/8-bootstrap-seed branch May 31, 2023 19:47
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.

4 participants