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
Show file tree
Hide file tree
Changes from 34 commits
Commits
Show all changes
45 commits
Select commit Hold shift + click to select a range
0e87757
Go 1.20
SaveTheRbtz Feb 8, 2023
026312d
Update golangci-lint
Kay-Zee Feb 8, 2023
85dcdc7
Merge branch 'master' into rbtz/go1.20
SaveTheRbtz Feb 9, 2023
a0e3434
replace math/rand in production code - remove depriacted functions in…
Mar 15, 2023
93b68dc
update crypto math/rand usage
Mar 16, 2023
92ea84b
merge master branch
Mar 16, 2023
9d19b51
add tests for new package utils/rand
Mar 16, 2023
92fdcae
fix more errors
Mar 16, 2023
5e2b255
more linter errors
Mar 16, 2023
5b58584
fix more linter errors
Mar 16, 2023
2b00584
1.20 never ending linter errors
Mar 16, 2023
2d315c0
merge master
Mar 16, 2023
5d1a431
update bootstrap beacon KG and fix integration errors
Mar 16, 2023
6f76e40
Revert "Update golangci-lint"
Mar 16, 2023
cd908b8
Revert "Go 1.20"
Mar 16, 2023
12f6663
more fixing
Mar 16, 2023
4377369
log error in pool random ejection and fallback to LRU
Mar 16, 2023
1533c2d
Merge branch 'master' into tarak/go1.20-rand-prep
tarakby Mar 16, 2023
4919ba3
fix import order
Mar 16, 2023
07dce62
fix a new issue after merging master
Mar 17, 2023
77d83f0
improve unsafeRandom test for randomness and add determinicity test
Mar 17, 2023
361bbb7
merge master
Apr 22, 2023
910c04c
remove added spaces
Apr 22, 2023
0bebc15
minor cleanups
Apr 22, 2023
eb36425
merge master and fix conflicts
May 16, 2023
cb1db83
fix merging bug
May 16, 2023
f726c2e
merge master
Jun 1, 2023
0a0e4e5
remove one more usage of math/rand in engine/execution
Jun 9, 2023
db663e3
avoid using math/rand in NewExponentialBackoff
Jun 9, 2023
ac5302b
Merge branch 'master' into tarak/go1.20-rand-prep
tarakby Jun 9, 2023
908212f
clean up and comment updates
Jun 9, 2023
5a1bca3
use Throw to bubble the error up
tarakby Jun 13, 2023
3c2f5bd
fix logic change in identity list sample
Jun 13, 2023
14df98f
fix returning before unlock and add warning log when randomness fails
Jun 13, 2023
4820fa6
add check for math/rand usage in production code
Jun 22, 2023
0112864
merge master
Jun 23, 2023
e3447dc
add exclude-dir so that GNU grep matches BSD grep for go-math-rand-check
Jun 23, 2023
54a6803
add tmate for remote debugging
Jul 7, 2023
d9ae9eb
merge master
Jul 7, 2023
3a3b8ba
fix merging bug
Jul 8, 2023
67b4390
add error description for newSource()
Jul 11, 2023
1f1a341
revert LRUEjector and remove random ejection fallback
Jul 12, 2023
183ce49
use fatal logging when system randomness fails
Jul 12, 2023
a2f5f4b
update grep command to work for different grep versions and remove tmate
Jul 12, 2023
8e21991
Merge branch 'master' into tarak/go1.20-rand-prep
tarakby Jul 14, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 2 additions & 4 deletions cmd/bootstrap/cmd/clusters.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ import (
"github.com/onflow/flow-go/model/flow/assignment"
"github.com/onflow/flow-go/model/flow/factory"
"github.com/onflow/flow-go/model/flow/filter"
"github.com/onflow/flow-go/utils/rand"
)

// Construct random cluster assignment with internal and partner nodes.
Expand Down Expand Up @@ -39,12 +38,11 @@ func constructClusterAssignment(partnerNodes, internalNodes []model.NodeInfo) (f
}

// shuffle both collector lists based on a non-deterministic algorithm
var err error
err = rand.Shuffle(uint(len(partners)), func(i, j uint) { partners[i], partners[j] = partners[j], partners[i] })
partners, err := partners.Shuffle()
if err != nil {
log.Fatal().Err(err).Msg("could not shuffle partners")
}
err = rand.Shuffle(uint(len(internals)), func(i, j uint) { internals[i], internals[j] = internals[j], internals[i] })
internals, err = internals.Shuffle()
if err != nil {
log.Fatal().Err(err).Msg("could not shuffle internals")
}
Expand Down
5 changes: 0 additions & 5 deletions cmd/scaffold.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ import (
"crypto/x509"
"errors"
"fmt"
"math/rand"
"os"
"runtime"
"strings"
Expand Down Expand Up @@ -1798,10 +1797,6 @@ func (fnb *FlowNodeBuilder) Build() (Node, error) {
}

func (fnb *FlowNodeBuilder) onStart() error {

// seed random generator
rand.Seed(time.Now().UnixNano())
Comment on lines -1777 to -1778
Copy link
Member

Choose a reason for hiding this comment

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

I think we should still leave this in. As of now, golang still follows the convention:

If Seed is not called, the generator behaves as if seeded by Seed(1).

which I don't think is desired.

Suggestion
Maybe including a TODO would be better:

	// TODO: up to and including golang 0.19, we need to seed the random generator
	// because go auto-seeds it Seed(1). This convention changes in golang 0.20, where `rand` will be auto-seeded randomly
	// see: https://pkg.go.dev/math/rand#Seed
	rand.Seed(time.Now().UnixNano())

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The purpose of all my PRs is to remove the use of math/rand completely from production. There is no need to initialize math/rand anymore since it's not used by the node software.


// init nodeinfo by reading the private bootstrap file if not already set
if fnb.NodeID == flow.ZeroID {
if err := fnb.initNodeInfo(); err != nil {
Expand Down
3 changes: 2 additions & 1 deletion consensus/hotstuff/signature/block_signer_decoder_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,8 @@ func (s *blockSignerDecoderSuite) Test_EpochTransition() {
blockView := s.block.Header.View
parentView := s.block.Header.ParentView
epoch1Committee := s.allConsensus
epoch2Committee := s.allConsensus.SamplePct(.8)
epoch2Committee, err := s.allConsensus.SamplePct(.8)
tarakby marked this conversation as resolved.
Show resolved Hide resolved
require.NoError(s.T(), err)

*s.committee = *hotstuff.NewDynamicCommittee(s.T())
s.committee.On("IdentitiesByEpoch", parentView).Return(epoch1Committee, nil).Maybe()
Expand Down
6 changes: 5 additions & 1 deletion consensus/integration/network_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -160,7 +160,11 @@ func (n *Network) publish(event interface{}, channel channels.Channel, targetIDs
// Engines attached to the same channel on other nodes. The targeted nodes are selected based on the selector.
// In this test helper implementation, multicast uses submit method under the hood.
func (n *Network) multicast(event interface{}, channel channels.Channel, num uint, targetIDs ...flow.Identifier) error {
targetIDs = flow.Sample(num, targetIDs...)
var err error
targetIDs, err = flow.Sample(num, targetIDs...)
if err != nil {
return fmt.Errorf("sampling failed: %w", err)
}
return n.submit(event, channel, targetIDs...)
}

Expand Down
5 changes: 4 additions & 1 deletion engine/access/rpc/backend/backend.go
Original file line number Diff line number Diff line change
Expand Up @@ -358,7 +358,10 @@ func executionNodesForBlockID(
}

// randomly choose upto maxExecutionNodesCnt identities
executionIdentitiesRandom := subsetENs.Sample(maxExecutionNodesCnt)
executionIdentitiesRandom, err := subsetENs.Sample(maxExecutionNodesCnt)
if err != nil {
return nil, fmt.Errorf("sampling failed: %w", err)
}

if len(executionIdentitiesRandom) == 0 {
return nil, fmt.Errorf("no matching execution node found for block ID %v", blockID)
Expand Down
5 changes: 4 additions & 1 deletion engine/access/rpc/backend/backend_transactions.go
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,10 @@ func (b *backendTransactions) chooseCollectionNodes(tx *flow.TransactionBody, sa
}

// select a random subset of collection nodes from the cluster to be tried in order
targetNodes := txCluster.Sample(sampleSize)
targetNodes, err := txCluster.Sample(sampleSize)
if err != nil {
return nil, fmt.Errorf("sampling failed: %w", err)
}

// collect the addresses of all the chosen collection nodes
var targetAddrs = make([]string, len(targetNodes))
Expand Down
6 changes: 5 additions & 1 deletion engine/common/requester/engine.go
Original file line number Diff line number Diff line change
Expand Up @@ -367,7 +367,11 @@ func (e *Engine) dispatchRequest() (bool, error) {
if len(providers) == 0 {
return false, fmt.Errorf("no valid providers available")
}
providerID = providers.Sample(1)[0].NodeID
id, err := providers.Sample(1)
if err != nil {
return false, fmt.Errorf("sampling failed: %w", err)
}
providerID = id[0].NodeID
}

// add item to list and set retry parameters
Expand Down
11 changes: 6 additions & 5 deletions engine/execution/computation/query/executor.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ import (
"context"
"encoding/hex"
"fmt"
"math/rand"
"strings"
"sync"
"time"
Expand All @@ -18,6 +17,7 @@ import (
"github.com/onflow/flow-go/model/flow"
"github.com/onflow/flow-go/module"
"github.com/onflow/flow-go/utils/debug"
"github.com/onflow/flow-go/utils/rand"
)

const (
Expand Down Expand Up @@ -71,7 +71,6 @@ type QueryExecutor struct {
vmCtx fvm.Context
derivedChainData *derived.DerivedChainData
rngLock *sync.Mutex
rng *rand.Rand
}

var _ Executor = &QueryExecutor{}
Expand All @@ -92,7 +91,6 @@ func NewQueryExecutor(
vmCtx: vmCtx,
derivedChainData: derivedChainData,
rngLock: &sync.Mutex{},
rng: rand.New(rand.NewSource(time.Now().UnixNano())),
}
}

Expand All @@ -115,8 +113,11 @@ func (e *QueryExecutor) ExecuteScript(
// TODO: this is a temporary measure, we could remove this in the future
if e.logger.Debug().Enabled() {
e.rngLock.Lock()
trackerID := e.rng.Uint32()
e.rngLock.Unlock()
defer e.rngLock.Unlock()
trackerID, err := rand.Uint32()
if err != nil {
return nil, fmt.Errorf("failed to generate trackerID: %w", err)
}

trackedLogger := e.logger.With().Hex("script_hex", script).Uint32("trackerID", trackerID).Logger()
trackedLogger.Debug().Msg("script is sent for execution")
Expand Down
7 changes: 5 additions & 2 deletions engine/verification/requester/requester.go
Original file line number Diff line number Diff line change
Expand Up @@ -331,8 +331,11 @@ func (e *Engine) requestChunkDataPack(request *verification.ChunkDataPackRequest
}

// publishes the chunk data request to the network
targetIDs := request.SampleTargets(int(e.requestTargets))
err := e.con.Publish(req, targetIDs...)
targetIDs, err := request.SampleTargets(int(e.requestTargets))
if err != nil {
return fmt.Errorf("target sampling failed: %w", err)
}
err = e.con.Publish(req, targetIDs...)
if err != nil {
return fmt.Errorf("could not publish chunk data pack request for chunk (id=%s): %w", request.ChunkID, err)
}
Expand Down
2 changes: 0 additions & 2 deletions integration/tests/consensus/inclusion_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -43,8 +43,6 @@ func (is *InclusionSuite) SetupTest() {
is.log = unittest.LoggerForTest(is.Suite.T(), zerolog.InfoLevel)
is.log.Info().Msgf("================> SetupTest")

// seed random generator

// to collect node confiis...
var nodeConfigs []testnet.NodeConfig

Expand Down
13 changes: 0 additions & 13 deletions model/flow/address_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ import (
"math/bits"
"math/rand"
"testing"
"time"

"github.com/onflow/cadence"
"github.com/onflow/cadence/runtime/common"
Expand Down Expand Up @@ -167,9 +166,6 @@ func testAddressConstants(t *testing.T) {
const invalidCodeWord = uint64(0xab2ae42382900010)

func testAddressGeneration(t *testing.T) {
// seed random generator
rand.Seed(time.Now().UnixNano())

// loops in each test
const loop = 50

Expand Down Expand Up @@ -260,9 +256,6 @@ func testAddressGeneration(t *testing.T) {
}

func testAddressesIntersection(t *testing.T) {
// seed random generator
rand.Seed(time.Now().UnixNano())

// loops in each test
const loop = 25

Expand Down Expand Up @@ -329,9 +322,6 @@ func testAddressesIntersection(t *testing.T) {
}

func testIndexFromAddress(t *testing.T) {
// seed random generator
rand.Seed(time.Now().UnixNano())

// loops in each test
const loop = 50

Expand Down Expand Up @@ -370,9 +360,6 @@ func testIndexFromAddress(t *testing.T) {
}

func TestUint48(t *testing.T) {
// seed random generator
rand.Seed(time.Now().UnixNano())

const loop = 50
// test consistensy of putUint48 and uint48
for i := 0; i < loop; i++ {
Expand Down
21 changes: 12 additions & 9 deletions model/flow/identifier.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ import (
"encoding/binary"
"encoding/hex"
"fmt"
"math/rand"
"reflect"

"github.com/ipfs/go-cid"
Expand All @@ -16,6 +15,7 @@ import (
"github.com/onflow/flow-go/crypto/hash"
"github.com/onflow/flow-go/model/fingerprint"
"github.com/onflow/flow-go/storage/merkle"
"github.com/onflow/flow-go/utils/rand"
)

const IdentifierLen = 32
Expand Down Expand Up @@ -179,21 +179,24 @@ func CheckConcatSum(sum Identifier, fps ...Identifier) bool {
return sum == computed
}

// Sample returns random sample of length 'size' of the ids
// [Fisher-Yates shuffle](https://en.wikipedia.org/wiki/Fisher-Yates_shuffle).
func Sample(size uint, ids ...Identifier) []Identifier {
// Sample returns non-deterministic random sample of length 'size' of the ids
func Sample(size uint, ids ...Identifier) ([]Identifier, error) {
n := uint(len(ids))
dup := make([]Identifier, 0, n)
dup = append(dup, ids...)
// if sample size is greater than total size, return all the elements
if n <= size {
return dup
return dup, nil
}
for i := uint(0); i < size; i++ {
j := uint(rand.Intn(int(n - i)))
dup[i], dup[j+i] = dup[j+i], dup[i]
swap := func(i, j uint) {
dup[i], dup[j] = dup[j], dup[i]
}
return dup[:size]

err := rand.Samples(n, size, swap)
if err != nil {
return nil, fmt.Errorf("generating randoms failed: %w", err)
}
return dup[:size], nil
}

func CidToId(c cid.Cid) (Identifier, error) {
Expand Down
10 changes: 1 addition & 9 deletions model/flow/identifierList.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ package flow

import (
"bytes"
"math/rand"
"sort"

"github.com/rs/zerolog/log"
Expand Down Expand Up @@ -103,15 +102,8 @@ func (il IdentifierList) Union(other IdentifierList) IdentifierList {
return union
}

// DeterministicSample returns deterministic random sample from the `IdentifierList` using the given seed
func (il IdentifierList) DeterministicSample(size uint, seed int64) IdentifierList {
rand.Seed(seed)
return il.Sample(size)
}

// Sample returns random sample of length 'size' of the ids
// [Fisher-Yates shuffle](https://en.wikipedia.org/wiki/Fisher–Yates_shuffle).
func (il IdentifierList) Sample(size uint) IdentifierList {
func (il IdentifierList) Sample(size uint) (IdentifierList, error) {
return Sample(size, il...)
}

Expand Down
3 changes: 1 addition & 2 deletions model/flow/identifierList_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ import (
"math/rand"
"sort"
"testing"
"time"

"github.com/stretchr/testify/require"

Expand All @@ -21,7 +20,7 @@ func TestIdentifierListSort(t *testing.T) {
var ids flow.IdentifierList = unittest.IdentifierListFixture(count)

// shuffles array before sorting to enforce some pseudo-randomness
rand.Seed(time.Now().UnixNano())

rand.Shuffle(ids.Len(), ids.Swap)

sort.Sort(ids)
Expand Down
14 changes: 9 additions & 5 deletions model/flow/identifier_test.go
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
package flow_test

import (
"crypto/rand"
"encoding/binary"
"encoding/json"
"fmt"
"math/rand"
"testing"

blocks "github.com/ipfs/go-block-format"
Expand Down Expand Up @@ -66,20 +66,23 @@ func TestIdentifierSample(t *testing.T) {

t.Run("Sample creates a random sample", func(t *testing.T) {
sampleSize := uint(5)
sample := flow.Sample(sampleSize, ids...)
sample, err := flow.Sample(sampleSize, ids...)
require.NoError(t, err)
require.Len(t, sample, int(sampleSize))
require.NotEqual(t, sample, ids[:sampleSize])
})

t.Run("sample size greater than total size results in the original list", func(t *testing.T) {
sampleSize := uint(len(ids) + 1)
sample := flow.Sample(sampleSize, ids...)
sample, err := flow.Sample(sampleSize, ids...)
require.NoError(t, err)
require.Equal(t, sample, ids)
})

t.Run("sample size of zero results in an empty list", func(t *testing.T) {
sampleSize := uint(0)
sample := flow.Sample(sampleSize, ids...)
sample, err := flow.Sample(sampleSize, ids...)
require.NoError(t, err)
require.Empty(t, sample)
})
}
Expand Down Expand Up @@ -131,7 +134,8 @@ func TestCIDConversion(t *testing.T) {

// generate random CID
data := make([]byte, 4)
rand.Read(data)
_, err = rand.Read(data)
require.NoError(t, err)
cid = blocks.NewBlock(data).Cid()

id, err = flow.CidToId(cid)
Expand Down
Loading