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

Wrong gas estimation in contract calling another contract #280

Closed
crystalin opened this issue Feb 1, 2021 · 10 comments
Closed

Wrong gas estimation in contract calling another contract #280

crystalin opened this issue Feb 1, 2021 · 10 comments
Labels
bug Something isn't working

Comments

@crystalin
Copy link
Collaborator

Description

The estimate Gas is (very) wrong when computing the needed gas when a contract calls another contract

Steps to Reproduce

Using those contract:

pragma solidity ^0.7.0;
contract Caller {
    Callee internal callee;
    uint public store;
    function someAction(address _addr, uint _number) public {
        callee = Callee(_addr);
        store = callee.addtwo(_number);
    }
}
contract Callee {
    function addtwo(uint _value) external pure returns (uint) {
        return _value + 2;
    }
}

Steps:

  1. Deploy Callee (fetch address)
  2. Deploy Caller
  3. Run someAction by passing Callee's address and a number. It will execute the function addtwo from the other contract

Expected vs. Actual Behavior

It returns 67M gas which is way over a normal expected value.

Logs, Errors or Screenshots

image

Additional Information

@crystalin crystalin added the bug Something isn't working label Feb 1, 2021
@jnaviask
Copy link
Contributor

jnaviask commented Feb 1, 2021

I believe that's what this PR was intended to fix: #269 -- try building with feature rpc_binary_search_estimate and see if it still gets it wrong.

@crystalin
Copy link
Collaborator Author

Thank you @jnaviask , I thought we had this one already included but is not. I'll double check once merged on our branch

@albertov19
Copy link

@jnaviask that fixed the issue in way - but we ran into another problem that just stalled the EVM. So the gas estimation problem still persists.

Logs from the evm trace when the problem happened:

2021-02-11 18:25:00.192  DEBUG        http.worker00 evm: Call execution using address 0x42e2…c741: Revert(Reverted)    
2021-02-11 18:25:00.192  DEBUG        http.worker00 evm: Execution Revert(Reverted) [source: 0x6be02d1d3665660d22ff9624b7be0551ee1ac91b, value: 0, gas_limit: 1000000000, actual_fee: 0]    
2021-02-11 18:25:00.268  DEBUG        http.worker00 evm: Call execution using address 0x42e2…c741: Revert(Reverted)    
2021-02-11 18:25:00.268  DEBUG        http.worker00 evm: Execution Revert(Reverted) [source: 0x6be02d1d3665660d22ff9624b7be0551ee1ac91b, value: 0, gas_limit: 1000000000, actual_fee: 0]    
2021-02-11 18:25:00.348  DEBUG        http.worker00 evm: Call execution using address 0x42e2…c741: Revert(Reverted) 

@notlesh
Copy link
Contributor

notlesh commented Feb 11, 2021

This issue is likely related: rust-ethereum/evm#8

@jnaviask
Copy link
Contributor

@jnaviask that fixed the issue in way - but we ran into another problem that just stalled the EVM. So the gas estimation problem still persists.

@albertov19 What can I do to reproduce this? Did it happen with the original contract @crystalin posted?

@albertov19
Copy link

@jnaviask No, it was with a more complex contract when deploying Uniswap V2. When swapping tokens. I don't a step by step way of explaining how to reproduce right now but in short:

  • Deploy Uniswap V2 Factory
  • Deploy WETH
  • Deploy Uniswap Router V2
  • Deploy 2 ERC20 Tokens (or 1 token and use WETH as the other)
  • Add Liquidity
  • Swap tokens (ERROR)

@notlesh
Copy link
Contributor

notlesh commented Feb 12, 2021

@jnaviask that fixed the issue in way - but we ran into another problem that just stalled the EVM. So the gas estimation problem still persists.

@albertov19 What can I do to reproduce this? Did it happen with the original contract @crystalin posted?

Edit: Sorry, I had the wrong context when I originally wrote this. The binary search approach seems to work for this trivial contract, but fails on the complex contract as @albertov19 mentioned. The rest of this applies to reproducing the original bug (without binary search enabled).

We have a PR on Moonbeam with an integration test that currently fails over this issue:

moonbeam-foundation/moonbeam#244

Reproducing it in this way should be as simple as:

  1. Build our repo (cargo build --release --all)
  2. Prepare test environment (cd moonbeam-types-bundle && npm install && cd ../tests && npm install)
  3. Run test (npm run test -- --grep "Gas Estimation")

@notlesh
Copy link
Contributor

notlesh commented Feb 12, 2021

Also note that in this case, the overestimation appears to be roughly equal to:

actual_gas_cost + (original_gas_limit / 64)

This seems to point to the l64() functionality as problematic.

@librelois
Copy link
Contributor

librelois commented Jul 22, 2021

@albertov19 I think the fix #297 should have solved your problem, could you test ? :)

@boundless-forest
Copy link
Collaborator

Very old one. Close it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

6 participants