fuzz: Avoid timeout and bloat in fuzz targets

[maflcko]

If the fuzz input contains invalid data in a loop, abort early. This will teach the fuzz engine to look for useful data and avoids bloating the fuzz input folder with useless (repeated) data.

[DrahtBot]

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	dergoegge, brunoerg

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #28438 (Use serialization parameters for CTransaction by ajtowns)
• #28280 (Don't empty dbcache on prune flushes: >30% faster IBD by andrewtoth)
• #28216 (fuzz: a new target for the coins database by darosior)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

[maflcko]

Can be tested by running the corresponding test case from #28812 (comment) and looking at the runtime

[brunoerg]

Concept ACK

Question: What is the difference compared to #27552 ("partial" fuzzers)?

[maflcko]

Question: What is the difference compared to #27552 ("partial" fuzzers)?

As I said on that pull request, it needs benchmarks and review on a case-by-case basis to check whether it is useful for a specific fuzz target. If someone wants to do those benchmarks on the fuzz targets touched in this pull request, and finds a positive outcome, nothing is holding anyone back from applying the concept.

The goal of this pull request is to prevent the fuzz engine from exploring invalid serializations in a loop, as they do not help the fuzz target in any way, increase runtime, and increase storage costs.

I've edited the pull request description to clarify that this only covers loop serialization.

[maflcko]

Did the same for coins_view, because it followed the same pattern, even though a timeout is currently not reported by Oss-Fuzz.

[dergoegge]

There seems to be a pattern here, maybe it makes sense to have something like a RepeatedCallOneOf that does the exit on bad data internally?

[maflcko]

Not sure, there are also some loops that do not pick out of several callables. Happy to push any refactoring, if someone uploads a diff, patch, or commit.

[dergoegge]

utACK fabb504

Didn't test but the rational makes a lot of sense to me. This should be better even if it doesn't solve the timeouts.

[brunoerg]

crACK fabb504

Logic makes sense for me. I didn't check all the targets, but it seems we could apply it in coincontrol as well.

[maflcko]

Logic makes sense for me. I didn't check all the targets, but it seems we could apply it in coincontrol as well.

Yes, I was thinking about this, too. In practise it doesn't matter because COutPoint happens to be (de)serialized as a fixed-size byte blob, which can only fail if the fuzz provider is out of data. In that case the loop will already abort. However, it could make sense to change this for consistency as well, or to protect against the case where COutPoint is "dynamically" deserialized and could fail even if the provider has more data to present.

Can be done in a follow-up, or in this pull, if I have to re-touch again.