Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Increase 'max gas' and 'vm cycle limit' from 10M to 100M #2065

Merged
merged 5 commits into from
May 15, 2024

Conversation

r3v4s
Copy link
Contributor

@r3v4s r3v4s commented May 9, 2024

closes #1788

similar closed pr #1901

This pr bumps 2 maximum limits

  1. max block gas
  2. max vm cycle

AFAIK, to use amount of increased gas, vm cycle also need to be bump too.

This is temporarily fix pr until we have clear ways to measure gas usage and vm cycle.

Contributors' checklist...
  • Added new tests, or not needed, or not feasible
  • Provided an example (e.g. screenshot) to aid review or the PR is self-explanatory
  • Updated the official documentation or not needed
  • No breaking changes were made, or a BREAKING CHANGE: xxx message was included in the description
  • Added references to related issues and PRs
  • Provided any useful hints for running manual tests
  • [] Added new benchmarks to generated graphs, if any. More info here.

@r3v4s r3v4s requested review from gfanton, moul, thehowl, zivkovicmilos, a team, jaekwon and piux2 as code owners May 9, 2024 11:55
@github-actions github-actions bot added 📦 🌐 tendermint v2 Issues or PRs tm2 related 📦 ⛰️ gno.land Issues or PRs gno.land package related labels May 9, 2024
Copy link

codecov bot commented May 9, 2024

Codecov Report

Attention: Patch coverage is 85.71429% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 54.19%. Comparing base (8057505) to head (ea548e2).
Report is 3 commits behind head on master.

Files Patch % Lines
gno.land/pkg/gnoland/node_inmemory.go 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2065      +/-   ##
==========================================
- Coverage   54.96%   54.19%   -0.77%     
==========================================
  Files         481      520      +39     
  Lines       67391    73194    +5803     
==========================================
+ Hits        37040    39670    +2630     
- Misses      27330    30297    +2967     
- Partials     3021     3227     +206     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@thehowl
Copy link
Member

thehowl commented May 9, 2024

From the review meeting, we're considering whether it makes sense to bump the limit 10x (not 1000x, as it most definitely is a Bad Idea)

We'll still take a second to consider this, but please in the meantime @r3v4s update the PR to be a 10x increase rather than 1000x; and provide examples of the contracts from GnoSwap which take more than 10M gas.

Thanks!

cc/ @jaekwon

@r3v4s r3v4s changed the title fix: Increase 'max gas' and 'vm cycle limit' from 10M to 10_000M fix: Increase 'max gas' and 'vm cycle limit' from 10M to 100M May 10, 2024
@r3v4s r3v4s requested review from a team as code owners May 10, 2024 03:43
@github-actions github-actions bot added the 🧾 package/realm Tag used for new Realms or Packages. label May 10, 2024
@r3v4s
Copy link
Contributor Author

r3v4s commented May 10, 2024

From the review meeting, we're considering whether it makes sense to bump the limit 10x (not 1000x, as it most definitely is a Bad Idea)

We'll still take a second to consider this, but please in the meantime @r3v4s update the PR to be a 10x increase rather than 1000x; and provide examples of the contracts from GnoSwap which take more than 10M gas.

Thanks!

cc/ @jaekwon

cc/ @thehowl @zivkovicmilos @jaekwon

Decreased bump size from x1000 to x10.

And about examples for max gas and max cycle thing... here are some information.

Tested Env

MAX GAS ISSUES

  1. Deploying uint256 in Gno takes 8.9M gas
    image

  2. Deploying a modified version of uint256 in GnoSwap takes 9.2M gas

$ gnokey maketx addpkg -pkgdir ./gnoswap/uint256 -pkgpath gno.land/p/demo/u256_with_512 -broadcast=true -gas-fee 1ugnot -gas-wanted 10000000 -insecure-password-stdin=true test1 -remote localhost:36657
Enter password.

OK!
GAS WANTED: 10000000
GAS USED:   9266840
HEIGHT:     5294
EVENTS:     []
  1. Deploying the same version uint256 from the above 2. with a longer pkg_path takes more gas (MUST BE A VM BUG)
  • deploy to gno.land/p/demo/gnoswap/u1 => 9.5M
  • deploy to gno.land/p/demo/gnoswap/u2 => 9.5M
  • deploy to gno.land/p/demo/gnoswap/what_an_long_pkg_path => out of gas
  • deploy to gno.land/p/demo/gnoswap/longer_path => 9.9M
    image
  1. Deploying the pool contract takes 9.9M
$ echo "" | gnokey maketx addpkg -pkgdir ./pool -pkgpath gno.land/r/demo/pool -broadcast=true -gas-fee 1ugnot -gas-wanted 10000000 -insecure-password-stdin=true test1 -remote localhost:36657 -insecure-password-stdin=true
Enter password.

OK!
GAS WANTED: 10000000
GAS USED:   9908296
HEIGHT:     89
EVENTS:     []

MAX VM_CYCLE ISSUE

  1. Calling SwapRoute in the router contract can cost more than 10M
// POOL
// Swap swaps token0 for token1, or token1 for token0
//
// Panics if any of the following conditions are met:
// - The caller is not the router contract
// - Target pool is being used by another transaction
// - The amountSpecified is 0
// - The SqrtPriceLimit is not within the range
func Swap(
	token0Path string,
	token1Path string,
	fee uint32,
	_recipient string,
	zeroForOne bool,
	_amountSpecified string, // int256
	_sqrtPriceLimitX96 string, // uint160
	_payer string, // router
) (string, string) { // int256 x2
	common.DisallowCallFromUser()
	common.AllowCallFromOnly(consts.ROUTER_PATH)

	if _amountSpecified == "0" {
		panic("[POOL] pool.gno__Swap() || _amountSpecified == 0")
	}

	amountSpecified := i256.MustFromDecimal(_amountSpecified)
	sqrtPriceLimitX96 := u256.MustFromDecimal(_sqrtPriceLimitX96)

	recipient := std.Address(_recipient)
	payer := std.Address(_payer)

	pool := GetPool(token0Path, token1Path, fee)
	slot0Start := pool.slot0

	if !(slot0Start.unlocked) {
		panic("[POOL] pool.gno__Swap() || slot0Start.unlocked must be unlocked(true)")
	}

	var feeProtocol uint8
	var feeGrowthGlobalX128 *u256.Uint

	if zeroForOne {
		minSqrtRatio := u256.MustFromDecimal(consts.MIN_SQRT_RATIO)

		cond1 := sqrtPriceLimitX96.Lt(slot0Start.sqrtPriceX96)
		cond2 := sqrtPriceLimitX96.Gt(minSqrtRatio)
		if !(cond1 && cond2) {
			panic(ufmt.Sprintf("[POOL] pool.gno__Swap() || sqrtPriceLimitX96(%s) < slot0Start.sqrtPriceX96(%s) && sqrtPriceLimitX96(%s) > consts.MIN_SQRT_RATIO(%s)", sqrtPriceLimitX96.ToString(), slot0Start.sqrtPriceX96.ToString(), sqrtPriceLimitX96.ToString(), consts.MIN_SQRT_RATIO))
		}

		feeProtocol = slot0Start.feeProtocol % 16
		feeGrowthGlobalX128 = pool.feeGrowthGlobal0X128
	} else {
		maxSqrtRatio := u256.MustFromDecimal(consts.MAX_SQRT_RATIO)

		cond1 := sqrtPriceLimitX96.Gt(slot0Start.sqrtPriceX96)
		cond2 := sqrtPriceLimitX96.Lt(maxSqrtRatio)
		if !(cond1 && cond2) {
			panic(ufmt.Sprintf("[POOL] pool.gno__Swap() || sqrtPriceLimitX96(%s) > slot0Start.sqrtPriceX96(%s) && sqrtPriceLimitX96(%s) < consts.MAX_SQRT_RATIO(%s)", sqrtPriceLimitX96.ToString(), slot0Start.sqrtPriceX96.ToString(), sqrtPriceLimitX96.ToString(), consts.MAX_SQRT_RATIO))
		}

		feeProtocol = slot0Start.feeProtocol / 16
		feeGrowthGlobalX128 = pool.feeGrowthGlobal1X128
	}

	pool.slot0.unlocked = false
	cache := newSwapCache(feeProtocol, pool.liquidity)
	state := pool.newSwapState(amountSpecified, feeGrowthGlobalX128, cache.liquidityStart)

	exactInput := amountSpecified.Gt(i256.Zero())

	// continue swapping as long as we haven't used the entire input/output and haven't reached the price limit
	for !(state.amountSpecifiedRemaining.IsZero()) && !(state.sqrtPriceX96.Eq(sqrtPriceLimitX96)) {
		var step StepComputations
		step.sqrtPriceStartX96 = state.sqrtPriceX96

		step.tickNext, step.initialized = pool.tickBitmapNextInitializedTickWithInOneWord(
			state.tick,
			pool.tickSpacing,
			zeroForOne,
		)

		// ensure that we do not overshoot the min/max tick, as the tick bitmap is not aware of these bounds
		if step.tickNext < consts.MIN_TICK {
			step.tickNext = consts.MIN_TICK
		} else if step.tickNext > consts.MAX_TICK {
			step.tickNext = consts.MAX_TICK
		}

		// get the price for the next tick
		step.sqrtPriceNextX96 = common.TickMathGetSqrtRatioAtTick(step.tickNext)

		isLower := step.sqrtPriceNextX96.Lt(sqrtPriceLimitX96)
		isHigher := step.sqrtPriceNextX96.Gt(sqrtPriceLimitX96)

		var sqrtRatioTargetX96 *u256.Uint
		if (zeroForOne && isLower) || (!zeroForOne && isHigher) {
			sqrtRatioTargetX96 = sqrtPriceLimitX96
		} else {
			sqrtRatioTargetX96 = step.sqrtPriceNextX96
		}

		_sqrtPriceX96Str, _amountInStr, _amountOutStr, _feeAmountStr := plp.SwapMathComputeSwapStepStr(
			state.sqrtPriceX96,
			sqrtRatioTargetX96,
			state.liquidity,
			state.amountSpecifiedRemaining,
			uint64(pool.fee),
		)
		state.sqrtPriceX96 = u256.MustFromDecimal(_sqrtPriceX96Str)
		step.amountIn = u256.MustFromDecimal(_amountInStr)
		step.amountOut = u256.MustFromDecimal(_amountOutStr)
		step.feeAmount = u256.MustFromDecimal(_feeAmountStr)

		amountInWithFee := i256.FromUint256(new(u256.Uint).Add(step.amountIn, step.feeAmount))
		if exactInput {
			state.amountSpecifiedRemaining = i256.Zero().Sub(state.amountSpecifiedRemaining, amountInWithFee)
			state.amountCalculated = i256.Zero().Sub(state.amountCalculated, i256.FromUint256(step.amountOut))
		} else {
			state.amountSpecifiedRemaining = i256.Zero().Add(state.amountSpecifiedRemaining, i256.FromUint256(step.amountOut))
			state.amountCalculated = i256.Zero().Add(state.amountCalculated, amountInWithFee)
		}

		// if the protocol fee is on, calculate how much is owed, decrement feeAmount, and increment protocolFee
		if cache.feeProtocol > 0 {
			delta := new(u256.Uint).Div(step.feeAmount, u256.NewUint(uint64(cache.feeProtocol)))
			step.feeAmount = new(u256.Uint).Sub(step.feeAmount, delta)
			state.protocolFee = new(u256.Uint).Add(state.protocolFee, delta)
		}

		// update global fee tracker
		if state.liquidity.Gt(u256.Zero()) {
			update := u256.MulDiv(step.feeAmount, u256.MustFromDecimal(consts.Q128), state.liquidity)
			state.feeGrowthGlobalX128 = new(u256.Uint).Add(state.feeGrowthGlobalX128, update)
		}

		// shift tick if we reached the next price
		if state.sqrtPriceX96.Eq(step.sqrtPriceNextX96) {
			// if the tick is initialized, run the tick transition
			if step.initialized {
				var fee0, fee1 *u256.Uint

				// check for the placeholder value, which we replace with the actual value the first time the swap crosses an initialized tick
				if zeroForOne {
					fee0 = state.feeGrowthGlobalX128
					fee1 = pool.feeGrowthGlobal1X128
				} else {
					fee0 = pool.feeGrowthGlobal0X128
					fee1 = state.feeGrowthGlobalX128
				}

				liquidityNet := pool.tickCross(
					step.tickNext,
					fee0,
					fee1,
				)

				// if we're moving leftward, we interpret liquidityNet as the opposite sign
				if zeroForOne {
					liquidityNet = i256.Zero().Neg(liquidityNet)
				}

				state.liquidity = liquidityMathAddDelta(state.liquidity, liquidityNet)
			}

			if zeroForOne {
				state.tick = step.tickNext - 1
			} else {
				state.tick = step.tickNext
			}
		} else if !(state.sqrtPriceX96.Eq(step.sqrtPriceStartX96)) {
			// recompute unless we're on a lower tick boundary (i.e. already transitioned ticks), and haven't moved
			state.tick = common.TickMathGetTickAtSqrtRatio(state.sqrtPriceX96)
		}
	}
	// END LOOP

	// update pool sqrtPrice
	pool.slot0.sqrtPriceX96 = state.sqrtPriceX96

	// update tick if it changed
	if state.tick != slot0Start.tick {
		pool.slot0.tick = state.tick
	}

	// update liquidity if it changed
	if !(cache.liquidityStart.Eq(state.liquidity)) {
		pool.liquidity = state.liquidity
	}

	// update fee growth global and, if necessary, protocol fees
	// overflow is acceptable, protocol has to withdraw before it hits MAX_UINT256 fees
	if zeroForOne {
		pool.feeGrowthGlobal0X128 = state.feeGrowthGlobalX128
		if state.protocolFee.Gt(u256.Zero()) {
			pool.protocolFees.token0 = new(u256.Uint).Add(pool.protocolFees.token0, state.protocolFee)
		}
	} else {
		pool.feeGrowthGlobal1X128 = state.feeGrowthGlobalX128
		if state.protocolFee.Gt(u256.Zero()) {
			pool.protocolFees.token1 = new(u256.Uint).Add(pool.protocolFees.token1, state.protocolFee)
		}
	}

	var amount0, amount1 *i256.Int
	if zeroForOne == exactInput {
		amount0 = i256.Zero().Sub(amountSpecified, state.amountSpecifiedRemaining)
		amount1 = state.amountCalculated
	} else {
		amount0 = state.amountCalculated
		amount1 = i256.Zero().Sub(amountSpecified, state.amountSpecifiedRemaining)
	}

	// actual swap
	if zeroForOne {
		// payer > POOL
		pool.transferFromAndVerify(payer, consts.POOL_ADDR, pool.token0Path, amount0, true)

		// POOL > recipient
		pool.transferAndVerify(recipient, pool.token1Path, amount1, false)

	} else {
		// payer > POOL
		pool.transferFromAndVerify(payer, consts.POOL_ADDR, pool.token1Path, amount1, false)

		// POOL > recipient
		pool.transferAndVerify(recipient, pool.token0Path, amount0, true)

	}

	pool.slot0.unlocked = true
	return amount0.ToString(), amount1.ToString()
}
  • The SwapRoute function in the router contract swap input <-> output tokens by calling the pool contract’s Swap()
    -- Currently, SwapRoute can route up to 21 different pools to find and provide the best ratio for the input <-> output tokens.
    -- So once it reaches the above point, the current VM cycle limit can be exceeded.
// SwapRoute swaps the input token to the output token and returns the result amount
// If swapType is EXACT_IN, it returns the amount of output token ≈ amount of user to receive
// If swapType is EXACT_OUT, it returns the amount of input token ≈ amount of user to pay
//
// Panics if any of the following conditions are met:
// - amountSpecified is zero or is not numeric
// - swapType is not EXACT_IN or EXACT_OUT
// - length of route and quotes are not the same
// - length of routes is not 1 ~ 7
// - sum of quotes is not 100
// - number of hops is not 1 ~ 3
// - too many token spend or too few token received
func SwapRoute(
	inputToken string,
	outputToken string,
	_amountSpecified string, // int256
	swapType string,
	strRouteArr string, // []string
	quoteArr string, // []int
	_tokenAmountLimit string, // uint256
) (string, string) { // tokneIn, tokenOut
	if common.GetLimitCaller() {
		isUserCalled := std.PrevRealm().PkgPath() == ""
		if !isUserCalled {
			panic("[ROUTER] router.gno__SwapRoute() || only user can call this function")
		}
	}

	amountSpecified := i256.MustFromDecimal(_amountSpecified)
	if amountSpecified.IsZero() {
		panic("[ROUTER] router.gno__SwapRoute() || amountSpecified == 0")
	}
	if amountSpecified.IsNeg() {
		panic("[ROUTER] router.gno__SwapRoute() || amountSpecified < 0")
	}

	tokenAmountLimit := u256.MustFromDecimal(_tokenAmountLimit)

	switch swapType {
	case "EXACT_IN":
		// do nothing
	case "EXACT_OUT":
		amountSpecified = i256.Zero().Neg(amountSpecified)
	default:
		panic("[ROUTER] router.gno__SwapRoute() || unknown swapType")
	}

	// check route length ( should be 1 ~ 7 )
	routes := strings.Split(strRouteArr, ",")
	isValidRouteLength := (1 <= len(routes)) && (len(routes) <= 7)
	if !isValidRouteLength {
		panic(ufmt.Sprintf("[ROUTER] router.gno__SwapRoute() || len(routes) should be 1 ~ 7 (len(routes)[%d])", len(routes)))
	}

	// check if routes length and quotes length are same
	quotes := strings.Split(quoteArr, ",")
	if len(routes) != len(quotes) {
		panic(ufmt.Sprintf("[ROUTER] router.gno__SwapRoute() || len(routes[%d]) != len(quotes[%d])", len(routes), len(quotes)))
	}

	// check if quotes are up to 100%
	quotesSum := int64(0)
	for _, quote := range quotes {
		intQuote, _ := strconv.Atoi(quote)
		quotesSum += int64(intQuote)
	}
	if quotesSum != 100 {
		panic(ufmt.Sprintf("[ROUTER] router.gno__SwapRoute() || quotesSum != 100 (quotesSum)[%d]", quotesSum))
	}

	// if input is gnot, wrap it
	userOldWugnotBalance := uint64(0)
	if inputToken == consts.GNOT {
		sent := std.GetOrigSend()
		ugnotSentByUser := uint64(sent.AmountOf("ugnot"))

		wrap(ugnotSentByUser)
		userOldWugnotBalance = wugnot.BalanceOf(a2u(std.GetOrigCaller()))
	} else if outputToken == consts.GNOT { // if output is gnot unwrap later (save user's current wugnot balance)
		userOldWugnotBalance = wugnot.BalanceOf(a2u(std.GetOrigCaller()))
	}

	resultAmountIn := u256.Zero()
	resultAmountOut := u256.Zero()

	for i, route := range routes {
		numHops := strings.Count(route, "*POOL*") + 1
		quote, _ := strconv.Atoi(quotes[i])

		if numHops < 1 || numHops > 3 {
			panic(ufmt.Sprintf("[ROUTER] router.gno__SwapRoute() || numHops should be 1 ~ 3 (numHops)[%d]", numHops))
		}

		toSwap := i256.Zero().Mul(amountSpecified, i256.NewInt(int64(quote)))
		toSwap = toSwap.Div(toSwap, i256.NewInt(100))

		if numHops == 1 { // SINGLE
			amountIn, amountOut := handleSingleSwap(route, toSwap, false)
			resultAmountIn = new(u256.Uint).Add(resultAmountIn, amountIn)
			resultAmountOut = new(u256.Uint).Add(resultAmountOut, amountOut)
		} else if numHops == 2 || numHops == 3 { // MULTI
			amountIn, amountOut := handleMultiSwap(swapType, route, numHops, toSwap, false)
			resultAmountIn = new(u256.Uint).Add(resultAmountIn, amountIn)
			resultAmountOut = new(u256.Uint).Add(resultAmountOut, amountOut)
		} else {
			panic("[ROUTER] router.gno__SwapRoute() || numHops should be 1 ~ 3")
		}
	}

	// PROTOCOL FEE
	resultAmountOut = handleProtocolFee(outputToken, resultAmountOut, false)

	// UNWRAP IF NECESSARY
	// if input was gnot, refund left over wugnot
	if inputToken == consts.GNOT {
		userNewWugnotBalance := wugnot.BalanceOf(a2u(std.GetOrigCaller()))
		unwrap(userNewWugnotBalance)
	} else if outputToken == consts.GNOT { // if output was gnot, unwrap result
		userNewWugnotBalance := wugnot.BalanceOf(a2u(std.GetOrigCaller()))
		userRecvWugnot := uint64(userNewWugnotBalance - userOldWugnotBalance) // received wugnot
		unwrap(userRecvWugnot)
	}

	if swapType == "EXACT_IN" {
		if !(tokenAmountLimit.Lte(resultAmountOut)) {
			panic(ufmt.Sprintf("[ROUTER] router.gno__SwapRoute() || too few received for user (expected minimum received:%s, actual received:%s)", _tokenAmountLimit, resultAmountOut.ToString()))
		}
	} else { // EXACT_OUT
		if !(resultAmountIn.Lte(tokenAmountLimit)) {
			panic(ufmt.Sprintf("[ROUTER] router.gno__SwapRoute() || too much spend for user (expected maximum spend:%s, actual spend:%s)", _tokenAmountLimit, resultAmountIn.ToString()))
		}
	}
	//return resultAmountIn.ToString(), resultAmountOut.ToString()

	intAmountOut := i256.FromUint256(resultAmountOut)
	return resultAmountIn.ToString(), i256.Zero().Neg(intAmountOut).ToString()
}

@Kouteki Kouteki added this to the 🏗4️⃣ test4.gno.land milestone May 10, 2024
@zivkovicmilos
Copy link
Member

@r3v4s can you open an issue for number 3? It's very disturbing, I'm not sure what package paths have to do with gas usage

@r3v4s
Copy link
Contributor Author

r3v4s commented May 13, 2024

@r3v4s can you open an issue for number 3? It's very disturbing, I'm not sure what package paths have to do with gas usage

Got it. (UPDATED // Number 3 Issue: #2083)

How about deploying uint256 spends too much gas? Do you want me to open separate issue for this or just leave it here.

@zivkovicmilos zivkovicmilos merged commit 155aba4 into gnolang:master May 15, 2024
162 checks passed
leohhhn pushed a commit to leohhhn/gno that referenced this pull request May 21, 2024
…g#2065)

closes gnolang#1788 

similar closed pr gnolang#1901 

This pr bumps 2 maximum limits
1. max block gas
2. max vm cycle

AFAIK, to use amount of increased gas, vm cycle also need to be bump
too.

This is temporarily fix pr until we have clear ways to measure gas usage
and vm cycle.

<!-- please provide a detailed description of the changes made in this
pull request. -->

<details><summary>Contributors' checklist...</summary>

- [x] Added new tests, or not needed, or not feasible
- [x] Provided an example (e.g. screenshot) to aid review or the PR is
self-explanatory
- [x] Updated the official documentation or not needed
- [x] No breaking changes were made, or a `BREAKING CHANGE: xxx` message
was included in the description
- [x] Added references to related issues and PRs
- [ ] Provided any useful hints for running manual tests
- [] Added new benchmarks to [generated
graphs](https://gnoland.github.io/benchmarks), if any. More info
[here](https://github.com/gnolang/gno/blob/master/.benchmarks/README.md).
</details>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📦 🌐 tendermint v2 Issues or PRs tm2 related 📦 ⛰️ gno.land Issues or PRs gno.land package related 🧾 package/realm Tag used for new Realms or Packages.
Projects
Status: Done
Status: Done
Development

Successfully merging this pull request may close these issues.

Increase tm2 max_block limits
5 participants