Skip to content

cmd/evm, core/vm, internal/ethapi: don't disable call gas metering#16229

Merged
karalabe merged 1 commit into
ethereum:masterfrom
karalabe:evm-call-fix
Mar 5, 2018
Merged

cmd/evm, core/vm, internal/ethapi: don't disable call gas metering#16229
karalabe merged 1 commit into
ethereum:masterfrom
karalabe:evm-call-fix

Conversation

@karalabe
Copy link
Copy Markdown
Member

@karalabe karalabe commented Mar 2, 2018

Fixes #16193

Our VM currently supports disabling gas metering, which was forcefully enabled for eth_call. Unfortunately, not all code paths check properly this debug property. This issue didn't surface until #15563 was merged, which started tracking some gas dynamics off the stack. These calculations didn't happen any more with DisableGasMetering == true, resulting in CALLs to precompiles to be done with gas 0. Apparently, some code didn't get the memo that gas metering was disabled and failed the call with OOG.

This PR gets rid of DisableGasMetering altogether. This might not be the best approach, since then gas calculation will be incurred with any eth_call. The reasons I did it like this is because 1) eth_call will behave exactly as eth_sendTransaction, which is arguably good; and 2) it seems dangerous to me especially long term that we'll have similar breakages.

Open to feedback or alternative proposals.

Comment thread internal/ethapi/api.go Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please keep the time limit and set gas to ^uint64(0).

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The default gas price is 50M if the user didn't provide any. We can increase that to maybe max uint64/2. #16160 reports issues originating from refunds, which I assume originate from specifying max uint64 as ga limit

@karalabe
Copy link
Copy Markdown
Member Author

karalabe commented Mar 2, 2018

@fjl PTAL

Comment thread internal/ethapi/api.go Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hope you're aware that doCall is used for gas estimation, too. I'm unsure if the timeout and increased default gas can be an issue there.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hate these hard coded crap.

Gas estimation is based on binary search. I.e. it always needs an upper limit, which is set to either what the user supplies, or the current block limit. Both cases should be fine from a runtime perspective. But yes, this does tag on an extra timeout, which might be undersirable (heck, why do even do a non configurable hard coded timeout in the first place).

@lionello
Copy link
Copy Markdown
Contributor

lionello commented Mar 3, 2018

Dare I say "missing test case"?

Copy link
Copy Markdown
Contributor

@holiman holiman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@karalabe
Copy link
Copy Markdown
Member Author

karalabe commented Mar 5, 2018

@lionello EVM behavior needs to be covered by consensus tests, RPC by RPC tests. The former are integrated into hive whilst the latter was being worked on to be part of hive, but got neglected at a certain point. Yes, tests are definitely needed, but it's better to pick up the hive suite and make it work than to hack in a very specific test for this case only.

@karalabe karalabe merged commit 223fe3f into ethereum:master Mar 5, 2018
@raindecastro
Copy link
Copy Markdown

raindecastro commented Mar 14, 2018

Hey @karalabe i'm not really that experienced when it comes to this and wondering if you could help me out too. I also have a function in my smart contract that returns a hash by using SHA256 and have deployed it to the private blockchain that i've created. Now when I call this function it only returns '0x'

I have tried to do this inside the testrpc and it seems to be working and returns the correct digest but I want to be able to make it work in my own private blockchain.

How will I be able to apply the solution you have suggested in order for it to work. Please, I need it for some testing in my school project. Here is my smart contract function that I'm calling for more info


function addTimeIn(string xname, string xtimeIn, string xtimeOut) public returns (bytes32) {
      TimeSheetDetails memory details = TimeSheetDetails(xname, xtimeIn, xtimeOut);

      bytes32 hashed = sha256(xname, xtimeIn, xtimeOut);
      TimeSheet[hashed] = details;
      return hashed;
  }

@lionello
Copy link
Copy Markdown
Contributor

@raindecastro Make sure there's a contract at the address you expect by calling eth.getCode("0x...") first. The same behavior happens when the contract deployment fails, for whatever reason.

@raindecastro
Copy link
Copy Markdown

@lionello hey there! thank you so much! Now it works! looks like my deployment did fail when I was trying it. Maybe it was because I didn't have enough gas? Is it correct that I have to put gas in the precompiled contracts or just sufficient amount of gas inside my contract itself? I'm trying to understand the cause of the problem more, i'm sorry for the beginner questions, i've been searching for information about precompiled contracts and it gets confusing for me.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants