Fix simulation bugs; Incorprates #2676 from Sunny #2677

jaekwon · 2018-11-04T09:13:37Z

Fixes issues as mentioned here: #2617

Targeted PR against correct branch (see CONTRIBUTING.md)
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)

Co-Authored-By: cwgoes <cwgoes@pluranimity.org>

…transubstantiated

…alidatorsByPower

R4R: Power Store Invariant

jaekwon · 2018-11-04T09:13:56Z

This includes #2676

cwgoes

A few questions, also I think we should add explanatory comments where a particular ordering has to be preserved or where we check a case that ought to be impossible.

x/stake/keeper/val_state_change.go

cwgoes · 2018-11-04T12:24:03Z

x/stake/keeper/delegation.go

+	// validator loses all tokens due to slashing.  In this case,
+	// make all future delegations invalid.
+	if validator.DelegatorShareExRate().IsZero() {
+		return sdk.ZeroDec(), sdk.ErrInsufficientCoins("cannot delegate to validators with zero tokens or ex-rate")


Odd error code, maybe ErrInvalidDelegation would be better

x/stake/handler.go

cwgoes · 2018-11-04T12:27:29Z

x/slashing/keeper.go

@@ -49,6 +49,12 @@ func (k Keeper) handleDoubleSign(ctx sdk.Context, addr crypto.Address, infractio
 		panic(fmt.Sprintf("Validator consensus-address %v not found", consAddr))
 	}

+	// Get validator.
+	validator := k.validatorSet.ValidatorByConsAddr(ctx, consAddr)
+	if validator == nil || validator.GetStatus() == sdk.Unbonded {


Hmm - if MaxEvidenceAge is equal to the unbonding period, shouldn't this case be impossible? Is this an off-by-one problem due to the next val set offset? If so can we add a comment about it?

++

I would replace // defensive with the actual comment describing what it's being defensive of.

Simulation doesn't take this into account, and Tendermint might break this assumption at some point.

cwgoes · 2018-11-04T12:36:23Z

Passes 100-block multi-seed simulation.

edit: Passes 500-block multi-seed simulation.

alexanderbez

Left some feedback. Mostly around keeper usage and adding comments on the defensive stuff 👍

cmd/gaia/app/app.go

alexanderbez · 2018-11-04T13:12:57Z

x/slashing/keeper.go

@@ -49,6 +49,12 @@ func (k Keeper) handleDoubleSign(ctx sdk.Context, addr crypto.Address, infractio
 		panic(fmt.Sprintf("Validator consensus-address %v not found", consAddr))
 	}

+	// Get validator.
+	validator := k.validatorSet.ValidatorByConsAddr(ctx, consAddr)
+	if validator == nil || validator.GetStatus() == sdk.Unbonded {


++

I would replace // defensive with the actual comment describing what it's being defensive of.

alexanderbez · 2018-11-04T13:15:44Z

x/stake/handler.go

+	// reset the intra-transaction counter
+	k.SetIntraTxCounter(ctx, 0)
+
+	// Calculate validator set changes


Suggested change

// Calculate validator set changes

// calculate validator set changes

alexanderbez · 2018-11-04T13:15:58Z

x/stake/handler.go

+	k.SetIntraTxCounter(ctx, 0)
+
+	// Calculate validator set changes
+	ValidatorUpdates = k.ApplyAndReturnValidatorSetUpdates(ctx)


Suggested change

ValidatorUpdates = k.ApplyAndReturnValidatorSetUpdates(ctx)

validatorUpdates = k.ApplyAndReturnValidatorSetUpdates(ctx)

alexanderbez · 2018-11-04T13:16:35Z

x/stake/handler.go

+	// Calculate validator set changes
+	ValidatorUpdates = k.ApplyAndReturnValidatorSetUpdates(ctx)
+
+	// Unbond all mature validators from the unbonding queue


Suggested change

// Unbond all mature validators from the unbonding queue

// unbond all mature validators from the unbonding queue

alexanderbez · 2018-11-04T13:18:45Z

x/stake/keeper/keeper.go

@@ -36,7 +36,7 @@ func NewKeeper(cdc *codec.Codec, key, tkey sdk.StoreKey, ck bank.Keeper, paramst
 }

 // Set the validator hooks
-func (k Keeper) WithHooks(sh sdk.StakingHooks) Keeper {
+func (k *Keeper) WithHooks(sh sdk.StakingHooks) *Keeper {


See comment above.

cmd/gaia/app/app.go

sunnya97 · 2018-11-05T01:14:13Z

cmd/gaia/app/app.go

@@ -109,38 +109,40 @@ func NewGaiaApp(logger log.Logger, db dbm.DB, traceStore io.Writer, baseAppOptio
 		app.cdc,
 		app.keyParams, app.tkeyParams,
 	)
-	app.stakeKeeper = stake.NewKeeper(
+	stakeKeeper := stake.NewKeeper(
 		app.cdc,
 		app.keyStake, app.tkeyStake,
 		app.bankKeeper, app.paramsKeeper.Subspace(stake.DefaultParamspace),


Shouldn't we change all passed around Keepers to be pointers? Or just save that for a seperate PR?

Lets consider it in the future for a separate PR... I'm not sure. I think for new keepers we do want pointers.

sunnya97 · 2018-11-05T01:16:23Z

x/distribution/keeper/hooks.go

@@ -38,11 +40,12 @@ func (k Keeper) onValidatorBonded(ctx sdk.Context, valAddr sdk.ValAddress) {
 	k.onValidatorModified(ctx, valAddr)
 }

-// XXX Consider removing this after debugging.
+// Sanity check, very useful!


Related to #2663?

Like maybe these sanity check so be run as an optional thing?

It's not super performance heavy, lets run it always for now.

sunnya97 · 2018-11-05T01:23:02Z

x/stake/keeper/validator.go

@@ -189,6 +190,9 @@ func (k Keeper) RemoveValidator(ctx sdk.Context, address sdk.ValAddress) {
 	if !found {
 		return
 	}
+	if validator.Status != sdk.Unbonded {
+		panic("Cannot call RemoveValidator on bonded or unbonding validators")


codecov · 2018-11-05T05:15:58Z

Codecov Report

Merging #2677 into develop will increase coverage by 0.27%.
The diff coverage is 28%.

@@             Coverage Diff             @@
##           develop    #2677      +/-   ##
===========================================
+ Coverage    58.16%   58.44%   +0.27%     
===========================================
  Files          153      152       -1     
  Lines         9505     9472      -33     
===========================================
+ Hits          5529     5536       +7     
+ Misses        3620     3561      -59     
- Partials       356      375      +19

rigelrozanski · 2018-11-06T14:18:33Z

Thanks for this!

@rigelrozanski

* Back to 50 initially bonded * Add query deposits cmds * Update test * Update PENDING.md * Back to on-operation=false, update PENDING.md * Remove unnecessary change, clarify amt in sim_test.go * Cleanup, additional item in PENDING.md * Update PENDING.md Co-Authored-By: alessio <quadrispro@ubuntu.com> * Update client/keys/utils.go Co-Authored-By: alessio <quadrispro@ubuntu.com> * update depositer addr * Address @rigelrozanski comments * Make linter happy * Update PENDING.md * Remove shorthand * Make "multi" const * Store last total power as sdk.Int, not sdk.Dec * Merge PR cosmos#2553: Renamed msg.Name() and msg.Type() to msg.Type() and msg.Route() * Fix stupid mistake * s/number/weight/ * LastValidatorPower is also an Int * Merge PR cosmos#2462: Add swagger-ui for gov, stake and slashing * 'make format' * Move PENDING to CHANGELOG * Linkify changelog * Fix db write perm * Additional cleanup * Remove logs from prior debugging * Merge PR cosmos#2599 from cosmos/jae/dist_refactor * Rename Pool -> DelRewards; PoolCommission -> ValCommision * FeePool.Pool -> FeePool.ValPool * WithdrawalHeight->DelPoolWithdrawalHeight * OnValidatorBeginUnbonding * Caught the bug's tail * Update vi.FeePoolWithdrawalHeight upon bonding * Fix staking slashUnbondingDelegation bug; fixes simulator failure cosmos#9 * Merge PR cosmos#2597: Add distribution accum invariants * PENDING.md => CHANGELOG.md * Manually linkify * Manually fix some links * Docs fixes in progress while running through the release process * More docs fixes * '--voter' is no longer required * Rectify validator setup documentation * Merge PR cosmos#2596: Cmds for validator unbondings and redelegations * Make simulation use a transition matrix for block size This enables simulating periods of high load, and periods of low to no load. (low load because future ops will still terminate in that time frame) * address bez's comments * fix flags in docs, closes cosmos#2530 * Merge PR cosmos#2616: Block redelegations to the same validator * Merge PR cosmos#2623: Speedup simulator by switching to goleveldb Due to requiring app.Commit() at the moment, golevel db is significantly faster than a memdb * fix block offsets in printing simulation block number * Merge PR cosmos#2644: Simulation: Print last block when there is an error There was an off by one error in the log printing function previously * Merge PR cosmos#2642: Add todo diagrams * Various sign command improvements - Exit with error if the user is attempting to sign with a key whose address is not among those who are expected to sign the transaction. - Add --print-signature-only to output only the generated signature. * Check sanity of signatures and report errors when run with --print-sigs * Improve errors reporting * Improve online docs * Refresh PENDING.md * Find better name for --print-signature-only * Fix integration tests * Validate --name * Fix integration tests * s/--print-sigs/--validate-signatures/ * s/--sig-only/--signature-only/ * Docs updated * Update PENDING.md * Rename append, it's go builtin * Set success = false when it fails * Apply suggestions from bez * Nest switches * Fix rebase * Document what --validate-signatures does * perform minor doc and function cleanup * move typedef * Merge PR cosmos#2614: Configurable Bech32 prefix for SDK users * Merge PR cosmos#2643: AppendTag function usage error. append elements do not work * simulation: Make validator choice use validator set This also had to change the default seed, since with the previous one it actually got into a state where there were no validators left bonded, lol. This also changes Unbond msgs from failing with almost 100% probability to now only failing with 33% probability. Thus more of the state machine is getting tested! * Update changelog * Merge PR cosmos#2657: Fix config.js * Merge PR cosmos#2589: Update Vesting Spec * Merge PR cosmos#2656: Revert read-only leveldb database * Revert read-only leveldb database Waiting on a fix for syndtr/goleveldb#240. * Update client/keys/utils.go * Include DNS alt name in certificate Closes: cosmos#2664 * Gaialite signal handling is broken, repair it * Merge PR cosmos#2665: simulation: Remove header from Invariant This got introduced recently, but wasn't actually needed, hence the reversion * Merge PR cosmos#2653: Add benchmark for get and set account * Fix test * Refactor TrapSignal * Fix lint * enforcing @jaekwon mergemaster * added querier redelegation * added validatorDelegations querier endpoint * LCD and CLI * cli fixes * removed redelegation stuff * address other comments * rebased * addressed comments * Make the simulator create the new comission rate sensibly * Update to TM v0.26.0 - Part I (cosmos#2679) * Update to TM v0.26.0 * Bez/tm0.26 update pt 2 redux (cosmos#2684) * Update to TM v0.26.0 * Update TODOs * Proof and verification updates * Fix linting * Fix key path creation * Temporarily fix tendermint revision to make tests pass * Fix merge conflict bug; Update PENDING * New genesis workflow (cosmos#2602) New genesis workflow: * `gaiad init` is now used to generate an empty `genesis.json`. * Genesis accounts need to be populated manually before running `gaiad collect-gentxs`. * This should support starfish too, see cosmos#2615 for more info. * Closes: cosmos#2596 cosmos#2615 * Validate validator address and address against respective account ex ante * Fix local testnet failures * New genesis tests * Run make format * Add --pubkey flag * gaiad collect-gentxs takes no args * Simulation improvements (logging fix, random genesis parameters) (cosmos#2617) * Print out initial update on every block * Randomize simulation parameters * Randomize initial liveness weightings * Randomize genesis parameters * fixed power store invariant * IterateValidatorsBonded -> IterateBondedValidatorsByPower * WriteValidators uses IterateLastValidators rather than IterateBondedValidatorsByPower * fixed democoin interface Closes cosmos#2556 Closes cosmos#2396 Via cosmos#2671: closes cosmos#2669 closes cosmos#2670 closes cosmos#2620 Offshoot issues: cosmos#2618 cosmos#2619 cosmos#2620 cosmos#2661 * Fix simulation bugs; Incorprates cosmos#2676 from Sunny (cosmos#2677) * Fix simulation bugs; Incorprates cosmos#2676 from Sunny * Address review feedback; Update PENDING * 'make format' * Revert "enforcing @jaekwon mergemaster" This reverts commit 15c2093. * Update x/stake/client/rest/query.go Co-Authored-By: sunnya97 <sunnya97@gmail.com> * addressed fede's comment * Switch gov proposal-queues to use iterators (cosmos#2638) * switched gov proposals queue to use iterators * update gov spec * update proposal.Equal * Amino api change * switched proposalID to uint64 * renamed Gov Procedures to Params * s/ActiveProposalQueueProposalKey/KeyActiveProposalQueueProposal/g * numLatestProposals -> Limit * fixed staking invariant breakage because of gov deposits * Send deposits to DepositedCoinsAccAddr or BurnedDepositCoinsAccAddr * Add general merkle absence proof (also for empty substores) (cosmos#2685) * Fix coins.IsLT() impl (cosmos#2686) * Fix coins.IsLT() impl * Fix coin.IsLT() impl * Coins.IsLT -> Coins.IsAllLT etc * Update testnet to use canonical genesis time (cosmos#2692) * Update testnet to use canonical genesis time * Fix linting in genesis test * Do not allow nil values to be set in CacheKVStore (cosmos#2708) * Do not allow nil values to be set in CacheKVStore * Makefile OS compatibility update * Merge PR cosmos#2714: Add commission data to MsgCreateValidator signature bytes * PENDING => CHANGELOG * Linkify changelog * Cleanup bank keeper * whitespacing * rand utile ... * moving stuff around a bit, trying to get rid of types * reorganize more * rename ambig naming of queueOperations * minimizing indentation * fix some duplicate to get passing * Address style comments * Reorganize CLI command structure. Fixes cosmos#2575 * Fix missing flags issue * Address linting issues * Fix gobash CLI testing * Fix typo * Cross-compiling get_tools Makefile added * operations functions * assertAllInvarients changes, Operation reorg * mock tendermint * util cleanup * event stats object, more general cleanup * compiling * pending * Removed comment from Makefile as per bez's request * val comments * Address PR comments * Update cmd/gaia/cmd/gaiacli/main.go Co-Authored-By: jackzampolin <jack.zampolin@gmail.com> * PENDING * Fix state export/import, add to CI (cosmos#2690) * Update slashing import/export * More slashing.WriteGenesis * Add test import/export to CI * Store equality comparison. * Fix validator bond intra-tx counter * Set timeslices for unbonding validators * WriteGenesis => ExportGenesis * Delete validators from unbonding queue when re-bonded * Hook for validator deletion, fix staking genesis tests * Merge 0.26.0 back to develop (cosmos#2718) * PENDING => CHANGELOG * Linkify changelog * Merge PR cosmos#2716: Temporarily disable gaia lite insecure mode * TODO: need to update CHANGELOG w/ import-export PR cosmos#2690 * Update CHANGELOG/PENDING for straggling PR cosmos#2690 * Add small utility to add account to genesis.json after gaiad init * Update CHANGELOG.md * s/WriteGenesisFile/ExportGenesisFile/ * Update PENDING.md * Add --chain-id to testnet command * Address remaining comments from cosmos#2690 * Update PENDING.md * add back in PeriodicInvariant * Linter fix * Fix TimeoutCommit (cosmos#2743) * Fix TimeoutCommit to 5 seconds instead of whatever it was before which was too short. * Gaia-9000: Update to TM 0.26.1-rc2 (cosmos#2753) * Update to tm 0.26.1-rc2 to fix prometheus issue and node disconnect issue. * Gaia-9000: Update to TM 0.26.1-rc3 -- pex SeedMode fix * fix typo I think it might be a spelling mistake * Slight distribution spec cleanup * More cleanup * use defer * Use correct Bech32 prefix for show-address command (cosmos#2746) * Use consensus address bech32 prefix * Update show-address CLI description * Generate random moniker when missing * Update moniker prefix * Require moniker instead of generating a random one * update to tendermint v0.26.1 * Fix test coverage * Correctly set return code * Fix date to be cross platform * Merge PR cosmos#2752: Don't hardcode bondable denom * R4R: Fix unbonding command flow (cosmos#2727) * Fix required flag * Fix redelegation command * Add pending entry * update swagger.yaml * use newQuery...Params * Link to issue * Fix DiffKVStore * Address PR review * Working on stake import/export * Only apply validator set updates on initial genesis * Clarify comment * Fix failing test * add back in CLI command after rebase * Fix CLI tests * update to amino 0.14.1 * pending * R4R: Query Gov Params (cosmos#2576) * gov query params * Merge PR cosmos#2744: Fix Makefile targets dependencies * Fix Makefile targets dependencies * Remove unnecessary build deps from install targets * Create a rule for each tool * Don't dep test_lint on tools * Update docs/spec/distribution/overview.md Co-Authored-By: alexanderbez <alexanderbez@users.noreply.github.com> * Update docs/spec/distribution/overview.md Co-Authored-By: alexanderbez <alexanderbez@users.noreply.github.com> * Update docs/spec/distribution/overview.md Co-Authored-By: alexanderbez <alexanderbez@users.noreply.github.com> * Update overview.md * Documentation Structure Change and Cleanup (cosmos#2808) * Update docs/sdk/clients.md * organize ADR directory like tendermint * docs: move spec-proposals into spec/ * remove lotion, moved to website repo * move getting-started to cosmos-hub, and voyager to website * docs: move lite/ into clients/lite/ * move introduction/ content to website repo * move resources/ content to website repo * mv sdk/clients.md to clients/clients.md * mv validators to cosmos-hub/validators * move deprecated sdk/ content to _attic * sdk/modules.md is duplicate with modules/README.md * consolidate remianing sdk/ files into a single sdk.md * move examples/ to docs/examples/ * mv docs/cosmos-hub to docs/gaia * Add keys/accounts section to localnet docs * Bring back banner (cosmos#2814) * Build docs in CircleCI (cosmos#2810) * error checking the API call * added docs build trigger to circleci job * Update contributing.md with new merge policy (cosmos#2789) * Update contribuiting.md with new merge policy * deleted obsolete file (cosmos#2817)

cwgoes and others added 24 commits October 28, 2018 23:08

Print out initial update on every block

4e44ddf

Randomize simulation parameters

38ceea1

Randomize initial liveness weightings

4e48217

Randomize genesis parameters

46e6b40

Update PENDING.md

3884b13

Avoid dividing-by-zero

991573e

More simulation seeds, more blocks in CI version

f5918db

Update cmd/gaia/app/sim_test.go

e2aa3e9

Co-Authored-By: cwgoes <cwgoes@pluranimity.org>

Update cmd/gaia/app/sim_test.go

01c235d

Co-Authored-By: cwgoes <cwgoes@pluranimity.org>

Update cmd/gaia/app/sim_test.go

f13f602

Co-Authored-By: cwgoes <cwgoes@pluranimity.org>

Merge remote-tracking branch 'origin/develop' into cwgoes/simulation-…

1868ee3

…transubstantiated

Merge tag 'v0.25.0' into develop

a84cf4f

Merge branch 'develop' into cwgoes/simulation-transubstantiated

fb0f4a4

Address comments

d9907cc

Minimize variable passing

f695a66

fixed power store invariant

39165c9

PENDING

3cf6687

IterateValidatorsBonded -> IterateBondedValidatorsByPower

42eb4e2

WriteValidators uses IterateLastValidators rather than IterateBondedV…

888c061

…alidatorsByPower

fixed democoin interface

13d933f

fixed comment

191a732

Merge PR #2671: Power Store Invariant

559ebb7

R4R: Power Store Invariant

Merge branch 'develop' into cwgoes/simulation-transubstantiated

7b10ac4

Fix simulation bugs; Incorprates #2676 from Sunny

4068d8a

jaekwon mentioned this pull request Nov 4, 2018

R4R: Fix premature RemoveValidator bug #2676

Closed

5 tasks

cwgoes reviewed Nov 4, 2018

View reviewed changes

alexanderbez reviewed Nov 4, 2018

View reviewed changes

alexanderbez added the game-of-stakes label Nov 4, 2018

alexanderbez added C:Simulations wip labels Nov 4, 2018

sunnya97 reviewed Nov 5, 2018

View reviewed changes

cmd/gaia/app/app.go Show resolved Hide resolved

sunnya97 reviewed Nov 5, 2018

View reviewed changes

Address review feedback; Update PENDING

ace50be

jaekwon changed the base branch from cwgoes/simulation-transubstantiated to develop November 5, 2018 05:59

jaekwon changed the title ~~WIP: Fix simulation bugs; Incorprates #2676 from Sunny~~ Fix simulation bugs; Incorprates #2676 from Sunny Nov 5, 2018

Merge branch 'develop' into jae/simulation-transubstantiated

95cf0c3

jaekwon merged commit 336415b into develop Nov 5, 2018

cwgoes deleted the jae/simulation-transubstantiated branch November 5, 2018 10:37

This was referenced Nov 5, 2018

WIP: Distr-simulation OnValidatorPowerDidChange panic fix #2632

Closed

Simulation Failure: "should not be slashing unbonded validator" #2661

Closed

snyk-bot mentioned this pull request Jul 6, 2021

[Snyk] Upgrade @vuepress/plugin-google-analytics from 1.3.1 to 1.8.2 terrorizer1980/cosmos-sdk#2

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix simulation bugs; Incorprates #2676 from Sunny #2677

Fix simulation bugs; Incorprates #2676 from Sunny #2677

jaekwon commented Nov 4, 2018 •

edited

Loading

jaekwon commented Nov 4, 2018

cwgoes left a comment

cwgoes Nov 4, 2018

cwgoes Nov 4, 2018

alexanderbez Nov 4, 2018

jaekwon Nov 5, 2018

cwgoes commented Nov 4, 2018 •

edited

Loading

alexanderbez left a comment

alexanderbez Nov 4, 2018

alexanderbez Nov 4, 2018

alexanderbez Nov 4, 2018

alexanderbez Nov 4, 2018

alexanderbez Nov 4, 2018

sunnya97 Nov 5, 2018

jaekwon Nov 5, 2018

sunnya97 Nov 5, 2018

jaekwon Nov 5, 2018

sunnya97 Nov 5, 2018

codecov bot commented Nov 5, 2018 •

edited

Loading

rigelrozanski commented Nov 6, 2018

	// Calculate validator set changes
	// calculate validator set changes

	ValidatorUpdates = k.ApplyAndReturnValidatorSetUpdates(ctx)
	validatorUpdates = k.ApplyAndReturnValidatorSetUpdates(ctx)

	// Unbond all mature validators from the unbonding queue
	// unbond all mature validators from the unbonding queue

Fix simulation bugs; Incorprates #2676 from Sunny #2677

Fix simulation bugs; Incorprates #2676 from Sunny #2677

Conversation

jaekwon commented Nov 4, 2018 • edited Loading

jaekwon commented Nov 4, 2018

cwgoes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cwgoes commented Nov 4, 2018 • edited Loading

alexanderbez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Nov 5, 2018 • edited Loading

Codecov Report

rigelrozanski commented Nov 6, 2018

jaekwon commented Nov 4, 2018 •

edited

Loading

cwgoes commented Nov 4, 2018 •

edited

Loading

codecov bot commented Nov 5, 2018 •

edited

Loading