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(forge): fuzz dictionary #731

Closed
wants to merge 17 commits into from
Closed

feat(forge): fuzz dictionary #731

wants to merge 17 commits into from

Conversation

brockelmore
Copy link
Member

Implements a fuzzing dictionary per #387

Takes a weighted union of 2 strategies, 40-60 weighted dictionary-random. Grabs state via a hack around the deconstruct method to get all state changes not applied (in our case, thats all of them), and flattens them into a hashset. We then pass this state hashset into the strategy creator which uses a selector to select a random value from the set as the input and transforms it into an input

Some limitations: bytes and strings types are only ever 32 bytes when pulling from state. probably improvements to be had there

@brockelmore brockelmore changed the title feat(forge): fuzz dictionary [WIP] feat(forge): fuzz dictionary Feb 12, 2022
@onbjerg onbjerg added the T-feature Type: feature label Feb 12, 2022
@gakonst gakonst marked this pull request as draft February 15, 2022 13:11
@gakonst
Copy link
Member

gakonst commented Feb 28, 2022

@onbjerg something to consider for the REVM fuzzer. For Revm it should be easier, as we can lift the changelog per call and add it to the dictionary?

@brockelmore brockelmore marked this pull request as ready for review March 5, 2022 18:11
@brockelmore brockelmore changed the title [WIP] feat(forge): fuzz dictionary feat(forge): fuzz dictionary Mar 5, 2022
@brockelmore brockelmore requested a review from gakonst March 5, 2022 18:15
Copy link
Member

@gakonst gakonst left a comment

Choose a reason for hiding this comment

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

Great work. Minor nits.

evm-adapters/src/fuzz.rs Outdated Show resolved Hide resolved
Comment on lines +189 to +190
state_weight: u32,
random_weight: u32,
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 under EvmOpts? Should we make a builder that makes this struct easier to construct?


// Snapshot the state before the test starts running
let pre_test_state = self.evm.borrow().state().clone();

let flattened_state = Rc::new(RefCell::new(self.evm.borrow().flatten_state()));

// we dont shrink for state strategy
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// we dont shrink for state strategy
// we dont shrink for state strategy because shrinking in a collection moves the iterator
// whereas here it doesn't make sense to shrink, given everything is a concrete value

@@ -122,6 +160,19 @@ impl<'a, S, E: Evm<S>> FuzzedExecutor<'a, E, S> {
}
);

{
let new_flattened = evm.flatten_state();
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
let new_flattened = evm.flatten_state();
// we extend the state dict with any returndata from the call in 32-byte chunks.
let new_flattened = evm.flatten_state();

) -> impl Strategy<Value = Bytes> {
// We need to compose all the strategies generated for each parameter in all
// possible combinations
// let strategy = proptest::sample::select(state.clone().into_iter().collect::<Vec<[u8;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// let strategy = proptest::sample::select(state.clone().into_iter().collect::<Vec<[u8;

Comment on lines 185 to 187
} else {
// because of metadata bs, we may hit this codepath. just ignore it
// and continue on
Copy link
Member

Choose a reason for hiding this comment

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

shouldn't be required?

Suggested change
} else {
// because of metadata bs, we may hit this codepath. just ignore it
// and continue on

if let Some(code) = code {
let mut i = 0;
while i < code.len() {
let wrapped_op = OpCode::from(Opcode(code[i]));
Copy link
Member

Choose a reason for hiding this comment

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

The OpCode struct should probably have a direct from(x: u8) method

});
}
}
for log in logs {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
for log in logs {
// Flatten all log addresses, topics & data into H256 and write them to the dictionary
for log in logs {

}
}

// we keep bytes as little endian
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// we keep bytes as little endian
// Flatten the old and new account values & storage in H256
// and insert them as little endian bytes in the dictionary

@onbjerg onbjerg mentioned this pull request Mar 12, 2022
8 tasks
This was referenced Mar 18, 2022
@onbjerg
Copy link
Member

onbjerg commented Mar 21, 2022

Rolled into #985

@onbjerg onbjerg closed this Mar 21, 2022
@gakonst gakonst deleted the brock/fuzz_dict branch March 21, 2022 08:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-feature Type: feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants