fix(fuzz) - consistent snapshot results between runs

[grandizzy]

Motivation

Closes #4917
Closes #5199 - tested with #5199 (comment)
Closes #5689 - tested with #5689 (comment)
Closes #6179
Closes #7942 - tested with https://github.com/cruzdanilo/foundry-snapshot-repro

Solution

see #7942 (comment) when fuzzing values from state there are cases when input could slightly differ between subsequent runs. Until we fuzz based on values type the proposed workaround is to avoid using state values when running forge snapshot.

The fix was tested with projects above but I couldn't minimize it to a simpler one suitable for unit test (in the sample I debugged the difference in snapshot appears when using an unchecked and assembly block).

CC @klkvr

[klkvr]

I think we should go after the initial reason behind non-deterministic results without disabling features for snapshot runs.

fuzz tests do not collect values from state since #7552. So I believe we are dealing here with non-deterministic initial dictionary construction. This is something I've already seen in #7106 (comment).

Basically, we want to ensure that this fn is deterministic:

foundry/crates/evm/fuzz/src/strategies/state.rs

Lines 108 to 129 in 1ddea96

	fn insert_db_values(&mut self, db_state: Vec<(&Address, &DbAccount)>) {
	for (address, account) in db_state {
	// Insert basic account information
	self.insert_value(address.into_word().into(), false);
	// Insert push bytes
	self.insert_push_bytes_values(address, &account.info, false);
	// Insert storage values.
	if self.config.include_storage {
	for (slot, value) in &account.storage {
	self.insert_storage_value(slot, value, false);
	}
	}
	}
	// need at least some state data if db is empty otherwise we can't select random data for
	// state fuzzing
	if self.values().is_empty() {
	// prefill with a random addresses
	self.insert_value(Address::random().into_word().into(), false);
	}
	}

At the moment I can see at least 2 issues here:

1. It does not sort storage keys before inserting
2. It uses Address::random() which is not deterministic.

I don't think 2 really matters as setup state always contains at least a single account of a test contract.

I've addressed 1 locally by sorting storage keys before inserting and it seems to fix the issue at least for the #7942 case.

[grandizzy]

@klkvr makes perfect sense, thanks! If you can push your local fix I can use it to test some others projects that were having similar issues

[klkvr]

@grandizzy pushed my local approach, Ig you'd have to drop your previous commit for it to actually take effect for snapshot

[grandizzy]

@grandizzy pushed my local approach, Ig you'd have to drop your previous commit for it to actually take effect for snapshot

@klkvr I tested #5689 (comment) and it looks good too

[cruzdanilo]

thanks! @grandizzy one thing i noticed is that the snapshots for the exact same tests that used to be flaky now change with unrelated code changes, even just natspec. here is an example in the same reproduction repository i created for the issue:

cruzdanilo/foundry-snapshot-repro@adbd43b

it's not so bad, but it does create a lot of noise in the snapshot diff and create snapshot diffs in commits that shouldn't have any, confusing collaborators.

[grandizzy]

@cruzdanilo most probably this happens because we also populate fuzz dict from push bytes (motivated by #1168 )

[cruzdanilo]

@grandizzy how can a natspec-only change impact the push bytes?

[grandizzy]

@cruzdanilo I meant probably the order of values we collect here is not ordered either and affected by such changes as well, will investigate more why this happens

[grandizzy]

@grandizzy how can a natspec-only change impact the push bytes?

@cruzdanilo so actually it is different, for example for contract

contract MyTest {
    function dummy() external {}
}

output of forge inspect MyTest bytecode is different than for a slightly changed version like

contract MyTest {
    /// Dummy function.
    function dummy() external {}
}

[klkvr]

such change only affects metadata, we should be able to just ignore it when collecting push bytes

though small unrelated test contract changes would still probably affect snapshot data

[grandizzy]

yeah, would be nice to have such, going to look into updating function to ignore metadata then

foundry/crates/evm/fuzz/src/strategies/state.rs

Lines 322 to 355 in 44d98ea

	fn collect_push_bytes(code: &[u8]) -> Vec<[u8; 32]> {
	let mut bytes: Vec<[u8; 32]> = Vec::new();
	// We use [SpecId::LATEST] since we do not really care what spec it is - we are not interested
	// in gas costs.
	let opcode_infos = spec_opcode_gas(SpecId::LATEST);
	let mut i = 0;
	while i < code.len().min(PUSH_BYTE_ANALYSIS_LIMIT) {
	let op = code[i];
	if opcode_infos[op as usize].is_push() {
	let push_size = (op - opcode::PUSH1 + 1) as usize;
	let push_start = i + 1;
	let push_end = push_start + push_size;
	// As a precaution, if a fuzz test deploys malformed bytecode (such as using `CREATE2`)
	// this will terminate the loop early.
	if push_start > code.len() || push_end > code.len() {
	return bytes;
	}
	let push_value = U256::try_from_be_slice(&code[push_start..push_end]).unwrap();
	bytes.push(push_value.to_be_bytes());
	// also add the value below and above the push value to the dictionary.
	if push_value != U256::ZERO {
	bytes.push((push_value - U256::from(1)).to_be_bytes());
	}
	if push_value != U256::MAX {
	bytes.push((push_value + U256::from(1)).to_be_bytes());
	}
	i += push_size;
	}
	i += 1;
	}
	bytes
	}

[grandizzy]

@cruzdanilo btw, if you find acceptable as an workaround until a fix is implemented - you could use forge snapshot --no-metadata for such