Skip to content
This repository has been archived by the owner on Feb 26, 2024. It is now read-only.

Wrong GAS estimation #2234

Open
1 task
andreafspeziale opened this issue Jul 19, 2019 · 4 comments
Open
1 task

Wrong GAS estimation #2234

andreafspeziale opened this issue Jul 19, 2019 · 4 comments

Comments

@andreafspeziale
Copy link


Issue

Idon't know if it is the right place where report it.
My ERC20 transfer gas estimation report a wrong number.

Steps to Reproduce

MockToken.sol

pragma solidity ^0.5.10;

import "openzeppelin-solidity/contracts/token/ERC20/ERC20Detailed.sol";
import "openzeppelin-solidity/contracts/token/ERC20/ERC20Mintable.sol";

contract MockToken is ERC20Detailed, ERC20Mintable {

  constructor(
    string memory _name,
    string memory _symbol,
    uint8 _decimals,
    uint _toBeMinted
  )
    public
    ERC20Detailed(_name, _symbol, _decimals)
  {
    mint(msg.sender, _toBeMinted);
  }
}

truffle-config.js

module.exports = {
  networks: {
    development: {
      host: '127.0.0.1',
      port: 7545,
      network_id: '47',
      gas: 4700000,
      gasPrice: 6000000000,
    },
  },
  compilers: {
    solc: {
      version: '0.5.10',
      settings: {
        optimizer: {
          enabled: true,
          runs: 200,
        },
      },
    },
  },
}

test.js

const Tx = require('ethereumjs-tx').Transaction

const BigNumber = web3.utils.BN

const MockToken = artifacts.require('./Mocks/MockToken.sol')

const sender = '0xdb1b9e1708aec862fee256821702fa1906ceff67'
const senderPrivateKey = Buffer.from('a8345d27c6d41e4816163fe133daddf38298bb74c16ea5f8245727d03a5f85f8', 'hex')

contract('MockToken', (accounts) => {
  const [TokenOwner] = accounts
  const [, whoever] = accounts

  describe('estimateGas', () => {
    let MockTokenInstance

    beforeEach(async () => {
      MockTokenInstance = await MockToken.new(
        'MockToken',
        'ERC20',
        18,
        web3.utils.toWei('100'),
        { from: TokenOwner },
      )

      await MockTokenInstance.transfer(sender, web3.utils.toWei('100'), { from: TokenOwner })
    })

    it('Should complete successfully the entire flow where an etherless account pay a transaction fee in ERC20', async () => {
      // sender:
      //    - has no ETH
      //    - has 100 ERC0
      //    - wants to send 10 ERC20 to whoever
      //    - whoever sends the required eth to the sender in order to complete the transfer of the 10 ERC20

      const transferTxData = MockTokenInstance.contract.methods.transfer(whoever, web3.utils.toWei('10')).encodeABI()
      const transferGasEstimation = new BigNumber(await MockTokenInstance.transfer.estimateGas(whoever, web3.utils.toWei('10')))

      const transferTx = new Tx({
        to: MockTokenInstance.address,
        data: transferTxData,
        nonce: web3.utils.toHex(await web3.eth.getTransactionCount(sender)),
        value: web3.utils.toHex('0'),
        gasPrice: web3.utils.toHex(web3.utils.toWei('0.000005')),
        gas: web3.utils.toHex(transferGasEstimation),
      })

      // sign primary tx
      transferTx.sign(senderPrivateKey)
      const transferRawTx = transferTx.serialize().toString('hex')
      const transferCost = transferTx.getUpfrontCost()

      // whoever sends to sender the transferCost
      await web3.eth.sendTransaction({ from: whoever, to: sender, value: transferCost})
      // sender now broadcast the transferRawTx
      await web3.eth.sendSignedTransaction(transferRawTx)
      // whoever has 10 ERC20
      expect(await MockTokenInstance.balanceOf(whoever).toString()).to.equal(web3.utils.toWei('10'))
      // sender still has 0 ETH
      expect(await web3.eth.getBalance(sender)).to.equal('0')
    })
  })
})

yarn truffle compile
yarn ganache-cli -p 7545 -i 47 -l 4700000
yarn truffle test

Expected Behavior

The transfer call will execute without any problem.

Actual Results

Error: Returned error: VM Exception while processing transaction: out of gas

Environment

  • Operating System: macOsX
"dependencies": {
    "ethereumjs-tx": "2.1.0",
    "ethereumjs-util": "6.1.0",
    "ganache-cli": "6.5.0",
    "openzeppelin-solidity": "2.3",
    "truffle": "5.0.26"
  }
  • node version (node --version): v10.16.0
  • npm version (npm --version): 6.10.0

wrong-gas-estimation.zip

@andreafspeziale
Copy link
Author

andreafspeziale commented Jul 21, 2019

I've switched to "truffle": "5.0.27" and I've inserted some logs. This are some results:

truffle contract => contractInstance.methodName.estimateGas() 23998
web3 => myContract.methods.myMethod().estimateGas() => 23998
web3 => web3.eth.estimateGas => 23998
ethereumjs-tx => getBaseFee() => 23192

In any case it is not sufficient and will run into out of gas

@andreafspeziale
Copy link
Author

Hi guys I reach the correct amount of GAS needed for the above transfer call.
The correct amount is 51382. Now the test is working cool.

How I reach the correct estimation?

All the above tries were made using ganache-cli.

I've tried to run the test on a parity dev client and I noticed that the test was failing on the estimation call with a general execution error.

The same error occurs when I tried to run the estimation directly on the parity console creating the contract instance and running the estimateGas method with web3.

Writing down it manually makes me notice that the transaction object from property was missing.
Actually the entire transaction object was missing.
As soon as I added it the estimateGas method went through and it returned a reasonable value and the test was "green light".

So I only changed this line
const transferGasEstimation = new BigNumber(await MockTokenInstance.transfer.estimateGas(whoever, web3.utils.toWei('10')))
in
const transferGasEstimation = new BigNumber(await MockTokenInstance.transfer.estimateGas(whoever, web3.utils.toWei('10'), { from: sender }))

Conclusion

  • Probably this issue is only ganache related.
  • I would expect the gasEstimation failing on ganache like the "real" client did.

Feel free to close the issue or go deeper investigating it.
Thanks!

@davidmurdoch
Copy link
Member

@andreafspeziale the issue looks to be a couple of things:

  1. You aren't sending the same transaction in the gasEstimate call and the transaction itself. This may cause the estimate to change.
  2. Web3 "helps out" if you don't specify a from in a transaction by defaulting to accounts[0]. So the from in the estimateGas call ends up being accounts[0].
  3. accounts[0] can't perform this transaction, so the transaction gets reverted.
  4. ganache-core still returns an accurate estimate for calling this transaction as specified (with the wrong from). Even though the transaction reverts it is a valid transaction with an actual gas cost. This seems to be different than what parity does, which reports the estimateGas call as an "execution error" instead of returning the estimate.

Parity's behavior seems more desirable than Ganache's, so we're working on a change that will align ganache-core's estimateGas behavior more closely with Parity's; exceptional transactions (where the transaction's status === 0) will no longer return a gas estimate. If we decide to go forward with this change we'll probably release it in our next major release, as I'm currently under the impression that this change qualifies as a breaking change, and not a bugfix/patch.

@andreafspeziale
Copy link
Author

@davidmurdoch Thanks for the inputs!
The main reason was point number 2 but I didn't realise it till I tried it using parity which was failing rather giving me a wrong value.

Plus I found another interesting thing.
When you estimate the gas needed for a valid transfer of a token you get a value but for example it will be less if your balance will be 0 afte that transfer. There is any more efficient way to get a proper gas value rather than use specific functions like the parity trace feature?

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

No branches or pull requests

3 participants