-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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: Simulate transactions for fee distribution, simulate inflation, and fix a multitude of bugs discovered in the process of doing so #2501
Conversation
Thanks for starting this Chris! |
Codecov Report
@@ Coverage Diff @@
## develop #2501 +/- ##
===========================================
- Coverage 60.32% 60.04% -0.29%
===========================================
Files 150 150
Lines 8613 8684 +71
===========================================
+ Hits 5196 5214 +18
- Misses 3069 3116 +47
- Partials 348 354 +6 |
I think we need to add some sanity checks:
Also, when I comment out those three messages, supply conservation is broken: Running quick Gaia simulation. This may take several minutes...
=== RUN TestFullGaiaSimulation
Starting SimulateFromSeed with randomness created with seed 42
Starting the simulation from time Sun Jan 31 13:59:47 UTC 26140, unixtime 762734152787
Simulating... block 27/400, operation 200/203. expected loose tokens to equal total steak held by accounts - pool.LooseTokens: 9738, sum of account tokens: 9737
Too many logs to display, instead writing to simulation_log_2018-10-17 00:43:37.txt
--- FAIL: TestFullGaiaSimulation (2.88s)
util.go:111:
FAIL
FAIL github.com/cosmos/cosmos-sdk/cmd/gaia/app 2.899s
make: *** [Makefile:172: test_sim_gaia_fast] Error 1 I also fixed a bug in the simulation where power wasn't set correctly for last-commit, so we may not really have been simulating fee distribution at all before. |
All good points -> with regards to fee distribution conservation of tokens - this very well could be due to the rounding down of tokens when you withdraw (and you are due a non-integer amount). - one easy way to deal with this in the short term is to just donate any trimmings of tokens to the community tax pool - again this will be extremely small amounts as we're going to be dealing with nano-atoms here. With regards to sanity checks, yeah I guess some sanity checks are missing, but obviously we don't want to be panicking if the handler gets a bad message, just returning an error early on. |
Beyond token conservation we could also maybe check accum invariants - however I don’t think we need to scope that in immediately |
Hmm this passes now but I think we need to actually charge fees in simulation. |
OK, I've found more problems - after disabling rounding the supply (which the simulation used to do) it fails even without any of the new fee distribution logic at all. I think we might be rounding somewhare in |
Can you highlight this change in with a commit? |
…/cosmos-sdk into cwgoes/check-supply-in-simulation
Interestingly, when I disable slashing I get an even faster simulation failure (same panic on individual > total accum). |
4df3260
to
7f43860
Compare
… into cwgoes/check-supply-in-simulation
Cleaned up debugging prints, removed a few unnecessary changes, and updated PENDING.md to reflect what this PR changes. This has passed 400-block single seed sim (locally and CI) & 100-block multi-seed sim (ran locally). |
We should add accum invariants, but that can probably happen separately - #2573. |
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.
woohoo! Few minor comments throughout - the improper use of periods is driving me a bit mad
@@ -150,6 +148,7 @@ func handleMsgEditValidator(ctx sdk.Context, msg types.MsgEditValidator, k keepe | |||
return err.Result() | |||
} | |||
validator.Commission = commission | |||
k.OnValidatorModified(ctx, msg.ValidatorAddr) |
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.
probably shouldn't be calling this before k.SetValidator(). Also hooks being called from handlers is weird.
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 think this is the correct order, but we should change the name of the hook - #2538.
Closes #1870
Closes #2368
Closes #2485
Add transactions (and maybe invariants?) for the new fee distribution module.
Standard checklist:
docs/
)PENDING.md
with issue #Files changed
in the github PR explorerFor Admin Use: