-
Notifications
You must be signed in to change notification settings - Fork 676
6.4.2: TypeError: Cannot read property '0' of undefined
#417
Comments
I can confirm that this problem exists, in my opinion, this is a critical issue as it concerns large amount of use cases. It also makes using ganache-cli with MetaMask impossible, as it crashes every time a transaction to a contract function has to be signed, so I had to switch back to version 6.4.1. The problem is related to the new "gas exactimation" methods used in versions > 6.4.1, I believe. |
Thanks @tyler17 for the report. I'll try to get a hotfix release out for this today! |
@ndeshev, what leads you to believe this crash is gas estimation related? Can you add some log output here? (starting ganache-cli with the |
David, yes, I tested it with the verbose option on. Also the contracts passed about 200 unit tests in Truffle where I never estimate gas usage and the only time the ganache-cli breaks with The estimated gas for the transaction that the MetaMask is displaying is almost the maximum I set for allowed for block gas limit. If I leave the MetaMask transaction window open and restart the ganache-cli it sends it successfully. Also, the problem did not exist in versions lower than 6.4.2 which is another thing that led me to think is gas estimation problem related as this was one of the few changes in versions 6.4.2 and 6.4.3. The estimated gas for the transactions that is displayed in the logs below are incorrect - it is almost the maximum allowed for the block, as mentioned above. I did some more testing, there are functions that do not break the ganache client when MetaMask is invoked, but they are very simple short ones, on calls to more complicated functions the problem persists. Update - More testing and information related to use cases with web3.js: After executing only the web3.js gas estimation method However, when I tried to run the same web3.js .estimateGas() method on a ganache-cli prior to v.6.4.2 it doesn't work at all throwing the following error: RPC Error: Internal JSON-RPC error. In summary, that might be a tricky one... OS: Windows 10, web3.js version: 1.00-beta.52 | Node - 10.14.1 | ganache-cli version used for the logs is 6.4.3 but when I tried 6.4.2 before a few days the outcome was the same. Log 1:
Sometimes ganache executes one or few more JSON RPC call before crashing, everything else is the same, the gas cost is again at the maximum allowed block gas limit.
|
@ndeshev we found a potential fix, but can't pinpoint the contract that causes the condition that triggers the bug. Can you share with us the contract and method(s) that are causing the crash? |
For our internal reference, here is the potential fix: b74050b |
Hey there David, I apologize for the delayed response. Unfortunately, at this stage I'm not able to provide the full contracts code, I will try to help by describing more of my observations using and testing the 6.4.3 version with the latest MM and web3.js (1.00-beta.52) for multi-contract Dapp, during the last few days.
All calls are going through multiple contracts, meaning: call to an entry contract - > that calls a logic contract -> that calls storage contract, etc. Solidity compiler version is 0.5.7, the Ethereum fork used is St. Petersburg, all ganche-cli options used:
The code of one of the most simple function calls that crash the client:
The log:
Another log on another execution of the same function:
I will test the potential fix in a few days |
@davidmurdoch Awesome, just wanted to confirm that I tried using that |
Confirming that the potential fix resolves the crashes during UI testing with MM & web3.js. The other gas estimation problems described in #417 (comment) are the same. |
Thanks @ndeshev for following up. I wasn't able to reproduce the crash with the contract examples you provided since they are incomplete contracts. I'm hesitant to release this patch without a test to ensure we don't regress in the future. Is there any chance you can provide us with a complete reproduction contract. The overestimate is definitely a separate bug. @nicholasjpaterno will likely be looking into this issue when he wraps up some of his current tasks. Changing the There is one edge case we do know about and don't try to handle: when a contract checks its gasLeft and behaves differently depending on this value. A contrived example being:
A function like the above would cause gas estimations to vary wildly depending on the amount of gas initially supplied to the transaction. Just to reiterate: the amount of gas required is often greater than the amount of gas used. Any chance you could provide a minimal reproduction of a fluctuating and wildly high gas estimate contracts? |
@davidmurdoch Are you still unable to reproduce the crash? I'm finding that it crashes with some (but not all) calls made to DS Proxy's execute function, maybe the delegatecall is causing the issue? |
Thanks @tyler17! Yes, we're still looking for a minimal reproduction of the issue. This line looks like great candidate to produce an edge case in opcode ordering we don't handle: https://github.com/dapphub/ds-proxy/blob/379f5e2fc0a6ed5a7a96d3f211cc5ed8761baf00/src/proxy.sol#L64 |
@davidmurdoch This still happens in beta 2.5.6. on eth_estimateGas The client code is in this method: The contracts code is here: The called method after transfer |
Expected Behavior
ganache-cli not crashing
Current Behavior
After upgrading to v6.4.2 (from v6.2.5), the testchain will randomly crash in the middle of our tests with
Steps to Reproduce
checkout branch
sdk-688
here: https://github.com/makerdao/dai.js/tree/sdk-688follow steps under
Developing
in Readmerun tests with
yarn test
Your Environment
The text was updated successfully, but these errors were encountered: