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

feat(cheatcodes): additional cheatcodes to aid in symbolic testing #8807

Merged
merged 7 commits into from
Sep 11, 2024

Conversation

grandizzy
Copy link
Collaborator

@grandizzy grandizzy commented Sep 4, 2024

Motivation

To be followed up with a PR for adding remaining expect*Call and fresh* cheatcodes.

Solution

  • setArbitraryStorage(address target):

    • added in Utilities group
    • when called, the target address is added in arbitrary_storage vec
    • in cheatcode inspector.step_end, generate random value if target address is marked as arbitrary storage, current opcode is SLOAD and storage slot untouched (cold ensure_arbitrary_storage fn added)
    • apply same logic to vm.load cheatcode
    • tests and kontrol ported tests added
  • copyStorage(address from, address to):

    • added in Utilities group
    • when called, from and to accounts are loaded from db and storage of to account replaced with storage of from account
    • tests and kontrol ported tests added
  • mockFunction(address callee, address target, bytes calldata data):

    • added in Evm group (same with other mock* cheatcodes)
    • when called, mapping of data -> target address is added for callee (new hashmap mocked_functions added)
    • in stack inspector.call, mock target is looked up based on call's calldata (strict match first, if no match then matching by selector is attempted) and call's bytecode address is replaced with mock target, e.g. if using
          vm.mockFunction(
              address(my_contract),
              address(model_contract),
              abi.encodeWithSelector(MockFunctionContract.mocked_args_function.selector, 456)
          );
      then my_contract.mocked_args_function(456) will be executed with model_contract address bytecode .
      Catch-all function mocks can be set too, for example if using:
          vm.mockFunction(
              address(my_contract),
              address(model_contract),
              abi.encodeWithSelector(MockFunctionContract.mocked_args_function.selector)
          );
      then my_contract.mocked_args_function(ANY_VALUE) will be executed with model_contract address bytecode
    • tests and kontrol ported tests added

}
}

contract CounterFreshStorageTest is DSTest {
Copy link

Choose a reason for hiding this comment

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

These tests look good overall. I would also add several more tests:

  • One that makes the storage fresh, then which reads/writes multiple storage values, and checks that we don't accidentally get aliasing between them somehow.
  • One that makes the storage fresh, then reads two different values, and puts a precondition (assume) that they are distinct value (not equal to each other), and then does something with those values.
  • One which reads a storage variable, but then bounds the result to something very small. This is to check if we can efficiently generate random numbers within a bound.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

sounds good, will add such

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

added a test here that I think it covers first 2 bullets

function test_fresh_storage_multiple_read_writes() public {
Counter counter = new Counter();
vm.freshStorage(address(counter));
uint256 slot1 = vm.randomUint(0, 100);
uint256 slot2 = vm.randomUint(0, 100);
require(slot1 != slot2, "random positions should be different");
address alice = counter.owners(slot1);
address bob = counter.owners(slot2);
require(alice != bob, "random storage values should be different");
counter.setOwner(slot1, bob);
counter.setOwner(slot2, alice);
assertEq(alice, counter.owners(slot2));
assertEq(bob, counter.owners(slot1));
}

not clear though on how a test for 3rd one should look like?

Copy link

Choose a reason for hiding this comment

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

Hmmm, the third test maybe doesn't make sense, I agree. That one can probably be left for now, and if we encounter unexpected behavior with the tool we can report it and get a test added then.

The main concern is that a user may want to do this:

vm.freshStorage(address(counter));
vm.bound(counter.a(), 10, 20);

This basically would be saying: set the storage to all symbolic, but the value of a should be between 10 and 20. Currently, I assume this would cause a lot of reverts because the counter.a() read will happen before the call to bound.

I'm not sure there is anything you can do about this in Foundry, it should work fine in Kontrol. But instead, for Foundry, you would have to do:

vm.freshStorage(address(counter));
uint256 a = vm.freshUInt(256, 10, 20); // produces the fresh value _and_ bounds it
counter.setA(a);

Then we know that the storage is all symbolic, but the correct bounds exist on a.

Copy link

Choose a reason for hiding this comment

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

The above is psuedocode, btw, just trying to brainstorm how to solve this issue of having a fully symbolic storage, but also allowing some bounds on specific values.

Comment on lines 13 to 34
contract DeterministicRandomnessTest is Test {

function testDeterministicRandomUint() public {
console.log(vm.randomUint());
console.log(vm.randomUint());
console.log(vm.randomUint());
}

function testDeterministicRandomUintRange() public {
uint256 min = 0;
uint256 max = 1000000000;
console.log(vm.randomUint(min, max));
console.log(vm.randomUint(min, max));
console.log(vm.randomUint(min, max));
}

function testDeterministicRandomAddress() public {
console.log(vm.randomAddress());
console.log(vm.randomAddress());
console.log(vm.randomAddress());
}
}
Copy link

Choose a reason for hiding this comment

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

I guess I would prefer the name freshUInt, freshAddress, for example, rather than random. Maybe they can be included in the forge-std, but then just simplify down to teh random variants? The reason is because we want to use the same cheatcodes as Foundry directly, but for us they're not random, but symbolic. fresh captures both random and symbolic as sub-categories.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

the randomAddress and uint are already added, going to work on the fresh* cheatcodes and maybe look into aliasing existing

Comment on lines 137 to 139
require(counter.a() != 0);
require(counter.b() != address(0));
require(counter.c() != 0);
Copy link

Choose a reason for hiding this comment

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

I actually don't see why these should hold.... As far as I can see, a, b, and c should all be fresh, meaning they could be zero as well as any other number.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes, that's correct, they could be 0 though is unlikely. I will remove these assertions as they could fail the test

Copy link
Collaborator Author

@grandizzy grandizzy Sep 4, 2024

Choose a reason for hiding this comment

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

the randomness though follows the fuzzing rules where you can specify a seed and have deterministic values. So maybe could leave the assertions but set a certain seed that will always generate non zero values for this sequence... will think if this is of any value for given test

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@ehildenb I removed the assertions and added a test using seed / producing deterministic values here:

contract Counter {
uint256[] public a;
address[] public b;
int8[] public c;
bytes32[] public d;
}
contract CounterFreshStorageTest is DSTest {
Vm vm = Vm(HEVM_ADDRESS);
function test_fresh_storage_with_seed() public {
Counter counter = new Counter();
vm.freshStorage(address(counter));
assertEq(counter.a(11), 85286582241781868037363115933978803127245343755841464083427462398552335014708);
assertEq(counter.b(22), 0x939180Daa938F9e18Ff0E76c112D25107D358B02);
assertEq(counter.c(33), -104);
assertEq(counter.d(44), 0x6c178fa9c434f142df61a5355cc2b8d07be691b98dabf5b1a924f2bce97a19c7);
}
}

Also ported Kontrol tests using same seed:

contract SymbolicStore {
uint256 public testNumber = 1337; // slot 0
constructor() {}
}
contract SymbolicStorageTest is DSTest {
Vm vm = Vm(HEVM_ADDRESS);
function test_SymbolicStorage() public {
uint256 slot = vm.randomUint(0, 100);
address addr = 0xEA674fdDe714fd979de3EdF0F56AA9716B898ec8;
vm.freshStorage(addr);
bytes32 value = vm.load(addr, bytes32(slot));
assertEq(uint256(value), 85286582241781868037363115933978803127245343755841464083427462398552335014708);
// Load slot again and make sure we get same value.
bytes32 value1 = vm.load(addr, bytes32(slot));
assertEq(uint256(value), uint256(value1));
}
function test_SymbolicStorage1() public {
uint256 slot = vm.randomUint(0, 100);
SymbolicStore myStore = new SymbolicStore();
vm.freshStorage(address(myStore));
bytes32 value = vm.load(address(myStore), bytes32(uint256(slot)));
assertEq(uint256(value), 85286582241781868037363115933978803127245343755841464083427462398552335014708);
}
function testEmptyInitialStorage(uint256 slot) public {
bytes32 storage_value = vm.load(address(vm), bytes32(slot));
assertEq(uint256(storage_value), 0);
}
}

Copy link

Choose a reason for hiding this comment

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

Nice, that's cool.

@grandizzy grandizzy force-pushed the issue-4072 branch 2 times, most recently from 0699b9d to a5bc7ee Compare September 5, 2024 13:35
@grandizzy grandizzy marked this pull request as ready for review September 6, 2024 05:31
@grandizzy grandizzy changed the title [WIP] feat(cheatcodes): additional cheatcodes to aid in symbolic testing feat(cheatcodes): additional cheatcodes to aid in symbolic testing Sep 6, 2024
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.

love all the additional tests, only have some questions about terminology

@@ -2303,6 +2312,14 @@ interface Vm {
/// Unpauses collection of call traces.
#[cheatcode(group = Utilities)]
function resumeTracing() external view;

/// Random storage for given target address.
Copy link
Member

Choose a reason for hiding this comment

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

what does this mean exactly, fresh to me indicates that the account's storage is reset/cleared

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yeah, agree it's confusing, it's basically random values, in #4072 there were 2 more suggestions for the name: symbolicStorage and setArbitraryStorage I personally like the setArbitraryStorage one or randomStorage, CC @ehildenb for thoughts

Copy link
Member

Choose a reason for hiding this comment

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

I like setArbitraryStorage

Copy link

Choose a reason for hiding this comment

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

Arbitrary sounds good to me too!

Copy link

Choose a reason for hiding this comment

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

Fresh is just shorter/easier to say.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

kk, will stick with setArbitraryStorage then

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

changed, pls recheck

crates/cheatcodes/src/evm.rs Outdated Show resolved Hide resolved
@@ -368,6 +371,9 @@ pub struct Cheatcodes {

/// Ignored traces.
pub ignored_traces: IgnoredTraces,

/// Addresses that should have fresh storage generated.
Copy link
Member

Choose a reason for hiding this comment

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

can we describe what fresh means, not obvious

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

added, pls recheck (did a force push to keep PR clean)

use foundry_config::{Config, FuzzConfig};
use foundry_test_utils::{forgetest_init, str};

forgetest_init!(test_assume_no_revert, |prj, cmd| {
Copy link
Member

@DaniPopes DaniPopes Sep 6, 2024

Choose a reason for hiding this comment

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

I would like these tests to be all in forge

we're also seeing that there are 2 ways to write tests, one in testdata/... .sol and other in forgetest!, which i'm not too fond of

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yep, will move them all under forge/tests/it then

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I reverted the change, indeed they should stay in cheats testdata, got
Will follow up with a PR to cleanup force/cli/test_cmd.rs as it is now a catch all thingy

[SOLC_VERSION] [ELAPSED]
Compiler run successful!

...
Copy link
Member

Choose a reason for hiding this comment

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

why was this changed? again the point of all these substitution is so that if the test output changes you can just run the test suite once with SNAPSHOTS=overwrite and it will be updated normally. now you have to do that and also re-add manually ... since those are manual filters

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yep, reverted, please check (got wrong the SNAPSHOTS=overwrite usage but now clear, thank you)

let val = ccx.ecx.sload(target, slot.into())?;
let mut val = ccx.ecx.sload(target, slot.into())?;
// Generate random value if target should have arbitrary storage and storage slot untouched.
if ccx.state.arbitrary_storage.contains(&target) && val.is_cold && val.data.is_zero() {
Copy link
Member

Choose a reason for hiding this comment

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

what's considered "untouched" here? eg in a single test calling the same contract two times from test scope would result in slot being cold on first call, and warm on second due to how our EVM works. is this expected?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yep, that's the expected behavior, subsequent loads of same slot should return same value

// Load slot again and make sure we get same value.

unless explicitly rewritten

// This should remain 0 if explicitly set.

let key = try_or_return!(interpreter.stack().peek(0));
let target_address = interpreter.contract().target_address;
if let Ok(value) = ecx.sload(target_address, key) {
if value.is_cold && value.data.is_zero() {
Copy link
Member

Choose a reason for hiding this comment

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

hmm, I think this would never be cold because revm marks slot as warm during execution of opcode?

Copy link
Member

Choose a reason for hiding this comment

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

though it looks like tests are passing? would need to look closer

Copy link
Member

Choose a reason for hiding this comment

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

I think it might be because we're reading the value instead of a key in interpreter.stack().peek(0). on step_end stack already contains read value

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes, that's my take too, the opcode is not yet executed / slot marked as warm at this point

Copy link
Member

@klkvr klkvr Sep 9, 2024

Choose a reason for hiding this comment

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

ah interpreter.current_opcode() returns next opcode in step_end, so it's fine

Copy link
Member

Choose a reason for hiding this comment

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

yes, that's my take too, the opcode is not yet executed / slot marked as warm at this point

I was thinking that we're entering ensure_arbitrary_storage after revm executed the opcode, so it's my bad

though can we document what's happening here anyway? especially that this would happen before sload even though we are in _end hook

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

sure, will add comment to make this clear

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@klkvr please check 16f068f Had to accommodate copy from arbitrary storage scenario pointed here #8807 (comment) too, so a little bit bigger change, ptal. thanks!

impl Cheatcode for copyStorageCall {
fn apply_stateful<DB: DatabaseExt>(&self, ccx: &mut CheatsCtxt<DB>) -> Result {
let Self { from, to } = self;
if let Ok(from_account) = ccx.load_account(*from) {
Copy link
Member

Choose a reason for hiding this comment

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

this won't work for forking but Ig it's out of scope?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Indeed, wasn't discussed to work for forking but could be added later if needed

@ehildenb
Copy link

ehildenb commented Sep 9, 2024

Another test I thought of: basically how do abitraryStorage and copyStorage interact?

I would expect (and what we would have for symbolic execution) that if I copy a given contracts storage, I get the same storage as that contract. So if I set a storage to arbitrary, then copy it, all future reads from both storages will agree (even if reading from uninitialized slots). So a test could be:

  • Create contract A and set its storage to abitrary.
  • Use copyStorage to copy storage of A to B.
  • Read from slots m and n from both contract A and B, and assert that they are equal.
  • Write to slot m of A, and assert that now it is different than what is in slot m on B.

@grandizzy
Copy link
Collaborator Author

grandizzy commented Sep 10, 2024

Another test I thought of: basically how do abitraryStorage and copyStorage interact?

yeah, this scenario wasn't covered, added support and suggested test here

function test_copy_storage_same_values_on_load() public {
// Make the storage of first contract symbolic
vm.setArbitraryStorage(address(csc_1));
vm.copyStorage(address(csc_1), address(csc_2));
uint256 slot1 = vm.randomUint(0, 100);
uint256 slot2 = vm.randomUint(0, 100);
bytes32 value1 = vm.load(address(csc_1), bytes32(slot1));
bytes32 value2 = vm.load(address(csc_1), bytes32(slot2));
bytes32 value3 = vm.load(address(csc_2), bytes32(slot1));
bytes32 value4 = vm.load(address(csc_2), bytes32(slot2));
// Check storage values are the same for both source and target contracts.
assertEq(value1, value3);
assertEq(value2, value4);
}
function test_copy_storage_consistent_values() public {
// Make the storage of first contract symbolic.
vm.setArbitraryStorage(address(csc_1));
// Copy arbitrary storage to 2 contracts.
vm.copyStorage(address(csc_1), address(csc_2));
vm.copyStorage(address(csc_1), address(csc_3));
uint256 slot1 = vm.randomUint(0, 100);
uint256 slot2 = vm.randomUint(0, 100);
// Load slot 1 from 1st copied contract and slot2 from symbolic contract.
bytes32 value3 = vm.load(address(csc_2), bytes32(slot1));
bytes32 value2 = vm.load(address(csc_1), bytes32(slot2));
bytes32 value1 = vm.load(address(csc_1), bytes32(slot1));
bytes32 value4 = vm.load(address(csc_2), bytes32(slot2));
// Make sure same values for both copied and symbolic contract.
assertEq(value3, value1);
assertEq(value2, value4);
uint256 x_1 = vm.randomUint();
// Change slot1 of 1st copied contract.
_storeUInt256(address(csc_2), slot1, x_1);
value3 = vm.load(address(csc_2), bytes32(slot1));
bytes32 value5 = vm.load(address(csc_3), bytes32(slot1));
// Make sure value for 1st contract copied is different than symbolic contract value.
assert(value3 != value1);
// Make sure same values for 2nd contract copied and symbolic contract.
assertEq(value5, value1);
uint256 x_2 = vm.randomUint();
// Change slot2 of symbolic contract.
_storeUInt256(address(csc_1), slot2, x_2);
value2 = vm.load(address(csc_1), bytes32(slot2));
bytes32 value6 = vm.load(address(csc_3), bytes32(slot2));
// Make sure value for symbolic contract value is different than 1st contract copied.
assert(value2 != value4);
// Make sure value for symbolic contract value is different than 2nd contract copied.
assert(value2 != value6);
assertEq(value4, value6);
}

crates/cheatcodes/src/utils.rs Show resolved Hide resolved
@@ -26,6 +27,7 @@ async fn test_cheats_local(test_data: &ForgeTestData) {
}

let mut config = test_data.config.clone();
config.fuzz.seed = Some(U256::from(100));
Copy link
Member

Choose a reason for hiding this comment

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

should this be set in test_data?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

good point, not sure we should be adding a new TEST_DATA_WITH_SEED (we don't have one for isolate either) but default tests shouldn't run with seed - since there's no option to set it inline I've added a test that runs contracts matching WithSeed (excluded from defaults), hope this makes sense
pls check 7e69723

call.bytecode_address = *target;
} else {
// Check if we have a catch-all mock set for selector.
if let Some(target) = mocks.get(&call.input.slice(..4)) {
Copy link
Member

Choose a reason for hiding this comment

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

can collapse into one get().or_else(|| ), also this panics if call.input.len() < 4, use call.input.get(..4)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

changed in 7e69723

interpreter: &mut Interpreter,
ecx: &mut EvmContext<DB>,
) {
if interpreter.current_opcode() == op::SLOAD {
Copy link
Member

Choose a reason for hiding this comment

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

should move the SLOAD check outside into step_end, and make it call one arbitrary_storage_end to avoid callsite bloat

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

changed in 7e69723

let mut val = ccx.ecx.sload(target, slot.into())?;

if val.is_cold && val.data.is_zero() {
let rand_value = ccx.state.rng().gen();
Copy link
Member

Choose a reason for hiding this comment

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

should not gen unless either is true

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yep, changed in 7e69723

grandizzy and others added 2 commits September 11, 2024 10:24
- separate cheatcodes tests with specific seed
- better way to match mocked function
- arbitrary_storage_end instead multiple calls
- generate arbitrary value only when needed
Co-authored-by: DaniPopes <57450786+DaniPopes@users.noreply.github.com>
async fn test_cheats_local(test_data: &ForgeTestData) {
let mut filter = Filter::new(".*", ".*", &format!(".*cheats{RE_PATH_SEPARATOR}*"))
.exclude_paths("Fork")
.exclude_contracts("Isolated");
.exclude_contracts("(Isolated|WithSeed)");
Copy link
Member

Choose a reason for hiding this comment

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

please add it to if cfg!(feature = "isolate-by-default") branch below too, tests with --features=isolate-by-default are failing because we're overriding it there

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

sorry, missed running the test with feature set locally, added here 02086f9 (had to add mock function test too)

Copy link
Member

Choose a reason for hiding this comment

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

had to add mock function test too

hmm, wondering why would it break with --isolate?

Copy link
Member

Choose a reason for hiding this comment

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

ah, I see, it's because we're ignoring bytecode_address when handling isolated calls

TxKind::Call(call.target_address),

Copy link
Member

@klkvr klkvr Sep 11, 2024

Choose a reason for hiding this comment

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

not sure how to handle this now, probably not a big deal

could be solved by instead of altering bytecode_address changing target address bytecode for duration of the call, though it would introduce new edge cases

let's keep it ignored for now

Copy link
Member

@klkvr klkvr left a comment

Choose a reason for hiding this comment

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

LGTM

@grandizzy grandizzy merged commit d663f38 into foundry-rs:master Sep 11, 2024
20 checks passed
@grandizzy grandizzy deleted the issue-4072 branch September 11, 2024 13:38
Comment on lines +1149 to +1151
if (self.arbitrary_storage.is_arbitrary(&interpreter.contract().target_address) ||
self.arbitrary_storage.is_copy(&interpreter.contract().target_address)) &&
interpreter.current_opcode() == op::SLOAD
Copy link
Member

@DaniPopes DaniPopes Sep 11, 2024

Choose a reason for hiding this comment

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

this should just have the op::SLOAD check and the others inside of the arbitrary_storage_end function; please be very mindful of what goes in step and step_end because these functions are called millions of times

Copy link
Collaborator Author

@grandizzy grandizzy Sep 11, 2024

Choose a reason for hiding this comment

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

oh, ok, will do a PR to change this, mind to explain the difference? (was thinking that is better to check if is arbitrary or copy of arbitrary storage first and only if one of them is true check if a SLOAD (which is more likely), as arbitrary and copy are not so commons)

Copy link
Member

@DaniPopes DaniPopes Sep 11, 2024

Choose a reason for hiding this comment

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

They are called on execution of every single opcode, meaning tens to hundreds of thousands per single execution of a test

we want them optimized for the common path of "do nothing" as most of these checks will be false most of the time; this means they should be always inlined and the checks should always be small in size so that these functions fit in as few cache lines as possible

because they are so hot this matters a lot

Copy link
Member

@DaniPopes DaniPopes Sep 11, 2024

Choose a reason for hiding this comment

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

wrt the checks order, the thinking is not correct because checking opcode is 1 or 2 instructions, while the others are hashmap lookups which are in the order of hundreds or thousands

in either case the most common and cheapest check is the SLOAD one, the rest should be outlined because most opcodes are not SLOAD

ideally hooks like these could be installed separately as custom opcodes, however we dont really have a framework for doing this currently

Copy link
Collaborator Author

@grandizzy grandizzy Sep 11, 2024

Choose a reason for hiding this comment

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

thank you, makes total sense, will follow up with a PR to get it inline

Re hashmap lookups - my logic was that it won't affect too much execution when cheatcodes not used (hence empty hashmap and no need to check the opcode)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

wonder if better to have arbitrary storage as an option, made a draft PR here #8848

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

Successfully merging this pull request may close these issues.

5 participants