#33637 validation: reduce block index comparison overhead
| Reviewability | Reviews | Agreement | Size | ||||
|---|---|---|---|---|---|---|---|
| #33637 validation: reduce block index comparison overhead | Ready | 0 (+2) | Strong | 26 + 244 tests | |||
|
|
|
| ||||
Reviewability: ReadyStalePaused Agreement: StrongPositivePositive w/ caveatsNeutralMildDisputedBlockedCrickets Reviews: current code-review ACKs, then (+stale ACKs) and -NACKs; greener = more ACKs. Size: lines added or modified outside tests, then in tests; greener = smaller. | |||||||
Goal
Inlines and optimizes `CBlockIndexWorkComparator` and `arith_uint256` comparisons by defining them directly in headers using `std::tie`, `noexcept`, and C++20 `operator<=>`. Replaces the out-of-line `CompareTo` member function on `base_uint` with an inline per-word three-way comparison loop. Adds dedicated microbenchmarks across multiple work bit-widths, unit tests, and differential fuzz tests.
Problem: Block index ordering in `setBlockIndexCandidates` and functions like `CheckBlockIndex` and `FindMostWorkChain` executes `CBlockIndexWorkComparator` heavily. The comparator and its underlying `arith_uint256` work comparisons were defined out-of-line with cascading conditionals, preventing compiler inlining and introducing unnecessary branch overhead.
Evidence of importance:
- Author's benchmarks show `CBlockIndexWorkComparator` throughput increases 6.5x–6.8x (from ~100M cmp/s to ~660-680M cmp/s across GCC and Clang) and `CheckBlockIndex` improves 1.4x–1.6x (from ~9-10k ops/s to ~14.4-14.8k ops/s).
Scope: Includes differential fuzz tests comparing the new comparator and arithmetic ordering implementations directly against pre-refactor snapshots.
Risk of a mistake: high (validation). A flaw in block index work comparison or tie-breaking could alter best-chain selection, potentially leading to chain forks or failure to activate the valid tip.
Changed since the previous assessment: Reorganized the commit stack to separate inlining from per-word `<=>` comparison changes, and extended the benchmark to test 12-bit and 96-bit chainwork cases alongside 256-bit cases.
Reviewability
Ready
Nothing known that would invalidate a review now.
Agreement
Strong
substantive support, no open objection (Raimo33, laanwj, mzumsande, optout21)
Thread notes: Initial record for PR #33637. Concerns raised by mzumsande (spaceship operator readability vs performance), sipa (need for differential fuzz testing), and maflcko (commit ordering of move-only inline changes) were all resolved with updated benchmarks, fuzz targets, and reordered commits.
Objections, suggestions, and questions:
| Reviewer | Type | Harm, request, or question | Status | Must be | Author replied | Quote |
|---|---|---|---|---|---|---|
| mzumsande (member) | objection, approach | the spaceship operator reduces readability for what appeared to be marginal speedup | fixed · fix pushed · v1, not yet re-checked | answered | yes | 2025-10-30: 'seems like inlining gives most of the speedup, whereas the gain of using the spaceship operator (which comes at the cost of readability) is marginal.' Status from: 2026-02-03: l0rinc: 'The biggest optimization is the spaceship (with inline), it's now the last commit.' |
| sipa (member) | objection, correctness | relying solely on unit tests risks missing equivalence bugs by relying on presumed knowledge of potential bugs | fixed · fix pushed · v1, not yet re-checked | answered | yes | 2026-02-02: 'You should really add a fuzz test here to compare old and new behavior as suggested before; it's the perfect use case for it, as it avoids the need for putting presumed knowledge about potential bugs in the unit tests you create.' Status from: 2026-02-02: l0rinc: 'Thanks for the hint, I've split the differential fuzzer and kept the simple unit tests' |
| maflcko (member) | objection, approach | bundling a trivial move-only speedup with complex changes increases review burden and obscures the value of each commit | fixed · fix pushed · v1, not yet re-checked | answered | yes | 2026-02-03: 'it would be better to just submit a trivial to review move-only change stand-alone, which wouldn't need in-depth review and dedicated differential unit/fuzz tests. Or at least the move-only change should be the first change.' Status from: 2026-02-03: l0rinc: 'I don't mind reordering the commits to make it obvious.' |
Support:
- Raimo33 (contributor) (Approach ACK, 2025-10-17): tested the new block index comparator
- laanwj (member) (ACK, 2025-10-22): the code changes look good, using <=> makes sense
- mzumsande (member) (Concept ACK, 2025-10-30): agrees with concept of optimizing comparator, though initially questioned spaceship operator gains
- optout21 (contributor) (ACK, 2025-11-03): an evident, localized micro-optimization in a hot-path
Participants:
- Raimo33 (contributor, support, 1 statements 2025-10-17 to 2025-10-17): Approach ACK after testing comparator
- sipa (member, objection, 2 statements 2025-10-17 to 2026-02-02): urged adding differential fuzz tests; author added fuzz targets
- Christewart (contributor, neutral, 1 statements 2025-10-18 to 2025-10-18): ran and posted benchmark output on macOS
- laanwj (member, support, 1 statements 2025-10-22 to 2025-10-22): code review ACK noting using <=> makes sense
- mzumsande (member, objection, 6 statements 2025-10-30 to 2025-10-30): Concept ACK, but questioned readability cost of spaceship operator
- optout21 (contributor, support, 6 statements 2025-11-03 to 2026-08-22): utACK and repeated reACKs for hot-path micro-optimization
- dergoegge (member, neutral, 1 statements 2025-11-06 to 2025-11-06): commented on fuzzing vs unit testing
- maflcko (member, objection, 2 statements 2025-11-06 to 2026-02-03): suggested structuring or separating move-only inline commit first
Files
244 lines under test/bench/ci.
- src/bench/blockstorage.cpp +69/-0
- src/test/fuzz/block_index_work_comparator.cpp +62/-0
- src/test/fuzz/arith_uint256_comparison_equivalence.cpp +57/-0
- src/test/blockchain_tests.cpp +34/-0
- src/node/blockstorage.cpp +0/-25
- src/test/arith_uint256_tests.cpp +19/-0
- src/node/blockstorage.h +15/-2
- src/arith_uint256.h +10/-3
- src/arith_uint256.cpp +0/-12
- src/test/fuzz/CMakeLists.txt +2/-0
- src/uint256.h +1/-1
- src/bench/CMakeLists.txt +1/-0
Card
The short text the ranking pass compares across PRs.
This PR optimizes block index work comparisons by inlining `CBlockIndexWorkComparator` and `arith_uint256` three-way comparisons in headers using `std::tie` and `operator<=>`. It eliminates out-of-line function calls and branching cascades during candidate chain searches and validation checks. The author reports a 6.5x to 6.8x speedup in isolated work comparisons and a 1.4x to 1.6x speedup in `CheckBlockIndex`. Equivalence fuzz targets and deterministic sorting tests verify that work ordering and pointer tie-breaking behavior are unchanged. No dependencies are stated.
Data
thread read through 2026-08-22 (39 statements), head 62ddd1197c
Discussion read under schema 1 rules: suggestions and questions may be missing, and an objection may combine several harms (schema history).
labels: Refactoring
Categories
Validation
P3 · speedup
Optimizes hot-path block index work comparison logic used during chainstate evaluation and candidates sorting. Benchmarks show `CBlockIndexWorkComparator` throughput increases 6.5x–6.8x and `CheckBlockIndex` ops/s improve 1.4x–1.6x, providing a worthwhile micro-optimization to block index operations.
Membership: Modifies `CBlockIndexWorkComparator` in `src/node/blockstorage.h` and arithmetic comparisons used for chainstate block index management.
Factors: security/stability 0, bug 0, performance 1, user value 0, leverage 0