From 7fdb0eaed21156d66c0ed029b620a4301a0ce334 Mon Sep 17 00:00:00 2001 From: fatcat22 Date: Tue, 16 May 2023 10:15:20 +0800 Subject: [PATCH] fix upgradeproposal (#3139) * remove map in upgrade proposal * remove panic * return nil if not ready * return nil if not ready * fix ut error * remove useless code * remove useless code --------- Co-authored-by: yangzhe --- x/params/keeper_upgrade.go | 2 +- x/params/keeper_upgrade_test.go | 2 +- x/params/types/upgrade_cache.go | 71 ++-------------------------- x/params/types/upgrade_cache_test.go | 22 ++------- x/params/upgrade_executor.go | 10 ++-- x/params/upgrade_executor_test.go | 46 +++++++----------- 6 files changed, 32 insertions(+), 121 deletions(-) diff --git a/x/params/keeper_upgrade.go b/x/params/keeper_upgrade.go index 7976e1045d..362fcdee9f 100644 --- a/x/params/keeper_upgrade.go +++ b/x/params/keeper_upgrade.go @@ -15,7 +15,7 @@ func (keeper *Keeper) ClaimReadyForUpgrade(name string, cb func(types.UpgradeInf keeper.upgradeCache.ClaimReadyForUpgrade(name, cb) } -func (keeper *Keeper) IsUpgradeEffective(ctx sdk.Context, name string) bool { +func (keeper *Keeper) isUpgradeEffective(ctx sdk.Context, name string) bool { _, err := keeper.GetEffectiveUpgradeInfo(ctx, name) return err == nil } diff --git a/x/params/keeper_upgrade_test.go b/x/params/keeper_upgrade_test.go index b61ff7cce2..9260726d54 100644 --- a/x/params/keeper_upgrade_test.go +++ b/x/params/keeper_upgrade_test.go @@ -116,7 +116,7 @@ func (suite *UpgradeKeeperSuite) TestUpgradeEffective() { } suite.Equal(tt.expectEffective, isUpgradeEffective(ctx, expectInfo)) - isEffective := suite.paramsKeeper.IsUpgradeEffective(ctx, expectInfo.Name) + isEffective := suite.paramsKeeper.isUpgradeEffective(ctx, expectInfo.Name) info, err := suite.paramsKeeper.GetEffectiveUpgradeInfo(ctx, expectInfo.Name) if tt.isStore { suite.Equal(tt.expectEffective, isEffective) diff --git a/x/params/types/upgrade_cache.go b/x/params/types/upgrade_cache.go index 63d9a855d3..4e7d831e4a 100644 --- a/x/params/types/upgrade_cache.go +++ b/x/params/types/upgrade_cache.go @@ -12,7 +12,6 @@ import ( var ( upgradeInfoPreifx = []byte("upgrade") - readyPrefix = []byte("readyUpgrade") ) type UpgradeCache struct { @@ -20,10 +19,8 @@ type UpgradeCache struct { logger log.Logger cdc *codec.Codec - readyLock sync.Mutex - upgradeReadyMap map[string][]func(UpgradeInfo) - infoLock sync.Mutex - upgradeInfoCache map[string]UpgradeInfo + readyLock sync.Mutex + upgradeReadyMap map[string][]func(UpgradeInfo) } func NewUpgreadeCache(storeKey *sdk.KVStoreKey, logger log.Logger, cdc *codec.Codec) *UpgradeCache { @@ -32,26 +29,12 @@ func NewUpgreadeCache(storeKey *sdk.KVStoreKey, logger log.Logger, cdc *codec.Co logger: logger, cdc: cdc, - upgradeReadyMap: make(map[string][]func(UpgradeInfo)), - upgradeInfoCache: make(map[string]UpgradeInfo), + upgradeReadyMap: make(map[string][]func(UpgradeInfo)), } } func (uc *UpgradeCache) ReadUpgradeInfo(ctx sdk.Context, name string) (UpgradeInfo, error) { - if ctx.UseParamCache() { - info, exist := uc.readUpgradeInfo(name) - if exist { - return info, nil - } - } - - info, err := readUpgradeInfoFromStore(ctx, name, uc.storeKey, uc.cdc) - if err != nil { - return info, err - } - - uc.writeUpgradeInfo(info) - return info, nil + return readUpgradeInfoFromStore(ctx, name, uc.storeKey, uc.cdc) } func (uc *UpgradeCache) ClaimReadyForUpgrade(name string, cb func(UpgradeInfo)) { @@ -63,14 +46,7 @@ func (uc *UpgradeCache) QueryReadyForUpgrade(name string) ([]func(UpgradeInfo), } func (uc *UpgradeCache) WriteUpgradeInfo(ctx sdk.Context, info UpgradeInfo, forceCover bool) sdk.Error { - if err := writeUpgradeInfoToStore(ctx, info, forceCover, uc.storeKey, uc.cdc, uc.logger); err != nil { - return err - } - - // store is updated, remove the info from cache so - // makeing ReadUpgradeInfo to re-read from store. - uc.removeUpgradeInfo(info.Name) - return nil + return writeUpgradeInfoToStore(ctx, info, forceCover, uc.storeKey, uc.cdc, uc.logger) } func (uc *UpgradeCache) IsUpgradeExist(ctx sdk.Context, name string) bool { @@ -97,28 +73,6 @@ func (uc *UpgradeCache) IterateAllUpgradeInfo(ctx sdk.Context, cb func(info Upgr return nil } -func (uc *UpgradeCache) readUpgradeInfo(name string) (UpgradeInfo, bool) { - uc.infoLock.Lock() - defer uc.infoLock.Unlock() - - info, ok := uc.upgradeInfoCache[name] - return info, ok -} - -func (uc *UpgradeCache) removeUpgradeInfo(name string) { - uc.infoLock.Lock() - defer uc.infoLock.Unlock() - - delete(uc.upgradeInfoCache, name) -} - -func (uc *UpgradeCache) writeUpgradeInfo(info UpgradeInfo) { - uc.infoLock.Lock() - defer uc.infoLock.Unlock() - - uc.upgradeInfoCache[info.Name] = info -} - func (uc *UpgradeCache) readClaim(name string) ([]func(UpgradeInfo), bool) { uc.readyLock.Lock() defer uc.readyLock.Unlock() @@ -171,18 +125,3 @@ func writeUpgradeInfoToStore(ctx sdk.Context, info UpgradeInfo, forceCover bool, func getUpgradeStore(ctx sdk.Context, skey *sdk.KVStoreKey) sdk.KVStore { return prefix.NewStore(ctx.KVStore(skey), upgradeInfoPreifx) } - -func readReadyFromStore(ctx sdk.Context, name string, skey *sdk.KVStoreKey) ([]byte, bool) { - store := getReadyStore(ctx, skey) - data := store.Get([]byte(name)) - return data, len(data) != 0 -} - -func writeReadyToStore(ctx sdk.Context, name string, skey *sdk.KVStoreKey) { - store := getReadyStore(ctx, skey) - store.Set([]byte(name), []byte(name)) -} - -func getReadyStore(ctx sdk.Context, skey *sdk.KVStoreKey) sdk.KVStore { - return prefix.NewStore(ctx.KVStore(skey), readyPrefix) -} diff --git a/x/params/types/upgrade_cache_test.go b/x/params/types/upgrade_cache_test.go index d23485be54..083f047198 100644 --- a/x/params/types/upgrade_cache_test.go +++ b/x/params/types/upgrade_cache_test.go @@ -52,13 +52,10 @@ func TestUpgradeKeeper(t *testing.T) { func (suite *UpgradeKeeperSuite) TestReadUpgradeInfo() { tests := []struct { - existAtCache bool existAtStore bool }{ - {true, true}, - {true, false}, - {false, true}, - {false, false}, + {true}, + {false}, } ctx := suite.Context(20) @@ -70,23 +67,16 @@ func (suite *UpgradeKeeperSuite) TestReadUpgradeInfo() { EffectiveHeight: uint64(i), Status: UpgradeStatusPreparing, } - if tt.existAtCache { - cache.writeUpgradeInfo(expectInfo) - } if tt.existAtStore { suite.NoError(writeUpgradeInfoToStore(ctx, expectInfo, false, suite.storeKey, suite.cdc, suite.logger)) } info, err := cache.ReadUpgradeInfo(ctx, expectInfo.Name) - if !tt.existAtCache && !tt.existAtStore { + if !tt.existAtStore { suite.Error(err) continue } - if tt.existAtStore { - info, exist := cache.readUpgradeInfo(expectInfo.Name) - suite.True(exist) - suite.Equal(expectInfo, info) - } + suite.NoError(err) suite.Equal(expectInfo, info) } } @@ -130,10 +120,6 @@ func (suite *UpgradeKeeperSuite) TestWriteUpgradeInfo() { info, err := cache.ReadUpgradeInfo(ctx, name) suite.NoError(err) suite.Equal(expectInfo2, info) - - info, exist := cache.readUpgradeInfo(name) - suite.True(exist) - suite.Equal(expectInfo2, info) } } diff --git a/x/params/upgrade_executor.go b/x/params/upgrade_executor.go index 722337264d..c951b543e4 100644 --- a/x/params/upgrade_executor.go +++ b/x/params/upgrade_executor.go @@ -43,11 +43,11 @@ func handleUpgradeProposal(ctx sdk.Context, k *Keeper, proposalID uint64, propos // that probably means program's version is too low. // To avoid status machine broken, we panic. errMsg := fmt.Sprintf("there's a upgrade proposal named '%s' has been take effective, "+ - "and the upgrade is incompatible, but your binary seems not ready for this upgrade. "+ - "To avoid state machine broken, the program is panic. "+ - "Using the latest version binary and re-run it to avoid this panic.", proposal.Name) + "and the upgrade is incompatible, but your binary seems not ready for this upgrade. current height: %d, confirm height %d", proposal.Name, curHeight, confirmHeight) k.Logger(ctx).Error(errMsg) - panic(errMsg) + // here must return nil but not an error, if an error is returned, the proposal won't be deleted + // from the waiting queue in gov keeper, result in this function is called endlessly in every block end. + return nil } storedInfo, err := storeEffectiveUpgrade(ctx, k, proposal, effectiveHeight) @@ -66,7 +66,7 @@ func handleUpgradeProposal(ctx sdk.Context, k *Keeper, proposalID uint64, propos func getUpgradeProposalConfirmHeight(currentHeight uint64, proposal types.UpgradeProposal) (uint64, sdk.Error) { // confirm height is the height proposal is confirmed. // confirmed is not become effective. Becoming effective will happen at - // the next block of confirm block. see `storeEffectiveUpgrade` and `IsUpgradeEffective` + // the next block of confirm block. see `storeEffectiveUpgrade` and `isUpgradeEffective` confirmHeight := proposal.ExpectHeight - 1 if proposal.ExpectHeight == 0 { // if height is not specified, this upgrade will become effective diff --git a/x/params/upgrade_executor_test.go b/x/params/upgrade_executor_test.go index e65978b2a8..76d3e55277 100644 --- a/x/params/upgrade_executor_test.go +++ b/x/params/upgrade_executor_test.go @@ -279,33 +279,31 @@ func (suite *UpgradeInfoStoreSuite) TestCheckUpgradeVote() { func (suite *UpgradeInfoStoreSuite) TestHandleUpgradeProposal() { tests := []struct { - expectHeight uint64 - currentHeight uint64 - claimReady bool - expectPanic bool - expect1stExecuteError bool - expectHitError bool + expectHeight uint64 + currentHeight uint64 + claimReady bool + expectError bool }{ { // expect height is not zero but less than current height - expectHeight: 10, currentHeight: 10, claimReady: false, expectPanic: true, expect1stExecuteError: false, + expectHeight: 10, currentHeight: 10, claimReady: false, expectError: true, }, { // expect height is not zero but only greater than current height 1; and not claim ready - expectHeight: 21, currentHeight: 20, claimReady: false, expectPanic: true, + expectHeight: 21, currentHeight: 20, claimReady: false, expectError: true, }, { // expect height is not zero and greater than current height; but not claim ready - expectHeight: 32, currentHeight: 30, claimReady: false, expectPanic: true, expect1stExecuteError: false, + expectHeight: 32, currentHeight: 30, claimReady: false, expectError: true, }, { // everything's ok: expect height is not zero and greater than current height; and claim ready - expectHeight: 42, currentHeight: 40, claimReady: true, expectPanic: false, expect1stExecuteError: false, expectHitError: false, + expectHeight: 42, currentHeight: 40, claimReady: true, expectError: false, }, { // everything's ok: expect height is zero and claim ready - expectHeight: 0, currentHeight: 50, claimReady: true, expectPanic: false, expect1stExecuteError: false, expectHitError: false, + expectHeight: 0, currentHeight: 50, claimReady: true, expectError: false, }, { // everything's ok: expect height is not zero and equal to current height; and claim ready - expectHeight: 60, currentHeight: 60, claimReady: true, expectPanic: false, expect1stExecuteError: false, expectHitError: false, + expectHeight: 60, currentHeight: 60, claimReady: true, expectError: false, }, { // everything's ok: expect height is not zero and less than current height; and claim ready - expectHeight: 70, currentHeight: 72, claimReady: true, expectPanic: false, expect1stExecuteError: false, expectHitError: false, + expectHeight: 70, currentHeight: 72, claimReady: true, expectError: false, }, } @@ -336,17 +334,9 @@ func (suite *UpgradeInfoStoreSuite) TestHandleUpgradeProposal() { }) } - if tt.expectPanic && confirmHeight <= tt.currentHeight { - suite.Panics(func() { _ = handler(ctx, proposal) }) - continue - } - // execute proposal err := handler(ctx, proposal) - if tt.expect1stExecuteError { - suite.Error(err) - continue - } + suite.NoError(err) if confirmHeight > tt.currentHeight { // proposal is inserted to gov waiting queue, execute it @@ -362,18 +352,14 @@ func (suite *UpgradeInfoStoreSuite) TestHandleUpgradeProposal() { suite.Equal(expectInfo, info) ctx := suite.Context(int64(confirmHeight)) - if tt.expectPanic { - suite.Panics(func() { _ = suite.govKeeper.HitHeight(ctx, confirmHeight, suite.T()) }) - continue - } err = suite.govKeeper.HitHeight(ctx, confirmHeight, suite.T()) - if tt.expectHitError { - suite.Error(err) - continue - } suite.NoError(err) } + if tt.expectError { + continue + } + // now proposal must be executed expectInfo := types.UpgradeInfo{ Name: upgradeProposal.Name,