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

test: proof-of-concept of using the standard go fuzzing during the executing of the simapp simulation #18946

Closed
wants to merge 1 commit into from

Conversation

ggrieco-tob
Copy link
Contributor

Description

This is a proof-of-concept for improving the testing when running the high-level simulation (simapp) using smart fuzzing. This is NOT in a merging state, since it requires a good amount of discussion and changes.

The problem is simple: the simapp will only perform random exploration based on single seed for exploring the chain state. This PR enhance the simapp using go fuzzing with a minimal amount of changes.

The idea is simple: we modified how the Source of a random seed is used to produce random values. This modified Source will allow the fuzzing engine to add or mutate "deterministic bytes" that are going to be used as random values directly. Initially, the random generation will be the same as the current simapp, but over the time, the fuzzer will have more and more control to randomly explore the input state.

To test the random exploration, just use go fuzzing as the simapp. For instance:

$ cd simapp
$ go test -mod=readonly -fuzz=FuzzFullAppSimulation -GenesisTime=1688995849 -Enabled=true -NumBlocks=2 -BlockSize=5 -Commit=true -Seed=0 -Period=1 -Verbose=1 -run=_

Using this new approach, we found a number of small bugs that resulted in a few PRs: #16951, #16978, #18542.

If you are interested in this approach, we need to discuss the next steps. In particular, we need to know how the UI can be modified to support both the usage of a single random seed (as it is now) and this new approach.


Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

  • included the correct type prefix in the PR title
  • confirmed ! in the type prefix if API or client breaking change
  • targeted the correct branch (see PR Targeting)
  • provided a link to the relevant issue or specification
  • reviewed "Files changed" and left comments if necessary
  • included the necessary unit and integration tests
  • added a changelog entry to CHANGELOG.md
  • updated the relevant documentation or specification, including comments for documenting Go code
  • confirmed all CI checks have passed

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

  • confirmed the correct type prefix in the PR title
  • confirmed all author checklist items have been addressed
  • reviewed state machine logic, API design and naming, documentation is accurate, tests and test coverage

@tac0turtle
Copy link
Member

tac0turtle commented Jan 5, 2024

Thanks for the pr!! We spent some time migrating our simulation system to gofuzz but became blocked on a deterministic environment to test. We have a team member working on this though. We would love to see where you take this and if there is anything we can help with

@ggrieco-tob
Copy link
Contributor Author

I'm wondering why gofuzz and not the native go fuzzing was used? (which is usually easier to integrate into the testing procedure)

@tac0turtle
Copy link
Member

go fuzzing landed much later than the simulator was written. We never migrated across

@ggrieco-tob
Copy link
Contributor Author

Which were the blockers on a deterministic environment to test you had? Is the code available in some branch so I can take a quick look to see if it affects this prototype?

@tac0turtle
Copy link
Member

Julien is working on integrating comet mock which will provide a deterministic environment to us #18842. This is the other pr that is currently blocked on getting the other pr over the finish line #17938

@ggrieco-tob
Copy link
Contributor Author

I see what you mean regarding non-determinism. I think it makes sense to rebase this PR after #18842 and #17938 are merged. I can follow these and re-run a quick fuzzing campaign after that to see how well it worked.

@ggrieco-tob
Copy link
Contributor Author

We just published a blog post explaining this idea: https://blog.trailofbits.com/2024/02/05/improving-the-state-of-cosmos-fuzzing/

Copy link
Contributor

github-actions bot commented Mar 7, 2024

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@github-actions github-actions bot added the Stale label Mar 7, 2024
@github-actions github-actions bot closed this Mar 11, 2024
alpe added a commit that referenced this pull request May 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants