-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
R4R: Slashing, validator set, and governance simulation #1783
Conversation
Codecov Report
@@ Coverage Diff @@
## release/v0.24.0 #1783 +/- ##
===================================================
- Coverage 64.86% 63.79% -1.07%
===================================================
Files 115 113 -2
Lines 6862 6671 -191
===================================================
- Hits 4451 4256 -195
- Misses 2127 2132 +5
+ Partials 284 283 -1 |
cmd/gaia/app/sim_test.go
Outdated
@@ -125,7 +125,7 @@ func TestFullGaiaSimulation(t *testing.T) { | |||
}, | |||
[]simulation.RandSetup{}, | |||
[]simulation.Invariant{ | |||
allInvariants, | |||
simulation.PeriodicInvariant(allInvariants, 100, 0), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it make sense to break up these invariants more? i.e. bank invariant every 100, staking/slashing every time?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Possibly! Currently optimized for ease of changing periodicity...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In addition to what I previously suggested, maybe we should have a separate convenient debug
full simulation function which takes in the seed and then run the invariants every time. (i.e. I'd prefer if we had a very easy way to see exactly what caused that randomized error once we've found an error)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can search by bisection.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(and also have a way to run the invariants every time, sure)
Makefile
Outdated
@@ -136,7 +136,7 @@ test_sim_modules: | |||
|
|||
test_sim_gaia_fast: | |||
@echo "Running full Gaia simulation. This may take several minutes..." | |||
@go test ./cmd/gaia/app -run TestFullGaiaSimulation -SimulationEnabled=true -SimulationNumBlocks=200 -timeout 2h | |||
@go test ./cmd/gaia/app -run TestFullGaiaSimulation -SimulationEnabled=true -SimulationNumBlocks=200 -timeout 24h |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This shouldn't be fast. Perhaps make a test_sim_gaia_release, and have that run only on merges to master
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @cwgoes! Left some feedback on UX and non-determinism.
@echo "Running individual module simulations..." | ||
@go test $(PACKAGES_SIMTEST) | ||
|
||
test_sim_gaia_fast: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not able to send a successful SIGINT
to this command.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point! We should resolve this, but I strongly think we should get this PR merged, and fix this once its merged. Reviewing / rebasing / etc. is becoming a pain, and it will be much easier to parallelize work once its on develop. Mentioned your point about SIGINT #1924
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just send SIGKILL
!
@echo "Running full Gaia simulation. This may take several minutes..." | ||
@go test ./cmd/gaia/app -run TestFullGaiaSimulation -SimulationEnabled=true -SimulationNumBlocks=1000 -v -timeout 24h | ||
|
||
test_sim_gaia_slow: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See above.
) { | ||
log := fmt.Sprintf("Starting SimulateFromSeed with randomness created with seed %d", int(seed)) | ||
keys, addrs := mock.GeneratePrivKeyAddressPairs(numKeys) | ||
fmt.Printf("%s\n", log) | ||
keys, accs := mock.GeneratePrivKeyAddressPairs(numKeys) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is non-deterministic regardless of seed which results in different state hashes in each iteration.
Reference:
func GenPrivKey() PrivKeyEd25519 {
return genPrivKey(crypto.CReader())
}
/cc @ValarDragon
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in #2059 , but lets get this PR merged in first.
Can we remove testing this from CLI, and get this merged in? We can open a new PR for bug fixes in the interrim. I think just getting the code here merged is valuable so that we can work on this in a way that is easier to review. I'll write the fix for the non-determinism in a separate PR aimed at this one (with the intent of being rebased onto develop once this is merged in) |
|
Did you call |
Continuation of #1620.
This PR does the following:
x/gov
andx/slashing
Will include:
Want to complete before R4R/merge:
or interactive CLI!)app.Commit()
logic (avoid the actual multistore commit)To be completed in a followup PR:
edit: Wishlist moved to #1924.
Standard checklist:
docs/
)PENDING.md
cmd/gaia
andexamples/
For Admin Use: