Throttle bug fixes + req refactors (#565)

* fixes

* Update throttle.go

* fix tests

* set replenish frac = 1.0 for all test runs

* rm unmarshal func

* req refactors

* small lint fix

* comment adjustment

* found <-> jailed order swap

* 100% change

tests/e2e/throttle.go

@@ -5,8 +5,10 @@ import (
 
 	sdktypes "github.com/cosmos/cosmos-sdk/types"
 	stakingtypes "github.com/cosmos/cosmos-sdk/x/staking/types"
+	channeltypes "github.com/cosmos/ibc-go/v3/modules/core/04-channel/types"
 	icstestingutils "github.com/cosmos/interchain-security/testutil/ibc_testing"
 	providertypes "github.com/cosmos/interchain-security/x/ccv/provider/types"
+	ccvtypes "github.com/cosmos/interchain-security/x/ccv/types"
 	tmtypes "github.com/tendermint/tendermint/types"
 )
 
@@ -203,7 +205,7 @@ func (s *CCVTestSuite) TestMultiConsumerSlashPacketThrottling() {
 
 	// Confirm that the slash packet and trailing VSC matured packet
 	// were handled immediately for the first consumer (this packet was recv first).
-	s.confirmValidatorJailed(valsToSlash[0])
+	s.confirmValidatorJailed(valsToSlash[0], true)
 	s.Require().Equal(uint64(0), providerKeeper.GetPendingPacketDataSize(
 		s.providerCtx(), senderBundles[0].Chain.ChainID))
 
@@ -246,7 +248,7 @@ func (s *CCVTestSuite) TestMultiConsumerSlashPacketThrottling() {
 	// Now all 3 expected vals are jailed, and there are no more queued
 	// slash/vsc matured packets.
 	for _, val := range valsToSlash {
-		s.confirmValidatorJailed(val)
+		s.confirmValidatorJailed(val, true)
 	}
 	s.Require().Equal(uint64(0), providerKeeper.GetPendingPacketDataSize(
 		s.providerCtx(), senderBundles[0].Chain.ChainID))
@@ -339,14 +341,149 @@ func (s *CCVTestSuite) TestSlashMeterAllowanceChanges() {
 
 }
 
-func (s *CCVTestSuite) confirmValidatorJailed(tmVal tmtypes.Validator) {
+// TestSlashSameValidator tests the edge case that that the total slashed validator power
+// queued up for a single block exceeds the slash meter allowance,
+// but some of the slash packets are for the same validator, and therefore some packets
+// will be applied to a validator that is already jailed but still not unbonded (ie. still slashable).
+func (s *CCVTestSuite) TestSlashSameValidator() {
+
+	s.SetupAllCCVChannels()
+
+	// Setup 4 validators with 25% of the total power each.
+	s.setupValidatorPowers()
+
+	providerKeeper := s.providerApp.GetProviderKeeper()
+
+	// Set replenish fraction to 1.0 so that all sent packets should handled immediately (no throttling)
+	params := providerKeeper.GetParams(s.providerCtx())
+	params.SlashMeterReplenishFraction = "1.0"
+	providerKeeper.SetParams(s.providerCtx(), params)
+	providerKeeper.InitializeSlashMeter(s.providerCtx())
+
+	// Send a downtime and double-sign slash packet for 3/4 validators
+	// This will have a total slashing power of 150% total power.
+	tmval1 := s.providerChain.Vals.Validators[1]
+	tmval2 := s.providerChain.Vals.Validators[2]
+	tmval3 := s.providerChain.Vals.Validators[3]
+	s.setDefaultValSigningInfo(*tmval1)
+	s.setDefaultValSigningInfo(*tmval2)
+	s.setDefaultValSigningInfo(*tmval3)
+
+	packets := []channeltypes.Packet{
+		s.constructSlashPacketFromConsumer(s.getFirstBundle(), *tmval1, stakingtypes.Downtime, 1),
+		s.constructSlashPacketFromConsumer(s.getFirstBundle(), *tmval2, stakingtypes.Downtime, 2),
+		s.constructSlashPacketFromConsumer(s.getFirstBundle(), *tmval3, stakingtypes.Downtime, 3),
+		s.constructSlashPacketFromConsumer(s.getFirstBundle(), *tmval1, stakingtypes.DoubleSign, 4),
+		s.constructSlashPacketFromConsumer(s.getFirstBundle(), *tmval2, stakingtypes.DoubleSign, 5),
+		s.constructSlashPacketFromConsumer(s.getFirstBundle(), *tmval3, stakingtypes.DoubleSign, 6),
+	}
+
+	// Recv and queue all slash packets.
+	for _, packet := range packets {
+		slashPacketData := ccvtypes.SlashPacketData{}
+		ccvtypes.ModuleCdc.MustUnmarshalJSON(packet.GetData(), &slashPacketData)
+		providerKeeper.OnRecvSlashPacket(s.providerCtx(), packet, slashPacketData)
+	}
+
+	// We should have 6 pending slash packet entries queued.
+	s.Require().Len(providerKeeper.GetAllPendingSlashPacketEntries(s.providerCtx()), 6)
+
+	// Call next block to process all pending slash packets in end blocker.
+	s.providerChain.NextBlock()
+
+	// All slash packets should have been handled immediately, even though they totaled to 150% of total power.
+	s.Require().Len(providerKeeper.GetAllPendingSlashPacketEntries(s.providerCtx()), 0)
+}
+
+// Similar to TestSlashSameValidator, but 100% of val power is jailed a single block,
+// and in the first packets recv for that block.
+// This edge case should not occur in practice, but is useful to validate that
+// the slash meter can allow any number of slash packets to be handled in a single block when
+// its allowance is set to "1.0".
+func (s CCVTestSuite) TestSlashAllValidators() {
+
+	s.SetupAllCCVChannels()
+
+	// Setup 4 validators with 25% of the total power each.
+	s.setupValidatorPowers()
+
+	providerKeeper := s.providerApp.GetProviderKeeper()
+
+	// Set replenish fraction to 1.0 so that all sent packets should be handled immediately (no throttling)
+	params := providerKeeper.GetParams(s.providerCtx())
+	params.SlashMeterReplenishFraction = "1.0"
+	providerKeeper.SetParams(s.providerCtx(), params)
+	providerKeeper.InitializeSlashMeter(s.providerCtx())
+
+	// The packets to be recv in a single block, ordered as they will be recv.
+	packets := []channeltypes.Packet{}
+
+	// Track and increment ibc seq num for each packet, since these need to be unique.
+	ibcSeqNum := uint64(1)
+
+	// Instantiate a slash packet for each validator,
+	// these first 4 packets should jail 100% of the total power.
+	for _, val := range s.providerChain.Vals.Validators {
+		s.setDefaultValSigningInfo(*val)
+		packets = append(packets, s.constructSlashPacketFromConsumer(
+			s.getFirstBundle(), *val, stakingtypes.Downtime, ibcSeqNum))
+		ibcSeqNum++
+	}
+
+	// add 5 more slash packets for each validator, that will be handled in the same block.
+	for idx, val := range s.providerChain.Vals.Validators {
+		// Set infraction type based on even/odd index.
+		var infractionType stakingtypes.InfractionType
+		if idx%2 == 0 {
+			infractionType = stakingtypes.Downtime
+		} else {
+			infractionType = stakingtypes.DoubleSign
+		}
+		for i := 0; i < 5; i++ {
+			packets = append(packets, s.constructSlashPacketFromConsumer(
+				s.getFirstBundle(), *val, infractionType, ibcSeqNum))
+			ibcSeqNum++
+		}
+	}
+
+	// Recv and queue all slash packets.
+	for _, packet := range packets {
+		slashPacketData := ccvtypes.SlashPacketData{}
+		ccvtypes.ModuleCdc.MustUnmarshalJSON(packet.GetData(), &slashPacketData)
+		providerKeeper.OnRecvSlashPacket(s.providerCtx(), packet, slashPacketData)
+	}
+
+	// We should have 24 pending slash packet entries queued.
+	s.Require().Len(providerKeeper.GetAllPendingSlashPacketEntries(s.providerCtx()), 24)
+
+	// Call next block to process all pending slash packets in end blocker.
+	s.providerChain.NextBlock()
+
+	// All slash packets should have been handled immediately,
+	// even though the first 4 packets jailed 100% of the total power.
+	s.Require().Len(providerKeeper.GetAllPendingSlashPacketEntries(s.providerCtx()), 0)
+
+	// Sanity check that all validators are jailed.
+	for _, val := range s.providerChain.Vals.Validators {
+		// Do not check power, since val power is not yet updated by staking endblocker.
+		s.confirmValidatorJailed(*val, false)
+	}
+
+	// Nextblock would fail the test now, since ibctesting fails when
+	// "applying the validator changes would result in empty set".
+}
+
+func (s *CCVTestSuite) confirmValidatorJailed(tmVal tmtypes.Validator, checkPower bool) {
 	sdkVal, found := s.providerApp.GetE2eStakingKeeper().GetValidator(
 		s.providerCtx(), sdktypes.ValAddress(tmVal.Address))
 	s.Require().True(found)
-	valPower := s.providerApp.GetE2eStakingKeeper().GetLastValidatorPower(
-		s.providerCtx(), sdkVal.GetOperator())
-	s.Require().Equal(int64(0), valPower)
 	s.Require().True(sdkVal.IsJailed())
+
+	if checkPower {
+		valPower := s.providerApp.GetE2eStakingKeeper().GetLastValidatorPower(
+			s.providerCtx(), sdkVal.GetOperator())
+		s.Require().Equal(int64(0), valPower)
+	}
 }
 
 func (s *CCVTestSuite) confirmValidatorNotJailed(tmVal tmtypes.Validator, expectedPower int64) {

tests/integration/config.go

@@ -182,7 +182,8 @@ func KeyAssignmentTestRun() TestRun {
 					".app_state.slashing.params.signed_blocks_window = \"2\" | " +
 					".app_state.slashing.params.min_signed_per_window = \"0.500000000000000000\" | " +
 					".app_state.slashing.params.downtime_jail_duration = \"2s\" | " +
-					".app_state.slashing.params.slash_fraction_downtime = \"0.010000000000000000\"",
+					".app_state.slashing.params.slash_fraction_downtime = \"0.010000000000000000\" | " +
+					".app_state.provider.params.slash_meter_replenish_fraction = \"1.0\"", // This disables slash packet throttling
 			},
 			chainID("consu"): {
 				chainId:        chainID("consu"),
@@ -221,7 +222,8 @@ func DefaultTestRun() TestRun {
 					".app_state.slashing.params.signed_blocks_window = \"2\" | " +
 					".app_state.slashing.params.min_signed_per_window = \"0.500000000000000000\" | " +
 					".app_state.slashing.params.downtime_jail_duration = \"2s\" | " +
-					".app_state.slashing.params.slash_fraction_downtime = \"0.010000000000000000\"",
+					".app_state.slashing.params.slash_fraction_downtime = \"0.010000000000000000\" | " +
+					".app_state.provider.params.slash_meter_replenish_fraction = \"1.0\"", // This disables slash packet throttling
 			},
 			chainID("consu"): {
 				chainId:        chainID("consu"),
@@ -260,7 +262,8 @@ func DemocracyTestRun() TestRun {
 					".app_state.slashing.params.signed_blocks_window = \"2\" | " +
 					".app_state.slashing.params.min_signed_per_window = \"0.500000000000000000\" | " +
 					".app_state.slashing.params.downtime_jail_duration = \"2s\" | " +
-					".app_state.slashing.params.slash_fraction_downtime = \"0.010000000000000000\"",
+					".app_state.slashing.params.slash_fraction_downtime = \"0.010000000000000000\" | " +
+					".app_state.provider.params.slash_meter_replenish_fraction = \"1.0\"", // This disables slash packet throttling
 			},
 			chainID("democ"): {
 				chainId:        chainID("democ"),
@@ -300,7 +303,8 @@ func MultiConsumerTestRun() TestRun {
 					".app_state.slashing.params.signed_blocks_window = \"2\" | " +
 					".app_state.slashing.params.min_signed_per_window = \"0.500000000000000000\" | " +
 					".app_state.slashing.params.downtime_jail_duration = \"2s\" | " +
-					".app_state.slashing.params.slash_fraction_downtime = \"0.010000000000000000\"",
+					".app_state.slashing.params.slash_fraction_downtime = \"0.010000000000000000\" | " +
+					".app_state.provider.params.slash_meter_replenish_fraction = \"1.0\"", // This disables slash packet throttling
 			},
 			chainID("consu"): {
 				chainId:        chainID("consu"),

tests/integration/steps_double_sign.go

@@ -3,8 +3,13 @@ package main
 // simulates double signing on provider and vsc propagation to consumer chains
 // steps continue from downtime tests state.
 //
-// Note: These steps are not affected by slash packet throttling since
-// only one consumer initiated slash is implemented.
+// Note: These steps ARE affected by slash packet throttling, since the
+// consumer-initiated slash steps are executed after consumer-initiated downtime
+// slashes have already occurred. However slash packet throttling is
+// psuedo-disabled in this test by setting the slash meter replenish
+// fraction to 1.0 in the config file.
+//
+// TODO: test throttling logic directly, https://github.com/cosmos/interchain-security/issues/509
 func stepsDoubleSign(consumer1, consumer2 string) []Step {
 	return []Step{
 		{

testutil/e2e/debug_test.go

@@ -168,6 +168,14 @@ func TestSlashMeterAllowanceChanges(t *testing.T) {
 	runCCVTestByName(t, "TestSlashMeterAllowanceChanges")
 }
 
+func TestSlashSameValidator(t *testing.T) {
+	runCCVTestByName(t, "TestSlashSameValidator")
+}
+
+func TestSlashAllValidators(t *testing.T) {
+	runCCVTestByName(t, "TestSlashAllValidators")
+}
+
 //
 // Unbonding tests
 //

x/ccv/provider/keeper/relay.go

@@ -249,12 +249,17 @@ func (k Keeper) OnRecvSlashPacket(ctx sdk.Context, packet channeltypes.Packet, d
 		panic(fmt.Errorf("SlashPacket received on unknown channel %s", packet.DestinationChannel))
 	}
 
+	// The slash packet validator address may be known only on the consumer chain,
+	// in this case, it must be mapped back to the consensus address on the provider chain
+	consumerConsAddr := sdk.ConsAddress(data.Validator.Address)
+	providerConsAddr := k.GetProviderAddrFromConsumerAddr(ctx, chainID, consumerConsAddr)
+
 	// Queue a pending slash packet entry to the parent queue, which will be seen by the throttling logic
 	k.QueuePendingSlashPacketEntry(ctx, providertypes.NewSlashPacketEntry(
-		ctx.BlockTime(), // recv time
-		chainID,         // consumer chain id that sent the packet
-		packet.Sequence, // IBC sequence number of the packet
-		data.Validator.Address))
+		ctx.BlockTime(),   // recv time
+		chainID,           // consumer chain id that sent the packet
+		packet.Sequence,   // IBC sequence number of the packet
+		providerConsAddr)) // Provider consensus address of val to be slashed
 
 	// Queue slash packet data in the same (consumer chain specific) queue as vsc matured packet data,
 	// to enforce order of handling between the two packet types.

x/ccv/provider/keeper/throttle.go

@@ -29,9 +29,20 @@ func (k Keeper) HandlePendingSlashPackets(ctx sdktypes.Context) {
 	// Iterate through ordered (by received time) slash packet entries from any consumer chain
 	k.IteratePendingSlashPacketEntries(ctx, func(entry providertypes.SlashPacketEntry) (stop bool) {
 
+		// Obtain validator from the provider's consensus address.
+		// Note: if validator is not found or unbonded, this will be handled appropriately in HandleSlashPacket
+		val, found := k.stakingKeeper.GetValidatorByConsAddr(ctx, entry.ProviderValConsAddr)
+
 		// Obtain the validator power relevant to the slash packet that's about to be handled
 		// (this power will be removed via jailing or tombstoning)
-		valPower := k.stakingKeeper.GetLastValidatorPower(ctx, entry.ValAddr)
+		var valPower int64
+		if !found || val.IsJailed() {
+			// If validator is not found, or found but jailed, it's power is 0. This path is explicitly defined since the
+			// staking keeper's LastValidatorPower values are not updated till the staking keeper's endblocker.
+			valPower = 0
+		} else {
+			valPower = k.stakingKeeper.GetLastValidatorPower(ctx, val.GetOperator())
+		}
 
 		// Subtract this power from the slash meter
 		meter = meter.Sub(sdktypes.NewInt(valPower))
@@ -42,8 +53,8 @@ func (k Keeper) HandlePendingSlashPackets(ctx sdktypes.Context) {
 		// Store handled entry to be deleted after iteration is completed
 		handledEntries = append(handledEntries, entry)
 
-		// Do not handle anymore slash packets if the meter is 0 or negative in value
-		stop = !meter.IsPositive()
+		// Do not handle anymore slash packets if the meter is negative in value
+		stop = meter.IsNegative()
 		return stop
 	})
 
@@ -184,7 +195,7 @@ func (k Keeper) QueuePendingSlashPacketEntry(ctx sdktypes.Context,
 	entry providertypes.SlashPacketEntry) {
 	store := ctx.KVStore(k.storeKey)
 	key := providertypes.PendingSlashPacketEntryKey(entry)
-	store.Set(key, entry.ValAddr)
+	store.Set(key, entry.ProviderValConsAddr)
 }
 
 // GetAllPendingSlashPacketEntries returns all pending slash packet entries in the queue

x/ccv/provider/keeper/throttle_test.go

@@ -13,9 +13,9 @@ import (
 	"github.com/golang/mock/gomock"
 
 	sdktypes "github.com/cosmos/cosmos-sdk/types"
+	cryptoutil "github.com/cosmos/interchain-security/testutil/crypto"
 	testkeeper "github.com/cosmos/interchain-security/testutil/keeper"
 	"github.com/stretchr/testify/require"
-	"github.com/tendermint/tendermint/crypto/ed25519"
 	tmtypes "github.com/tendermint/tendermint/types"
 	"golang.org/x/exp/slices"
 )
@@ -626,7 +626,7 @@ func TestPendingSlashPacketEntries(t *testing.T) {
 		entry := providertypes.NewSlashPacketEntry(now.Local(),
 			fmt.Sprintf("chain-%d", i),
 			8, // all with seq = 8
-			ed25519.GenPrivKey().PubKey().Address())
+			cryptoutil.NewCryptoIdentityFromIntSeed(i).SDKConsAddress())
 		providerKeeper.QueuePendingSlashPacketEntry(ctx, entry)
 	}
 	entries = providerKeeper.GetAllPendingSlashPacketEntries(ctx)
@@ -637,7 +637,7 @@ func TestPendingSlashPacketEntries(t *testing.T) {
 		entry := providertypes.NewSlashPacketEntry(now.Add(time.Hour).Local(),
 			fmt.Sprintf("chain-%d", i),
 			9, // all with seq = 9
-			ed25519.GenPrivKey().PubKey().Address())
+			cryptoutil.NewCryptoIdentityFromIntSeed(i).SDKConsAddress())
 		providerKeeper.QueuePendingSlashPacketEntry(ctx, entry)
 	}
 
@@ -660,7 +660,7 @@ func TestPendingSlashPacketEntries(t *testing.T) {
 		entry := providertypes.NewSlashPacketEntry(now.Add(2*time.Hour).Local(),
 			fmt.Sprintf("chain-%d", i+5),
 			7697, // all with seq = 7697
-			ed25519.GenPrivKey().PubKey().Address())
+			cryptoutil.NewCryptoIdentityFromIntSeed(i).SDKConsAddress())
 		providerKeeper.QueuePendingSlashPacketEntry(ctx, entry)
 	}
 
@@ -689,17 +689,17 @@ func TestPendingSlashPacketEntries(t *testing.T) {
 			require.Equal(t, now, entry.RecvTime)
 			require.Equal(t, fmt.Sprintf("chain-%d", idx), entry.ConsumerChainID)
 			require.Equal(t, uint64(8), entry.IbcSeqNum)
-			require.NotEmpty(t, entry.ValAddr)
+			require.NotEmpty(t, entry.ProviderValConsAddr)
 		case 3, 4, 5:
 			require.Equal(t, now.Add(time.Hour), entry.RecvTime)
 			require.Equal(t, fmt.Sprintf("chain-%d", idx-3), entry.ConsumerChainID)
 			require.Equal(t, uint64(9), entry.IbcSeqNum)
-			require.NotEmpty(t, entry.ValAddr)
+			require.NotEmpty(t, entry.ProviderValConsAddr)
 		case 6, 7, 8:
 			require.Equal(t, now.Add(2*time.Hour), entry.RecvTime)
 			require.Equal(t, fmt.Sprintf("chain-%d", idx-6+5), entry.ConsumerChainID)
 			require.Equal(t, uint64(7697), entry.IbcSeqNum)
-			require.NotEmpty(t, entry.ValAddr)
+			require.NotEmpty(t, entry.ProviderValConsAddr)
 		default:
 			t.Fatalf("unexpected entry index %d", idx)
 		}
@@ -761,13 +761,13 @@ func TestPendingSlashPacketEntryDeletion(t *testing.T) {
 
 	// Instantiate entries in the expected order we wish to get them back as (ordered by recv time)
 	entries = []providertypes.SlashPacketEntry{}
-	entries = append(entries, providertypes.NewSlashPacketEntry(now, "chain-0", 1, ed25519.GenPrivKey().PubKey().Address()))
-	entries = append(entries, providertypes.NewSlashPacketEntry(now.Add(time.Hour).UTC(), "chain-1", 178, ed25519.GenPrivKey().PubKey().Address()))
-	entries = append(entries, providertypes.NewSlashPacketEntry(now.Add(2*time.Hour).Local(), "chain-2", 89, ed25519.GenPrivKey().PubKey().Address()))
-	entries = append(entries, providertypes.NewSlashPacketEntry(now.Add(3*time.Hour).In(time.FixedZone("UTC-8", -8*60*60)), "chain-3", 23423, ed25519.GenPrivKey().PubKey().Address()))
-	entries = append(entries, providertypes.NewSlashPacketEntry(now.Add(4*time.Hour).Local(), "chain-4", 323, ed25519.GenPrivKey().PubKey().Address()))
-	entries = append(entries, providertypes.NewSlashPacketEntry(now.Add(5*time.Hour).UTC(), "chain-5", 18, ed25519.GenPrivKey().PubKey().Address()))
-	entries = append(entries, providertypes.NewSlashPacketEntry(now.Add(6*time.Hour).Local(), "chain-6", 2, ed25519.GenPrivKey().PubKey().Address()))
+	entries = append(entries, providertypes.NewSlashPacketEntry(now, "chain-0", 1, cryptoutil.NewCryptoIdentityFromIntSeed(0).SDKConsAddress()))
+	entries = append(entries, providertypes.NewSlashPacketEntry(now.Add(time.Hour).UTC(), "chain-1", 178, cryptoutil.NewCryptoIdentityFromIntSeed(1).SDKConsAddress()))
+	entries = append(entries, providertypes.NewSlashPacketEntry(now.Add(2*time.Hour).Local(), "chain-2", 89, cryptoutil.NewCryptoIdentityFromIntSeed(2).SDKConsAddress()))
+	entries = append(entries, providertypes.NewSlashPacketEntry(now.Add(3*time.Hour).In(time.FixedZone("UTC-8", -8*60*60)), "chain-3", 23423, cryptoutil.NewCryptoIdentityFromIntSeed(3).SDKConsAddress()))
+	entries = append(entries, providertypes.NewSlashPacketEntry(now.Add(4*time.Hour).Local(), "chain-4", 323, cryptoutil.NewCryptoIdentityFromIntSeed(4).SDKConsAddress()))
+	entries = append(entries, providertypes.NewSlashPacketEntry(now.Add(5*time.Hour).UTC(), "chain-5", 18, cryptoutil.NewCryptoIdentityFromIntSeed(5).SDKConsAddress()))
+	entries = append(entries, providertypes.NewSlashPacketEntry(now.Add(6*time.Hour).Local(), "chain-6", 2, cryptoutil.NewCryptoIdentityFromIntSeed(6).SDKConsAddress()))
 
 	// Instantiate shuffled copy of above slice
 	shuffledEntries := append([]providertypes.SlashPacketEntry{}, entries...)