internal/ethapi: eth_estimateGas should take block number

[kumavis]

System information

Geth version: Geth/v1.4.3-stable/linux/go1.5.1
OS & Version: Linux

Summary

behaviour of eth_estimateGas changed

1. can no longer estimate gas of a simple value transfer tx. while a simple value transfer has a fixed cost, i normally just dump tx params here for any kind of tx to get the estimated cost) - now i’ll need to fork the logic on the frontend based on whether or not the receiving address has code - unnecesary complexity, geth should handle this case just as it used to.
2. eth_estimateGas params are, according to the wiki, supposed to match the eth_call params, and take a blockTag. I don’t use this feature other than setting it to “pending” but now it throws saying wrong arg count. I don't really care how this one is resolved, but this is a breaking change that broke metamask.

Steps to reproduce the behaviour

rejection of simple value transfer

curl -d '{"id": 1, "method":"eth_estimateGas","params": [{"to":"0x5fda30bb72b8dfe20e48a00dfc108d0915be9bb0","value":"0x1234"}]}' -X POST https://mainnet.infura.io

rejection of blockTag

curl -d '{"id": 1, "method":"eth_estimateGas","params": [{"to":"0x5fda30bb72b8dfe20e48a00dfc108d0915be9bb0","value":"0x1234"}, "latest"]}' -X POST https://mainnet.infura.io

confirm geth version

curl -d '{"id": 1, "method":"web3_clientVersion","params": []}' -X POST https://mainnet.infura.io

[obscuren]

1. dup of eth/api: eth_estimateGas broken for non-contract recipients #2577
2. the specification should change. It's a bit pointless to execute against anything other than the pending state.

[kumavis]

1. whoops, i did a quick initial search for duplicates but missed it
2. agree

[kumavis]

reopening bc you should match the spec in the meantime -- even if you ignore the blockTag parameter.

[karalabe]

The first issue should be fixed by #2589. Could you please check if it sorts out the issue for you?

[kumavis]

@karalabe yes that resolves (1)
remaining issue is (2) eth_estimateGas params does not match spec per wiki

this can be resolved by changing the spec, or allowing the optional blockTag

[lastperson]

Dunno if it should be here or not, please point me in the right direction.
Currently estimateGas works against the clean pending block, doesn't taking into account even the pending transactions from the same address. This behavior renders it impossible to check if your transaction will be successful(or estimate gas) after your previous transaction, while the previous transaction is still pending. Currently I mitigate this by waiting for the significant transaction to be mined first, then executing estimations and calls for the next one. Probably it is correct to call the default defaultBlock for calls and estimateGas latest, and use pending optionally if you want to take into account your pending tx's.

[karalabe]

It should take into account all pending transactions in the particular block. There's little sense to use the latest block for it. AFAIK pending was always the designed use case. Whay Geth version are you using? Can you give us an example of what it does vs. what it should do?

[lastperson]

I'm using the 1.4.5, will prepare an example now. Also all pending transactions is not quite safe, because you can't be sure which transactions will be mined before yours, but you can always be sure that your own transactions will be mined in sequence.

[lastperson]

@karalabe

contract PendingCall {
    bool step = false;
    function next() returns(bool) {
        step = !step;
        return step;
    }
}

var address = web3.eth.accounts[0];
var pendingcall = web3.eth.contract([{"constant":false,"inputs":[],"name":"next","outputs":[{"name":"","type":"bool"}],"type":"function"}])
  .at('0x3a6e732c55b7d576c84d65ae0ad02b9bb913006a');
pendingcall.next.call(function(err, afterFirstTx) {
  console.log("After first tx state: " + afterFirstTx);
  pendingcall.next({from: address, gas: 50000}, function(err, tx) {
    console.log("Tx: " + tx);
    var check = function() { 
      pendingcall.next.call(function(err, afterSecondTx) {
        console.log("After second tx state: " + afterSecondTx);
        if (afterFirstTx === afterSecondTx) {
          // Always getting this on the network.
          console.log("Call didn't take into account pending tx, simulated result is the same as for pending tx.");
        } else {
          // Always getting this on testrpc, because mining is immediate there.
          console.log("Call took into account pending tx(or it was mined blazingly fast), simulated result is the same as starting.");
        }
      });
    };
    setTimeout(check, 2000); // Just to make sure there is no race condition in the geth.
  });
});

Please try it. If it is not self explanatory I will do my best to explain.

[lastperson]

Btw, if we alter second call in this way:

pendingcall.next.call('pending', function(err, afterSecondTx) {

it will start work as expected.
The problem is that estimateGas doesn't take defaultBlock parameter.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[LefterisJP]

Hey guys. @karalabe

I stumbled upon this problem this week. In Raiden we are using eth_estimateGas to see if a transaction we are going to send is going to fail or not so that we don't send it if it's gonna fail anyway.

Without specifying a block on which to do the check. the eth_estimateGas is going to be flaky by default as there can be other transactions or blocks mined which will invalidate the call you are about to do. Thankfully the spec allows us to specify a block parameter.

It works fine with parity and with py-evm. But geth does not accept the block_identifier parameter for eth_estimateGas and instead returns {'code': -32602, 'message': 'too many arguments, want at most 1'}.

Why not implement the call as per the spec? I described a valid use-case that requires a block identifier and where the "pending" block is not always the correct answer.

[karalabe]

I'm unsure that it's a valid use case. If estimating on top of pending fails, why wouldn't it fail when actually executing it? Wouldn't the same failure scenario happen?

That said, I don't think there's any particular difficulty in supporting this use case if it's speced.

[ligi]

@LefterisJP can you elaborate a bit more on the use-case? Also opened an magcians thread about it - we should really remove this stumbling block ..

https://ethereum-magicians.org/t/eth-estimategas-and-block-parameter/3302

[LefterisJP]

@ligi It's mostly to run the EVM on a particular state.

For real real gas estimation indeed only pending makes sense since you want to know how much gas an operation will cost and that operation can only happen in the next block.

At this point I think this should just be implemented for parity between geth, parity and the json-rpc spec.

[ligi]

not sure about this tbh. I rather think that when there is no compelling use-case the spec should be changed. And I do not see a use-case at all currently. Just because parity jumped out the window - why should geth jump?

[wighawag]

I think the blockNumber parameter is valuable.
It can for example ensure your estimation is executed against a recent block and not some very old block.
If you specify a recent block and the node you are talking to is not synced up, it will return an error as it does not have the block. This allow you to retry.

[ligi]

I don't think that's a usecase for this parameter. The default is to "latest" anyway.
IMHO what you want to do is better done by just checking the sync state which is already possible and not abusing this parameter for getting this information.
I think this parameter is only useful if you need to get gas-estimation for old blocks - and I struggle to see any use-case for this currently.

[wighawag]

This would require another request which in some setup is not guarantee to reach the same node (infura)
Passing the blockNumber ensure you get the estimate on the state at that blockNumber or an error. This is in no way an abuse, just what you expect for every other call that let you pass the blockNumber.

[ligi]

ok - that's a good point

[LefterisJP]

@ligi What @wighawag is saying is what I have been trying to say above with the state machine. Can't always be checking the syncing state. When the code goes into a codepatch checking the state in a specific block we need to know it's that block and only that block. Can't have the state of the world change under us.

[karalabe]

@wighawag How do you know which block number is recent? :) You still need to query it.
@LefterisJP Specifying the block number doens't guarantee that either. A mini reorg can change it.

[LefterisJP]

@karalabe That's why we specify the block identifier (hash) and not just the number. If the block identifier is not found the that means we had a reorg.

[ligi]

@LefterisJP but the block parameter would not allow this as far as I see:
https://github.com/ethereum/wiki/wiki/JSON-RPC#the-default-block-parameter

The following options are possible for the defaultBlock parameter:

HEX String - an integer block number
String "earliest" for the earliest/genesis block
String "latest" - for the latest mined block
String "pending" - for the pending state/transactions

does any implementation allow you to specify a block hash there?

[LefterisJP]

Hmmm you are correct. I was confused since we are using web3.py and it accepts block hashes as identifiers for all calls. But it seems that under the hood it queries the block number of the block with the particular hash:

https://github.com/ethereum/web3.py/blob/11ef9df28dfbe4b83683a84fec184406165f18d5/web3/contract.py#L1353-L1354

That's really unfortunate then. So client implementations don't accept blockhashes as block identifiers at all? That would have been a nice feature.

[wighawag]

@karalabe

How do you know which block number is recent? :) You still need to query it.

of course you need to query it but you can use whatever method you want here. The point was about state view consistency between 2 different request.

And I agree that blockNumber is not ideal, blockHash would be better, so I would vote for that too

[deacix]

Based on Ethereum JSON-RPC specification function eth_estimateGas should support the block tag.
https://github.com/ethereum/wiki/wiki/JSON-RPC#eth_estimategas
That now means, that Geth is not conform with Ethereum JSON-RPC specification.
Where is the problem to implement it? Parity has it already im place...

[ankitm123]

Is someone working on this? I can work on it.

[moodysalem]

It should take into account all pending transactions in the particular block. There's little sense to use the latest block for it. AFAIK pending was always the designed use case. Whay Geth version are you using? Can you give us an example of what it does vs. what it should do?

If the node is not a mining node (e.g. Infura), the pending block is meaningless because the actual transaction order of the next block is random as far as the node is concerned, and all unconfirmed transactions (except for the ones sent from the same account with a lower nonce) in the mempool should be ignored in estimating the gas.

I think the result is many transactions can be blocked from submission via failing gas estimates, if a pending transaction exists that may never be confirmed due to gas price, but that transaction changes the state so others cannot be confirmed after it.

We are seeing many failing gas estimates today for the Uniswap interface, and I suspect it may related to this issue. Could you confirm the behavior of geth eth_estimateGas with regards to other pending transactions/ordering?