From 2be19b9d339ff390765bde1aefa827c2a636e80b Mon Sep 17 00:00:00 2001 From: Hendrik Hofstadt Date: Wed, 19 Dec 2018 17:05:33 +0100 Subject: [PATCH 1/6] Merge PR #3163: Withdraw commission on self bond removal * Withdraw commission on self bond removal * Update PENDING.md --- PENDING.md | 2 ++ x/distribution/keeper/hooks.go | 4 ++++ x/distribution/keeper/validator.go | 17 +++++++++++++---- 3 files changed, 19 insertions(+), 4 deletions(-) diff --git a/PENDING.md b/PENDING.md index 4d87301f0..3ed6d0746 100644 --- a/PENDING.md +++ b/PENDING.md @@ -10,6 +10,8 @@ BREAKING CHANGES * SDK + * \#3163 Withdraw commission on self bond removal + * Tendermint diff --git a/x/distribution/keeper/hooks.go b/x/distribution/keeper/hooks.go index e9d31b8a2..613419846 100644 --- a/x/distribution/keeper/hooks.go +++ b/x/distribution/keeper/hooks.go @@ -87,6 +87,10 @@ func (k Keeper) onDelegationSharesModified(ctx sdk.Context, delAddr sdk.AccAddre // Withdrawal all validator distribution rewards and cleanup the distribution record func (k Keeper) onDelegationRemoved(ctx sdk.Context, delAddr sdk.AccAddress, valAddr sdk.ValAddress) { + if valAddr.Equals(sdk.ValAddress(delAddr)) { + feePool, commission := k.withdrawValidatorCommission(ctx, valAddr) + k.WithdrawToDelegator(ctx, feePool, delAddr, commission) + } k.RemoveDelegationDistInfo(ctx, delAddr, valAddr) } diff --git a/x/distribution/keeper/validator.go b/x/distribution/keeper/validator.go index 4ce0834a7..8d861fef8 100644 --- a/x/distribution/keeper/validator.go +++ b/x/distribution/keeper/validator.go @@ -42,6 +42,9 @@ func (k Keeper) RemoveValidatorDistInfo(ctx sdk.Context, valAddr sdk.ValAddress) if vdi.DelAccum.Accum.IsPositive() { panic("Should not delete validator with unwithdrawn delegator accum") } + if !vdi.ValCommission.IsZero() { + panic("Should not delete validator with unwithdrawn validator commission") + } store := ctx.KVStore(k.storeKey) store.Delete(GetValidatorDistInfoKey(valAddr)) @@ -119,6 +122,15 @@ func (k Keeper) takeValidatorFeePoolRewards(ctx sdk.Context, operatorAddr sdk.Va return nil } +func (k Keeper) withdrawValidatorCommission(ctx sdk.Context, operatorAddr sdk.ValAddress) (types.FeePool, types.DecCoins) { + valInfo := k.GetValidatorDistInfo(ctx, operatorAddr) + wc := k.GetWithdrawContext(ctx, operatorAddr) + valInfo, feePool, commission := valInfo.WithdrawCommission(wc) + k.SetValidatorDistInfo(ctx, valInfo) + + return feePool, commission +} + // withdrawal all the validator rewards including the commission func (k Keeper) WithdrawValidatorRewardsAll(ctx sdk.Context, operatorAddr sdk.ValAddress) sdk.Error { if !k.HasValidatorDistInfo(ctx, operatorAddr) { @@ -130,11 +142,8 @@ func (k Keeper) WithdrawValidatorRewardsAll(ctx sdk.Context, operatorAddr sdk.Va withdraw := k.withdrawDelegationRewardsAll(ctx, accAddr) // withdrawal validator commission rewards - valInfo := k.GetValidatorDistInfo(ctx, operatorAddr) - wc := k.GetWithdrawContext(ctx, operatorAddr) - valInfo, feePool, commission := valInfo.WithdrawCommission(wc) + feePool, commission := k.withdrawValidatorCommission(ctx, operatorAddr) withdraw = withdraw.Plus(commission) - k.SetValidatorDistInfo(ctx, valInfo) k.WithdrawToDelegator(ctx, feePool, accAddr, withdraw) return nil From b2c5ffeb3103fbc9bea9deb0416e7bb18956cf72 Mon Sep 17 00:00:00 2001 From: Christopher Goes Date: Mon, 17 Dec 2018 23:49:50 +0100 Subject: [PATCH 2/6] Fix linter errors (#3138) --- cmd/logjack/main.go | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/cmd/logjack/main.go b/cmd/logjack/main.go index c724ee4a1..cb4048814 100644 --- a/cmd/logjack/main.go +++ b/cmd/logjack/main.go @@ -12,6 +12,7 @@ import ( cmn "github.com/tendermint/tendermint/libs/common" ) +//nolint const Version = "0.0.2" const sleepSeconds = 1 // Every second const readBufferSize = 1024 // 1KB at a time @@ -24,7 +25,7 @@ func parseFlags() (headPath string, chopSize int64, limitSize int64, version boo flagSet.StringVar(&chopSizeStr, "chop", "100M", "Move file if greater than this") flagSet.StringVar(&limitSizeStr, "limit", "10G", "Only keep this much (for each specified file). Remove old files.") flagSet.BoolVar(&version, "version", false, "Version") - flagSet.Parse(os.Args[1:]) + flagSet.Parse(os.Args[1:]) //nolint chopSize = parseBytesize(chopSizeStr) limitSize = parseBytesize(limitSizeStr) return @@ -59,10 +60,10 @@ func main() { buf := make([]byte, readBufferSize) for { n, err := os.Stdin.Read(buf) - group.Write(buf[:n]) - group.Flush() + group.Write(buf[:n]) //nolint + group.Flush() //nolint if err != nil { - group.Stop() + group.Stop() //nolint if err == io.EOF { os.Exit(0) } else { From 9aaa1c2199838a7d4c8d332aff5902cde8b700d9 Mon Sep 17 00:00:00 2001 From: Christopher Goes Date: Wed, 19 Dec 2018 17:09:42 +0100 Subject: [PATCH 3/6] CHANGELOG => PENDING --- CHANGELOG.md | 13 ++++++++++--- PENDING.md | 2 -- 2 files changed, 10 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1ff420adb..536522779 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,8 +1,15 @@ # Changelog +## 0.29.0 + +BREAKING CHANGES + +* SDK + * [\#3163](https://github.com/cosmos/cosmos-sdk/issues/3163) Withdraw commission on self bond removal + ## 0.28.1 -BREAKNG CHANGES +BREAKING CHANGES * Gaia REST API (`gaiacli advanced rest-server`) * [lcd] [\#3045](https://github.com/cosmos/cosmos-sdk/pull/3045) Fix quoted json return on GET /keys (keys list) @@ -25,11 +32,11 @@ FEATURES IMPROVEMENTS * Gaia REST API (`gaiacli advanced rest-server`) - * \#2879, \#2880 Update deposit and vote endpoints to perform a direct txs query + * [\#2879](https://github.com/cosmos/cosmos-sdk/issues/2879), [\#2880](https://github.com/cosmos/cosmos-sdk/issues/2880) Update deposit and vote endpoints to perform a direct txs query when a given proposal is inactive and thus having votes and deposits removed from state. * Gaia CLI (`gaiacli`) - * \#2879, \#2880 Update deposit and vote CLI commands to perform a direct txs query + * [\#2879](https://github.com/cosmos/cosmos-sdk/issues/2879), [\#2880](https://github.com/cosmos/cosmos-sdk/issues/2880) Update deposit and vote CLI commands to perform a direct txs query when a given proposal is inactive and thus having votes and deposits removed from state. * Gaia diff --git a/PENDING.md b/PENDING.md index 3ed6d0746..4d87301f0 100644 --- a/PENDING.md +++ b/PENDING.md @@ -10,8 +10,6 @@ BREAKING CHANGES * SDK - * \#3163 Withdraw commission on self bond removal - * Tendermint From b76825ed84df6c2bbb700b9a1f0d7fd00005fd97 Mon Sep 17 00:00:00 2001 From: Christopher Goes Date: Tue, 18 Dec 2018 17:17:50 +0100 Subject: [PATCH 4/6] Merge PR #3148: Fix gaiad export * Add boolean for LoadLatestVersion * Update PENDING.md --- PENDING.md | 2 ++ client/lcd/test_helpers.go | 2 +- cmd/gaia/app/app.go | 10 ++++++---- cmd/gaia/app/app_test.go | 4 ++-- cmd/gaia/app/sim_test.go | 14 +++++++------- cmd/gaia/cmd/gaiad/main.go | 4 ++-- cmd/gaia/cmd/gaiareplay/main.go | 2 +- 7 files changed, 21 insertions(+), 17 deletions(-) diff --git a/PENDING.md b/PENDING.md index 4d87301f0..f70b51ac4 100644 --- a/PENDING.md +++ b/PENDING.md @@ -47,6 +47,8 @@ BUG FIXES * Gaia + * \#3148 Fix `gaiad export` by adding a boolean to `NewGaiaApp` determining whether or not to load the latest version + * SDK * Tendermint diff --git a/client/lcd/test_helpers.go b/client/lcd/test_helpers.go index 58b8426e5..e1272f173 100644 --- a/client/lcd/test_helpers.go +++ b/client/lcd/test_helpers.go @@ -230,7 +230,7 @@ func InitializeTestLCD( privVal.Reset() db := dbm.NewMemDB() - app := gapp.NewGaiaApp(logger, db, nil) + app := gapp.NewGaiaApp(logger, db, nil, true) cdc = gapp.MakeCodec() genesisFile := config.GenesisFile() diff --git a/cmd/gaia/app/app.go b/cmd/gaia/app/app.go index ebcdc8820..4930f44ef 100644 --- a/cmd/gaia/app/app.go +++ b/cmd/gaia/app/app.go @@ -68,7 +68,7 @@ type GaiaApp struct { } // NewGaiaApp returns a reference to an initialized GaiaApp. -func NewGaiaApp(logger log.Logger, db dbm.DB, traceStore io.Writer, baseAppOptions ...func(*bam.BaseApp)) *GaiaApp { +func NewGaiaApp(logger log.Logger, db dbm.DB, traceStore io.Writer, loadLatest bool, baseAppOptions ...func(*bam.BaseApp)) *GaiaApp { cdc := MakeCodec() bApp := bam.NewBaseApp(appName, logger, db, auth.DefaultTxDecoder(cdc), baseAppOptions...) @@ -167,9 +167,11 @@ func NewGaiaApp(logger log.Logger, db dbm.DB, traceStore io.Writer, baseAppOptio app.MountStoresTransient(app.tkeyParams, app.tkeyStake, app.tkeyDistr) app.SetEndBlocker(app.EndBlocker) - err := app.LoadLatestVersion(app.keyMain) - if err != nil { - cmn.Exit(err.Error()) + if loadLatest { + err := app.LoadLatestVersion(app.keyMain) + if err != nil { + cmn.Exit(err.Error()) + } } return app diff --git a/cmd/gaia/app/app_test.go b/cmd/gaia/app/app_test.go index 238f22966..87b2ec2a9 100644 --- a/cmd/gaia/app/app_test.go +++ b/cmd/gaia/app/app_test.go @@ -50,11 +50,11 @@ func setGenesis(gapp *GaiaApp, accs ...*auth.BaseAccount) error { func TestGaiadExport(t *testing.T) { db := db.NewMemDB() - gapp := NewGaiaApp(log.NewTMLogger(log.NewSyncWriter(os.Stdout)), db, nil) + gapp := NewGaiaApp(log.NewTMLogger(log.NewSyncWriter(os.Stdout)), db, nil, true) setGenesis(gapp) // Making a new app object with the db, so that initchain hasn't been called - newGapp := NewGaiaApp(log.NewTMLogger(log.NewSyncWriter(os.Stdout)), db, nil) + newGapp := NewGaiaApp(log.NewTMLogger(log.NewSyncWriter(os.Stdout)), db, nil, true) _, _, err := newGapp.ExportAppStateAndValidators(false) require.NoError(t, err, "ExportAppStateAndValidators should not have an error") } diff --git a/cmd/gaia/app/sim_test.go b/cmd/gaia/app/sim_test.go index b4504bdf8..67fa05ac5 100644 --- a/cmd/gaia/app/sim_test.go +++ b/cmd/gaia/app/sim_test.go @@ -215,7 +215,7 @@ func BenchmarkFullGaiaSimulation(b *testing.B) { db.Close() os.RemoveAll(dir) }() - app := NewGaiaApp(logger, db, nil) + app := NewGaiaApp(logger, db, nil, true) // Run randomized simulation // TODO parameterize numbers, save for a later PR @@ -257,7 +257,7 @@ func TestFullGaiaSimulation(t *testing.T) { db.Close() os.RemoveAll(dir) }() - app := NewGaiaApp(logger, db, nil, fauxMerkleModeOpt) + app := NewGaiaApp(logger, db, nil, true, fauxMerkleModeOpt) require.Equal(t, "GaiaApp", app.Name()) // Run randomized simulation @@ -298,7 +298,7 @@ func TestGaiaImportExport(t *testing.T) { db.Close() os.RemoveAll(dir) }() - app := NewGaiaApp(logger, db, nil, fauxMerkleModeOpt) + app := NewGaiaApp(logger, db, nil, true, fauxMerkleModeOpt) require.Equal(t, "GaiaApp", app.Name()) // Run randomized simulation @@ -334,7 +334,7 @@ func TestGaiaImportExport(t *testing.T) { newDB.Close() os.RemoveAll(newDir) }() - newApp := NewGaiaApp(log.NewNopLogger(), newDB, nil, fauxMerkleModeOpt) + newApp := NewGaiaApp(log.NewNopLogger(), newDB, nil, true, fauxMerkleModeOpt) require.Equal(t, "GaiaApp", newApp.Name()) var genesisState GenesisState err = app.cdc.UnmarshalJSON(appState, &genesisState) @@ -394,7 +394,7 @@ func TestGaiaSimulationAfterImport(t *testing.T) { db.Close() os.RemoveAll(dir) }() - app := NewGaiaApp(logger, db, nil, fauxMerkleModeOpt) + app := NewGaiaApp(logger, db, nil, true, fauxMerkleModeOpt) require.Equal(t, "GaiaApp", app.Name()) // Run randomized simulation @@ -436,7 +436,7 @@ func TestGaiaSimulationAfterImport(t *testing.T) { newDB.Close() os.RemoveAll(newDir) }() - newApp := NewGaiaApp(log.NewNopLogger(), newDB, nil, fauxMerkleModeOpt) + newApp := NewGaiaApp(log.NewNopLogger(), newDB, nil, true, fauxMerkleModeOpt) require.Equal(t, "GaiaApp", newApp.Name()) newApp.InitChain(abci.RequestInitChain{ AppStateBytes: appState, @@ -471,7 +471,7 @@ func TestAppStateDeterminism(t *testing.T) { for j := 0; j < numTimesToRunPerSeed; j++ { logger := log.NewNopLogger() db := dbm.NewMemDB() - app := NewGaiaApp(logger, db, nil) + app := NewGaiaApp(logger, db, nil, true) // Run randomized simulation simulation.SimulateFromSeed( diff --git a/cmd/gaia/cmd/gaiad/main.go b/cmd/gaia/cmd/gaiad/main.go index f39ddae8c..2791415c9 100644 --- a/cmd/gaia/cmd/gaiad/main.go +++ b/cmd/gaia/cmd/gaiad/main.go @@ -55,7 +55,7 @@ func main() { } func newApp(logger log.Logger, db dbm.DB, traceStore io.Writer) abci.Application { - return app.NewGaiaApp(logger, db, traceStore, + return app.NewGaiaApp(logger, db, traceStore, true, baseapp.SetPruning(viper.GetString("pruning")), baseapp.SetMinimumFees(viper.GetString("minimum_fees")), ) @@ -64,7 +64,7 @@ func newApp(logger log.Logger, db dbm.DB, traceStore io.Writer) abci.Application func exportAppStateAndTMValidators( logger log.Logger, db dbm.DB, traceStore io.Writer, height int64, forZeroHeight bool, ) (json.RawMessage, []tmtypes.GenesisValidator, error) { - gApp := app.NewGaiaApp(logger, db, traceStore) + gApp := app.NewGaiaApp(logger, db, traceStore, false) if height != -1 { err := gApp.LoadHeight(height) if err != nil { diff --git a/cmd/gaia/cmd/gaiareplay/main.go b/cmd/gaia/cmd/gaiareplay/main.go index cd58640e4..7e6392bf1 100644 --- a/cmd/gaia/cmd/gaiareplay/main.go +++ b/cmd/gaia/cmd/gaiareplay/main.go @@ -105,7 +105,7 @@ func run(rootDir string) { // Application fmt.Println("Creating application") myapp := app.NewGaiaApp( - ctx.Logger, appDB, traceStoreWriter, + ctx.Logger, appDB, traceStoreWriter, true, baseapp.SetPruning("everything"), // nothing ) From 45a48f4e4804f177d9272b7604052edfefe87596 Mon Sep 17 00:00:00 2001 From: Christopher Goes Date: Wed, 19 Dec 2018 17:14:21 +0100 Subject: [PATCH 5/6] PENDING => CHANGELOG --- CHANGELOG.md | 4 ++++ PENDING.md | 2 -- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 536522779..b37dca1c9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,9 +4,13 @@ BREAKING CHANGES +* Gaia + * [\#3148](https://github.com/cosmos/cosmos-sdk/issues/3148) Fix `gaiad export` by adding a boolean to `NewGaiaApp` determining whether or not to load the latest version + * SDK * [\#3163](https://github.com/cosmos/cosmos-sdk/issues/3163) Withdraw commission on self bond removal + ## 0.28.1 BREAKING CHANGES diff --git a/PENDING.md b/PENDING.md index f70b51ac4..4d87301f0 100644 --- a/PENDING.md +++ b/PENDING.md @@ -47,8 +47,6 @@ BUG FIXES * Gaia - * \#3148 Fix `gaiad export` by adding a boolean to `NewGaiaApp` determining whether or not to load the latest version - * SDK * Tendermint From 3387261e6b800ed44b5f2dba44617195c32834d7 Mon Sep 17 00:00:00 2001 From: Christopher Goes Date: Wed, 19 Dec 2018 23:36:25 +0100 Subject: [PATCH 6/6] Add comment outlining safety of commission-withdraw (#3175) --- x/distribution/keeper/hooks.go | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/x/distribution/keeper/hooks.go b/x/distribution/keeper/hooks.go index 613419846..910f6eafa 100644 --- a/x/distribution/keeper/hooks.go +++ b/x/distribution/keeper/hooks.go @@ -87,6 +87,13 @@ func (k Keeper) onDelegationSharesModified(ctx sdk.Context, delAddr sdk.AccAddre // Withdrawal all validator distribution rewards and cleanup the distribution record func (k Keeper) onDelegationRemoved(ctx sdk.Context, delAddr sdk.AccAddress, valAddr sdk.ValAddress) { + // Withdraw validator commission when validator self-bond is removed. + // Because we maintain the invariant that all delegations must be removed + // before a validator is deleted, this ensures that commission will be withdrawn + // before the validator is deleted (and the corresponding ValidatorDistInfo removed). + // If we change other parts of the code such that a self-delegation might remain after + // a validator is deleted, this logic will no longer be safe. + // TODO: Consider instead implementing this in a "BeforeValidatorRemoved" hook. if valAddr.Equals(sdk.ValAddress(delAddr)) { feePool, commission := k.withdrawValidatorCommission(ctx, valAddr) k.WithdrawToDelegator(ctx, feePool, delAddr, commission)