-
Notifications
You must be signed in to change notification settings - Fork 18
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
add support for return data #63
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change looks good, but I think we need to extend the support to the fuzzing features.
harness/src/fuzz/mollusk.rs
Outdated
@@ -94,6 +94,7 @@ impl From<&FuzzEffects> for InstructionResult { | |||
execution_time, | |||
program_result, | |||
raw_result, | |||
return_data: None, // TODO: Omitted for now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add this field to the non-Firedancer protobuf and fixture lib? It should be pretty simple, let's just do Vec<u8>
like FD does.
Now that the Mollusk result type tracks return data, the Mollusk-based fixture effects should also.
harness/src/fuzz/firedancer.rs
Outdated
@@ -235,6 +235,7 @@ fn parse_fixture_effects( | |||
.compute_budget | |||
.compute_unit_limit | |||
.saturating_sub(effects.compute_units_available), | |||
return_data: None, // TODO: Omitted for now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you seen fixtures in the Firedancer test-vectors that test the return data for Stake? If so, we might want to add this to the Firedancer support here.
I can get a test going that actually runs against real test vectors to make sure this thing actually works, and you could rebase onto it if you want.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's the PR: #65
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yea a lot of the fixtures contain expected return data for GetMinimumDelegation
, i just havent looked through the fuzz and fd-fuzz code yet to see what support is needed since my usecase only depends on harness. will do that tho
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You shouldn't even have to mess with the fuzz-related crates, the change(s) should be local to the main harness's fuzz
dir.
But lmk if you want me to land my stuff first, so we have the coverage.
ok it took me a couple tries but i think i got it now haha |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks perfect. Thank you!!
@@ -191,7 +193,7 @@ fn build_fixture_effects(context: &FuzzContext, result: &InstructionResult) -> F | |||
compute_units_available: context | |||
.compute_units_available | |||
.saturating_sub(result.compute_units_consumed), | |||
return_data: Vec::new(), // TODO: Mollusk doesn't capture return data. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Niceeee
using this for minimum delegation tests with normal mollusk, i didnt modify any of the fuzz types since im not sure what support is needed for those