EVM behavior does not match Geth/Parity.

[MicahZoltu]

Expected Behavior

EVM execution behavior to match Geth/Parity.

Current Behavior

VM Exception while processing transaction: revert

Possible Solution

Unfortunately I don' tknow what is causing this, though I do have a repro case.

Steps to Reproduce (for bugs)

1. git clone git@github.com:AugurProject/augur-core.git
2. git checkout 69e6e1acc4f99ba3abaf1b0d05a25f50a4821462
3. npm install
4. node --inspect-brk=19867 node_modules\mocha\bin\_mocha output/tests-integration/**/*.js --no-timeouts
5. (wait a bit, contract compilation takes a while; it will cache for following runs)
6. Notice that the test fails with

   VM Exception while processing transaction: revert

7. docker-compose up --build --force-recreate parity-integration-tests geth-integration-tests
8. Notice that the same scripts pass against both Geth and Parity.

Context

It appears the problem is with source/contracts/libraries/Delegator.sol which does a bit of fancy footwork. Even when I comment out Universe.initialize body so it just returns true, the call still fails which means it must be failing on the delegate call to that function. If I call a non-delegated method on the same contract it behaves as expected and returns the correct result.

Your Environment

• Version used: 6.0.1
• Environment name and version (e.g. PHP 5.4 on nginx 1.9.1): Node v8.5.0
• Operating System and version: Windows 10
• Link to your project: https://github.com/AugurProject/augur-core

[benjamincburns]

Hi Micah,

It seems you tracked this down to an out of gas issue, which I commented on over in #411.

As I said over there, our goal isn't to behave identically to geth or parity, but rather to somewhat pedantically implement the JSON RPC spec.

That said, since testing the byzantium changes, I've been finding that some cases which used to produce a very helpful out of gas message have now shifted to produce a revert message. When I see revert, I think my code has hit an error. When I see out of gas - I know what to do.

The problem is, I think the revert is actually done as a check by the solidity runtime. I've already brought this up with that team. Perhaps they can add something that makes it clearer that the revert was done to avoid an out of gas condition.

[benjamincburns]

@MicahZoltu I believe this behaviour will get much better once ethereum/solidity#1686 is completed. Closing this for now, as I suspect the Solidity team will provide better messages on revert once that enhancement is ready.

[MicahZoltu]

@benjamincburns This is not an OOG issue as I am working around #411 by setting the gas provided to eth_estimateGas to 6.5M and this contract call should only require hundreds of thousands to execute. If it is running out of gas, it is because the EVM that TestRPC uses does not behave the same way as the EVM in Geth/Parity and it is somehow getting stuck in an infinite loop or something.

Strongly recommend re-opening and investigating as I believe this is a consensus issue with TestRPC's EVM.

[benjamincburns]

I hereby heed your strong recommendation, and offer a sincere thanks for your persistence.

Also your repro steps kick ass. Thanks for making it easy.

[benjamincburns]

Note to self - is #367 related?

[Stradivario]

I think i am stuck to the same issue but i am not running out of gas... my code was working fine previews week now i am stuck for 14 hours figuring out how to fix this....

trufflesuite/truffle#748

[nuevoalex]

Pinging this issue again as it is preventing us from developing against TestRPC and it would be great to be able to do so. Thanks!

[mikeseese]

I looked into this issue a little bit, and it appears the first revert error is caused due to not providing a gas option for an eth_call transaction. I believe this is the same behavior as #411 but with eth_call (and looking at that issue, @MicahZoltu was also stating eth_call should succeed regardless if gas is provided).

I implemented the fix in trufflesuite/ganache#25 for eth_call and I no longer get that specific revert error. I later get another revert error when I execute sendRawTransaction with a gas that is 10% larger than the result from eth_estimateGas. I'm wondering if there is some lingering unexpected behavior that wasn't fully addressed in #411 that's causing a low gas estimate (also looks like it could be related to ethereum/go-ethereum#15896).

It's late so I'll continue this tomorrow, but I wanted to report my findings thusfar.

Side note: As part of a bounty for Augur, I'm developing a Solidity Debugger runtime and accompanying VS Code extension built around the ganache-core/ethereumjs-vm libraries. Augur's test suite/contracts are the forefront in terms of practical testing.

Side-side note: Great work on TestRPC/Ganache-cli/core! It was fairly easy and straight forward to integrate with!

[benjamincburns]

Hi @seesemichaelj I decided to look into this again today as well, and it seems we're on the same track, though there is already a fix in place which causes eth_call to use the block gas limit if no gas argument is specified. If memory serves, this fix should be present in ganache-cli v6.0.3. I could be mistaken however, as I've been testing with ganache-cli v6.1.0-beta.0 today.

From what I can tell there seems to be a problem with eth_sendRawTransaction. If I switch the logic behind Universe.initialize to use eth_sendTransaction, it no longer reverts. My initial, unsubstantiated guess is that we're not decoding the signed transaction correctly.

Edit: I misinterpreted what I was seeing and spoke too soon: this just causes things to fail earlier.

Edit2: I don't think this is a gas estimation issue, either. If I force the transaction to use the block gas limit it still reverts.

[benjamincburns]

Yanno, @seesemichaelj I really should've clicked that PR link before replying - I was thinking that you submitted a new one. Had I clicked it, I would've seen that it was already merged! 😊

[benjamincburns]

Forget edit 2 in my comment above. If I set an absurdly high block gas limit, and if I change localCall and remoteCall in source/libraries/ContractInterfaces.ts to specify said absurdly high block gas limit, then the test passes.

As @seesemichaelj pointed out, eth_call seems to not be running using the block gas limit. This is an easy fix. I'll do it now.

As for why Universe.initialize fails, I have a suspicion that it's to do with a couple of things...

1. Universe.initialize makes use of a delegate contract.
2. Operations handled by CALLs and CREATEs are limited to 63/64 of the remaining gas
3. eth_estimateGas reports the gasUsed for a transaction, as we've interpreted it's return value to mean "the amount of gas expected to be consumed by running this transaction" rather than "the minimal gasLimit required to run this transaction successfully." Note that I'm not saying that this is correct, just that this is how we've interpreted it.
4. Your contract is written in solidity which inserts runtime logic which, prior to executing certain expensive operations, will check the gas remaining vs the upper bound cost of that operation and revert if there isn't enough gas available. This is what causes the revert rather than the out of gas.

Edit: this theory is likely somewhat incorrect, namely because of #367. I've just confirmed that this issue is still present in the EVM.

More edit: Per comments on ethereumjs/ethereumjs-monorepo#255, the gas accounting is implemented in the ethereumjs-vm, it's just not reported to the vm itself correctly, so this theory seems plausible.

[benjamincburns]

Also a heads up as well that there appears to be a race condition around nonce calculation. That is, if I comment out the ethjsquery.estimateGas call in remoteCall in source/libraries/ContractInterfaces.ts, I get incorrect nonce errors. It's entirely possible that this is a problem with ganache's new request queuing logic, however.

[mikeseese]

After some more testing (including the fix in trufflesuite/ganache@2421f4a), I've concluded that only remoteCall in source/libraries/ContractInterfaces.ts requires the large gas limit to get the test to pass.

Changing gas = BN.min(new BN(6950000), gas.add(gas.div(new BN(10)))); to gas = new BN(6500000); (note the difference in the "large gas limit"; 6950000 is too large in my test) provides a successful test pass.

Still investigating root cause

[benjamincburns]

@seesemichaelj thanks so much for taking the time to look into this! I'm sorry to say that I've been juggling too many things to follow this thread as deeply as you have.

Given the behavior you describe, my strong suspicion at this point is that the underlying cause is a bug in ethereumjs-vm. There's really very little ganache-core can do to impact what you're seeing.

If you feel like you'd rather pass this off to someone else, it might be worth capturing a minimal test example and submitting it as an issue over there, or even better, as a test in the ethereum/tests project.

[mikeseese]

No problem! I've been getting pretty familiar with the ganache-core/ethereumjs-vm relationship/inner workings so it's within my grasp to work on, and the bug is currently preventing me from moving forward on my solidity debugger.

I agree with you that this seems to be pointing at an ethereumjs-vm bug. I will probably continue to document my findings here until I can find it's an issue somewhere else.

[benjamincburns]

I will probably continue to document my findings here until I can find it's an issue somewhere else.

Feel free!

[mikeseese]

Given the fix you put in at trufflesuite/ganache@2421f4a, the next revert error was happening in a remote call that came from https://github.com/AugurProject/augur-core/blob/master/source/tests-integration/TestFixture.ts#L54

I noticed one thing that in that transaction, we were receiving a 15,000 gas refund for a SSTORE; from the yellow paper:

Finally there is A_r, the refund balance, increased through using the SSTORE instruction in order to reset contract storage to zero from some non-zero value. Though not immediately refunded, it is allowed to partially offset the total execution costs.

ethereumjs-vm implements this (whether or not this is correct, I'm not sure) by subtracting the refund from the gasUsed (see https://github.com/ethereumjs/ethereumjs-vm/blob/master/lib/runTx.js#L140-L148)

It makes sense, but it's a little confusing because you need that gas to run the transaction. I feel that two changes should be proposed to mitigate this issue:

• Insert results.gasRefund = gasRefund; before line 143 in ethereumjs-vm/lib/runTx.js to report the refund in the transaction
• Change ganache-core/lib/statemanager.js line 549 to result = results.gasRefund ? to.hex(results.gasUsed.add(results.gasRefund)) : to.hex(results.gasUsed);

After implementing these, the test passes.

Thoughts?

[mikeseese]

Ha! I'm reading your chat now in Gitter on ethereumjs-lib and it seems y'all came up to the same conclusion as me. I can put write issues/PRs if you'd like?

[benjamincburns]

@seesemichaelj sorry it took me so long to get back to you - I was traveling last week.

Writing up the relevant issues/PRs is encouraged, thanks!

[mikeseese]

No worries; I've been moving forward! I will go ahead and take charge on this one.

Thanks for the help!

[mikeseese]

After trufflesuite/ganache@2421f4a, this issue can be closed in favor of trufflesuite/ganache#26 as trufflesuite/ganache#26 is the root cause to the last revert error.

[benjamincburns]

Closing 'cause Mike is on the team now, and Mike knows best :-D