From d98ef7053b29ca62269ae47aa12fad88d2afdafa Mon Sep 17 00:00:00 2001 From: Shawn Marshall-Spitzbart <44221603+smarshall-spitzbart@users.noreply.github.com> Date: Thu, 1 Dec 2022 15:40:26 -0800 Subject: [PATCH 01/31] some panics --- x/ccv/provider/keeper/keeper.go | 14 ++++++-------- x/ccv/provider/keeper/relay.go | 8 ++------ 2 files changed, 8 insertions(+), 14 deletions(-) diff --git a/x/ccv/provider/keeper/keeper.go b/x/ccv/provider/keeper/keeper.go index d61f30bfd8..1ee659daa8 100644 --- a/x/ccv/provider/keeper/keeper.go +++ b/x/ccv/provider/keeper/keeper.go @@ -303,14 +303,13 @@ func (k Keeper) SetConsumerChain(ctx sdk.Context, channelID string) error { } // Save UnbondingOp by unique ID -func (k Keeper) SetUnbondingOp(ctx sdk.Context, unbondingOp ccv.UnbondingOp) error { +func (k Keeper) SetUnbondingOp(ctx sdk.Context, unbondingOp ccv.UnbondingOp) { store := ctx.KVStore(k.storeKey) bz, err := unbondingOp.Marshal() if err != nil { - return err + panic(fmt.Errorf("unbonding op could not be marshaled: %w", err)) } store.Set(types.UnbondingOpKey(unbondingOp.Id), bz) - return nil } // Get UnbondingOp by unique ID @@ -450,13 +449,13 @@ func (k Keeper) GetMaturedUnbondingOps(ctx sdk.Context) (ids []uint64, err error } // AppendMaturedUnbondingOps adds a list of ids to the list of matured unbonding operation ids -func (k Keeper) AppendMaturedUnbondingOps(ctx sdk.Context, ids []uint64) error { +func (k Keeper) AppendMaturedUnbondingOps(ctx sdk.Context, ids []uint64) { if len(ids) == 0 { - return nil + return } existingIds, err := k.GetMaturedUnbondingOps(ctx) if err != nil { - return err + panic(fmt.Sprintf("failed to get matured unbonding operations: %s", err)) } maturedOps := ccv.MaturedUnbondingOps{ @@ -466,10 +465,9 @@ func (k Keeper) AppendMaturedUnbondingOps(ctx sdk.Context, ids []uint64) error { store := ctx.KVStore(k.storeKey) bz, err := maturedOps.Marshal() if err != nil { - return err + panic(fmt.Sprintf("failed to marshal matured unbonding operations: %s", err)) } store.Set(types.MaturedUnbondingOpsKey(), bz) - return nil } // ConsumeMaturedUnbondingOps empties and returns list of matured unbonding operation ids (if it exists) diff --git a/x/ccv/provider/keeper/relay.go b/x/ccv/provider/keeper/relay.go index 3943301d20..166104be55 100644 --- a/x/ccv/provider/keeper/relay.go +++ b/x/ccv/provider/keeper/relay.go @@ -56,14 +56,10 @@ func (k Keeper) OnRecvVSCMaturedPacket( // Delete unbonding op k.DeleteUnbondingOp(ctx, unbondingOp.Id) } else { - if err := k.SetUnbondingOp(ctx, unbondingOp); err != nil { - panic(fmt.Errorf("unbonding op could not be persisted: %w", err)) - } + k.SetUnbondingOp(ctx, unbondingOp) } } - if err := k.AppendMaturedUnbondingOps(ctx, maturedIds); err != nil { - panic(fmt.Errorf("mature unbonding ops could not be appended: %w", err)) - } + k.AppendMaturedUnbondingOps(ctx, maturedIds) // clean up index k.DeleteUnbondingOpIndex(ctx, chainID, data.ValsetUpdateId) From 19f153df259f18317478802f427a4180d6883e18 Mon Sep 17 00:00:00 2001 From: Shawn Marshall-Spitzbart <44221603+smarshall-spitzbart@users.noreply.github.com> Date: Thu, 1 Dec 2022 15:55:49 -0800 Subject: [PATCH 02/31] Update proposal.go --- x/ccv/provider/keeper/proposal.go | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/x/ccv/provider/keeper/proposal.go b/x/ccv/provider/keeper/proposal.go index ae218acc6d..d24629ba4f 100644 --- a/x/ccv/provider/keeper/proposal.go +++ b/x/ccv/provider/keeper/proposal.go @@ -193,14 +193,10 @@ func (k Keeper) StopConsumerChain(ctx sdk.Context, chainID string, lockUbd, clos // Delete unbonding op k.DeleteUnbondingOp(ctx, unbondingOp.Id) } else { - if err := k.SetUnbondingOp(ctx, unbondingOp); err != nil { - panic(fmt.Errorf("unbonding op could not be persisted: %w", err)) - } + k.SetUnbondingOp(ctx, unbondingOp) } } - if err := k.AppendMaturedUnbondingOps(ctx, maturedIds); err != nil { - panic(fmt.Errorf("mature unbonding ops could not be appended: %w", err)) - } + k.AppendMaturedUnbondingOps(ctx, maturedIds) vscIDs = append(vscIDs, vscID) return false // do not stop the iteration From 54d63a0e772baa555bc17bf7f75f83ebb6f361c7 Mon Sep 17 00:00:00 2001 From: Shawn Marshall-Spitzbart <44221603+smarshall-spitzbart@users.noreply.github.com> Date: Thu, 1 Dec 2022 15:59:33 -0800 Subject: [PATCH 03/31] mas fixes --- x/ccv/provider/keeper/genesis.go | 8 ++------ x/ccv/provider/keeper/hooks.go | 11 +++++------ x/ccv/provider/keeper/keeper_test.go | 6 ++---- 3 files changed, 9 insertions(+), 16 deletions(-) diff --git a/x/ccv/provider/keeper/genesis.go b/x/ccv/provider/keeper/genesis.go index 25233404cc..d866a81d3e 100644 --- a/x/ccv/provider/keeper/genesis.go +++ b/x/ccv/provider/keeper/genesis.go @@ -40,17 +40,13 @@ func (k Keeper) InitGenesis(ctx sdk.Context, genState *types.GenesisState) { k.SetPendingConsumerRemovalProp(ctx, prop.ChainId, prop.StopTime) } for _, ubdOp := range genState.UnbondingOps { - if err := k.SetUnbondingOp(ctx, ubdOp); err != nil { - panic(fmt.Errorf("unbonding op could not be persisted: %w", err)) - } + k.SetUnbondingOp(ctx, ubdOp) } // Note that MatureUnbondingOps aren't stored across blocks, but it // might be used after implementing sovereign to consumer transition if genState.MatureUnbondingOps != nil { - if err := k.AppendMaturedUnbondingOps(ctx, genState.MatureUnbondingOps.Ids); err != nil { - panic(err) - } + k.AppendMaturedUnbondingOps(ctx, genState.MatureUnbondingOps.Ids) } // Set initial state for each consumer chain diff --git a/x/ccv/provider/keeper/hooks.go b/x/ccv/provider/keeper/hooks.go index 79f1e42118..ffd9dae56d 100644 --- a/x/ccv/provider/keeper/hooks.go +++ b/x/ccv/provider/keeper/hooks.go @@ -46,12 +46,11 @@ func (h Hooks) AfterUnbondingInitiated(ctx sdk.Context, ID uint64) error { h.k.SetUnbondingOpIndex(ctx, consumerChainID, valsetUpdateID, index) } - // Set unbondingOp - if err := h.k.SetUnbondingOp(ctx, unbondingOp); err != nil { - // If there was an error persisting the unbonding op, panic to end execution for - // the current tx and prevent committal of this invalid state. - panic(fmt.Errorf("unbonding op could not be persisted: %w", err)) - } + // Set unbonding op. + // If there is an error persisting the unbonding op, the method will + // panic to end execution for the current tx and prevent committal + // of this invalid state. + h.k.SetUnbondingOp(ctx, unbondingOp) // Call back into staking to tell it to stop this op from unbonding when the unbonding period is complete if err := h.k.stakingKeeper.PutUnbondingOnHold(ctx, ID); err != nil { diff --git a/x/ccv/provider/keeper/keeper_test.go b/x/ccv/provider/keeper/keeper_test.go index b0aee669a6..44b40d9f07 100644 --- a/x/ccv/provider/keeper/keeper_test.go +++ b/x/ccv/provider/keeper/keeper_test.go @@ -286,8 +286,7 @@ func TestMaturedUnbondingOps(t *testing.T) { require.Nil(t, ids) unbondingOpIds := []uint64{0, 1, 2, 3, 4, 5, 6} - err = providerKeeper.AppendMaturedUnbondingOps(ctx, unbondingOpIds) - require.NoError(t, err) + providerKeeper.AppendMaturedUnbondingOps(ctx, unbondingOpIds) ids, err = providerKeeper.ConsumeMaturedUnbondingOps(ctx) require.NoError(t, err) @@ -508,8 +507,7 @@ func TestIterateOverUnbondingOps(t *testing.T) { } for _, op := range ops { - err := pk.SetUnbondingOp(ctx, op) - require.NoError(t, err, "SetUnbondingOp returned err %s", err) + pk.SetUnbondingOp(ctx, op) } result := []ccv.UnbondingOp{} From 19ff67fef400e5c6b7f51156bde00eef8757b575 Mon Sep 17 00:00:00 2001 From: Shawn Marshall-Spitzbart <44221603+smarshall-spitzbart@users.noreply.github.com> Date: Thu, 1 Dec 2022 17:33:48 -0800 Subject: [PATCH 04/31] wip --- tests/e2e/slashing.go | 372 +++++++++++++-------------- x/ccv/consumer/keeper/relay.go | 5 + x/ccv/provider/keeper/keeper_test.go | 4 +- x/ccv/provider/keeper/relay.go | 150 +++++++---- 4 files changed, 297 insertions(+), 234 deletions(-) diff --git a/tests/e2e/slashing.go b/tests/e2e/slashing.go index 1345b4ef72..a1f7aaadac 100644 --- a/tests/e2e/slashing.go +++ b/tests/e2e/slashing.go @@ -1,7 +1,6 @@ package e2e import ( - "fmt" "time" cryptocodec "github.com/cosmos/cosmos-sdk/crypto/codec" @@ -238,14 +237,13 @@ func (suite *CCVTestSuite) TestHandleSlashPacketDoubleSigning() { slashingtypes.ValidatorSigningInfo{Address: consAddr.String()}, ) - _, err := providerKeeper.HandleSlashPacket(suite.providerCtx(), suite.consumerChain.ChainID, + providerKeeper.HandleSlashPacket(suite.providerCtx(), suite.consumerChain.ChainID, ccv.NewSlashPacketData( abci.Validator{Address: tmVal.Address, Power: 0}, uint64(0), stakingtypes.DoubleSign, ), ) - suite.NoError(err) // verify that validator is jailed in the staking and slashing modules' states suite.Require().True(providerStakingKeeper.IsValidatorJailed(suite.providerCtx(), consAddr)) @@ -256,189 +254,191 @@ func (suite *CCVTestSuite) TestHandleSlashPacketDoubleSigning() { } // TestHandleSlashPacketErrors tests errors for the HandleSlashPacket method in an e2e testing setting -func (suite *CCVTestSuite) TestHandleSlashPacketErrors() { - providerStakingKeeper := suite.providerApp.GetE2eStakingKeeper() - ProviderKeeper := suite.providerApp.GetProviderKeeper() - providerSlashingKeeper := suite.providerApp.GetE2eSlashingKeeper() - consumerChainID := suite.consumerChain.ChainID - - // sync contexts block height - ctx := suite.providerCtx() - - // expect an error if initial block height isn't set for consumer chain - _, err := ProviderKeeper.HandleSlashPacket(ctx, consumerChainID, ccv.SlashPacketData{}) - suite.Require().Error(err, "slash validator with invalid infraction height") - - // save VSC ID - vID := ProviderKeeper.GetValidatorSetUpdateId(ctx) - - // remove block height for current VSC ID - ProviderKeeper.DeleteValsetUpdateBlockHeight(ctx, vID) - - // expect an error if block height mapping VSC ID is zero - _, err = ProviderKeeper.HandleSlashPacket(ctx, consumerChainID, ccv.SlashPacketData{ValsetUpdateId: vID}) - suite.Require().Error(err, "slash with height mapping to zero") - - // construct slashing packet with non existing validator - slashingPkt := ccv.NewSlashPacketData( - abci.Validator{Address: ed25519.GenPrivKey().PubKey().Address(), - Power: int64(0)}, uint64(0), stakingtypes.Downtime, - ) - - // Set initial block height for consumer chain - ProviderKeeper.SetInitChainHeight(ctx, consumerChainID, uint64(ctx.BlockHeight())) - - // expect the slash to not succeed if validator doesn't exist - success, err := ProviderKeeper.HandleSlashPacket(ctx, consumerChainID, slashingPkt) - suite.Require().NoError(err, "slashing an unknown validator should not result in error") - suite.Require().False(success, "did slash unknown validator") - - // jail an existing validator - val := suite.providerChain.Vals.Validators[0] - consAddr := sdk.ConsAddress(val.Address) - providerStakingKeeper.Jail(ctx, consAddr) - // commit block to set VSC ID - suite.coordinator.CommitBlock(suite.providerChain) - // Update suite.ctx bc CommitBlock updates only providerChain's current header block height - ctx = suite.providerChain.GetContext() - suite.Require().NotZero(ProviderKeeper.GetValsetUpdateBlockHeight(ctx, vID)) - - // create validator signing info - valInfo := slashingtypes.NewValidatorSigningInfo(sdk.ConsAddress(val.Address), ctx.BlockHeight(), - ctx.BlockHeight()-1, time.Time{}.UTC(), false, int64(0)) - providerSlashingKeeper.SetValidatorSigningInfo(ctx, sdk.ConsAddress(val.Address), valInfo) - - // update validator address and VSC ID - slashingPkt.Validator.Address = val.Address - slashingPkt.ValsetUpdateId = vID - - // expect to slash and jail validator - _, err = ProviderKeeper.HandleSlashPacket(ctx, consumerChainID, slashingPkt) - suite.Require().NoError(err, "did slash jail validator") - - // expect error when infraction type in unspecified - tmAddr := suite.providerChain.Vals.Validators[1].Address - slashingPkt.Validator.Address = tmAddr - slashingPkt.Infraction = stakingtypes.InfractionEmpty - - valInfo.Address = sdk.ConsAddress(tmAddr).String() - providerSlashingKeeper.SetValidatorSigningInfo(ctx, sdk.ConsAddress(tmAddr), valInfo) - - _, err = ProviderKeeper.HandleSlashPacket(ctx, consumerChainID, slashingPkt) - suite.Require().EqualError(err, fmt.Sprintf("invalid infraction type: %v", stakingtypes.InfractionEmpty)) - - // expect to slash jail validator - slashingPkt.Infraction = stakingtypes.DoubleSign - _, err = ProviderKeeper.HandleSlashPacket(ctx, consumerChainID, slashingPkt) - suite.Require().NoError(err) - - // expect the slash to not succeed when validator is tombstoned - success, _ = ProviderKeeper.HandleSlashPacket(ctx, consumerChainID, slashingPkt) - suite.Require().False(success) -} - -// TestHandleSlashPacketDistribution tests the slashing of an undelegation balance -// by varying the slash packet VSC ID mapping to infraction heights -// lesser, equal or greater than the undelegation entry creation height -func (suite *CCVTestSuite) TestHandleSlashPacketDistribution() { - providerKeeper := suite.providerApp.GetProviderKeeper() - providerStakingKeeper := suite.providerApp.GetE2eStakingKeeper() - providerSlashingKeeper := suite.providerApp.GetE2eSlashingKeeper() - - // choose a validator - tmValidator := suite.providerChain.Vals.Validators[0] - valAddr, err := sdk.ValAddressFromHex(tmValidator.Address.String()) - suite.Require().NoError(err) - - validator, found := providerStakingKeeper.GetValidator(suite.providerChain.GetContext(), valAddr) - suite.Require().True(found) - - // unbonding operations parameters - delAddr := suite.providerChain.SenderAccount.GetAddress() - bondAmt := sdk.NewInt(1000000) - - // new delegator shares used - testShares := sdk.Dec{} - - // setup the test with a delegation, a no-op and an undelegation - setupOperations := []struct { - fn func(suite *CCVTestSuite) error - }{ - { - func(suite *CCVTestSuite) error { - testShares, err = providerStakingKeeper.Delegate(suite.providerChain.GetContext(), delAddr, bondAmt, stakingtypes.Unbonded, stakingtypes.Validator(validator), true) - return err - }, - }, { - func(suite *CCVTestSuite) error { - return nil - }, - }, { - // undelegate a quarter of the new shares created - func(suite *CCVTestSuite) error { - _, err = providerStakingKeeper.Undelegate(suite.providerChain.GetContext(), delAddr, valAddr, testShares.QuoInt64(4)) - return err - }, - }, - } - - // execute the setup operations, distributed uniformly in three blocks. - // For each of them, save their current VSC Id value which map correspond respectively - // to the block heights lesser, equal and greater than the undelegation creation height. - vscIDs := make([]uint64, 0, 3) - for _, so := range setupOperations { - err := so.fn(suite) - suite.Require().NoError(err) - - vscIDs = append(vscIDs, providerKeeper.GetValidatorSetUpdateId(suite.providerChain.GetContext())) - suite.providerChain.NextBlock() - } - - // create validator signing info to test slashing - providerSlashingKeeper.SetValidatorSigningInfo( - suite.providerChain.GetContext(), - sdk.ConsAddress(tmValidator.Address), - slashingtypes.ValidatorSigningInfo{Address: tmValidator.Address.String()}, - ) - - // the test cases verify that only the unbonding tokens get slashed for the VSC ids - // mapping to the block heights before and during the undelegation otherwise not. - testCases := []struct { - expSlash bool - vscID uint64 - }{ - {expSlash: true, vscID: vscIDs[0]}, - {expSlash: true, vscID: vscIDs[1]}, - {expSlash: false, vscID: vscIDs[2]}, - } - - // save unbonding balance before slashing tests - ubd, found := providerStakingKeeper.GetUnbondingDelegation( - suite.providerChain.GetContext(), delAddr, valAddr) - suite.Require().True(found) - ubdBalance := ubd.Entries[0].Balance - - for _, tc := range testCases { - slashPacket := ccv.NewSlashPacketData( - abci.Validator{Address: tmValidator.Address, Power: tmValidator.VotingPower}, - tc.vscID, - stakingtypes.Downtime, - ) - - // slash - _, err := providerKeeper.HandleSlashPacket(suite.providerChain.GetContext(), suite.consumerChain.ChainID, slashPacket) - suite.Require().NoError(err) - - ubd, found := providerStakingKeeper.GetUnbondingDelegation(suite.providerChain.GetContext(), delAddr, valAddr) - suite.Require().True(found) - - isUbdSlashed := ubdBalance.GT(ubd.Entries[0].Balance) - suite.Require().True(tc.expSlash == isUbdSlashed) - - // update balance - ubdBalance = ubd.Entries[0].Balance - } -} +// TODO: will need to update this ish +// TODO: also change capatalization of provider keeper +// TODO: also more tests? +// func (suite *CCVTestSuite) TestHandleSlashPacketErrors() { +// providerStakingKeeper := suite.providerApp.GetE2eStakingKeeper() +// ProviderKeeper := suite.providerApp.GetProviderKeeper() +// providerSlashingKeeper := suite.providerApp.GetE2eSlashingKeeper() +// consumerChainID := suite.consumerChain.ChainID + +// // sync contexts block height +// ctx := suite.providerCtx() + +// // expect an error if initial block height isn't set for consumer chain +// _, err := ProviderKeeper.HandleSlashPacket(ctx, consumerChainID, ccv.SlashPacketData{}) +// suite.Require().Error(err, "slash validator with invalid infraction height") + +// // save VSC ID +// vID := ProviderKeeper.GetValidatorSetUpdateId(ctx) + +// // remove block height for current VSC ID +// ProviderKeeper.DeleteValsetUpdateBlockHeight(ctx, vID) + +// // expect an error if block height mapping VSC ID is zero +// _, err = ProviderKeeper.HandleSlashPacket(ctx, consumerChainID, ccv.SlashPacketData{ValsetUpdateId: vID}) +// suite.Require().Error(err, "slash with height mapping to zero") + +// // construct slashing packet with non existing validator +// slashingPkt := ccv.NewSlashPacketData( +// abci.Validator{Address: ed25519.GenPrivKey().PubKey().Address(), +// Power: int64(0)}, uint64(0), stakingtypes.Downtime, +// ) + +// // Set initial block height for consumer chain +// ProviderKeeper.SetInitChainHeight(ctx, consumerChainID, uint64(ctx.BlockHeight())) + +// // expect the slash to not succeed if validator doesn't exist +// success, err := ProviderKeeper.HandleSlashPacket(ctx, consumerChainID, slashingPkt) +// suite.Require().NoError(err, "slashing an unknown validator should not result in error") +// suite.Require().False(success, "did slash unknown validator") + +// // jail an existing validator +// val := suite.providerChain.Vals.Validators[0] +// consAddr := sdk.ConsAddress(val.Address) +// providerStakingKeeper.Jail(ctx, consAddr) +// // commit block to set VSC ID +// suite.coordinator.CommitBlock(suite.providerChain) +// // Update suite.ctx bc CommitBlock updates only providerChain's current header block height +// ctx = suite.providerChain.GetContext() +// suite.Require().NotZero(ProviderKeeper.GetValsetUpdateBlockHeight(ctx, vID)) + +// // create validator signing info +// valInfo := slashingtypes.NewValidatorSigningInfo(sdk.ConsAddress(val.Address), ctx.BlockHeight(), +// ctx.BlockHeight()-1, time.Time{}.UTC(), false, int64(0)) +// providerSlashingKeeper.SetValidatorSigningInfo(ctx, sdk.ConsAddress(val.Address), valInfo) + +// // update validator address and VSC ID +// slashingPkt.Validator.Address = val.Address +// slashingPkt.ValsetUpdateId = vID + +// // expect to slash and jail validator +// _, err = ProviderKeeper.HandleSlashPacket(ctx, consumerChainID, slashingPkt) +// suite.Require().NoError(err, "did slash jail validator") + +// // expect error when infraction type in unspecified +// tmAddr := suite.providerChain.Vals.Validators[1].Address +// slashingPkt.Validator.Address = tmAddr +// slashingPkt.Infraction = stakingtypes.InfractionEmpty + +// valInfo.Address = sdk.ConsAddress(tmAddr).String() +// providerSlashingKeeper.SetValidatorSigningInfo(ctx, sdk.ConsAddress(tmAddr), valInfo) + +// _, err = ProviderKeeper.HandleSlashPacket(ctx, consumerChainID, slashingPkt) +// suite.Require().EqualError(err, fmt.Sprintf("invalid infraction type: %v", stakingtypes.InfractionEmpty)) + +// // expect to slash jail validator +// slashingPkt.Infraction = stakingtypes.DoubleSign +// _, err = ProviderKeeper.HandleSlashPacket(ctx, consumerChainID, slashingPkt) +// suite.Require().NoError(err) + +// // expect the slash to not succeed when validator is tombstoned +// success, _ = ProviderKeeper.HandleSlashPacket(ctx, consumerChainID, slashingPkt) +// suite.Require().False(success) +// } + +// // TestHandleSlashPacketDistribution tests the slashing of an undelegation balance +// // by varying the slash packet VSC ID mapping to infraction heights +// // lesser, equal or greater than the undelegation entry creation height +// func (suite *CCVTestSuite) TestHandleSlashPacketDistribution() { +// providerKeeper := suite.providerApp.GetProviderKeeper() +// providerStakingKeeper := suite.providerApp.GetE2eStakingKeeper() +// providerSlashingKeeper := suite.providerApp.GetE2eSlashingKeeper() + +// // choose a validator +// tmValidator := suite.providerChain.Vals.Validators[0] +// valAddr, err := sdk.ValAddressFromHex(tmValidator.Address.String()) +// suite.Require().NoError(err) + +// validator, found := providerStakingKeeper.GetValidator(suite.providerChain.GetContext(), valAddr) +// suite.Require().True(found) + +// // unbonding operations parameters +// delAddr := suite.providerChain.SenderAccount.GetAddress() +// bondAmt := sdk.NewInt(1000000) + +// // new delegator shares used +// testShares := sdk.Dec{} + +// // setup the test with a delegation, a no-op and an undelegation +// setupOperations := []struct { +// fn func(suite *CCVTestSuite) error +// }{ +// { +// func(suite *CCVTestSuite) error { +// testShares, err = providerStakingKeeper.Delegate(suite.providerChain.GetContext(), delAddr, bondAmt, stakingtypes.Unbonded, stakingtypes.Validator(validator), true) +// return err +// }, +// }, { +// func(suite *CCVTestSuite) error { +// return nil +// }, +// }, { +// // undelegate a quarter of the new shares created +// func(suite *CCVTestSuite) error { +// _, err = providerStakingKeeper.Undelegate(suite.providerChain.GetContext(), delAddr, valAddr, testShares.QuoInt64(4)) +// return err +// }, +// }, +// } + +// // execute the setup operations, distributed uniformly in three blocks. +// // For each of them, save their current VSC Id value which map correspond respectively +// // to the block heights lesser, equal and greater than the undelegation creation height. +// vscIDs := make([]uint64, 0, 3) +// for _, so := range setupOperations { +// err := so.fn(suite) +// suite.Require().NoError(err) + +// vscIDs = append(vscIDs, providerKeeper.GetValidatorSetUpdateId(suite.providerChain.GetContext())) +// suite.providerChain.NextBlock() +// } + +// // create validator signing info to test slashing +// providerSlashingKeeper.SetValidatorSigningInfo( +// suite.providerChain.GetContext(), +// sdk.ConsAddress(tmValidator.Address), +// slashingtypes.ValidatorSigningInfo{Address: tmValidator.Address.String()}, +// ) + +// // the test cases verify that only the unbonding tokens get slashed for the VSC ids +// // mapping to the block heights before and during the undelegation otherwise not. +// testCases := []struct { +// expSlash bool +// vscID uint64 +// }{ +// {expSlash: true, vscID: vscIDs[0]}, +// {expSlash: true, vscID: vscIDs[1]}, +// {expSlash: false, vscID: vscIDs[2]}, +// } + +// // save unbonding balance before slashing tests +// ubd, found := providerStakingKeeper.GetUnbondingDelegation( +// suite.providerChain.GetContext(), delAddr, valAddr) +// suite.Require().True(found) +// ubdBalance := ubd.Entries[0].Balance + +// for _, tc := range testCases { +// slashPacket := ccv.NewSlashPacketData( +// abci.Validator{Address: tmValidator.Address, Power: tmValidator.VotingPower}, +// tc.vscID, +// stakingtypes.Downtime, +// ) + +// // slash +// providerKeeper.HandleSlashPacket(suite.providerChain.GetContext(), suite.consumerChain.ChainID, slashPacket) + +// ubd, found := providerStakingKeeper.GetUnbondingDelegation(suite.providerChain.GetContext(), delAddr, valAddr) +// suite.Require().True(found) + +// isUbdSlashed := ubdBalance.GT(ubd.Entries[0].Balance) +// suite.Require().True(tc.expSlash == isUbdSlashed) + +// // update balance +// ubdBalance = ubd.Entries[0].Balance +// } +// } // TestValidatorDowntime tests if a slash packet is sent // and if the outstanding slashing flag is switched diff --git a/x/ccv/consumer/keeper/relay.go b/x/ccv/consumer/keeper/relay.go index 28735cd950..70ed7c7bc7 100644 --- a/x/ccv/consumer/keeper/relay.go +++ b/x/ccv/consumer/keeper/relay.go @@ -211,6 +211,11 @@ func (k Keeper) SendPackets(ctx sdk.Context) { // in conjunction with the ibc module's execution of "acknowledgePacket", // according to https://github.com/cosmos/ibc/tree/main/spec/core/ics-004-channel-and-packet-semantics#processing-acknowledgements func (k Keeper) OnAcknowledgementPacket(ctx sdk.Context, packet channeltypes.Packet, ack channeltypes.Acknowledgement) error { + + // TODO: remove all this shiz, just log an error + // working protocol should never receive an error ack + // let the consumer run still tho! + if err := ack.GetError(); err != "" { // Reasons for ErrorAcknowledgment // - packet data could not be successfully decoded diff --git a/x/ccv/provider/keeper/keeper_test.go b/x/ccv/provider/keeper/keeper_test.go index 44b40d9f07..8082e7e001 100644 --- a/x/ccv/provider/keeper/keeper_test.go +++ b/x/ccv/provider/keeper/keeper_test.go @@ -245,9 +245,7 @@ func TestHandleSlashPacketDoubleSigning(t *testing.T) { providerKeeper.SetInitChainHeight(ctx, chainId, uint64(infractionHeight)) - success, err := providerKeeper.HandleSlashPacket(ctx, chainId, slashPacket) - require.NoError(t, err) - require.True(t, success) + providerKeeper.HandleSlashPacket(ctx, chainId, slashPacket) } func TestIterateOverUnbondingOpIndex(t *testing.T) { diff --git a/x/ccv/provider/keeper/relay.go b/x/ccv/provider/keeper/relay.go index 166104be55..747064e2ef 100644 --- a/x/ccv/provider/keeper/relay.go +++ b/x/ccv/provider/keeper/relay.go @@ -34,14 +34,33 @@ func (k Keeper) OnRecvVSCMaturedPacket( packet channeltypes.Packet, data ccv.VSCMaturedPacketData, ) exported.Acknowledgement { - // check that the channel is established - chainID, found := k.GetChannelToChain(ctx, packet.DestinationChannel) - if !found { - // VSCMatured packet was sent on a channel different than any of the established CCV channels; - // this should never happen - panic(fmt.Errorf("VSCMaturedPacket received on unknown channel %s", packet.DestinationChannel)) + + if err := k.validateVSCMaturedPacket(ctx, packet, data); err != nil { + return channeltypes.NewErrorAcknowledgement(err.Error()) + } + + chainID := k.getChainIdOrPanic(ctx, packet) + k.handleVSCMaturedPacket(ctx, chainID, data) + return channeltypes.NewResultAcknowledgement([]byte{byte(1)}) +} + +// validateVSCMaturedPacket validates a recv VSCMatured packet before it is +// handled or persisted in store. An error is returned if the packet is invalid, +// and an error ack should be relayed to the sender. +func (k Keeper) validateVSCMaturedPacket(ctx sdk.Context, + packet channeltypes.Packet, data ccv.VSCMaturedPacketData) error { + + // check that a ccv channel is established via the dest channel of the recv packet + _ = k.getChainIdOrPanic(ctx, packet) + + if data.ValsetUpdateId == 0 { + return fmt.Errorf("VSCMaturedPacket's data.ValsetUpdateId cannot be 0") } + return nil +} + +func (k Keeper) handleVSCMaturedPacket(ctx sdk.Context, chainID string, data ccv.VSCMaturedPacketData) { // iterate over the unbonding operations mapped to (chainID, data.ValsetUpdateId) unbondingOps, _ := k.GetUnbondingOpsFromIndex(ctx, chainID, data.ValsetUpdateId) var maturedIds []uint64 @@ -66,9 +85,6 @@ func (k Keeper) OnRecvVSCMaturedPacket( // remove the VSC timeout timestamp for this chainID and vscID k.DeleteVscSendTimestamp(ctx, chainID, data.ValsetUpdateId) - - ack := channeltypes.NewResultAcknowledgement([]byte{byte(1)}) - return ack } // CompleteMaturedUnbondingOps attempts to complete all matured unbonding operations @@ -211,55 +227,76 @@ func (k Keeper) EndBlockCIS(ctx sdk.Context) { // OnRecvSlashPacket slashes and jails the given validator in the packet data func (k Keeper) OnRecvSlashPacket(ctx sdk.Context, packet channeltypes.Packet, data ccv.SlashPacketData) exported.Acknowledgement { - // check that the channel is established - chainID, found := k.GetChannelToChain(ctx, packet.DestinationChannel) - if !found { - // SlashPacket packet was sent on a channel different than any of the established CCV channels; - // this should never happen - panic(fmt.Errorf("SlashPacket received on unknown channel %s", packet.DestinationChannel)) + + if err := k.validateSlashPacket(ctx, packet, data); err != nil { + return channeltypes.NewErrorAcknowledgement(err.Error()) } // apply slashing - if _, err := k.HandleSlashPacket(ctx, chainID, data); err != nil { - errAck := channeltypes.NewErrorAcknowledgement(err.Error()) - return &errAck - } + chainID := k.getChainIdOrPanic(ctx, packet) + k.HandleSlashPacket(ctx, chainID, data) - ack := channeltypes.NewResultAcknowledgement([]byte{byte(1)}) - return ack + return channeltypes.NewResultAcknowledgement([]byte{byte(1)}) } -// HandleSlashPacket slash and jail a misbehaving validator according the infraction type -func (k Keeper) HandleSlashPacket(ctx sdk.Context, chainID string, data ccv.SlashPacketData) (success bool, err error) { - // map VSC ID to infraction height for the given chain ID - var infractionHeight uint64 - var found bool +// validateSlashPacket validates a recv slash packet before it is +// handled or persisted in store. An error is returned if the packet is invalid, +// and an error ack should be relayed to the sender. + +// TODO: test this ish? If so, base it off throttle PR? +func (k Keeper) validateSlashPacket(ctx sdk.Context, + packet channeltypes.Packet, data ccv.SlashPacketData) error { + + // check that a ccv channel is established via the dest channel of the recv packet + chainID := k.getChainIdOrPanic(ctx, packet) + + // make sure the validator is not yet unbonded; + // stakingKeeper.Slash() panics otherwise + // TODO: Key assignment will change the following line + val, found := k.stakingKeeper.GetValidatorByConsAddr(ctx, sdk.ConsAddress(data.Validator.Address)) + if !found || val.IsUnbonded() { + return fmt.Errorf("validator %s not found or is unbonded", data.Validator.Address) + } + if data.ValsetUpdateId == 0 { - infractionHeight, found = k.GetInitChainHeight(ctx, chainID) - } else { - infractionHeight, found = k.GetValsetUpdateBlockHeight(ctx, data.ValsetUpdateId) + return fmt.Errorf("invalid valset update id: %d", data.ValsetUpdateId) } + _, found = k.getMappedInfractionHeight(ctx, chainID, data.ValsetUpdateId) // return error if we cannot find infraction height matching the validator update id if !found { - return false, fmt.Errorf("cannot find infraction height matching the validator update id %d for chain %s", data.ValsetUpdateId, chainID) + return fmt.Errorf("cannot find infraction height matching "+ + "the validator update id %d for chain %s", data.ValsetUpdateId, chainID) } - // get the validator - consAddr := sdk.ConsAddress(data.Validator.Address) - validator, found := k.stakingKeeper.GetValidatorByConsAddr(ctx, consAddr) + if data.Infraction != stakingtypes.DoubleSign && data.Infraction != stakingtypes.Downtime { + return fmt.Errorf("invalid infraction type: %s", data.Infraction) + } - // make sure the validator is not yet unbonded; - // stakingKeeper.Slash() panics otherwise + return nil +} + +// HandleSlashPacket slash and jail a misbehaving validator according the infraction type +func (k Keeper) HandleSlashPacket(ctx sdk.Context, chainID string, data ccv.SlashPacketData) { + + // Get the validator + validator, found := k.stakingKeeper.GetValidatorByConsAddr(ctx, sdk.ConsAddress(data.Validator.Address)) if !found || validator.IsUnbonded() { - // TODO add warning log message - // fmt.Sprintf("consumer chain %s trying to slash unbonded validator %s", chainID, consAddr.String()) - return false, nil + // if validator is not found or is unbonded drop slash packet and log error + + // TODO: Confirm this will not cause a panic. + // See: https://github.com/cosmos/interchain-security/issues/541 + + k.Logger(ctx).Error("validator not found or is unbonded. This validator"+ + "was found and bonded at slash packet recv time", "validator", data.Validator.Address) + return } - // tombstoned validators should not be slashed multiple times + // tombstoned validators should not be slashed multiple times. + consAddr := sdk.ConsAddress(data.Validator.Address) if k.slashingKeeper.IsTombstoned(ctx, consAddr) { - return false, nil + // Drop packet if validator is tombstoned. + return } // slash and jail validator according to their infraction type @@ -282,11 +319,13 @@ func (k Keeper) HandleSlashPacket(ctx sdk.Context, chainID string, data ccv.Slas slashFraction = k.slashingKeeper.SlashFractionDoubleSign(ctx) jailTime = evidencetypes.DoubleSignJailEndTime k.slashingKeeper.Tombstone(ctx, consAddr) - default: - return false, fmt.Errorf("invalid infraction type: %v", data.Infraction) } // slash validator + infractionHeight, found := k.getMappedInfractionHeight(ctx, chainID, data.ValsetUpdateId) + if !found { + panic("infraction height not found. But was found during slash packet validation") + } k.stakingKeeper.Slash( ctx, consAddr, @@ -312,11 +351,9 @@ func (k Keeper) HandleSlashPacket(ctx sdk.Context, chainID string, data ccv.Slas sdk.NewAttribute(ccv.AttributeValSetUpdateID, strconv.Itoa(int(data.ValsetUpdateId))), ), ) - - return true, nil } -// EndBlockCIS contains the EndBlock logic needed for +// EndBlockCCR contains the EndBlock logic needed for // the Consumer Chain Removal sub-protocol func (k Keeper) EndBlockCCR(ctx sdk.Context) { currentTime := ctx.BlockTime() @@ -381,3 +418,26 @@ func (k Keeper) EndBlockCCR(ctx sdk.Context) { } } } + +// getMappedInfractionHeight gets the infraction height mapped from val set ID for the given chain ID +func (k Keeper) getMappedInfractionHeight(ctx sdk.Context, + chainID string, valsetUpdateID uint64) (height uint64, found bool) { + + if valsetUpdateID == 0 { + return k.GetInitChainHeight(ctx, chainID) + } else { + return k.GetValsetUpdateBlockHeight(ctx, valsetUpdateID) + } +} + +// getChainIdOrPanic returns the chainID from a recv packet, +// or panics if the packet was sent on a different channel than any of the established CCV channels. +func (k Keeper) getChainIdOrPanic(ctx sdk.Context, packet channeltypes.Packet) string { + chainID, found := k.GetChannelToChain(ctx, packet.DestinationChannel) + if !found { + // Packet was sent on a channel different than any of the established CCV channels; + // this should never happen + panic(fmt.Sprintf("channel %s not found", packet.DestinationChannel)) + } + return chainID +} From 59d293bb18b9eb6a9e451bd88945821fcfbfc3b2 Mon Sep 17 00:00:00 2001 From: Shawn Marshall-Spitzbart <44221603+smarshall-spitzbart@users.noreply.github.com> Date: Thu, 1 Dec 2022 17:45:11 -0800 Subject: [PATCH 05/31] bugfix --- x/ccv/provider/keeper/relay.go | 4 ---- 1 file changed, 4 deletions(-) diff --git a/x/ccv/provider/keeper/relay.go b/x/ccv/provider/keeper/relay.go index 747064e2ef..c61e4ed714 100644 --- a/x/ccv/provider/keeper/relay.go +++ b/x/ccv/provider/keeper/relay.go @@ -258,10 +258,6 @@ func (k Keeper) validateSlashPacket(ctx sdk.Context, return fmt.Errorf("validator %s not found or is unbonded", data.Validator.Address) } - if data.ValsetUpdateId == 0 { - return fmt.Errorf("invalid valset update id: %d", data.ValsetUpdateId) - } - _, found = k.getMappedInfractionHeight(ctx, chainID, data.ValsetUpdateId) // return error if we cannot find infraction height matching the validator update id if !found { From 948c6bf2ea8f1a6d34d19c1b448ce921a2a48b3b Mon Sep 17 00:00:00 2001 From: Shawn Marshall-Spitzbart <44221603+smarshall-spitzbart@users.noreply.github.com> Date: Thu, 1 Dec 2022 18:24:10 -0800 Subject: [PATCH 06/31] small --- x/ccv/provider/keeper/relay.go | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/x/ccv/provider/keeper/relay.go b/x/ccv/provider/keeper/relay.go index c61e4ed714..550b8bd2fc 100644 --- a/x/ccv/provider/keeper/relay.go +++ b/x/ccv/provider/keeper/relay.go @@ -53,9 +53,10 @@ func (k Keeper) validateVSCMaturedPacket(ctx sdk.Context, // check that a ccv channel is established via the dest channel of the recv packet _ = k.getChainIdOrPanic(ctx, packet) - if data.ValsetUpdateId == 0 { - return fmt.Errorf("VSCMaturedPacket's data.ValsetUpdateId cannot be 0") - } + // TODO: needed? + // if data.ValsetUpdateId == 0 { + // return fmt.Errorf("VSCMaturedPacket's data.ValsetUpdateId cannot be 0") + // } return nil } @@ -242,8 +243,6 @@ func (k Keeper) OnRecvSlashPacket(ctx sdk.Context, packet channeltypes.Packet, d // validateSlashPacket validates a recv slash packet before it is // handled or persisted in store. An error is returned if the packet is invalid, // and an error ack should be relayed to the sender. - -// TODO: test this ish? If so, base it off throttle PR? func (k Keeper) validateSlashPacket(ctx sdk.Context, packet channeltypes.Packet, data ccv.SlashPacketData) error { @@ -255,7 +254,7 @@ func (k Keeper) validateSlashPacket(ctx sdk.Context, // TODO: Key assignment will change the following line val, found := k.stakingKeeper.GetValidatorByConsAddr(ctx, sdk.ConsAddress(data.Validator.Address)) if !found || val.IsUnbonded() { - return fmt.Errorf("validator %s not found or is unbonded", data.Validator.Address) + // TODO: return error here. See: ___ (new issue, this PR is just refactoring) } _, found = k.getMappedInfractionHeight(ctx, chainID, data.ValsetUpdateId) @@ -276,20 +275,21 @@ func (k Keeper) validateSlashPacket(ctx sdk.Context, func (k Keeper) HandleSlashPacket(ctx sdk.Context, chainID string, data ccv.SlashPacketData) { // Get the validator - validator, found := k.stakingKeeper.GetValidatorByConsAddr(ctx, sdk.ConsAddress(data.Validator.Address)) + consAddr := sdk.ConsAddress(data.Validator.Address) + // TODO: Key assignment will change the following line + validator, found := k.stakingKeeper.GetValidatorByConsAddr(ctx, consAddr) if !found || validator.IsUnbonded() { // if validator is not found or is unbonded drop slash packet and log error // TODO: Confirm this will not cause a panic. // See: https://github.com/cosmos/interchain-security/issues/541 - k.Logger(ctx).Error("validator not found or is unbonded. This validator"+ - "was found and bonded at slash packet recv time", "validator", data.Validator.Address) + k.Logger(ctx).Error("validator not found or is unbonded. However, this validator"+ + " was found and bonded at slash packet recv time", "validator", data.Validator.Address) return } // tombstoned validators should not be slashed multiple times. - consAddr := sdk.ConsAddress(data.Validator.Address) if k.slashingKeeper.IsTombstoned(ctx, consAddr) { // Drop packet if validator is tombstoned. return From 329b4837b2e74791af7066543cdfe12f15bdf831 Mon Sep 17 00:00:00 2001 From: Shawn Marshall-Spitzbart <44221603+smarshall-spitzbart@users.noreply.github.com> Date: Thu, 1 Dec 2022 19:36:38 -0800 Subject: [PATCH 07/31] mas --- tests/e2e/slashing.go | 415 ++++++++++++++++++--------------- testutil/e2e/debug_test.go | 4 +- x/ccv/provider/keeper/relay.go | 18 +- 3 files changed, 237 insertions(+), 200 deletions(-) diff --git a/tests/e2e/slashing.go b/tests/e2e/slashing.go index a1f7aaadac..abf9d45564 100644 --- a/tests/e2e/slashing.go +++ b/tests/e2e/slashing.go @@ -1,6 +1,7 @@ package e2e import ( + "fmt" "time" cryptocodec "github.com/cosmos/cosmos-sdk/crypto/codec" @@ -253,192 +254,234 @@ func (suite *CCVTestSuite) TestHandleSlashPacketDoubleSigning() { suite.Require().True(signingInfo.Tombstoned) } -// TestHandleSlashPacketErrors tests errors for the HandleSlashPacket method in an e2e testing setting -// TODO: will need to update this ish -// TODO: also change capatalization of provider keeper -// TODO: also more tests? -// func (suite *CCVTestSuite) TestHandleSlashPacketErrors() { -// providerStakingKeeper := suite.providerApp.GetE2eStakingKeeper() -// ProviderKeeper := suite.providerApp.GetProviderKeeper() -// providerSlashingKeeper := suite.providerApp.GetE2eSlashingKeeper() -// consumerChainID := suite.consumerChain.ChainID - -// // sync contexts block height -// ctx := suite.providerCtx() - -// // expect an error if initial block height isn't set for consumer chain -// _, err := ProviderKeeper.HandleSlashPacket(ctx, consumerChainID, ccv.SlashPacketData{}) -// suite.Require().Error(err, "slash validator with invalid infraction height") - -// // save VSC ID -// vID := ProviderKeeper.GetValidatorSetUpdateId(ctx) - -// // remove block height for current VSC ID -// ProviderKeeper.DeleteValsetUpdateBlockHeight(ctx, vID) - -// // expect an error if block height mapping VSC ID is zero -// _, err = ProviderKeeper.HandleSlashPacket(ctx, consumerChainID, ccv.SlashPacketData{ValsetUpdateId: vID}) -// suite.Require().Error(err, "slash with height mapping to zero") - -// // construct slashing packet with non existing validator -// slashingPkt := ccv.NewSlashPacketData( -// abci.Validator{Address: ed25519.GenPrivKey().PubKey().Address(), -// Power: int64(0)}, uint64(0), stakingtypes.Downtime, -// ) - -// // Set initial block height for consumer chain -// ProviderKeeper.SetInitChainHeight(ctx, consumerChainID, uint64(ctx.BlockHeight())) - -// // expect the slash to not succeed if validator doesn't exist -// success, err := ProviderKeeper.HandleSlashPacket(ctx, consumerChainID, slashingPkt) -// suite.Require().NoError(err, "slashing an unknown validator should not result in error") -// suite.Require().False(success, "did slash unknown validator") - -// // jail an existing validator -// val := suite.providerChain.Vals.Validators[0] -// consAddr := sdk.ConsAddress(val.Address) -// providerStakingKeeper.Jail(ctx, consAddr) -// // commit block to set VSC ID -// suite.coordinator.CommitBlock(suite.providerChain) -// // Update suite.ctx bc CommitBlock updates only providerChain's current header block height -// ctx = suite.providerChain.GetContext() -// suite.Require().NotZero(ProviderKeeper.GetValsetUpdateBlockHeight(ctx, vID)) - -// // create validator signing info -// valInfo := slashingtypes.NewValidatorSigningInfo(sdk.ConsAddress(val.Address), ctx.BlockHeight(), -// ctx.BlockHeight()-1, time.Time{}.UTC(), false, int64(0)) -// providerSlashingKeeper.SetValidatorSigningInfo(ctx, sdk.ConsAddress(val.Address), valInfo) - -// // update validator address and VSC ID -// slashingPkt.Validator.Address = val.Address -// slashingPkt.ValsetUpdateId = vID - -// // expect to slash and jail validator -// _, err = ProviderKeeper.HandleSlashPacket(ctx, consumerChainID, slashingPkt) -// suite.Require().NoError(err, "did slash jail validator") - -// // expect error when infraction type in unspecified -// tmAddr := suite.providerChain.Vals.Validators[1].Address -// slashingPkt.Validator.Address = tmAddr -// slashingPkt.Infraction = stakingtypes.InfractionEmpty - -// valInfo.Address = sdk.ConsAddress(tmAddr).String() -// providerSlashingKeeper.SetValidatorSigningInfo(ctx, sdk.ConsAddress(tmAddr), valInfo) - -// _, err = ProviderKeeper.HandleSlashPacket(ctx, consumerChainID, slashingPkt) -// suite.Require().EqualError(err, fmt.Sprintf("invalid infraction type: %v", stakingtypes.InfractionEmpty)) - -// // expect to slash jail validator -// slashingPkt.Infraction = stakingtypes.DoubleSign -// _, err = ProviderKeeper.HandleSlashPacket(ctx, consumerChainID, slashingPkt) -// suite.Require().NoError(err) - -// // expect the slash to not succeed when validator is tombstoned -// success, _ = ProviderKeeper.HandleSlashPacket(ctx, consumerChainID, slashingPkt) -// suite.Require().False(success) -// } - -// // TestHandleSlashPacketDistribution tests the slashing of an undelegation balance -// // by varying the slash packet VSC ID mapping to infraction heights -// // lesser, equal or greater than the undelegation entry creation height -// func (suite *CCVTestSuite) TestHandleSlashPacketDistribution() { -// providerKeeper := suite.providerApp.GetProviderKeeper() -// providerStakingKeeper := suite.providerApp.GetE2eStakingKeeper() -// providerSlashingKeeper := suite.providerApp.GetE2eSlashingKeeper() - -// // choose a validator -// tmValidator := suite.providerChain.Vals.Validators[0] -// valAddr, err := sdk.ValAddressFromHex(tmValidator.Address.String()) -// suite.Require().NoError(err) - -// validator, found := providerStakingKeeper.GetValidator(suite.providerChain.GetContext(), valAddr) -// suite.Require().True(found) - -// // unbonding operations parameters -// delAddr := suite.providerChain.SenderAccount.GetAddress() -// bondAmt := sdk.NewInt(1000000) - -// // new delegator shares used -// testShares := sdk.Dec{} - -// // setup the test with a delegation, a no-op and an undelegation -// setupOperations := []struct { -// fn func(suite *CCVTestSuite) error -// }{ -// { -// func(suite *CCVTestSuite) error { -// testShares, err = providerStakingKeeper.Delegate(suite.providerChain.GetContext(), delAddr, bondAmt, stakingtypes.Unbonded, stakingtypes.Validator(validator), true) -// return err -// }, -// }, { -// func(suite *CCVTestSuite) error { -// return nil -// }, -// }, { -// // undelegate a quarter of the new shares created -// func(suite *CCVTestSuite) error { -// _, err = providerStakingKeeper.Undelegate(suite.providerChain.GetContext(), delAddr, valAddr, testShares.QuoInt64(4)) -// return err -// }, -// }, -// } - -// // execute the setup operations, distributed uniformly in three blocks. -// // For each of them, save their current VSC Id value which map correspond respectively -// // to the block heights lesser, equal and greater than the undelegation creation height. -// vscIDs := make([]uint64, 0, 3) -// for _, so := range setupOperations { -// err := so.fn(suite) -// suite.Require().NoError(err) - -// vscIDs = append(vscIDs, providerKeeper.GetValidatorSetUpdateId(suite.providerChain.GetContext())) -// suite.providerChain.NextBlock() -// } - -// // create validator signing info to test slashing -// providerSlashingKeeper.SetValidatorSigningInfo( -// suite.providerChain.GetContext(), -// sdk.ConsAddress(tmValidator.Address), -// slashingtypes.ValidatorSigningInfo{Address: tmValidator.Address.String()}, -// ) - -// // the test cases verify that only the unbonding tokens get slashed for the VSC ids -// // mapping to the block heights before and during the undelegation otherwise not. -// testCases := []struct { -// expSlash bool -// vscID uint64 -// }{ -// {expSlash: true, vscID: vscIDs[0]}, -// {expSlash: true, vscID: vscIDs[1]}, -// {expSlash: false, vscID: vscIDs[2]}, -// } - -// // save unbonding balance before slashing tests -// ubd, found := providerStakingKeeper.GetUnbondingDelegation( -// suite.providerChain.GetContext(), delAddr, valAddr) -// suite.Require().True(found) -// ubdBalance := ubd.Entries[0].Balance - -// for _, tc := range testCases { -// slashPacket := ccv.NewSlashPacketData( -// abci.Validator{Address: tmValidator.Address, Power: tmValidator.VotingPower}, -// tc.vscID, -// stakingtypes.Downtime, -// ) - -// // slash -// providerKeeper.HandleSlashPacket(suite.providerChain.GetContext(), suite.consumerChain.ChainID, slashPacket) - -// ubd, found := providerStakingKeeper.GetUnbondingDelegation(suite.providerChain.GetContext(), delAddr, valAddr) -// suite.Require().True(found) - -// isUbdSlashed := ubdBalance.GT(ubd.Entries[0].Balance) -// suite.Require().True(tc.expSlash == isUbdSlashed) - -// // update balance -// ubdBalance = ubd.Entries[0].Balance -// } -// } +// TestOnRecvSlashPacketErrors tests errors for the OnRecvSlashPacket method in an e2e testing setting + +// TODO: likely just replace this with unit tests for the validation and handler! +func (suite *CCVTestSuite) TestOnRecvSlashPacketErrors() { + + providerStakingKeeper := suite.providerApp.GetE2eStakingKeeper() + providerKeeper := suite.providerApp.GetProviderKeeper() + providerSlashingKeeper := suite.providerApp.GetE2eSlashingKeeper() + firstBundle := suite.getFirstBundle() + consumerChainID := firstBundle.Chain.ChainID + + suite.SetupAllCCVChannels() + + // sync contexts block height + ctx := suite.providerCtx() + + // Expect panic if ccv channel is not established via dest channel of packet + suite.Panics(func() { + providerKeeper.OnRecvSlashPacket(ctx, channeltypes.Packet{}, ccv.SlashPacketData{}) + }) + + // Add correct channelID to packet. Now we will not panic anymore. + packet := channeltypes.Packet{DestinationChannel: firstBundle.Path.EndpointB.ChannelID} + + // Init chain height is set by established CCV channel + // Delete init chain height and confirm expected error + initChainHeight, found := providerKeeper.GetInitChainHeight(ctx, consumerChainID) + suite.Require().True(found) + providerKeeper.DeleteInitChainHeight(ctx, consumerChainID) + + packetData := ccv.SlashPacketData{ValsetUpdateId: 0} + errAck := providerKeeper.OnRecvSlashPacket(ctx, packet, packetData) + suite.Require().False(errAck.Success()) + errAckCast := errAck.(channeltypes.Acknowledgement) + suite.Require().Equal( + fmt.Sprintf("cannot find infraction height matching the validator update id 0 for chain %s", + firstBundle.Chain.ChainID), errAckCast.GetError()) + + // Restore init chain height + providerKeeper.SetInitChainHeight(ctx, consumerChainID, initChainHeight) + + // now the method will fail at infraction height check. + packetData.Infraction = stakingtypes.InfractionEmpty + errAck = providerKeeper.OnRecvSlashPacket(ctx, packet, packetData) + suite.Require().False(errAck.Success()) + errAckCast = errAck.(channeltypes.Acknowledgement) + suite.Require().Equal( + fmt.Sprintf("invalid infraction type: %s", packetData.Infraction.String()), errAckCast.GetError()) + + // save current VSC ID + vscID := providerKeeper.GetValidatorSetUpdateId(ctx) + + // remove block height value mapped to current VSC ID + providerKeeper.DeleteValsetUpdateBlockHeight(ctx, vscID) + + // Instantiate packet data with current VSC ID + packetData = ccv.SlashPacketData{ValsetUpdateId: vscID} + + // expect an error if mapped block height is not found + errAck = providerKeeper.OnRecvSlashPacket(ctx, packet, packetData) + suite.Require().False(errAck.Success()) + errAckCast = errAck.(channeltypes.Acknowledgement) + suite.Require().Equal( + fmt.Sprintf("cannot find infraction height matching the validator update id %d for chain %s", + vscID, firstBundle.Chain.ChainID), errAckCast.GetError()) + + // construct slashing packet with non existing validator + slashingPkt := ccv.NewSlashPacketData( + abci.Validator{Address: ed25519.GenPrivKey().PubKey().Address(), + Power: int64(0)}, uint64(0), stakingtypes.Downtime, + ) + + // Set initial block height for consumer chain + providerKeeper.SetInitChainHeight(ctx, consumerChainID, uint64(ctx.BlockHeight())) + + // Expect no error ack if validator does not exist + // TODO: this behavior should be changed to return an error + ack := providerKeeper.OnRecvSlashPacket(ctx, packet, slashingPkt) + suite.Require().True(ack.Success()) + + // jail an existing validator + val := suite.providerChain.Vals.Validators[0] + consAddr := sdk.ConsAddress(val.Address) + providerStakingKeeper.Jail(ctx, consAddr) + // commit block to set VSC ID + suite.coordinator.CommitBlock(suite.providerChain) + // Update suite.ctx bc CommitBlock updates only providerChain's current header block height + ctx = suite.providerChain.GetContext() + suite.Require().NotZero(providerKeeper.GetValsetUpdateBlockHeight(ctx, vscID)) + + // create validator signing info + valInfo := slashingtypes.NewValidatorSigningInfo(sdk.ConsAddress(val.Address), ctx.BlockHeight(), + ctx.BlockHeight()-1, time.Time{}.UTC(), false, int64(0)) + providerSlashingKeeper.SetValidatorSigningInfo(ctx, sdk.ConsAddress(val.Address), valInfo) + + // update validator address and VSC ID + slashingPkt.Validator.Address = val.Address + slashingPkt.ValsetUpdateId = vscID + + // expect to slash and jail validator + ack = providerKeeper.OnRecvSlashPacket(ctx, packet, slashingPkt) + suite.Require().True(ack.Success()) + suite.Require().True(providerStakingKeeper.IsValidatorJailed(ctx, consAddr)) + + // expect error ack when infraction type in unspecified + tmAddr := suite.providerChain.Vals.Validators[1].Address + slashingPkt.Validator.Address = tmAddr + slashingPkt.Infraction = stakingtypes.InfractionEmpty + + valInfo.Address = sdk.ConsAddress(tmAddr).String() + providerSlashingKeeper.SetValidatorSigningInfo(ctx, sdk.ConsAddress(tmAddr), valInfo) + + errAck = providerKeeper.OnRecvSlashPacket(ctx, packet, slashingPkt) + suite.Require().False(errAck.Success()) + + // expect to slash and jail validator + slashingPkt.Infraction = stakingtypes.DoubleSign + ack = providerKeeper.OnRecvSlashPacket(ctx, packet, slashingPkt) + suite.Require().True(ack.Success()) + suite.Require().True(providerStakingKeeper.IsValidatorJailed(ctx, sdk.ConsAddress(tmAddr))) + + // expect the slash to not succeed when validator is tombstoned + ack = providerKeeper.OnRecvSlashPacket(ctx, packet, slashingPkt) + suite.Require().True(ack.Success()) + // TODO: prove slash didn't happen +} + +// TestHandleSlashPacketDistribution tests the slashing of an undelegation balance +// by varying the slash packet VSC ID mapping to infraction heights +// lesser, equal or greater than the undelegation entry creation height +func (suite *CCVTestSuite) TestHandleSlashPacketDistribution() { + providerKeeper := suite.providerApp.GetProviderKeeper() + providerStakingKeeper := suite.providerApp.GetE2eStakingKeeper() + providerSlashingKeeper := suite.providerApp.GetE2eSlashingKeeper() + + // choose a validator + tmValidator := suite.providerChain.Vals.Validators[0] + valAddr, err := sdk.ValAddressFromHex(tmValidator.Address.String()) + suite.Require().NoError(err) + + validator, found := providerStakingKeeper.GetValidator(suite.providerChain.GetContext(), valAddr) + suite.Require().True(found) + + // unbonding operations parameters + delAddr := suite.providerChain.SenderAccount.GetAddress() + bondAmt := sdk.NewInt(1000000) + + // new delegator shares used + testShares := sdk.Dec{} + + // setup the test with a delegation, a no-op and an undelegation + setupOperations := []struct { + fn func(suite *CCVTestSuite) error + }{ + { + func(suite *CCVTestSuite) error { + testShares, err = providerStakingKeeper.Delegate(suite.providerChain.GetContext(), delAddr, bondAmt, stakingtypes.Unbonded, stakingtypes.Validator(validator), true) + return err + }, + }, { + func(suite *CCVTestSuite) error { + return nil + }, + }, { + // undelegate a quarter of the new shares created + func(suite *CCVTestSuite) error { + _, err = providerStakingKeeper.Undelegate(suite.providerChain.GetContext(), delAddr, valAddr, testShares.QuoInt64(4)) + return err + }, + }, + } + + // execute the setup operations, distributed uniformly in three blocks. + // For each of them, save their current VSC Id value which map correspond respectively + // to the block heights lesser, equal and greater than the undelegation creation height. + vscIDs := make([]uint64, 0, 3) + for _, so := range setupOperations { + err := so.fn(suite) + suite.Require().NoError(err) + + vscIDs = append(vscIDs, providerKeeper.GetValidatorSetUpdateId(suite.providerChain.GetContext())) + suite.providerChain.NextBlock() + } + + // create validator signing info to test slashing + providerSlashingKeeper.SetValidatorSigningInfo( + suite.providerChain.GetContext(), + sdk.ConsAddress(tmValidator.Address), + slashingtypes.ValidatorSigningInfo{Address: tmValidator.Address.String()}, + ) + + // the test cases verify that only the unbonding tokens get slashed for the VSC ids + // mapping to the block heights before and during the undelegation otherwise not. + testCases := []struct { + expSlash bool + vscID uint64 + }{ + {expSlash: true, vscID: vscIDs[0]}, + {expSlash: true, vscID: vscIDs[1]}, + {expSlash: false, vscID: vscIDs[2]}, + } + + // save unbonding balance before slashing tests + ubd, found := providerStakingKeeper.GetUnbondingDelegation( + suite.providerChain.GetContext(), delAddr, valAddr) + suite.Require().True(found) + ubdBalance := ubd.Entries[0].Balance + + for _, tc := range testCases { + slashPacket := ccv.NewSlashPacketData( + abci.Validator{Address: tmValidator.Address, Power: tmValidator.VotingPower}, + tc.vscID, + stakingtypes.Downtime, + ) + + // slash + providerKeeper.HandleSlashPacket(suite.providerChain.GetContext(), suite.consumerChain.ChainID, slashPacket) + + ubd, found := providerStakingKeeper.GetUnbondingDelegation(suite.providerChain.GetContext(), delAddr, valAddr) + suite.Require().True(found) + + isUbdSlashed := ubdBalance.GT(ubd.Entries[0].Balance) + suite.Require().True(tc.expSlash == isUbdSlashed) + + // update balance + ubdBalance = ubd.Entries[0].Balance + } +} // TestValidatorDowntime tests if a slash packet is sent // and if the outstanding slashing flag is switched diff --git a/testutil/e2e/debug_test.go b/testutil/e2e/debug_test.go index b779082972..945a5d076b 100644 --- a/testutil/e2e/debug_test.go +++ b/testutil/e2e/debug_test.go @@ -116,8 +116,8 @@ func TestHandleSlashPacketDoubleSigning(t *testing.T) { runCCVTestByName(t, "TestHandleSlashPacketDoubleSigning") } -func TestHandleSlashPacketErrors(t *testing.T) { - runCCVTestByName(t, "TestHandleSlashPacketErrors") +func TestOnRecvSlashPacketErrors(t *testing.T) { + runCCVTestByName(t, "TestOnRecvSlashPacketErrors") } func TestHandleSlashPacketDistribution(t *testing.T) { diff --git a/x/ccv/provider/keeper/relay.go b/x/ccv/provider/keeper/relay.go index 550b8bd2fc..fe62d1b404 100644 --- a/x/ccv/provider/keeper/relay.go +++ b/x/ccv/provider/keeper/relay.go @@ -249,15 +249,10 @@ func (k Keeper) validateSlashPacket(ctx sdk.Context, // check that a ccv channel is established via the dest channel of the recv packet chainID := k.getChainIdOrPanic(ctx, packet) - // make sure the validator is not yet unbonded; - // stakingKeeper.Slash() panics otherwise - // TODO: Key assignment will change the following line - val, found := k.stakingKeeper.GetValidatorByConsAddr(ctx, sdk.ConsAddress(data.Validator.Address)) - if !found || val.IsUnbonded() { - // TODO: return error here. See: ___ (new issue, this PR is just refactoring) - } + // TODO: callout in PR that check needs to be added here, to return an ibc erro ack when val not found. + // TODO: This will need it's own issue, so this PR can stay as refactoring - _, found = k.getMappedInfractionHeight(ctx, chainID, data.ValsetUpdateId) + _, found := k.getMappedInfractionHeight(ctx, chainID, data.ValsetUpdateId) // return error if we cannot find infraction height matching the validator update id if !found { return fmt.Errorf("cannot find infraction height matching "+ @@ -278,12 +273,11 @@ func (k Keeper) HandleSlashPacket(ctx sdk.Context, chainID string, data ccv.Slas consAddr := sdk.ConsAddress(data.Validator.Address) // TODO: Key assignment will change the following line validator, found := k.stakingKeeper.GetValidatorByConsAddr(ctx, consAddr) + + // make sure the validator is not yet unbonded; + // stakingKeeper.Slash() panics otherwise if !found || validator.IsUnbonded() { // if validator is not found or is unbonded drop slash packet and log error - - // TODO: Confirm this will not cause a panic. - // See: https://github.com/cosmos/interchain-security/issues/541 - k.Logger(ctx).Error("validator not found or is unbonded. However, this validator"+ " was found and bonded at slash packet recv time", "validator", data.Validator.Address) return From 4de3898236838836cd1e9ed0b993a34596fe3e70 Mon Sep 17 00:00:00 2001 From: Shawn Marshall-Spitzbart <44221603+smarshall-spitzbart@users.noreply.github.com> Date: Thu, 1 Dec 2022 20:08:55 -0800 Subject: [PATCH 08/31] Update slashing.go --- tests/e2e/slashing.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/tests/e2e/slashing.go b/tests/e2e/slashing.go index abf9d45564..00dbbf0521 100644 --- a/tests/e2e/slashing.go +++ b/tests/e2e/slashing.go @@ -255,8 +255,6 @@ func (suite *CCVTestSuite) TestHandleSlashPacketDoubleSigning() { } // TestOnRecvSlashPacketErrors tests errors for the OnRecvSlashPacket method in an e2e testing setting - -// TODO: likely just replace this with unit tests for the validation and handler! func (suite *CCVTestSuite) TestOnRecvSlashPacketErrors() { providerStakingKeeper := suite.providerApp.GetE2eStakingKeeper() From ac724452fd5cd9da31a56be19430e65208c7b6d6 Mon Sep 17 00:00:00 2001 From: Shawn Marshall-Spitzbart <44221603+smarshall-spitzbart@users.noreply.github.com> Date: Fri, 2 Dec 2022 08:49:10 -0800 Subject: [PATCH 09/31] cleans --- tests/e2e/slashing.go | 8 ++------ x/ccv/provider/keeper/relay.go | 4 ++-- 2 files changed, 4 insertions(+), 8 deletions(-) diff --git a/tests/e2e/slashing.go b/tests/e2e/slashing.go index 00dbbf0521..79daa8f9ea 100644 --- a/tests/e2e/slashing.go +++ b/tests/e2e/slashing.go @@ -328,7 +328,8 @@ func (suite *CCVTestSuite) TestOnRecvSlashPacketErrors() { providerKeeper.SetInitChainHeight(ctx, consumerChainID, uint64(ctx.BlockHeight())) // Expect no error ack if validator does not exist - // TODO: this behavior should be changed to return an error + // TODO: this behavior should be changed to return an error ack, + // see: https://github.com/cosmos/interchain-security/issues/546 ack := providerKeeper.OnRecvSlashPacket(ctx, packet, slashingPkt) suite.Require().True(ack.Success()) @@ -372,11 +373,6 @@ func (suite *CCVTestSuite) TestOnRecvSlashPacketErrors() { ack = providerKeeper.OnRecvSlashPacket(ctx, packet, slashingPkt) suite.Require().True(ack.Success()) suite.Require().True(providerStakingKeeper.IsValidatorJailed(ctx, sdk.ConsAddress(tmAddr))) - - // expect the slash to not succeed when validator is tombstoned - ack = providerKeeper.OnRecvSlashPacket(ctx, packet, slashingPkt) - suite.Require().True(ack.Success()) - // TODO: prove slash didn't happen } // TestHandleSlashPacketDistribution tests the slashing of an undelegation balance diff --git a/x/ccv/provider/keeper/relay.go b/x/ccv/provider/keeper/relay.go index fe62d1b404..fc83a14a6e 100644 --- a/x/ccv/provider/keeper/relay.go +++ b/x/ccv/provider/keeper/relay.go @@ -250,7 +250,6 @@ func (k Keeper) validateSlashPacket(ctx sdk.Context, chainID := k.getChainIdOrPanic(ctx, packet) // TODO: callout in PR that check needs to be added here, to return an ibc erro ack when val not found. - // TODO: This will need it's own issue, so this PR can stay as refactoring _, found := k.getMappedInfractionHeight(ctx, chainID, data.ValsetUpdateId) // return error if we cannot find infraction height matching the validator update id @@ -266,7 +265,8 @@ func (k Keeper) validateSlashPacket(ctx sdk.Context, return nil } -// HandleSlashPacket slash and jail a misbehaving validator according the infraction type +// HandleSlashPacket potentially slashes, jails and/or tombstones +// a misbehaving validator according to infraction type. func (k Keeper) HandleSlashPacket(ctx sdk.Context, chainID string, data ccv.SlashPacketData) { // Get the validator From d4b77492ca46cbf15ed7c5c2a81623ae28947f09 Mon Sep 17 00:00:00 2001 From: Shawn Marshall-Spitzbart <44221603+smarshall-spitzbart@users.noreply.github.com> Date: Fri, 2 Dec 2022 08:55:07 -0800 Subject: [PATCH 10/31] cleans --- x/ccv/provider/keeper/relay.go | 7 ------- 1 file changed, 7 deletions(-) diff --git a/x/ccv/provider/keeper/relay.go b/x/ccv/provider/keeper/relay.go index fc83a14a6e..a70d98bd54 100644 --- a/x/ccv/provider/keeper/relay.go +++ b/x/ccv/provider/keeper/relay.go @@ -53,11 +53,6 @@ func (k Keeper) validateVSCMaturedPacket(ctx sdk.Context, // check that a ccv channel is established via the dest channel of the recv packet _ = k.getChainIdOrPanic(ctx, packet) - // TODO: needed? - // if data.ValsetUpdateId == 0 { - // return fmt.Errorf("VSCMaturedPacket's data.ValsetUpdateId cannot be 0") - // } - return nil } @@ -249,8 +244,6 @@ func (k Keeper) validateSlashPacket(ctx sdk.Context, // check that a ccv channel is established via the dest channel of the recv packet chainID := k.getChainIdOrPanic(ctx, packet) - // TODO: callout in PR that check needs to be added here, to return an ibc erro ack when val not found. - _, found := k.getMappedInfractionHeight(ctx, chainID, data.ValsetUpdateId) // return error if we cannot find infraction height matching the validator update id if !found { From 2766fff300cf4cd22b2897dc8d2dcbe4ba27be42 Mon Sep 17 00:00:00 2001 From: Shawn Marshall-Spitzbart <44221603+smarshall-spitzbart@users.noreply.github.com> Date: Fri, 2 Dec 2022 09:12:02 -0800 Subject: [PATCH 11/31] Update relay.go --- x/ccv/consumer/keeper/relay.go | 4 ---- 1 file changed, 4 deletions(-) diff --git a/x/ccv/consumer/keeper/relay.go b/x/ccv/consumer/keeper/relay.go index 70ed7c7bc7..22b94437b2 100644 --- a/x/ccv/consumer/keeper/relay.go +++ b/x/ccv/consumer/keeper/relay.go @@ -212,10 +212,6 @@ func (k Keeper) SendPackets(ctx sdk.Context) { // according to https://github.com/cosmos/ibc/tree/main/spec/core/ics-004-channel-and-packet-semantics#processing-acknowledgements func (k Keeper) OnAcknowledgementPacket(ctx sdk.Context, packet channeltypes.Packet, ack channeltypes.Acknowledgement) error { - // TODO: remove all this shiz, just log an error - // working protocol should never receive an error ack - // let the consumer run still tho! - if err := ack.GetError(); err != "" { // Reasons for ErrorAcknowledgment // - packet data could not be successfully decoded From ef27009eae410082d3e7e168732e92dcf33d04fb Mon Sep 17 00:00:00 2001 From: Shawn Marshall-Spitzbart <44221603+smarshall-spitzbart@users.noreply.github.com> Date: Fri, 2 Dec 2022 09:47:53 -0800 Subject: [PATCH 12/31] Update relay.go --- x/ccv/consumer/keeper/relay.go | 1 - 1 file changed, 1 deletion(-) diff --git a/x/ccv/consumer/keeper/relay.go b/x/ccv/consumer/keeper/relay.go index 22b94437b2..28735cd950 100644 --- a/x/ccv/consumer/keeper/relay.go +++ b/x/ccv/consumer/keeper/relay.go @@ -211,7 +211,6 @@ func (k Keeper) SendPackets(ctx sdk.Context) { // in conjunction with the ibc module's execution of "acknowledgePacket", // according to https://github.com/cosmos/ibc/tree/main/spec/core/ics-004-channel-and-packet-semantics#processing-acknowledgements func (k Keeper) OnAcknowledgementPacket(ctx sdk.Context, packet channeltypes.Packet, ack channeltypes.Acknowledgement) error { - if err := ack.GetError(); err != "" { // Reasons for ErrorAcknowledgment // - packet data could not be successfully decoded From c058ce9688e676f0ceb45bab39911c828688659d Mon Sep 17 00:00:00 2001 From: Shawn Marshall-Spitzbart <44221603+smarshall-spitzbart@users.noreply.github.com> Date: Fri, 2 Dec 2022 10:27:38 -0800 Subject: [PATCH 13/31] UT --- x/ccv/provider/keeper/relay.go | 11 ++++++----- x/ccv/provider/keeper/relay_test.go | 23 +++++++++++++++++++++++ 2 files changed, 29 insertions(+), 5 deletions(-) diff --git a/x/ccv/provider/keeper/relay.go b/x/ccv/provider/keeper/relay.go index a70d98bd54..2c6d9f3df7 100644 --- a/x/ccv/provider/keeper/relay.go +++ b/x/ccv/provider/keeper/relay.go @@ -35,19 +35,19 @@ func (k Keeper) OnRecvVSCMaturedPacket( data ccv.VSCMaturedPacketData, ) exported.Acknowledgement { - if err := k.validateVSCMaturedPacket(ctx, packet, data); err != nil { + if err := k.ValidateVSCMaturedPacket(ctx, packet, data); err != nil { return channeltypes.NewErrorAcknowledgement(err.Error()) } chainID := k.getChainIdOrPanic(ctx, packet) - k.handleVSCMaturedPacket(ctx, chainID, data) + k.HandleVSCMaturedPacket(ctx, chainID, data) return channeltypes.NewResultAcknowledgement([]byte{byte(1)}) } -// validateVSCMaturedPacket validates a recv VSCMatured packet before it is +// ValidateVSCMaturedPacket validates a recv VSCMatured packet before it is // handled or persisted in store. An error is returned if the packet is invalid, // and an error ack should be relayed to the sender. -func (k Keeper) validateVSCMaturedPacket(ctx sdk.Context, +func (k Keeper) ValidateVSCMaturedPacket(ctx sdk.Context, packet channeltypes.Packet, data ccv.VSCMaturedPacketData) error { // check that a ccv channel is established via the dest channel of the recv packet @@ -56,7 +56,8 @@ func (k Keeper) validateVSCMaturedPacket(ctx sdk.Context, return nil } -func (k Keeper) handleVSCMaturedPacket(ctx sdk.Context, chainID string, data ccv.VSCMaturedPacketData) { +// TODO: Unit test this method. +func (k Keeper) HandleVSCMaturedPacket(ctx sdk.Context, chainID string, data ccv.VSCMaturedPacketData) { // iterate over the unbonding operations mapped to (chainID, data.ValsetUpdateId) unbondingOps, _ := k.GetUnbondingOpsFromIndex(ctx, chainID, data.ValsetUpdateId) var maturedIds []uint64 diff --git a/x/ccv/provider/keeper/relay_test.go b/x/ccv/provider/keeper/relay_test.go index dbfd008167..4c27acf46a 100644 --- a/x/ccv/provider/keeper/relay_test.go +++ b/x/ccv/provider/keeper/relay_test.go @@ -6,6 +6,7 @@ import ( "github.com/golang/mock/gomock" cryptocodec "github.com/cosmos/cosmos-sdk/crypto/codec" + channeltypes "github.com/cosmos/ibc-go/v3/modules/core/04-channel/types" ibcsimapp "github.com/cosmos/ibc-go/v3/testing/simapp" testkeeper "github.com/cosmos/interchain-security/testutil/keeper" ccv "github.com/cosmos/interchain-security/x/ccv/types" @@ -14,6 +15,28 @@ import ( "github.com/stretchr/testify/require" ) +func TestValidateVSCMaturedPacket(t *testing.T) { + providerKeeper, ctx, ctrl, _ := testkeeper.GetProviderKeeperAndCtx( + t, testkeeper.NewInMemKeeperParams(t)) + defer ctrl.Finish() + + packet := channeltypes.Packet{DestinationChannel: "channel-7"} + data := ccv.VSCMaturedPacketData{} + + // validate method panics if there is no established ccv channel + // with channel ID specified in packet. + require.Panics(t, func() { + providerKeeper.ValidateVSCMaturedPacket(ctx, packet, data) + }) + + // Pseudo setup ccv channel for channel ID specified in packet. + providerKeeper.SetChannelToChain(ctx, "channel-7", "consumer-chain-id") + + // validate method now passes. + err := providerKeeper.ValidateVSCMaturedPacket(ctx, packet, data) + require.NoError(t, err) +} + // TestQueueVSCPackets tests queueing validator set updates. func TestQueueVSCPackets(t *testing.T) { key := ibcsimapp.CreateTestPubKeys(1)[0] From 9bbe227781829ec72c938eca8a90bdc4da801639 Mon Sep 17 00:00:00 2001 From: Shawn Marshall-Spitzbart <44221603+smarshall-spitzbart@users.noreply.github.com> Date: Fri, 2 Dec 2022 11:06:55 -0800 Subject: [PATCH 14/31] validate slash packet UT --- x/ccv/provider/keeper/relay.go | 6 +-- x/ccv/provider/keeper/relay_test.go | 66 +++++++++++++++++++++++++++++ 2 files changed, 69 insertions(+), 3 deletions(-) diff --git a/x/ccv/provider/keeper/relay.go b/x/ccv/provider/keeper/relay.go index 2c6d9f3df7..e55b071e42 100644 --- a/x/ccv/provider/keeper/relay.go +++ b/x/ccv/provider/keeper/relay.go @@ -222,10 +222,10 @@ func (k Keeper) EndBlockCIS(ctx sdk.Context) { k.SetValsetUpdateBlockHeight(ctx, valUpdateID, uint64(ctx.BlockHeight()+1)) } -// OnRecvSlashPacket slashes and jails the given validator in the packet data +// OnRecvSlashPacket receives a slash packet, validates it, then handles it if valid. func (k Keeper) OnRecvSlashPacket(ctx sdk.Context, packet channeltypes.Packet, data ccv.SlashPacketData) exported.Acknowledgement { - if err := k.validateSlashPacket(ctx, packet, data); err != nil { + if err := k.ValidateSlashPacket(ctx, packet, data); err != nil { return channeltypes.NewErrorAcknowledgement(err.Error()) } @@ -239,7 +239,7 @@ func (k Keeper) OnRecvSlashPacket(ctx sdk.Context, packet channeltypes.Packet, d // validateSlashPacket validates a recv slash packet before it is // handled or persisted in store. An error is returned if the packet is invalid, // and an error ack should be relayed to the sender. -func (k Keeper) validateSlashPacket(ctx sdk.Context, +func (k Keeper) ValidateSlashPacket(ctx sdk.Context, packet channeltypes.Packet, data ccv.SlashPacketData) error { // check that a ccv channel is established via the dest channel of the recv packet diff --git a/x/ccv/provider/keeper/relay_test.go b/x/ccv/provider/keeper/relay_test.go index 4c27acf46a..b5c1901587 100644 --- a/x/ccv/provider/keeper/relay_test.go +++ b/x/ccv/provider/keeper/relay_test.go @@ -6,6 +6,7 @@ import ( "github.com/golang/mock/gomock" cryptocodec "github.com/cosmos/cosmos-sdk/crypto/codec" + stakingtypes "github.com/cosmos/cosmos-sdk/x/staking/types" channeltypes "github.com/cosmos/ibc-go/v3/modules/core/04-channel/types" ibcsimapp "github.com/cosmos/ibc-go/v3/testing/simapp" testkeeper "github.com/cosmos/interchain-security/testutil/keeper" @@ -104,3 +105,68 @@ func TestQueueVSCPackets(t *testing.T) { require.Equal(t, tc.expectNextValsetUpdateId, valUpdateID, "valUpdateID (%v != %v) mismatch in case: '%s'", tc.expectNextValsetUpdateId, valUpdateID, tc.name) } } + +// TestValidateSlashPacket tests ValidateSlashPacket. +func TestValidateSlashPacket(t *testing.T) { + + validVscID := uint64(98) + + testCases := []struct { + name string + packetData ccv.SlashPacketData + expectErr bool + }{ + {"no block height found for given vscID", + ccv.SlashPacketData{ValsetUpdateId: 61}, + true}, + {"non-set infraction type", + ccv.SlashPacketData{ValsetUpdateId: validVscID}, + true}, + {"invalid infraction type", + ccv.SlashPacketData{ValsetUpdateId: validVscID, Infraction: stakingtypes.MaxMonikerLength}, + true}, + {"valid double sign packet with non-zero vscID", + ccv.SlashPacketData{ValsetUpdateId: validVscID, Infraction: stakingtypes.DoubleSign}, + false}, + {"valid downtime packet with non-zero vscID", + ccv.SlashPacketData{ValsetUpdateId: validVscID, Infraction: stakingtypes.Downtime}, + false}, + {"valid double sign packet with zero vscID", + ccv.SlashPacketData{ValsetUpdateId: 0, Infraction: stakingtypes.DoubleSign}, + false}, + {"valid downtime packet with zero vscID", + ccv.SlashPacketData{ValsetUpdateId: 0, Infraction: stakingtypes.Downtime}, + false}, + } + + for _, tc := range testCases { + providerKeeper, ctx, ctrl, _ := testkeeper.GetProviderKeeperAndCtx( + t, testkeeper.NewInMemKeeperParams(t)) + defer ctrl.Finish() + + packet := channeltypes.Packet{DestinationChannel: "channel-9"} + + // validate method panics if there is no established ccv channel + // with channel ID specified in packet. + require.Panics(t, func() { + providerKeeper.ValidateSlashPacket(ctx, packet, tc.packetData) + }) + + // Pseudo setup ccv channel for channel ID specified in packet. + providerKeeper.SetChannelToChain(ctx, "channel-9", "consumer-chain-id") + + // Setup init chain height for consumer (allowing 0 vscID to be valid). + providerKeeper.SetInitChainHeight(ctx, "consumer-chain-id", uint64(89)) + + // Setup valset update ID to block height mapping using var instantiated above. + providerKeeper.SetValsetUpdateBlockHeight(ctx, validVscID, uint64(100)) + + // Test error behavior as specified in tc. + err := providerKeeper.ValidateSlashPacket(ctx, packet, tc.packetData) + if tc.expectErr { + require.Error(t, err, "expected error in case: '%s'", tc.name) + } else { + require.NoError(t, err, "unexpected error in case: '%s'", tc.name) + } + } +} From 9e885770605dd820804ff1853748d28be3aec689 Mon Sep 17 00:00:00 2001 From: Shawn Marshall-Spitzbart <44221603+smarshall-spitzbart@users.noreply.github.com> Date: Fri, 2 Dec 2022 13:19:02 -0800 Subject: [PATCH 15/31] final UT --- testutil/keeper/expectations.go | 58 ++++++++++++ x/ccv/provider/keeper/keeper_test.go | 65 ------------- x/ccv/provider/keeper/relay.go | 10 +- x/ccv/provider/keeper/relay_test.go | 136 +++++++++++++++++++++++++++ 4 files changed, 200 insertions(+), 69 deletions(-) diff --git a/testutil/keeper/expectations.go b/testutil/keeper/expectations.go index 9654ba7e61..1478aff546 100644 --- a/testutil/keeper/expectations.go +++ b/testutil/keeper/expectations.go @@ -5,6 +5,7 @@ import ( sdk "github.com/cosmos/cosmos-sdk/types" capabilitytypes "github.com/cosmos/cosmos-sdk/x/capability/types" + stakingtypes "github.com/cosmos/cosmos-sdk/x/staking/types" clienttypes "github.com/cosmos/ibc-go/v3/modules/core/02-client/types" conntypes "github.com/cosmos/ibc-go/v3/modules/core/03-connection/types" channeltypes "github.com/cosmos/ibc-go/v3/modules/core/04-channel/types" @@ -90,6 +91,63 @@ func GetMocksForStopConsumerChain(ctx sdk.Context, mocks *MockedKeepers) []*gomo } } +func GetMocksForHandleSlashPacket(ctx sdk.Context, mocks MockedKeepers, + expectedPacketData ccv.SlashPacketData, valToReturn stakingtypes.Validator) []*gomock.Call { + + // These first two calls are always made. + calls := []*gomock.Call{ + + mocks.MockStakingKeeper.EXPECT().GetValidatorByConsAddr( + ctx, sdk.ConsAddress(expectedPacketData.Validator.Address)).Return( + valToReturn, true, + ).Times(1), + + mocks.MockSlashingKeeper.EXPECT().IsTombstoned(ctx, + sdk.ConsAddress(expectedPacketData.Validator.Address)).Return(false).Times(1), + } + + // Different calls are made depending on the type infraction. + if expectedPacketData.Infraction == stakingtypes.Downtime { + calls = append(calls, + mocks.MockSlashingKeeper.EXPECT().SlashFractionDowntime(ctx).Return(sdk.NewDec(1)).Times(1), + mocks.MockSlashingKeeper.EXPECT().DowntimeJailDuration(ctx).Return(time.Hour).Times(1), + ) + + } else if expectedPacketData.Infraction == stakingtypes.DoubleSign { + calls = append(calls, + mocks.MockSlashingKeeper.EXPECT().SlashFractionDoubleSign(ctx).Return(sdk.NewDec(1)).Times(1), + mocks.MockSlashingKeeper.EXPECT().Tombstone(ctx, + sdk.ConsAddress(expectedPacketData.Validator.Address)).Times(1), + ) + } + + // Slash is always called. + calls = append(calls, mocks.MockStakingKeeper.EXPECT().Slash( + ctx, + sdk.ConsAddress(expectedPacketData.Validator.Address), + gomock.Any(), // infraction height + int64(0), // power + sdk.NewDec(1), // Slash fraction + expectedPacketData.Infraction).Return().Times(1), + ) + + // Jail() is only called if the validator is not already jailed. + if !valToReturn.IsJailed() { + calls = append(calls, mocks.MockStakingKeeper.EXPECT().Jail( + gomock.Eq(ctx), + gomock.Eq(sdk.ConsAddress(expectedPacketData.Validator.Address)), + ).Return()) + } + + // JailUntil() time is always updated. + calls = append(calls, + mocks.MockSlashingKeeper.EXPECT().JailUntil(ctx, sdk.ConsAddress(expectedPacketData.Validator.Address), + gomock.Any()).Times(1), + ) + + return calls +} + func ExpectLatestConsensusStateMock(ctx sdk.Context, mocks MockedKeepers, clientID string, consState *ibctmtypes.ConsensusState) *gomock.Call { return mocks.MockClientKeeper.EXPECT(). GetLatestClientConsensusState(ctx, clientID).Return(consState, true).Times(1) diff --git a/x/ccv/provider/keeper/keeper_test.go b/x/ccv/provider/keeper/keeper_test.go index 8082e7e001..072038f6bc 100644 --- a/x/ccv/provider/keeper/keeper_test.go +++ b/x/ccv/provider/keeper/keeper_test.go @@ -5,13 +5,8 @@ import ( "testing" "time" - evidencetypes "github.com/cosmos/cosmos-sdk/x/evidence/types" - "github.com/golang/mock/gomock" - cryptocodec "github.com/cosmos/cosmos-sdk/crypto/codec" - "github.com/cosmos/cosmos-sdk/crypto/keys/ed25519" sdk "github.com/cosmos/cosmos-sdk/types" - stakingtypes "github.com/cosmos/cosmos-sdk/x/staking/types" ibcsimapp "github.com/cosmos/ibc-go/v3/testing/simapp" @@ -188,66 +183,6 @@ func TestInitHeight(t *testing.T) { } } -// TestHandleSlashPacketDoubleSigning tests the handling of a double-signing related slash packet, with mocks and unit tests -func TestHandleSlashPacketDoubleSigning(t *testing.T) { - - chainId := "consumer" - infractionHeight := int64(5) - - keeperParams := testkeeper.NewInMemKeeperParams(t) - ctx := keeperParams.Ctx - - slashPacket := ccv.NewSlashPacketData( - abci.Validator{Address: ed25519.GenPrivKey().PubKey().Address(), - Power: int64(0)}, - uint64(0), - stakingtypes.DoubleSign, - ) - - ctrl := gomock.NewController(t) - defer ctrl.Finish() - mocks := testkeeper.NewMockedKeepers(ctrl) - mockSlashingKeeper := mocks.MockSlashingKeeper - mockStakingKeeper := mocks.MockStakingKeeper - - // Setup expected mock calls - gomock.InOrder( - - mockStakingKeeper.EXPECT().GetValidatorByConsAddr( - ctx, sdk.ConsAddress(slashPacket.Validator.Address)).Return( - stakingtypes.Validator{Status: stakingtypes.Bonded}, true, - ).Times(1), - - mockSlashingKeeper.EXPECT().IsTombstoned(ctx, sdk.ConsAddress(slashPacket.Validator.Address)).Return(false).Times(1), - - mockSlashingKeeper.EXPECT().SlashFractionDoubleSign(ctx).Return(sdk.NewDec(1)).Times(1), - - mockSlashingKeeper.EXPECT().Tombstone(ctx, sdk.ConsAddress(slashPacket.Validator.Address)).Times(1), - - mockStakingKeeper.EXPECT().Slash( - ctx, - sdk.ConsAddress(slashPacket.Validator.Address), - infractionHeight, - int64(0), // power - sdk.NewDec(1), // Slash fraction - stakingtypes.DoubleSign).Return().Times(1), - - mockStakingKeeper.EXPECT().Jail( - gomock.Eq(ctx), - gomock.Eq(sdk.ConsAddress(slashPacket.Validator.Address)), - ).Return(), - - mockSlashingKeeper.EXPECT().JailUntil(ctx, sdk.ConsAddress(slashPacket.Validator.Address), - evidencetypes.DoubleSignJailEndTime).Times(1), - ) - - providerKeeper := testkeeper.NewInMemProviderKeeper(keeperParams, mocks) - - providerKeeper.SetInitChainHeight(ctx, chainId, uint64(infractionHeight)) - - providerKeeper.HandleSlashPacket(ctx, chainId, slashPacket) -} - func TestIterateOverUnbondingOpIndex(t *testing.T) { providerKeeper, ctx, ctrl, _ := testkeeper.GetProviderKeeperAndCtx(t, testkeeper.NewInMemKeeperParams(t)) diff --git a/x/ccv/provider/keeper/relay.go b/x/ccv/provider/keeper/relay.go index e55b071e42..e66bcf9558 100644 --- a/x/ccv/provider/keeper/relay.go +++ b/x/ccv/provider/keeper/relay.go @@ -290,6 +290,12 @@ func (k Keeper) HandleSlashPacket(ctx sdk.Context, chainID string, data ccv.Slas slashFraction sdk.Dec ) + // Obtain infraction height or panic + infractionHeight, found := k.getMappedInfractionHeight(ctx, chainID, data.ValsetUpdateId) + if !found { + panic("infraction height not found. But was found during slash packet validation") + } + switch data.Infraction { case stakingtypes.Downtime: // set the downtime slash fraction and duration @@ -306,10 +312,6 @@ func (k Keeper) HandleSlashPacket(ctx sdk.Context, chainID string, data ccv.Slas } // slash validator - infractionHeight, found := k.getMappedInfractionHeight(ctx, chainID, data.ValsetUpdateId) - if !found { - panic("infraction height not found. But was found during slash packet validation") - } k.stakingKeeper.Slash( ctx, consAddr, diff --git a/x/ccv/provider/keeper/relay_test.go b/x/ccv/provider/keeper/relay_test.go index b5c1901587..e984b9208e 100644 --- a/x/ccv/provider/keeper/relay_test.go +++ b/x/ccv/provider/keeper/relay_test.go @@ -6,6 +6,7 @@ import ( "github.com/golang/mock/gomock" cryptocodec "github.com/cosmos/cosmos-sdk/crypto/codec" + sdk "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" ibcsimapp "github.com/cosmos/ibc-go/v3/testing/simapp" @@ -170,3 +171,138 @@ func TestValidateSlashPacket(t *testing.T) { } } } + +// TestHandleSlashPacket tests the handling of slash packets. +func TestHandleSlashPacket(t *testing.T) { + + chainId := "consumer-id" + validVscID := uint64(234) + + testCases := []struct { + name string + packetData ccv.SlashPacketData + // The mocks that we expect to be called for the specified packet data. + expectedCalls func(sdk.Context, testkeeper.MockedKeepers, ccv.SlashPacketData) []*gomock.Call + expectPanic bool + }{ + { + "not found validator", + ccv.SlashPacketData{}, + func(ctx sdk.Context, mocks testkeeper.MockedKeepers, + expectedPacketData ccv.SlashPacketData, + ) []*gomock.Call { + return []*gomock.Call{ + // We only expect a single call to GetValidatorByConsAddr. + // Method will return once validator is not found. + mocks.MockStakingKeeper.EXPECT().GetValidatorByConsAddr( + ctx, sdk.ConsAddress(expectedPacketData.Validator.Address)).Return( + stakingtypes.Validator{}, false, // false = Not found. + ).Times(1), + } + }, + false, // No panic expected. + }, + { + "found, but tombstoned validator", + ccv.SlashPacketData{}, + func(ctx sdk.Context, mocks testkeeper.MockedKeepers, + expectedPacketData ccv.SlashPacketData, + ) []*gomock.Call { + return []*gomock.Call{ + mocks.MockStakingKeeper.EXPECT().GetValidatorByConsAddr( + ctx, sdk.ConsAddress(expectedPacketData.Validator.Address)).Return( + stakingtypes.Validator{}, true, // true = Found. + ).Times(1), + // Execution will stop after this call as validator is tombstoned. + mocks.MockSlashingKeeper.EXPECT().IsTombstoned(ctx, + sdk.ConsAddress(expectedPacketData.Validator.Address)).Return(true).Times(1), + } + }, + false, // No panic expected. + }, + { + "panic on infraction height not found", + ccv.SlashPacketData{ValsetUpdateId: 78}, // Keeper doesn't have a height mapped to this vscID. + func(ctx sdk.Context, mocks testkeeper.MockedKeepers, + expectedPacketData ccv.SlashPacketData, + ) []*gomock.Call { + return []*gomock.Call{ + + mocks.MockStakingKeeper.EXPECT().GetValidatorByConsAddr( + ctx, sdk.ConsAddress(expectedPacketData.Validator.Address)).Return( + stakingtypes.Validator{}, true, + ).Times(1), + + mocks.MockSlashingKeeper.EXPECT().IsTombstoned(ctx, + sdk.ConsAddress(expectedPacketData.Validator.Address)).Return(false).Times(1), + } + }, + true, // Panic expected. + }, + { + "full downtime packet handling, uses init chain height and non-jailed validator", + ccv.NewSlashPacketData(abci.Validator{}, 0, stakingtypes.Downtime), // ValsetUpdateId = 0 uses init chain height. + func(ctx sdk.Context, mocks testkeeper.MockedKeepers, + expectedPacketData ccv.SlashPacketData, + ) []*gomock.Call { + return testkeeper.GetMocksForHandleSlashPacket( + ctx, mocks, expectedPacketData, stakingtypes.Validator{Jailed: false}) + }, + false, // No panic expected. + }, + { + "full downtime packet handling, uses valid vscID and jailed validator", + ccv.NewSlashPacketData(abci.Validator{}, validVscID, stakingtypes.Downtime), + func(ctx sdk.Context, mocks testkeeper.MockedKeepers, + expectedPacketData ccv.SlashPacketData, + ) []*gomock.Call { + return testkeeper.GetMocksForHandleSlashPacket( + ctx, mocks, expectedPacketData, stakingtypes.Validator{Jailed: true}) + }, + false, // No panic expected. + }, + { + "full double sign packet handling, uses init chain height and jailed validator", + ccv.NewSlashPacketData(abci.Validator{}, 0, stakingtypes.DoubleSign), + func(ctx sdk.Context, mocks testkeeper.MockedKeepers, + expectedPacketData ccv.SlashPacketData, + ) []*gomock.Call { + return testkeeper.GetMocksForHandleSlashPacket( + ctx, mocks, expectedPacketData, stakingtypes.Validator{Jailed: true}) + }, + false, // No panic expected. + }, + { + "full double sign packet handling, uses valid vsc id and non-jailed validator", + ccv.NewSlashPacketData(abci.Validator{}, validVscID, stakingtypes.DoubleSign), + func(ctx sdk.Context, mocks testkeeper.MockedKeepers, + expectedPacketData ccv.SlashPacketData, + ) []*gomock.Call { + return testkeeper.GetMocksForHandleSlashPacket( + ctx, mocks, expectedPacketData, stakingtypes.Validator{Jailed: false}) + }, + false, // No panic expected. + }, + } + + for _, tc := range testCases { + + providerKeeper, ctx, ctrl, mocks := testkeeper.GetProviderKeeperAndCtx( + t, testkeeper.NewInMemKeeperParams(t)) + + // Setup expected mock calls + gomock.InOrder(tc.expectedCalls(ctx, mocks, tc.packetData)...) + + // Setup init chain height and a single valid valset update ID to block height mapping. + providerKeeper.SetInitChainHeight(ctx, chainId, 5) + providerKeeper.SetValsetUpdateBlockHeight(ctx, validVscID, 99) + + // Execute method and assert expected mock calls. + if tc.expectPanic { + require.Panics(t, func() { providerKeeper.HandleSlashPacket(ctx, chainId, tc.packetData) }) + } else { + providerKeeper.HandleSlashPacket(ctx, chainId, tc.packetData) + } + ctrl.Finish() + } +} From 44b370d0aa7665f73cdc3dab92e02008926f62e4 Mon Sep 17 00:00:00 2001 From: Shawn Marshall-Spitzbart <44221603+smarshall-spitzbart@users.noreply.github.com> Date: Fri, 2 Dec 2022 13:25:22 -0800 Subject: [PATCH 16/31] lint fix --- x/ccv/provider/keeper/relay_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/x/ccv/provider/keeper/relay_test.go b/x/ccv/provider/keeper/relay_test.go index e984b9208e..b7662f1480 100644 --- a/x/ccv/provider/keeper/relay_test.go +++ b/x/ccv/provider/keeper/relay_test.go @@ -28,7 +28,7 @@ func TestValidateVSCMaturedPacket(t *testing.T) { // validate method panics if there is no established ccv channel // with channel ID specified in packet. require.Panics(t, func() { - providerKeeper.ValidateVSCMaturedPacket(ctx, packet, data) + _ = providerKeeper.ValidateVSCMaturedPacket(ctx, packet, data) }) // Pseudo setup ccv channel for channel ID specified in packet. @@ -150,7 +150,7 @@ func TestValidateSlashPacket(t *testing.T) { // validate method panics if there is no established ccv channel // with channel ID specified in packet. require.Panics(t, func() { - providerKeeper.ValidateSlashPacket(ctx, packet, tc.packetData) + _ = providerKeeper.ValidateSlashPacket(ctx, packet, tc.packetData) }) // Pseudo setup ccv channel for channel ID specified in packet. From 50e7f23945d3110e32d216650dc289250390925a Mon Sep 17 00:00:00 2001 From: Shawn Marshall-Spitzbart <44221603+smarshall-spitzbart@users.noreply.github.com> Date: Fri, 2 Dec 2022 17:04:41 -0800 Subject: [PATCH 17/31] Update relay.go --- x/ccv/provider/keeper/relay.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x/ccv/provider/keeper/relay.go b/x/ccv/provider/keeper/relay.go index e66bcf9558..de76b321e0 100644 --- a/x/ccv/provider/keeper/relay.go +++ b/x/ccv/provider/keeper/relay.go @@ -293,7 +293,7 @@ func (k Keeper) HandleSlashPacket(ctx sdk.Context, chainID string, data ccv.Slas // Obtain infraction height or panic infractionHeight, found := k.getMappedInfractionHeight(ctx, chainID, data.ValsetUpdateId) if !found { - panic("infraction height not found. But was found during slash packet validation") + k.Logger(ctx).Error("infraction height not found. But was found during slash packet validation") } switch data.Infraction { From 0480126be2bee827d00599bc5234fe01d96f2533 Mon Sep 17 00:00:00 2001 From: Shawn Marshall-Spitzbart <44221603+smarshall-spitzbart@users.noreply.github.com> Date: Fri, 2 Dec 2022 17:07:57 -0800 Subject: [PATCH 18/31] Update relay.go --- x/ccv/provider/keeper/relay.go | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/x/ccv/provider/keeper/relay.go b/x/ccv/provider/keeper/relay.go index de76b321e0..3b1f81912c 100644 --- a/x/ccv/provider/keeper/relay.go +++ b/x/ccv/provider/keeper/relay.go @@ -245,7 +245,13 @@ func (k Keeper) ValidateSlashPacket(ctx sdk.Context, // check that a ccv channel is established via the dest channel of the recv packet chainID := k.getChainIdOrPanic(ctx, packet) - _, found := k.getMappedInfractionHeight(ctx, chainID, data.ValsetUpdateId) + validator, found := k.stakingKeeper.GetValidatorByConsAddr( + ctx, sdk.ConsAddress(data.Validator.Address)) + if !found || validator.IsUnbonded() { + return fmt.Errorf("validator with addr %s not found or unbonded", data.Validator.Address) + } + + _, found = k.getMappedInfractionHeight(ctx, chainID, data.ValsetUpdateId) // return error if we cannot find infraction height matching the validator update id if !found { return fmt.Errorf("cannot find infraction height matching "+ @@ -290,10 +296,11 @@ func (k Keeper) HandleSlashPacket(ctx sdk.Context, chainID string, data ccv.Slas slashFraction sdk.Dec ) - // Obtain infraction height or panic + // Obtain infraction height, or log error and drop packet infractionHeight, found := k.getMappedInfractionHeight(ctx, chainID, data.ValsetUpdateId) if !found { k.Logger(ctx).Error("infraction height not found. But was found during slash packet validation") + return } switch data.Infraction { From d32b4042908f7ea07d619968ddf455528e0ff7a1 Mon Sep 17 00:00:00 2001 From: Shawn Marshall-Spitzbart <44221603+smarshall-spitzbart@users.noreply.github.com> Date: Fri, 2 Dec 2022 17:10:44 -0800 Subject: [PATCH 19/31] Update relay.go --- x/ccv/provider/keeper/relay.go | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/x/ccv/provider/keeper/relay.go b/x/ccv/provider/keeper/relay.go index 3b1f81912c..607387cf8f 100644 --- a/x/ccv/provider/keeper/relay.go +++ b/x/ccv/provider/keeper/relay.go @@ -245,13 +245,7 @@ func (k Keeper) ValidateSlashPacket(ctx sdk.Context, // check that a ccv channel is established via the dest channel of the recv packet chainID := k.getChainIdOrPanic(ctx, packet) - validator, found := k.stakingKeeper.GetValidatorByConsAddr( - ctx, sdk.ConsAddress(data.Validator.Address)) - if !found || validator.IsUnbonded() { - return fmt.Errorf("validator with addr %s not found or unbonded", data.Validator.Address) - } - - _, found = k.getMappedInfractionHeight(ctx, chainID, data.ValsetUpdateId) + _, found := k.getMappedInfractionHeight(ctx, chainID, data.ValsetUpdateId) // return error if we cannot find infraction height matching the validator update id if !found { return fmt.Errorf("cannot find infraction height matching "+ From eeb742061e8a3da755a7fa5cfd2353a2530cd508 Mon Sep 17 00:00:00 2001 From: Shawn Marshall-Spitzbart <44221603+smarshall-spitzbart@users.noreply.github.com> Date: Fri, 2 Dec 2022 17:12:14 -0800 Subject: [PATCH 20/31] Update relay.go --- x/ccv/provider/keeper/relay.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/x/ccv/provider/keeper/relay.go b/x/ccv/provider/keeper/relay.go index 607387cf8f..142d7d3fbf 100644 --- a/x/ccv/provider/keeper/relay.go +++ b/x/ccv/provider/keeper/relay.go @@ -272,8 +272,7 @@ func (k Keeper) HandleSlashPacket(ctx sdk.Context, chainID string, data ccv.Slas // stakingKeeper.Slash() panics otherwise if !found || validator.IsUnbonded() { // if validator is not found or is unbonded drop slash packet and log error - k.Logger(ctx).Error("validator not found or is unbonded. However, this validator"+ - " was found and bonded at slash packet recv time", "validator", data.Validator.Address) + k.Logger(ctx).Error("validator not found or is unbonded", "validator", data.Validator.Address) return } From 568e1c4769113ba455a15730a22822b7c302ac08 Mon Sep 17 00:00:00 2001 From: Shawn Marshall-Spitzbart <44221603+smarshall-spitzbart@users.noreply.github.com> Date: Fri, 2 Dec 2022 17:15:12 -0800 Subject: [PATCH 21/31] Update relay_test.go --- x/ccv/provider/keeper/relay_test.go | 16 ++-------------- 1 file changed, 2 insertions(+), 14 deletions(-) diff --git a/x/ccv/provider/keeper/relay_test.go b/x/ccv/provider/keeper/relay_test.go index b7662f1480..8b2a88df7a 100644 --- a/x/ccv/provider/keeper/relay_test.go +++ b/x/ccv/provider/keeper/relay_test.go @@ -183,7 +183,6 @@ func TestHandleSlashPacket(t *testing.T) { packetData ccv.SlashPacketData // The mocks that we expect to be called for the specified packet data. expectedCalls func(sdk.Context, testkeeper.MockedKeepers, ccv.SlashPacketData) []*gomock.Call - expectPanic bool }{ { "not found validator", @@ -200,7 +199,6 @@ func TestHandleSlashPacket(t *testing.T) { ).Times(1), } }, - false, // No panic expected. }, { "found, but tombstoned validator", @@ -218,10 +216,9 @@ func TestHandleSlashPacket(t *testing.T) { sdk.ConsAddress(expectedPacketData.Validator.Address)).Return(true).Times(1), } }, - false, // No panic expected. }, { - "panic on infraction height not found", + "drop packet when infraction height not found", ccv.SlashPacketData{ValsetUpdateId: 78}, // Keeper doesn't have a height mapped to this vscID. func(ctx sdk.Context, mocks testkeeper.MockedKeepers, expectedPacketData ccv.SlashPacketData, @@ -237,7 +234,6 @@ func TestHandleSlashPacket(t *testing.T) { sdk.ConsAddress(expectedPacketData.Validator.Address)).Return(false).Times(1), } }, - true, // Panic expected. }, { "full downtime packet handling, uses init chain height and non-jailed validator", @@ -248,7 +244,6 @@ func TestHandleSlashPacket(t *testing.T) { return testkeeper.GetMocksForHandleSlashPacket( ctx, mocks, expectedPacketData, stakingtypes.Validator{Jailed: false}) }, - false, // No panic expected. }, { "full downtime packet handling, uses valid vscID and jailed validator", @@ -259,7 +254,6 @@ func TestHandleSlashPacket(t *testing.T) { return testkeeper.GetMocksForHandleSlashPacket( ctx, mocks, expectedPacketData, stakingtypes.Validator{Jailed: true}) }, - false, // No panic expected. }, { "full double sign packet handling, uses init chain height and jailed validator", @@ -270,7 +264,6 @@ func TestHandleSlashPacket(t *testing.T) { return testkeeper.GetMocksForHandleSlashPacket( ctx, mocks, expectedPacketData, stakingtypes.Validator{Jailed: true}) }, - false, // No panic expected. }, { "full double sign packet handling, uses valid vsc id and non-jailed validator", @@ -281,7 +274,6 @@ func TestHandleSlashPacket(t *testing.T) { return testkeeper.GetMocksForHandleSlashPacket( ctx, mocks, expectedPacketData, stakingtypes.Validator{Jailed: false}) }, - false, // No panic expected. }, } @@ -298,11 +290,7 @@ func TestHandleSlashPacket(t *testing.T) { providerKeeper.SetValsetUpdateBlockHeight(ctx, validVscID, 99) // Execute method and assert expected mock calls. - if tc.expectPanic { - require.Panics(t, func() { providerKeeper.HandleSlashPacket(ctx, chainId, tc.packetData) }) - } else { - providerKeeper.HandleSlashPacket(ctx, chainId, tc.packetData) - } + providerKeeper.HandleSlashPacket(ctx, chainId, tc.packetData) ctrl.Finish() } } From 660f4243412d0b0ffa3077a900dd08b7a85c5564 Mon Sep 17 00:00:00 2001 From: Shawn Marshall-Spitzbart <44221603+smarshall-spitzbart@users.noreply.github.com> Date: Mon, 5 Dec 2022 16:00:44 -0800 Subject: [PATCH 22/31] rm getChainIDOrPanic --- x/ccv/provider/keeper/relay.go | 54 +++++++++++------------------ x/ccv/provider/keeper/relay_test.go | 30 +--------------- 2 files changed, 21 insertions(+), 63 deletions(-) diff --git a/x/ccv/provider/keeper/relay.go b/x/ccv/provider/keeper/relay.go index 142d7d3fbf..e11cd41391 100644 --- a/x/ccv/provider/keeper/relay.go +++ b/x/ccv/provider/keeper/relay.go @@ -35,27 +35,21 @@ func (k Keeper) OnRecvVSCMaturedPacket( data ccv.VSCMaturedPacketData, ) exported.Acknowledgement { - if err := k.ValidateVSCMaturedPacket(ctx, packet, data); err != nil { - return channeltypes.NewErrorAcknowledgement(err.Error()) + // check that the channel is established, panic if not + chainID, found := k.GetChannelToChain(ctx, packet.DestinationChannel) + if !found { + // VSCMatured packet was sent on a channel different than any of the established CCV channels; + // this should never happen + panic(fmt.Errorf("VSCMaturedPacket received on unknown channel %s", packet.DestinationChannel)) } - chainID := k.getChainIdOrPanic(ctx, packet) + // Note: no validation is needed for recv VSCMatured packets, + // since the packet data only includes a valset update id that can take any uint64 value. + k.HandleVSCMaturedPacket(ctx, chainID, data) return channeltypes.NewResultAcknowledgement([]byte{byte(1)}) } -// ValidateVSCMaturedPacket validates a recv VSCMatured packet before it is -// handled or persisted in store. An error is returned if the packet is invalid, -// and an error ack should be relayed to the sender. -func (k Keeper) ValidateVSCMaturedPacket(ctx sdk.Context, - packet channeltypes.Packet, data ccv.VSCMaturedPacketData) error { - - // check that a ccv channel is established via the dest channel of the recv packet - _ = k.getChainIdOrPanic(ctx, packet) - - return nil -} - // TODO: Unit test this method. func (k Keeper) HandleVSCMaturedPacket(ctx sdk.Context, chainID string, data ccv.VSCMaturedPacketData) { // iterate over the unbonding operations mapped to (chainID, data.ValsetUpdateId) @@ -225,26 +219,30 @@ func (k Keeper) EndBlockCIS(ctx sdk.Context) { // OnRecvSlashPacket receives a slash packet, validates it, then handles it if valid. func (k Keeper) OnRecvSlashPacket(ctx sdk.Context, packet channeltypes.Packet, data ccv.SlashPacketData) exported.Acknowledgement { - if err := k.ValidateSlashPacket(ctx, packet, data); err != nil { + // check that the channel is established, panic if not + chainID, found := k.GetChannelToChain(ctx, packet.DestinationChannel) + if !found { + // SlashPacket packet was sent on a channel different than any of the established CCV channels; + // this should never happen + panic(fmt.Errorf("SlashPacket received on unknown channel %s", packet.DestinationChannel)) + } + + if err := k.ValidateSlashPacket(ctx, chainID, packet, data); err != nil { return channeltypes.NewErrorAcknowledgement(err.Error()) } // apply slashing - chainID := k.getChainIdOrPanic(ctx, packet) k.HandleSlashPacket(ctx, chainID, data) return channeltypes.NewResultAcknowledgement([]byte{byte(1)}) } -// validateSlashPacket validates a recv slash packet before it is +// ValidateSlashPacket validates a recv slash packet before it is // handled or persisted in store. An error is returned if the packet is invalid, // and an error ack should be relayed to the sender. -func (k Keeper) ValidateSlashPacket(ctx sdk.Context, +func (k Keeper) ValidateSlashPacket(ctx sdk.Context, chainID string, packet channeltypes.Packet, data ccv.SlashPacketData) error { - // check that a ccv channel is established via the dest channel of the recv packet - chainID := k.getChainIdOrPanic(ctx, packet) - _, found := k.getMappedInfractionHeight(ctx, chainID, data.ValsetUpdateId) // return error if we cannot find infraction height matching the validator update id if !found { @@ -415,15 +413,3 @@ func (k Keeper) getMappedInfractionHeight(ctx sdk.Context, return k.GetValsetUpdateBlockHeight(ctx, valsetUpdateID) } } - -// getChainIdOrPanic returns the chainID from a recv packet, -// or panics if the packet was sent on a different channel than any of the established CCV channels. -func (k Keeper) getChainIdOrPanic(ctx sdk.Context, packet channeltypes.Packet) string { - chainID, found := k.GetChannelToChain(ctx, packet.DestinationChannel) - if !found { - // Packet was sent on a channel different than any of the established CCV channels; - // this should never happen - panic(fmt.Sprintf("channel %s not found", packet.DestinationChannel)) - } - return chainID -} diff --git a/x/ccv/provider/keeper/relay_test.go b/x/ccv/provider/keeper/relay_test.go index 8b2a88df7a..eaf2d38dd7 100644 --- a/x/ccv/provider/keeper/relay_test.go +++ b/x/ccv/provider/keeper/relay_test.go @@ -17,28 +17,6 @@ import ( "github.com/stretchr/testify/require" ) -func TestValidateVSCMaturedPacket(t *testing.T) { - providerKeeper, ctx, ctrl, _ := testkeeper.GetProviderKeeperAndCtx( - t, testkeeper.NewInMemKeeperParams(t)) - defer ctrl.Finish() - - packet := channeltypes.Packet{DestinationChannel: "channel-7"} - data := ccv.VSCMaturedPacketData{} - - // validate method panics if there is no established ccv channel - // with channel ID specified in packet. - require.Panics(t, func() { - _ = providerKeeper.ValidateVSCMaturedPacket(ctx, packet, data) - }) - - // Pseudo setup ccv channel for channel ID specified in packet. - providerKeeper.SetChannelToChain(ctx, "channel-7", "consumer-chain-id") - - // validate method now passes. - err := providerKeeper.ValidateVSCMaturedPacket(ctx, packet, data) - require.NoError(t, err) -} - // TestQueueVSCPackets tests queueing validator set updates. func TestQueueVSCPackets(t *testing.T) { key := ibcsimapp.CreateTestPubKeys(1)[0] @@ -147,12 +125,6 @@ func TestValidateSlashPacket(t *testing.T) { packet := channeltypes.Packet{DestinationChannel: "channel-9"} - // validate method panics if there is no established ccv channel - // with channel ID specified in packet. - require.Panics(t, func() { - _ = providerKeeper.ValidateSlashPacket(ctx, packet, tc.packetData) - }) - // Pseudo setup ccv channel for channel ID specified in packet. providerKeeper.SetChannelToChain(ctx, "channel-9", "consumer-chain-id") @@ -163,7 +135,7 @@ func TestValidateSlashPacket(t *testing.T) { providerKeeper.SetValsetUpdateBlockHeight(ctx, validVscID, uint64(100)) // Test error behavior as specified in tc. - err := providerKeeper.ValidateSlashPacket(ctx, packet, tc.packetData) + err := providerKeeper.ValidateSlashPacket(ctx, "consumer-chain-id", packet, tc.packetData) if tc.expectErr { require.Error(t, err, "expected error in case: '%s'", tc.name) } else { From 80cc6467e842970ee873fd256e4e0c76552464db Mon Sep 17 00:00:00 2001 From: Shawn Marshall-Spitzbart <44221603+smarshall-spitzbart@users.noreply.github.com> Date: Mon, 5 Dec 2022 16:03:25 -0800 Subject: [PATCH 23/31] Update relay.go --- x/ccv/provider/keeper/relay.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x/ccv/provider/keeper/relay.go b/x/ccv/provider/keeper/relay.go index e11cd41391..05f51d6538 100644 --- a/x/ccv/provider/keeper/relay.go +++ b/x/ccv/provider/keeper/relay.go @@ -43,7 +43,7 @@ func (k Keeper) OnRecvVSCMaturedPacket( panic(fmt.Errorf("VSCMaturedPacket received on unknown channel %s", packet.DestinationChannel)) } - // Note: no validation is needed for recv VSCMatured packets, + // Note: no validation is needed (that'd return an IBC err ack) for recv VSCMatured packets, // since the packet data only includes a valset update id that can take any uint64 value. k.HandleVSCMaturedPacket(ctx, chainID, data) From dbbd40038f8857d0ebc26ed25cbe0c7b74b58b2b Mon Sep 17 00:00:00 2001 From: Shawn Marshall-Spitzbart <44221603+smarshall-spitzbart@users.noreply.github.com> Date: Tue, 6 Dec 2022 19:10:43 -0800 Subject: [PATCH 24/31] slash frac var --- testutil/keeper/expectations.go | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/testutil/keeper/expectations.go b/testutil/keeper/expectations.go index 1478aff546..133c56ada2 100644 --- a/testutil/keeper/expectations.go +++ b/testutil/keeper/expectations.go @@ -94,6 +94,9 @@ func GetMocksForStopConsumerChain(ctx sdk.Context, mocks *MockedKeepers) []*gomo func GetMocksForHandleSlashPacket(ctx sdk.Context, mocks MockedKeepers, expectedPacketData ccv.SlashPacketData, valToReturn stakingtypes.Validator) []*gomock.Call { + // An arbitrary slash fraction returned and later used by mock calls. + arbitrarySlashFraction := sdk.NewDec(1) + // These first two calls are always made. calls := []*gomock.Call{ @@ -109,13 +112,13 @@ func GetMocksForHandleSlashPacket(ctx sdk.Context, mocks MockedKeepers, // Different calls are made depending on the type infraction. if expectedPacketData.Infraction == stakingtypes.Downtime { calls = append(calls, - mocks.MockSlashingKeeper.EXPECT().SlashFractionDowntime(ctx).Return(sdk.NewDec(1)).Times(1), + mocks.MockSlashingKeeper.EXPECT().SlashFractionDowntime(ctx).Return(arbitrarySlashFraction).Times(1), mocks.MockSlashingKeeper.EXPECT().DowntimeJailDuration(ctx).Return(time.Hour).Times(1), ) } else if expectedPacketData.Infraction == stakingtypes.DoubleSign { calls = append(calls, - mocks.MockSlashingKeeper.EXPECT().SlashFractionDoubleSign(ctx).Return(sdk.NewDec(1)).Times(1), + mocks.MockSlashingKeeper.EXPECT().SlashFractionDoubleSign(ctx).Return(arbitrarySlashFraction).Times(1), mocks.MockSlashingKeeper.EXPECT().Tombstone(ctx, sdk.ConsAddress(expectedPacketData.Validator.Address)).Times(1), ) @@ -125,9 +128,9 @@ func GetMocksForHandleSlashPacket(ctx sdk.Context, mocks MockedKeepers, calls = append(calls, mocks.MockStakingKeeper.EXPECT().Slash( ctx, sdk.ConsAddress(expectedPacketData.Validator.Address), - gomock.Any(), // infraction height - int64(0), // power - sdk.NewDec(1), // Slash fraction + gomock.Any(), // infraction height + int64(0), // power + arbitrarySlashFraction, expectedPacketData.Infraction).Return().Times(1), ) From 7b5a01297ce5c21372b67e6ac075d09fb72360cb Mon Sep 17 00:00:00 2001 From: Shawn Marshall-Spitzbart <44221603+smarshall-spitzbart@users.noreply.github.com> Date: Tue, 6 Dec 2022 19:14:00 -0800 Subject: [PATCH 25/31] rm comment --- x/ccv/provider/keeper/hooks.go | 4 ---- 1 file changed, 4 deletions(-) diff --git a/x/ccv/provider/keeper/hooks.go b/x/ccv/provider/keeper/hooks.go index ffd9dae56d..49aa24169f 100644 --- a/x/ccv/provider/keeper/hooks.go +++ b/x/ccv/provider/keeper/hooks.go @@ -46,10 +46,6 @@ func (h Hooks) AfterUnbondingInitiated(ctx sdk.Context, ID uint64) error { h.k.SetUnbondingOpIndex(ctx, consumerChainID, valsetUpdateID, index) } - // Set unbonding op. - // If there is an error persisting the unbonding op, the method will - // panic to end execution for the current tx and prevent committal - // of this invalid state. h.k.SetUnbondingOp(ctx, unbondingOp) // Call back into staking to tell it to stop this op from unbonding when the unbonding period is complete From e49b82f321b007b10528f3cd985cead8f01cdd3f Mon Sep 17 00:00:00 2001 From: Shawn Marshall-Spitzbart <44221603+smarshall-spitzbart@users.noreply.github.com> Date: Tue, 6 Dec 2022 19:17:48 -0800 Subject: [PATCH 26/31] handle vsc comment --- x/ccv/provider/keeper/relay.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/x/ccv/provider/keeper/relay.go b/x/ccv/provider/keeper/relay.go index 6ba4be5e02..4d4cb162c8 100644 --- a/x/ccv/provider/keeper/relay.go +++ b/x/ccv/provider/keeper/relay.go @@ -50,6 +50,11 @@ func (k Keeper) OnRecvVSCMaturedPacket( return channeltypes.NewResultAcknowledgement([]byte{byte(1)}) } +// HandleVSCMaturedPacket handles a VSCMatured packet. +// +// Note: This method should only panic for a system critical error like a +// failed marshal/unmarshal, or persistence of critical data. +// // TODO: Unit test this method. func (k Keeper) HandleVSCMaturedPacket(ctx sdk.Context, chainID string, data ccv.VSCMaturedPacketData) { // iterate over the unbonding operations mapped to (chainID, data.ValsetUpdateId) From 29fa60fd465e5bb256ebe5a06ee99c3d2c9c3408 Mon Sep 17 00:00:00 2001 From: Shawn Marshall-Spitzbart <44221603+smarshall-spitzbart@users.noreply.github.com> Date: Tue, 6 Dec 2022 19:19:15 -0800 Subject: [PATCH 27/31] on recv slash comment --- x/ccv/provider/keeper/relay.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x/ccv/provider/keeper/relay.go b/x/ccv/provider/keeper/relay.go index 4d4cb162c8..bf0c6b5604 100644 --- a/x/ccv/provider/keeper/relay.go +++ b/x/ccv/provider/keeper/relay.go @@ -221,7 +221,7 @@ func (k Keeper) EndBlockCIS(ctx sdk.Context) { k.SetValsetUpdateBlockHeight(ctx, valUpdateID, uint64(ctx.BlockHeight()+1)) } -// OnRecvSlashPacket receives a slash packet, validates it, then handles it if valid. +// OnRecvSlashPacket delivers a received slash packet: validates it and then handles it if valid. func (k Keeper) OnRecvSlashPacket(ctx sdk.Context, packet channeltypes.Packet, data ccv.SlashPacketData) exported.Acknowledgement { // check that the channel is established, panic if not From c7e3ef36f0da0223534e5163811dbd642d763ebc Mon Sep 17 00:00:00 2001 From: Shawn Marshall-Spitzbart <44221603+smarshall-spitzbart@users.noreply.github.com> Date: Tue, 6 Dec 2022 19:21:55 -0800 Subject: [PATCH 28/31] comment --- x/ccv/provider/keeper/relay.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x/ccv/provider/keeper/relay.go b/x/ccv/provider/keeper/relay.go index bf0c6b5604..eacfc20eb0 100644 --- a/x/ccv/provider/keeper/relay.go +++ b/x/ccv/provider/keeper/relay.go @@ -236,7 +236,7 @@ func (k Keeper) OnRecvSlashPacket(ctx sdk.Context, packet channeltypes.Packet, d return channeltypes.NewErrorAcknowledgement(err.Error()) } - // apply slashing + // handle slashing request k.HandleSlashPacket(ctx, chainID, data) return channeltypes.NewResultAcknowledgement([]byte{byte(1)}) From a8ec9e97b045e475a3563e8f8cba5412c8b054b1 Mon Sep 17 00:00:00 2001 From: Shawn Marshall-Spitzbart <44221603+smarshall-spitzbart@users.noreply.github.com> Date: Tue, 6 Dec 2022 19:23:18 -0800 Subject: [PATCH 29/31] not found comment --- x/ccv/provider/keeper/relay.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/x/ccv/provider/keeper/relay.go b/x/ccv/provider/keeper/relay.go index eacfc20eb0..88e06679ce 100644 --- a/x/ccv/provider/keeper/relay.go +++ b/x/ccv/provider/keeper/relay.go @@ -274,7 +274,10 @@ func (k Keeper) HandleSlashPacket(ctx sdk.Context, chainID string, data ccv.Slas // make sure the validator is not yet unbonded; // stakingKeeper.Slash() panics otherwise if !found || validator.IsUnbonded() { - // if validator is not found or is unbonded drop slash packet and log error + // if validator is not found or is unbonded, drop slash packet and log error. + // Note that it is impossible for the validator to be not found or unbonded if both the provider + // and the consumer are following the protocol. Thus if this branch is taken then one or both + // chains is incorrect, but it is impossible to tell which. k.Logger(ctx).Error("validator not found or is unbonded", "validator", data.Validator.Address) return } From cfdf6627115b9d2d1e6692cc4b22f5b294acbd89 Mon Sep 17 00:00:00 2001 From: Shawn Marshall-Spitzbart <44221603+smarshall-spitzbart@users.noreply.github.com> Date: Tue, 6 Dec 2022 19:25:28 -0800 Subject: [PATCH 30/31] log --- x/ccv/provider/keeper/relay.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/x/ccv/provider/keeper/relay.go b/x/ccv/provider/keeper/relay.go index 88e06679ce..f3d69e683a 100644 --- a/x/ccv/provider/keeper/relay.go +++ b/x/ccv/provider/keeper/relay.go @@ -284,7 +284,11 @@ func (k Keeper) HandleSlashPacket(ctx sdk.Context, chainID string, data ccv.Slas // tombstoned validators should not be slashed multiple times. if k.slashingKeeper.IsTombstoned(ctx, consAddr) { - // Drop packet if validator is tombstoned. + // Log and drop packet if validator is tombstoned. + k.Logger(ctx).Info( + "slash packet dropped because validator is already tombstoned", + "validator cons addr", consAddr, + ) return } From 125f63b6860c60ecd5d64735e01e07cb8dbe3de8 Mon Sep 17 00:00:00 2001 From: Shawn Marshall-Spitzbart <44221603+smarshall-spitzbart@users.noreply.github.com> Date: Tue, 6 Dec 2022 19:27:01 -0800 Subject: [PATCH 31/31] infrac comments --- x/ccv/provider/keeper/relay.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x/ccv/provider/keeper/relay.go b/x/ccv/provider/keeper/relay.go index f3d69e683a..2235941e4a 100644 --- a/x/ccv/provider/keeper/relay.go +++ b/x/ccv/provider/keeper/relay.go @@ -299,10 +299,10 @@ func (k Keeper) HandleSlashPacket(ctx sdk.Context, chainID string, data ccv.Slas slashFraction sdk.Dec ) - // Obtain infraction height, or log error and drop packet infractionHeight, found := k.getMappedInfractionHeight(ctx, chainID, data.ValsetUpdateId) if !found { k.Logger(ctx).Error("infraction height not found. But was found during slash packet validation") + // drop packet return }