perf(consensus): batch Merkle hashing with AVX2 - #30
Conversation
|
Warning Review limit reached
Next review available in: 46 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (9)
📝 WalkthroughSummary by CodeRabbit
WalkthroughChangesThe consensus crate adds runtime-dispatched eight-way AVX2 SHA256d hashing and TXID-specific Merkle reduction. Node preflight reuses prepared TXIDs and scratch storage. Benchmarks, replay data, and performance documentation record correctness and performance results. AVX2 Merkle validation
Sequence Diagram(s)sequenceDiagram
participant WindowPreflight
participant MerkleVerifier
participant Avx2Sha256d64
participant ScalarFallback
WindowPreflight->>MerkleVerifier: validate block with prepared TXIDs
MerkleVerifier->>Avx2Sha256d64: hash eight Merkle pairs when AVX2 is available
MerkleVerifier->>ScalarFallback: process remaining pairs and fallback path
Avx2Sha256d64-->>MerkleVerifier: return parent TXIDs
ScalarFallback-->>MerkleVerifier: return parent TXIDs
MerkleVerifier-->>WindowPreflight: accept or abort window proof generation
Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@codex review |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
crates/consensus/src/verify_block.rs (1)
144-155: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument that this function destroys the caller's buffer.
The doc comment says the function computes a root from caller-supplied transaction IDs. It does not say that
txidsis reduced in place and ends holding one element. That is exactly the fact a second caller needs, and the signature&mut Vec<Txid>alone does not say it. The name says "matches", which reads as a pure predicate. It is not.Also state that
falsecovers three distinct cases: root mismatch, empty input, and an encoding failure. Apubitem, hidden or not, gets used.♻️ Proposed doc change
/// Root-only precheck for the hot windowed apply path. /// /// Computes the merkle root from caller-supplied transaction IDs and compares /// it to the block header. Mutation is intentionally ignored here; the later /// consensus path owns the mutation check and its error precedence. +/// +/// `txids` is scratch storage and is consumed: the reduction runs in place and +/// leaves the vector holding a single element. Callers must refill it before +/// any further use. +/// +/// Returns `false` when the root differs, when `txids` is empty, or when +/// encoding fails. #[doc(hidden)]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/consensus/src/verify_block.rs` around lines 144 - 155, Update the doc comment for block_merkle_root_matches_txids to state that it mutates the caller-provided buffer in place, reducing txids to one element. Document that false is returned for a merkle-root mismatch, empty input, or encoding failure, while preserving the existing implementation behavior.crates/consensus/benches/merkle.rs (2)
27-52: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftThird copy of the same scalar reducer.
This is now the third implementation of scalar Merkle reduction in the tree:
next_merkle_levelincrates/consensus/src/verify_block.rs,next_merkle_level_scalarnext to it, and this one. Three copies of consensus-shaped logic drift. When they drift, the benchmark and the parity test both keep passing while measuring different things, which is worse than having no oracle at all.The benchmark cannot reach a
#[cfg(test)]item, so the fix is to expose the scalar reducer behind a#[doc(hidden)] pubentry point, or abenchfeature, and have all three call it.Separately, the two
assert!calls sit inside the measured loop.consensus_encodeinto an in-memory hash engine cannot fail. Hoist them out or drop them; do not measure branches you do not care about.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/consensus/benches/merkle.rs` around lines 27 - 52, Replace the duplicate scalar Merkle reducer in scalar_merkle and the neighboring implementations with one shared reducer, exposing the canonical entry point from verify_block.rs as #[doc(hidden)] pub or through the existing bench feature so the benchmark can call it. Update all three callers to use that entry point while preserving mutation detection and reduction behavior. Remove or hoist the infallible consensus_encode assert branches out of the measured reduction loop.
92-103: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueThe measured region includes the buffer refill.
scratch.clone_from(&input)runs insideb.iteron both arms. At 1024 parent pairs that is a 64 KB copy per iteration charged to both the AVX2 arm and the scalar arm. The ratio is therefore understated, not inflated, so the reported numbers are conservative and the conclusion holds.State that in the custody record.
criterion.parents_1024_speedupindocs/benchmarks/data/end-to-end-sync/avx2-merkle-custody-v1.jsonreads as a kernel ratio, and it is not one. Useiter_batchedwithBatchSize::SmallInputif you want the kernel number.Also applies to: 112-126
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/consensus/benches/merkle.rs` around lines 92 - 103, Update the Merkle benchmark functions around block_merkle_root_matches_txids and scalar_merkle to use Criterion’s iter_batched with BatchSize::SmallInput, moving scratch.clone_from(&input) into the setup phase so only the kernel execution is measured. If the custody record remains a kernel-ratio claim, regenerate the reported parents_1024 speedup accordingly; otherwise document that the existing criterion.parents_1024_speedup includes buffer-refill cost and is conservative rather than a pure kernel ratio.crates/consensus/src/sha256d64.rs (1)
358-362: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winEvery test for this feature passes without testing the feature. Runtime dispatch means
detect_avx2()decides what runs, and no test records which path it got. On a host without AVX2 the kernel tests return immediately and the parity tests compare the scalar oracle against the scalar oracle. The suite goes green having verified nothing about eight-way hashing. The issue requires byte-identical scalar and AVX2 outputs; right now CI cannot tell you whether it ever checked.
crates/consensus/src/sha256d64.rs#L358-L362: print a skip marker in theelsearm of thelet Some(avx2) = detect_avx2()binding, and apply the same change at Lines 387-391.crates/consensus/src/verify_block.rs#L551-L575: report the detected backend so a green parity run can be distinguished from a scalar-versus-scalar tautology, and apply the same to the other reducer-equivalence tests below it.Then make one CI lane assert that AVX2 was detected, so a host silently losing the feature fails the build instead of quietly skipping the consensus kernel.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/consensus/src/sha256d64.rs` around lines 358 - 362, The AVX2 tests can silently skip or compare scalar output against itself, and CI does not require AVX2 coverage. In crates/consensus/src/sha256d64.rs:358-362 and the sibling site at 387-391, print an explicit skip marker when detect_avx2() returns None; in crates/consensus/src/verify_block.rs:551-575 and the other reducer-equivalence tests below it, report the detected backend. Add a CI lane that asserts AVX2 detection so the feature tests cannot pass without exercising the AVX2 implementation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/benchmarks/data/end-to-end-sync/avx2-merkle-custody-v1.json`:
- Around line 417-424: Update the optional_local_archive entries in the
benchmark artifact to remove machine-specific /home/alpha/ prefixes, replacing
them with a reproducible placeholder such as $BENCH_RESULTS or removing the
block entirely.
In
`@docs/solutions/performance-issues/txid-parallelization-delivers-2x-but-core-still-leads.md`:
- Around line 2-13: Rename the document to reflect its current conclusion that
bitcoin-rs beats Core by 1.32x, then update every reference to the old filename,
including the link in CONCEPTS.md. Use repository-wide search for the stale path
to ensure no references remain.
---
Nitpick comments:
In `@crates/consensus/benches/merkle.rs`:
- Around line 27-52: Replace the duplicate scalar Merkle reducer in
scalar_merkle and the neighboring implementations with one shared reducer,
exposing the canonical entry point from verify_block.rs as #[doc(hidden)] pub or
through the existing bench feature so the benchmark can call it. Update all
three callers to use that entry point while preserving mutation detection and
reduction behavior. Remove or hoist the infallible consensus_encode assert
branches out of the measured reduction loop.
- Around line 92-103: Update the Merkle benchmark functions around
block_merkle_root_matches_txids and scalar_merkle to use Criterion’s
iter_batched with BatchSize::SmallInput, moving scratch.clone_from(&input) into
the setup phase so only the kernel execution is measured. If the custody record
remains a kernel-ratio claim, regenerate the reported parents_1024 speedup
accordingly; otherwise document that the existing criterion.parents_1024_speedup
includes buffer-refill cost and is conservative rather than a pure kernel ratio.
In `@crates/consensus/src/sha256d64.rs`:
- Around line 358-362: The AVX2 tests can silently skip or compare scalar output
against itself, and CI does not require AVX2 coverage. In
crates/consensus/src/sha256d64.rs:358-362 and the sibling site at 387-391, print
an explicit skip marker when detect_avx2() returns None; in
crates/consensus/src/verify_block.rs:551-575 and the other reducer-equivalence
tests below it, report the detected backend. Add a CI lane that asserts AVX2
detection so the feature tests cannot pass without exercising the AVX2
implementation.
In `@crates/consensus/src/verify_block.rs`:
- Around line 144-155: Update the doc comment for
block_merkle_root_matches_txids to state that it mutates the caller-provided
buffer in place, reducing txids to one element. Document that false is returned
for a merkle-root mismatch, empty input, or encoding failure, while preserving
the existing implementation behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d8ec4aa6-90c3-4a4f-8e23-553259bcc60e
📒 Files selected for processing (9)
CONCEPTS.mdcrates/consensus/Cargo.tomlcrates/consensus/benches/merkle.rscrates/consensus/src/lib.rscrates/consensus/src/sha256d64.rscrates/consensus/src/verify_block.rscrates/node/src/apply.rsdocs/benchmarks/data/end-to-end-sync/avx2-merkle-custody-v1.jsondocs/solutions/performance-issues/txid-parallelization-delivers-2x-but-core-still-leads.md
📜 Review details
🧰 Additional context used
🪛 LanguageTool
docs/solutions/performance-issues/txid-parallelization-delivers-2x-but-core-still-leads.md
[uncategorized] ~519-~519: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...831s | | index publication | 1.751s | | full body persistence | 4.660s | The 2.831s valu...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
🪛 OpenGrep (1.26.0)
docs/benchmarks/data/end-to-end-sync/avx2-merkle-custody-v1.json
[ERROR] 343-343: Possible credit card number (PAN) detected in source code. Credit card numbers should never be hardcoded or stored in source files. Use a secrets manager or tokenization service instead.
(coderabbit.pii.credit-card-number)
🔍 Remote MCP Context7, Github Grep
Additional review context
- Bitcoin Core detects duplicate adjacent hashes before odd-leaf duplication, then duplicates the final hash when the level has an odd count and applies
SHA256D64to each pair. Empty input returns a zero hash. This is the reference ordering for mutation semantics. - Core defines
SHA256D64as double-SHA256 over independent 64-byte inputs, producing 32-byte outputs; its tests compare optimized results against individualCHash256computations. - Core includes an AVX2
Transform_8waySHA256d64 implementation, confirming that eight-way batching matches the upstream optimization model. - Rust documents
is_x86_feature_detected!("avx2")as runtime CPUID-based detection on x86/x86-64. Architecture-specific code should be conditionally compiled, and#[target_feature(enable = "avx2")]functions require unsafe calls guarded by feature detection. - Rust’s standard examples use the expected pattern: architecture
cfgguard → runtime AVX2 detection → unsafe AVX2 function → scalar fallback.
🔇 Additional comments (11)
crates/consensus/src/lib.rs (1)
45-46: LGTM!crates/consensus/src/sha256d64.rs (2)
50-126: LGTM!Also applies to: 145-349
128-144: 📐 Maintainability & Code QualityNo change needed; the toolchain is already pinned.
rust-toolchain.tomldeclares1.95.0, the workspace setsrust-version = "1.95.0", and CI installsdtolnay/rust-toolchain@1.95.0everywhere, so this intrinsic behavior is covered.crates/consensus/src/verify_block.rs (2)
157-220: LGTM!
222-254: LGTM!Also applies to: 510-549, 577-714
crates/consensus/benches/merkle.rs (1)
1-25: LGTM!Also applies to: 54-90
docs/benchmarks/data/end-to-end-sync/avx2-merkle-custody-v1.json (1)
1-416: LGTM!CONCEPTS.md (1)
51-52: LGTM!Also applies to: 64-65, 85-86
docs/solutions/performance-issues/txid-parallelization-delivers-2x-but-core-still-leads.md (1)
25-31: LGTM!Also applies to: 495-541
crates/consensus/Cargo.toml (1)
43-46: 📐 Maintainability & Code QualityNo change needed.
criterionis already declared incrates/consensus[dev-dependencies].crates/node/src/apply.rs (1)
1482-1490: 🗄️ Data Integrity & IntegrationNo action needed.
tx_plan.txids()comes fromKernelBlock::txids()parsed on the supplied raw block; the kernel and portable paths derive oneTxidper parsed transaction, andparse_block_for_applyrejects mismatched parsed/decoded transaction counts.
a30b831 to
37ef167
Compare
|
Review-body findings at
Fresh validation after rebasing onto PR #29:
No production reducer logic or Criterion harness changed, so the end-to-end replay and microbenchmark values were not regenerated. No new behavioral test was added, so mutation verification does not apply. |
37ef167 to
b7287ca
Compare
Use a runtime-gated eight-lane SHA256d64 kernel for complete Merkle pair batches while preserving scalar tails, unsupported hosts, mutation detection, and error ordering.
Expose the reducer backend in tests, document scratch-buffer consumption, and make custody paths and Criterion scope portable.
b7287ca to
9dc1281
Compare
|
@coderabbitai ultrareview |
|
@codex review |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== changed files ==='
git diff --name-status HEAD^ HEAD
echo '=== PR diff stat ==='
git diff --stat HEAD^ HEAD
echo '=== source outlines ==='
for f in \
crates/consensus/src/sha256d64.rs \
crates/consensus/src/verify_block.rs \
crates/node/src/apply.rs \
crates/consensus/benches/merkle.rs \
crates/consensus/src/lib.rs
do
if [ -f "$f" ]; then
echo "--- $f ---"
ast-grep outline "$f" --items all || true
fi
done
echo '=== complete changed Rust sources ==='
for f in \
crates/consensus/src/sha256d64.rs \
crates/consensus/src/verify_block.rs \
crates/node/src/apply.rs \
crates/consensus/benches/merkle.rs \
crates/consensus/src/lib.rs
do
if [ -f "$f" ]; then
echo "--- $f ---"
cat -n "$f"
fi
done
echo '=== exact diff for implementation and callers ==='
git diff --find-renames --find-copies HEAD^ HEAD -- \
crates/consensus/src/sha256d64.rs \
crates/consensus/src/verify_block.rs \
crates/node/src/apply.rs \
crates/consensus/benches/merkle.rs \
crates/consensus/src/lib.rs \
crates/consensus/Cargo.toml
echo '=== API and call-site search ==='
rg -n -C 4 \
'block_merkle_root_matches_txids|check_merkle_root|compute_merkle_root|detect_avx2|transform_8way|sha256d64' \
cratesLength of output: 123263 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== branches and merge-base candidates ==='
git branch -a --no-color
git show -s --format='HEAD=%H%nparents=%P%nsubject=%s' HEAD
for ref in origin/main origin/master main master; do
if git rev-parse --verify -q "$ref" >/dev/null; then
printf '%s merge-base: ' "$ref"
git merge-base HEAD "$ref"
fi
done
echo '=== AVX2 kernel ==='
sed -n '1,460p' crates/consensus/src/sha256d64.rs | cat -n
echo '=== Merkle reducer and public helpers ==='
sed -n '120,275p' crates/consensus/src/verify_block.rs | cat -n
echo '=== Merkle tests ==='
sed -n '540,780p' crates/consensus/src/verify_block.rs | cat -n
echo '=== apply_window Merkle preflight ==='
sed -n '1510,1605p' crates/node/src/apply.rs | cat -n
echo '=== single-block Merkle validation and Tx-plan creation ==='
sed -n '870,940p' crates/node/src/apply.rs | cat -n
rg -n -C 5 'struct BlockTxPlan|impl BlockTxPlan|fn tx_plan|txids\(&self\)' crates/node/src
echo '=== PR-wide diff against available default branch ==='
base=''
for ref in origin/main origin/master main master; do
if git rev-parse --verify -q "$ref" >/dev/null; then
base=$(git merge-base HEAD "$ref")
break
fi
done
if [ -n "$base" ]; then
echo "BASE=$base"
git diff --find-renames --find-copies "$base"...HEAD -- \
crates/consensus/src/sha256d64.rs \
crates/consensus/src/verify_block.rs \
crates/node/src/apply.rs \
crates/consensus/benches/merkle.rs \
crates/consensus/Cargo.toml
else
echo 'No default branch reference is available.'
fi
echo '=== related allocation and error-path tests ==='
rg -n -C 5 \
'apply_window|merkle_scratch|MerkleRoot|MerkleMutation|prepared.*txid|tx_plan\.txids' \
crates/node/src/apply.rs crates/consensus/src/verify_block.rsLength of output: 97882
The AVX2 dispatch boundary is correctly runtime-gated. The reducer preserves duplicate detection before odd-leaf duplication. Complete eight-pair groups use AVX2. Remaining pairs use the scalar path. The window precheck reuses prepared transaction IDs without bypassing the later mutation check. I found no correctness or integration defect in the reviewed changes. This conclusion is based on static inspection. The sandbox cannot execute repository code.
|
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
Summary
Processing-bound mainnet replay now beats the matched Bitcoin Core 31.0 control on both wall time and CPU time. The change reuses txids already prepared for script verification, then hashes eight independent Merkle parent pairs per AVX2 batch instead of repeating scalar SHA256d work.
The SIMD boundary stays private and runtime-gated. Unsupported targets and incomplete eight-pair tails use the existing scalar path. The reducer preserves Bitcoin's odd-leaf duplication and mutation semantics.
Performance
All panels used the same local blocks 0–150,000 corpus, full validation,
taskset -c 0-31, three runs per side, and a 30-second cooldown.Candidate peak RSS was 1.042× Core in the final interleaved panel, inside the 1.05× gate. The committed custody JSON contains the commands, every paired sample, source and binary hashes, corpus identity, exact backend state, and selected Criterion ratios.
Validation
cargo fmt --all -- --checkcargo clippy -p bitcoin-rs-consensus --all-targets -- -D warningscargo clippy -p bitcoin-rs-node --example mainnet_prefix_replay --features 'mimalloc,rocksdb,redb' -- -D warningscargo test --workspace --no-fail-fast: 1,354 passed across 116 suites; 15 ignoredCloses #27