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

Optimize gas estimation #28589

Closed
wants to merge 4 commits into from
Closed

Optimize gas estimation #28589

wants to merge 4 commits into from

Conversation

orenyomtov
Copy link
Contributor

From my tests, it reduces the number of tx simulations for each gas estimation down from 16-18 to just 2 for most transactions (actually for all the transactions that I've tried).

The gist is that after the first simulation, it optimistically tries a second simulation with gasLimit set to usedGas + gasRefund and if it succeeds, it returns that value.

This change was inspired by @karalabe's tweet:
https://twitter.com/peter_szilagyi/status/1726884606492999754

From my tests, it reduces the number of tx simulations for each gas estimation down from 16-18 to just 2 for most transactions (actually for all the transactions that I've tried).
@karalabe
Copy link
Member

karalabe commented Nov 23, 2023

Hmm, this is a very interesting idea. Would be nice to see if reducing it further works or this is the "minimum" (at least for what percentage of the txs that's the case). By reducing it further I mean as a "test", not in live code. Just to evaluate the idea better.

@orenyomtov
Copy link
Contributor Author

orenyomtov commented Nov 23, 2023

@karalabe thanks for taking a look so quickly!

From my tests (just a couple) it returns the same value as the binary search approach does.

I personally can't come up with a hypothetical scenario where it doesn't, but I can be missing some EVM internals knowledge.

@holiman
Copy link
Contributor

holiman commented Nov 24, 2023

From my tests, it reduces the number of tx simulations for each gas estimation down from 16-18 to just 2 for most transactions (actually for all the transactions that I've tried).

16-18 really? Sounds very large, but I may be wrong. The thing is, the gas estimator is not a 'balanced' binary searcher between floor and ceiling, it is skewed to stick closer to the floor, and so it should land pretty close already.

			// Most txs don't need much higher gas limit than their gas used, and most txs don't
			// require near the full block limit of gas, so the selection of where to bisect the
			// range here is skewed to favor the low side.
			mid = lo * 2

@karalabe
Copy link
Member

The 16-18 is correct FWIW, that's what the current one does.

@karalabe
Copy link
Member

Wrote up a tester that runs various gas estimation algos and their combinations on every transaction that comes in on mainnet.

The idea in this PR does capture a few transactions, reducing their estimation iterations to 2. However, most of the time it's not because it captures the refunds correctly, rather because there are no refunds. For simpler transactions, the initial gascap iteration calculates the gasUsed and lower-caps the binary search to gasUsed-1. This PR runs the check for gasUsed (since refunds == 0), so for txs without refunds, in 2 iterations it captures the correct result.

It is relatively rare that this optimization produces a hit however. I verified this by running both the early return in this PR and a modified version which does a hi-cap instead of returning. It is very rare that the early return version produces a valid estimation but the hi-capper iterates to calculate a result. That kind of means, that for non-trivial transactions, this optimization fails to pick a correct gas estimate.

My guess is that the optimization is borked by two EVM mechanisms: the 2300 gas stipend and the 63/64 gas forwarding rules. In both of these cases, the outer call needs more gas than gasUsed (with or without gasRefund). I need to check what happens if I bump the optimistic gas still further up by +2300 + 1/64. That should (TM) cover these two machanics.

@karalabe
Copy link
Member

karalabe commented Nov 24, 2023

optimisticGasLimit := (result.UsedGas + result.RefundedGas + params.CallStipend) * 64 / 63

This captures almost all transactions successfully.

@orenyomtov
Copy link
Contributor Author

Oh nice catches!

And great work creating that "test on any new TX" setup for validating these different ideas - I had the same thought.

You say it captures "almost all transactions successfully" - can you elaborate or give examples for transactions that this doesn't work for?
Do you know why it doesn't work for them?

@karalabe
Copy link
Member

Unsure, will look into them a bit more Monday. I'm cross building from arm to amd64 docker images (to deploy on our test infra) so trying every idea takes 6 minutes+, which is a very annoying way to debug.

@MariusVanDerWijden
Copy link
Member

Can this be closed after #28600 @karalabe ?

@orenyomtov
Copy link
Contributor Author

I don't think that #28600 implemented optimizations, just moved it to a separate package

@karalabe
Copy link
Member

Yeah, I wanted to do the clean move first and then mess with the optimizations when they can be reviewed alone.

@karalabe
Copy link
Member

I've cherry picked this PR onto the rest of the optimizations I've introduced in #28618, closing this, but PTAL at the new one.

@karalabe karalabe closed this Nov 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants