fuzz: Improve stability for txorphan and mini_miner harnesses #30306
Conversation
The addresses of the iterator values are non-deterministic (i.e. they depend on where the values were allocated). This causes stability issues when fuzzing (e.g. in the `txorphan` and `mini_miner` harnesses), due the orders (derived from IteratorComparator) not being deterministic. Improve stability by comparing the first element in the iterator value pair instead of using the the value addresses.
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. |
| @@ -92,7 +92,7 @@ class TxOrphanage { | |||
| template<typename I> | |||
| bool operator()(const I& a, const I& b) const | |||
| { | |||
| return &(*a) < &(*b); | |||
| return a->first < b->first; | |||
There was a problem hiding this comment.
I wonder if the set could just be made a vector, because duplicates shouldn't happen?
There was a problem hiding this comment.
Maybe but this sounds like more effort than I wanted to spend on this.
I don't think the comparison change here is gonna have any noticeable impact on performance tbh. In most cases the first 8 byte of the txids are already gonna differ, so the performance should be similar to comparing the pointers (assuming 8 byte addresses) most of the time.
There was a problem hiding this comment.
Tested ACK e009bf6. Using afl++, stability for txorphan went from 82% to ~94% and for mini_miner it went from 84% to 97%. I ran them both using the corpora in qa-assets.
Summary: Backport of [[bitcoin/bitcoin#30186 | core#30186]] and [[bitcoin/bitcoin#30306 | core#30306]]. Note that we don't have the fuzz test yet, the point is to get the module up-to-date with upstream. Depends on D16505. Test Plan: ninja all check-all Reviewers: #bitcoin_abc, PiRK Reviewed By: #bitcoin_abc, PiRK Differential Revision: https://reviews.bitcoinabc.org/D16507
See #29018.
Stability for
txorphanis now >90%.mini_minerneeds further investigation, stability still low (although slightly improved by this PR) at ~62%.