candidate fix for v18 #6144

p0mvn · 2023-08-24T07:43:21Z

Closes: #XXX

What is the purpose of the change

Needs to be cherry-picked onto main

Testing and Verifying

(Please pick one of the following options)

This change is a trivial rework / code cleanup without any test coverage.

(or)

This change is already covered by existing tests, such as (please describe tests).

(or)

This change added tests and can be verified as follows:

(example:)

Added unit test that validates ...
Added integration tests for end-to-end deployment with ...
Extended integration test for ...
Manually verified the change by ...

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

app/upgrades/v18/upgrades_test.go

p0mvn · 2023-08-24T08:03:13Z

x/incentives/keeper/distribute.go

+		// In this case, remove redundant cases.
+		// Namely: gauge empty OR gauge coins undistributable.
+		if remainCoins.Empty() {
+			ctx.Logger().Debug(fmt.Sprintf("gauge debug, this gauge is empty, why is it being ran %d. Balancer code", gauge.Id))
+			err := k.updateGaugePostDistribute(ctx, gauge, totalDistrCoins)
+			return totalDistrCoins, err
+		}
+
+		// Remove some spam gauges, is state compatible.
+		// If they're to pool 1 they can't distr at this small of a quantity.
+		if remainCoins.Len() == 1 && remainCoins[0].Amount.LTE(sdk.NewInt(10)) && gauge.DistributeTo.Denom == "gamm/pool/1" && remainCoins[0].Denom != "uosmo" {
+			ctx.Logger().Debug(fmt.Sprintf("gauge debug, this gauge is perceived spam, skipping %d", gauge.Id))
+			err := k.updateGaugePostDistribute(ctx, gauge, totalDistrCoins)
+			return totalDistrCoins, err
+		}


This is the state-compatible fix to speed up incentive distribution. Makes sense to me to include it

p0mvn · 2023-08-24T08:05:27Z

x/lockup/keeper/lock.go

@@ -295,6 +295,22 @@ func (k Keeper) clearKeysByPrefix(ctx sdk.Context, prefix []byte) {
 	}
 }

+func (k Keeper) RebuildAccumulationStoreForDenom(ctx sdk.Context, denom string) {


TODO: unit test this if time

* fix balancer liquidity breaking incentives * clean up * comment * added swap checks * check that LPing fails before the upgrade * fix test --------- Co-authored-by: Nicolas Lara <nicolaslara@gmail.com>

This reverts commit eb0cb01.

This reverts commit 1129ee8.

ValarDragon · 2023-08-24T14:11:46Z

x/incentives/keeper/distribute.go

@@ -330,7 +330,7 @@ func (k Keeper) distributeInternal(
 			// Note: reason why we do millisecond conversion is because floats are non-deterministic.
 			emissionRate := sdk.NewDecFromInt(remainAmountPerEpoch).QuoTruncate(sdk.NewDec(currentEpoch.Duration.Milliseconds()).QuoInt(sdk.NewInt(1000)))

-			ctx.Logger().Debug("distributeInternal, CreateIncentiveRecord NoLock gauge", "module", types.ModuleName, "gaugeId", gauge.Id, "poolId", pool.GetId(), "remainCoinPerEpoch", remainCoinPerEpoch, "height", ctx.BlockHeight())
+			ctx.Logger().Info("distributeInternal, CreateIncentiveRecord NoLock gauge", "module", types.ModuleName, "gaugeId", gauge.Id, "poolId", pool.GetId(), "remainCoinPerEpoch", remainCoinPerEpoch, "height", ctx.BlockHeight())


This got converted to Info, it should be debug

ValarDragon · 2023-08-24T14:12:21Z

x/incentives/keeper/distribute.go

@@ -330,7 +330,7 @@ func (k Keeper) distributeInternal(
 			// Note: reason why we do millisecond conversion is because floats are non-deterministic.
 			emissionRate := sdk.NewDecFromInt(remainAmountPerEpoch).QuoTruncate(sdk.NewDec(currentEpoch.Duration.Milliseconds()).QuoInt(sdk.NewInt(1000)))

-			ctx.Logger().Debug("distributeInternal, CreateIncentiveRecord NoLock gauge", "module", types.ModuleName, "gaugeId", gauge.Id, "poolId", pool.GetId(), "remainCoinPerEpoch", remainCoinPerEpoch, "height", ctx.BlockHeight())
+			ctx.Logger().Info("distributeInternal, CreateIncentiveRecord NoLock gauge", "module", types.ModuleName, "gaugeId", gauge.Id, "poolId", pool.GetId(), "remainCoinPerEpoch", remainCoinPerEpoch, "height", ctx.BlockHeight())


Suggested change

ctx.Logger().Info("distributeInternal, CreateIncentiveRecord NoLock gauge", "module", types.ModuleName, "gaugeId", gauge.Id, "poolId", pool.GetId(), "remainCoinPerEpoch", remainCoinPerEpoch, "height", ctx.BlockHeight())

ctx.Logger().Debug("distributeInternal, CreateIncentiveRecord NoLock gauge", "module", types.ModuleName, "gaugeId", gauge.Id, "poolId", pool.GetId(), "remainCoinPerEpoch", remainCoinPerEpoch, "height", ctx.BlockHeight())

ValarDragon · 2023-08-24T14:14:20Z

x/superfluid/keeper/stake.go

@@ -66,7 +66,8 @@ func (k Keeper) RefreshIntermediaryDelegationAmounts(ctx sdk.Context) {
 		if !found {
 			// continue if current delegation is 0, in case its really a dust delegation
 			// that becomes worth something after refresh.
-			k.Logger(ctx).Info(fmt.Sprintf("Existing delegation not found for %s with %s during superfluid refresh."+
+			// TODO: We have a correct explanation for this in some github issue, lets amend this correctly.
+			k.Logger(ctx).Debug(fmt.Sprintf("Existing delegation not found for %s with %s during superfluid refresh."+


Spares our epoch logs 🙏

ValarDragon

Tested this on a flow where we:
Make a v18 with broken accum store
Make a v19 with these changes

We saw that v18 caused the break as expected, and v19 fixed it.

Hence this as v19 should fix the mainnet problem.

The load bearing part of the fix is actually ran every testnet as InitGenesis

ValarDragon and others added 3 commits August 23, 2023 23:01

Proper fix of rebuilding sumtree

1ebf6ba

State compatible epoch speedup

b32ff3d

Change to correct package

c02c658

github-actions bot added C:x/lockup C:x/incentives C:x/superfluid C:app-wiring Changes to the app folder labels Aug 24, 2023

Add upgrade test

9be2a4b

p0mvn commented Aug 24, 2023

View reviewed changes

app/upgrades/v18/upgrades_test.go Outdated Show resolved Hide resolved

p0mvn commented Aug 24, 2023

View reviewed changes

app/upgrades/v18/upgrades_test.go Outdated Show resolved Hide resolved

app/upgrades/v18/upgrades_test.go Outdated Show resolved Hide resolved

p0mvn commented Aug 24, 2023

View reviewed changes

p0mvn and others added 6 commits August 24, 2023 11:24

extra info logs

0a9f4e8

Add Paniccing code to upgrade test

9815fa0

updates

320e957

reset sumtree for all pools

eb0cb01

remove unused constant for max pool id

1129ee8

fix balancer liquidity breaking incentives (#6145)

08ac3ba

* fix balancer liquidity breaking incentives * clean up * comment * added swap checks * check that LPing fails before the upgrade * fix test --------- Co-authored-by: Nicolas Lara <nicolaslara@gmail.com>

github-actions bot added the C:x/concentrated-liquidity label Aug 24, 2023

p0mvn added 4 commits August 24, 2023 14:39

zero qualifying shares bug fix

a6f2d54

Revert "reset sumtree for all pools"

28b3dc6

This reverts commit eb0cb01.

Revert "remove unused constant for max pool id"

d39ef3f

This reverts commit 1129ee8.

reset sumtree until first CL pool id

fb2ec9e

ValarDragon marked this pull request as ready for review August 24, 2023 14:10

ValarDragon requested review from stackman27, AlpinYukseloglu and czarcas7ic as code owners August 24, 2023 14:10

ValarDragon reviewed Aug 24, 2023

View reviewed changes

Update Changelog

6b53e24

nicolaslara approved these changes Aug 24, 2023

View reviewed changes

ValarDragon approved these changes Aug 24, 2023

View reviewed changes

ValarDragon merged commit 1c5f25d into v18.x Aug 24, 2023
1 check passed

ValarDragon deleted the dev/v18.x_fix branch August 24, 2023 14:20

p0mvn mentioned this pull request Aug 29, 2023

refactor: allow zero qualifying balancer shares in CL incentives (backport #6162) #6234

Closed

github-actions bot mentioned this pull request May 15, 2024

Broken Links Detected #8269

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

candidate fix for v18 #6144

candidate fix for v18 #6144

p0mvn commented Aug 24, 2023 •

edited

Loading

p0mvn Aug 24, 2023

p0mvn Aug 24, 2023

ValarDragon Aug 24, 2023 •

edited

Loading

ValarDragon Aug 24, 2023

ValarDragon Aug 24, 2023

ValarDragon left a comment

	ctx.Logger().Info("distributeInternal, CreateIncentiveRecord NoLock gauge", "module", types.ModuleName, "gaugeId", gauge.Id, "poolId", pool.GetId(), "remainCoinPerEpoch", remainCoinPerEpoch, "height", ctx.BlockHeight())
	ctx.Logger().Debug("distributeInternal, CreateIncentiveRecord NoLock gauge", "module", types.ModuleName, "gaugeId", gauge.Id, "poolId", pool.GetId(), "remainCoinPerEpoch", remainCoinPerEpoch, "height", ctx.BlockHeight())

candidate fix for v18 #6144

candidate fix for v18 #6144

Conversation

p0mvn commented Aug 24, 2023 • edited Loading

What is the purpose of the change

Testing and Verifying

Documentation and Release Note

p0mvn Aug 24, 2023

Choose a reason for hiding this comment

p0mvn Aug 24, 2023

Choose a reason for hiding this comment

ValarDragon Aug 24, 2023 • edited Loading

Choose a reason for hiding this comment

ValarDragon Aug 24, 2023

Choose a reason for hiding this comment

ValarDragon Aug 24, 2023

Choose a reason for hiding this comment

ValarDragon left a comment

Choose a reason for hiding this comment

p0mvn commented Aug 24, 2023 •

edited

Loading

ValarDragon Aug 24, 2023 •

edited

Loading