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

Better snapshot management: no duplicates; keep same snapshot alive; better cleanup #6366

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

naps62
Copy link
Contributor

@naps62 naps62 commented Nov 20, 2023

Motivation

Current snapshot logic seems a bit finicky:

  • Callling evm_snapshot multiple times will create duplicate copies (even though they should be identical)
  • reverting to a snapshot will remove it from the active_snapshots list, even though it is theoretically still valid, and would be useful to allow users to continuously revert to the same past point. The backend re-pushes it to the same db, but since we remove it from active_snapshots it becomes unreachable by the RPC anyway
  • the DB deletes all future snapshots, but the backend active_snapshots map would retain them

All these problems get exposed a bit more due to what I'm also proposing in #6364 . for an outside application to manage and view snapshots, this would be very limiting

Seems this was sort of tackled at #5487, but not fully?

Solution

  • if evm_snapshot is called at a point for which a snapshot already exists, we do nothing and return that same ID instead
  • during evm_revert, we don't remove the snapshot from the list, instead we clone it, apply it, and remove only future ones

@naps62
Copy link
Contributor Author

naps62 commented Nov 20, 2023

Need some help particularly reviewing the ForkedDatabase part. not as familiar with that side of the codebase

Copy link
Member

@mattsse mattsse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Imo makes sense to also keep snapshots alive and mimic forge behaviour.

but how do we solve deleting snapshots?

@@ -39,6 +39,20 @@ impl<T> Snapshots<T> {
snapshot
}

/// Removes all snapshots after `id`.
///
/// This will not remove the `id` itself, only clone its data, since the snapshot will still
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this make it impossible to delete snapshots?

same issue as #6411

Copy link
Contributor Author

@naps62 naps62 Nov 27, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

makes it impossible to delete the very first snapshot. but reverting to the oldest one should revert all other ones. so arguably not a big problem?

or did I misunderstand your point?
anyway, probably a cheatcode for cleaning snapshots, as you suggested in that issue, makes sense. I can also take a shot at that, but probably as a separate PR?

Copy link
Member

@mattsse mattsse Nov 27, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah same issue:

think of the vm calls as rpc calls, this will never delete snapshots:

for(uint cc = 0; cc < 10000; cc++) {
                preState = vm.snapshot();
                vm.revertTo(preState);
            }

anyway, probably a cheatcode for cleaning snapshots, as you suggested in that issue, makes sense. I can also take a shot at that, but probably as a separate PR?

a followup PR for this would be great @naps62

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh nevermind, I see your point now. In my head I was mixing the active_snapshots map with the actual snapshot storage (the first gets cleaned up, the second doesn't)

I can take care of that 👍

crates/evm/core/src/snapshot.rs Show resolved Hide resolved
crates/anvil/src/eth/backend/mem/mod.rs Show resolved Hide resolved
Copy link
Member

@mattsse mattsse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

okay, since we persisted snapshots on revert for forge, we should do the same for anvil with this PR

though we need a solution to actually delete them

Copy link
Member

@Evalir Evalir left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry, have a nit re: deleting snapshots > id

@@ -686,6 +693,8 @@ impl Backend {
..Default::default()
};
}

self.active_snapshots.lock().retain(|key, _| *key <= id);
Copy link
Member

@Evalir Evalir Nov 27, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this removing snapshots that are > id? If so, this behavior was removed with #5487 and should not be reintroduced (We deviate from ganache's & hardhat's behavior on this). We need a cheatcode that can 100% delete snapshots to deal with memory growth, but we now have users which rely on snapshots not getting deleted at all.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm. so snapshots > id are still useful?
I was under the impression they could no longer be used. but perhaps it was just a limitation on anvil's side? never tested that with forge

Copy link
Member

@Evalir Evalir Nov 27, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct, they're not deleted and can be reused now. To be more specific, snapshots now:

  • Can be used repeatedly (e.g, create a snapshot outside a loop, and on every loop iteration, modify state and then revert back to the snapshot created outside the loop)
  • Deleting a snapshot does not delete future snapshots. (e.g, deleting snapshot id does not delete id + 1, id + 2... id + n)

this wasn't really a limitation, it just was behavior we inherited from hardhat—but so many people requested persisting snapshots that we modified the behavior.

Copy link
Contributor Author

@naps62 naps62 Nov 27, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry, but I'm a bit confused right now

the current behaviour seems to be that snapshots > id are no longer usable. at least with anvil:

~/contrib/foundry-rs
❯ anvil --version
anvil 0.2.0 (6432031 2023-11-27T00:17:45.338056018Z)

~/contrib/foundry-rs
❯ cast rpc --rpc-url localhost:8545 evm_snapshot
"0x0"

~/contrib/foundry-rs
❯ cast rpc --rpc-url localhost:8545 evm_mine
"0x0"

~/contrib/foundry-rs
❯ cast rpc --rpc-url localhost:8545 evm_snapshot
"0x1"

~/contrib/foundry-rs
❯ cast rpc --rpc-url localhost:8545 evm_revert 0x0
true

~/contrib/foundry-rs
❯ cast rpc --rpc-url localhost:8545 evm_revert 0x1
Error:
(code: -32001, message: Resource not found, data: None)

The issues linked seem to be about forge tests, and your fix above is on evm/. so perhaps this is just some inconsistency between forge & anvil?

From glancing at the anvil code, I'd guess future snapshots don't work, due to this call (which seems to be where the error above comes from), but not sure

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep! sorry for the confusion here. It's true that anvil and forge behave differently—anvil should match forge's behavior which is what i described above.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

okay after reading the original spec, idk how to proceed with this.

the behaviour described by @Evalir is much simpler in forge because in forge tests we never commit (1 test = 1 tx), so snapshots are just memory snapshots effectively. with a node you run into a variety of issues

hardhat and ganache have two for evm_revert:

  1. snapshot is deleted on revert
  2. all subsequent snapshots are deleted

Revert the state of the blockchain to a previous snapshot. Takes a single parameter, which is the snapshot id to revert to. This deletes the given snapshot, as well as any snapshots taken after (e.g.: reverting to id 0x1 will delete snapshots with ids 0x1, 0x2, etc.)

https://github.com/trufflesuite/ganache/blob/ef1858d5d6f27e4baeb75cccd57fb3dc77a45ae8/src/chains/ethereum/ethereum/RPC-METHODS.md#evm_revert

I'm not sure we should break from the OG behaviour of evm_snapshot+evm_revert

Copy link
Contributor Author

@naps62 naps62 Dec 1, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for taking the time!

it sounds, from what you're describing, that it's a much more complex problem to keep future snapshots around with a node. and even if possible, would be out of scope here.

seems like it could be warranted to have different behaviour between anvil & forge then (which I believe it's what this PR achieves already, but need to double-check, don't fully remember right now).

as for the decision of whether or not to delete the target snapshot:
The OG behaviour seems a bit weird to me (as in, it's limiting usability for no apparent reason). I'd be fine with break it, but also understand that keeping within spec is important and I probably don't know the full repercussions.

If we do keep it, I can work around that on my app's side: by automatically creating a new snapshot everytime a revert occurs. not ideal, but I can make it work

so I guess it's your call. I can fix the PR according to whatever you decide :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

eems like it could be warranted to have different behaviour between anvil & forge then

agree

The OG behaviour seems a bit weird to me

yeah they even mention in the docs that in order to achieve this you're supposed to create a new snapshot after revert.

I'd be fine with break it

I think that's reasonable.

Though, we'd need additional call or arguments to get that behaviour. because if we keep it then there's currently no way to delete a snapshot.

we can however simply add a new optional bool parameter to evm_revert, this doesn't break the OG behaviour but allows you to force keeping it.

wdyt?

Copy link
Contributor Author

@naps62 naps62 Dec 1, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add a new optional bool parameter to evm_revert

didn't think of this, but sounds good!

so, would this be a good plan?

  • refactor this PR to simply keep the hardhat behaviour. i.e.: just solve point 3 from the original PR description, which seems to be a bug
  • 2nd PR: add the optional evm_revert(id, true) which keeps the target snapshot alive instead of deleting it
  • 3rd PR: add anvil_delete_snapshot(id) and/or anvil_clear_snapshots

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sg!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-anvil Command: anvil
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants