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

Revert "ci: Run fuzzer task for the master branch only" #24292

Merged
merged 2 commits into from Feb 21, 2022

Conversation

maflcko
Copy link
Member

@maflcko maflcko commented Feb 8, 2022

This reverts commit 5a9e255.

I think we should attempt to maintain the fuzz tasks for release branches as well.

If it is too difficult for one branch, it could make sense to disable it for that branch, but not for all branches unconditionally.

Also, bump to jammy.

MarcoFalke added 2 commits February 8, 2022 20:18
This gives them a newer clang version, which may have more sanitizers
available.
@DrahtBot DrahtBot added the Tests label Feb 8, 2022
@laanwj
Copy link
Member

laanwj commented Feb 10, 2022

I remember part of the original reasoning to disable it for branches was that the corpus is generated for master, and release branches might differ enough for it to run into problems. Is this resolved?

@maflcko
Copy link
Member Author

maflcko commented Feb 10, 2022

Fuzz inputs are literally untrusted, arbitrary data. The goal of fuzzing is to harden the code base. I think this is especially important for release branches, since releases are built from them. If the fuzz tests on a release branch fail, I think it should be investigated, not suppressed upfront so that we don't even learn about the issue.

@maflcko
Copy link
Member Author

maflcko commented Feb 11, 2022

And about the fuzz inputs only being generated for one branch: In fact they are only generated for one commit. Any further commit or code change may render them (or parts of them) "invalid". I don't think we can maintain all fuzz inputs for every commit, at least I won't do that and I don't think it is necessary.

My point is that we should investigate any issue that comes up, not completely disable the fuzz test (not even trying to compile them).

I think, on a case-by-case basis, where it makes sense, we could checkout a specific commit of the qa-assets repo. Or alternatively discard it completely, so that at least compilation is tested. However, finding the appropriate solution can be done when there is a problem.

@maflcko maflcko added this to the 23.0 milestone Feb 14, 2022
@maflcko
Copy link
Member Author

maflcko commented Feb 14, 2022

Assigned 23.0

@laanwj
Copy link
Member

laanwj commented Feb 14, 2022

It's ok with me as long as it doesn't cause too many false positives, that everyone is going to ignore anyway at some point.

@maflcko
Copy link
Member Author

maflcko commented Feb 14, 2022

If there is any fuzz issue, feel free to create an issue and ping me if needed.

Copy link
Member

@fanquake fanquake left a comment

Choose a reason for hiding this comment

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

ACK fa27745 - we'll see how we go with the 23.x release branch.

@fanquake fanquake merged commit 2ab4fbe into bitcoin:master Feb 21, 2022
@maflcko maflcko deleted the 2202-ciFuzz branch February 21, 2022 13:25
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Feb 22, 2022
…h only"

fa27745 ci: Bump fuzz tasks to jammy (MarcoFalke)
fab8cd5 Revert "ci: Run fuzzer task for the master branch only" (MarcoFalke)

Pull request description:

  This reverts commit 5a9e255.

  I think we should attempt to maintain the fuzz tasks for release branches as well.

  If it is too difficult for one branch, it could make sense to disable it for that branch, but not for all branches unconditionally.

  Also, bump to jammy.

ACKs for top commit:
  fanquake:
    ACK fa27745 - we'll see how we go with the 23.x release branch.

Tree-SHA512: d6d08e7dce0884b556c51ff1896aebbbb5a805c22decd58af81a04192d19876978696017b489ec55886ddfd5c022963baaab5f11022369ae5291016826ff8017
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Feb 22, 2022
@bitcoin bitcoin locked and limited conversation to collaborators Feb 21, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants