feat(cheatcodes): enable --isolate by default for tests when snapshotGas cheatcodes are used

[gabkov]

Component

Forge

Describe the feature you would like

Currently, --gas-report uses --isolate by default (as noted in #6578 (comment)) to ensure accurate gas measurements. Additionally, it has been suggested (see #8573 (comment)) that --isolate should be used for the most precise gas measurements when vm.lastCallGas() is utilized.

Given this context, I think it would be logical to enable --isolate by default for tests that include snapshotGas cheatcodes.

For example, the current command:

FORGE_SNAPSHOT_CHECK=true forge test --isolate --mt testGas -vvv

could be simplified to:

FORGE_SNAPSHOT_CHECK=true forge test --mt testGas -vvv

Additional context

No response

[zerosnacks]

Hi @gabkov

Thanks for flagging this, I was working on this in #9649 but also prefer the approach of automatically enabling isolation mode when the cheatcode is used rather than warning the user it should be enabled.

[simplyoptimistic]

@zerosnacks

Thanks for flagging this, I was working on this in #9649 but also prefer the approach of automatically enabling isolation mode when the cheatcode is used rather than warning the user it should be enabled.

Just want to clarify your comment but is this feasible / in the works? tyia

[zerosnacks]

Automatic detection of the use of the cheatcode would be ideal but I currently don't see a feasible way of doing that due to the program flow.

An alternative would be to make gas snapshot running explict, similar to gas report or coverage.

Similar to coverage turning of optimizations by default we enable isolation mode when running gas snapshots.

Plan would be to:

• Rename gas_snapshot_emit to gas_snapshot and set tofalse by default
• Only run gas_snapshot if passed as flag, similar to --gas-report
• When gas_snapshot is enabled we automatically configure isolation mode as it is a requirement for accurate gas

Optional:

• rename FORGE_SNAPSHOT_CHECK into FORGE_GAS_SNAPSHOT_CHECK
• rename FORGE_SNAPSHOT into FORGE_GAS_SNAPSHOT

I want to think a bit more through it / collect feedback before starting considering this would a breaking change.

As an intermediary solution I want to document isolation mode in the book and the requirement to enable it when creating gas snapshots.

I'll add it to the 1.1 milestone for me so it stays top of mind.

[klkvr]

@zerosnacks @grandizzy should we consider just making --isolate a default?

[grandizzy]

@zerosnacks @grandizzy should we consider just making --isolate a default?

That's a good option but I think we should first solve these 2
#7277
#9564

[zerosnacks]

I was hesitant to default to isolate due to possible overhead but it is a much more intuitive way for end users to understand how tests are ran. If we feel that the performance degradation is small I would definitely be in favor.

[simplyoptimistic]

I did some very quick testing a while back and I believe the performance degradation is particularly bad for fuzzing, so if possible, please review this as part of the decision making process (and if it's significant, consider excluding --isolate from fuzzing).

[zerosnacks]

Basic benchmarks against latest nightly (9066359bc0), including #9938 shows with --isolate:

Ran 2 tests for test/Counter.t.sol:CounterTest
[PASS] testFuzz_SetNumber(uint256) (runs: 100000, μ: 55425, ~: 55606)
[PASS] test_Increment() (gas: 54915)
Suite result: ok. 2 passed; 0 failed; 0 skipped; finished in 1.42s (1.42s CPU time)

Compared to latest stable (1.0.0-stable, e144b82070):

Ran 2 tests for test/Counter.t.sol:CounterTest
[PASS] testFuzz_SetNumber(uint256) (runs: 100000, μ: 53445, ~: 53606)
[PASS] test_Increment() (gas: 52915)
Suite result: ok. 2 passed; 0 failed; 0 skipped; finished in 6.39s (6.39s CPU time)

In both versions without --isolate, no difference:

Ran 2 tests for test/Counter.t.sol:CounterTest
[PASS] testFuzz_SetNumber(uint256) (runs: 100000, μ: 32113, ~: 32354)
[PASS] test_Increment() (gas: 31851)
Suite result: ok. 2 passed; 0 failed; 0 skipped; finished in 956.27ms (955.84ms CPU time)

So on a simple scenario roughly 1.5x overhead currently