bench: add fluent API for untimed `setup` steps in nanobench #34208

l0rinc commented at 11:45 AM on January 6, 2026: contributor

Context

As described in https://github.com/martinus/nanobench/issues/130, we have a few benchmarks where we have to reset the state between runs; otherwise, the repetitions will do something different than the first iteration.

Upstream

I have opened a PR to nanobench to introduce an untimed setup phase, see: https://github.com/martinus/nanobench/pull/136

Tests

Tests were only added upstream. It would be a bit awkward to wire them into nanobench.h outside the benchmarking setup: https://github.com/martinus/nanobench/pull/136/changes/58350cfe5912d7e0533ccfcd759eea5d62eb55bb#diff-88160f647ce57661afe7d755fa70a5fa342a2b79d72d3511596878e69ed5cdc3

Fix

I have moved the changes here as well and applied them to a few simple benchmarks as a demonstration. We can revert the ones that are controversial and add others in follow-ups. This PR is mostly meant to add the setup feature.

Benchmarks

Most benchmarks show a modest "speedup"; others a "slowdown" - but it's only the effect of the setup that's not measured anymore - and a run phase that does the same operation in each epoch iteration (wallet benchmark changes were reverted for simplicity): <img width="1496" height="882" alt="image" src="https://github.com/user-attachments/assets/34c14565-f3df-41e5-9a86-95b2ca21703a" />

DrahtBot added the label Tests on Jan 6, 2026

DrahtBot commented at 11:45 AM 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/34208.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	janb84, sedited, achow101
Concept ACK	sipa, furszy, davidgumberg
Stale ACK	bensig

If your review is incorrectly listed, please copy-paste <code></code> into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#33160 (bench: Add more realistic Coin Selection Bench by murchandamus)
#32729 (test,refactor: extract script template helpers & widen sigop count coverage by l0rinc)
#32554 (bench: replace embedded raw block with configurable block generator by l0rinc)
#31682 ([IBD] specialize CheckBlock's input & coinbase checks by l0rinc)
#29700 (kernel, refactor: return error status on all fatal errors by ryanofsky)

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.

LLM Linter (✨ experimental)

Possible typos and grammar issues:

positioned to the start of the file -> positioned at the start of the file [preposition "to" is incorrect here; "positioned at" correctly describes location]

2026-03-08 12:03:01

DrahtBot added the label CI failed on Jan 6, 2026

DrahtBot commented at 1:02 PM on January 6, 2026: contributor

🚧 At least one of the CI tasks failed. Task tidy: https://github.com/bitcoin/bitcoin/actions/runs/20747321605/job/59567659454 LLM reason (✨ experimental): clang-tidy failed the build due to a use-after-move error in wallet_loading.cpp, causing the CI to fail.

<details><summary>Hints</summary>

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.

</details>

l0rinc force-pushed on Jan 6, 2026

janb84 commented at 2:21 PM on January 6, 2026: contributor

ACK e9e3ea1341caeab87aef8cb25dc0b58f7c3b27c8

PR patches Vendored Code from Nanobench to include untimed setup phase.

Personally I like the fluent style way we can now setup a benchmark, after the patch. The patch looks solid and is backwards compatible. <del> A downside (i see) of patching vendored code is that it now contains "untested" code. (Yes it's only benchmark code, so not on a critical path, I get it) </del> I has tests upstream 👍

Have thought about how to "fix" this, maybe include a "test" benchmark that shows the code works (with and without the setup) but that does not proves a lot imho. Anyhow not blocking.

l0rinc commented at 2:26 PM on January 6, 2026: contributor

vendored code is that it now contains "untested" code

Thanks for the review, please see the tests in the upstream PR: https://github.com/martinus/nanobench/pull/136/changes/58350cfe5912d7e0533ccfcd759eea5d62eb55bb#diff-88160f647ce57661afe7d755fa70a5fa342a2b79d72d3511596878e69ed5cdc3

DrahtBot removed the label CI failed on Jan 6, 2026

bensig commented at 9:54 PM on January 8, 2026: contributor

ACK e9e3ea1341caeab87aef8cb25dc0b58f7c3b27c8

Tested -

DrahtBot added the label Needs rebase on Jan 14, 2026

l0rinc force-pushed on Jan 15, 2026

l0rinc commented at 12:50 PM on January 15, 2026: contributor

Rebased after the benchmark priority cleanup. Ready for review again.

DrahtBot removed the label Needs rebase on Jan 15, 2026

DrahtBot added the label Needs rebase on Jan 21, 2026

l0rinc force-pushed on Jan 21, 2026

DrahtBot removed the label Needs rebase on Jan 21, 2026

DrahtBot added the label CI failed on Jan 22, 2026

DrahtBot commented at 12:25 AM on January 22, 2026: contributor

🚧 At least one of the CI tasks failed. Task tidy: https://github.com/bitcoin/bitcoin/actions/runs/21228258011/job/61081107788 LLM reason (✨ experimental): CI failed due to a clang-tidy error (unused return value in coin_selection.cpp).

<details><summary>Hints</summary>

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.

</details>

l0rinc force-pushed on Jan 22, 2026

DrahtBot removed the label CI failed on Jan 22, 2026

janb84 commented at 12:40 PM on January 23, 2026: contributor

re ACK 1f4c9b6f093522cdbdd55e9f99b239106e6d6daa

Removing the setup time from the actual benchmark timing will result in a more representative benchmark, still think this is an improvement over benchmarking with setup steps included.

sipa commented at 9:18 PM on January 27, 2026: member

Concept ACK. This would help with a number of benchmarks I've written before.

sedited commented at 8:46 AM on March 8, 2026: contributor

Concept ACK

Can you also add the description for the changes in the first commit from the upstream PR?

bench: add fluent API for untimed setup steps in `nanobench`

Some benchmarks need per-epoch state reset so every measured run does the same work.
Add `Bench::setup(...).run(...)` for untimed per-epoch setup.
The existing `run()` now delegates to `runImpl()` with an empty setup lambda, keeping the old API unchanged.

This vendors the upstream change from `martinus/nanobench`.
The upstream PR also adds tests that verify setup is excluded from timing and runs once before each epoch's iterations.
Those tests are not copied here because wiring them into `src/bench/nanobench.h` outside the benchmarking setup would be awkward.

The `Default is 1ms, so we are mostly relying ...` comment update matches current upstream `nanobench` master.

-------

Running a few benchmarks (which will be migrated in the next commit to use the new setup method) several times to showcase the spread:

./build/bin/bench_bitcoin -filter='^(BnBExhaustion|AddrManAddThenGood|DeserializeBlockTest|DeserializeAndCheckBlockTest|CheckBlockTest|LoadExternalBlockFile|FindByte|WalletCreatePlain|WalletCreateEncrypted|WalletLoadingDescriptors)$'

|               ns/op |                op/s |    err% |     total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
|       26,400,542.00 |               37.88 |    0.4% |      0.29 | `AddrManAddThenGood`
|          189,075.00 |            5,288.91 |    0.4% |      0.01 | `BnBExhaustion`

|            ns/block |             block/s |    err% |     total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
|        1,237,000.00 |              808.41 |    2.4% |      0.01 | `DeserializeAndCheckBlockTest`
|          893,333.00 |            1,119.40 |    0.6% |      0.01 | `DeserializeBlockTest`

|               ns/op |                op/s |    err% |     total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
|               31.62 |       31,622,370.70 |    0.2% |      0.01 | `FindByte`
|        5,506,875.00 |              181.59 |    1.4% |      0.06 | `LoadExternalBlockFile`
|      593,480,333.00 |                1.68 |    0.4% |      6.53 | `WalletCreateEncrypted`
|      174,305,167.00 |                5.74 |    0.7% |      1.93 | `WalletCreatePlain`
|      160,833,875.00 |                6.22 |    0.2% |      0.80 | `WalletLoadingDescriptors`

|               ns/op |                op/s |    err% |     total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
|       26,005,125.00 |               38.45 |    1.3% |      0.29 | `AddrManAddThenGood`
|          181,909.67 |            5,497.23 |    0.1% |      0.01 | `BnBExhaustion`

|            ns/block |             block/s |    err% |     total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
|        1,223,000.00 |              817.66 |    2.8% |      0.01 | `DeserializeAndCheckBlockTest`
|          892,917.00 |            1,119.92 |    0.7% |      0.01 | `DeserializeBlockTest`

|               ns/op |                op/s |    err% |     total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
|               31.58 |       31,660,608.70 |    0.5% |      0.01 | `FindByte`
|        5,612,750.00 |              178.17 |    1.1% |      0.06 | `LoadExternalBlockFile`
|      594,012,250.00 |                1.68 |    0.2% |      6.53 | `WalletCreateEncrypted`
|      174,668,334.00 |                5.73 |    0.8% |      1.92 | `WalletCreatePlain`
|      158,494,375.00 |                6.31 |    0.3% |      0.79 | `WalletLoadingDescriptors`

83b8528ddb

refactor: improve benchmark setup and execution for various tests

Note that `make_hard_case` already clears the UTXO pool in `coin_selection.cpp`.

./build/bin/bench_bitcoin -filter='^(BnBExhaustion|AddrManAddThenGood|DeserializeBlockTest|DeserializeAndCheckBlockTest|CheckBlockTest|LoadExternalBlockFile|FindByte|WalletCreatePlain|WalletCreateEncrypted|WalletLoadingDescriptors)$'

|               ns/op |                op/s |    err% |     total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
|       15,088,500.00 |               66.28 |    0.2% |      0.17 | `AddrManAddThenGood`
|          179,208.00 |            5,580.11 |    2.0% |      0.00 | `BnBExhaustion`

|            ns/block |             block/s |    err% |     total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
|          318,166.00 |            3,143.01 |    3.5% |      0.00 | `CheckBlockTest`
|          886,750.00 |            1,127.71 |    0.8% |      0.01 | `DeserializeBlockTest`

|               ns/op |                op/s |    err% |     total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
|               42.00 |       23,809,523.81 |    2.4% |      0.00 | `FindByte`
|        5,473,208.00 |              182.71 |    0.4% |      0.06 | `LoadExternalBlockFile`
|      584,168,041.00 |                1.71 |    0.3% |      6.43 | `WalletCreateEncrypted`
|      168,040,458.00 |                5.95 |    1.1% |      1.85 | `WalletCreatePlain`
|      155,446,625.00 |                6.43 |    0.7% |      0.78 | `WalletLoadingDescriptors`

|               ns/op |                op/s |    err% |     total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
|       14,894,917.00 |               67.14 |    0.3% |      0.16 | `AddrManAddThenGood`
|          177,667.00 |            5,628.51 |    1.3% |      0.00 | `BnBExhaustion`

|            ns/block |             block/s |    err% |     total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
|          313,791.00 |            3,186.83 |    3.8% |      0.00 | `CheckBlockTest`
|          888,208.00 |            1,125.86 |    0.7% |      0.01 | `DeserializeBlockTest`

|               ns/op |                op/s |    err% |     total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
|               41.00 |       24,390,243.90 |    2.4% |      0.00 | `FindByte`
|        5,445,208.00 |              183.65 |    1.0% |      0.06 | `LoadExternalBlockFile`
|      581,800,500.00 |                1.72 |    0.4% |      6.40 | `WalletCreateEncrypted`
|      166,035,583.00 |                6.02 |    0.5% |      1.82 | `WalletCreatePlain`
|      153,574,792.00 |                6.51 |    0.1% |      0.77 | `WalletLoadingDescriptors`

8825051e08

l0rinc force-pushed on Mar 8, 2026

l0rinc commented at 12:05 PM on March 8, 2026: contributor

Added in the latest rebase: updated the first commit message with the upstream description of the setup(...).run(...) change, noted that run() now delegates to runImpl() with an empty setup lambda, and mentioned that the corresponding tests were only added upstream. No code changes, just commit message updates.

<details><summary>Range diff for latest push</summary>

B=1f4c9b6f093522cdbdd55e9f99b239106e6d6daa A=8825051e0845e036ca284717806468193d4501b4 && git fetch origin $B $A && git range-diff --creation-factor=95 $B...$A

</details>

l0rinc requested review from janb84 on Mar 8, 2026

furszy commented at 5:07 PM on March 8, 2026: member

Concept ACK. I thought way too many times about adding something like this. Even suggested it recently #31774 (review).

davidgumberg commented at 11:38 PM on March 10, 2026: contributor

Concept ACK, have wanted this numerous times e.g. #31774.

janb84 commented at 9:38 AM on March 11, 2026: contributor

re ACK 8825051e0845e036ca284717806468193d4501b4

No code changes since last ACK, only commit message improvements.

DrahtBot requested review from furszy on Mar 11, 2026

DrahtBot requested review from davidgumberg on Mar 11, 2026

DrahtBot requested review from sipa on Mar 11, 2026

DrahtBot requested review from sedited on Mar 11, 2026

sedited approved

sedited commented at 8:41 AM on March 20, 2026: contributor

ACK 8825051e0845e036ca284717806468193d4501b4

achow101 commented at 8:45 PM on April 6, 2026: member

ACK 8825051e0845e036ca284717806468193d4501b4

achow101 merged this on Apr 6, 2026

achow101 closed this on Apr 6, 2026

in src/bench/checkblock.cpp:32 in 8825051e08

  37 | -        bool rewound = stream.Rewind(benchmark::data::block413567.size());
  38 | -        assert(rewound);
  39 | -    });
  40 | +    DataStream stream;
  41 | +    bench.unit("block").epochIterations(1)
  42 | +        .setup([&] { stream = DataStream{benchmark::data::block413567}; })

maflcko commented at 11:26 AM on April 7, 2026:

in theory, they could be using SpanReader, according to #34483

I've checked that this does not change the benchmark result, but allows to skip the setup code:

diff --git a/src/bench/checkblock.cpp b/src/bench/checkblock.cpp
index cbba543f2d..42a247b97c 100644
--- a/src/bench/checkblock.cpp
+++ b/src/bench/checkblock.cpp
@@ -29,6 +29,6 @@ static void DeserializeBlockTest(benchmark::Bench& bench)
 {
-    DataStream stream;
-    bench.unit("block").epochIterations(1)
-        .setup([&] { stream = DataStream{benchmark::data::block413567}; })
-        .run([&] { CBlock block; stream >> TX_WITH_WITNESS(block); });
+    bench.unit("block").run([&] {
+        CBlock block;
+        SpanReader{benchmark::data::block413567} >> TX_WITH_WITNESS(block);
+    });
 }
@@ -41,7 +41,7 @@ static void CheckBlockTest(benchmark::Bench& bench)
     CBlock block;
-    bench.unit("block").epochIterations(1)
+    bench.unit("block")
+        .epochIterations(1)
         .setup([&] {
             block = CBlock{};
-            DataStream stream{benchmark::data::block413567};
-            stream >> TX_WITH_WITNESS(block);
+            SpanReader{benchmark::data::block413567} >> TX_WITH_WITNESS(block);
         })

l0rinc commented at 12:04 PM on April 7, 2026:

Yes, that's basically how I'm rebasing https://github.com/bitcoin/bitcoin/pull/32554/changes#diff-547fa26a77a99ccd77aca7ce1c69c0544666f788d463dc7bff664001f9ff1c88R23 now. Will add you as coauthor.

l0rinc commented at 12:13 PM on April 7, 2026:

Note that this is what I already did in https://github.com/l0rinc/bitcoin/pull/31/changes#diff-547fa26a77a99ccd77aca7ce1c69c0544666f788d463dc7bff664001f9ff1c88R29 - and I can confirm that the speed is very close to DataStream.

l0rinc commented at 7:29 PM on April 7, 2026:

Extracted to https://github.com/bitcoin/bitcoin/pull/35025

l0rinc deleted the branch on Apr 7, 2026