fix: slash meter replenishment (#687)

* this test should fail

* changes

* refactors

* smol

* comments

* naming

* smalls

* update E2e tests to validate new behavior

* nit

* whoops

* change key name

* set time w/in method

* fix typo

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

@@ -136,8 +136,8 @@ message QueryThrottleStateResponse {
   // allowance of voting power units (int) that the slash meter is given per replenish period
   // this also serves as the max value for the meter.
   int64 slash_meter_allowance = 2;
-  // last time the slash meter was full
-  google.protobuf.Timestamp last_full_time = 3
+  // next time the slash meter could potentially be replenished, iff it's not full
+  google.protobuf.Timestamp next_replenish_candidate = 3
     [(gogoproto.stdtime) = true, (gogoproto.nullable) = false];
   // data relevant to currently throttled slash packets
   repeated ThrottledSlashPacket packets = 4;

tests/e2e/throttle.go

@@ -44,9 +44,7 @@ func (s *CCVTestSuite) TestBasicSlashPacketThrottling() {
 		params.SlashMeterReplenishFraction = tc.replenishFraction
 		s.providerApp.GetProviderKeeper().SetParams(s.providerCtx(), params)
 
-		// Elapse a replenish period and check for replenishment, so new param is fully in effect.
-		customCtx := s.getCtxWithReplenishPeriodElapsed(s.providerCtx())
-		s.providerApp.GetProviderKeeper().CheckForSlashMeterReplenishment(customCtx)
+		s.providerApp.GetProviderKeeper().InitializeSlashMeter(s.providerCtx())
 
 		slashMeter := s.providerApp.GetProviderKeeper().GetSlashMeter(s.providerCtx())
 		s.Require().Equal(tc.expectedMeterBeforeFirstSlash, slashMeter.Int64())
@@ -90,39 +88,58 @@ func (s *CCVTestSuite) TestBasicSlashPacketThrottling() {
 		slashMeter = s.providerApp.GetProviderKeeper().GetSlashMeter(s.providerCtx())
 		s.Require().Equal(tc.expectedMeterAfterFirstSlash, slashMeter.Int64())
 
+		// For the remainder of this test we use a cached context in which we can mutate block time
+		cacheCtx := s.providerCtx()
+
 		// Replenish slash meter until it is positive
 		for i := 0; i < tc.expectedReplenishesTillPositive; i++ {
 
-			// Mutate context with a block time where replenish period has passed.
-			customCtx = s.getCtxWithReplenishPeriodElapsed(s.providerCtx())
+			// Mutate cached context to have a block time after the current replenish candidate time.
+			cacheCtx = s.getCtxAfterReplenishCandidate(cacheCtx)
+			candidate := s.providerApp.GetProviderKeeper().GetSlashMeterReplenishTimeCandidate(cacheCtx)
+			s.Require().True(cacheCtx.BlockTime().After(candidate))
 
 			// CheckForSlashMeterReplenishment should replenish meter here.
-			slashMeterBefore := s.providerApp.GetProviderKeeper().GetSlashMeter(s.providerCtx())
-			s.providerApp.GetProviderKeeper().CheckForSlashMeterReplenishment(customCtx)
-			slashMeter = s.providerApp.GetProviderKeeper().GetSlashMeter(s.providerCtx())
+			slashMeterBefore := s.providerApp.GetProviderKeeper().GetSlashMeter(cacheCtx)
+			s.providerApp.GetProviderKeeper().CheckForSlashMeterReplenishment(cacheCtx)
+			slashMeter = s.providerApp.GetProviderKeeper().GetSlashMeter(cacheCtx)
 			s.Require().True(slashMeter.GT(slashMeterBefore))
 
-			// Check that slash meter is still negative or 0,
-			// unless we are on the last iteration.
+			// Replenish candidate time should have been updated to be block time + replenish period.
+			expected := cacheCtx.BlockTime().Add(params.SlashMeterReplenishPeriod)
+			actual := s.providerApp.GetProviderKeeper().GetSlashMeterReplenishTimeCandidate(cacheCtx)
+			s.Require().Equal(expected, actual)
+
+			// CheckForSlashMeterReplenishment should not replenish meter here again (w/o another period elapsed).
+			// Replenish candidate should be in the future, and will not change.
+			candidate = s.providerApp.GetProviderKeeper().GetSlashMeterReplenishTimeCandidate(cacheCtx)
+			s.Require().True(cacheCtx.BlockTime().Before(candidate))
+			slashMeterBefore = s.providerApp.GetProviderKeeper().GetSlashMeter(cacheCtx)
+			s.providerApp.GetProviderKeeper().CheckForSlashMeterReplenishment(cacheCtx)
+			s.Require().Equal(slashMeterBefore, s.providerApp.GetProviderKeeper().GetSlashMeter(cacheCtx))
+			s.Require().Equal(candidate, s.providerApp.GetProviderKeeper().GetSlashMeterReplenishTimeCandidate(cacheCtx))
+
+			// Check that slash meter is still negative or 0, unless we are on the last iteration.
+			slashMeter = s.providerApp.GetProviderKeeper().GetSlashMeter(cacheCtx)
 			if i != tc.expectedReplenishesTillPositive-1 {
 				s.Require().False(slashMeter.IsPositive())
 			}
 		}
 
 		// Meter is positive at this point, and ready to handle the second slash packet.
-		slashMeter = s.providerApp.GetProviderKeeper().GetSlashMeter(s.providerCtx())
+		slashMeter = s.providerApp.GetProviderKeeper().GetSlashMeter(cacheCtx)
 		s.Require().True(slashMeter.IsPositive())
 
 		// Assert validator 2 is jailed once pending slash packets are handled in ccv endblocker.
 		s.providerChain.NextBlock()
-		vals = providerStakingKeeper.GetAllValidators(s.providerCtx())
+		vals = providerStakingKeeper.GetAllValidators(cacheCtx)
 		slashedVal = vals[2]
 		s.Require().True(slashedVal.IsJailed())
 
 		// Assert validator 2 has no power, this should be apparent next block,
 		// since the staking endblocker runs before the ccv endblocker.
 		s.providerChain.NextBlock()
-		lastValPower = providerStakingKeeper.GetLastValidatorPower(s.providerCtx(), slashedVal.GetOperator())
+		lastValPower = providerStakingKeeper.GetLastValidatorPower(cacheCtx, slashedVal.GetOperator())
 		s.Require().Equal(int64(0), lastValPower)
 	}
 }
@@ -494,9 +511,8 @@ func (s *CCVTestSuite) TestSlashingSmallValidators() {
 	delegateByIdx(s, delAddr, sdktypes.NewInt(9999999), 3)
 	s.providerChain.NextBlock()
 
-	// Replenish slash meter with default params and new total voting power.
-	customCtx := s.getCtxWithReplenishPeriodElapsed(s.providerCtx())
-	s.providerApp.GetProviderKeeper().CheckForSlashMeterReplenishment(customCtx)
+	// Initialize slash meter
+	s.providerApp.GetProviderKeeper().InitializeSlashMeter(s.providerCtx())
 
 	// Assert that we start out with no jailings
 	providerStakingKeeper := s.providerApp.GetE2eStakingKeeper()
@@ -747,8 +763,8 @@ func (s *CCVTestSuite) TestLeadingVSCMaturedAreDequeued() {
 	// Set slash meter to negative value to not allow any slash packets to be handled.
 	providerKeeper.SetSlashMeter(s.providerCtx(), sdktypes.NewInt(-1))
 
-	// Set last full time to block time, so no slash meter replenishment happens on end block.
-	providerKeeper.SetLastSlashMeterFullTime(s.providerCtx(), s.providerCtx().BlockTime())
+	// Set replenish time candidate so that no replenishment happens next block.
+	providerKeeper.SetSlashMeterReplenishTimeCandidate(s.providerCtx())
 
 	// Execute end blocker to dequeue only the leading vsc matured packets.
 	s.providerChain.NextBlock()
@@ -797,11 +813,8 @@ func (s *CCVTestSuite) replenishSlashMeterTillPositive() {
 	}
 }
 
-func (s *CCVTestSuite) getCtxWithReplenishPeriodElapsed(ctx sdktypes.Context) sdktypes.Context {
-
+func (s *CCVTestSuite) getCtxAfterReplenishCandidate(ctx sdktypes.Context) sdktypes.Context {
 	providerKeeper := s.providerApp.GetProviderKeeper()
-	lastFullTime := providerKeeper.GetLastSlashMeterFullTime(ctx)
-	replenishPeriod := providerKeeper.GetSlashMeterReplenishPeriod(ctx)
-
-	return ctx.WithBlockTime(lastFullTime.Add(replenishPeriod).Add(time.Minute))
+	candidate := providerKeeper.GetSlashMeterReplenishTimeCandidate(ctx)
+	return ctx.WithBlockTime(candidate.Add(time.Minute))
 }

x/ccv/provider/keeper/genesis_test.go

@@ -122,9 +122,9 @@ func TestInitAndExportGenesis(t *testing.T) {
 	expectedSlashMeterValue := sdk.NewInt(replenishFraction.MulInt(sdk.NewInt(100)).RoundInt64())
 	require.Equal(t, expectedSlashMeterValue, slashMeter)
 
-	// Expect last slash meter full time to be current block time
-	lastFullTime := pk.GetLastSlashMeterFullTime(ctx)
-	require.Equal(t, lastFullTime, ctx.BlockTime())
+	// Expect slash meter replenishment time candidate to be set to the current block time + replenish period
+	expectedCandidate := ctx.BlockTime().Add(pk.GetSlashMeterReplenishPeriod(ctx))
+	require.Equal(t, expectedCandidate, pk.GetSlashMeterReplenishTimeCandidate(ctx))
 
 	// check local provider chain states
 	ubdOps, found := pk.GetUnbondingOp(ctx, vscID)

x/ccv/provider/keeper/grpc_query.go

@@ -143,7 +143,7 @@ func (k Keeper) QueryThrottleState(goCtx context.Context, req *types.QueryThrott
 
 	meter := k.GetSlashMeter(ctx)
 	allowance := k.GetSlashMeterAllowance(ctx)
-	lastFullTime := k.GetLastSlashMeterFullTime(ctx) // always UTC
+	candidate := k.GetSlashMeterReplenishTimeCandidate(ctx) // always UTC
 	packets := []*types.ThrottledSlashPacket{}
 
 	// iterate global slash entries from all consumer chains
@@ -165,10 +165,10 @@ func (k Keeper) QueryThrottleState(goCtx context.Context, req *types.QueryThrott
 	}
 
 	return &types.QueryThrottleStateResponse{
-		SlashMeter:          meter.Int64(),
-		SlashMeterAllowance: allowance.Int64(),
-		LastFullTime:        lastFullTime,
-		Packets:             packets,
+		SlashMeter:             meter.Int64(),
+		SlashMeterAllowance:    allowance.Int64(),
+		NextReplenishCandidate: candidate,
+		Packets:                packets,
 	}, nil
 }
 

x/ccv/provider/keeper/relay.go

@@ -263,7 +263,7 @@ func (k Keeper) EndBlockCIS(ctx sdk.Context) {
 	// Note: CheckForSlashMeterReplenishment contains panics for the following scenarios, any of which should never occur
 	// if the protocol is correct and external data is properly validated:
 	//
-	// - Either SlashMeter or LastSlashMeterFullTime have not been set (both of which should be set in InitGenesis, see InitializeSlashMeter).
+	// - Either SlashMeter or SlashMeterReplenishTimeCandidate have not been set (both of which should be set in InitGenesis, see InitializeSlashMeter).
 	// - Params not being set (all of which should be set in InitGenesis).
 	// - Marshaling and/or store corruption errors.
 	// - Setting invalid slash meter values (see SetSlashMeter).

x/ccv/provider/keeper/throttle.go

@@ -98,22 +98,21 @@ func (k Keeper) HandlePacketDataForChain(ctx sdktypes.Context, consumerChainID s
 }
 
 // InitializeSlashMeter initializes the slash meter to it's max value (also its allowance),
-// and sets the last replenish time to the current block time.
+// and sets the replenish time candidate to one replenish period from current block time.
 func (k Keeper) InitializeSlashMeter(ctx sdktypes.Context) {
 	k.SetSlashMeter(ctx, k.GetSlashMeterAllowance(ctx))
-	k.SetLastSlashMeterFullTime(ctx, ctx.BlockTime())
+	k.SetSlashMeterReplenishTimeCandidate(ctx)
 }
 
 // CheckForSlashMeterReplenishment checks if the slash meter should be replenished, and if so, replenishes it.
-// Note: initial "last slash meter full time" is set in InitGenesis.
+// Note: initial slash meter replenish time candidate is set in InitGenesis.
 func (k Keeper) CheckForSlashMeterReplenishment(ctx sdktypes.Context) {
 
-	lastFullTime := k.GetLastSlashMeterFullTime(ctx)
-	replenishPeriod := k.GetSlashMeterReplenishPeriod(ctx)
-
-	// Replenish slash meter if enough time has passed since the last time it was full.
-	if ctx.BlockTime().UTC().After(lastFullTime.Add(replenishPeriod)) {
+	// Replenish slash meter if current time is equal to or after the current replenish candidate time.
+	if !ctx.BlockTime().UTC().Before(k.GetSlashMeterReplenishTimeCandidate(ctx)) {
 		k.ReplenishSlashMeter(ctx)
+		// Set replenish time candidate to one replenish period from now, since we just replenished.
+		k.SetSlashMeterReplenishTimeCandidate(ctx)
 	}
 
 	// The following logic exists to ensure the slash meter is not greater than the allowance for this block,
@@ -123,8 +122,9 @@ func (k Keeper) CheckForSlashMeterReplenishment(ctx sdktypes.Context) {
 	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())
+		// Update/set replenish time candidate to one replenish period from now.
+		// This time candidate will be updated in every future block until the slash meter becomes NOT full.
+		k.SetSlashMeterReplenishTimeCandidate(ctx)
 
 		// Ensure the slash meter is not greater than allowance this block,
 		// considering current total voting power.
@@ -582,26 +582,34 @@ func (k Keeper) SetSlashMeter(ctx sdktypes.Context, value sdktypes.Int) {
 	store.Set(providertypes.SlashMeterKey(), bz)
 }
 
-// GetLastSlashMeterFullTime returns the last UTC time the slash meter was full.
-func (k Keeper) GetLastSlashMeterFullTime(ctx sdktypes.Context) time.Time {
+// GetSlashMeterReplenishTimeCandidate returns the next UTC time the slash meter could potentially be replenished.
+//
+// Note: this value is the next time the slash meter will be replenished IFF the slash meter is NOT full.
+// Otherwise this value will be updated in every future block until the slash meter becomes NOT full.
+func (k Keeper) GetSlashMeterReplenishTimeCandidate(ctx sdktypes.Context) time.Time {
 	store := ctx.KVStore(k.storeKey)
-	bz := store.Get(providertypes.LastSlashMeterFullTimeKey())
+	bz := store.Get(providertypes.SlashMeterReplenishTimeCandidateKey())
 	if bz == nil {
-		// Last slash meter full time should be set as a part of InitGenesis and periodically updated by throttle logic,
+		// Slash meter replenish time candidate should be set as a part of InitGenesis and periodically updated by throttle logic,
 		// there is no deletion method exposed, so nil bytes would indicate something is very wrong.
-		panic("last slash meter full time not set")
+		panic("slash meter replenish time candidate not set")
 	}
 	time, err := sdktypes.ParseTimeBytes(bz)
 	if err != nil {
-		// We should have obtained value bytes that were serialized in SetLastSlashMeterFullTime,
+		// We should have obtained value bytes that were serialized in SetSlashMeterReplenishTimeCandidate,
 		// so an error here would indicate something is very wrong.
-		panic(fmt.Sprintf("failed to parse last slash meter full time: %s", err))
+		panic(fmt.Sprintf("failed to parse slash meter replenish time candidate: %s", err))
 	}
 	return time.UTC()
 }
 
-// SetLastSlashMeterReplenishTime sets the most recent time the slash meter was full.
-func (k Keeper) SetLastSlashMeterFullTime(ctx sdktypes.Context, time time.Time) {
+// SetSlashMeterReplenishTimeCandidate sets the next time the slash meter may be replenished
+// to the current block time + the configured slash meter replenish period.
+//
+// Note: this value is the next time the slash meter will be replenished IFF the slash meter is NOT full.
+// Otherwise this value will be updated in every future block until the slash meter becomes NOT full.
+func (k Keeper) SetSlashMeterReplenishTimeCandidate(ctx sdktypes.Context) {
 	store := ctx.KVStore(k.storeKey)
-	store.Set(providertypes.LastSlashMeterFullTimeKey(), sdktypes.FormatTimeBytes(time.UTC()))
+	timeToStore := ctx.BlockTime().UTC().Add(k.GetSlashMeterReplenishPeriod(ctx))
+	store.Set(providertypes.SlashMeterReplenishTimeCandidateKey(), sdktypes.FormatTimeBytes(timeToStore))
 }