fuzz: add target for CoinsResult #30461

1. 
   brunoerg commented at 6:40 pm on July 16, 2024: contributor

   This PR adds fuzz coverage for CoinsResult.

   This was addressed with another new harness proposed in #28236. However, besides the PR appearing to be abandoned (3 months since last author iteration), reviewers agreed that having a specific target for CoinsResult would be better.

2. fuzz: add target for `CoinsResult`

   Co-authored-by: Ayush Singh <ayushsingh.as1700@gmail.com>

   385ec3f12b
3. 
   DrahtBot commented at 6:40 pm on July 16, 2024: contributor

   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
   Concept ACK	kevkevinpal

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

4. DrahtBot added the label Tests on Jul 16, 2024
5. hebasto added the label Needs CMake port on Jul 16, 2024
6. 
   maflcko commented at 7:30 pm on July 16, 2024: member

   Spend.cpp coverage is not too bad already: https://maflcko.github.io/b-c-cov/fuzz.coverage/src/wallet/spend.cpp.gcov.html

   It may be good to clarify the new coverage a bit.

7. 
   brunoerg commented at 8:32 pm on July 16, 2024: contributor

   I think fuzzing it directly is good and there are some CoinsResult functions that are not fuzzed (All, Erase, Clear), but tbh just noticed they’re used only in unit test, so the only benefit would be the performance of fuzzing it directly. I’m ok on closing it if case.
8. in src/wallet/test/fuzz/coinsresult.cpp:64 in 385ec3f12b

   59+                (void)coins_result.All();
   60+            },
   61+            [&] {
   62+                coins_result.Clear();
   63+                const auto coins{coins_result.All()};
   64+                const auto size{coins_result.Size()};
   

   ---

   ---

   

   kevkevinpal commented at 1:39 am on July 17, 2024:

   does it make sense to add another CallOneOf with just coin_result.Clear() because aren’t we forcing the pattern of Clear() -> All() -> Size() and never Clear() -> Size()

   not sure if that is needed, just a thought

   ---

   

   brunoerg commented at 4:25 pm on July 17, 2024:

   I don’t think so, there is nothing special with them.
9. 
   kevkevinpal commented at 1:40 am on July 17, 2024: contributor

   Concept ACK 385ec3f

   I see value in adding fuzz coverage to CoinsResult All, Erase, and Clear

10. 
    maflcko commented at 10:01 am on July 18, 2024: member

    Not sure what the point here is. All functions are trivial for-loops, and already fuzzed.

    The only function that isn’t CoinsResult::Erase is unused outside of tests, so I don’t see why CPU, storage, code, review, maintenance, etc should be spent on this.

    It may be better to remove the function or move it to the tests?

11. 
    brunoerg commented at 10:14 am on July 18, 2024: contributor

    It may be better to remove the function or move it to the tests?

    Probably move it to the tests. Anyway, I’ll close this for now.

12. brunoerg closed this on Jul 18, 2024

    ---

13. maflcko removed the label Needs CMake port on Jul 25, 2024

Contributors
brunoerg DrahtBot maflcko kevkevinpal

Labels
Tests