Expose ganache's "out of gas" error upon benchmarking #4

feuGeneA · 2022-01-19T01:53:13Z

For ganache I disabled quiet logging and discovered that it's not actually executing the full transaction that's being benchmarked:

Simulating shutdown...
  eth_sendTransaction
  
    Transaction: 0x08af3eb8b0c5584aabb872319e3227308cca834ca16223b5bff2cc3750ed52c3
    Gas usage: 30000000
    Block number: 13724060
    Block time: Wed Jan 19 2022 00:55:41 GMT+0000 (Coordinated Universal Time)
    Runtime error: out of gas
  
  eth_chainId
  eth_getTransactionReceipt
  eth_blockNumber
  eth_chainId
simulate-shutdown: 11700.075ms

After adding the assertions:

Simulating shutdown...
  eth_sendTransaction
  
    Transaction: 0x08af3eb8b0c5584aabb872319e3227308cca834ca16223b5bff2cc3750ed52c3
    Gas usage: 30000000
    Block number: 13724060
    Block time: Wed Jan 19 2022 01:33:48 GMT+0000 (Coordinated Universal Time)
    Runtime error: out of gas
  
  eth_chainId
  eth_getTransactionReceipt
  eth_blockNumber
  eth_chainId
  AssertionError [ERR_ASSERTION]: transaction failed. receipt: {"to":"0xF403C135812408BFbE8713b5A23a04b3D48AAE31","from":"0xa3C5A1e09150B75ff251c1a7815A07182c3de2FB","contractAddress":null,"transactionIndex":0,"gasUsed":{"type":"BigNumber","hex":"0x01c9c380"},"logsBloom":"0x00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000","blockHash":"0x3191002a084b22ac94f25bc18740c2c8c22c6ef6a4e1273cc6308ebc028aecd3","transactionHash":"0x08af3eb8b0c5584aabb872319e3227308cca834ca16223b5bff2cc3750ed52c3","logs":[],"blockNumber":13724060,"confirmations":1,"cumulativeGasUsed":{"type":"BigNumber","hex":"0x01c9c380"},"effectiveGasPrice":{"type":"BigNumber","hex":"0x0f8b9c7251"},"status":0,"type":2,"byzantium":true}
      at main (/convex-shutdown-simulation/scripts/convex.ganache.ts:54:10)
      at runNextTicks (internal/process/task_queues.js:62:5)
      at listOnTimeout (internal/timers.js:523:9)
      at processTimers (internal/timers.js:497:7) {
    generatedMessage: false,
    code: 'ERR_ASSERTION',
    actual: 0,
    expected: true,
    operator: '=='
  }
error Command failed with exit code 1.

I feel that the assertions are essential for ensuring trust in this benchmark, but I also kept in the logging because it's not very much output.

I'd love to put analogous verification in place for the foundry test as well, but I haven't yet been able to get that environment set up.

mds1 · 2022-01-19T03:34:26Z

Interesting find! Good call checking the receipt status. Just ran it myself and confirmed that the ganache simulation does indeed seem to run out of gas. Paging @gnidan since these seems like a bug?

For foundry, you can try the new foundryup to get it installed easily. Though when the test runs out of gas you'll see an error clearly, e.g. if we set the gas limit to 15M you see:

$ forge test --gas-limit 15000000
compiling...
success.
Running 1 test for ConvexTest.json:ConvexTest
[FAIL. Reason: ] testShutdownCost() (gas: 6654)
Error: 
   0: Encountered a total of 1 failing tests, 0 tests succeeded

Location:
   cli/src/cmd/test.rs:187

The reason for the failure isn't too clear, so I just created foundry-rs/foundry#499 to track this

gnidan · 2022-01-19T05:09:34Z

Thanks for the heads up! @davidmurdoch you should see this :)

davidmurdoch · 2022-01-19T13:19:34Z

👀

davidmurdoch · 2022-01-19T14:43:26Z

Looks like all the tools benchmarked here consume a different amount of gas when calling this function. I'm not sure which tool can be trusted to deliver an accurate gas estimate or accurate result. Perhaps the the logs can help here; i'll try to get some others from the truffle team to help look into this further with me soon (:tm:).

feuGeneA · 2022-01-19T23:30:54Z

With f96bae4 in place:

$ make benchmark-foundry
bash scripts/benchmark-foundry.sh
compiling...
no files changed, compilation skipped.
Running 1 test for ConvexTest.json:ConvexTest
[PASS] testShutdownCost() (gas: 18876589)
Logs:
  shutdown gas used: 18864842


real	4m39.219s
user	0m1.607s
sys	0m0.676s

davidmurdoch · 2022-01-20T00:39:51Z

I was actually referring to the transaction event logs that are emitted over the course of the transaction. Diffing and exploring these event logs between clients may help us all improve our forking implementations.

feuGeneA · 2022-01-20T00:51:31Z

I was actually referring to the transaction event logs that are emitted over the course of the transaction. Diffing and exploring these event logs between clients may help us all improve our forking implementations.

Right, I didn't push that commit as a response to your comment. I pushed it because I think it's important to be able to quickly visually inspect to see whether the test is running to completion, just like I had already done for ganache and hardhat in 2bb428a. (I hadn't gotten the foundry environment together until more recently.)

davidmurdoch · 2022-01-20T00:57:21Z

Cool. If you want to explore this stuff together over zoom sometime next week let me know!

mds1 · 2022-01-20T02:48:35Z

hardhat.config.ts

@@ -13,6 +13,7 @@ const config: HardhatUserConfig = {
        url: String(process.env.ETH_RPC_URL),
        blockNumber: Number(process.env.FORK_BLOCK),
      },
+      loggingEnabled: true,


Does this have any significant impact on performance? From my quick test it seems to make no noticeable difference, but just want to confirm

I doubt it's relevant, yes. But turning it off shouldn't hurt either.

mds1 · 2022-01-20T02:48:54Z

scripts/convex.ganache.ts

@@ -89,7 +90,7 @@ async function prepareGanache({
      defaultBalance: ethers.utils.formatEther(defaultBalance),
    },
    logging: {
-      quiet: true,
+      quiet: false,


Same question about impact on performance?

Honestly, I have no idea. @davidmurdoch ?

mds1 · 2022-01-20T03:39:09Z

@feuGeneA Can you make sure the "allow edits by maintainers" box is checked? Just pushing a small change to add --verbosity 2 to the dapptools script but seems the push was rejected

After that + answers around performance impact I think this PR is good to merge, then I'll open an issue to continue the discussion of the ganache issue

feuGeneA · 2022-01-20T16:52:44Z

@feuGeneA Can you make sure the "allow edits by maintainers" box is checked?

It seems that GitHub only shows that check box on user-owned forks, and I forked this under @nomiclabs rather than under my own account 😕

I went into the settings for the nomiclabs/convex-shutdown-simulation repo and invited you to be a collaborator with Role: write. Hopefully that does the trick; please let me know if not.

feuGeneA · 2022-01-20T17:00:56Z

performance impact

I can't speak to the specific performance impact of logging, for any of these tools (including Hardhat, without doing some research).

If you're interested in benchmarking the most performant scenario possible then I could understand why you'd be hesitant to enable logging. However, if you're only interested in an "apples to apples" comparison, and in ensuring that it's a "level playing field" across the different tools, then I would presume logging enablement to be a non-issue, if it's enabled for all of the tools.

mds1 · 2022-01-20T18:58:40Z

Since logging seems to have no impact on performance I think it's ok to leave it ok enabled for now. Going to merge this and create a new issue to continue tracking the ganache bug—thanks!

mds1 · 2022-01-20T19:05:29Z

Created #7 to track the ganache issue

feuGeneA added 2 commits January 18, 2022 19:59

enable logging for ganache and hardhat

2bb428a

ganache&hardhat: assert success of benchmarked tx

4a199d5

foundry: emit logs for easy success verification

f96bae4

mds1 reviewed Jan 20, 2022

View reviewed changes

dapptools: emit logs for easy success verification

a9cd638

mds1 merged commit aa2e54e into mds1:main Jan 20, 2022

mds1 mentioned this pull request Jan 20, 2022

fix: ganache simulation fails with out of gas #7

Closed

fvictorio deleted the ganache-out-of-gas branch January 21, 2022 10:50

MicaiahReid mentioned this pull request Jun 13, 2022

fix: upgrade Ganache to v7.3.0, fixing out of gas bug #21

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expose ganache's "out of gas" error upon benchmarking #4

Expose ganache's "out of gas" error upon benchmarking #4

feuGeneA commented Jan 19, 2022

mds1 commented Jan 19, 2022 •

edited

Loading

gnidan commented Jan 19, 2022

davidmurdoch commented Jan 19, 2022

davidmurdoch commented Jan 19, 2022 •

edited

Loading

feuGeneA commented Jan 19, 2022

davidmurdoch commented Jan 20, 2022

feuGeneA commented Jan 20, 2022

davidmurdoch commented Jan 20, 2022

mds1 Jan 20, 2022

fvictorio Jan 20, 2022

mds1 Jan 20, 2022

feuGeneA Jan 20, 2022

mds1 commented Jan 20, 2022

feuGeneA commented Jan 20, 2022

feuGeneA commented Jan 20, 2022

mds1 commented Jan 20, 2022

mds1 commented Jan 20, 2022

Expose ganache's "out of gas" error upon benchmarking #4

Expose ganache's "out of gas" error upon benchmarking #4

Conversation

feuGeneA commented Jan 19, 2022

mds1 commented Jan 19, 2022 • edited Loading

gnidan commented Jan 19, 2022

davidmurdoch commented Jan 19, 2022

davidmurdoch commented Jan 19, 2022 • edited Loading

feuGeneA commented Jan 19, 2022

davidmurdoch commented Jan 20, 2022

feuGeneA commented Jan 20, 2022

davidmurdoch commented Jan 20, 2022

mds1 Jan 20, 2022

Choose a reason for hiding this comment

fvictorio Jan 20, 2022

Choose a reason for hiding this comment

mds1 Jan 20, 2022

Choose a reason for hiding this comment

feuGeneA Jan 20, 2022

Choose a reason for hiding this comment

mds1 commented Jan 20, 2022

feuGeneA commented Jan 20, 2022

feuGeneA commented Jan 20, 2022

mds1 commented Jan 20, 2022

mds1 commented Jan 20, 2022

mds1 commented Jan 19, 2022 •

edited

Loading

davidmurdoch commented Jan 19, 2022 •

edited

Loading