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

Implement Gas Exactimation #948

Open
juli opened this issue Aug 7, 2019 · 7 comments
Open

Implement Gas Exactimation #948

juli opened this issue Aug 7, 2019 · 7 comments

Comments

@juli
Copy link

juli commented Aug 7, 2019

RSKj does not implement the binary search solution implemented by geth and parity in eth_estimateGas, some transactions can fail unexpectedly with OOG.

Truffle Suite recently published a less CPU intensive solution they called Gas Exactimation

Latest implementation: https://github.com/trufflesuite/ganache-core/blob/3ec34fe972dc17a5600eb566c8c309cd5ddb38bd/lib/utils/gasEstimation.js

This issue will be more relevant if RSKj implements EIP-150 63/64 rule (#944)

@joaquinlpereyra-iov
Copy link
Contributor

The problem with our current implementation is that it does take into account gas-returning OPCODES like SUICIDE, returning the gas used, not the gas you must send.

So if you use the gas returned by eth_estimageGas, you may get OOG.

@SergioDemianLerner
Copy link
Contributor

SergioDemianLerner commented Aug 13, 2019

Also does not take into account gas refunds because of SSTORE changes.
And also gas amounts locked by CALL and the 2300 pre-send amount required by CALLs with value.

My suggestion to solve this issue is to code Gas Exactimation [but it must be analyzed for correctness, because it's written mainly for Ethereum and the 63/64 rule]:

OR, as a simpler alternative that will work 99.99% of the time (I'm trying hard to find examples where this wouldn't work):

  • Change TransactionExecutor to return not only the gas consumed but also the gas paid in advance before refunds.

  • Add 2300 to the transaction limit IF there is any CALL at non-top level that transfers value.

@SergioDemianLerner
Copy link
Contributor

PR with gas exactimation method here #990

@SergioDemianLerner
Copy link
Contributor

SergioDemianLerner commented Aug 27, 2019

I'm analyzing this issue deeper since I discovered that the previous code already skips refunds using the localCall variable when estimating gas (in a very ugly way). This is good news (the previous code works in most cases) and bad news (the previous code sucks).

@SergioDemianLerner
Copy link
Contributor

I detected another problem with estimateGas(). It does not differentiate between an OOG exception and a successful finalization. Therefore the gas limit passed to estimageGas() should always be higher than the actual gas consumed. By doing that one can differentiate the cases: returnedValue==gasLimit means OOG.

@SergioDemianLerner
Copy link
Contributor

New code in the PR ready. Tests passed and all ready to go.
I identified the simple case where the previous estimation would fail. Solidity code is this:

function () external payable { }
function callWithValue() public payable {
address(this).transfer(100);
}

The old estimator would estimate 2300 less units of gas than the required to properly execute the method callWithValue().

@pmprete
Copy link

pmprete commented Jul 23, 2020

This issue is still relevant, and is causing #1279

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants