From 752144685b70ec79b28cbe332348f98fedf6e2c1 Mon Sep 17 00:00:00 2001 From: rigelrozanski Date: Tue, 21 May 2019 12:52:45 -0400 Subject: [PATCH 1/7] add trouble seed --- contrib/runsim/main.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contrib/runsim/main.go b/contrib/runsim/main.go index b8ceb2d11ee7..5d6ba9640724 100644 --- a/contrib/runsim/main.go +++ b/contrib/runsim/main.go @@ -25,7 +25,7 @@ var ( 3012, 4728, 37827, 981928, 87821, 891823782, 989182, 89182391, 11, 22, 44, 77, 99, 2020, 3232, 123123, 124124, 582582, 18931893, - 29892989, 30123012, 47284728, 7601778, + 29892989, 30123012, 47284728, 7601778, 8090485, } // goroutine-safe process map From 0f042ced1a0790df3ee44e7435471f249efa3630 Mon Sep 17 00:00:00 2001 From: rigelrozanski Date: Tue, 21 May 2019 17:42:01 -0400 Subject: [PATCH 2/7] currentStakeRoundUp is now always atleast currentStake + smallest-decimal-precision --- .pending/bugfixes/sdk/4383---currentStakeR | 1 + simapp/sim_test.go | 9 ++++--- types/decimal.go | 5 ++-- x/distribution/keeper/delegation.go | 15 ++++++++--- x/simulation/params.go | 3 --- x/simulation/simulate.go | 29 +++++++++++----------- 6 files changed, 37 insertions(+), 25 deletions(-) create mode 100644 .pending/bugfixes/sdk/4383---currentStakeR diff --git a/.pending/bugfixes/sdk/4383---currentStakeR b/.pending/bugfixes/sdk/4383---currentStakeR new file mode 100644 index 000000000000..1e38bc2baaec --- /dev/null +++ b/.pending/bugfixes/sdk/4383---currentStakeR @@ -0,0 +1 @@ +#4383 - currentStakeRoundUp is now always atleast currentStake + smallest-decimal-precision \ No newline at end of file diff --git a/simapp/sim_test.go b/simapp/sim_test.go index 640e3eec9e30..e2b7cbf8a6fd 100644 --- a/simapp/sim_test.go +++ b/simapp/sim_test.go @@ -49,6 +49,7 @@ var ( lean bool commit bool period int + onOperation bool // TODO Remove in favor of binary search for invariant violation ) func init() { @@ -61,15 +62,16 @@ func init() { flag.BoolVar(&lean, "SimulationLean", false, "lean simulation log output") flag.BoolVar(&commit, "SimulationCommit", false, "have the simulation commit") flag.IntVar(&period, "SimulationPeriod", 1, "run slow invariants only once every period assertions") + flag.BoolVar(&onOperation, "SimulateEveryOperation", false, "run slow invariants every operation") } // helper function for populating input for SimulateFromSeed func getSimulateFromSeedInput(tb testing.TB, w io.Writer, app *SimApp) ( testing.TB, io.Writer, *baseapp.BaseApp, simulation.AppStateFn, int64, - simulation.WeightedOperations, sdk.Invariants, int, int, bool, bool) { + simulation.WeightedOperations, sdk.Invariants, int, int, bool, bool, bool) { return tb, w, app.BaseApp, appStateFn, seed, - testAndRunTxs(app), invariants(app), numBlocks, blockSize, commit, lean + testAndRunTxs(app), invariants(app), numBlocks, blockSize, commit, lean, onOperation } func appStateFromGenesisFileFn(r *rand.Rand, accs []simulation.Account, genesisTimestamp time.Time, @@ -584,6 +586,7 @@ func TestAppStateDeterminism(t *testing.T) { 100, true, false, + false, ) appHash := app.LastCommitID().Hash appHashList[j] = appHash @@ -609,7 +612,7 @@ func BenchmarkInvariants(b *testing.B) { // 2. Run parameterized simulation (w/o invariants) _, err := simulation.SimulateFromSeed( b, ioutil.Discard, app.BaseApp, appStateFn, seed, testAndRunTxs(app), - []sdk.Invariant{}, numBlocks, blockSize, commit, lean, + []sdk.Invariant{}, numBlocks, blockSize, commit, lean, onOperation, ) if err != nil { fmt.Println(err) diff --git a/types/decimal.go b/types/decimal.go index 8fdad0ce06b9..86c9698c7d5e 100644 --- a/types/decimal.go +++ b/types/decimal.go @@ -46,8 +46,9 @@ func precisionInt() *big.Int { } // nolint - common values -func ZeroDec() Dec { return Dec{new(big.Int).Set(zeroInt)} } -func OneDec() Dec { return Dec{precisionInt()} } +func ZeroDec() Dec { return Dec{new(big.Int).Set(zeroInt)} } +func OneDec() Dec { return Dec{precisionInt()} } +func SmallestDec() Dec { return Dec{new(big.Int).Set(oneInt)} } // calculate the precision multiplier func calcPrecisionMultiplier(prec int64) *big.Int { diff --git a/x/distribution/keeper/delegation.go b/x/distribution/keeper/delegation.go index 01b3ffaa64f4..b17fdb8b242e 100644 --- a/x/distribution/keeper/delegation.go +++ b/x/distribution/keeper/delegation.go @@ -99,14 +99,23 @@ func (k Keeper) calculateDelegationRewards(ctx sdk.Context, val sdk.Validator, d currentStake := val.TokensFromShares(del.GetShares()) if stake.GT(currentStake) { // account for rounding errors due to stake being multiplied by slash fractions - currentStakeRoundUp := val.TokensFromSharesRoundUp(del.GetShares()) + currentStakeRoundUp := sdk.MaxDec( + val.TokensFromSharesRoundUp(del.GetShares()), + currentStake.Add(sdk.SmallestDec()), + ) if stake.Equal(currentStakeRoundUp) { stake = currentStake } else { + test := currentStake.Add(sdk.SmallestDec()) + fmt.Printf("debug test: %v\n", test) + fmt.Printf("\ndebug val.DelegatorShares: %v\n", val.GetDelegatorShares()) + fmt.Printf("debug val.Tokens: %v\n", val.GetTokens()) + fmt.Printf("debug del.Shares: %v\n", del.GetShares()) panic(fmt.Sprintf("calculated final stake for delegator %s greater than current stake"+ "\n\tfinal stake:\t%s"+ - "\n\tcurrent stake:\t%s", - del.GetDelegatorAddr(), stake, currentStake)) + "\n\tcurrent stake:\t%s"+ + "\n\t(currentRoundUp stake:\t%s)", + del.GetDelegatorAddr(), stake, currentStake, currentStakeRoundUp)) } } diff --git a/x/simulation/params.go b/x/simulation/params.go index 47b039af5c68..07a56170ccb0 100644 --- a/x/simulation/params.go +++ b/x/simulation/params.go @@ -13,9 +13,6 @@ const ( // Maximum time per block maxTimePerBlock int64 = 10000 - - // TODO Remove in favor of binary search for invariant violation - onOperation bool = false ) // TODO explain transitional matrix usage diff --git a/x/simulation/simulate.go b/x/simulation/simulate.go index 86e238a12d62..288c446a88d4 100644 --- a/x/simulation/simulate.go +++ b/x/simulation/simulate.go @@ -21,15 +21,15 @@ import ( // AppStateFn returns the app state json bytes, the genesis accounts, and the chain identifier type AppStateFn func(r *rand.Rand, accs []Account, genesisTimestamp time.Time) (appState json.RawMessage, accounts []Account, chainId string) -// Simulate tests application by sending random messages. -func Simulate(t *testing.T, app *baseapp.BaseApp, - appStateFn AppStateFn, ops WeightedOperations, - invariants sdk.Invariants, numBlocks, blockSize int, commit, lean bool) (bool, error) { - - time := time.Now().UnixNano() - return SimulateFromSeed(t, os.Stdout, app, appStateFn, time, ops, - invariants, numBlocks, blockSize, commit, lean) -} +//// Simulate tests application by sending random messages. +//func Simulate(t *testing.T, app *baseapp.BaseApp, +//appStateFn AppStateFn, ops WeightedOperations, +//invariants sdk.Invariants, numBlocks, blockSize int, commit, lean, onOperation bool) (bool, error) { + +//time := time.Now().UnixNano() +//return SimulateFromSeed(t, os.Stdout, app, appStateFn, time, ops, +//invariants, numBlocks, blockSize, commit, lean, onOperation) +//} // initialize the chain for the simulation func initChain( @@ -56,7 +56,7 @@ func SimulateFromSeed( tb testing.TB, w io.Writer, app *baseapp.BaseApp, appStateFn AppStateFn, seed int64, ops WeightedOperations, invariants sdk.Invariants, - numBlocks, blockSize int, commit, lean bool, + numBlocks, blockSize int, commit, lean, onOperation bool, ) (stopEarly bool, simError error) { // in case we have to end early, don't os.Exit so that we can run cleanup code. @@ -116,7 +116,7 @@ func SimulateFromSeed( blockSimulator := createBlockSimulator( testingMode, tb, t, w, params, eventStats.tally, invariants, ops, operationQueue, timeOperationQueue, - numBlocks, blockSize, logWriter, lean) + numBlocks, blockSize, logWriter, lean, onOperation) if !testingMode { b.ResetTimer() @@ -229,7 +229,7 @@ type blockSimFn func(r *rand.Rand, app *baseapp.BaseApp, ctx sdk.Context, func createBlockSimulator(testingMode bool, tb testing.TB, t *testing.T, w io.Writer, params Params, event func(string), invariants sdk.Invariants, ops WeightedOperations, operationQueue OperationQueue, timeOperationQueue []FutureOperation, - totalNumBlocks, avgBlockSize int, logWriter LogWriter, lean bool) blockSimFn { + totalNumBlocks, avgBlockSize int, logWriter LogWriter, lean, onOperation bool) blockSimFn { lastBlocksizeState := 0 // state for [4 * uniform distribution] blocksize := 0 @@ -274,10 +274,11 @@ func createBlockSimulator(testingMode bool, tb testing.TB, t *testing.T, w io.Wr queueOperations(operationQueue, timeOperationQueue, futureOps) if testingMode { if onOperation { + fmt.Fprintf(w, "\rSimulating... block %d/%d, operation %d/%d. ", + header.Height, totalNumBlocks, opCount, blocksize) eventStr := fmt.Sprintf("operation: %v", opMsg.String()) assertAllInvariants(t, app, invariants, eventStr, logWriter) - } - if opCount%50 == 0 { + } else if opCount%50 == 0 { fmt.Fprintf(w, "\rSimulating... block %d/%d, operation %d/%d. ", header.Height, totalNumBlocks, opCount, blocksize) } From 763a8fac44a09f505bc8342bea8938ef2bf25c14 Mon Sep 17 00:00:00 2001 From: rigelrozanski Date: Tue, 21 May 2019 17:43:02 -0400 Subject: [PATCH 3/7] remove unused code --- x/simulation/simulate.go | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/x/simulation/simulate.go b/x/simulation/simulate.go index 288c446a88d4..4fa5004d0c02 100644 --- a/x/simulation/simulate.go +++ b/x/simulation/simulate.go @@ -21,16 +21,6 @@ import ( // AppStateFn returns the app state json bytes, the genesis accounts, and the chain identifier type AppStateFn func(r *rand.Rand, accs []Account, genesisTimestamp time.Time) (appState json.RawMessage, accounts []Account, chainId string) -//// Simulate tests application by sending random messages. -//func Simulate(t *testing.T, app *baseapp.BaseApp, -//appStateFn AppStateFn, ops WeightedOperations, -//invariants sdk.Invariants, numBlocks, blockSize int, commit, lean, onOperation bool) (bool, error) { - -//time := time.Now().UnixNano() -//return SimulateFromSeed(t, os.Stdout, app, appStateFn, time, ops, -//invariants, numBlocks, blockSize, commit, lean, onOperation) -//} - // initialize the chain for the simulation func initChain( r *rand.Rand, params Params, accounts []Account, From 611116502eb87b8469bdfa15b59270dc436105f0 Mon Sep 17 00:00:00 2001 From: rigelrozanski Date: Tue, 21 May 2019 17:44:29 -0400 Subject: [PATCH 4/7] remove debugs --- x/distribution/keeper/delegation.go | 5 ----- 1 file changed, 5 deletions(-) diff --git a/x/distribution/keeper/delegation.go b/x/distribution/keeper/delegation.go index b17fdb8b242e..d7d0965e908a 100644 --- a/x/distribution/keeper/delegation.go +++ b/x/distribution/keeper/delegation.go @@ -106,11 +106,6 @@ func (k Keeper) calculateDelegationRewards(ctx sdk.Context, val sdk.Validator, d if stake.Equal(currentStakeRoundUp) { stake = currentStake } else { - test := currentStake.Add(sdk.SmallestDec()) - fmt.Printf("debug test: %v\n", test) - fmt.Printf("\ndebug val.DelegatorShares: %v\n", val.GetDelegatorShares()) - fmt.Printf("debug val.Tokens: %v\n", val.GetTokens()) - fmt.Printf("debug del.Shares: %v\n", del.GetShares()) panic(fmt.Sprintf("calculated final stake for delegator %s greater than current stake"+ "\n\tfinal stake:\t%s"+ "\n\tcurrent stake:\t%s"+ From 58f18a1d0ea3b97beb66daaaeb410df45ec3ec89 Mon Sep 17 00:00:00 2001 From: rigelrozanski Date: Wed, 22 May 2019 14:16:02 -0400 Subject: [PATCH 5/7] @alexanderbez comment --- x/distribution/keeper/delegation.go | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/x/distribution/keeper/delegation.go b/x/distribution/keeper/delegation.go index d7d0965e908a..e5754099cff7 100644 --- a/x/distribution/keeper/delegation.go +++ b/x/distribution/keeper/delegation.go @@ -98,12 +98,18 @@ func (k Keeper) calculateDelegationRewards(ctx sdk.Context, val sdk.Validator, d // we had arbitrary-precision rationals. currentStake := val.TokensFromShares(del.GetShares()) if stake.GT(currentStake) { - // account for rounding errors due to stake being multiplied by slash fractions - currentStakeRoundUp := sdk.MaxDec( - val.TokensFromSharesRoundUp(del.GetShares()), - currentStake.Add(sdk.SmallestDec()), - ) - if stake.Equal(currentStakeRoundUp) { + + // account for rounding inconsistencies between: + // currentStake: calculated as in staking with a single computation + // stake: calculated as an accumulation of stake + // calculations across validator's distribution periods + // these inconsistencies are due to differing order of operations which + // will inevitably have different accumulated rounding and may lead to + // the smallest decimal place being one greater in stake than + // currentStake. This amount of error is tolerated and corrected for, + // however any greater amount should be considered a breach in expected + // behaviour. + if stake.Equal(currentStake.Add(sdk.SmallestDec())) { stake = currentStake } else { panic(fmt.Sprintf("calculated final stake for delegator %s greater than current stake"+ From 711ad3a7b528e7b3586cadedcbae8507a1e9c132 Mon Sep 17 00:00:00 2001 From: rigelrozanski Date: Wed, 22 May 2019 16:23:34 -0400 Subject: [PATCH 6/7] compile fix --- x/distribution/keeper/delegation.go | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/x/distribution/keeper/delegation.go b/x/distribution/keeper/delegation.go index e5754099cff7..6ee22c478dcd 100644 --- a/x/distribution/keeper/delegation.go +++ b/x/distribution/keeper/delegation.go @@ -114,9 +114,8 @@ func (k Keeper) calculateDelegationRewards(ctx sdk.Context, val sdk.Validator, d } else { panic(fmt.Sprintf("calculated final stake for delegator %s greater than current stake"+ "\n\tfinal stake:\t%s"+ - "\n\tcurrent stake:\t%s"+ - "\n\t(currentRoundUp stake:\t%s)", - del.GetDelegatorAddr(), stake, currentStake, currentStakeRoundUp)) + "\n\tcurrent stake:\t%s", + del.GetDelegatorAddr(), stake, currentStake)) } } From d2d8c7e99b9b27f7dcb05a714f8619487350177c Mon Sep 17 00:00:00 2001 From: rigelrozanski Date: Fri, 24 May 2019 20:55:50 -0400 Subject: [PATCH 7/7] better comment, increase tolerance to 3 smallest decimal points --- x/distribution/keeper/delegation.go | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/x/distribution/keeper/delegation.go b/x/distribution/keeper/delegation.go index 6ee22c478dcd..223d16879767 100644 --- a/x/distribution/keeper/delegation.go +++ b/x/distribution/keeper/delegation.go @@ -99,17 +99,25 @@ func (k Keeper) calculateDelegationRewards(ctx sdk.Context, val sdk.Validator, d currentStake := val.TokensFromShares(del.GetShares()) if stake.GT(currentStake) { - // account for rounding inconsistencies between: + // Account for rounding inconsistencies between: // currentStake: calculated as in staking with a single computation // stake: calculated as an accumulation of stake // calculations across validator's distribution periods - // these inconsistencies are due to differing order of operations which + // These inconsistencies are due to differing order of operations which // will inevitably have different accumulated rounding and may lead to // the smallest decimal place being one greater in stake than - // currentStake. This amount of error is tolerated and corrected for, + // currentStake. When we calculated slashing by period, even if we + // round down for each slash fraction, it's possible due to how much is + // being rounded that we slash less when slashing by period instead of + // for when we slash without periods. In other words, the single slash, + // and the slashing by period could both be rounding down but the + // slashing by period is simply rounding down less, thus making stake > + // currentStake + // + // A small amount of this error is tolerated and corrected for, // however any greater amount should be considered a breach in expected // behaviour. - if stake.Equal(currentStake.Add(sdk.SmallestDec())) { + if stake.Equal(currentStake.Add(sdk.SmallestDec().MulInt64(3))) { stake = currentStake } else { panic(fmt.Sprintf("calculated final stake for delegator %s greater than current stake"+