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

Protorev smarter logic and more testing #4181

Merged
merged 23 commits into from
Feb 16, 2023

Conversation

NotJeremyLiu
Copy link
Contributor

@NotJeremyLiu NotJeremyLiu commented Jan 31, 2023

What is the purpose of the change

This PR adds more testing to x/protorev and improves the arb calculation logic to be more efficient.

Brief Changelog

  • Given a route, we now calculate if the minAmountIn is profitable as a pre-check. If minAmountIn is not profitable, then the route is not profitable and we don't run the full binary search algo. This will make Protorev take much less compute time in production compared to its previous design (which would run the full binary search even when a route has no-arb). We only increment the pool point counter if the trade is profitable, thereby saving us compute for swaps that create no backrunning opportunity.
  • Given a route, we now calculate if the maxAmountIn and maxAmountIn + 1 profit is increasing as a pre-check and increase the range if so. If the profit from maxInputIn is still increasing, this means that we need to increase the range to capture the max profit.
  • Adds tests that verifies the limit/point system works properly, >3 pool routes work, and other denom besides osmo/atom work.
  • Deprecate hot routes from being included in genesis state for simplicity.
  • Add E2E testing that
    • Checks that the module is correctly init on start
    • Checks that the module can correctly spot a back running opportunity after a swap
    • Checks that the module executes a profitable cyclic arbitrage trade
    • Checks that the module did not mint or alter the supplies from the bank
  • Add support for gamm swap msgs
  • Deprecate atom from the list of base denoms that are used on genesis for route creation

Testing and Verifying

  • All previous tests pass with new changes
  • Added a test to ensure range extension works properly
  • Added a test for swapExactAmountOut
  • Added benchmark testing
  • Added doomsday testing to ensure point system works properly and doesn't allow protorev to keep running
  • Adds tests for generalized denoms
  • Adds a test to ensure panic catching is handled properly. This currently FAILS. This is on purpose, this PR is not to be merged until Panic Handling In MultihopEstimateOutGivenExactAmountIn borks Protorev #4180 is resolved
  • Add test for gamm msg swaps
  • Add tests for E2E (as described above)

@davidterpay davidterpay added the V:state/breaking State machine breaking PR label Jan 31, 2023
@NotJeremyLiu NotJeremyLiu requested a review from bpiv400 February 1, 2023 14:27
@davidterpay davidterpay mentioned this pull request Feb 3, 2023
8 tasks
@davidterpay davidterpay force-pushed the protorev-smarter-logic-and-more-testing branch from 5ff4ed4 to 8a27d62 Compare February 13, 2023 17:14
@NotJeremyLiu NotJeremyLiu marked this pull request as ready for review February 14, 2023 15:02
Copy link
Contributor Author

@NotJeremyLiu NotJeremyLiu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed, left 1 comment about the changes made. Going to fix up the doomsday test given the new point refund logic.

if err != nil {
return sdk.Coin{}, sdk.ZeroInt(), err
} else if minInProfit.LTE(sdk.ZeroInt()) {
return sdk.Coin{}, sdk.ZeroInt(), fmt.Errorf("no profit for route %d", route.Route.PoolIds())
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I imagine a majority of small swaps will result in no profits, so this will throw an error often. Feels kinda like we want to view no profit in a route as a non-error. maybe we log in some other way if we want to log?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Resolved in ad5ea73

@NotJeremyLiu
Copy link
Contributor Author

NotJeremyLiu commented Feb 14, 2023

Another thing I noticed is that we still have a hardcoded denom for atom:

var AtomDenomination string = "ibc/27394FB092D2ECCD56123C74F36E4C1F926001CEADA9CA97EA622B25F41E5EB2"

This is no longer used in protorev itself, but used throughout our test suite. Is there a way for us to move this easily to only be for test suite and not in any of the core module files (to avoid changing all the tests that reference it, but not have any unnecessary code in core module files).

I think this is a non-issue though

@davidterpay
Copy link
Contributor

Another thing I noticed is that we still have a hardcoded denom for atom:

var AtomDenomination string = "ibc/27394FB092D2ECCD56123C74F36E4C1F926001CEADA9CA97EA622B25F41E5EB2"

This is no longer used in protorev itself, but used throughout our test suite. Is there a way for us to move this easily to only be for test suite and not in any of the core module files (to avoid changing all the tests that reference it, but not have any unnecessary code in core module files).

I think this is a non-issue though

Update, I deprecated it from the module.

Comment on lines -14 to -15
// Hot routes that are configured on genesis
repeated TokenPairArbRoutes token_pairs = 2 [ (gogoproto.nullable) = false ];
Copy link
Contributor

@davidterpay davidterpay Feb 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for reviewers: deprecating the use of hot routes in genesis since we will not be using it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hrm, I feel like it should be added back (in a subsequent non-release affecting PR), so that ExportGenesis -> ImportGenesis still works. (So state export based testnets for instance can use this)

The ideal is that for state' = import(export(state)), that state' is effectively the same as state.

MinGasPrice = "0.000"
IbcSendAmount = 3300000000
ValidatorWalletName = "val"
// chainA
ChainAID = "osmo-test-a"
OsmoBalanceA = 200000000000
OsmoBalanceA = 20000000000000
Copy link
Contributor

@davidterpay davidterpay Feb 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for reviewers: osmo balance being increased so that the pools that protorev testing creates have enough funds

Comment on lines +66 to +71
UstBalanceA = 500000000000000
LuncBalanceA = 500000000000000
Copy link
Contributor

@davidterpay davidterpay Feb 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for reviewers: denoms used in protorev testing

@NotJeremyLiu NotJeremyLiu requested review from a team and ValarDragon February 15, 2023 23:17
@@ -52,24 +52,29 @@ const (
AtomDenom = "uatom"
OsmoIBCDenom = "ibc/ED07A3391A112B175915CD8FAF43A2DA8E4790EDE12566649D0C2F97716B8518"
StakeIBCDenom = "ibc/C053D637CCA2A2BA030E2C5EE1B28A16F71CCB0E45E8BE52766DC1B241B7787"
UstIBCDenom = "ibc/BE1BB42D4BE3C30D50B68D7C41DB4DFCE9678E8EF8C539F6E6A9345048894FCC"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @davidterpay . Please manually backport just this file to v14.x branch.

Once merged, please ping me, and I will give you an image tag that will need to be put here:

previousVersionInitTag = "v14.x-f6813bcb-1674140667-manual"

We do this to be able to build genesis and config files from v14.x to start up Osmosis containers of that version and perform the upgrade to the next (v15) version in your branch.

var StepSize = sdk.NewInt(1_000_000)
// ExtendedMaxInputAmount is the upper bound index for finding the optimal in amount
// when determining route profitability for an arb that's above the default range (2 ^ 17) = 131,072
var ExtendedMaxInputAmount = sdk.NewInt(131_072)

// Max iterations for binary search (log2(16_384) = 14)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// Max iterations for binary search (log2(16_384) = 14)
// Max iterations for binary search (log2(131_072) = 17)


for i := 0; i < b.N; i++ {
b.StartTimer()
suite.App.ProtoRevKeeper.UpdatePools(suite.Ctx)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs enough pools setup to be meaningful, right?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Atm, the test suite sets up ~50 pools. I attached the benchmark results to the comment. if this scales linearly and we assume mainnet has around ~1000 pools, the execution time of UpdatePools should be around 5 ms in total.
Screenshot 2023-02-16 at 12 27 14 PM

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah awesome! Didn't realize this was the same setup test with everything else, that makes sense!

NotJeremyLiu and others added 12 commits February 16, 2023 11:19
Added:
- 4 pool mainnet route test
- 2 pool mainnet route test
- Non-osmo/atom denom test
- Currently panics due to solveCFMMBinarySearchMulti in stableswap calculation
- Tests that the per tx limit works properly
- Tests that the per block limit works properly
- Adds a pre-check before the binary search that tells us if we need to run the binary search at all
- Reduces overall computation/time by avoiding binary searching profit amounts over routes without any profit opportunity
- Added "test/3" denom to test non-osmo/atom denoms in previous PR
- didn't notice it broke one test that expected us to only have 2 denoms, so adding "test/3" here so everything passes
- This implements a way for us to have a binary search bound that can increase in size to account for large trades above the default range
- I believe this is the minimum-amount-of-new-code approach, but may not be the optimal approach (what this method avoids is having to save the amount in or amount out of the original swap, and then backtracking/converting that amount to the base denom for an arb, and creating bounds off of that)
- Doubling isn't the wanted behavior (only wanted if lower bound is 1), just increasing bound by the same range size is wanted (so adding MaxInputAmount achieves this)
- Increases max iterations to 17 to allow for situation when we need to increase the upper bound
- Add new ExtendedMaxInputAmount variable for us to use as this new max bound range
- Replace bound changing logic from iteratively changing the range to immediately giving our max range
- Tests binary search range extension logic works properly
NotJeremyLiu and others added 6 commits February 16, 2023 11:19
- This currently fails, this is on purpose so we don't merge the PR and think we are good until this passes and the panics are handled properly
- Makes pool 41 reserves to have an arb opportunity in the doomsday routes
- Changes tx limit and block limit specifically for doomsday testing
panic(err)
}

// Update the pools on genesis
if err := k.UpdatePools(ctx); err != nil {
// Setting the admin account by default to a trusted address
Copy link
Member

@ValarDragon ValarDragon Feb 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we document the source of this address? (Perhaps link to the commonwealth post once it exists)

(Lets make a followup issue for this)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, will do. Just for reference this is an account we created and are storing on a ledger. We will use it to enable authz (which we have already test) for a different account so that the admin keys are safely stored.

@davidterpay davidterpay force-pushed the protorev-smarter-logic-and-more-testing branch from eb19820 to 450fd52 Compare February 16, 2023 16:55
Comment on lines +789 to +800
{ // Pool 41 - Used for doomsday testing
initialLiquidity: sdk.NewCoins(
sdk.NewCoin("usdc", sdk.NewInt(1000000000000000)),
sdk.NewCoin("usdt", sdk.NewInt(1000000000000000)),
sdk.NewCoin("busd", sdk.NewInt(2000000000000000)),
),
poolParams: stableswap.PoolParams{
SwapFee: sdk.NewDecWithPrec(1, 4),
ExitFee: sdk.NewDecWithPrec(0, 2),
},
scalingFactors: []uint64{1, 1, 1},
},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding all these pools to whats protorev configured in the tests!

inputCoin, profit, err := k.FindMaxProfitForRoute(ctx, route, inputDenom)
if err != nil {
k.Logger(ctx).Error("Error finding max profit for route: ", err)
// Get the total number of pool points that can be consumed in this transaction
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice job on all the code comments in this file, quite easy to follow the logic :)

Comment on lines +117 to +120
// If a cyclic arb exists with an optimal amount in above our minimum amount in,
// then inputting the minimum amount in will result in a profit. So we check for that first.
// If there is no profit, then we can return early and not run the binary search.
_, minInProfit, err := k.EstimateMultihopProfit(ctx, inputDenom, curLeft.Mul(route.StepSize), route.Route)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

Copy link
Member

@ValarDragon ValarDragon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice job on this! LGTM to merge!

@ValarDragon ValarDragon added the A:backport/v15.x backport patches to v15.x branch label Feb 16, 2023
@ValarDragon ValarDragon merged commit 23b13a1 into main Feb 16, 2023
@ValarDragon ValarDragon deleted the protorev-smarter-logic-and-more-testing branch February 16, 2023 18:21
mergify bot pushed a commit that referenced this pull request Feb 16, 2023
* Add generalized tests

Added:
- 4 pool mainnet route test
- 2 pool mainnet route test
- Non-osmo/atom denom test

* Add stableswap doomsday test

- Currently panics due to solveCFMMBinarySearchMulti in stableswap calculation

* Add tests for pool point limits

- Tests that the per tx limit works properly
- Tests that the per block limit works properly

* Add pre-check before binary search

- Adds a pre-check before the binary search that tells us if we need to run the binary search at all
- Reduces overall computation/time by avoiding binary searching profit amounts over routes without any profit opportunity

* Add denom to make test pass

- Added "test/3" denom to test non-osmo/atom denoms in previous PR
- didn't notice it broke one test that expected us to only have 2 denoms, so adding "test/3" here so everything passes

* Implement minimum change to have smarter binary search bounds

- This implements a way for us to have a binary search bound that can increase in size to account for large trades above the default range
- I believe this is the minimum-amount-of-new-code approach, but may not be the optimal approach (what this method avoids is having to save the amount in or amount out of the original swap, and then backtracking/converting that amount to the base denom for an arb, and creating bounds off of that)

* Switch range increasing logic

- Doubling isn't the wanted behavior (only wanted if lower bound is 1), just increasing bound by the same range size is wanted (so adding MaxInputAmount achieves this)

* Add logic to extend search bounds when finding optimal amount in

- Increases max iterations to 17 to allow for situation when we need to increase the upper bound
- Add new ExtendedMaxInputAmount variable for us to use as this new max bound range
- Replace bound changing logic from iteratively changing the range to immediately giving our max range

* Move range extension into it's own helper function

* basic benchmark testing for posthandler and epoch hook

* Add SwapAmountOut Test

* Add extended range test

- Tests binary search range extension logic works properly

* Add panic catching test

- This currently fails, this is on purpose so we don't merge the PR and think we are good until this passes and the panics are handled properly

* dynamic step size

* pool points only incremented if profitable

* adding sanity checks for pool point calcs, nits

* Update doomsday testing accounting for refund system

- Makes pool 41 reserves to have an arb opportunity in the doomsday routes
- Changes tx limit and block limit specifically for doomsday testing

* Return nil for no profit opportunity

* adding E2E, removing atom as a base denom, more testing for post handler

* find max profit test fix

* nit

* backporting version tag to 14.x

* comment update for protorev admin account

---------

Co-authored-by: David Terpay <[email protected]>
(cherry picked from commit 23b13a1)

# Conflicts:
#	tests/e2e/configurer/chain/queries.go
ValarDragon added a commit that referenced this pull request Feb 17, 2023
* Protorev smarter logic and more testing (#4181)

* Add generalized tests

Added:
- 4 pool mainnet route test
- 2 pool mainnet route test
- Non-osmo/atom denom test

* Add stableswap doomsday test

- Currently panics due to solveCFMMBinarySearchMulti in stableswap calculation

* Add tests for pool point limits

- Tests that the per tx limit works properly
- Tests that the per block limit works properly

* Add pre-check before binary search

- Adds a pre-check before the binary search that tells us if we need to run the binary search at all
- Reduces overall computation/time by avoiding binary searching profit amounts over routes without any profit opportunity

* Add denom to make test pass

- Added "test/3" denom to test non-osmo/atom denoms in previous PR
- didn't notice it broke one test that expected us to only have 2 denoms, so adding "test/3" here so everything passes

* Implement minimum change to have smarter binary search bounds

- This implements a way for us to have a binary search bound that can increase in size to account for large trades above the default range
- I believe this is the minimum-amount-of-new-code approach, but may not be the optimal approach (what this method avoids is having to save the amount in or amount out of the original swap, and then backtracking/converting that amount to the base denom for an arb, and creating bounds off of that)

* Switch range increasing logic

- Doubling isn't the wanted behavior (only wanted if lower bound is 1), just increasing bound by the same range size is wanted (so adding MaxInputAmount achieves this)

* Add logic to extend search bounds when finding optimal amount in

- Increases max iterations to 17 to allow for situation when we need to increase the upper bound
- Add new ExtendedMaxInputAmount variable for us to use as this new max bound range
- Replace bound changing logic from iteratively changing the range to immediately giving our max range

* Move range extension into it's own helper function

* basic benchmark testing for posthandler and epoch hook

* Add SwapAmountOut Test

* Add extended range test

- Tests binary search range extension logic works properly

* Add panic catching test

- This currently fails, this is on purpose so we don't merge the PR and think we are good until this passes and the panics are handled properly

* dynamic step size

* pool points only incremented if profitable

* adding sanity checks for pool point calcs, nits

* Update doomsday testing accounting for refund system

- Makes pool 41 reserves to have an arb opportunity in the doomsday routes
- Changes tx limit and block limit specifically for doomsday testing

* Return nil for no profit opportunity

* adding E2E, removing atom as a base denom, more testing for post handler

* find max profit test fix

* nit

* backporting version tag to 14.x

* comment update for protorev admin account

---------

Co-authored-by: David Terpay <[email protected]>
(cherry picked from commit 23b13a1)

# Conflicts:
#	tests/e2e/configurer/chain/queries.go

* Fix merge conflict

---------

Co-authored-by: Jeremy Liu <[email protected]>
Co-authored-by: Dev Ojha <[email protected]>
pysel pushed a commit that referenced this pull request Feb 18, 2023
* Add generalized tests

Added:
- 4 pool mainnet route test
- 2 pool mainnet route test
- Non-osmo/atom denom test

* Add stableswap doomsday test

- Currently panics due to solveCFMMBinarySearchMulti in stableswap calculation

* Add tests for pool point limits

- Tests that the per tx limit works properly
- Tests that the per block limit works properly

* Add pre-check before binary search

- Adds a pre-check before the binary search that tells us if we need to run the binary search at all
- Reduces overall computation/time by avoiding binary searching profit amounts over routes without any profit opportunity

* Add denom to make test pass

- Added "test/3" denom to test non-osmo/atom denoms in previous PR
- didn't notice it broke one test that expected us to only have 2 denoms, so adding "test/3" here so everything passes

* Implement minimum change to have smarter binary search bounds

- This implements a way for us to have a binary search bound that can increase in size to account for large trades above the default range
- I believe this is the minimum-amount-of-new-code approach, but may not be the optimal approach (what this method avoids is having to save the amount in or amount out of the original swap, and then backtracking/converting that amount to the base denom for an arb, and creating bounds off of that)

* Switch range increasing logic

- Doubling isn't the wanted behavior (only wanted if lower bound is 1), just increasing bound by the same range size is wanted (so adding MaxInputAmount achieves this)

* Add logic to extend search bounds when finding optimal amount in

- Increases max iterations to 17 to allow for situation when we need to increase the upper bound
- Add new ExtendedMaxInputAmount variable for us to use as this new max bound range
- Replace bound changing logic from iteratively changing the range to immediately giving our max range

* Move range extension into it's own helper function

* basic benchmark testing for posthandler and epoch hook

* Add SwapAmountOut Test

* Add extended range test

- Tests binary search range extension logic works properly

* Add panic catching test

- This currently fails, this is on purpose so we don't merge the PR and think we are good until this passes and the panics are handled properly

* dynamic step size

* pool points only incremented if profitable

* adding sanity checks for pool point calcs, nits

* Update doomsday testing accounting for refund system

- Makes pool 41 reserves to have an arb opportunity in the doomsday routes
- Changes tx limit and block limit specifically for doomsday testing

* Return nil for no profit opportunity

* adding E2E, removing atom as a base denom, more testing for post handler

* find max profit test fix

* nit

* backporting version tag to 14.x

* comment update for protorev admin account

---------

Co-authored-by: David Terpay <[email protected]>
pysel pushed a commit that referenced this pull request Feb 21, 2023
* Add generalized tests

Added:
- 4 pool mainnet route test
- 2 pool mainnet route test
- Non-osmo/atom denom test

* Add stableswap doomsday test

- Currently panics due to solveCFMMBinarySearchMulti in stableswap calculation

* Add tests for pool point limits

- Tests that the per tx limit works properly
- Tests that the per block limit works properly

* Add pre-check before binary search

- Adds a pre-check before the binary search that tells us if we need to run the binary search at all
- Reduces overall computation/time by avoiding binary searching profit amounts over routes without any profit opportunity

* Add denom to make test pass

- Added "test/3" denom to test non-osmo/atom denoms in previous PR
- didn't notice it broke one test that expected us to only have 2 denoms, so adding "test/3" here so everything passes

* Implement minimum change to have smarter binary search bounds

- This implements a way for us to have a binary search bound that can increase in size to account for large trades above the default range
- I believe this is the minimum-amount-of-new-code approach, but may not be the optimal approach (what this method avoids is having to save the amount in or amount out of the original swap, and then backtracking/converting that amount to the base denom for an arb, and creating bounds off of that)

* Switch range increasing logic

- Doubling isn't the wanted behavior (only wanted if lower bound is 1), just increasing bound by the same range size is wanted (so adding MaxInputAmount achieves this)

* Add logic to extend search bounds when finding optimal amount in

- Increases max iterations to 17 to allow for situation when we need to increase the upper bound
- Add new ExtendedMaxInputAmount variable for us to use as this new max bound range
- Replace bound changing logic from iteratively changing the range to immediately giving our max range

* Move range extension into it's own helper function

* basic benchmark testing for posthandler and epoch hook

* Add SwapAmountOut Test

* Add extended range test

- Tests binary search range extension logic works properly

* Add panic catching test

- This currently fails, this is on purpose so we don't merge the PR and think we are good until this passes and the panics are handled properly

* dynamic step size

* pool points only incremented if profitable

* adding sanity checks for pool point calcs, nits

* Update doomsday testing accounting for refund system

- Makes pool 41 reserves to have an arb opportunity in the doomsday routes
- Changes tx limit and block limit specifically for doomsday testing

* Return nil for no profit opportunity

* adding E2E, removing atom as a base denom, more testing for post handler

* find max profit test fix

* nit

* backporting version tag to 14.x

* comment update for protorev admin account

---------

Co-authored-by: David Terpay <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A:backport/v15.x backport patches to v15.x branch V:state/breaking State machine breaking PR
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants