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 <[email protected]>
okx · May 16, 2023 · 7fdb0ea · 7fdb0ea
1 parent ec80d21
commit 7fdb0ea
Show file tree

Hide file tree

Showing 6 changed files with 32 additions and 121 deletions.
diff --git 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
@@ -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
@@ -12,18 +12,15 @@ import (
 
 var (
 	upgradeInfoPreifx = []byte("upgrade")
-	readyPrefix       = []byte("readyUpgrade")
 )
 
 type UpgradeCache struct {
 	storeKey *sdk.KVStoreKey
 	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
@@ -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
@@ -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
@@ -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,