Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Gas cost discrepancies when testing calls through a proxy #2503

Closed
2 tasks done
elenadimitrova opened this issue Jul 28, 2022 · 8 comments
Closed
2 tasks done

Gas cost discrepancies when testing calls through a proxy #2503

elenadimitrova opened this issue Jul 28, 2022 · 8 comments
Labels
A-gas-snapshots Area: gas snapshotting/reporting C-forge Command: forge Cmd-forge-test Command: forge test T-bug Type: bug

Comments

@elenadimitrova
Copy link

Component

Forge

Have you ensured that all of these are up to date?

  • Foundry
  • Foundryup

What version of Foundry are you on?

forge 0.2.0 (33dbaa5 2022-07-23T00:04:10.615195Z)

What command(s) is the bug in?

forge test --gas-report

Operating System

macOS (Apple Silicon)

Describe the bug

When comparing the gas cost test results between hardhat-gas-reporter and foundry they significantly differ for the following sample test cases:
a) ContractA.mint() calling external contract ContractB.mint() where ContractB is the implementation of a Minimal Proxy

foundry reports 68,530 gas
hardhat reports 105,796 gas

b) ContractB.mint() getting called on top of an implementation Proxy (where ContractB is the implementation set on Proxy)

foundry reports 68,992 gas
hardhat reports 93,040 gas

I would normally expect test case a) to be more expensive than test case b) as it is doing a cross contract call (even if in test case B the proxy does an extra read from storage to get implementation address).

Additionally (as this maybe related symptom), when measuring gas costs for calls routed through a proxy referencing an implementation contract, there are two sets of gas costs foundry reports, one for the Proxy itself and another for the implementation contract as seen in screenshot below

Screenshot 2022-07-27 at 20 47 32

However when a different proxy is used that does no local storage reads (e.g. a minimal proxy or the same as above but with immutable implementation instantiated in the proxy constructor) the Proxy gas cost table is no longer displayed and only implementation costs show.

@elenadimitrova elenadimitrova added the T-bug Type: bug label Jul 28, 2022
@gakonst gakonst added this to Foundry Jul 28, 2022
@gakonst gakonst moved this to Todo in Foundry Jul 28, 2022
@onbjerg
Copy link
Member

onbjerg commented Jul 28, 2022

Are the first gas numbers from forge test or from forge test --gas-report? Also, it is expected that there are two sets of reports (one for proxy, one for implementation) as we do not give special treatment to proxies (and probably won't). I would expect the gas to differ somewhat since Forge still obeys cold/warm account and storage slots, so given everything is in the context of one call, you may end up in situations where the gas cost is lower because the slot or account is already warm when called

Another thing to note is that the gas report from forge test --gas-report does not include the 21000 gas stipend which seems to make up most of the difference.

@onbjerg onbjerg added Cmd-forge-test Command: forge test C-forge Command: forge A-gas-snapshots Area: gas snapshotting/reporting labels Jul 28, 2022
@onbjerg onbjerg moved this from Todo to May be solved in Foundry Jul 28, 2022
@elenadimitrova
Copy link
Author

elenadimitrova commented Jul 28, 2022

First gas numbers are from forge test --gas-report

I still don't understand the separate reports you have for the proxy and the implementation, what are you saying is the difference for example for the call to mint function which is only ever called once in the test but is reported under both the Proxy and the implementation?

As for the cold/warm storage costs, that cannot explain the gas cost difference as I am executing the exact same single unit test on foundry and hardhat and executing both on a clean slate of contracts as you would expect in a unit test.

There is 37,266 gas difference in the first call and 24,048 gas difference in the second call so still a lot unaccounted for.

EDIT: And also why the Proxy gas costs are only shown sometimes?

@onbjerg
Copy link
Member

onbjerg commented Jul 28, 2022

still don't understand the separate reports you have for the proxy and the implementation, what are you saying is the difference for example for the call to mint function which is only ever called once in the test but is reported under both the Proxy and the implementation?

I would assume that the proxy points to MockSoundEditionV1? If so, because we are constructing the gas reports using traces, the calldata will hit the proxy and the implementation, so it will show up as two calls. The gas used for the proxy accounts for all gas of the proxy itself + the call to the underlying implementation, whereas the gas usage of the second contract is just the gas usage for the logic in that contract. If we had to filter it out, we would need to give special treatment to proxies, and there are so many implementations it could prove to be somewhat unmaintainable.

I think Hardhat only produces 1 report because it looks at the transaction you directly send to the RPC. We don't have an RPC, so we decode all calls between the contracts.

As for the cold/warm storage costs, that cannot explain the gas cost difference as I am executing the exact same single unit test on foundry and hardhat and executing both on a clean slate of contracts as you would expect in a unit test.

It can still explain some of the difference - Hardhat does RPC calls, so 100% clean slate, where a call to a test function in Foundry necessarily is already not a 100% clean slate, since there is some setup involved with calling and executing the test function itself in the first place. It might not make up for the entire difference, but I was just noting that this is something that I expect to have an impact.

There is 37,266 gas difference in the first call and 24,048 gas difference in the second call so still a lot unaccounted for.

The gas reports Forge produces is without the 21000 tx gas stipend, whereas I assume Hardhat includes it - that accounts for 21000 of the difference, so we have ~16k and ~3k left to account for, I assume. I'm not entirely sure how the two scenarios differ - is there a chance you could expand a bit on it?

EDIT: And also why the Proxy gas costs are only shown sometimes?

No that sounds pretty weird 🤔 Do you have some pointers on how I could maybe reproduce?

@elenadimitrova
Copy link
Author

I've reproduced the issue in an MVP in https://github.com/elenadimitrova/foundry-hardhat-gas-compare where you can run the two identical set of tests and see the difference between foundry

Screenshot 2022-07-29 at 14 31 31

vs hardhat
Screenshot 2022-07-29 at 14 30 31

@onbjerg
Copy link
Member

onbjerg commented Jul 30, 2022

  1. The difference for MinimalProxyFactory is accounted for as explained above; Foundry's output does not include the 21000 gas stipend. There is a small difference of about 400 gas outside of that which is probably explained by the fact that the account is hot as you are not deploying the factory in setUp
  2. I would assume it is a similar story for the mint call on MintModule - both the implementation and the MintModule contract are hot
  3. The RouterProxy contract shows up because we analyze gas usage based on EVM traces, not individual start-to-end transactions like Hardhat does. It's not really possible for us to do it like Hardhat does, since each test is executed within the context of a single transaction

I don't have a lot of time currently to go in and individually dissect each opcode to find out if there is some larger thing unaccounted for, but the EVM implementation we use conforms to the Ethereum tests that all nodes use, so I am fairly confident we are not far off

We've also investigated the smaller gas discrepancies more closely before, where we did account for it: https://github.com/mds1/convex-shutdown-simulation (see the note about Foundry)

I'd love to hear some input/ideas from anyone who has suggestions on how we can improve the accuracy of the reports, but it does not seem like a trivial problem to solve due to how Forge executes tests vs other frameworks

@elenadimitrova
Copy link
Author

@onbjerg I have confirmed the storage slots aren't "hot" as you explain, with this commit which is testing storage slots are "cold" before writing.

There is effectively around 5200-5500 unexplained gas difference in the mint functions in both types of proxies used. I think this is non negligible considering I am testing a minimal repro scenario, with potentially this gas difference being more significant in more complex cases.

foundry hardhat diff
MintImplementation.mint 22425 + 210000 = 43425 48660 -5235
MintModule.mint 23236 + 21000 = 44236 49808 -5572

This is quite critical considering the importance of gas cost analysis in smart contracts for decision making, i.e. we are currently trying to weigh in the costs of the two different proxy patters and finding it difficult to trust what foundry reports.

@onbjerg
Copy link
Member

onbjerg commented Aug 1, 2022

The account is still hot, though. For more information on what that means: https://eips.ethereum.org/EIPS/eip-2929

Specifically:

When an address is either the target of a (EXTCODESIZE (0x3B), EXTCODECOPY (0x3C), EXTCODEHASH (0x3F) or BALANCE (0x31)) opcode or the target of a (CALL (0xF1), CALLCODE (0xF2), DELEGATECALL (0xF4), STATICCALL (0xFA)) opcode, the gas costs are computed as follows:

  • If the target is not in accessed_addresses, charge COLD_ACCOUNT_ACCESS_COST gas, and add the address to accessed_addresses.
  • Otherwise, charge WARM_STORAGE_READ_COST gas.

Since the contract is created in the same transaction as you are calling mint on it, the account itself is considered to be hot. I'm sure there are other such cases here that explain the discrepancy, since it is not only SLOAD/SSTORE that is affected by EIP 2929.

@zerosnacks
Copy link
Member

This has likely been resolved by the new isolation mode enabled by default when using --gas-report: #7186

Tentatively closing as resolved, feel free to re-open if this is still an issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-gas-snapshots Area: gas snapshotting/reporting C-forge Command: forge Cmd-forge-test Command: forge test T-bug Type: bug
Projects
Archived in project
Development

No branches or pull requests

3 participants