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

Merged
merged 58 commits into from Oct 23, 2018

Conversation

Projects
None yet
3 participants
@cwgoes
Contributor

cwgoes commented Oct 16, 2018

Closes #1870
Closes #2368
Closes #2485

Add transactions (and maybe invariants?) for the new fee distribution module.

Standard checklist:

  • Linked to github-issue with discussion and accepted design OR link to spec that describes this work.
  • Wrote tests
  • Updated relevant documentation (docs/)
  • Added entries in PENDING.md with issue #
  • rereviewed Files changed in the github PR explorer

For Admin Use:

  • Added appropriate labels to PR (ex. wip, ready-for-review, docs)
  • Reviewers Assigned
  • Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

@cwgoes cwgoes changed the title from WIP: Assert supply conservation in simulation to WIP: Simulate transactions for fee distribution Oct 16, 2018

@rigelrozanski

This comment has been minimized.

Member

rigelrozanski commented Oct 16, 2018

Thanks for starting this Chris!

@codecov

This comment has been minimized.

codecov bot commented Oct 16, 2018

Codecov Report

Merging #2501 into develop will decrease coverage by 0.28%.
The diff coverage is 46.82%.

@@             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

cwgoes added some commits Oct 16, 2018

@cwgoes

This comment has been minimized.

Contributor

cwgoes commented Oct 16, 2018

I think we need to add some sanity checks:

  • For nonexistent delegation reward info on WithdrawDelegatorReward (maybe the delegator never delegated to the validator)
  • For nonexistent validator reward info on WithdrawValidatorRewardsAll (maybe the provided address isn't even a validator)
  • For nonexistent delegation reward info on WithdrawDelegatorRewardsAll (maybe the address has never delegated)

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.

@rigelrozanski

This comment has been minimized.

Member

rigelrozanski commented Oct 17, 2018

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.

@rigelrozanski

This comment has been minimized.

Member

rigelrozanski commented Oct 17, 2018

Beyond token conservation we could also maybe check accum invariants - however I don’t think we need to scope that in immediately

@cwgoes cwgoes referenced this pull request Oct 17, 2018

Closed

SDK 0.25 / Gaia-9000 release checklist #2220

21 of 23 tasks complete

@cwgoes cwgoes referenced this pull request Oct 18, 2018

Merged

WIP: Distribution fixes from simulation #2526

5 of 5 tasks complete

cwgoes added some commits Oct 18, 2018

@cwgoes cwgoes changed the title from WIP: Simulate transactions for fee distribution to R4R: Simulate transactions for fee distribution (and associated fixes) Oct 18, 2018

@cwgoes cwgoes changed the title from R4R: Simulate transactions for fee distribution (and associated fixes) to WIP: Simulate transactions for fee distribution (and associated fixes) Oct 18, 2018

@cwgoes

This comment has been minimized.

Contributor

cwgoes commented Oct 18, 2018

Hmm this passes now but I think we need to actually charge fees in simulation.

@cwgoes

This comment has been minimized.

Contributor

cwgoes commented Oct 18, 2018

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 x/stake and not dealing with the remainder correctly.

@rigelrozanski

This comment has been minimized.

Member

rigelrozanski commented Oct 19, 2018

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 x/stake and not dealing with the remainder correctly.

Can you highlight this change in with a commit?
@cwgoes

@cwgoes

This comment has been minimized.

Contributor

cwgoes commented Oct 23, 2018

Interestingly, when I disable slashing I get an even faster simulation failure (same panic on individual > total accum).

jaekwon added some commits Oct 23, 2018

@rigelrozanski rigelrozanski force-pushed the cwgoes/check-supply-in-simulation branch from 4df3260 to 7f43860 Oct 23, 2018

jaekwon and others added some commits Oct 23, 2018

Merge remote-tracking branch 'origin/jae/check-supply-in-simulation2'…
… into cwgoes/check-supply-in-simulation

@cwgoes cwgoes changed the title from WIP: Simulate transactions for fee distribution, simulate inflation, and fix a multitude of bugs discovered in the process of doing so to R4R: Simulate transactions for fee distribution, simulate inflation, and fix a multitude of bugs discovered in the process of doing so Oct 23, 2018

@cwgoes cwgoes added ready-for-review and removed wip labels Oct 23, 2018

@cwgoes

This comment has been minimized.

Contributor

cwgoes commented Oct 23, 2018

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).

@cwgoes

This comment has been minimized.

Contributor

cwgoes commented Oct 23, 2018

We should add accum invariants, but that can probably happen separately - #2573.

@rigelrozanski

woohoo! Few minor comments throughout - the improper use of periods is driving me a bit mad

Show resolved Hide resolved cmd/gaia/app/sim_test.go Outdated
Show resolved Hide resolved x/stake/keeper/keeper.go Outdated
Show resolved Hide resolved x/distribution/keeper/delegation.go
Show resolved Hide resolved x/distribution/keeper/validator.go
Show resolved Hide resolved x/distribution/types/delegator_info.go Outdated
Show resolved Hide resolved types/decimal.go Outdated
Show resolved Hide resolved x/distribution/types/validator_info.go Outdated
Show resolved Hide resolved x/slashing/keeper.go Outdated
Show resolved Hide resolved x/slashing/keeper.go
Show resolved Hide resolved x/stake/keeper/keeper.go Outdated

cwgoes added some commits Oct 23, 2018

@cwgoes cwgoes merged commit 60d188d into develop Oct 23, 2018

11 of 12 checks passed

codecov/patch 46.82% of diff hit (target 60.32%)
Details
ci/circleci: integration_tests Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: localnet Your tests passed on CircleCI!
Details
ci/circleci: setup_dependencies Your tests passed on CircleCI!
Details
ci/circleci: test_cover Your tests passed on CircleCI!
Details
ci/circleci: test_sim_gaia_fast Your tests passed on CircleCI!
Details
ci/circleci: test_sim_gaia_multi_seed Your tests passed on CircleCI!
Details
ci/circleci: test_sim_gaia_nondeterminism Your tests passed on CircleCI!
Details
ci/circleci: test_sim_modules Your tests passed on CircleCI!
Details
ci/circleci: upload_coverage Your tests passed on CircleCI!
Details
codecov/project 60.04% (-0.29%) compared to 617c3e8
Details

@cwgoes cwgoes deleted the cwgoes/check-supply-in-simulation branch Oct 23, 2018

@@ -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)

This comment has been minimized.

@jaekwon

jaekwon Oct 24, 2018

Contributor

probably shouldn't be calling this before k.SetValidator(). Also hooks being called from handlers is weird.

This comment has been minimized.

@cwgoes

cwgoes Oct 24, 2018

Contributor

I think this is the correct order, but we should change the name of the hook - #2538.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment