Parallelize block input script checks for performance improvement - #7
Conversation
Flatten full block verification into ordered per-input Rayon work while preserving kernel authority and transaction/input error precedence. The paired 0..150k kernel/Fjall replay improved total wall time by 1.148x and script verification by 1.42x with the canonical stop hash. A final current-tree replay completed in 183.84s.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
📝 WalkthroughSummary by CodeRabbit
WalkthroughChangesConsensus block verification
Sequence Diagram(s)sequenceDiagram
participant Node as Node block application
participant Consensus as verify_block_input_scripts
participant Rayon as Rayon workers
participant UTXO as UTXO views
Node->>UTXO: Resolve transaction prevouts in block order
UTXO-->>Node: Owned prevout matrix
Node->>Consensus: Verify block input scripts
Consensus->>Rayon: Run input script checks in parallel
Rayon-->>Consensus: Return script results
Consensus-->>Node: Return first deterministic failure or success
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 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 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/consensus/src/verify_tx.rs (1)
513-517: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winYou cache the serialization in prep, then clone the whole thing per input. That's not caching, that's laundering an allocation.
prep.serializedis computed exactly once inprepare_block_input_checks(Line 467) so the transaction bytes are paid for one time. Then here, in the parallel per-input closure,prep.serialized.clone()deep-copies the entire serialized transaction for every input. A transaction with K inputs now allocates and memcpy's the full tx K times — on the single hottest path in the whole node. The single-tx path (Lines 174-177) got this right by threading one&mut Option<Vec<u8>>across inputs; the parallel path regressed it.Share the bytes instead of copying them: make
PreparedTx::serializedanArc<Vec<u8>>and clone theArc(a refcount bump), or handverify_input_script_portableanOption<&[u8]>read-only view.🤖 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_tx.rs` around lines 513 - 517, Update the parallel per-input verification path around PreparedTx::serialized and verify_input_script_portable to avoid deep-copying the serialized transaction for each input. Share the cached bytes by changing PreparedTx::serialized to an Arc<Vec<u8>> and cloning only the Arc, or pass an Option<&[u8]> read-only view while preserving the existing non-bitcoinconsensus behavior.
🤖 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.
Nitpick comments:
In `@crates/consensus/src/verify_tx.rs`:
- Around line 513-517: Update the parallel per-input verification path around
PreparedTx::serialized and verify_input_script_portable to avoid deep-copying
the serialized transaction for each input. Share the cached bytes by changing
PreparedTx::serialized to an Arc<Vec<u8>> and cloning only the Arc, or pass an
Option<&[u8]> read-only view while preserving the existing non-bitcoinconsensus
behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: QUIET
Plan: Pro Plus
Run ID: 9fffb1a3-01d7-4b67-9108-d04218720118
📒 Files selected for processing (5)
crates/consensus/Cargo.tomlcrates/consensus/src/kernel.rscrates/consensus/src/lib.rscrates/consensus/src/verify_tx.rscrates/node/src/apply.rs
📜 Review details
⏰ Context from checks skipped due to timeout. (4)
- GitHub Check: kernel-node
- GitHub Check: bench-smoke
- GitHub Check: test
- GitHub Check: clippy
🔇 Additional comments (9)
crates/consensus/Cargo.toml (1)
33-33: LGTM!crates/consensus/src/verify_tx.rs (4)
150-182: LGTM!Also applies to: 184-253, 255-280, 282-317
339-489: LGTM!
1178-1405: LGTM!
503-510: 🩺 Stability & AvailabilityNo change needed.
PreparedKernelTxis onlypar_iter-shared in Rust, and the shared-precompute kernel API is intentionally designed for concurrent per-input verification.> Likely an incorrect or invalid review comment.crates/consensus/src/kernel.rs (1)
24-96: LGTM!Also applies to: 175-176
crates/consensus/src/lib.rs (1)
64-66: LGTM!Also applies to: 168-175
crates/node/src/apply.rs (2)
921-971: LGTM!
1781-1863: LGTM!
No description provided.