vm.snapshot / vm.revertTo non deterministic

[sakulstra]

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 (08a629a 2023-06-03T00:04:22.625130135Z)

What command(s) is the bug in?

vm.snapshot

Operating System

Linux

Describe the bug

There seems to be an issue with nested snapshots and reverts.
I don't really have an explanation of what is happening exactly.

I have a testsuite iterating over a list of assets and performing some tasks, including deposit some link on aave.
Now the interesting thing is, when i create a single snapshot in the beginning and revert to it after testing each asset at some point LINK will exceed supply cap, although i'm always depositing the same amount and reverting to the previous state.

I assume that somehow:
These revertTo: https://github.com/bgd-labs/aave-helpers/blob/master/src/ProtocolV3TestBase.sol#L147
Seem to mess with this revertTo: https://github.com/bgd-labs/aave-helpers/blob/master/src/ProtocolV3TestBase.sol#L115

This code fails:

uint256 snapshot = vm.snapshot();
    for (uint256 i; i < configs.length; i++) {
      if (_includeInE2e(configs[i])) {
        e2eTestAsset(pool, collateralConfig, configs[i]);
        vm.revertTo(snapshot);
      }
}

This code works:

    for (uint256 i; i < configs.length; i++) {
      if (_includeInE2e(configs[i])) {
        uint256 snapshot = vm.snapshot();
        e2eTestAsset(pool, collateralConfig, configs[i]);
        vm.revertTo(snapshot);
      }
}

Analysing the code the behavior should be the same as it shouldn't matter if i:

• create a snapshot at the beginning and revert to it at the end of each loop
• create a new snapshot at the beginning of each loop
  So there seems to be an issue with creating snapshots / reverting to a snapshot.

As a reproduction you can run: forge test --match-contract ProtocolV3TestE2ETestAll -vv and change the snapshot accordingly.

[mds1]

@Evalir just flagging this one as it may be related to snapshot/revert soundness issues from #4974 or #3792 (also maybe the latter should have also been closed by #4794? not sure haven't looked closely), which we should def resolve before v1

[Evalir]

Yup this looks quite weird, will prio taking a look at this tomorrow, it should've been fixed already.

[sakulstra]

Hello,
seems like we're again facing this issue - this time the reproduction looks a bit simpler so I created it here: bgd-labs/aave-helpers#120

So what we roughly do is:

1. create Snapshot
2. doStuff() & revert
3. doSthElse() & revert -> if commenting out the test will succeed
4. doSthElse() & revert -> here the test will fail unexpectedly

To mitigate we noticed that when creating another snapshot within doSthElse the test will suddenly succeed (although there is no plausible reason why it should).

[Evalir]

Thanks @sakulstra — I'll take a look soon

[Evalir]

Hey @sakulstra — so i took a bit of a dive into this, and this is actually expected behavior:

• This is how the feature is documented
• This is also how hardhat & ganache work

The trick is essentially to "regen" the snapshot after being used (e.g on a loop on the OG post). You can't revert to a snapshot more than once.

[sakulstra]

@Evalir oh wow. Somehow didn't stumble over this.

Do you think that's desired behavior, though?
To me it feels quite unnatural.

[Evalir]

Hmm yeah, I think maybe we could have something like a "persistent" snapshot which is not deleted after usage, or some flag on the snapshot cheatcode, or a flag in revertTo as well. The first one would not be a breaking change though so could be implemented and brought in sooner. I do see the value in this feature imho.

wdyt @mds1 and @mattsse ? I haven't dug deep enough to see technical limitations in implementing this, but I doubt there'd be any. I'm just leaning towards a new cheatcode to maintain parity with hardhat/ganache's behavior with the standard one.

[mds1]

Does anyone rely on the existing behavior? I can't imagine so, and making snapshots persistent so you can always revert to them doesn't feel like a breaking change, and it also feels strictly better to make them persistent. Though I don't know what the motivation for making them one-time-use was (e.g. technical, footgun, etc). Might be some info in the PRs that implemented the feature

[Evalir]

Does anyone rely on the existing behavior? I can't imagine so, and making snapshots persistent so you can always revert to them doesn't feel like a breaking change, and it also feels strictly better to make them persistent. Though I don't know what the motivation for making them one-time-use was (e.g. technical, footgun, etc). Might be some info in the PRs that implemented the feature

Yeah AFAIK now the current behavior is to mimic hardhat/ganache's behavior, but I'm happy to make that change as IMHO snapshots being persistent is better, albeit the departure from the previous behavior is notable and i wanna be mindful of it.