-
Notifications
You must be signed in to change notification settings - Fork 678
Transaction status is 1, but last opcode is REVERT #611
Comments
I noticed that the call_trace is shorter than I expect:
It ends with ChiToken, like I expect, but there should be more calls to |
So I copied this test and changed it to work for kyber. And the kyber test had the correct return_value set! So I think maybe uniswap using vyper might be related. |
If I skip the gas token burning and shorten the amount of calls, it gives a return_value. So I think this is related to some complexities in freeing GST2/CHI |
Should I open this on ganache-core instead? |
@wysenynja I was able to use Github's transfer function to move this to ganache-core as we do try to keep core functionality separate from the CLI repo. (I think we're thinking of a monorepo to simplify all of this in the future) |
@wysenynja I'm having issues with just running your setup. When I finally run the test command you provided, I get:
The only thing I changed was here's my bash history:
was i supposed to run both |
I think you want to leave it as Since you've already run BURN_GAS_TOKEN just enables or disable the gas token minting/freeing. It shouldn't change this error. |
I upgraded brownie and all the other dependencies and also upgraded to ganache-cli 6.10.2-forking.0. I've changed things around and am using LGT instead of GST2 now, but that hasn't changed the issue. The test trades ether to token, a token to another token, and that token back to ether. The transaction's status claims success and there is no revert reason, but the
|
@wysenynja I'm trying to run the tests on a separate process of ganache so I can debug it and monitor logs. I created a new
Any idea what I need to change? My config looks like:
|
Nevermind, it appears that if I'm already ganache on the same port brownie won't try to launch it |
Alright, I have a pretty stable way to reproduce this issue and debug what's going on. Some preliminary findings: If I run ganache in a separate process and run the test, I receive the error as stated in this issue. If i quit brownie, brownie will revert a bunch of snapshots to "revert" the ganache change back to the start. If I run the test again without restarting ganache, I get a different error:
This is interesting because it's the same error that the call trace points to when I ran the test in ganache the first time. Digging into how I tried my changes in #613 and it's still not working. I'm going to continue digging into this specific issue a tad further, but my hunch is that solving other forking issues will solve this issue |
@wysenynja are we able to run these tests on ganache without forking? Like deploying any contracts we need to and running tests against those on a completely fresh ganache state? |
That sounds like some good progress! The tests could be rewritten to not depend on forking mainnet, but they would have to deploy some ERC20s and Uniswap/Kyber and bootstrap them with liquidity. That's how the early versions of my code did it, but it was fragile and done by a bunch of bash scripts that glued together their deploy scripts. This test doesn't use a real exchange, but it does use all of my contracts: https://github.com/SatoshiAndKin/argobytes-contracts-brownie/blob/03befdc4f87f2f4ceefc35885a98ed869d807bf4/tests/test_argobytes_owned_vault.py#L30 |
I assume my earlier #605 is the same issue as this? |
You are correct @haltman-at, I'll mark that one as a duplicate since this one is older (it was originally on the ganache-cli repo so that's probably why you didn't find it when you made #605) However, I will use your #605 example to try to figure out the common root cause here. Thanks for letting me know! |
@mds1 commented in #571 with reproduction steps that point to very similar reproduction steps that this issue has. I believe #571 was originally calling out the same issue, but it got shifted to solving a different problem with a similar symptom I've put @mds1's comment below so it's easy for me to see all of the relevant recent information quickly My current plan of attack here is to study both of these reproduction steps to understand the root issue and solve that. I don't believe this has to do with reentrance, but storage lookups are not adjusting as they should and are causing something to go awry and hit a piece of code that says you reentered I'm using ganache-core 2.11.3-beta.0, and it seems I'm still seeing this bug in certain cases. Steps to reproduce:
In step 4, only two tests in
The reentrant call error is from the zkSync contract, found here. The full console output from the error is below:
It seems this has something to do with the use of I also just noticed I'm missing some |
As you alluded to @mds1, I believe that snapshots/reverts are not handled properly in forking. I believe this is at a minimum affecting all storage, but it could also be an issue with how we handle storage deletes in between snapshot/revert. I'm got a ganache-core reproducing the error, so we're nearing a fix! |
Turns out my test was incorrectly written and actually does work, so I still don't have a unit test that reproduces this. I did some refactoring that I was hoping was going to fix issues, but alas it does not. Investigating further to figure out exact root cause |
Giving a little update here. I still don't have a unit test that reproduces the issues from @mds1's repo, but I was able to get a quick test setup that allowed me to debug it. I'm happy to say that I have code that resolves the issue within @mds1's repo! Early next week, I'll test the changes against @wysenynja's repo to see if I killed 2 birds with 1 stone. Regardless, we'll look at making a beta release sometime next week Hope yall have a good Labor Day! |
Looks like I did not solve @wysenynja's issue which is specific to debug_traceTransaction. I will be moving @mds1's issue (again haha) to #624 since his were specifically due to snapshots (which probably partially affected this issue) We'll go forward with a beta release for that as I figure out debug_traceTransaction |
Thanks for all your hard work! |
Quick update here: I've figured out the root cause of @wysenynja's bug (at least the next hurdle haha). I can reproduce it in a test, and hoping to get over this one soon! |
Seems like I'm able to get that one test to pass @wysenynja! Hoping we can get a beta release out shortly after PR reviews |
This was released in https://github.com/trufflesuite/ganache-core/releases/tag/v2.12.2-beta.0 |
@wysenynja have you been able to test this yet? Are your forking issues resolved? Looks like it also made into a stable release a couple weeks ago: https://github.com/trufflesuite/ganache-core/releases/tag/v2.13.0 | https://github.com/trufflesuite/ganache-cli/releases/tag/v6.12.0 |
Yes! My tests so far are much better. I haven't had time to finish them all (been working on proxy contracts instead), but the ones that were failing are passing now. Thank you!
… On Oct 26, 2020, at 10:40 AM, Mike Seese ***@***.***> wrote:
@wysenynja have you been able to test this yet? Are your forking issues resolved? Looks like it also made into a stable release a couple weeks ago: https://github.com/trufflesuite/ganache-core/releases/tag/v2.13.0 | https://github.com/trufflesuite/ganache-cli/releases/tag/v6.12.0
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or unsubscribe.
|
This is awesome; thanks! |
I have a project that uses brownie and ganache-cli --fork against a geth node. (Geth is running according to these steps).
A few of my tests check the return_value of my transactions. Most succeed, but one has no return value despite not reverting.
Expected Behavior
A transaction with a status of 1 should have a return value.
Current Behavior
Steps to Reproduce
./scripts/test.sh tests/test_uniswap_v1_arbitrage.py -I
Context
Due to previous issues with reverting traces pointing to incorrect lines, I've disabled solc's runs, so that shouldn't be the problem.
I suspected that gastoken2/chi might be related to the issue, but removing that code didn't change anything.
I'm sure the transaction isn't actually reverting because my test balance increases after the transaction is completed. If the transaction had reverted, the balance would not increase.
I don't think Brownie is the problem here. Brownie just gets the trace from ganache.
Your Environment
The text was updated successfully, but these errors were encountered: