internal/ethapi: optimize & clean up EstimateGas #27710

roberto-bayardo · 2023-07-12T19:34:35Z

Optimizations:

Currently if a transaction is reverting, EstimateGas will exhibit worst-case behavior and binary search up to the max gas limit (~40 state-clone + tx executions). This PR allows EstimateGas to return after only a single unconstrained execution in this scenario.
Uses the gas used from the unconstrained execution to bias the remaining binary search towards the likely solution in a simple way that doesn't impact the worst case. For a typical contract-invoking transaction, this reduces the median number of state-clone+executions from 25 to 18 (28% reduction).

Cleanup:

added & improved function + code comments
correct the EstimateGas documentation to clarify the gas limit determination is at latest block, not pending, if the blockNr is unspecified.

The performance analysis was conducted over 2300 contract-invoking transactions randomly sampled (via tx hash starting with "00") from the mempool over the past 24 hours. These txs were put through the new implementation & old implementation with the exact same state when the tx hit the mempool, and the # of executions for each recorded. The output from both calls was also compared to ensure they were equivalent. There were a few (<< 1%) instances where the values differed, upon further investigation these were due to transactions that monitor gas-remaining and attempt to adjust computation accordingly (such as batch NFT minting, example).

internal/ethapi/api.go

roberto-bayardo · 2023-07-12T20:54:20Z

internal/ethapi/api.go

-		// If the error is not nil(consensus error), it means the provided message
-		// call or transaction will never be accepted no matter how much gas it is
-		// assigned. Return the error directly, don't struggle any more.
+		if mid > lo*2 {


There is likely room for further average-case improvement by tweaking (lowering) the constant multiplier here, but this would worsen the worst-case and also introduces new boundary conditions that would need to be checked.

This is more of an acceptance that there are a much larger number of transactions on the low size, 21k - 500k gas?

bit of both I guess: 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. I guess this trick takes advantage of both facts. (FWIW EstimateGas doesn't seem to be called for simple 21k gas transfers by wallets like MetaMask so I excluded those from the analysis)

internal/ethapi/api.go

lightclient · 2023-07-12T21:49:42Z

internal/ethapi/api.go

+	// probably don't want to return a lowest possible gas limit for these cases anyway.
+	lo = gasUsed - 1
+
+	// Binary search for the smallest gas limit that allows the tx to execute successfully.


Not really so much binary search if you adjust the mid point yeah?

It's a pure binary search after 1-2 doublings of 'lo' in the typical case, lg(n) in the worst. Wasn't sure it's worth clarifying that detail, but I certainly could if it helps.

I think it's still basically a binary search, it divides up the range of possible outcomes every iteration, and discards one of the two subranges. The trick here is that the point where the range is divided is not on on the arithmetic middle.

Some clarifying might be good, e.g. what you wrote below, something like

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 is skewed to favour the low side.

added, though at the line where the midpoint is adjusted rather than here.

lightclient · 2023-07-12T21:50:54Z

internal/ethapi/api.go

-		// If the error is not nil(consensus error), it means the provided message
-		// call or transaction will never be accepted no matter how much gas it is
-		// assigned. Return the error directly, don't struggle any more.
+		if mid > lo*2 {


This is more of an acceptance that there are a much larger number of transactions on the low size, 21k - 500k gas?

internal/ethapi/api.go

karalabe · 2023-07-13T06:58:24Z

Whilst I understand that this code might make things a bit faster, but they also make it a lot more complicated. Do we have a specific use case where we need that extra drop of speed?

karalabe · 2023-07-13T07:08:15Z

Another interesting optimization we could consider is whether it's worth it to hone in on the exact gas usage needed, or if something "close enough" is enough.

I.e. By binary searching until hi == lo + 1, we're wasting a lot of executions to get the exact to-the-gas accuracy. Most wallets will just add some random number on top to account for network conditions. And I don't think it's relevant in the grand scheme of things when trying to fit txs into a block.

Lowering to accuracy from 1 gas to 1024 gas for example would save 10 iterations of the binary search.

roberto-bayardo · 2023-07-13T13:54:16Z

Whilst I understand that this code might make things a bit faster, but they also make it a lot more complicated. Do we have a specific use case where we need that extra drop of speed?

My main goal was to improve the handling around handling tx revert, which contributed to a recent issue I encountered that ending up fixed here.

Bulk of the change to this end is around moving the unconstrained execution up front instead leaving it to the end, which I felt makes the impl more understandable.

The additional complexity I did end up adding is ~4 LOC (L1228 initialization of lo, and the the midpoint adjustment in the main loop) -- seemed worth the savings given this is a very expensive call. Figured RPC providers that make use of this would benefit.

roberto-bayardo · 2023-07-13T13:57:23Z

Another interesting optimization we could consider is whether it's worth it to hone in on the exact gas usage needed, or if something "close enough" is enough.

I.e. By binary searching until hi == lo + 1, we're wasting a lot of executions to get the exact to-the-gas accuracy. Most wallets will just add some random number on top to account for network conditions. And I don't think it's relevant in the grand scheme of things when trying to fit txs into a block.

Lowering to accuracy from 1 gas to 1024 gas for example would save 10 iterations of the binary search.

Agree that the "lowest possible" aspect of this function is probably unnecessary.

fjl · 2023-07-18T12:38:06Z

internal/ethapi/api.go

-
-// ErrorCode returns the JSON error code for a revertal.
-// See: https://github.com/ethereum/wiki/wiki/JSON-RPC-Error-Codes-Improvement-Proposal
-func (e *revertError) ErrorCode() int {


The ErrorCode and ErrorData methods are used by the RPC server and will be returned as part of the JSON-RPC response. So they need to be kept here.

ahah, restored.

ahah, restored.

Is it? I don't see it??

weird, I wonder if I undid it when I merged with upstream. let me try again.

I think I got it this time :/

fjl · 2023-07-18T12:50:40Z

It'd be nice to have the script you used to sample the transactions. That way we could also repeat your analysis.

roberto-bayardo · 2023-07-18T14:00:22Z

It'd be nice to have the script you used to sample the transactions. That way we could also repeat your analysis.

there is no standalone script unfortunately. the analysis was over logs from a hacked client which invoked both old & new implementations for each call and logged the # of iterations of both. I then added a hook in the txpool to call EstimateGas for each sampled transaction, and let it run over night before extracting the log data and dumping into a spreadsheet.

antonydenyer · 2023-07-31T12:06:24Z

There's a reasonably good test in besu that covers some funny edge cases with the 1/64th rule TestDepth.sol

    depth(1) == 45554
    depth(2) == 47387
    depth(3) == 49249
    depth(4) == 51141
    depth(5) == 53063
    depth(10) == 63139
    depth(65) == 246462

https://github.com/hyperledger/besu/blob/main/acceptance-tests/tests/src/test/java/org/hyperledger/besu/tests/acceptance/jsonrpc/EthEstimateGasAcceptanceTest.java#L54

holiman

LGTM, some minor nits, and I wonder if @fjl's comments were really addressed?

holiman · 2023-08-10T11:39:35Z

internal/ethapi/api.go

+	err = overrides.Apply(state)
+	if err != nil {


Suggested change

err = overrides.Apply(state)

if err != nil {

if err := overrides.Apply(state); err != nil {

I nkow you just lifted it up, but still...

holiman · 2023-08-10T11:49:00Z

internal/ethapi/api.go

+	// probably don't want to return a lowest possible gas limit for these cases anyway.
+	lo = gasUsed - 1
+
+	// Binary search for the smallest gas limit that allows the tx to execute successfully.


I think it's still basically a binary search, it divides up the range of possible outcomes every iteration, and discards one of the two subranges. The trick here is that the point where the range is divided is not on on the arithmetic middle.

Some clarifying might be good, e.g. what you wrote below, something like

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 is skewed to favour the low side.

fjl · 2023-08-15T12:33:31Z

My concerns were adressed. I can't really comment on the binary search thing, seems alright though.

holiman

LGTM

Optimizations: - Previously, if a transaction was reverting, EstimateGas would exhibit worst-case behavior and binary search up to the max gas limit (~40 state-clone + tx executions). This change allows EstimateGas to return after only a single unconstrained execution in this scenario. - Uses the gas used from the unconstrained execution to bias the remaining binary search towards the likely solution in a simple way that doesn't impact the worst case. For a typical contract-invoking transaction, this reduces the median number of state-clone+executions from 25 to 18 (28% reduction). Cleanup: - added & improved function + code comments - correct the EstimateGas documentation to clarify the gas limit determination is at latest block, not pending, if the blockNr is unspecified.

)" This reverts commit 8be8e98.

Conflicts: internal/ethapi/api.go: We modified call to DoCall, upstream moved it to an internal function, kept our modifications in the new place.

roberto-bayardo force-pushed the estimate-gas branch from c17c73c to 9b8e908 Compare July 12, 2023 20:49

roberto-bayardo commented Jul 12, 2023

View reviewed changes

internal/ethapi/api.go Outdated Show resolved Hide resolved

roberto-bayardo commented Jul 12, 2023

View reviewed changes

roberto-bayardo marked this pull request as ready for review July 12, 2023 20:55

lightclient reviewed Jul 12, 2023

View reviewed changes

roberto-bayardo force-pushed the estimate-gas branch 3 times, most recently from 84b4579 to b920b2b Compare July 12, 2023 22:58

roberto-bayardo requested a review from lightclient July 12, 2023 23:02

fjl requested changes Jul 18, 2023

View reviewed changes

fjl changed the title ~~optimize & clean up EstimateGas~~ internal/ethapi: optimize & clean up EstimateGas Jul 18, 2023

roberto-bayardo force-pushed the estimate-gas branch 2 times, most recently from 54f14c3 to aac3cdf Compare July 18, 2023 13:53

roberto-bayardo force-pushed the estimate-gas branch from aac3cdf to 0230a4a Compare July 18, 2023 14:02

roberto-bayardo requested a review from fjl July 18, 2023 15:51

antonydenyer mentioned this pull request Jul 31, 2023

internal/ethapi: reduce hi and low range on DoEstimateGas #27820

Closed

roberto-bayardo force-pushed the estimate-gas branch from 0230a4a to 80fd388 Compare August 8, 2023 19:58

holiman added the status:triage label Aug 10, 2023

holiman reviewed Aug 10, 2023

View reviewed changes

roberto-bayardo force-pushed the estimate-gas branch from 80fd388 to 38c489d Compare August 10, 2023 14:21

optimize & clean up EstimateGas

2e3f647

roberto-bayardo force-pushed the estimate-gas branch from 38c489d to 2e3f647 Compare August 10, 2023 14:25

roberto-bayardo requested a review from holiman August 10, 2023 14:31

fjl removed the status:triage label Aug 15, 2023

holiman added this to the 1.13.0 milestone Aug 18, 2023

holiman approved these changes Aug 18, 2023

View reviewed changes

holiman merged commit 950ccdd into ethereum:master Aug 18, 2023
1 check passed

devopsbo3 added a commit to HorizenOfficial/go-ethereum that referenced this pull request Nov 10, 2023

Revert "internal/ethapi: optimize & clean up EstimateGas (ethereum#27710

f5d6fd3

)" This reverts commit 8be8e98.

devopsbo3 added a commit to HorizenOfficial/go-ethereum that referenced this pull request Nov 10, 2023

Revert "internal/ethapi: optimize & clean up EstimateGas (ethereum#27710

49c5e5d

)" This reverts commit 8be8e98.

trinhdn2 mentioned this pull request Dec 9, 2023

Fix and optimize eth_estimateGas API BuildOnViction/victionchain#409

Closed

1 task

sduchesneau pushed a commit to streamingfast/go-ethereum that referenced this pull request Feb 26, 2024

Merge upstream PR ethereum#27710

b5a526d

Conflicts: internal/ethapi/api.go: We modified call to DoCall, upstream moved it to an internal function, kept our modifications in the new place.

c98tristan mentioned this pull request Oct 23, 2024

Fix & Update EstimateGas API BuildOnViction/victionchain#462

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

internal/ethapi: optimize & clean up EstimateGas #27710

internal/ethapi: optimize & clean up EstimateGas #27710

roberto-bayardo commented Jul 12, 2023 •

edited

Loading

roberto-bayardo Jul 12, 2023

lightclient Jul 12, 2023

roberto-bayardo Jul 12, 2023

lightclient Jul 12, 2023

roberto-bayardo Jul 12, 2023

holiman Aug 10, 2023

roberto-bayardo Aug 10, 2023 •

edited

Loading

lightclient Jul 12, 2023

karalabe commented Jul 13, 2023

karalabe commented Jul 13, 2023

roberto-bayardo commented Jul 13, 2023 •

edited

Loading

roberto-bayardo commented Jul 13, 2023

fjl Jul 18, 2023

roberto-bayardo Jul 18, 2023

holiman Aug 10, 2023

roberto-bayardo Aug 10, 2023 •

edited

Loading

roberto-bayardo Aug 10, 2023

fjl commented Jul 18, 2023

roberto-bayardo commented Jul 18, 2023 •

edited

Loading

antonydenyer commented Jul 31, 2023 •

edited

Loading

holiman left a comment

holiman Aug 10, 2023

roberto-bayardo Aug 10, 2023

holiman Aug 10, 2023

fjl commented Aug 15, 2023

holiman left a comment

	err = overrides.Apply(state)
	if err != nil {
	if err := overrides.Apply(state); err != nil {

internal/ethapi: optimize & clean up EstimateGas #27710

internal/ethapi: optimize & clean up EstimateGas #27710

Conversation

roberto-bayardo commented Jul 12, 2023 • 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

roberto-bayardo Aug 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karalabe commented Jul 13, 2023

karalabe commented Jul 13, 2023

roberto-bayardo commented Jul 13, 2023 • edited Loading

roberto-bayardo commented Jul 13, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roberto-bayardo Aug 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fjl commented Jul 18, 2023

roberto-bayardo commented Jul 18, 2023 • edited Loading

antonydenyer commented Jul 31, 2023 • edited Loading

holiman left a 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

fjl commented Aug 15, 2023

holiman left a comment

Choose a reason for hiding this comment

roberto-bayardo commented Jul 12, 2023 •

edited

Loading

roberto-bayardo Aug 10, 2023 •

edited

Loading

roberto-bayardo commented Jul 13, 2023 •

edited

Loading

roberto-bayardo Aug 10, 2023 •

edited

Loading

roberto-bayardo commented Jul 18, 2023 •

edited

Loading

antonydenyer commented Jul 31, 2023 •

edited

Loading