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

Bring solfuzz-agave to the mono repo #955

Closed
wants to merge 8 commits into from

Conversation

LucasSte
Copy link

@LucasSte LucasSte commented Apr 22, 2024

Problem

The fuzzer we are developing together with Firedancer needs this glue code to work with Agave. The repository was previously outside the monorepo, hindering proper maintenance.

Summary of Changes

I brought the repository and made some adjustments to satisfy clippy and the dependencies path.

This is another workspace under Agave, as it is not a published crate and its compilation commands require specific settings.

@codecov-commenter
Copy link

codecov-commenter commented Apr 22, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.8%. Comparing base (57ab944) to head (34bf2cb).
Report is 6 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff            @@
##           master     #955     +/-   ##
=========================================
- Coverage    81.9%    81.8%   -0.1%     
=========================================
  Files         855      855             
  Lines      232154   232154             
=========================================
- Hits       190170   190129     -41     
- Misses      41984    42025     +41     

@LucasSte LucasSte force-pushed the solfuzz branch 2 times, most recently from 021c1c7 to 5ba1465 Compare April 23, 2024 13:46
@LucasSte LucasSte marked this pull request as ready for review April 23, 2024 16:40
@pgarg66
Copy link

pgarg66 commented Apr 23, 2024

@LucasSte a few quick questions:

  1. Were any changes made to the new files?
  2. Can we find a better name for the top level folder? Just in case we have other fuzzers in the future.

Choose a reason for hiding this comment

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

We might want to rewrite this self-test in Rust in the future.
I just wrote it in C originally because it was the easiest way to get the test working.

Copy link
Author

Choose a reason for hiding this comment

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

Yes, we can do it in another PR. The .cargo/config.toml file may need adjustments as cargo pass down the flags to all compiler invocations, whereas it only uses the flag for the project when we use the --target x86_64-unknown-linux-gnu argument. I haven't taken the time to investigate further how to set different flags for tests.

Choose a reason for hiding this comment

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

In the future, we might want to move these Protobuf definitions to some shared place.
Maybe storage-proto?

Firedancer has functionality to create these test vectors from real ledgers (it allows us to debug complex program executions). Agave might want to add the same feature.

Copy link
Author

Choose a reason for hiding this comment

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

In the future, we might want to move these Protobuf definitions to some shared place. Maybe storage-proto?

I was going to suggest this as well. Replicating files in many repositories is not a good practice. Shall we have a separate repo and add it as a submodule here?

Choose a reason for hiding this comment

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

@LucasSte I suggest we use solana-foundation/specs. Let's also register the schema on https://buf.build/

@LucasSte
Copy link
Author

@LucasSte a few quick questions:

  1. Were any changes made to the new files?

I tried to bring the repository as it is now. I made small changes to make our clippy tests happy. There are separate commits to address the warnings I got. There was another PR merged in solfuzz-agave (firedancer-io/solfuzz-agave#3) with more changes, but I thought it was best to add them after that I merge this PR. The repository solfuzz-agave will be archived once we merge this PR.

  1. Can we find a better name for the top level folder? Just in case we have other fuzzers in the future.

Yes. Do you have any suggestion? Solfuzz?

@LucasSte
Copy link
Author

@LucasSte a few quick questions:

  1. Were any changes made to the new files?

Forgot to mention:
I removed the original Makefile in favor of .cargo/config.toml and build.rs to align the style to our repository practices.

@t-nelson
Copy link

The repository was previously outside the monorepo, hindering proper maintenance.

can this be elaborated? we're trying to get stuff out of the monorepo, not put more in

@LucasSte
Copy link
Author

The repository was previously outside the monorepo, hindering proper maintenance.

can this be elaborated? we're trying to get stuff out of the monorepo, not put more in

solfuzz is a fuzzer engine developed by the Firedancer team that can be used to fuzz the Agave validator, or run differential fuzzing between versions of Agave or between Firedancer and Agave. There has been an effort in concentrating the fuzzers we are developing in solfuzz. Sean and I are expanding it to cover the VM and the transaction processing APIs.

solfuzz is dynamically linked to a glue code we need to run the fuzzer either in Firedancer or in Agave. Firedancer has this glue in their own repository already, and we thought we could bring it closer to Agave, as it is going to be mostly used by Anza's engineers. The repository today is located in the Firedancer organization: https://github.com/firedancer-io/solfuzz-agave. There was an idea to bring it to the monorepo, but I think creating another repository under the Github Anza organization would also work.

@ripatel-fd Please, feel free to add more context if I forgot anything.

Now, @t-nelson we can create another repo under Anza. What do you think?

@yihau
Copy link
Member

yihau commented Apr 25, 2024

do you have any name suggestions if we decide to move it to another repository? btw, it looks like we can just fork from Firedancer and then make some custom modifications 🤔

@yihau
Copy link
Member

yihau commented Apr 29, 2024

looks like we haven't had a conclusion 🤔 I just forked the repo here: https://github.com/anza-xyz/solfuzz-agave in case we need it!

@LucasSte
Copy link
Author

@t-nelson Few other points came to my mind during the weekend to justify moving the fuzzer to the monorepo. If any of the target APIs receive updates, the mono repo CI would point that the fuzzer needs the same updates. If the fuzzer is a separate repository, these updates would need to be manual.

In addition, selecting Agave's version to fuzz with the project on the monorepo is as simple as doing git checkout, while in an external repository we need to manually add the commit/version in the Cargo.toml file and make the necessary adaptations.

@LucasSte
Copy link
Author

Closing the PR in favor of maintaining the fuzzer outside the monrepo, as a separate repository: https://github.com/anza-xyz/solfuzz-agave

@LucasSte LucasSte closed this Apr 30, 2024
@LucasSte LucasSte deleted the solfuzz branch May 6, 2024 20:13
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.

6 participants