Change in memory limit calculation

[DaniPopes]

We've encountered a memory limit while bumping Revm from 3.5.0 to 1609e07 (v26...1609e07) in foundry-rs/foundry#6281 while testing pcaversaccio/snekmate's Multicall.

Some calls to Multicall::multicall_self in these tests use about 60MiB in total, but split across multiple internal calls. Foundry by default uses 32MiB memory limit, so these calls failed after updating Revm with MemoryLimitOOG.

I believe the root cause is in this commit: b5aa4c9
Since it's the only one that changed this logic.

Before:

new_size > ($interp.memory_limit as usize)

After:

(self.last_checkpoint + new_size) as u64 > self.memory_limit

I'm not sure if this is correct. Should this just be checking new_size only again?

cc @rakita @lorenzofero @pcaversaccio

[thedevbirb]

Hi Dani! Yes I probably had a misunderstanding about memory limit when I worked on this, I'm sorry. Right now it checks that, between all contexts, the amount of memory allocated does not overcome memory_limit.

Looking at the original code it seems that you're allowed to allocate up to 32MiB in every context, which is different than the behavior of the current shared memory.

If you agree I can make a little PR to fix this: as you said we should be checking new_size only.

[rakita]

memory_limit was introduced for foundry, I noticed this change in behaviour It felt nicer that it is over all memory allocated, if it was 32mb per stack than the maximum is 32mb*1024. Like this better but wouldn't mind reverting back to limit per stack

[pcaversaccio]

@rakita It would be awesome to revert this change as since foundry-rs/foundry#6281 is now merged and all my commits in snekmate will now fail (I use foundry nightly builds) with these 2 specific failures. Maybe as a background why I do such crazy tests: Snekmate is a Vyper library and in Vyper you statically allocate memory at compile time for e.g. dynamic arrays. Since snekmate should be a generic module essentially I put high placeholders there in multicall for the memory allocation. People using this contract can reduce it according to their needs. But I test kind of the upper limit.

[rakita]

@rakita It would be awesome to revert this change as since foundry-rs/foundry#6281 is now merged and all my commits in snekmate will now fail (I use foundry nightly builds) with these 2 specific failures. Maybe as a background why I do such crazy tests: Snekmate is a Vyper library and in Vyper you statically allocate memory at compile time for e.g. dynamic arrays. Since snekmate should be a generic module essentially I put high placeholders there in multicall for the memory allocation. People using this contract can reduce it according to their needs. But I test kind of the upper limit.

Solution could be for foundry to expose memory limit as a config or just bump current value to 256mb or 512mb, that would solve the problem.

[pcaversaccio]

I'm indifferent to how it's solved and leave that to you guys; I just need a solution that works :)

[DaniPopes]

We believe the new behavior makes more sense, so we're bumping the default memory limit to 128MiB in foundry-rs/foundry#6338. Both config (profile.memory_limit) and CLI option (--memory-limit) are available to modify this default already.