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

accounts/abi/bind, internal/ethapi: binary search gas estimation #3587

Merged
merged 1 commit into from Jan 20, 2017

Conversation

Projects
None yet
5 participants
@karalabe
Copy link
Member

karalabe commented Jan 18, 2017

Gas estimation currently mostly works, but can underestimate for more funky refunds. This is because various ops (e.g. CALL) need more gas to run than they actually consume (e.g. 2300 stipend that is refunded if not used). With more intricate contract interplays, it becomes almost impossible to return a proper value to the user.

This PR swaps out the simplistic gas estimation to a binary search approach, honing in on the correct gas use. This does mean that gas estimation needs to rerun the transaction log(max-price) times to measure whether it fails or not, but it's a price paid by the transaction issuer, and it should be worth it to support proper estimates.

Fixes: #3282 #2970 #1590


Verified in both the simulator tests as well as on Ropsten:

contract FunkyGasPattern {
	string public field;

	function SetField(string value) {
		// This check will screw gas estimation! Good, good!
		if (msg.gas < 100000) {
			throw;
		}
		field = value;
	}
}

Deployed at 0xf347ee4bF34a1a4e5c7a11694669602b5324Fc56

Calling SetField is estimated as

> eth.estimateGas({from: eth.accounts[0], to: "0xf347ee4bF34a1a4e5c7a11694669602b5324Fc56", data: "0x23fcf32a0000000000000000000000000000000000000000000000000000000000000020000000000000000000000000000000000000000000000000000000000000000564756e6e6f000000000000000000000000000000000000000000000000000000"})
122418

Running with this estimated gas cost:

> eth.sendTransaction({from: eth.accounts[0], to: "0xf347ee4bF34a1a4e5c7a11694669602b5324Fc56", gas: "122418", data: "0x23fcf32a0000000000000000000000000000000000000000000000000000000000000020000000000000000000000000000000000000000000000000000000000000000564756e6e6f000000000000000000000000000000000000000000000000000000"})
"0x31912e1743c5db3a9af4739c937fa1b595277e7d445b12c977dcecf2230430da"

Results in successful transaction 0x31912e1743c5db3a9af4739c937fa1b595277e7d445b12c977dcecf2230430da

Note, actual gas usage is 42918, way below the estimated 122418!

Running with 1 gas below the estimated cost:

> eth.sendTransaction({from: eth.accounts[0], to: "0xf347ee4bF34a1a4e5c7a11694669602b5324Fc56", gas: "122417", data: "0x23fcf32a0000000000000000000000000000000000000000000000000000000000000020000000000000000000000000000000000000000000000000000000000000000564756e6e6f000000000000000000000000000000000000000000000000000000"})
"0xffb8ffbf722de204781ba9f94c7c4065edc03de7bbfb5583b859b782ee815718"

Results in a failed transaction 0xffb8ffbf722de204781ba9f94c7c4065edc03de7bbfb5583b859b782ee815718

Note, even though the end gas usage would have been 42918, the contract goes OOG with only 122417 allowance!

@mention-bot

This comment has been minimized.

Copy link

mention-bot commented Jan 18, 2017

@karalabe, thanks for your PR! By analyzing the history of the files in this pull request, we identified @fjl, @zsfelfoldi and @obscuren to be potential reviewers.

@karalabe karalabe force-pushed the karalabe:binary-search-gas-estimation branch Jan 18, 2017

@karalabe karalabe added pr:review and removed in progress labels Jan 18, 2017

@karalabe karalabe added this to the 1.5.8 milestone Jan 18, 2017

@holiman

This comment has been minimized.

Copy link
Contributor

holiman commented Jan 18, 2017

Hm. That's not quite accurate ... It doesn't go out of gas, you're triggering a bad jump put there by Solidity

@holiman

This comment has been minimized.

Copy link
Contributor

holiman commented Jan 18, 2017

But I agree that this approach is probably better in general.

@karalabe

This comment has been minimized.

Copy link
Member Author

karalabe commented Jan 18, 2017

@holiman Well yeah, but it simulates a contract requiring more gas to run than actually consumed, which is the corner case estimated badly.

internal/ethapi/api.go Outdated
// Otherwise assume the transaction succeeded, lower the gas limit
hi = mid
}
return (*hexutil.Big)(big.NewInt(int64(hi))), nil

This comment has been minimized.

Copy link
@karalabe

karalabe Jan 20, 2017

Author Member

new(big.Int).SetUint64()

@karalabe karalabe force-pushed the karalabe:binary-search-gas-estimation branch to 8d52e51 Jan 20, 2017

@karalabe

This comment has been minimized.

Copy link
Member Author

karalabe commented Jan 20, 2017

@fjl PTAL

@fjl fjl merged commit 682875a into ethereum:master Jan 20, 2017

3 of 4 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
ci/circleci Your tests passed on CircleCI!
Details
commit-message-check/gitcop All commit messages are valid
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@fjl fjl removed the pr:review label Jan 20, 2017

farazdagi added a commit to status-im/go-ethereum that referenced this pull request Feb 23, 2017

accounts/abi/bind, internal/ethapi: binary search gas estimation (eth…
…ereum#3587)

Gas estimation currently mostly works, but can underestimate for more funky
refunds. This is because various ops (e.g. CALL) need more gas to run than they
actually consume (e.g. 2300 stipend that is refunded if not used). With more
intricate contract interplays, it becomes almost impossible to return a proper
value to the user.

This commit swaps out the simplistic gas estimation to a binary search approach,
honing in on the correct gas use. This does mean that gas estimation needs to
rerun the transaction log(max-price) times to measure whether it fails or not,
but it's a price paid by the transaction issuer, and it should be worth it to
support proper estimates.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.