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

console: seed random number generator #2227

Merged
merged 1 commit into from
Feb 19, 2016
Merged

Conversation

bas-vk
Copy link
Member

@bas-vk bas-vk commented Feb 19, 2016

The otto VM uses math/rand as source for Math.random(). The math/rand package uses by default a constant seed which generates the exact same sequence of (pseudo) random floats each time the console starts. This is unexpected behavior and brittle in the context of the console. If somewhere in the code base (e.g. one of the dependencies) resets the seed for math/rand the otto vm will produce predictable pseudo random numbers.

This PR will inject a pseudo random generator into the otto vm that is only used by the otto vm and is initialized with a new seed each time the console starts.

It required an upgrade of the otto vm package.

Fix: #2181

@robotally
Copy link

Vote Count Reviewers
👍 1 @fjl
👎 0

Updated: Fri Feb 19 12:08:56 UTC 2016

@codecov-io
Copy link

Current coverage is 48.17%

Merging #2227 into develop will increase coverage by +0.05% as of 48f5ddd

Powered by Codecov. Updated on successful CI builds.

@karalabe
Copy link
Member

@bas-vk Please update your godep and repeat the dependency update. The new godep doesn't pull in test files of dependencies any more as they are really not needed.

@bas-vk
Copy link
Member Author

bas-vk commented Feb 19, 2016

@karalabe, check, thx.

@karalabe
Copy link
Member

Ah, much better, thanks :D

f := float64(src.Int63()) / (1 << 63)
if f == 1 {
goto again // resample; this branch is taken O(never)
}
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this si correct here, buy maybe I misunderstand.

src.Int63() should return an 63 bit random integer. That is, between 0 and 2^64 - 1. If you divide by 1 << 64 it should produce the intended results [0, 1). Whereas dividing by 1 << 63 actually produces quite a lot of values over 1.

@fjl
Copy link
Contributor

fjl commented Feb 19, 2016

Please don't build the logic to make random floats yourself.
You can setup a rand.Rand in runEventLoop, then just set r.Float64 as the source on the otto VM.

seed := make([]byte, 8)
crand.Read(seed)
r := rand.New(rand.NewSource(binary.BigEndian.Int64(seed)))
...
vm.SetRandomSource(r.Float64)

@karalabe It cannot be used concurrently.

@bas-vk bas-vk force-pushed the mathrandom branch 2 times, most recently from 132c9fc to 6b0d266 Compare February 19, 2016 11:47
@fjl
Copy link
Contributor

fjl commented Feb 19, 2016

I think we overlapped a bit with me editing the comment and you seeing it.
func () float64 { return r.Float64() } is equivalent to r.Float64, it's just more code.

@bas-vk bas-vk force-pushed the mathrandom branch 2 times, most recently from 35af4f3 to 6777531 Compare February 19, 2016 11:55
@fjl
Copy link
Contributor

fjl commented Feb 19, 2016

👍

@bas-vk
Copy link
Member Author

bas-vk commented Feb 19, 2016

@fjl, we did just pushed the change as you suggested. Far cleaner solution.

@karalabe
Copy link
Member

LGTM 👍

fjl added a commit that referenced this pull request Feb 19, 2016
console: seed random number generator
@fjl fjl merged commit c305005 into ethereum:develop Feb 19, 2016
@obscuren obscuren removed the review label Feb 19, 2016
maoueh pushed a commit to streamingfast/go-ethereum that referenced this pull request Apr 30, 2024
* core: use finalized block as the chain freeze indicator (ethereum#28683)
* core/rawdb: use max(finality, head-90k) as chain freezing threshold
* core: impl multi database for block data
* core: fix db inspect total size bug
* core: add tips for user who use multi-database
* core: adapt some cmd for multi-database
* core: adapter blockBlobSidecars
* core: fix freezer readHeader bug

---------
Co-authored-by: rjl493456442 <garyrong0905@gmail.com>
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.

6 participants