WIP: Jae/simulator improvements #2900
Conversation
jaekwon
requested review from
cwgoes,
ebuchman and
rigelrozanski
as code owners
|
Working on fixes to this PR in a separate one for clarity - #2901. |
| @@ -62,3 +62,29 @@ func RandTimestamp(r *rand.Rand) time.Time { | |||
| unixTime := r.Int63n(253373529600) | |||
| return time.Unix(unixTime, 0) | |||
| } | |||
|
|
|||
| // Derive a new rand deterministically from a rand. | |||
| // Unlike rand.New(rand.NewSource(seed)), the result is "more random" | |||
There was a problem hiding this comment.
Why is this "more random"? I don't really follow - is there a substantive difference between reinitializing a new PRNG and just stepping the same PRNG?
cc @ValarDragon
There was a problem hiding this comment.
This is not more random in a meaningful way. This doesn't even increase the repetitivity of the internal RNG state. (Since your combining multiple PRNG's with the same cycle size via xor)
This typically may be done to combine more entropy in some strange settings, however here that is also not the case since each seed is derived from the original r instance.
The rng used in golang is essentially a lagged fibonacci random number generator, with fixes that improve the seeding. (Source: https://groups.google.com/forum/#!topic/golang-nuts/RZ1G3_cxMcM) That has an insane cycle length (2^603) and is sufficiently random for our purposes. It is not suitable for things like monte-carlo simulations, but nor is this. It would make sense to switch to a monte-carlo safe rng in the future.
There was a problem hiding this comment.
The repetitivity/cycle-length doesn't matter, as you mentioned. A problem of the original math.Rand (with default Source) is that seed values that have the same remainder when divided by 2^31-1 generate the same pseudo-random sequence.. There are only two billion unique initial random sequences for a new Rand (with default Source)!
In order to make simulation deterministic while being able to make changes in the middle somewhere, we can't just re-use the same *Rand, because changes to the usage of the *Rand of a middle transaction causes rippling changes for all future operations. In some situations this can make debugging more difficult, because e.g. just because you mutate/mute/remove a single operation of a block doesn't mean that you're really testing the same scenario, as later operations of the same block (as an example) would be completely different.
The solution is to construct a tree of rands, as it's already done here... Here, we construct a predetermined set of operations, and then for further logic within an operation, we pass in a derived Rand. In the future we may want to derive third and fourth order rands in this way.
Once you're deriving a bunch of rands in this way, the limitation of 2^31-1 possible sequences does become meaningful. Sometimes you'll end up with the same seed for a derived rand, which means for that branch of rand usage, it's no longer random... the same sequence was used in another branch of simulation logic!
This DeriveRand() function is designed to solve this issue. The possibility of deriving the same rand sequence becomes negligible.
There was a problem hiding this comment.
Thanks for the clarification, I didn't know that! The comment should be amended from making the result more "random", which its not, to instead increasing the seed space.
However I still don't think this is worth doing. With there being 2**31 different valid seeds, by application of birthday collisions we'd have to do 54 thousand simulations with no change to the simulator in order for there to be a 50% probability of a single duplicate seed. We'd have to do 8.3 million simulations before there is a 1% probability each successive simulation was for nought. I frankly don't think we will do 54 thousand long simulations ever before changing the simulator, let alone 8.3 million. I don't think this is worth the performance trade-off, as speed of the simulator matters.
|
Merging #2901 since it seems non-controversial and I want to debug other sim fixes separately. |
* Fix upstream versions * Linter fix * Pin exact revisions, not branches * Rename 'MountStoresIAVL' to 'MountStores'
|
The normal Gaia simulation fails with seed 99: make test_sim_gaia_fastSimulating... block 14/500, operation 650/696. Invariants broken after BeginBlock
unexpected leftover validator pool coins: 23.2500000000
Too many logs to display, instead writing to simulation_log_2018-11-26 15:32:43.txt
--- FAIL: TestFullGaiaSimulation (3.68s)
invariants.go:27:
FAILEverything else seems OK, import/export & simulation after import work on all other seeds. Still running 500-block multi-seed sim. |
|
interestingly enough, this is not an accum issue, I ran the validator accum invariants when this fails and everything is proper. This suggests to me that this is possibly a rounding issue when we withdraw. |
| // make a tree and save it | ||
| func newTree(t *testing.T, db dbm.DB) (*iavl.MutableTree, CommitID) { | ||
| // make a tree with data from above and save it | ||
| func newAlohaTree(t *testing.T, db dbm.DB) (*iavl.MutableTree, CommitID) { |
There was a problem hiding this comment.
It's full of some random data include the word "aloha". Differentiates it from a "newTree".
|
figured out what the new bug is related to for seed |
Codecov Report
@@ Coverage Diff @@
## develop #2900 +/- ##
===========================================
- Coverage 56.72% 56.35% -0.38%
===========================================
Files 120 120
Lines 8396 8416 +20
===========================================
- Hits 4763 4743 -20
- Misses 3311 3351 +40
Partials 322 322 |
See the Tendermint PR for more details: tendermint/tendermint#2913
This PR makes the CacheKVStore.Iterator/ReverseIterator faster, and also makes simulation faster by faking the IAVL store mount and using a db adapter instead.
Requires corresponding dependency changes in Tendermint and IAVL.