bench: Remove -priority-level= option

maflcko commented at 4:43 pm on January 6, 2026: member

The option was added in #26158, when the project was using an autotools-based build system. However, in the meantime this option is unused:

First, commit 27f11217ca63e0f8f78f14db139150052dcd9962 removed the option from one CI task
Then #32310 removed the option from CMakeList.txt, because:
- they only run as a sanity check (fastest version)
- no one otherwise runs them, not even CI
- issues have been missed due to this

Finally, after commit 0ad4376a49fae6f705128b326ba92317cb8e0639, I don’t see a single reason to keep this option, so remove it.

Also, there is a commit to turn a silent ignore of duplicate bench names into an error.

DrahtBot renamed this:
~~bench: Remove -priority-level= option~~
bench: Remove -priority-level= option
on Jan 6, 2026

DrahtBot added the label Tests on Jan 6, 2026

DrahtBot commented at 4:43 pm on January 6, 2026: contributor

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/34210.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	l0rinc, hebasto, achow101
Concept ACK	fanquake
Stale ACK	bensig

If your review is incorrectly listed, please copy-paste <!–meta-tag:bot-skip–> into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#34208 (bench: add fluent API for untimed setup steps in nanobench by l0rinc)
#33740 (RFC: bench: Add multi thread benchmarking by fjahr)
#32729 (test,refactor: extract script template helpers & widen sigop count coverage by l0rinc)
#31682 ([IBD] specialize CheckBlock’s input & coinbase checks by l0rinc)

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.

hebasto commented at 4:53 pm on January 6, 2026: member

Concept ACK.

maflcko force-pushed on Jan 6, 2026

DrahtBot added the label CI failed on Jan 6, 2026

DrahtBot commented at 5:07 pm on January 6, 2026: contributor

🚧 At least one of the CI tasks failed. Task tidy: https://github.com/bitcoin/bitcoin/actions/runs/20755188728/job/59595176823 LLM reason (✨ experimental): clang-tidy failed due to unused using declarations (e.g., Join/SplitString).

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

DrahtBot removed the label CI failed on Jan 6, 2026

bensig commented at 9:35 pm on January 7, 2026: contributor

ACK faa2eaba730831f9cf084f10590b6290f67842c3

Tested:

-priority-level option removed from help
Benchmarks list and run correctly
Sanity check passes

DrahtBot requested review from hebasto on Jan 7, 2026

in src/bench/bench.h:63 in faa2eaba73 outdated

49@@ -60,25 +50,24 @@ struct Args {
50     fs::path output_csv;
51     fs::path output_json;
52     std::string regex_filter;
53-    uint8_t priority;

hebasto commented at 7:35 pm on January 12, 2026:

faf3699d709744f44ff2c07b93acd6f8ffbb8331

This enables removing #include <cstdint>.

maflcko commented at 7:41 am on January 13, 2026:

thx, removed

in src/bench/bench.h:57 in faa2eaba73 outdated

57 class BenchRunner
58 {
59-    // maps from "name" -> (function, priority_level)
60-    typedef std::map<std::string, std::pair<BenchFunction, PriorityLevel>> BenchmarkMap;
61+    // maps from "name" -> function
62+    using BenchmarkMap = std::map<std::string, BenchFunction>;

hebasto commented at 7:36 pm on January 12, 2026:

faf3699d709744f44ff2c07b93acd6f8ffbb8331

This enables removing #include <utility>.

maflcko commented at 7:41 am on January 13, 2026:

thx, removed

hebasto approved

hebasto commented at 7:50 pm on January 12, 2026: member

ACK faa2eaba730831f9cf084f10590b6290f67842c3, I have reviewed the code and it looks OK.

I attempted to create duplicate benchmark names but was unable to, which is intended.

hebasto commented at 7:50 pm on January 12, 2026: member

cc @furszy

in src/bench/bench.h:43 in faa2eaba73

41@@ -42,16 +42,6 @@ using ankerl::nanobench::Bench;
42 
43 typedef std::function<void(Bench&)> BenchFunction;

l0rinc commented at 8:49 pm on January 12, 2026:

nit: we might as well update this, too:

0using BenchFunction = std::function<void(Bench&)>;

maflcko commented at 7:42 am on January 13, 2026:

thx, done

in src/bench/prevector.cpp:119 in faa2eaba73

115@@ -116,12 +116,12 @@ static void PrevectorFillVectorIndirect(benchmark::Bench& bench)
116     {                                                                \
117         Prevector##name<nontrivial_t>(bench);                        \
118     }                                                                \
119-    BENCHMARK(Prevector##name##Nontrivial, benchmark::PriorityLevel::HIGH);         \
120+    BENCHMARK(Prevector##name##Nontrivial);         \

l0rinc commented at 8:54 pm on January 12, 2026:

can we reformat these?

maflcko commented at 7:42 am on January 13, 2026:

thx, done

in src/bench/bench.cpp:1 in faa2eaba73 outdated

l0rinc commented at 8:56 pm on January 12, 2026:

nit in faa2eaba730831f9cf084f10590b6290f67842c3:

0- it makes dupliction bugs silent
1+ it makes duplication bugs silent

l0rinc commented at 9:04 pm on January 12, 2026:

funny that this didn’t even include a priority to begin with

maflcko commented at 7:42 am on January 13, 2026:

thx, done

in src/bench/wallet_create_tx.cpp:220 in faa2eaba73 outdated

214@@ -215,6 +215,6 @@ static void WalletCreateTxUsePresetInputsAndCoinSelection(benchmark::Bench& benc
215 
216 static void WalletAvailableCoins(benchmark::Bench& bench) { AvailableCoins(bench, {OutputType::BECH32M}); }
217 
218-BENCHMARK(WalletCreateTxUseOnlyPresetInputs, benchmark::PriorityLevel::LOW)
219-BENCHMARK(WalletCreateTxUsePresetInputsAndCoinSelection, benchmark::PriorityLevel::LOW)
220-BENCHMARK(WalletAvailableCoins, benchmark::PriorityLevel::LOW);
221+BENCHMARK(WalletCreateTxUseOnlyPresetInputs)
222+BENCHMARK(WalletCreateTxUsePresetInputsAndCoinSelection)
223+BENCHMARK(WalletAvailableCoins);

l0rinc commented at 8:59 pm on January 12, 2026:

We don’t need semicolons here, we could clean these up while we’re here:

0-BEGIN VERIFY SCRIPT-
1sed -i -E 's/^(BENCHMARK\([^)]+\));$/\1/' $(git grep -l '^BENCHMARK(')
2-END VERIFY SCRIPT-

maflcko commented at 7:42 am on January 13, 2026:

thx, done something

in src/bench/hashpadding.cpp:29 in faa2eaba73 outdated

25@@ -26,7 +26,7 @@ static void PrePadded(benchmark::Bench& bench)
26     });
27 }
28 
29-BENCHMARK(PrePadded, benchmark::PriorityLevel::HIGH);
30+BENCHMARK(PrePadded);

l0rinc commented at 8:59 pm on January 12, 2026:

for consistency we could move this to the end

maflcko commented at 7:42 am on January 13, 2026:

I don’t think the position matters, so leaving as-is.

l0rinc changes_requested

l0rinc commented at 9:06 pm on January 12, 2026: contributor

Concept ACK - please see a few remaining suggestions that I would prefer we still did if we’re already touching these lines.

bench: Remove -priority-level= option fa790c3eea

scripted-diff: Remove priority_level from BENCHMARK macro

-BEGIN VERIFY SCRIPT-

 sed --in-place --regexp-extended 's/BENCHMARK\(([^,]+), benchmark::PriorityLevel::(HIGH|LOW)\)/BENCHMARK(\1)/g' $( git grep -l PriorityLevel )
 sed --in-place                   's/#define BENCHMARK(n, priority_level)/#define BENCHMARK(n)/g'                ./src/bench/bench.h

-END VERIFY SCRIPT-

fa51a28a94

bench: Remove incorrect __LINE__ in BENCHMARK macro

Duplicate benchmarks with the same name are not supported. Expanding the
name with __LINE__ is confusing and brittle, because it makes duplication
bugs silent.

Fix this twofold:

* By enforcing unique benchmarks at compile-time and link-time. For
  example, a link failure may now look like:
  "mold: error: duplicate symbol: bench_runner_AddrManAdd"
* By enforcing unique benchmarks at run-time. This should never happen,
  due to the build-failure, but a failure may look like:
  "Assertion `benchmarks().try_emplace(std::move(name), std::move(func)).second' failed."

fa8938f08c

bench: Require semicolon after BENCHMARK(foo)

This makes the code more consistent.

Also, use "using BenchFunction = ..." while touching the header.

Also, fixup the whitespace after and earlier scripted-diff.

fa3df52712

maflcko force-pushed on Jan 13, 2026

maflcko commented at 7:44 am on January 13, 2026: member

Should be trivial to re-review the nits with git range-diff

l0rinc approved

l0rinc commented at 10:16 am on January 13, 2026: contributor

ACK fa3df5271232ee342c225da183be95dc47bde77f

In a follow-up we could remove all semicolons with the mentioned scripted diff - I have tested it locally and they’re redundant.

DrahtBot requested review from hebasto on Jan 13, 2026

maflcko commented at 10:43 am on January 13, 2026: member

In a follow-up we could remove all semicolons with the mentioned scripted diff - I have tested it locally and they’re redundant.

I don’t think they are redundant. There is only one semicolon after macro expansion. In fact, including the semicolon in the macro itself may lead to redundant semicolons. Generally, macros should not include a trailing semicolon, because it is confusing, and inconsistent. Also, tools (such as clang-format) may get confused.

Edit: To find other places where the semicolon could be removed from the macro:

0git ls-files | xargs perl -0777 -ne 'print "$ARGV: $&\n" while /#define\s*\S*\(([^\\\n]|\\\n)*?;\n/sg'

fanquake commented at 11:07 am on January 13, 2026: member

Concept ACK - these were introduced to be able to skip running benchmarks that were very slow. This ultimately just led to nobody running the benchmarks, even when changing the code, so they were silently broken (#32277).

hebasto approved

hebasto commented at 11:39 am on January 13, 2026: member

re-ACK fa3df5271232ee342c225da183be95dc47bde77f, only suggested changes since my recent review.

DrahtBot requested review from fanquake on Jan 13, 2026

achow101 commented at 10:41 pm on January 14, 2026: member

ACK fa3df5271232ee342c225da183be95dc47bde77f

achow101 merged this on Jan 14, 2026

achow101 closed this on Jan 14, 2026

maflcko deleted the branch on Jan 15, 2026

bench: Remove -priority-level= option #34210

Code Coverage & Benchmarks

Reviews

Conflicts