From df3bb77ce6b0cac8d363a6d98990a2e28536808d Mon Sep 17 00:00:00 2001 From: omer mishael Date: Sun, 16 Apr 2023 10:55:05 +0300 Subject: [PATCH 1/3] do not jail if not enough providers exist --- x/pairing/keeper/unresponsive_provider.go | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/x/pairing/keeper/unresponsive_provider.go b/x/pairing/keeper/unresponsive_provider.go index 8fc1a97b37..e45c76d678 100644 --- a/x/pairing/keeper/unresponsive_provider.go +++ b/x/pairing/keeper/unresponsive_provider.go @@ -52,9 +52,21 @@ func (k Keeper) UnstakeUnresponsiveProviders(ctx sdk.Context, epochsNumToCheckCU return nil } + // TODO: when we use the policy providers number, this should be updated + minimumProvidersCount, err := k.ServicersToPairCount(ctx, uint64(ctx.BlockHeight())) + if err != nil { + return utils.LavaError(ctx, k.Logger(ctx), "param_reading", map[string]string{"err": err.Error()}, "couldn't read k.ServicersToPairCount(ctx, uint64(ctx.BlockHeight()))") + } + // Go over the staked provider entries (on all chains) for _, providerStakeStorage := range providerStakeStorageList { - for _, providerStakeEntry := range providerStakeStorage.GetStakeEntries() { + providerStakeEntriesForChain := providerStakeStorage.GetStakeEntries() + existingProviders := uint64(len(providerStakeEntriesForChain)) + for _, providerStakeEntry := range providerStakeEntriesForChain { + if existingProviders <= minimumProvidersCount { + // not enough providers, skip jailing any more providers + break + } if minHistoryBlock < providerStakeEntry.StakeAppliedBlock { // this staked provider has too short history (either since staking // or since it was last unfrozen) - do not consider for jailing @@ -69,6 +81,7 @@ func (k Keeper) UnstakeUnresponsiveProviders(ctx sdk.Context, epochsNumToCheckCU // providerPaymentStorageKeyList is not empty -> provider should be punished if len(providerPaymentStorageKeyList) != 0 { err = k.punishUnresponsiveProvider(ctx, minPaymentBlock, providerPaymentStorageKeyList, providerStakeEntry.GetAddress(), providerStakeEntry.GetChain()) + existingProviders-- if err != nil { return utils.LavaError(ctx, k.Logger(ctx), "punish_unresponsive_provider", map[string]string{"err": err.Error()}, "couldn't punish unresponsive provider") } From b61c9bf78102948ffef645719dff3baaf96f6c30 Mon Sep 17 00:00:00 2001 From: omer mishael Date: Sun, 16 Apr 2023 13:01:56 +0300 Subject: [PATCH 2/3] fix unitest --- .../keeper/unresponsive_provider_test.go | 100 ++++++++++-------- 1 file changed, 55 insertions(+), 45 deletions(-) diff --git a/x/pairing/keeper/unresponsive_provider_test.go b/x/pairing/keeper/unresponsive_provider_test.go index 940cfd300d..c21daca865 100644 --- a/x/pairing/keeper/unresponsive_provider_test.go +++ b/x/pairing/keeper/unresponsive_provider_test.go @@ -112,8 +112,8 @@ func TestUnresponsivenessStressTest(t *testing.T) { // Test to measure the time the check for unresponsiveness every epoch start takes func TestUnstakingProviderForUnresponsiveness(t *testing.T) { // setup test for unresponsiveness - testClientAmount := 4 - testProviderAmount := 2 + testClientAmount := 1 + testProviderAmount := 10 ts := setupClientsAndProvidersForUnresponsiveness(t, testClientAmount, testProviderAmount) // get recommendedEpochNumToCollectPayment @@ -130,12 +130,18 @@ func TestUnstakingProviderForUnresponsiveness(t *testing.T) { ts.ctx = testkeeper.AdvanceEpoch(ts.ctx, ts.keepers) } + // find two providers in the pairing + pairingProviders, err := ts.keepers.Pairing.GetPairingForClient(sdk.UnwrapSDKContext(ts.ctx), ts.spec.Name, ts.clients[0].Addr) + require.NoError(t, err) + provider0_addr := sdk.MustAccAddressFromBech32(pairingProviders[0].Address) + provider1_addr := sdk.MustAccAddressFromBech32(pairingProviders[1].Address) + // get provider1's balance before the stake - staked_amount, _, _ := ts.keepers.Epochstorage.GetStakeEntryByAddressCurrent(sdk.UnwrapSDKContext(ts.ctx), epochstoragetypes.ProviderKey, ts.spec.Name, ts.providers[1].Addr) - balanceProvideratBeforeStake := staked_amount.Stake.Amount.Int64() + ts.keepers.BankKeeper.GetBalance(sdk.UnwrapSDKContext(ts.ctx), ts.providers[1].Addr, epochstoragetypes.TokenDenom).Amount.Int64() + staked_amount, _, _ := ts.keepers.Epochstorage.GetStakeEntryByAddressCurrent(sdk.UnwrapSDKContext(ts.ctx), epochstoragetypes.ProviderKey, ts.spec.Name, provider1_addr) + balanceProvideratBeforeStake := staked_amount.Stake.Amount.Int64() + ts.keepers.BankKeeper.GetBalance(sdk.UnwrapSDKContext(ts.ctx), provider1_addr, epochstoragetypes.TokenDenom).Amount.Int64() // create unresponsive data that includes provider1 being unresponsive - unresponsiveProvidersData, err := json.Marshal([]string{ts.providers[1].Addr.String()}) + unresponsiveProvidersData, err := json.Marshal([]string{provider1_addr.String()}) require.Nil(t, err) // create relay requests for provider0 that contain complaints about provider1 @@ -143,7 +149,7 @@ func TestUnstakingProviderForUnresponsiveness(t *testing.T) { for clientIndex := 0; clientIndex < testClientAmount; clientIndex++ { // testing testClientAmount of complaints var Relays []*types.RelaySession relayRequest := &types.RelaySession{ - Provider: ts.providers[0].Addr.String(), + Provider: provider0_addr.String(), ContentHash: []byte(ts.spec.Apis[0].Name), SessionId: uint64(0), SpecId: ts.spec.Name, @@ -159,7 +165,7 @@ func TestUnstakingProviderForUnresponsiveness(t *testing.T) { Relays = append(Relays, relayRequest) // send relay payment and check the funds did transfer normally - payAndVerifyBalance(t, ts, types.MsgRelayPayment{Creator: ts.providers[0].Addr.String(), Relays: Relays}, true, ts.clients[clientIndex].Addr, ts.providers[0].Addr) + payAndVerifyBalance(t, ts, types.MsgRelayPayment{Creator: provider0_addr.String(), Relays: Relays}, true, ts.clients[clientIndex].Addr, provider0_addr) } // advance enough epochs so the unresponsive provider will be punished @@ -171,21 +177,21 @@ func TestUnstakingProviderForUnresponsiveness(t *testing.T) { } // test the unresponsive provider1 has been unstaked - _, unstakeStoragefound, _ := ts.keepers.Epochstorage.UnstakeEntryByAddress(sdk.UnwrapSDKContext(ts.ctx), epochstoragetypes.ProviderKey, ts.providers[1].Addr) + _, unstakeStoragefound, _ := ts.keepers.Epochstorage.UnstakeEntryByAddress(sdk.UnwrapSDKContext(ts.ctx), epochstoragetypes.ProviderKey, provider1_addr) require.True(t, unstakeStoragefound) - _, stakeStorageFound, _ := ts.keepers.Epochstorage.GetStakeEntryByAddressCurrent(sdk.UnwrapSDKContext(ts.ctx), epochstoragetypes.ProviderKey, ts.spec.Name, ts.providers[1].Addr) + _, stakeStorageFound, _ := ts.keepers.Epochstorage.GetStakeEntryByAddressCurrent(sdk.UnwrapSDKContext(ts.ctx), epochstoragetypes.ProviderKey, ts.spec.Name, provider1_addr) require.False(t, stakeStorageFound) // validate the complainers CU field in provider1's providerPaymentStorage has been reset after being punished (note we use the epoch from the relay because that is when it got reported) - providerPaymentStorageKey := ts.keepers.Pairing.GetProviderPaymentStorageKey(sdk.UnwrapSDKContext(ts.ctx), ts.spec.Name, uint64(relayEpoch), ts.providers[1].Addr) + providerPaymentStorageKey := ts.keepers.Pairing.GetProviderPaymentStorageKey(sdk.UnwrapSDKContext(ts.ctx), ts.spec.Name, uint64(relayEpoch), provider1_addr) providerPaymentStorage, found := ts.keepers.Pairing.GetProviderPaymentStorage(sdk.UnwrapSDKContext(ts.ctx), providerPaymentStorageKey) require.Equal(t, true, found) require.Equal(t, uint64(0), providerPaymentStorage.GetComplainersTotalCu()) // test the responsive provider0 hasn't been unstaked - _, unstakeStoragefound, _ = ts.keepers.Epochstorage.UnstakeEntryByAddress(sdk.UnwrapSDKContext(ts.ctx), epochstoragetypes.ProviderKey, ts.providers[0].Addr) + _, unstakeStoragefound, _ = ts.keepers.Epochstorage.UnstakeEntryByAddress(sdk.UnwrapSDKContext(ts.ctx), epochstoragetypes.ProviderKey, provider0_addr) require.False(t, unstakeStoragefound) - _, stakeStorageFound, _ = ts.keepers.Epochstorage.GetStakeEntryByAddressCurrent(sdk.UnwrapSDKContext(ts.ctx), epochstoragetypes.ProviderKey, ts.spec.Name, ts.providers[0].Addr) + _, stakeStorageFound, _ = ts.keepers.Epochstorage.GetStakeEntryByAddressCurrent(sdk.UnwrapSDKContext(ts.ctx), epochstoragetypes.ProviderKey, ts.spec.Name, provider0_addr) require.True(t, stakeStorageFound) // advance enough epochs so the current block will be deleted (advance more than the chain's memory - blocksToSave) @@ -201,22 +207,22 @@ func TestUnstakingProviderForUnresponsiveness(t *testing.T) { } // validate that the provider is no longer unstaked - _, unstakeStoragefound, _ = ts.keepers.Epochstorage.UnstakeEntryByAddress(sdk.UnwrapSDKContext(ts.ctx), epochstoragetypes.ProviderKey, ts.providers[1].Addr) + _, unstakeStoragefound, _ = ts.keepers.Epochstorage.UnstakeEntryByAddress(sdk.UnwrapSDKContext(ts.ctx), epochstoragetypes.ProviderKey, provider1_addr) require.False(t, unstakeStoragefound) // also validate that the provider hasn't returned to the stake pool - _, stakeStorageFound, _ = ts.keepers.Epochstorage.GetStakeEntryByAddressCurrent(sdk.UnwrapSDKContext(ts.ctx), epochstoragetypes.ProviderKey, ts.spec.Name, ts.providers[1].Addr) + _, stakeStorageFound, _ = ts.keepers.Epochstorage.GetStakeEntryByAddressCurrent(sdk.UnwrapSDKContext(ts.ctx), epochstoragetypes.ProviderKey, ts.spec.Name, provider1_addr) require.False(t, stakeStorageFound) // validate that the provider's balance after the unstake is the same as before he staked - balanceProviderAfterUnstakeMoneyReturned := ts.keepers.BankKeeper.GetBalance(sdk.UnwrapSDKContext(ts.ctx), ts.providers[1].Addr, epochstoragetypes.TokenDenom).Amount.Int64() + balanceProviderAfterUnstakeMoneyReturned := ts.keepers.BankKeeper.GetBalance(sdk.UnwrapSDKContext(ts.ctx), provider1_addr, epochstoragetypes.TokenDenom).Amount.Int64() require.Equal(t, balanceProvideratBeforeStake, balanceProviderAfterUnstakeMoneyReturned) } func TestUnstakingProviderForUnresponsivenessContinueComplainingAfterUnstake(t *testing.T) { // setup test for unresponsiveness - testClientAmount := 4 - testProviderAmount := 2 + testClientAmount := 1 + testProviderAmount := 3 ts := setupClientsAndProvidersForUnresponsiveness(t, testClientAmount, testProviderAmount) // get recommendedEpochNumToCollectPayment @@ -233,34 +239,38 @@ func TestUnstakingProviderForUnresponsivenessContinueComplainingAfterUnstake(t * ts.ctx = testkeeper.AdvanceEpoch(ts.ctx, ts.keepers) } + // find two providers in the pairing + pairingProviders, err := ts.keepers.Pairing.GetPairingForClient(sdk.UnwrapSDKContext(ts.ctx), ts.spec.Name, ts.clients[0].Addr) + require.NoError(t, err) + provider0_addr := sdk.MustAccAddressFromBech32(pairingProviders[0].Address) + provider1_addr := sdk.MustAccAddressFromBech32(pairingProviders[1].Address) // create unresponsive data that includes provider1 being unresponsive - unresponsiveProvidersData, err := json.Marshal([]string{ts.providers[1].Addr.String()}) + unresponsiveProvidersData, err := json.Marshal([]string{provider1_addr.String()}) require.Nil(t, err) // create relay requests for provider0 that contain complaints about provider1 relayEpoch := sdk.UnwrapSDKContext(ts.ctx).BlockHeight() - for clientIndex := 0; clientIndex < testClientAmount; clientIndex++ { // testing testClientAmount of complaints - var Relays []*types.RelaySession - relayRequest := &types.RelaySession{ - Provider: ts.providers[0].Addr.String(), - ContentHash: []byte(ts.spec.Apis[0].Name), - SessionId: uint64(0), - SpecId: ts.spec.Name, - CuSum: ts.spec.Apis[0].ComputeUnits * 10, - Epoch: relayEpoch, - RelayNum: 0, - UnresponsiveProviders: unresponsiveProvidersData, // create the complaint - } - - sig, err := sigs.SignRelay(ts.clients[clientIndex].SK, *relayRequest) - relayRequest.Sig = sig - require.Nil(t, err) - Relays = append(Relays, relayRequest) - // send relay payment and check the funds did transfer normally - payAndVerifyBalance(t, ts, types.MsgRelayPayment{Creator: ts.providers[0].Addr.String(), Relays: Relays}, true, ts.clients[clientIndex].Addr, ts.providers[0].Addr) + var Relays []*types.RelaySession + relayRequest := &types.RelaySession{ + Provider: provider0_addr.String(), + ContentHash: []byte(ts.spec.Apis[0].Name), + SessionId: uint64(0), + SpecId: ts.spec.Name, + CuSum: ts.spec.Apis[0].ComputeUnits * 10, + Epoch: relayEpoch, + RelayNum: 0, + UnresponsiveProviders: unresponsiveProvidersData, // create the complaint } + sig, err := sigs.SignRelay(ts.clients[0].SK, *relayRequest) + relayRequest.Sig = sig + require.Nil(t, err) + Relays = append(Relays, relayRequest) + + // send relay payment and check the funds did transfer normally + payAndVerifyBalance(t, ts, types.MsgRelayPayment{Creator: provider0_addr.String(), Relays: Relays}, true, ts.clients[0].Addr, provider0_addr) + // advance enough epochs so the unresponsive provider will be punished if largerConst < recommendedEpochNumToCollectPayment { largerConst = recommendedEpochNumToCollectPayment @@ -270,13 +280,13 @@ func TestUnstakingProviderForUnresponsivenessContinueComplainingAfterUnstake(t * } // test the provider has been unstaked - _, unStakeStoragefound, _ := ts.keepers.Epochstorage.UnstakeEntryByAddress(sdk.UnwrapSDKContext(ts.ctx), epochstoragetypes.ProviderKey, ts.providers[1].Addr) + _, unStakeStoragefound, _ := ts.keepers.Epochstorage.UnstakeEntryByAddress(sdk.UnwrapSDKContext(ts.ctx), epochstoragetypes.ProviderKey, provider1_addr) require.True(t, unStakeStoragefound) - _, stakeStorageFound, _ := ts.keepers.Epochstorage.GetStakeEntryByAddressCurrent(sdk.UnwrapSDKContext(ts.ctx), epochstoragetypes.ProviderKey, ts.spec.Name, ts.providers[1].Addr) + _, stakeStorageFound, _ := ts.keepers.Epochstorage.GetStakeEntryByAddressCurrent(sdk.UnwrapSDKContext(ts.ctx), epochstoragetypes.ProviderKey, ts.spec.Name, provider1_addr) require.False(t, stakeStorageFound) // validate the complainers CU field in provider1's providerPaymentStorage has been reset after being punished (note we use the epoch from the relay because that is when it got reported) - providerPaymentStorageKey := ts.keepers.Pairing.GetProviderPaymentStorageKey(sdk.UnwrapSDKContext(ts.ctx), ts.spec.Name, uint64(relayEpoch), ts.providers[1].Addr) + providerPaymentStorageKey := ts.keepers.Pairing.GetProviderPaymentStorageKey(sdk.UnwrapSDKContext(ts.ctx), ts.spec.Name, uint64(relayEpoch), provider1_addr) providerPaymentStorage, found := ts.keepers.Pairing.GetProviderPaymentStorage(sdk.UnwrapSDKContext(ts.ctx), providerPaymentStorageKey) require.Equal(t, true, found) require.Equal(t, uint64(0), providerPaymentStorage.GetComplainersTotalCu()) @@ -290,7 +300,7 @@ func TestUnstakingProviderForUnresponsivenessContinueComplainingAfterUnstake(t * for clientIndex := 0; clientIndex < testClientAmount; clientIndex++ { // testing testClientAmount of complaints var RelaysAfter []*types.RelaySession relayRequest := &types.RelaySession{ - Provider: ts.providers[0].Addr.String(), + Provider: provider0_addr.String(), ContentHash: []byte(ts.spec.Apis[0].Name), SessionId: uint64(2), SpecId: ts.spec.Name, @@ -305,13 +315,13 @@ func TestUnstakingProviderForUnresponsivenessContinueComplainingAfterUnstake(t * RelaysAfter = append(RelaysAfter, relayRequest) // send relay payment and check the funds did transfer normally - payAndVerifyBalance(t, ts, types.MsgRelayPayment{Creator: ts.providers[0].Addr.String(), Relays: RelaysAfter}, true, ts.clients[clientIndex].Addr, ts.providers[0].Addr) + payAndVerifyBalance(t, ts, types.MsgRelayPayment{Creator: provider0_addr.String(), Relays: RelaysAfter}, true, ts.clients[clientIndex].Addr, provider0_addr) } // test the provider is still unstaked - _, stakeStorageFound, _ = ts.keepers.Epochstorage.GetStakeEntryByAddressCurrent(sdk.UnwrapSDKContext(ts.ctx), epochstoragetypes.ProviderKey, ts.spec.Name, ts.providers[1].Addr) + _, stakeStorageFound, _ = ts.keepers.Epochstorage.GetStakeEntryByAddressCurrent(sdk.UnwrapSDKContext(ts.ctx), epochstoragetypes.ProviderKey, ts.spec.Name, provider1_addr) require.False(t, stakeStorageFound) - _, unStakeStoragefound, _ = ts.keepers.Epochstorage.UnstakeEntryByAddress(sdk.UnwrapSDKContext(ts.ctx), epochstoragetypes.ProviderKey, ts.providers[1].Addr) + _, unStakeStoragefound, _ = ts.keepers.Epochstorage.UnstakeEntryByAddress(sdk.UnwrapSDKContext(ts.ctx), epochstoragetypes.ProviderKey, provider1_addr) require.True(t, unStakeStoragefound) // get the current unstake storage @@ -321,7 +331,7 @@ func TestUnstakingProviderForUnresponsivenessContinueComplainingAfterUnstake(t * // validate the punished provider is not shown twice (or more) in the unstake storage var numberOfAppearances int for _, stored := range storage.StakeEntries { - if stored.Address == ts.providers[1].Addr.String() { + if stored.Address == provider1_addr.String() { numberOfAppearances += 1 } } From d77ca8a9b700e95a9c164308ab05e714f39f42cf Mon Sep 17 00:00:00 2001 From: omer mishael Date: Sun, 16 Apr 2023 13:04:00 +0300 Subject: [PATCH 3/3] add unitest for new protection --- .../keeper/unresponsive_provider_test.go | 70 +++++++++++++++++++ 1 file changed, 70 insertions(+) diff --git a/x/pairing/keeper/unresponsive_provider_test.go b/x/pairing/keeper/unresponsive_provider_test.go index c21daca865..b83fc11a01 100644 --- a/x/pairing/keeper/unresponsive_provider_test.go +++ b/x/pairing/keeper/unresponsive_provider_test.go @@ -337,3 +337,73 @@ func TestUnstakingProviderForUnresponsivenessContinueComplainingAfterUnstake(t * } require.Equal(t, numberOfAppearances, 1) } + +func TestNotUnstakingProviderForUnresponsivenessWithMinProviders(t *testing.T) { + // setup test for unresponsiveness + testClientAmount := 1 + testProviderAmount := 2 + ts := setupClientsAndProvidersForUnresponsiveness(t, testClientAmount, testProviderAmount) + + // get recommendedEpochNumToCollectPayment + recommendedEpochNumToCollectPayment := ts.keepers.Pairing.RecommendedEpochNumToCollectPayment(sdk.UnwrapSDKContext(ts.ctx)) + + // check which const is larger + largerConst := pairing.EPOCHS_NUM_TO_CHECK_CU_FOR_UNRESPONSIVE_PROVIDER + if largerConst < pairing.EPOCHS_NUM_TO_CHECK_FOR_COMPLAINERS { + largerConst = pairing.EPOCHS_NUM_TO_CHECK_FOR_COMPLAINERS + } + + // advance enough epochs so we can check punishment due to unresponsiveness (if the epoch is too early, there's no punishment) + for i := uint64(0); i < uint64(largerConst)+recommendedEpochNumToCollectPayment; i++ { + ts.ctx = testkeeper.AdvanceEpoch(ts.ctx, ts.keepers) + } + + // find two providers in the pairing + pairingProviders, err := ts.keepers.Pairing.GetPairingForClient(sdk.UnwrapSDKContext(ts.ctx), ts.spec.Name, ts.clients[0].Addr) + require.NoError(t, err) + provider0_addr := sdk.MustAccAddressFromBech32(pairingProviders[0].Address) + provider1_addr := sdk.MustAccAddressFromBech32(pairingProviders[1].Address) + + // create unresponsive data that includes provider1 being unresponsive + unresponsiveProvidersData, err := json.Marshal([]string{provider1_addr.String()}) + require.Nil(t, err) + + // create relay requests for provider0 that contain complaints about provider1 + relayEpoch := sdk.UnwrapSDKContext(ts.ctx).BlockHeight() + for clientIndex := 0; clientIndex < testClientAmount; clientIndex++ { // testing testClientAmount of complaints + var Relays []*types.RelaySession + relayRequest := &types.RelaySession{ + Provider: provider0_addr.String(), + ContentHash: []byte(ts.spec.Apis[0].Name), + SessionId: uint64(0), + SpecId: ts.spec.Name, + CuSum: ts.spec.Apis[0].ComputeUnits*10 + uint64(clientIndex), + Epoch: relayEpoch, + RelayNum: 0, + UnresponsiveProviders: unresponsiveProvidersData, // create the complaint + } + + sig, err := sigs.SignRelay(ts.clients[clientIndex].SK, *relayRequest) + relayRequest.Sig = sig + require.Nil(t, err) + Relays = append(Relays, relayRequest) + + // send relay payment and check the funds did transfer normally + payAndVerifyBalance(t, ts, types.MsgRelayPayment{Creator: provider0_addr.String(), Relays: Relays}, true, ts.clients[clientIndex].Addr, provider0_addr) + } + + // advance enough epochs so the unresponsive provider will be punished + if largerConst < recommendedEpochNumToCollectPayment { + largerConst = recommendedEpochNumToCollectPayment + } + for i := uint64(0); i < largerConst; i++ { + ts.ctx = testkeeper.AdvanceEpoch(ts.ctx, ts.keepers) + } + + // test the unresponsive provider1 has been unstaked + _, unstakeStoragefound, _ := ts.keepers.Epochstorage.UnstakeEntryByAddress(sdk.UnwrapSDKContext(ts.ctx), epochstoragetypes.ProviderKey, provider1_addr) + require.False(t, unstakeStoragefound) + _, stakeStorageFound, _ := ts.keepers.Epochstorage.GetStakeEntryByAddressCurrent(sdk.UnwrapSDKContext(ts.ctx), epochstoragetypes.ProviderKey, ts.spec.Name, provider1_addr) + require.True(t, stakeStorageFound) + +}