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

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31153.

Reviews

See 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.

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #30987 (Don't zero-after-free DataStream: Faster IBD on some configurations by davidgumberg)
• #28792 (Embed default ASMap as binary dump header file by fjahr)

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.

Concept ACK - agree with stickies-v re updating the doc.

Concept ACK

Perhaps also worth updating doc/benchmarking.md with a brief section on which benchmarks are typically useful to add, and which benchmarks should rather be ephemeral (e.g. provided on a separate branch, supplementary to a PR)?

I think this documentation update would be helpful to get in even without this PR.

People can be asked to add benchmarks for performance improvements or to verify that a change to existing code doesn't degrade performance

Imo, performance improvements (and accompanying benchmarks) should only be made to code that makes critical things slower than they need to be. Otherwise we're just improving performance for the sake of doing it, which is not something we have the resources for. We should aim our resources with intention, and the benchmarks can act as the scope.

For example, should we spend our time making hex parsing 500x faster or maybe shave 10% from IBD? I'm arguing for scoping our tests to the latter (and other critical things e.g. mining) so that we don't get distracted.

Are any of the benchmarks called by outside sites that track their results over time?

https://corecheck.dev sort of does but it's quite noisy.

Will add the doc changes next week

Hex parsing is used in mining via DecodeHexBlk, no?

Also, while some of the benchmarks do not exist to be optimized, but merely to give a feeling and to avoid a regression where an algorithm is swapped to go from O(n) to O(n^2). So in a sense, they are a bit of a unit test.

In any case, updating the docs should be uncontroversial and useful going forward.

but merely to give a feeling and to avoid a regression where an algorithm is swapped to go from O(n) to O(n^2). So in a sense, they are a bit of a unit test.

This is a good reason to keep.

I'm arguing for scoping our tests to the latter (and other critical things e.g. mining) so that we don't get distracted.

And this is an important statement.

We don't necessarily need to throw away work that has previously been a useful metric in issues (#17501) to achieve the goal of new benchmark contributions being focused on specific areas.

throw away work that has previously been a useful metric in issues (#17501)

The benchmarks weren't mentioned in the issue you linked or the PR that fixed the issue?

throw away work that has previously been a useful metric in issues (#17501)

The benchmarks weren't mentioned in the issue you linked or the PR that fixed the issue?

I said it's a useful metric, as in given that context, would it not be nice to have the benchmark?

as in given that context

I see. No I don't think they would have been useful in that context, because all the base58 benchmarks run with inputs of the same size, so they couldn't have found that particular issue and therefore were also not useful in verifying the fix.

practicalswift mentioned a fuzz test in the PR, which is likely how he found the issue (speculating).

as in given that context

I see. No I don't think they would have been useful in that context, because all the base58 benchmarks run with inputs of the same size, so they couldn't have found that particular issue and therefore were also not useful in verifying the fix.

practicalswift mentioned a fuzz test in the PR, which is likely how he found the issue (speculating).

I'm not intending to argue, and as i said your main point in the PR is valid.

If a bench didn't exist at the time of the issue, I imagine a review comment would have mentioned it would be nice to have, as a metric. I don't think that's controversial.

Added the doc commit, ready for further review

Concept ACK, Approach NACK

I think there should be a burden on PR's that claim to improve performance to demonstrate a user-impacting scenario where the improvements are relevant, and I agree that having benchmarks that test workloads that will never impact users leads to PR's being opened and review being spent on things that don't deserve attention.

I think a similar burden exists for this PR to demonstrate or at least describe a rationale for why these removed benchmarks don't test behavior that would impact users.

Not yet sure on this PR we already have priority_level for benchmarks

All the benchmarks being removed seem to be benchmark::PriorityLevel::HIGH
not sure if it makes sense to move them down a level?

This frequently comes up with new contributors and they can't be blamed for thinking that e.g. hex parsing should be made faster if a benchmark test exists.

I understand this is one of the motivations for this change does this happen often for benchmarks that are marked benchmark::PriorityLevel::LOW

I'm not sure if using the priority levels is a good alternative to deleting the benches. Afaict, the priority level was introduced to avoid running slow benchmarks in the CI as apposed to indicating how "important" they are. As a result, we're not catching issues with these tests until someone executes them (e.g. #29061 (comment)) and I'd prefer not to expand on that.

Not sure why it was closed. It seemed like a useful doc update to me, but maybe I am missing something. Feel free to ACK/NACK #31690, which is just the doc update from here cherry-picked.