fix(vm.cool) Persist storage changes #5852

ditto-eth · 2023-09-19T04:07:31Z

Motivation

See #5830 (comment) (the earlier merged PR) for more context. Had to start over since that approach was ultimately wrong due to potential issues with side effects and also that storage.clear() actually reset the storage so that values didn't persist.

~~this is a wip, a lot of things to finish up but wanted to get feedback as i finally got somewhere.~~ (also happy to chat in more real time on twitter/telegram)

Help Test!

gh pr checkout 5852
cargo r --bin forge test --contracts ../proj_root --config-path ../proj_root/foundry.toml

apologies for the lack of foundry/rust knowledge!
what needs to be done in relation to refunds/reverts, if at all?
~~removed the account warm/cold for now, just focusing on the slots~~ done
- (@mds1 q was about preventing certain addresses from being cold because they are always warm at the start of the tx (precompiles, from, to). How do you know which address is from or to? (the test address?) do I just check state for the first address touched? q around what the expected behavior should be

@mds1: For the from and to address it's complicated because of how forge works (each test is one giant transaction), but perhaps we document this behavior (i.e. that from and to are always warm) to make sure users use this cheat properly. The other option is to pretend all calls that are executed from the test contract are individual txs, and therefore treat the sender address (e.g. the default sender or prank/broadcast addr) AND the target of each call as always being warm. This might get complicated though.

test multiple slots/addresses
function cool(StorageSlot[] memory slots) external; not implemented but should be simple if this approach works (i think this can just be a separate pr)
in testing against my proj, getting a few gas difference when running the same code twice (the latter with vm.cool but it's not a sstore/sload issue then
not sure exactly what tests to write to verify gas is expected compared to mainnet etc

Solution

via revm telegram:

mattsse: the issue with gas tracking here is, that gas is always deducted, only refunds are added back
so even if you clear something, this should not affect gas metering
I think the only way to do this is playing the Gas return value of the Inspector::call{_end} function calls
rakita: This could potentially introduce sideeffects, as changed storage values gets removed. You can maybe craete your own JournalState (that would need to track reverts) and just override the gas used inside step_end

^ approach seems to generally make sense? added a step_end to cheatcodes inspector, so on SSTORE/SLOAD i can manipulate the interpreter.gas value. The idea is that a certain amount of cost needs to be added back depending on sstore.

~~fyi i kept all my println calls in case someone wanted to check it out locally, will remove when the pr is ready to go~~ (moved to vm.cool-fix-print branch.

    pub cool: HashMap<Address, CoolState>,
    pub cool_storage: HashMap<Address, HashMap<U256, CoolStorageState>>,
    pub cool_additional_gas_next_op: u64,

vm.cool sets the variables ^. It just saves whether an address' slots are cool/warm or not by resetting it everytime the vm.cool is called. then just calculate the added gas cost (usually +2100), but different if an sstore is done and mark the slot as "warm" so the regular gas is sufficient (until the next vm.cool is called if necessary). can add a few more kinds of tests.

also i'm sure it can be optimized (didn't know any rust till last pr, so understand that there might be simpler refactors
edit: rebased a few times to fix the type changes (that took a while but made it easier overall)

refs
https://eips.ethereum.org/EIPS/eip-2200
https://eips.ethereum.org/EIPS/eip-2929
evm.codes

crates/evm/src/executor/inspector/cheatcodes/mod.rs

Evalir · 2023-09-21T02:07:23Z

just looping in @mds1 for discussing approach — will review soon!

crates/evm/src/executor/inspector/cheatcodes/mod.rs

Evalir

Still haven't reviewed for correctness—some nits

crates/evm/src/executor/inspector/cheatcodes/env.rs

crates/evm/src/executor/inspector/cheatcodes/mod.rs

Evalir · 2023-09-21T13:52:56Z

crates/evm/src/executor/inspector/cheatcodes/mod.rs

+#[derive(Clone, Debug)]
+pub enum CoolState {
+    CalledAndAccessed,
+    Called,
+}
+
+#[derive(Clone, Debug)]
+pub enum CoolStorageState {
+    Warm,
+    WarmAndSSTORED,


Let's add docs for these—wdyt about naming @mds1?

FWIW I can't tell what this enum is supposed to represent from the names here alone, so we probably need to change the names and agree with adding docs. CoolStorageState::Warm and CoolStorageState::WarmAndSSTORED feel like conflicting names, e.g. how is the state cool and warm? Also maybe WarmWithSSTORE or WarmWithSstore would be easier to read since SSTORED isn't a usage I've seen

right, i didn't know what to name the hashmap itself so it's coolstoragestate but it's more the storage state that is tracked when calling vm.cool, not that the state is actually both cool and warm at the same time. i'm ok just calling storagestate but it felt generic

it's tracking whether it's warm, or warm and the storage store was set, or that it's cool. Maybe very confusing but if it's empty it means it's cool (because each vm.cool resets the whole hashmap to be empty)

so it's either empty value (none), or warm (the sload makes it warm), or warm and sstore was called

I suppose this means cool, sloaded, or sstored, and each of those should change the gas increase

crates/evm/src/executor/inspector/cheatcodes/mod.rs

ditto-eth · 2023-09-22T16:33:51Z

ok updated the renames, attempted to fix the ci issues..

ditto-eth · 2023-09-25T17:57:15Z

test integration (1/3) keeps failing for me - the local tests pass fine but i don't know how to run these tests locally so I have to guess what the issue is to try to fix it. I assume it's a different build/nightly/foundry toml so the gas has different values depending on the optimizations? it is true these tests are too exact so test could be flaky if solc changes

thread 'cheats::test_cheats_local' panicked at 'called `Result::unwrap()` on an `Err` value: Test testCool_call() did not pass as expected.
Reason: None
Logs:
Error: a == b not satisfied [uint]
  Expected: 44835
    Actual: 45299
Error: a == b not satisfied [uint]
  Expected: 27735
    Actual: 28199

Evalir · 2023-09-25T17:58:26Z

@ditto-eth you can use cargo t cheats_local

ditto-eth · 2023-09-25T18:45:56Z

@ditto-eth you can use cargo t cheats_local

ah i'd been using a custom command I made up cargo r --bin forge test --contracts testdata/cheats/Cool.t.sol --match-test testCool -vv, but that passes for me so i'm confused by the ci failure (there seems to be a gas difference of 13 in some tests and now others. The command you mentioned passes and either finishes but doesn't exit or is hanging?

ditto-eth · 2023-09-25T18:53:58Z

crates/evm/src/executor/inspector/cheatcodes/mod.rs

+
+    // if cool cheatcode was ever called on this address
+    let contract_address = interpreter.contract().address.to_ethers();
+    if state.addresses.get(&contract_address).is_some() {


vm.cool involves two parts: making the address itself cool, making the storage slots cool

check that vm.cool was passed to this address

if so, track a few opcodes (call/balance/etc) to mark the address as warm, as well as queuing up additional gas to be added to the next opcode. a warm address is 100, a cool one is 2600, so add 2500.

check the storage slots

if.an sload or sstore, mark address as warm but add different wants of extra gas depending on the situation.

emo-eth · 2023-09-25T20:11:45Z

Curious what the behavior will be for cold-marked accounts/slots accessed within reverting callframes (some context)

For EVM-accurate measurements, reverting callframes will have to "re-cool" vm.cooled slots.

This is something I was wrestling with in my much more roundabout account/slot enumeration cheatcodes.

ditto-eth · 2023-09-25T20:28:54Z

awesome thanks for the feedback/comment!

Curious what the behavior will be for cold-marked accounts/slots accessed within reverting callframes (some context)

i'm not that familiar - is this when a new contract/function is called that fails within a try/catch? if so, then yea it would need to deduct the same amount back? i didn't think that would be a common case in the context of a gas test but maybe just ignorant of that (and why I wasn't sure what handling reverts meant as in my mind it would just fail the test anyway)

emo-eth · 2023-09-25T20:39:52Z

awesome thanks for the feedback/comment!

Curious what the behavior will be for cold-marked accounts/slots accessed within reverting callframes (some context)

i'm not that familiar - is this when a new contract/function is called that fails within a try/catch? if so, then yea it would need to deduct the same amount back? i didn't think that would be a common case in the context of a gas test but maybe just ignorant of that (and why I wasn't sure what handling reverts meant as in my mind it would just fail the test anyway)

Yeah, a try/catch or a low-level call where success is checked/handled/ignored. It's definitely a more niche case – but there are certainly contracts designed to handle failing edge cases gracefully. (A fun one is forcefully transferring native tokens to smart contracts that rejects them, such as Solady's selfdestruct mechanism)

It probably shouldn't be a blocker to shipping the fix, since this helps a ton with getting gas reports more accurate – but full accuracy for complex protocols probably relies on supporting these edge cases.

ditto-eth · 2023-09-25T21:09:20Z

i guess the selfdestruct won't be happening if it's being deprecated later i heard?

but full accuracy for complex protocols probably relies on supporting these edge cases.

down to make it accurate as possible in the next PRs, as i'm trying to test it on my protocol now which spurred my interest in figuring this out!

also in writing the tests, i started getting confused what i'd even expect the result to be 🙂 so curious on reviewing that. tests are probably based on number of opt runs which change the opcodes being done (this is annoying to test sload/sstore, as you can't just do a contract.var since it will get optimized out, so have to set it to a variable which costs some other amount of gas) - there's no way to just do the basic read/write operations without assembly right? I wonder if one can turn off the compiler settings for specific tests and do sload/sstore in yul, but i'd probably need some help on that?

ditto-eth · 2023-09-25T22:25:48Z

does anyone know how the gas is modified with reverts normally? sounds like i'd need to track the additional gas per frame and then subtract it if revert goes through? maybe the code related to gas metering in mod.rs helps, it seems to just reset the gas at various opcodes.

emo-eth · 2023-09-26T20:02:05Z

does anyone know how the gas is modified with reverts normally? sounds like i'd need to track the additional gas per frame and then subtract it if revert goes through? maybe the code related to gas metering in mod.rs helps, it seems to just reset the gas at various opcodes.

I think moreso that vm.cooled accounts/slots should have the extra gas charged within a callframe – but if that callframe gets reverted, Forge should re-mark the accounts/slots as vm.cooled again. "Cold" accounts/slots will be charged "cold" gas within a reverting callframe, but they will still be cold after the callframe exists and the surcharge will need to be added again if they get accessed again.

(An exception here is reverting CREATEs; the "CREATE"d address always gets marked warm, even if create fails)

ditto-eth · 2023-09-27T20:02:56Z

i'm not sure what else needs to be done for this pr if we move the following points out of this

moving function cool(StorageSlot[] memory slots) external; to be a nice to have (based on the current implementation, one would need to mark all previous state changes as warm first)
handling reverts/callframe (mark address as cool again, seems like it would be best to re-use the data in feat(cheatcodes): recordAccountAccesses and recordStorageAccesses cheatcodes #5794 and simply call cool_account on any addresses that need to be re-cooled when it happens)
not setting the address as cool for the from and to address?

mattsse · 2023-10-06T19:34:22Z

@mds1 I'm a bit out of the loop here

does this sound conceptionally right?

ditto-eth · 2023-10-06T20:51:11Z

@mds1 I'm a bit out of the loop here

does this sound conceptionally right?

explained a bit here, but i can break it down more: #5852 (review)

`vm.cool` cools both the `address` itself and the address' storage slots.

this approach is doing a lot work just to modify the gas amounts per opcode to change gasleft, doesn't modify anything else.
track address state via a new state.addresses hashmap
track storage slots via a double hashmap state.address_storage which is by contract address and then by slot number.
add another state variable to track how much gas to add to the next opcode
in the cheatcodes/mod.rs, modify the inspector so that at the end of a step, check which opcodes were run.
- If they match a sload/sstore (which marks storage as warm, then update the tracked state accordingly and also add the extra gas to the next opcode). there are some special amounts based on the storage values.
- if they match an address opcode (create/call/balance etc) then mark the address as warm + extra gas.

there are some edge cases I mentioned ^. The only thing i might want to do still is deciding (or documenting) how to handle warm address for to and from, and need to figure out why it's a bit different when testing locally, one of the 2500 accesses is incorrect

~~also looks like i need to rebase~~

ditto-eth · 2023-10-20T15:26:31Z

light ping, let me know if there's anything else I should explain or add!

DaniPopes · 2023-11-07T14:58:01Z

Hey @ditto-eth, we recently refactored how cheatcodes are implemented in #6131. You can read more about it in the dev docs. Please let me know if you have any problems rebasing!

ditto-eth · 2023-11-08T17:25:13Z

@DaniPopes thanks for the ping, wasn't too bad as I already did a previous rebase so a bit more familiar now - also decided to squash into 1 commit to make that easier.

crates/cheatcodes/src/evm.rs

crates/cheatcodes/src/inspector.rs

ditto-eth · 2023-12-07T19:00:07Z

@emo-eth informed me that #6310 landed which does similar things it seems - should I refactor this again to somehow use that data/code? if so, I think I'll need a lot of help to continue

mattsse · 2023-12-11T20:56:31Z

hey @ditto-eth

which does similar things it seems

Can you elaborate on whether there are synergies between the two?
I'm a bit out of the loop here but happy to help

sorry for the late reply

ditto-eth force-pushed the vm.cool-fix branch from dccebea to 3814faf Compare September 19, 2023 04:11

ditto-eth commented Sep 19, 2023

View reviewed changes

crates/evm/src/executor/inspector/cheatcodes/mod.rs Outdated Show resolved Hide resolved

Evalir requested review from Evalir, mattsse and mds1 September 20, 2023 14:50

ditto-eth force-pushed the vm.cool-fix branch 3 times, most recently from 09c6522 to 33e3500 Compare September 21, 2023 01:46

ditto-eth changed the title ~~(WIP) fix(vm.cool) Persist storage changes~~ fix(vm.cool) Persist storage changes Sep 21, 2023

ditto-eth commented Sep 21, 2023

View reviewed changes

crates/evm/src/executor/inspector/cheatcodes/mod.rs Outdated Show resolved Hide resolved

ditto-eth commented Sep 21, 2023

View reviewed changes

crates/evm/src/executor/inspector/cheatcodes/mod.rs Outdated Show resolved Hide resolved

Evalir reviewed Sep 21, 2023

View reviewed changes

ditto-eth force-pushed the vm.cool-fix branch 4 times, most recently from a6ffc9d to 68c3804 Compare September 24, 2023 03:09

ditto-eth mentioned this pull request Sep 25, 2023

feat: cool cheatcode #5830

Merged

ditto-eth force-pushed the vm.cool-fix branch from 68c3804 to 9643e4d Compare September 25, 2023 18:51

ditto-eth commented Sep 25, 2023

View reviewed changes

ditto-eth force-pushed the vm.cool-fix branch from 9643e4d to 18d8fd3 Compare September 27, 2023 16:25

ditto-eth force-pushed the vm.cool-fix branch from 18d8fd3 to 7a09292 Compare October 6, 2023 21:58

ditto-eth force-pushed the vm.cool-fix branch from 7a09292 to c1a9bb6 Compare November 8, 2023 17:21

DaniPopes requested changes Nov 8, 2023

View reviewed changes

crates/cheatcodes/src/evm.rs Outdated Show resolved Hide resolved

crates/cheatcodes/src/evm.rs Outdated Show resolved Hide resolved

crates/cheatcodes/src/inspector.rs Show resolved Hide resolved

crates/cheatcodes/src/inspector.rs Show resolved Hide resolved

fix(vm.cool) Persist storage changes

0a53cbe

ditto-eth force-pushed the vm.cool-fix branch from c1a9bb6 to 0a53cbe Compare November 9, 2023 16:50

refactor for a single mapping

5b9728e

ditto-eth force-pushed the vm.cool-fix branch from 823447f to ee35f1b Compare November 9, 2023 17:39

refactor to use a tuple

632656c

ditto-eth force-pushed the vm.cool-fix branch from ee35f1b to 632656c Compare November 9, 2023 17:48

emo-eth mentioned this pull request Dec 12, 2023

feat(cheatcodes): add vm.meterTxGas cheatcode #6590

Closed

DaniPopes mentioned this pull request Dec 20, 2023

feat: automatically generate Vm.sol from cheatcodes.json foundry-rs/forge-std#487

Merged

zerosnacks added this to the v1.0.0 milestone Jul 31, 2024

zerosnacks added the A-cheatcodes Area: cheatcodes label Jul 31, 2024

norswap mentioned this pull request Oct 8, 2024

vm.cool cheatcode #1340

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(vm.cool) Persist storage changes #5852

fix(vm.cool) Persist storage changes #5852

ditto-eth commented Sep 19, 2023 •

edited

Loading

Evalir commented Sep 21, 2023

Evalir left a comment

Evalir Sep 21, 2023

mds1 Sep 21, 2023

ditto-eth Sep 22, 2023 •

edited

Loading

ditto-eth commented Sep 22, 2023

ditto-eth commented Sep 25, 2023

Evalir commented Sep 25, 2023

ditto-eth commented Sep 25, 2023

ditto-eth Sep 25, 2023

emo-eth commented Sep 25, 2023

ditto-eth commented Sep 25, 2023

emo-eth commented Sep 25, 2023 •

edited

Loading

ditto-eth commented Sep 25, 2023

ditto-eth commented Sep 25, 2023

emo-eth commented Sep 26, 2023

ditto-eth commented Sep 27, 2023

mattsse commented Oct 6, 2023

ditto-eth commented Oct 6, 2023 •

edited

Loading

ditto-eth commented Oct 20, 2023

DaniPopes commented Nov 7, 2023

ditto-eth commented Nov 8, 2023

ditto-eth commented Dec 7, 2023

mattsse commented Dec 11, 2023

fix(vm.cool) Persist storage changes #5852

Are you sure you want to change the base?

fix(vm.cool) Persist storage changes #5852

Conversation

ditto-eth commented Sep 19, 2023 • edited Loading

Motivation

Help Test!

Solution

Evalir commented Sep 21, 2023

Evalir left a comment

Choose a reason for hiding this comment

Evalir Sep 21, 2023

Choose a reason for hiding this comment

mds1 Sep 21, 2023

Choose a reason for hiding this comment

ditto-eth Sep 22, 2023 • edited Loading

Choose a reason for hiding this comment

ditto-eth commented Sep 22, 2023

ditto-eth commented Sep 25, 2023

Evalir commented Sep 25, 2023

ditto-eth commented Sep 25, 2023

ditto-eth Sep 25, 2023

Choose a reason for hiding this comment

emo-eth commented Sep 25, 2023

ditto-eth commented Sep 25, 2023

emo-eth commented Sep 25, 2023 • edited Loading

ditto-eth commented Sep 25, 2023

ditto-eth commented Sep 25, 2023

emo-eth commented Sep 26, 2023

ditto-eth commented Sep 27, 2023

mattsse commented Oct 6, 2023

ditto-eth commented Oct 6, 2023 • edited Loading

vm.cool cools both the address itself and the address' storage slots.

ditto-eth commented Oct 20, 2023

DaniPopes commented Nov 7, 2023

ditto-eth commented Nov 8, 2023

ditto-eth commented Dec 7, 2023

mattsse commented Dec 11, 2023

ditto-eth commented Sep 19, 2023 •

edited

Loading

ditto-eth Sep 22, 2023 •

edited

Loading

emo-eth commented Sep 25, 2023 •

edited

Loading

ditto-eth commented Oct 6, 2023 •

edited

Loading

`vm.cool` cools both the `address` itself and the address' storage slots.