[Uptime Incentives]: Add authorized uptime validation for new NoLock gauges #7376

AlpinYukseloglu · 2024-01-28T20:51:37Z

Closes: #7367

What is the purpose of the change

This PR adds the relevant validation logic for authorized uptimes on CL gauge durations. Given how this logic touches existing state and a heavily depended on component of our codebase, the changes have heavy comments explaining the reasoning and implications.

This PR also makes a change to how CL gauges are linked to pool IDs in state. It is important to note that this breaks the previous setup where there was a "general linking" and then an additional link for internal gauges, reducing this to a single link based on gauge duration. The implications and next steps to ensure the safety of this are outlined in great detail in #7375.

Testing and Verifying

Tests covering the validation logic are added to gauge_test.go. The key changes are covered by existing tests, which were how the issue was discovered in the first place.

Documentation and Release Note

Does this pull request introduce a new feature or user-facing behavior changes?
Changelog entry added to Unreleased section of CHANGELOG.md?

Where is the change documented?

Specification (x/{module}/README.md)
Osmosis documentation site
Code comments?
N/A

AlpinYukseloglu · 2024-01-28T20:58:04Z

x/incentives/keeper/gauge_test.go

-				// Assert that pool id and gauge id link meant for internally incentivized gauges is unset.
-				_, err := s.App.PoolIncentivesKeeper.GetPoolGaugeId(s.Ctx, tc.poolId, tc.distrTo.Duration)
-				s.Require().Error(err)
-


Highlighting this for review @p0mvn. The key-related change in this PR means that the "general linking" between gauges and pools is now based on duration, which means that this check is no longer necessary.

It also technically means that we can skip the "internal gauge link" component of the internal gauge setup, but that logic is not well abstracted so it would require meaningful additional work to save on the duplicate write that would be happening here.

AlpinYukseloglu · 2024-01-28T21:09:15Z

x/incentives/keeper/distribute_test.go

@@ -1139,6 +1139,7 @@ func (s *KeeperTestSuite) CreateNoLockExternalGauges(clPoolId uint64, externalGa
 	clPoolExternalGaugeId, err := s.App.IncentivesKeeper.CreateGauge(s.Ctx, numEpochsPaidOver == 1, gaugeCreator, externalGaugeCoins,
 		lockuptypes.QueryCondition{
 			LockQueryType: lockuptypes.NoLock,
+			Duration:      time.Nanosecond,


Note to reviewer: CL gauges with unauthorized uptimes can no longer be created due to the new validation logic, so existing tests are updated to use 1ns which is authorized by default. See #7375 for discussion on backwards compatibility with gauges currently live on mainnet with unauthorized uptimes.

p0mvn

Nice work! LGTM so far.

I will aim to review replies and run tests more before approving later today or whenever ready

x/incentives/keeper/gauge.go

x/pool-incentives/keeper/grpc_query_test.go

p0mvn

LGTM.

I had some recommendations for making the execution flow stricter but not a blocker

x/incentives/keeper/gauge.go

p0mvn · 2024-01-29T06:05:23Z

x/incentives/keeper/gauge.go

+	if isExternalConcentratedPoolGauge {
+		durations = k.clk.GetParams(ctx).AuthorizedUptimes
+	} else if isInternalConcentratedPoolGauge {
+		// Internal CL gauges use epoch time as their duration. This is a legacy
+		// property that does not affect the uptime on created records, which is
+		// determined by the gov param for internal incentive uptimes.
+		durations = []time.Duration{k.GetEpochInfo(ctx).Duration}
+	} else {
+		// This branch is applicable to CFMM pool types such as balancer and stableswap.
+		durations = k.GetLockableDurations(ctx)
+	}


This would make the conditional branching stricter and eliminate the likelihood of invalid field setup causing issues.

nit: the else branch is also not covered by a test at the moment and might benefit from it

Suggested change

if isExternalConcentratedPoolGauge {

durations = k.clk.GetParams(ctx).AuthorizedUptimes

} else if isInternalConcentratedPoolGauge {

// Internal CL gauges use epoch time as their duration. This is a legacy

// property that does not affect the uptime on created records, which is

// determined by the gov param for internal incentive uptimes.

durations = []time.Duration{k.GetEpochInfo(ctx).Duration}

} else {

// This branch is applicable to CFMM pool types such as balancer and stableswap.

durations = k.GetLockableDurations(ctx)

}

if isExternalConcentratedPoolGauge {

durations = k.clk.GetParams(ctx).AuthorizedUptimes

} else if isInternalConcentratedPoolGauge {

// Internal CL gauges use epoch time as their duration. This is a legacy

// property that does not affect the uptime on created records, which is

// determined by the gov param for internal incentive uptimes.

durations = []time.Duration{k.GetEpochInfo(ctx).Duration}

} else if !isConcentratedPoolGauge {

// This branch is applicable to CFMM pool types such as balancer and stableswap.

durations = k.GetLockableDurations(ctx)

} else {

return 0, fmt.Errorf("invalid gauge type: %s", distrTo.LockQueryType)

}

I see what you're getting at here, but this flow is not intended to serve as a check or filter where we return an error. For example, your implementation would error if CreateGauge is called for a Group gauge, which is not what we want here.

It does seem as though this logic is somewhat unclear though. I moved this check into the block where the duration checks happen, as that probably makes it clearer (i.e. if NoLock gauge || ByDuration gauge, run this logic implies that we are covering everything we need to with the logic branches in the original implementation). Hopefully this makes it a bit clearer.

Please let me know if I misunderstood your suggestion

…o appropriate logic branch

mattverse · 2024-01-29T07:21:45Z

x/pool-incentives/keeper/genesis.go

@@ -28,7 +28,7 @@ func (k Keeper) InitGenesis(ctx sdk.Context, genState *types.GenesisState) {
 	}
 	if genState.ConcentratedPoolToNoLockGauges != nil {
 		for _, record := range genState.ConcentratedPoolToNoLockGauges.PoolToGauge {
-			k.SetPoolGaugeIdNoLock(ctx, record.PoolId, record.GaugeId)
+			k.SetPoolGaugeIdNoLock(ctx, record.PoolId, record.GaugeId, record.Duration)


Are we planning for any migration PR after this? 👀

We shouldn't need to - please see #7375 (if you see any problems with the reasoning in that issue please flag them!)

mattverse · 2024-01-29T07:42:43Z

x/incentives/keeper/gauge_test.go

@@ -507,7 +507,8 @@ func (s *KeeperTestSuite) TestCreateGauge_NoLockGauges() {
 			distrTo: lockuptypes.QueryCondition{
 				LockQueryType: lockuptypes.NoLock,
 				// Note: this assumes the gauge is external
-				Denom: "",
+				Denom:    "",


Can we also add non-failing cases for the three branches that has been added to nolock createGauge?

The non-failing case should be covered by existing tests, but will re review to make sure I'm not missing anything

Sg! On first site of existing tests, test cases did not seem obvious in which case touches which branch. Please feel free to merge once you have re-checked!

Right – since 1 nanosecond is an authorized uptime by default, the test cases with that duration should be covering the non-failing branches

czarcas7ic

This should already be addressed in #7369, but if it isn’t yet, an invalid or unauthorized gauge duration should fall back to the default uptime of 1ns in this line (instead of directly using the gauge’s duration):

I just had a question about this statement. Why should we fall back in this case? Shouldn't we be able to control this so that distribute never comes across an invalid or unauthorized duration?

AlpinYukseloglu · 2024-01-30T20:50:32Z

I just had a question about this statement. Why should we fall back in this case? Shouldn't we be able to control this so that distribute never comes across an invalid or unauthorized duration?

#7369 has a breakdown of the reasoning. Tldr is this 1. minimizes the surface for existing gauges to unintentionally trigger chain halts/block incentives, and 2. is a clever way for us to ensure uptimes can be "unauthorized" in the future.

There are probably marginal gains we can make on these fronts with a meaningfully more complex implementation, but this approach seems quite elegant (and secure) to me given the current tradeoff space. As always, feel free to push back!

…gauges (#7376) * add uptime validation logic and fix pool gauge link * add tests and clean up implementation * add changelog and clean up prints/diff * update changelog entry to be clearer * clean up comments and add additional tests * fix broken genesis tests * extract default NoLock duration to variable and add further comments * switch naming from concentrated to NoLock and move duration setup into appropriate logic branch

AlpinYukseloglu added 2 commits January 28, 2024 10:44

add uptime validation logic and fix pool gauge link

317ae1c

add tests and clean up implementation

ff27caf

AlpinYukseloglu requested review from czarcas7ic, p0mvn and mattverse as code owners January 28, 2024 20:51

github-actions bot added C:x/incentives C:x/pool-incentives labels Jan 28, 2024

AlpinYukseloglu commented Jan 28, 2024

View reviewed changes

AlpinYukseloglu added 2 commits January 28, 2024 12:58

add changelog and clean up prints/diff

a12082b

update changelog entry to be clearer

815245e

AlpinYukseloglu added the V:state/breaking State machine breaking PR label Jan 28, 2024

AlpinYukseloglu commented Jan 28, 2024

View reviewed changes

AlpinYukseloglu added 2 commits January 28, 2024 13:16

clean up comments and add additional tests

e036a7b

fix broken genesis tests

39ba614

p0mvn reviewed Jan 28, 2024

View reviewed changes

x/incentives/keeper/gauge.go Outdated Show resolved Hide resolved

x/incentives/keeper/gauge.go Outdated Show resolved Hide resolved

x/incentives/keeper/gauge.go Outdated Show resolved Hide resolved

x/pool-incentives/keeper/grpc_query_test.go Outdated Show resolved Hide resolved

extract default NoLock duration to variable and add further comments

00b845b

AlpinYukseloglu requested a review from p0mvn January 28, 2024 22:36

p0mvn approved these changes Jan 29, 2024

View reviewed changes

switch naming from concentrated to NoLock and move duration setup int…

34bba93

…o appropriate logic branch

mattverse reviewed Jan 29, 2024

View reviewed changes

czarcas7ic approved these changes Jan 29, 2024

View reviewed changes

AlpinYukseloglu merged commit 14078fe into main Jan 30, 2024
1 check passed

AlpinYukseloglu deleted the alpo/gauge-uptime-validation branch January 30, 2024 21:17

github-actions bot mentioned this pull request Mar 1, 2024

Broken Links Detected #7641

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Uptime Incentives]: Add authorized uptime validation for new NoLock gauges #7376

[Uptime Incentives]: Add authorized uptime validation for new NoLock gauges #7376

AlpinYukseloglu commented Jan 28, 2024 •

edited

Loading

AlpinYukseloglu Jan 28, 2024

AlpinYukseloglu Jan 28, 2024

p0mvn left a comment

p0mvn left a comment

p0mvn Jan 29, 2024

AlpinYukseloglu Jan 29, 2024 •

edited

Loading

AlpinYukseloglu Jan 29, 2024

mattverse Jan 29, 2024

AlpinYukseloglu Jan 29, 2024

mattverse Jan 29, 2024

AlpinYukseloglu Jan 29, 2024

mattverse Jan 30, 2024

AlpinYukseloglu Jan 30, 2024

czarcas7ic left a comment

AlpinYukseloglu commented Jan 30, 2024

[Uptime Incentives]: Add authorized uptime validation for new NoLock gauges #7376

[Uptime Incentives]: Add authorized uptime validation for new NoLock gauges #7376

Conversation

AlpinYukseloglu commented Jan 28, 2024 • edited Loading

What is the purpose of the change

Testing and Verifying

Documentation and Release Note

Choose a reason for hiding this comment

Choose a reason for hiding this comment

p0mvn left a comment

Choose a reason for hiding this comment

p0mvn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlpinYukseloglu Jan 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

czarcas7ic left a comment

Choose a reason for hiding this comment

AlpinYukseloglu commented Jan 30, 2024

AlpinYukseloglu commented Jan 28, 2024 •

edited

Loading

AlpinYukseloglu Jan 29, 2024 •

edited

Loading