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

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:

• #32673 (clang-tidy: Apply modernize-deprecated-headers by maflcko)
• #32554 (bench: replace embedded raw block with configurable block generator by l0rinc)
• #32521 (policy: make pathological transactions packed with legacy sigops non-standard by darosior)
• #31989 (BIP-119 (OP_CHECKTEMPLATEVERIFY) (regtest only) by jamesob)
• #31682 ([IBD] specialize CheckBlock's input & coinbase checks by l0rinc)
• #29060 (Policy: Report reason inputs are non standard by ismaelsadeeq)

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.

I can't tell from the pull description, but is there a end-user visible performance difference? If yes, how much?

Thanks for the observations @tapcrafter, addressed your concerns in the latest push, adding you as a co-author in one of the commits.

@maflcko, to clarify the effect on the overall behavior, I slimmed down the DeserializeAndCheckBlockTest similarly to #32457 and added the results to the PR's description.

I ran

cmake -B build -DBUILD_BENCH=ON -DCMAKE_BUILD_TYPE=Release
cmake --build build -j$(nproc)
build/bin/bench_bitcoin -filter='CheckBlockBench' --min-time=10000

several times before and after the SigOp fast-path.

To isolate the remaining hot-spots I also:

• split the former DeserializeAndCheckBlockTest into a pure‐serialization (optimized independetly) and a pure CheckBlock benchmark;
• switched to block 784,588 (contains every modern script type) - see bench: replace benchmark block with more representative one (413567 → 784588) #32457;
• included another related optimization (special-cases the coinbase & duplicate-input checks - [IBD] specialize CheckBlock's input & coinbase checks #31682).

Before (ef73758 - bench: measure behavior of a more representative block):

|            ns/block |             block/s |    err% |       ins/block |       cyc/block |    IPC |      bra/block |   miss% |     total | benchmark
|--------------------:|--------------------:|--------:|----------------:|----------------:|-------:|---------------:|--------:|----------:|:----------
|        1,077,514.54 |              928.06 |    0.0% |    8,231,717.91 |    3,867,061.18 |  2.129 |   1,234,943.74 |    2.5% |     11.00 | `CheckBlockBench`
|        1,077,039.60 |              928.47 |    0.1% |    8,231,717.76 |    3,865,853.67 |  2.129 |   1,234,943.80 |    2.5% |     11.01 | `CheckBlockBench`
|        1,077,973.36 |              927.67 |    0.1% |    8,231,717.62 |    3,869,437.50 |  2.127 |   1,234,943.75 |    2.5% |     11.01 | `CheckBlockBench`

Before (9d71910 - specialize CheckBlock's input & coinbase checks):

|            ns/block |             block/s |    err% |       ins/block |       cyc/block |    IPC |      bra/block |   miss% |     total | benchmark
|--------------------:|--------------------:|--------:|----------------:|----------------:|-------:|---------------:|--------:|----------:|:----------
|          836,899.54 |            1,194.89 |    0.1% |    6,618,933.87 |    3,003,330.32 |  2.204 |     847,006.84 |    2.4% |     10.77 | `CheckBlockBench`
|          838,135.14 |            1,193.13 |    0.0% |    6,618,933.87 |    3,008,503.94 |  2.200 |     847,006.84 |    2.4% |     10.78 | `CheckBlockBench`
|          835,303.73 |            1,197.17 |    0.1% |    6,618,933.87 |    2,997,539.89 |  2.208 |     847,006.84 |    2.4% |     10.77 | `CheckBlockBench`

After (ddc82fd - script: short-circuit GetLegacySigOpCount for known scripts):

|            ns/block |             block/s |    err% |       ins/block |       cyc/block |    IPC |      bra/block |   miss% |     total | benchmark
|--------------------:|--------------------:|--------:|----------------:|----------------:|-------:|---------------:|--------:|----------:|:----------
|          753,774.48 |            1,326.66 |    0.0% |    5,224,278.78 |    2,705,172.05 |  1.931 |     597,353.76 |    3.4% |     10.70 | `CheckBlockBench`
|          753,337.59 |            1,327.43 |    0.0% |    5,224,278.78 |    2,704,108.71 |  1.932 |     597,353.76 |    3.4% |     10.70 | `CheckBlockBench`
|          749,374.04 |            1,334.45 |    0.0% |    5,224,278.78 |    2,690,235.39 |  1.942 |     597,353.75 |    3.4% |     10.69 | `CheckBlockBench`

~11.4% additional validation speed-up thanks to short-circuit SigOp counting (the two changes together result in a ~43.7% validation speedup for the given block).
Note that instruction count also dropped from 6,6M to 5.2M, branch executions from 0.8M to 0.6M.

Ah, ok. So from extrapolating the previous comment and #31682 (comment), the IBD speedup should be around 1%?

Might also help to add all the benchmark and test changes and simpler improvements like inlining in a separate PR, so it's a more clear how tricky the sensitive changes here are, and if the sensitive changes are really worth making.

@ryanofsky, @darosior, thank you for expressing your concerns.
I have removed the optimization part, I understand why that can be considered risky. The new rebase only contains extraction of the common script parts to avoid some work duplication and to help with understanding this part of the codebase better. It's also covered extensively with deterministic and random tests, comparing against fixed values, non-standard scripts, and completely random ones compared against the original implementation. We can reconsider the optimizations if this gets accepted.

Also note that the optimizations weren't there for IBD necessarily, but as the checkblock bench states:

// These are the two major time-sinks which happen after we have fully received
// a block off the wire, but before we can relay the block on to peers using
// compact block relay.

🚧 At least one of the CI tasks failed.
_{Task lint: https://github.com/bitcoin/bitcoin/runs/43633911782
LLM reason (✨ experimental): The CI failure is due to a lint check failure caused by improper include syntax in source files.}

@darosior, this change isn't about IBD, it was mostly about block connect time optimization originally - now it's rather meant as a cleanup and flooding it with tests and benchmarks to enable optimizations in follow-ups.

This further strengthens my point. I don't think we should make a major overhaul to tricky consensus-critical code that was never touched in 15 years just to "make it nicer" (and possibly enabling very marginal optimisations in the future).

I don't think we should make a major overhaul to tricky consensus-critical code that was never touched in 15 years

@darosior I almost wonder if we are looking at the same PR. The current set of changes seem pretty trivial and are certainly not a major overhaul.

@l0rinc since the general arguments darosior's making are worth taking seriously and apply to a wide variety of changes maybe including the changes made here, it could make sense to separate the concerns more by opening a different PR to only improve the benchmarks and tests and not refactor the script code.

IMO, having now looked at the script changes, I think they are trivial and not risky, and they could be merged with the normal review process and 3 acks. But a conversation about raising the standard for changes like this could be worth having here or somewhere else. I do think a more productive way of raising the standard would be to do something like increase the number of expected acks, instead of relying on people to shoot down changes individually with nacks.

In a project like this it is rightfully very hard to argue on the side of taking any risk, but we should look out for places where risks are conserved and trying to reduce them one place increases them other places. IMO, it is important for changes like this to be able to progress with sufficient review so we are not put in a position of maintaining code that we are afraid to modify and that no remaining developers will have familiarity with. They also help test our review process and build confidence that it works and catches real issues. And at least in the past, they've been useful for deploying security fixes before revealing them publicly.

A related question is how to decide what changes to spend time working on and reviewing given limited review and development bandwidth. I'm not sure there is an answer to this other than letting people individually decide for themselves, but I think if we want to shift attention different places, proactively asking for reviews and highlighting changes we want to encourage is probably also a better approach than trying to actively discourage other changes. "Don't think of a pink elephant" is not a wildly effective strategy.

@ryanofsky Maybe "overhaul" isn't the right term if it implies that a change is necessarily huge, and i should have used "refactoring" instead. Regardless, my point stands.

Thanks @ryanofsky, I have extracted the tests and benchmarks to #32729 - adding a few more test cases based on missing code and feature coverage I have observed since.
The non-test codes changes are either move only with tiny changes (e.g. front() instead of (*this)[0] ), separated to different commits to ease the review as much as possible.
@darosior, you're also welcome to review or to ignore, up to you of course.

Edit: Now that the testing part was extracted I'm closing this to avoid confusion