Throttle refactors

docs/throttle.md

@@ -16,7 +16,7 @@ TODO: Update `Provider Slashing Warranty` to accommodate that slash requests are
 
 ## Practical/Implementation Properties
 
-One property of this implementation (that probably doesn't need to be in the spec) is that if any of the chain-specific packet data queues become larger than `MaxPendingSlashingPackets (param)`, then the provider binary will panic, and the provider chain will halt. Therefore this param should be set carefully. See [PanicIfTooMuchThrottledPacketData](../x/ccv/provider/keeper/throttle.go#L264) for more details. This behavior is included so that if the provider binaries are queuing up more packet data than machines can handle, the provider chain halts deterministically between validators.
+One property of this implementation (that probably doesn't need to be in the spec) is that if any of the chain-specific packet data queues become larger than `MaxThrottledPackets (param)`, then the provider binary will panic, and the provider chain will halt. Therefore this param should be set carefully. See [PanicIfTooMuchThrottledPacketData](../x/ccv/provider/keeper/throttle.go#L264) for more details. This behavior is included so that if the provider binaries are queuing up more packet data than machines can handle, the provider chain halts deterministically between validators.
 
 ## Data structure - Global entry queue
 
@@ -48,7 +48,7 @@ Upon the provider receiving a VSCMatured packet from any of the established cons
 
 There exists one slash meter on the provider which stores an amount of voting power (integer), corresponding to an allowance of validators that can be jailed/tombstoned over time. This meter is initialized to a certain value on genesis, decremented whenever a slash packet is handled, and periodically replenished as decided by on-chain params.
 
-## Endblocker handling - HandlePendingSlashPackets
+## Endblocker handling - HandleThrottleQueues
 
 Every endblocker the following psuedocode is executed
 
@@ -67,7 +67,7 @@ while meter.IsPositive() && entriesExist() {
      handleSlashPacketAndTrailingVSCMaturedPackets(entry)
      // Delete entry in global queue, delete handled data
      entry.Delete()
-     deletePendingSlashPacketData()
+     deleteThrottledSlashPacketData()
      deleteTrailingVSCMaturedPacketData()
 }
 ```

go.mod

@@ -34,6 +34,7 @@ require (
 	github.com/golang/mock v1.6.0
 	github.com/oxyno-zeta/gomock-extra-matcher v1.1.0
 	github.com/regen-network/cosmos-proto v0.3.1
+	golang.org/x/exp v0.0.0-20220722155223-a9213eeb770e
 )
 
 require (
@@ -129,7 +130,6 @@ require (
 	github.com/tidwall/pretty v1.2.0 // indirect
 	github.com/zondax/hid v0.9.1 // indirect
 	go.etcd.io/bbolt v1.3.6 // indirect
-	golang.org/x/exp v0.0.0-20220722155223-a9213eeb770e // indirect
 	golang.org/x/term v0.1.0 // indirect
 	golang.org/x/text v0.4.0 // indirect
 	gopkg.in/ini.v1 v1.67.0 // indirect

proto/interchain_security/ccv/provider/v1/provider.proto

@@ -102,9 +102,9 @@ message Params {
   // This param also serves as a maximum fraction of total voting power that the slash meter can hold.
   string slash_meter_replenish_fraction = 7;
 
-  // The maximum amount of pending slash packets that can be queued for a consumer
-  // before the provider chain halts.
-  int64 max_pending_slash_packets = 8;
+  // The maximum amount of throttled slash or vsc matured packets 
+  // that can be queued for a single consumer before the provider chain halts.
+  int64 max_throttled_packets = 8;
 }
 
 message HandshakeMetadata {

tests/e2e/throttle.go

@@ -329,8 +329,8 @@ func (s *CCVTestSuite) TestPacketSpam() {
 // in the global queue (relevant to a single chain) matches the ordering of slash packet
 // data in the chain specific queues, even in the presence of packet spam.
 //
-// Note: The global queue is ordered by: time, then IBC sequence number, see PendingSlashPacketEntryKey.
-// The chain specific queue is ordered by: IBC sequence number, see PendingSlashPacketDataKey.
+// Note: The global queue is ordered by: time, then IBC sequence number, see GlobalSlashEntryKey.
+// The chain specific queue is ordered by: IBC sequence number, see ThrottledPacketDataKey.
 func (s *CCVTestSuite) TestQueueOrdering() {
 
 	// Setup ccv channels to all consumers

x/ccv/provider/keeper/params.go

@@ -69,11 +69,11 @@ func (k Keeper) GetSlashMeterReplenishFraction(ctx sdk.Context) string {
 	return f
 }
 
-// GetMaxPendingSlashingPackets returns the maximum number of pending slash packets that can be queued for a consumer
-// before the provider chain halts.
-func (k Keeper) GetMaxPendingSlashingPackets(ctx sdk.Context) int64 {
+// GetMaxThrottledPackets returns the maximum amount of throttled slash or vsc matured packets
+// that can be queued for a single consumer before the provider chain halts.
+func (k Keeper) GetMaxThrottledPackets(ctx sdk.Context) int64 {
 	var p int64
-	k.paramSpace.Get(ctx, types.KeyMaxPendingSlashPackets, &p)
+	k.paramSpace.Get(ctx, types.KeyMaxThrottledPackets, &p)
 	return p
 }
 
@@ -87,7 +87,7 @@ func (k Keeper) GetParams(ctx sdk.Context) types.Params {
 		k.GetVscTimeoutPeriod(ctx),
 		k.GetSlashMeterReplenishPeriod(ctx),
 		k.GetSlashMeterReplenishFraction(ctx),
-		k.GetMaxPendingSlashingPackets(ctx),
+		k.GetMaxThrottledPackets(ctx),
 	)
 }
 

x/ccv/provider/keeper/proposal_test.go

@@ -712,7 +712,7 @@ func TestMakeConsumerGenesis(t *testing.T) {
 		VscTimeoutPeriod:            providertypes.DefaultVscTimeoutPeriod,
 		SlashMeterReplenishPeriod:   providertypes.DefaultSlashMeterReplenishPeriod,
 		SlashMeterReplenishFraction: providertypes.DefaultSlashMeterReplenishFraction,
-		MaxPendingSlashPackets:      providertypes.DefaultMaxPendingSlashPackets,
+		MaxThrottledPackets:         providertypes.DefaultMaxThrottledPackets,
 	}
 	providerKeeper.SetParams(ctx, moduleParams)
 	defer ctrl.Finish()
@@ -744,7 +744,7 @@ func TestMakeConsumerGenesis(t *testing.T) {
 	var expectedGenesis consumertypes.GenesisState
 	err = json.Unmarshal([]byte(jsonString), &expectedGenesis)
 	require.NoError(t, err)
-	
+
 	// Zeroing out different fields that are challenging to mock
 	actualGenesis.InitialValSet = []abci.ValidatorUpdate{}
 	expectedGenesis.InitialValSet = []abci.ValidatorUpdate{}

x/ccv/provider/keeper/throttle.go

@@ -109,14 +109,17 @@ func (k Keeper) CheckForSlashMeterReplenishment(ctx sdktypes.Context) {
 		k.ReplenishSlashMeter(ctx)
 	}
 
+	// The following logic exists to ensure the slash meter is not greater than the allowance for this block,
+	// in the event that the total voting power of the provider chain has decreased since previous blocks.
+
 	// If slash meter is full, or more than full considering updated allowance/total power,
 	allowance := k.GetSlashMeterAllowance(ctx)
 	if k.GetSlashMeter(ctx).GTE(allowance) {
 
 		// set the most recent time the slash meter was full to current block time.
 		k.SetLastSlashMeterFullTime(ctx, ctx.BlockTime())
 
-		// Ensure the slash meter is not greater than allowance,
+		// Ensure the slash meter is not greater than allowance this block,
 		// considering current total voting power.
 		k.SetSlashMeter(ctx, allowance)
 	}
@@ -139,8 +142,12 @@ func (k Keeper) ReplenishSlashMeter(ctx sdktypes.Context) {
 }
 
 // GetSlashMeterAllowance returns the amount of voting power units (int)
-// that would be added to the slash meter for a replenishment this block,
+// that would be added to the slash meter for a replenishment that would happen this block,
 // this allowance value also serves as the max value for the meter for this block.
+//
+// Note: allowance can change between blocks, since it is directly correlated to total voting power.
+// The slash meter must be less than or equal to the allowance for this block, before any slash
+// packet handling logic can be executed.
 func (k Keeper) GetSlashMeterAllowance(ctx sdktypes.Context) sdktypes.Int {
 
 	strFrac := k.GetSlashMeterReplenishFraction(ctx)
@@ -226,15 +233,6 @@ const (
 	vscMaturedPacketData
 )
 
-// PanicIfTooMuchThrottledPacketData is a sanity check to ensure that the chain-specific
-// throttled packet data queue does not grow too large for a single consumer chain.
-func (k Keeper) PanicIfTooMuchThrottledPacketData(ctx sdktypes.Context, consumerChainID string) {
-	size := k.GetThrottledPacketDataSize(ctx, consumerChainID)
-	if size > uint64(k.GetMaxPendingSlashingPackets(ctx)) {
-		panic(fmt.Sprintf("throttled packet data queue for chain %s is too large: %d", consumerChainID, size))
-	}
-}
-
 // GetThrottledPacketDataSize returns the size of the throttled packet data queue for the given consumer chain
 func (k Keeper) GetThrottledPacketDataSize(ctx sdktypes.Context, consumerChainID string) uint64 {
 	store := ctx.KVStore(k.storeKey)
@@ -251,6 +249,14 @@ func (k Keeper) GetThrottledPacketDataSize(ctx sdktypes.Context, consumerChainID
 
 // SetThrottledPacketDataSize sets the size of the throttled packet data queue for the given consumer chain
 func (k Keeper) SetThrottledPacketDataSize(ctx sdktypes.Context, consumerChainID string, size uint64) {
+
+	// Sanity check to ensure that the chain-specific throttled packet data queue does not grow too
+	// large for a single consumer chain. This check ensures that binaries would panic deterministically
+	// if the queue does grow too large.
+	if size >= uint64(k.GetMaxThrottledPackets(ctx)) {
+		panic(fmt.Sprintf("throttled packet data queue for chain %s is too large: %d", consumerChainID, size))
+	}
+
 	store := ctx.KVStore(k.storeKey)
 	key := providertypes.ThrottledPacketDataSizeKey(consumerChainID)
 	bz := sdktypes.Uint64ToBigEndian(size)
@@ -286,8 +292,6 @@ func (k Keeper) QueueThrottledVSCMaturedPacketData(
 func (k Keeper) QueueThrottledPacketData(
 	ctx sdktypes.Context, consumerChainID string, ibcSeqNum uint64, packetData interface{}) {
 
-	k.PanicIfTooMuchThrottledPacketData(ctx, consumerChainID)
-
 	store := ctx.KVStore(k.storeKey)
 
 	var bz []byte
@@ -319,8 +323,12 @@ func (k Keeper) QueueThrottledPacketData(
 // Note: this method is not tested directly, but is covered indirectly
 // by TestHandlePacketDataForChain and e2e tests.
 func (k Keeper) GetSlashAndTrailingData(ctx sdktypes.Context, consumerChainID string) (
-	slashFound bool, slashData ccvtypes.SlashPacketData,
-	vscMaturedData []ccvtypes.VSCMaturedPacketData, ibcSeqNums []uint64) {
+	slashFound bool, slashData ccvtypes.SlashPacketData, vscMaturedData []ccvtypes.VSCMaturedPacketData,
+	// Note: this slice contains the IBC sequence numbers of the slash packet data
+	// and trailing vsc matured packet data instances. This is used by caller to delete the
+	// data after it has been handled.
+	ibcSeqNums []uint64,
+) {
 
 	store := ctx.KVStore(k.storeKey)
 	iteratorPrefix := providertypes.ChainIdWithLenKey(providertypes.ThrottledPacketDataBytePrefix, consumerChainID)
@@ -332,27 +340,26 @@ func (k Keeper) GetSlashAndTrailingData(ctx sdktypes.Context, consumerChainID st
 	vscMaturedData = []ccvtypes.VSCMaturedPacketData{}
 	ibcSeqNums = []uint64{}
 
-iteratorLoop:
 	for ; iterator.Valid(); iterator.Next() {
+
 		bz := iterator.Value()
-		switch bz[0] {
-		case slashPacketData:
+		if bz[0] == slashPacketData {
 			if slashFound {
 				// Break for-loop, we've already found first slash packet data instance.
-				break iteratorLoop
+				break
 			} else {
 				if err := slashData.Unmarshal(bz[1:]); err != nil {
 					panic(fmt.Sprintf("failed to unmarshal pending packet data: %v", err))
 				}
 				slashFound = true
 			}
-		case vscMaturedPacketData:
+		} else if bz[0] == vscMaturedPacketData {
 			vscMData := ccvtypes.VSCMaturedPacketData{}
 			if err := vscMData.Unmarshal(bz[1:]); err != nil {
 				panic(fmt.Sprintf("failed to unmarshal pending packet data: %v", err))
 			}
 			vscMaturedData = append(vscMaturedData, vscMData)
-		default:
+		} else {
 			panic("invalid packet data type")
 		}
 

x/ccv/provider/keeper/throttle_test.go

@@ -864,27 +864,29 @@ func TestDeleteThrottledPacketDataForConsumer(t *testing.T) {
 	require.Len(t, vscMaturedData, 1)
 }
 
-// TestPanicIfTooMuchThrottledPacketData tests the PanicIfTooMuchThrottledPacketData method.
+// TestPanicIfTooMuchThrottledPacketData tests that the provider panics
+// when the number of throttled (queued) packets exceeds the max allowed by params.
 func TestPanicIfTooMuchThrottledPacketData(t *testing.T) {
 
 	testCases := []struct {
 		max int64
 	}{
-		{max: 1},
+		{max: 3}, // Max must be greater than 2 since we queue 2 packets for another chain in the test
 		{max: 5},
 		{max: 10},
 		{max: 15},
 		{max: 25},
+		{max: 100},
 	}
 
 	for _, tc := range testCases {
 
 		providerKeeper, ctx, ctrl, _ := testkeeper.GetProviderKeeperAndCtx(t, testkeeper.NewInMemKeeperParams(t))
 		defer ctrl.Finish()
 
-		// Set max pending slash packets param
+		// Set max throttled packets param
 		defaultParams := providertypes.DefaultParams()
-		defaultParams.MaxPendingSlashPackets = tc.max
+		defaultParams.MaxThrottledPackets = tc.max
 		providerKeeper.SetParams(ctx, defaultParams)
 
 		rand.Seed(time.Now().UnixNano())
@@ -895,7 +897,7 @@ func TestPanicIfTooMuchThrottledPacketData(t *testing.T) {
 
 		// Queue packet data instances until we reach the max (some slash packets, some VSC matured packets)
 		reachedMax := false
-		for i := 0; i < int(tc.max+2); i++ { // iterate up to tc.max+1
+		for i := 0; i < int(tc.max); i++ {
 			randBool := rand.Intn(2) == 0
 			var data interface{}
 			if randBool {
@@ -904,7 +906,7 @@ func TestPanicIfTooMuchThrottledPacketData(t *testing.T) {
 				data = testkeeper.GetNewVSCMaturedPacketData()
 			}
 			// Panic only if we've reached the max
-			if i == int(tc.max+1) {
+			if i == int(tc.max-1) {
 				require.Panics(t, func() {
 					providerKeeper.QueueThrottledPacketData(ctx, "chain-88", uint64(i), data)
 				})
@@ -917,11 +919,16 @@ func TestPanicIfTooMuchThrottledPacketData(t *testing.T) {
 	}
 }
 
-// TestPendingPacketDataSize tests the getter, setter and incrementer for pending packet data size.
-func TestPendingPacketDataSize(t *testing.T) {
+// TestThrottledPacketDataSize tests the getter, setter and incrementer for throttled packet data size.
+func TestThrottledPacketDataSize(t *testing.T) {
+
 	providerKeeper, ctx, ctrl, _ := testkeeper.GetProviderKeeperAndCtx(t, testkeeper.NewInMemKeeperParams(t))
 	defer ctrl.Finish()
 
+	// Set params so we can use the default max throttled packet data size
+	params := providertypes.DefaultParams()
+	providerKeeper.SetParams(ctx, params)
+
 	// Confirm initial size is 0
 	require.Equal(t, uint64(0), providerKeeper.GetThrottledPacketDataSize(ctx, "chain-0"))
 

x/ccv/provider/types/genesis_test.go

@@ -157,7 +157,7 @@ func TestValidateGenesisState(t *testing.T) {
 					types.DefaultVscTimeoutPeriod,
 					types.DefaultSlashMeterReplenishPeriod,
 					types.DefaultSlashMeterReplenishFraction,
-					types.DefaultMaxPendingSlashPackets),
+					types.DefaultMaxThrottledPackets),
 				nil,
 				nil,
 				nil,
@@ -182,7 +182,7 @@ func TestValidateGenesisState(t *testing.T) {
 					types.DefaultVscTimeoutPeriod,
 					types.DefaultSlashMeterReplenishPeriod,
 					types.DefaultSlashMeterReplenishFraction,
-					types.DefaultMaxPendingSlashPackets),
+					types.DefaultMaxThrottledPackets),
 				nil,
 				nil,
 				nil,
@@ -207,7 +207,7 @@ func TestValidateGenesisState(t *testing.T) {
 					types.DefaultVscTimeoutPeriod,
 					types.DefaultSlashMeterReplenishPeriod,
 					types.DefaultSlashMeterReplenishFraction,
-					types.DefaultMaxPendingSlashPackets),
+					types.DefaultMaxThrottledPackets),
 				nil,
 				nil,
 				nil,
@@ -232,7 +232,7 @@ func TestValidateGenesisState(t *testing.T) {
 					types.DefaultVscTimeoutPeriod,
 					types.DefaultSlashMeterReplenishPeriod,
 					types.DefaultSlashMeterReplenishFraction,
-					types.DefaultMaxPendingSlashPackets),
+					types.DefaultMaxThrottledPackets),
 				nil,
 				nil,
 				nil,
@@ -257,7 +257,7 @@ func TestValidateGenesisState(t *testing.T) {
 					0, // 0 vsc timeout here
 					types.DefaultSlashMeterReplenishPeriod,
 					types.DefaultSlashMeterReplenishFraction,
-					types.DefaultMaxPendingSlashPackets),
+					types.DefaultMaxThrottledPackets),
 				nil,
 				nil,
 				nil,
@@ -282,7 +282,7 @@ func TestValidateGenesisState(t *testing.T) {
 					types.DefaultVscTimeoutPeriod,
 					0, // 0 slash meter replenish period here
 					types.DefaultSlashMeterReplenishFraction,
-					types.DefaultMaxPendingSlashPackets),
+					types.DefaultMaxThrottledPackets),
 				nil,
 				nil,
 				nil,
@@ -307,7 +307,7 @@ func TestValidateGenesisState(t *testing.T) {
 					types.DefaultVscTimeoutPeriod,
 					types.DefaultSlashMeterReplenishPeriod,
 					"1.15",
-					types.DefaultMaxPendingSlashPackets),
+					types.DefaultMaxThrottledPackets),
 				nil,
 				nil,
 				nil,

x/ccv/provider/types/keys.go

@@ -42,11 +42,11 @@ const (
 	// ValidatorSetUpdateIdByteKey is the byte key that stores the current validator set update id
 	ValidatorSetUpdateIdByteKey
 
-	// SlashMeterBytePrefix is the byte key for storing the slash meter
+	// SlashMeterByteKey is the byte key for storing the slash meter
 	SlashMeterByteKey
 
-	// LastSlashMeterReplenishTimeBytePrefix is the byte key for storing
-	// the last time the slash meter was replenished
+	// LastSlashMeterFullTimeByteKey is the byte key for storing
+	// the last time the slash meter was full.
 	LastSlashMeterFullTimeByteKey
 
 	// ChainToChannelBytePrefix is the byte prefix for storing mapping
@@ -106,7 +106,7 @@ const (
 	// ThrottledPacketDataBytePrefix is the byte prefix storing throttled packet data
 	ThrottledPacketDataBytePrefix
 
-	// PendingSlashPacketEntryBytePrefix is the byte prefix storing global slash queue entries
+	// GlobalSlashEntryBytePrefix is the byte prefix storing global slash queue entries
 	GlobalSlashEntryBytePrefix
 
 	// ConsumerValidatorsBytePrefix is the byte prefix that will store the validator assigned keys for every consumer chain