From 3f06d2431773288bf88ece85dd5e2fedce8fbe24 Mon Sep 17 00:00:00 2001 From: Oren Date: Tue, 3 Sep 2024 18:13:20 +0300 Subject: [PATCH 1/6] nilaway fixes --- x/conflict/keeper/conflict.go | 12 +++++++++++ x/conflict/keeper/msg_server_detection.go | 8 ++++++++ x/conflict/types/conflict.go | 24 ++++++++++++++++++++++ x/pairing/keeper/pairing.go | 2 +- x/pairing/keeper/pairing_test.go | 9 ++++---- x/pairing/keeper/scores/geo_req.go | 25 +++++++++++++++-------- x/pairing/keeper/scores/pairing_slot.go | 2 +- x/pairing/keeper/scores/qos_req.go | 20 +++++++++++------- x/pairing/keeper/scores/stake_req.go | 8 +++++++- x/projects/keeper/creation.go | 5 ++--- x/projects/types/project.go | 3 +++ x/spec/keeper/spec.go | 6 ++++++ x/spec/types/api_collection.go | 3 +++ x/spec/types/combinable.go | 2 +- x/spec/types/spec.go | 5 ++++- x/subscription/keeper/subscription.go | 10 +++++++++ 16 files changed, 116 insertions(+), 28 deletions(-) create mode 100644 x/conflict/types/conflict.go diff --git a/x/conflict/keeper/conflict.go b/x/conflict/keeper/conflict.go index a3e6e31a1a..b1f5c19ae9 100644 --- a/x/conflict/keeper/conflict.go +++ b/x/conflict/keeper/conflict.go @@ -20,6 +20,11 @@ func (k Keeper) ValidateFinalizationConflict(ctx sdk.Context, conflictData *type } func (k Keeper) ValidateResponseConflict(ctx sdk.Context, conflictData *types.ResponseConflict, clientAddr sdk.AccAddress) error { + // 0. validate conflictData is not nil + if conflictData.IsDataNil() { + return fmt.Errorf("ValidateResponseConflict: conflict data is nil") + } + // 1. validate mismatching data chainID := conflictData.ConflictRelayData0.Request.RelaySession.SpecId if chainID != conflictData.ConflictRelayData1.Request.RelaySession.SpecId { @@ -279,6 +284,10 @@ func (k Keeper) ValidateSameProviderConflict(ctx sdk.Context, conflictData *type func (k Keeper) validateBlockHeights(relayFinalization *types.RelayFinalization, spec *spectypes.Spec) (finalizedBlocksMarshalled map[int64]string, earliestFinalizedBlock int64, latestFinalizedBlock int64, err error) { EMPTY_MAP := map[int64]string{} + // verify spec is not nil + if spec == nil { + return EMPTY_MAP, 0, 0, fmt.Errorf("validateBlockHeights: spec is nil") + } // Unmarshall finalized blocks finalizedBlocks := map[int64]string{} @@ -312,6 +321,9 @@ func (k Keeper) validateBlockHeights(relayFinalization *types.RelayFinalization, } func (k Keeper) validateFinalizedBlock(relayFinalization *types.RelayFinalization, latestFinalizedBlock int64, spec *spectypes.Spec) error { + if spec == nil { + return fmt.Errorf("validateFinalizedBlock: spec is nil") + } latestBlock := relayFinalization.GetLatestBlock() blockDistanceToFinalization := int64(spec.BlockDistanceForFinalizedData) diff --git a/x/conflict/keeper/msg_server_detection.go b/x/conflict/keeper/msg_server_detection.go index e9d262062d..ea1e30fe53 100644 --- a/x/conflict/keeper/msg_server_detection.go +++ b/x/conflict/keeper/msg_server_detection.go @@ -15,6 +15,9 @@ import ( ) func DetectionIndex(creatorAddr string, conflict *types.ResponseConflict, epochStart uint64) string { + if conflict.IsDataNil() { + return "" + } return creatorAddr + conflict.ConflictRelayData0.Request.RelaySession.Provider + conflict.ConflictRelayData1.Request.RelaySession.Provider + strconv.FormatUint(epochStart, 10) } @@ -125,6 +128,11 @@ func (k msgServer) handleSameProviderFinalizationConflict(ctx sdk.Context, confl } func (k msgServer) handleResponseConflict(ctx sdk.Context, goCtx context.Context, conflict *types.ResponseConflict, clientAddr sdk.AccAddress) (eventData map[string]string, err error) { + if conflict.IsDataNil() { + return nil, utils.LavaFormatWarning("conflict data is nil", fmt.Errorf("handleResponseConflict: cannot handle response conflict"), + utils.LogAttr("client", clientAddr.String()), + ) + } err = k.Keeper.ValidateResponseConflict(ctx, conflict, clientAddr) if err != nil { return nil, utils.LavaFormatWarning("Simulation: invalid response conflict detection", err, diff --git a/x/conflict/types/conflict.go b/x/conflict/types/conflict.go new file mode 100644 index 0000000000..3b9a7c72bb --- /dev/null +++ b/x/conflict/types/conflict.go @@ -0,0 +1,24 @@ +package types + +func (c *ResponseConflict) IsDataNil() bool { + if c == nil { + return true + } + if c.ConflictRelayData0 == nil || c.ConflictRelayData1 == nil { + return true + } + if c.ConflictRelayData0.Request == nil || c.ConflictRelayData1.Request == nil { + return true + } + if c.ConflictRelayData0.Request.RelayData == nil || c.ConflictRelayData1.Request.RelayData == nil { + return true + } + if c.ConflictRelayData0.Request.RelaySession == nil || c.ConflictRelayData1.Request.RelaySession == nil { + return true + } + if c.ConflictRelayData0.Reply == nil || c.ConflictRelayData1.Reply == nil { + return true + } + + return false +} diff --git a/x/pairing/keeper/pairing.go b/x/pairing/keeper/pairing.go index 023eae047d..cc2cd2705e 100644 --- a/x/pairing/keeper/pairing.go +++ b/x/pairing/keeper/pairing.go @@ -363,7 +363,7 @@ func (k Keeper) ValidatePairingForClient(ctx sdk.Context, chainID string, provid utils.LavaFormatPanic("critical: invalid provider address for payment", err, utils.Attribute{Key: "chainID", Value: chainID}, utils.Attribute{Key: "client", Value: project.Subscription}, - utils.Attribute{Key: "provider", Value: providerAccAddr.String()}, + utils.Attribute{Key: "provider", Value: possibleAddr.Address}, utils.Attribute{Key: "epochBlock", Value: strconv.FormatUint(epoch, 10)}, ) } diff --git a/x/pairing/keeper/pairing_test.go b/x/pairing/keeper/pairing_test.go index f1e607aff0..6540f2881e 100644 --- a/x/pairing/keeper/pairing_test.go +++ b/x/pairing/keeper/pairing_test.go @@ -1203,7 +1203,7 @@ func verifyGeoScoreForTesting(providerScores []*pairingscores.PairingScore, slot }) geoReqObject := pairingscores.GeoReq{} - geoReq, ok := slot.Reqs[geoReqObject.GetName()].(pairingscores.GeoReq) + geoReq, ok := slot.Reqs[geoReqObject.GetName()].(*pairingscores.GeoReq) if !ok { return false } @@ -1331,7 +1331,8 @@ func TestNoRequiredGeo(t *testing.T) { // TestGeoSlotCalc checks that the calculated slots always hold a single bit geo req func TestGeoSlotCalc(t *testing.T) { - geoReqName := pairingscores.GeoReq{}.GetName() + geoReq := pairingscores.GeoReq{} + geoReqName := geoReq.GetName() allGeos := planstypes.GetAllGeolocations() maxGeo := lavaslices.Max(allGeos) @@ -1347,7 +1348,7 @@ func TestGeoSlotCalc(t *testing.T) { slots := pairingscores.CalcSlots(&policy) for _, slot := range slots { geoReqFromMap := slot.Reqs[geoReqName] - geoReq, ok := geoReqFromMap.(pairingscores.GeoReq) + geoReq, ok := geoReqFromMap.(*pairingscores.GeoReq) if !ok { require.Fail(t, "slot geo req is not of GeoReq type") } @@ -1366,7 +1367,7 @@ func TestGeoSlotCalc(t *testing.T) { slots := pairingscores.CalcSlots(&policy) for _, slot := range slots { geoReqFromMap := slot.Reqs[geoReqName] - geoReq, ok := geoReqFromMap.(pairingscores.GeoReq) + geoReq, ok := geoReqFromMap.(*pairingscores.GeoReq) if !ok { require.Fail(t, "slot geo req is not of GeoReq type") } diff --git a/x/pairing/keeper/scores/geo_req.go b/x/pairing/keeper/scores/geo_req.go index b658cd3781..4ed5e62960 100644 --- a/x/pairing/keeper/scores/geo_req.go +++ b/x/pairing/keeper/scores/geo_req.go @@ -20,13 +20,17 @@ const ( minGeoLatency = 1 ) -func (gr GeoReq) Init(policy planstypes.Policy) bool { - return true +func (gr *GeoReq) Init(policy planstypes.Policy) bool { + return gr != nil } // Score calculates the geo score of a provider based on preset latency data // Note: each GeoReq must have exactly a single geolocation (bit) -func (gr GeoReq) Score(score PairingScore) math.Uint { +func (gr *GeoReq) Score(score PairingScore) math.Uint { + if gr == nil { + return calculateCostFromLatency(maxGeoLatency) + } + // check if the provider supports the required geolocation if gr.Geo&^score.Provider.Geolocation == 0 { return calculateCostFromLatency(minGeoLatency) @@ -38,13 +42,16 @@ func (gr GeoReq) Score(score PairingScore) math.Uint { return cost } -func (gr GeoReq) GetName() string { +func (gr *GeoReq) GetName() string { + if gr == nil { + return "" + } return geoReqName } // Equal() used to compare slots to determine slot groups -func (gr GeoReq) Equal(other ScoreReq) bool { - otherGeoReq, ok := other.(GeoReq) +func (gr *GeoReq) Equal(other ScoreReq) bool { + otherGeoReq, ok := other.(*GeoReq) if !ok { return false } @@ -54,17 +61,17 @@ func (gr GeoReq) Equal(other ScoreReq) bool { // TODO: this function doesn't return the optimal geo reqs for the case // that there are more required geos than providers to pair -func (gr GeoReq) GetReqForSlot(policy planstypes.Policy, slotIdx int) ScoreReq { +func (gr *GeoReq) GetReqForSlot(policy planstypes.Policy, slotIdx int) ScoreReq { policyGeoEnums := planstypes.GetGeolocationsFromUint(policy.GeolocationProfile) if len(policyGeoEnums) == 0 { utils.LavaFormatError("length of policyGeoEnums is zero", fmt.Errorf("critical: Attempt to divide by zero"), utils.LogAttr("policyGeoProfile", policy.GeolocationProfile), ) - return GeoReq{Geo: int32(planstypes.Geolocation_USC)} + return &GeoReq{Geo: int32(planstypes.Geolocation_USC)} } - return GeoReq{Geo: int32(policyGeoEnums[slotIdx%len(policyGeoEnums)])} + return &GeoReq{Geo: int32(policyGeoEnums[slotIdx%len(policyGeoEnums)])} } // CalcGeoCost() finds the minimal latency between the required geo and the provider's supported geolocations diff --git a/x/pairing/keeper/scores/pairing_slot.go b/x/pairing/keeper/scores/pairing_slot.go index 5d6935f56e..fe13eaff63 100644 --- a/x/pairing/keeper/scores/pairing_slot.go +++ b/x/pairing/keeper/scores/pairing_slot.go @@ -36,7 +36,7 @@ func (psg PairingSlotGroup) Subtract(other *PairingSlotGroup) *PairingSlot { otherReq, found := other.Reqs[key] if !found { reqsDiff[key] = req - } else if !req.Equal(otherReq) { + } else if req != nil && !req.Equal(otherReq) { reqsDiff[key] = req } } diff --git a/x/pairing/keeper/scores/qos_req.go b/x/pairing/keeper/scores/qos_req.go index 38d9a6f22c..6d20dd2802 100644 --- a/x/pairing/keeper/scores/qos_req.go +++ b/x/pairing/keeper/scores/qos_req.go @@ -16,12 +16,12 @@ type QosGetter interface { // QosReq implements the ScoreReq interface for provider staking requirement(s) type QosReq struct{} -func (qr QosReq) Init(policy planstypes.Policy) bool { - return true +func (qr *QosReq) Init(policy planstypes.Policy) bool { + return qr != nil } // Score calculates the the provider's qos score -func (qr QosReq) Score(score PairingScore) math.Uint { +func (qr *QosReq) Score(score PairingScore) math.Uint { // TODO: update Qos in providerQosFS properly and uncomment this code below // Also, the qos score should range between 0.5-2 @@ -31,19 +31,25 @@ func (qr QosReq) Score(score PairingScore) math.Uint { // } // return math.Uint(qosScore) + if qr == nil { + return math.NewUint(1) + } return math.NewUint(1) } -func (qr QosReq) GetName() string { +func (qr *QosReq) GetName() string { + if qr == nil { + return "" + } return qosReqName } // Equal used to compare slots to determine slot groups. // Equal always returns true (there are no different "types" of qos) -func (qr QosReq) Equal(other ScoreReq) bool { - return true +func (qr *QosReq) Equal(other ScoreReq) bool { + return qr != nil } -func (qr QosReq) GetReqForSlot(policy planstypes.Policy, slotIdx int) ScoreReq { +func (qr *QosReq) GetReqForSlot(policy planstypes.Policy, slotIdx int) ScoreReq { return qr } diff --git a/x/pairing/keeper/scores/stake_req.go b/x/pairing/keeper/scores/stake_req.go index d118c90a5b..fb30a2f9e9 100644 --- a/x/pairing/keeper/scores/stake_req.go +++ b/x/pairing/keeper/scores/stake_req.go @@ -12,11 +12,14 @@ const stakeReqName = "stake-req" type StakeReq struct{} func (sr *StakeReq) Init(policy planstypes.Policy) bool { - return true + return sr != nil } // Score calculates the the provider score as the normalized stake func (sr *StakeReq) Score(score PairingScore) math.Uint { + if sr == nil { + return math.OneUint() + } effectiveStake := score.Provider.EffectiveStake() if !effectiveStake.IsPositive() { return math.OneUint() @@ -25,6 +28,9 @@ func (sr *StakeReq) Score(score PairingScore) math.Uint { } func (sr *StakeReq) GetName() string { + if sr == nil { + return "" + } return stakeReqName } diff --git a/x/projects/keeper/creation.go b/x/projects/keeper/creation.go index 423aed7227..a3f1c75741 100644 --- a/x/projects/keeper/creation.go +++ b/x/projects/keeper/creation.go @@ -191,7 +191,7 @@ func (k Keeper) registerKey(ctx sdk.Context, key types.ProjectKey, project *type // check that the developer key is valid, and that it does not already // belong to a different project. - if found && devkeyData.ProjectID != project.Index { + if found && devkeyData.ProjectID != project.GetIndex() { return utils.LavaFormatWarning("failed to register key", fmt.Errorf("key already exists"), utils.Attribute{Key: "key", Value: key.Key}, @@ -254,10 +254,9 @@ func (k Keeper) unregisterKey(ctx sdk.Context, key types.ProjectKey, project *ty // the developer key belongs to a different project if devkeyData.ProjectID != project.GetIndex() { return utils.LavaFormatWarning("failed to unregister key", legacyerrors.ErrNotFound, - utils.Attribute{Key: "projectID", Value: project.Index}, + utils.Attribute{Key: "projectID", Value: project.GetIndex()}, utils.Attribute{Key: "key", Value: key.Key}, utils.Attribute{Key: "keyTypes", Value: key.Kinds}, - utils.Attribute{Key: "projectID", Value: project.GetIndex()}, utils.Attribute{Key: "otherID", Value: devkeyData.ProjectID}, ) } diff --git a/x/projects/types/project.go b/x/projects/types/project.go index d1f7f5e3a2..9289e67257 100644 --- a/x/projects/types/project.go +++ b/x/projects/types/project.go @@ -82,6 +82,9 @@ func (project *Project) GetKey(key string) ProjectKey { } func (project *Project) AppendKey(key ProjectKey) bool { + if project == nil { + return false + } for i, projectKey := range project.ProjectKeys { if projectKey.Key == key.Key { project.ProjectKeys[i].Kinds |= key.Kinds diff --git a/x/spec/keeper/spec.go b/x/spec/keeper/spec.go index 59c0a5e6ce..b52e77fe1a 100644 --- a/x/spec/keeper/spec.go +++ b/x/spec/keeper/spec.go @@ -107,6 +107,9 @@ func (k Keeper) RefreshSpec(ctx sdk.Context, spec types.Spec, ancestors []types. } if details, err := spec.ValidateSpec(k.MaxCU(ctx)); err != nil { + if details != nil { + details = map[string]string{} + } details["invalidates"] = spec.Index attrs := utils.StringMapToAttributes(details) return nil, utils.LavaFormatWarning("spec refresh failed (invalidate)", err, attrs...) @@ -137,6 +140,9 @@ func (k Keeper) doExpandSpec( inherit *map[string]bool, details string, ) (string, error) { + if spec == nil { + return "", fmt.Errorf("doExpandSpec: spec is nil") + } parentsCollections := map[types.CollectionData][]*types.ApiCollection{} if len(spec.Imports) != 0 { diff --git a/x/spec/types/api_collection.go b/x/spec/types/api_collection.go index 06561b16bf..7df10e64ee 100644 --- a/x/spec/types/api_collection.go +++ b/x/spec/types/api_collection.go @@ -65,6 +65,9 @@ func (apic *ApiCollection) InheritAllFields(myCollections map[CollectionData]*Ap // changes in place inside the apic // nil merge maps means not to combine that field func (apic *ApiCollection) CombineWithOthers(others []*ApiCollection, combineWithDisabled, allowOverwrite bool) (err error) { + if apic == nil { + return fmt.Errorf("CombineWithOthers: API collection is nil") + } mergedApis := map[string]interface{}{} mergedHeaders := map[string]interface{}{} mergedParsers := map[string]interface{}{} diff --git a/x/spec/types/combinable.go b/x/spec/types/combinable.go index f0c9b781a5..2ee3c5a7b8 100644 --- a/x/spec/types/combinable.go +++ b/x/spec/types/combinable.go @@ -141,7 +141,7 @@ func CombineUnique[T Combinable](appendFrom, appendTo []T, currentMap map[string } else { // overwriting the inherited field might need Overwrite actions if overwritten, isOverwritten := current.currentCombinable.Overwrite(combinable); isOverwritten { - if appendTo[current.index].Differeniator() != combinable.Differeniator() { + if len(appendTo) <= current.index || appendTo[current.index].Differeniator() != combinable.Differeniator() { return nil, fmt.Errorf("differentiator mismatch in overwrite %s vs %s", combinable.Differeniator(), appendTo[current.index].Differeniator()) } overwrittenT, ok := overwritten.(T) diff --git a/x/spec/types/spec.go b/x/spec/types/spec.go index 237feb9bf4..f41ee8fda0 100644 --- a/x/spec/types/spec.go +++ b/x/spec/types/spec.go @@ -200,6 +200,9 @@ func (spec Spec) ValidateSpec(maxCU uint64) (map[string]string, error) { } func (spec *Spec) CombineCollections(parentsCollections map[CollectionData][]*ApiCollection) error { + if spec == nil { + return fmt.Errorf("CombineCollections: spec is nil") + } collectionDataList := make([]CollectionData, 0) // Populate the keys slice with the map keys for key := range parentsCollections { @@ -225,7 +228,7 @@ func (spec *Spec) CombineCollections(parentsCollections map[CollectionData][]*Ap break } } - if !combined.Enabled { + if combined == nil || !combined.Enabled { // no collections enabled to combine, we skip this continue } diff --git a/x/subscription/keeper/subscription.go b/x/subscription/keeper/subscription.go index 7a8a04d3f9..7cde34e7ed 100644 --- a/x/subscription/keeper/subscription.go +++ b/x/subscription/keeper/subscription.go @@ -192,6 +192,9 @@ func (k Keeper) verifySubscriptionBuyInputAndGetPlan(ctx sdk.Context, block uint func (k Keeper) createNewSubscription(ctx sdk.Context, plan *planstypes.Plan, creator, consumer string, block uint64, autoRenewalFlag bool, ) (types.Subscription, error) { + if plan == nil { + return types.Subscription{}, utils.LavaFormatError("plan is nil", fmt.Errorf("createNewSubscription: cannot create new subscription")) + } autoRenewalNextPlan := types.AUTO_RENEWAL_PLAN_NONE if autoRenewalFlag { // On subscription creation, auto renewal is set to the subscription's plan @@ -223,6 +226,13 @@ func (k Keeper) createNewSubscription(ctx sdk.Context, plan *planstypes.Plan, cr } func (k Keeper) upgradeSubscriptionPlan(ctx sdk.Context, sub *types.Subscription, newPlan *planstypes.Plan) error { + if newPlan == nil { + return utils.LavaFormatError("new plan is nil", fmt.Errorf("upgradeSubscriptionPlan: cannot upgrade subscription plan")) + } + if sub == nil { + return utils.LavaFormatError("subscription is nil", fmt.Errorf("upgradeSubscriptionPlan: cannot upgrade subscription plan")) + } + block := uint64(ctx.BlockHeight()) nextEpoch, err := k.epochstorageKeeper.GetNextEpoch(ctx, block) From 50a264f77ae5a44f78b70ea025d6db61b2b3d058 Mon Sep 17 00:00:00 2001 From: Oren Date: Wed, 4 Sep 2024 13:51:51 +0300 Subject: [PATCH 2/6] added warning comment on detection index --- x/conflict/keeper/msg_server_detection.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/x/conflict/keeper/msg_server_detection.go b/x/conflict/keeper/msg_server_detection.go index ea1e30fe53..509fece136 100644 --- a/x/conflict/keeper/msg_server_detection.go +++ b/x/conflict/keeper/msg_server_detection.go @@ -14,6 +14,8 @@ import ( "golang.org/x/exp/slices" ) +// DetectionIndex creates an index for detection instances. +// WARNING: the detection index should not be used for prefixed iteration. func DetectionIndex(creatorAddr string, conflict *types.ResponseConflict, epochStart uint64) string { if conflict.IsDataNil() { return "" From bfaa18c0969b329cfb0d19481615c5f0cec66cfe Mon Sep 17 00:00:00 2001 From: Oren Date: Tue, 10 Sep 2024 17:06:41 +0300 Subject: [PATCH 3/6] remove redundant conflict data checks --- x/conflict/keeper/msg_server_detection.go | 8 -------- 1 file changed, 8 deletions(-) diff --git a/x/conflict/keeper/msg_server_detection.go b/x/conflict/keeper/msg_server_detection.go index 509fece136..3f92b6829b 100644 --- a/x/conflict/keeper/msg_server_detection.go +++ b/x/conflict/keeper/msg_server_detection.go @@ -17,9 +17,6 @@ import ( // DetectionIndex creates an index for detection instances. // WARNING: the detection index should not be used for prefixed iteration. func DetectionIndex(creatorAddr string, conflict *types.ResponseConflict, epochStart uint64) string { - if conflict.IsDataNil() { - return "" - } return creatorAddr + conflict.ConflictRelayData0.Request.RelaySession.Provider + conflict.ConflictRelayData1.Request.RelaySession.Provider + strconv.FormatUint(epochStart, 10) } @@ -130,11 +127,6 @@ func (k msgServer) handleSameProviderFinalizationConflict(ctx sdk.Context, confl } func (k msgServer) handleResponseConflict(ctx sdk.Context, goCtx context.Context, conflict *types.ResponseConflict, clientAddr sdk.AccAddress) (eventData map[string]string, err error) { - if conflict.IsDataNil() { - return nil, utils.LavaFormatWarning("conflict data is nil", fmt.Errorf("handleResponseConflict: cannot handle response conflict"), - utils.LogAttr("client", clientAddr.String()), - ) - } err = k.Keeper.ValidateResponseConflict(ctx, conflict, clientAddr) if err != nil { return nil, utils.LavaFormatWarning("Simulation: invalid response conflict detection", err, From b2c155b44d7902f6ba4371cdbcb58ad1d51caea6 Mon Sep 17 00:00:00 2001 From: Oren Date: Tue, 10 Sep 2024 17:15:15 +0300 Subject: [PATCH 4/6] reverted qos req and geo req turning pointers --- x/pairing/keeper/pairing_test.go | 9 ++++----- x/pairing/keeper/scores/geo_req.go | 25 +++++++++---------------- x/pairing/keeper/scores/qos_req.go | 20 +++++++------------- x/pairing/keeper/scores/stake_req.go | 5 +---- 4 files changed, 21 insertions(+), 38 deletions(-) diff --git a/x/pairing/keeper/pairing_test.go b/x/pairing/keeper/pairing_test.go index 6540f2881e..f1e607aff0 100644 --- a/x/pairing/keeper/pairing_test.go +++ b/x/pairing/keeper/pairing_test.go @@ -1203,7 +1203,7 @@ func verifyGeoScoreForTesting(providerScores []*pairingscores.PairingScore, slot }) geoReqObject := pairingscores.GeoReq{} - geoReq, ok := slot.Reqs[geoReqObject.GetName()].(*pairingscores.GeoReq) + geoReq, ok := slot.Reqs[geoReqObject.GetName()].(pairingscores.GeoReq) if !ok { return false } @@ -1331,8 +1331,7 @@ func TestNoRequiredGeo(t *testing.T) { // TestGeoSlotCalc checks that the calculated slots always hold a single bit geo req func TestGeoSlotCalc(t *testing.T) { - geoReq := pairingscores.GeoReq{} - geoReqName := geoReq.GetName() + geoReqName := pairingscores.GeoReq{}.GetName() allGeos := planstypes.GetAllGeolocations() maxGeo := lavaslices.Max(allGeos) @@ -1348,7 +1347,7 @@ func TestGeoSlotCalc(t *testing.T) { slots := pairingscores.CalcSlots(&policy) for _, slot := range slots { geoReqFromMap := slot.Reqs[geoReqName] - geoReq, ok := geoReqFromMap.(*pairingscores.GeoReq) + geoReq, ok := geoReqFromMap.(pairingscores.GeoReq) if !ok { require.Fail(t, "slot geo req is not of GeoReq type") } @@ -1367,7 +1366,7 @@ func TestGeoSlotCalc(t *testing.T) { slots := pairingscores.CalcSlots(&policy) for _, slot := range slots { geoReqFromMap := slot.Reqs[geoReqName] - geoReq, ok := geoReqFromMap.(*pairingscores.GeoReq) + geoReq, ok := geoReqFromMap.(pairingscores.GeoReq) if !ok { require.Fail(t, "slot geo req is not of GeoReq type") } diff --git a/x/pairing/keeper/scores/geo_req.go b/x/pairing/keeper/scores/geo_req.go index 4ed5e62960..b658cd3781 100644 --- a/x/pairing/keeper/scores/geo_req.go +++ b/x/pairing/keeper/scores/geo_req.go @@ -20,17 +20,13 @@ const ( minGeoLatency = 1 ) -func (gr *GeoReq) Init(policy planstypes.Policy) bool { - return gr != nil +func (gr GeoReq) Init(policy planstypes.Policy) bool { + return true } // Score calculates the geo score of a provider based on preset latency data // Note: each GeoReq must have exactly a single geolocation (bit) -func (gr *GeoReq) Score(score PairingScore) math.Uint { - if gr == nil { - return calculateCostFromLatency(maxGeoLatency) - } - +func (gr GeoReq) Score(score PairingScore) math.Uint { // check if the provider supports the required geolocation if gr.Geo&^score.Provider.Geolocation == 0 { return calculateCostFromLatency(minGeoLatency) @@ -42,16 +38,13 @@ func (gr *GeoReq) Score(score PairingScore) math.Uint { return cost } -func (gr *GeoReq) GetName() string { - if gr == nil { - return "" - } +func (gr GeoReq) GetName() string { return geoReqName } // Equal() used to compare slots to determine slot groups -func (gr *GeoReq) Equal(other ScoreReq) bool { - otherGeoReq, ok := other.(*GeoReq) +func (gr GeoReq) Equal(other ScoreReq) bool { + otherGeoReq, ok := other.(GeoReq) if !ok { return false } @@ -61,17 +54,17 @@ func (gr *GeoReq) Equal(other ScoreReq) bool { // TODO: this function doesn't return the optimal geo reqs for the case // that there are more required geos than providers to pair -func (gr *GeoReq) GetReqForSlot(policy planstypes.Policy, slotIdx int) ScoreReq { +func (gr GeoReq) GetReqForSlot(policy planstypes.Policy, slotIdx int) ScoreReq { policyGeoEnums := planstypes.GetGeolocationsFromUint(policy.GeolocationProfile) if len(policyGeoEnums) == 0 { utils.LavaFormatError("length of policyGeoEnums is zero", fmt.Errorf("critical: Attempt to divide by zero"), utils.LogAttr("policyGeoProfile", policy.GeolocationProfile), ) - return &GeoReq{Geo: int32(planstypes.Geolocation_USC)} + return GeoReq{Geo: int32(planstypes.Geolocation_USC)} } - return &GeoReq{Geo: int32(policyGeoEnums[slotIdx%len(policyGeoEnums)])} + return GeoReq{Geo: int32(policyGeoEnums[slotIdx%len(policyGeoEnums)])} } // CalcGeoCost() finds the minimal latency between the required geo and the provider's supported geolocations diff --git a/x/pairing/keeper/scores/qos_req.go b/x/pairing/keeper/scores/qos_req.go index 6d20dd2802..38d9a6f22c 100644 --- a/x/pairing/keeper/scores/qos_req.go +++ b/x/pairing/keeper/scores/qos_req.go @@ -16,12 +16,12 @@ type QosGetter interface { // QosReq implements the ScoreReq interface for provider staking requirement(s) type QosReq struct{} -func (qr *QosReq) Init(policy planstypes.Policy) bool { - return qr != nil +func (qr QosReq) Init(policy planstypes.Policy) bool { + return true } // Score calculates the the provider's qos score -func (qr *QosReq) Score(score PairingScore) math.Uint { +func (qr QosReq) Score(score PairingScore) math.Uint { // TODO: update Qos in providerQosFS properly and uncomment this code below // Also, the qos score should range between 0.5-2 @@ -31,25 +31,19 @@ func (qr *QosReq) Score(score PairingScore) math.Uint { // } // return math.Uint(qosScore) - if qr == nil { - return math.NewUint(1) - } return math.NewUint(1) } -func (qr *QosReq) GetName() string { - if qr == nil { - return "" - } +func (qr QosReq) GetName() string { return qosReqName } // Equal used to compare slots to determine slot groups. // Equal always returns true (there are no different "types" of qos) -func (qr *QosReq) Equal(other ScoreReq) bool { - return qr != nil +func (qr QosReq) Equal(other ScoreReq) bool { + return true } -func (qr *QosReq) GetReqForSlot(policy planstypes.Policy, slotIdx int) ScoreReq { +func (qr QosReq) GetReqForSlot(policy planstypes.Policy, slotIdx int) ScoreReq { return qr } diff --git a/x/pairing/keeper/scores/stake_req.go b/x/pairing/keeper/scores/stake_req.go index fb30a2f9e9..4da1659a90 100644 --- a/x/pairing/keeper/scores/stake_req.go +++ b/x/pairing/keeper/scores/stake_req.go @@ -12,7 +12,7 @@ const stakeReqName = "stake-req" type StakeReq struct{} func (sr *StakeReq) Init(policy planstypes.Policy) bool { - return sr != nil + return true } // Score calculates the the provider score as the normalized stake @@ -28,9 +28,6 @@ func (sr *StakeReq) Score(score PairingScore) math.Uint { } func (sr *StakeReq) GetName() string { - if sr == nil { - return "" - } return stakeReqName } From bf3a29686a0fdbbdae58c596c73605ca16890e1a Mon Sep 17 00:00:00 2001 From: Oren Date: Wed, 4 Sep 2024 18:02:23 +0300 Subject: [PATCH 5/6] removed pairing query cache --- x/pairing/keeper/keeper.go | 8 ------ x/pairing/keeper/pairing.go | 28 +++--------------- x/pairing/keeper/pairing_cache.go | 24 ---------------- x/pairing/keeper/pairing_cache_test.go | 39 +------------------------- 4 files changed, 5 insertions(+), 94 deletions(-) diff --git a/x/pairing/keeper/keeper.go b/x/pairing/keeper/keeper.go index 5451e3da5d..88642b77c0 100644 --- a/x/pairing/keeper/keeper.go +++ b/x/pairing/keeper/keeper.go @@ -4,7 +4,6 @@ import ( "fmt" storetypes "github.com/cosmos/cosmos-sdk/store/types" - epochstoragetypes "github.com/lavanet/lava/v3/x/epochstorage/types" timerstoretypes "github.com/lavanet/lava/v3/x/timerstore/types" "github.com/cometbft/cometbft/libs/log" @@ -35,8 +34,6 @@ type ( downtimeKeeper types.DowntimeKeeper dualstakingKeeper types.DualstakingKeeper stakingKeeper types.StakingKeeper - - pairingQueryCache *map[string][]epochstoragetypes.StakeEntry } ) @@ -74,8 +71,6 @@ func NewKeeper( ps = ps.WithKeyTable(types.ParamKeyTable()) } - emptypairingQueryCache := map[string][]epochstoragetypes.StakeEntry{} - keeper := &Keeper{ cdc: cdc, storeKey: storeKey, @@ -91,7 +86,6 @@ func NewKeeper( downtimeKeeper: downtimeKeeper, dualstakingKeeper: dualstakingKeeper, stakingKeeper: stakingKeeper, - pairingQueryCache: &emptypairingQueryCache, } // note that the timer and badgeUsedCu keys are the same (so we can use only the second arg) @@ -113,8 +107,6 @@ func (k Keeper) Logger(ctx sdk.Context) log.Logger { func (k Keeper) BeginBlock(ctx sdk.Context) { if k.epochStorageKeeper.IsEpochStart(ctx) { - // reset pairing query cache every epoch - *k.pairingQueryCache = map[string][]epochstoragetypes.StakeEntry{} // remove old session payments k.RemoveOldEpochPayments(ctx) // unstake/jail unresponsive providers diff --git a/x/pairing/keeper/pairing.go b/x/pairing/keeper/pairing.go index cc2cd2705e..638c01648a 100644 --- a/x/pairing/keeper/pairing.go +++ b/x/pairing/keeper/pairing.go @@ -80,7 +80,7 @@ func (k Keeper) GetPairingForClient(ctx sdk.Context, chainID string, clientAddre return nil, fmt.Errorf("invalid user for pairing: %s", err.Error()) } - providers, _, _, err = k.getPairingForClient(ctx, chainID, block, strictestPolicy, cluster, project.Index, false, true) + providers, _, _, err = k.getPairingForClient(ctx, chainID, block, strictestPolicy, cluster, project.Index, false) return providers, err } @@ -90,7 +90,7 @@ func (k Keeper) CalculatePairingChance(ctx sdk.Context, provider string, chainID totalScore := cosmosmath.ZeroUint() providerScore := cosmosmath.ZeroUint() - _, _, scores, err := k.getPairingForClient(ctx, chainID, uint64(ctx.BlockHeight()), policy, cluster, "dummy", true, false) + _, _, scores, err := k.getPairingForClient(ctx, chainID, uint64(ctx.BlockHeight()), policy, cluster, "dummy", true) if err != nil { return cosmosmath.LegacyZeroDec(), err } @@ -117,22 +117,12 @@ func (k Keeper) CalculatePairingChance(ctx sdk.Context, provider string, chainID // function used to get a new pairing from provider and client // first argument has all metadata, second argument is only the addresses -// useCache is a boolean argument that is used to determine whether pairing cache should be used -// Note: useCache should only be true for queries! functions that write to the state and use this function should never put useCache=true -func (k Keeper) getPairingForClient(ctx sdk.Context, chainID string, block uint64, policy *planstypes.Policy, cluster string, projectIndex string, calcChance bool, useCache bool) (providers []epochstoragetypes.StakeEntry, allowedCU uint64, providerScores []*pairingscores.PairingScore, errorRet error) { +func (k Keeper) getPairingForClient(ctx sdk.Context, chainID string, block uint64, policy *planstypes.Policy, cluster string, projectIndex string, calcChance bool) (providers []epochstoragetypes.StakeEntry, allowedCU uint64, providerScores []*pairingscores.PairingScore, errorRet error) { epoch, providersType, err := k.VerifyPairingData(ctx, chainID, block) if err != nil { return nil, 0, nil, fmt.Errorf("invalid pairing data: %s", err) } - // to be used only in queries as this changes gas calculations, and therefore must not be part of consensus - if useCache { - providers, found := k.GetPairingQueryCache(projectIndex, chainID, epoch) - if found { - return providers, policy.EpochCuLimit, nil, nil - } - } - stakeEntries := k.epochStorageKeeper.GetAllStakeEntriesForEpochChainId(ctx, epoch, chainID) if len(stakeEntries) == 0 { return nil, 0, nil, fmt.Errorf("did not find providers for pairing: epoch:%d, chainID: %s", block, chainID) @@ -149,9 +139,6 @@ func (k Keeper) getPairingForClient(ctx sdk.Context, chainID string, block uint6 stakeEntriesFiltered = append(stakeEntriesFiltered, stakeEntries[i]) } } - if useCache { - k.SetPairingQueryCache(projectIndex, chainID, epoch, stakeEntriesFiltered) - } return stakeEntriesFiltered, policy.EpochCuLimit, nil, nil } @@ -171,9 +158,6 @@ func (k Keeper) getPairingForClient(ctx sdk.Context, chainID string, block uint6 for _, score := range providerScores { filteredEntries = append(filteredEntries, *score.Provider) } - if useCache { - k.SetPairingQueryCache(projectIndex, chainID, epoch, filteredEntries) - } return filteredEntries, policy.EpochCuLimit, nil, nil } @@ -194,10 +178,6 @@ func (k Keeper) getPairingForClient(ctx sdk.Context, chainID string, block uint6 prevGroupSlot = group } - if useCache { - k.SetPairingQueryCache(projectIndex, chainID, epoch, providers) - } - return providers, policy.EpochCuLimit, providerScores, nil } @@ -350,7 +330,7 @@ func (k Keeper) ValidatePairingForClient(ctx sdk.Context, chainID string, provid return false, allowedCU, []epochstoragetypes.StakeEntry{}, fmt.Errorf("invalid user for pairing: %s", err.Error()) } - validAddresses, allowedCU, _, err = k.getPairingForClient(ctx, chainID, epoch, strictestPolicy, cluster, project.Index, false, false) + validAddresses, allowedCU, _, err = k.getPairingForClient(ctx, chainID, epoch, strictestPolicy, cluster, project.Index, false) if err != nil { return false, allowedCU, []epochstoragetypes.StakeEntry{}, err } diff --git a/x/pairing/keeper/pairing_cache.go b/x/pairing/keeper/pairing_cache.go index 5cad220cd6..a3652babc8 100644 --- a/x/pairing/keeper/pairing_cache.go +++ b/x/pairing/keeper/pairing_cache.go @@ -38,27 +38,3 @@ func (k Keeper) ResetPairingRelayCache(ctx sdk.Context) { store.Delete(iterator.Key()) } } - -// the cache used for the query, does not write into state -func (k Keeper) SetPairingQueryCache(project string, chainID string, epoch uint64, pairedProviders []epochstoragetypes.StakeEntry) { - if k.pairingQueryCache == nil { - // pairing cache is not initialized, will be in next epoch so simply skip - return - } - key := types.NewPairingCacheKey(project, chainID, epoch) - - (*k.pairingQueryCache)[key] = pairedProviders -} - -func (k Keeper) GetPairingQueryCache(project string, chainID string, epoch uint64) ([]epochstoragetypes.StakeEntry, bool) { - if k.pairingQueryCache == nil { - // pairing cache is not initialized, will be in next epoch so simply skip - return nil, false - } - key := types.NewPairingCacheKey(project, chainID, epoch) - if providers, ok := (*k.pairingQueryCache)[key]; ok { - return providers, true - } - - return nil, false -} diff --git a/x/pairing/keeper/pairing_cache_test.go b/x/pairing/keeper/pairing_cache_test.go index 31b6669192..fb1f0829ef 100644 --- a/x/pairing/keeper/pairing_cache_test.go +++ b/x/pairing/keeper/pairing_cache_test.go @@ -7,44 +7,7 @@ import ( "github.com/stretchr/testify/require" ) -// TestPairingQueryCache tests the following: -// 1. The pairing query cache is reset every epoch -// 2. Getting pairing with a query using an existent cache entry consumes fewer gas than without one -func TestPairingQueryCache(t *testing.T) { - ts := newTester(t) - ts.setupForPayments(1, 1, 0) // 1 provider, 1 client, default providers-to-pair - - _, consumer := ts.GetAccount(common.CONSUMER, 0) - - getPairingGas := func(ts *tester) uint64 { - gm := ts.Ctx.GasMeter() - before := gm.GasConsumed() - _, err := ts.QueryPairingGetPairing(ts.spec.Index, consumer) - require.NoError(t, err) - return gm.GasConsumed() - before - } - - // query for pairing for the first time - empty cache - emptyCacheGas := getPairingGas(ts) - - // query for pairing for the second time - non-empty cache - filledCacheGas := getPairingGas(ts) - - // second time gas should be smaller than first time - require.Less(t, filledCacheGas, emptyCacheGas) - - // advance block to test it stays the same (should still be less than empty cache gas) - ts.AdvanceBlock() - filledAfterBlockCacheGas := getPairingGas(ts) - require.Less(t, filledAfterBlockCacheGas, emptyCacheGas) - - // advance epoch to reset the cache - ts.AdvanceEpoch() - emptyCacheAgainGas := getPairingGas(ts) - require.Equal(t, emptyCacheGas, emptyCacheAgainGas) -} - -// TestPairingQueryCache tests the following: +// TestPairingRelayCache tests the following: // 1. The pairing relay cache is reset every block // 2. Getting pairing in relay payment using an existent cache entry consumes fewer gas than without one func TestPairingRelayCache(t *testing.T) { From 9218823d827a66b0faf97dd88a5b3a32be1b11cf Mon Sep 17 00:00:00 2001 From: Oren Date: Sun, 22 Sep 2024 16:50:48 +0300 Subject: [PATCH 6/6] fix comment --- x/conflict/keeper/msg_server_detection.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/x/conflict/keeper/msg_server_detection.go b/x/conflict/keeper/msg_server_detection.go index 3f92b6829b..5a2dbd3727 100644 --- a/x/conflict/keeper/msg_server_detection.go +++ b/x/conflict/keeper/msg_server_detection.go @@ -15,7 +15,8 @@ import ( ) // DetectionIndex creates an index for detection instances. -// WARNING: the detection index should not be used for prefixed iteration. +// WARNING: the detection index should not be used for prefixed iteration since it doesn't contain delimeters +// thus it's not sanitized for such iterations and could cause issues in the future as the codebase evolves. func DetectionIndex(creatorAddr string, conflict *types.ResponseConflict, epochStart uint64) string { return creatorAddr + conflict.ConflictRelayData0.Request.RelaySession.Provider + conflict.ConflictRelayData1.Request.RelaySession.Provider + strconv.FormatUint(epochStart, 10) }