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

feat: vm.finalize #2844

Closed
2 tasks done
0xA5DF opened this issue Aug 19, 2022 · 5 comments
Closed
2 tasks done

feat: vm.finalize #2844

0xA5DF opened this issue Aug 19, 2022 · 5 comments
Labels
A-gas-snapshots Area: gas snapshotting/reporting C-forge Command: forge Cmd-forge-test Command: forge test D-hard Difficulty: hard P-low Priority: low T-bug Type: bug

Comments

@0xA5DF
Copy link

0xA5DF commented Aug 19, 2022

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 (6b3db2a 2022-08-19T00:04:42.579272325Z)

What command(s) is the bug in?

forge test --gas-report

Operating System

Linux

Describe the bug

#1543 discusses 'each test being considered one tx' regarding selfdestruct.
However there's another aspect that this affects - gas reports.

Since some opcodes depend on whether the address/key is warm or cold (i.e. if it has been used in current tx), considering the test as one tx can make the gas cost estimation significantly lower (since using cold key/address are very expensive, for example warm sload would cost 0.1K units while a cold one would cost 2.1K).

I've created a gist that demonstrates that.

For full list of opcodes that are affected by this, see this link.

Solution

As suggested in #1543, maybe allow to finalize a tx from the test (maybe also allow an auto-finalizing before each call to non-test contracts).

@0xA5DF 0xA5DF added the T-bug Type: bug label Aug 19, 2022
@gakonst gakonst added this to Foundry Aug 19, 2022
@gakonst gakonst moved this to Todo in Foundry Aug 19, 2022
@rkrasiuk rkrasiuk added Cmd-forge-test Command: forge test C-forge Command: forge A-gas-snapshots Area: gas snapshotting/reporting labels Aug 19, 2022
@onbjerg onbjerg added D-hard Difficulty: hard P-low Priority: low labels Aug 19, 2022
@onbjerg onbjerg changed the title Forge considers each test as one tx, significantly affecting gas usage in some cases feat: vm.finalize Aug 19, 2022
@k06a
Copy link

k06a commented Sep 29, 2022

Any progress on this?

@mattsse
Copy link
Member

mattsse commented Sep 29, 2022

I can see how this is useful.

just to summarize, vm.finalize should

commit the current state changes in a transaction?

what should happen with the current (in memory) state of the test at this point? Like all variables etc.?

would cheatcodes that load/unload address/slots be equivalent here?

@0xA5DF
Copy link
Author

0xA5DF commented Oct 2, 2022

The finalize() is needed to solve 2 issues:

  1. As mentioned here, gas calculations depends on if a key or address is 'warm' or 'cold'. At the beginning of each tx all addresses and keys are cold, therefore finalize() should make all keys and addresses cold.
  2. As mentioned in issue bug: selfdestruct has no effect in test whereas a user expects it would #1543, selfdestruct should take effect at the end of a tx, therefore finalize() should also clear out the code and storage of destructed addresses.

I don't think there's a need to change the state of the memory of the stack, since those don't affect external calls.

The way I see it is that usually the 'real' tx are the external calls done to the source code, and finalize() would be called in the test code. Therefore as long as the calls to those external functions in the source code act as separate tx that's all we need.

It'd also be nice if there would be an option to set the finalize function to run automatically between each call from the test-code to an external function in the source code, since that's when it's normally needed (people are just unaware of this issue currently, and are getting wrong gas calculations due to that. Enabling this auto-finalize by default would help them get more accurate result without putting too much effort). But this might be too complicated to implement so I'll understand if you'll skip this 'auto-finalize'.

@deluca-mike
Copy link

deluca-mike commented Dec 1, 2023

I guess a yearly poke is in order. Any update on this? Not being able to inform the VM to reset access lists effectively makes almost all gas reports generated by forge useless, so I'd consider this to be a pretty important feature, or dare I say, fix.

Thanks!

@zerosnacks
Copy link
Member

I guess a yearly poke is in order. Any update on this? Not being able to inform the VM to reset access lists effectively makes almost all gas reports generated by forge useless, so I'd consider this to be a pretty important feature, or dare I say, fix.

Thanks!

We've recently added support for isolation mode (--isolate) which should address the issue. This has been enabled by default for --gas-report. See: #7186

Marking this as not planned as it is was superseded by the isolation mode.

@zerosnacks zerosnacks closed this as not planned Won't fix, can't repro, duplicate, stale Jun 26, 2024
@jenpaff jenpaff moved this from Todo to Completed in Foundry Sep 30, 2024
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 D-hard Difficulty: hard P-low Priority: low T-bug Type: bug
Projects
Archived in project
Development

No branches or pull requests

7 participants