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

eth/api: eth_estimateGas broken for non-contract recipients #2577

Closed
obscuren opened this issue May 17, 2016 · 6 comments
Closed

eth/api: eth_estimateGas broken for non-contract recipients #2577

obscuren opened this issue May 17, 2016 · 6 comments
Assignees

Comments

@obscuren
Copy link
Contributor

obscuren commented May 17, 2016

System information

Geth version: 1.4.1 and upward
OS & Version: Windows/Linux/OSX

Expected behaviour

When doing an eth_estimateCall to non-contracts I expect it to return the amount of gas required to do the call.

Actual behaviour

When doing an eth_estimateCall to a non-contract it returns an error telling me that there's no code at the given address (which is true).

Steps to reproduce the behaviour

> eth.estimateGas({from:eth.accounts[0], to:eth.accounts[1], gas:1000000})
no contract code at given address
@obscuren
Copy link
Contributor Author

The issue is that estimate gas uses the generic doCall. This method checks whether there's code and returns an error when no code could be found at the recipients address.

This check was added by @karalabe and assume that it was added for abigen / bindings code. Please elaborate on why this was added so it can be properly addressed when creating the PR to fix this regression.

@karalabe
Copy link
Member

I'm not sure if this is a true regression or a change towards the behavior it should have had originally.

This change was added by #2496. The rationale was that a lot of different error scenarios were mixed in under the same behavior:

  • Contract threw or otherwise did invalid operations: return nil
  • Contract didn't exist in the first place: return nil
  • Chain is out of sync and contract doesn't exist yet: return nil

In case of a call, in all of the above cases a native DApp only receives a parse failure from abi, which cannot unmarshal nil into whatever return types (which can also occur if our abi package is faulty and/or the loaded evm abi doesn't match the bytecode). The issue is that opposed to a user who can make an intelligent guess as to what's wrong, a program needs to know which of the failure scenario occurred. The most important one of them is to know whether the contract doesn't exist or whether the contract code failed: in the first case the solution is to sync, etc... in the second case however the solution is to fail as there's no way to recover if the contract doesn't work.

In the case of estimate, the issue is even worse because given that the contract doesn't exist, estimate plain returned 21000 gas (maybe a bit more if input data was also provided). This results is signing transactions that don't have enough gas and will surely go out of gas, taking the associated ether with it.

@obscuren
Copy link
Contributor Author

I disagree. Doing a call should not result in an error if there was no error to be returned. You're abusing an error field to get an indication whether an execution of the EVM may or may not happen. If you require some indication whether code exists at an address check so in advance of the call and not depend on an error field.

The most important one of them is to know whether the contract doesn't exist or whether the contract code failed: in the first case the solution is to sync, etc.

Exactly. Use len(GetCode(addr)) instead of abusing an error field. Not having code at at address is not an error. Even computers can make very accurate guesses ;-)

In the case of estimate, the issue is even worse because given that the contract doesn't exist, estimate plain returned 21000 gas (maybe a bit more if input data was also provided).

Worse? Why would a call go out of gas when performed? The estimate call method's purpose is to estimate the gas, even for non-contract calls. This is a feature and this feature no longer works properly, therefor: regression.

@obscuren
Copy link
Contributor Author

Additionally if the abi does not handle nil very well then the abi should be fixed.

@karalabe
Copy link
Member

Hmmm, that's a possible interpretation too. In this light estimate is a bit more low level than I'd like, though it could still be made to work a bit higher up with getCode. Though it would need some love to prevent wasting tons of calls constantly verifying code availability. I'll look into fixing this.

@perl5577
Copy link

I have coded fonction for payout all user .
With perfect amount gaz for make transaction.
Now is broken and compliance with wiki .

For me is big regression, i can not payout my users .

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

4 participants