Skip to content

Conversation

brunoerg
Copy link
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.

Co-authored-by: Ayush Singh <ayushsingh.as1700@gmail.com>
@DrahtBot
Copy link
Contributor

DrahtBot commented Jul 16, 2024

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.

@maflcko
Copy link
Member

maflcko commented Jul 16, 2024

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.

@brunoerg
Copy link
Contributor Author

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.

Copy link
Contributor

@kevkevinpal kevkevinpal left a comment

Choose a reason for hiding this comment

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

Concept ACK 385ec3f

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

@maflcko
Copy link
Member

maflcko commented Jul 18, 2024

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?

@brunoerg
Copy link
Contributor Author

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.

@brunoerg brunoerg closed this Jul 18, 2024
@bitcoin bitcoin locked and limited conversation to collaborators Jul 25, 2025
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.

5 participants