Skip to content

Conversation

murchandamus
Copy link
Contributor

I used the process described in the README of my fuzzing-helpers repository: https://github.com/murchandamus/fuzzing-helpers?tab=readme-ov-file#upstreaming-the-results-about-every-two-months

@maflcko
Copy link
Contributor

maflcko commented Feb 4, 2025

Looks like a mild improvement. Thx

@maflcko maflcko merged commit 317f1e6 into bitcoin-core:main Feb 4, 2025
4 checks passed
@maflcko
Copy link
Contributor

maflcko commented Feb 4, 2025

Sorry, I went ahead to temporarily revert this in commit 131d343, as it seems to be increasing the load on the CI sufficiently to cause timeouts.

This should be fixed on the CI side. A few options are:

  • Bump 2h timeout for the fuzz task
  • Bump the CI machine(s) for a faster CPU
  • Make the fuzz targets faster
  • Something else?

@murchandamus
Copy link
Contributor Author

murchandamus commented Feb 4, 2025

There do seem to be a few targets that always take a lot of time during the merge. Are these the ones that are also causing an issue here? E.g., package_rbf, addrman_serdeser, wallet_notifications come to mind.

I could also remove the threads that are fuzzing with shorter limits, if we think that this is unduly inflating the count of inputs that we carry around, although I assume that it just finds shorter inputs faster that we would eventually find with more fuzzing. Perhaps we should prune more often?

@maflcko
Copy link
Contributor

maflcko commented Feb 4, 2025

Yeah, ideally all fuzz targets are fast, but I think the low hanging fruits to improve there are taken.

Moreover, it looks like you are using all sanitizers to merge the folders, but none when searching new fuzz inputs? I wonder if this will bloat the merged set of files, given that the fuzz engine does not specifically try to reduce fuzz inputs based on sanitizer coverage while searching for new inputs?

I don't know the answer, but my recommendation would be to either also search with sanitizers, or not use them to merge.

@murchandamus
Copy link
Contributor Author

I do have a couple threads that run with sanitizers on the nightly fuzzing while most run without sanitizers, but maybe that’s not enough

@maflcko
Copy link
Contributor

maflcko commented Feb 4, 2025

Ok, then it should be fine. The option to merge without sanitizers remains open, so that the qa-assets repo is a slimmer version focussing on CI. Obviously this could mean that sanitizer coverage is smaller in the CI, but it may be fine, given that people curate the sanitizer coverage outside of qa-assets either way?

@murchandamus
Copy link
Contributor Author

I was merging without sanitizers for previous months, but then my submissions caused CI errors. I’m happy to adjust my procedure to always use sanitizers, never use sanitzers, or something else, but I must admit, it’s not entirely clear to me what would work best.

Do you think I should try to fuzz with a mix of sanitized threads and unsanitized threads, but merge without sanitizers afterwards? Wouldn’t that cause CI errors on submission again?

@maflcko
Copy link
Contributor

maflcko commented Feb 4, 2025

I was merging without sanitizers for previous months, but then my submissions caused CI errors.

I presume you are referring to (mostly) integer sanitizer issues that were lacking a suppression or minor code fixup? In that case, I think it is preferable to discover triggerable errors than not to. If they were ignored, someone else will just run into them later on (if they haven't already).

I’m happy to adjust my procedure to always use sanitizers, never use sanitzers, or something else, but I must admit, it’s not entirely clear to me what would work best.
Do you think I should try to fuzz with a mix of sanitized threads and unsanitized threads, but merge without sanitizers afterwards? Wouldn’t that cause CI errors on submission again?

Yes, I think that makes sense, because:

  • Using sanitizers (in combination with non-sanitizer builds) during fuzz input generation is recommended practise and done by OSS-Fuzz and I presume everyone else fuzzing Bitcoin Core.
  • Assuming you refer to fuzz crashes as CI errors, this is desired, because the whole point of fuzzing is to find fuzz crashes.
  • It may lead to a slimmer qa-assets, causing less bloat. This is desirable, because at some level of bloat (even if the CI machines are upgraded to be faster), GitHub may delete the qa-assets repo due to it being outside the "reasonable" size.

So if you don't mind, can you re-do this merge without sanitizers, so that the size and runtime can be compared?

@murchandamus
Copy link
Contributor Author

Sure, I am re-doing the merge with the same inputs, i.e., the active-fuzzing corpus I set aside yesterday to make this upstream PR, using a branch that does not have sanitizers enabled. I did enable the suppressions, though.

@murchandamus
Copy link
Contributor Author

I opened another pull request for the inputs merged without using sanitizers in #218.

murchandamus added a commit to murchandamus/fuzzing-helpers that referenced this pull request Feb 6, 2025
Also:
- Use a couple sanitized threads in fuzz_given.sh
- Rename qa-merge to qa-fuzz-sanitized

Merging without sanitizers was discussed here:
bitcoin-core/qa-assets#217
@murchandamus
Copy link
Contributor Author

I also updated my documentation to recommend merging without sanitizers: https://github.com/murchandamus/fuzzing-helpers

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