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

ci: route mollusk fixtures into fd conformance #29

Merged
merged 3 commits into from
Dec 5, 2024

Conversation

buffalojoec
Copy link
Contributor

@buffalojoec buffalojoec commented Nov 25, 2024

This change adds an additional step to the fd-conformance.mjs script (and CI job), which takes the Mollusk-generated Firedancer fuzzing fixtures and plugs them into the conformance harness.

Interestingly, this tests all of the program's unit tests against both the Mollusk and SolFuzz-Agave entrypoints. Although similar, they are two separate harnesses on the program-runtime, which gives us a bit more diversity in testing.

@buffalojoec
Copy link
Contributor Author

@lorisleiva @joncinque this is ready for review, just waiting on firedancer-io/solfuzz-agave#169

@joncinque
Copy link

The change looks good to me, but I'm not sure I understand the motivation. What are the benefits to using multiple harnesses? And how are they different?

@buffalojoec
Copy link
Contributor Author

The change looks good to me, but I'm not sure I understand the motivation. What are the benefits to using multiple harnesses? And how are they different?

Each harness handles configuration of the environment differently, albeit slightly. For example, Mollusk and SolFuzz-Agave populate sysvars differently, especially based on sysvar accounts provided in fixtures. Additionally, SolFuzz-Agave uses a different concept of a "result". It tracks "CUs remaining" instead of "CUs used" (not a huge deal), and also tracks only modified accounts versus just a list of all of the provided accounts including any changes.

Particularly, the conformance tool - when run using exec-fixtures - is much more strict than our unit tests, since we don't define a Check for every single thing, wheras the conformance harness will evaluate the entire result end-to-end, ensuring the expected result was returned.

@joncinque
Copy link

Gotcha, thanks for the info. It sounds like there's a lot of duplication between the two tools, so it seems redundant to run them both, but we can do it for now, or until we feel confident enough that one of them is complete / easy to run / whatever else we care about.

@buffalojoec
Copy link
Contributor Author

buffalojoec commented Nov 27, 2024

it seems redundant to run them both

Would you rather I feed them into the earlier steps and run them against both programs? That way we are still testing conformance, just with more fixtures, rather than only invoking the BPF one.

@buffalojoec
Copy link
Contributor Author

Like this ^

@joncinque
Copy link

Sorry, I totally misunderstood what this is meant to do 😓 let me know if I have this right:

  • currently, we run the tests through mollusk directly by invoking the tests at program/tests
  • currently, we also run the differential fuzzer (native vs BPF) through firedancer's test vectors (which I'm guessing are different than the unit tests?)
  • mollusk can output fixtures compatible with the fuzzer
  • this PR also wants to run mollusk's fixtures through the fuzzer

By running mollusk's fixtures through the fuzzer, what are we testing exactly? This is a legit question, not rhetorical.

If I understand correctly, that can allow the fuzzer to eventually mutate those fixtures and test more cases, which would give more coverage. This new step also checks to make sure that mollusk is outputting correct fixtures. Do we assume that the existing test vectors don't contain these cases already?

Originally, this PR was only going to test these fixtures against the BPF version of the program, and with the most recent commit, will instead run them through both BPF and native.

Does that mean we get slightly more coverage through the most recent commit? If that's the case, I would just opt for that, but I might be missing something.

@buffalojoec
Copy link
Contributor Author

buffalojoec commented Nov 27, 2024

Originally, this PR was only going to test these fixtures against the BPF version of the program, and with the most recent commit, will instead run them through both BPF and native.

Yeah, originally it was maybe a bit redundant because it can be considered akin to running the unit tests again with a different harness.

By running mollusk's fixtures through the fuzzer, what are we testing exactly? This is a legit question, not rhetorical.

With the latest commit, we basically add our unit tests to the conformance vectors to ensure they do the exact same thing on both programs. The key here is, if we change any unit tests, we get new fixtures, and we can plug those in through CI. Firedancer may have our unit tests covered from the builtin suite, but if we change them, they wouldn't have those new tests. Furthermore, if we changed any tests porting the program over they wouldn't have those changes either.

If I understand correctly, that can allow the fuzzer to eventually mutate those fixtures and test more cases, which would give more coverage.

Yes, we would get more fixture coverage in fuzzing as well as conformance, if we wanted to hook up solfuzz as well. Right now, it's just about my previous paragraph (builtin vs BPF on a fixed set of vectors).

Also, you have the steps right except for this point of clarification:

  • currently, we also run the differential fuzzer (native vs BPF) through firedancer's test vectors (which I'm guessing are different than the unit tests?)

We're not differential fuzzing with solana-conformance, rather we're running thousands of fixtures that represent test cases and expected results to ensure both programs do the exact same thing. There's a small subset of fixtures that came from unit tests (from the builtin), but most are previously failed fuzzing runs between Firedancer and Agave, both against the builtin.

Without solfuzz, there is no mutation going on here at the moment. This would be a future step.

@joncinque
Copy link

Ok gotcha, thanks for the explanation! Either way, this looks good to me whenever you want to land it

@buffalojoec buffalojoec marked this pull request as ready for review December 5, 2024 04:23
@buffalojoec buffalojoec merged commit 7ced055 into main Dec 5, 2024
10 checks passed
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.

2 participants