Skip to content

[zero] perf(zebra-consensus): cache successful transparent script verification - #45

Open
aphelionz wants to merge 1 commit into
mainfrom
claude/zooko-pr-feedback-2a26d1
Open

[zero] perf(zebra-consensus): cache successful transparent script verification#45
aphelionz wants to merge 1 commit into
mainfrom
claude/zooko-pr-feedback-2a26d1

Conversation

@aphelionz

@aphelionz aphelionz commented Aug 21, 2026

Copy link
Copy Markdown
Member

Reintroduces transparent script verification reuse: a transaction verified at mempool admission is not re-verified in the block that mines it (zebra lost this in upstream #10494). This is the caching half of #34, redesigned per its review; the overlapping-UTXO-lookups half is #46.

  • Key: the transaction's WtxId alone. The ZIP-244 txid commits to the consensus branch id and to the outpoints, which pin the immutable prevout scripts and values; the authorizing-data digest commits to the scriptSigs, so a CVE-2026-34377 same-txid twin misses. v1-v4 ids don't commit to the branch id and are never cached.
  • Replacement: random victim slot via seeded SipHash-1-3 (siphasher, already in the dependency tree and vet-exempted); unguessable seed in production, fixed seed in tests. One deviation from the review's sketch: the victim is the hash of an insert counter, not of the incoming key, so the choice is uniform over the slots (a per-key victim would be direct-mapped, and two hot colliding keys would evict each other forever).
  • The cache key is derived from the transaction being verified, never from the caller-supplied request hash, so no field on a public request type can decide whether scripts run.
  • Cache check, script verification, and success-only insert are co-located in make_transparent_input_and_output_checks (renamed: it builds deferred checks, it doesn't verify).
  • Transactions spending unmined mempool outputs bypass the cache entirely (zebra#10346 pairing history).

Benchmarks (cargo bench -p zebra-consensus --bench script; a 1001-input P2SH consolidation with a real ECDSA signature on every input, so a miss pays the interpreter, ZIP-244 sighash, and signature verification per input):

bench time
the per-input script work a hit skips (serial) 165 ms
block-path verification, cache miss 18.0 ms
block-path verification, cache hit 2.1 ms

The miss/hit gap is smaller than the serial number because the verifier spreads input checks across cores; a 4-vCPU host sits correspondingly closer to the serial figure.

145/145 cargo nextest run -p zebra-consensus; fmt, clippy -D warnings, and cargo vet --locked clean.

🤖 Generated with Claude Code

Copilot AI lite review requested due to automatic review settings August 21, 2026 14:16

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Reintroduces a process-global cache in zebra-consensus so successful transparent script verification performed during mempool admission can be reused during subsequent block verification (v5+ only, keyed by WtxId), reducing repeated script-interpreter/FFI work.

Changes:

  • Add a bounded, randomly-evicting transparent script verification cache (transaction/script_cache.rs) keyed by WtxId, and consult/insert it from the transparent script-check pipeline.
  • Wire the cache into both block and mempool transaction verification paths, including bypass for spends of unmined mempool outputs.
  • Add extensive tests and a Criterion benchmark to validate and measure cache hits/misses.

Reviewed changes

Copilot reviewed 6 out of 7 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
zebra/zebra-consensus/src/transaction.rs Integrates script-cache keying and hit/miss behavior into transparent script verification checks for block/mempool verification.
zebra/zebra-consensus/src/transaction/script_cache.rs New bounded, process-global cache for successful transparent script verifications with keyed random replacement + metrics/test hooks.
zebra/zebra-consensus/src/transaction/tests.rs Adds cache-focused integration tests covering key soundness (twins, v4 exclusion, poisoning resistance, cross-upgrade behavior, mempool→block reuse).
zebra/zebra-consensus/Cargo.toml Adds siphasher dependency and registers the new script benchmark target.
zebra/zebra-consensus/benches/script.rs New benchmark measuring per-input script verification cost and end-to-end block verification miss vs hit.
zebra/Cargo.toml Adds siphasher to workspace dependencies.
zebra/Cargo.lock Locks siphasher into the dependency graph for zebra-consensus.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread zebra/zebra-consensus/src/transaction/script_cache.rs

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 6 out of 7 changed files in this pull request and generated no new comments.

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

zebra/zebra-consensus/src/transaction/script_cache.rs:168

  • victim_index calls self.siphasher.hash(&key.as_bytes()), but SipHasher13 is typically used via the std::hash::Hasher API (stateful), not via a pure hash() helper. If this is using the Hasher implementation, reusing a single SipHasher13 instance would make victim selection depend on prior calls and risks thread-safety issues; if hash() is not available, this won’t compile. Prefer storing the SipHash key/seed and constructing a fresh SipHasher13 per victim_index call (or cloning/resetting per call) to get a deterministic PRF output for just key.
    /// The slot a full cache replaces when inserting `key`.
    fn victim_index(&self, key: &WtxId) -> usize {
        // Casts are lossless: `capacity` is a usize, and the modulus keeps
        // the result below it.
        (self.siphasher.hash(&key.as_bytes()) % self.capacity as u64) as usize
    }

@aphelionz
aphelionz force-pushed the claude/zooko-pr-feedback-2a26d1 branch from 0ddc4d4 to 5ad9173 Compare August 25, 2026 13:40
Copilot AI review requested due to automatic review settings August 25, 2026 13:40

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 6 out of 7 changed files in this pull request and generated no new comments.

aphelionz added a commit that referenced this pull request Aug 26, 2026
Path-gated informational job (not a required check): runs the script-cache
and UTXO-lookup criterion benches on PRs and main pushes that touch the
benched crates, and posts the timings to the step summary. Bench steps are
guarded on their bench files existing, so this lands before #45/#46 and
activates as they merge. rust-cache persists target/criterion, so criterion
also reports a change estimate against the previous cached run.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings August 26, 2026 13:32
@aphelionz
aphelionz force-pushed the claude/zooko-pr-feedback-2a26d1 branch from 5ad9173 to f4f43f0 Compare August 26, 2026 13:32
…ification

Block verification re-runs every input script already verified at mempool
admission; zcashd skips the repeat via its script and signature caches, and
zebra has had no reuse since upstream #10494. Remember each verified
transaction by its WtxId (v5+ only: the ZIP-244 id commits to the branch id,
the outpoints, and the scriptSigs), evict randomly via keyed siphash, and
skip only the per-input script checks on a hit. Check, verification, and
insert are co-located in one function; transactions spending unmined mempool
outputs bypass the cache (zebra#10346).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@aphelionz
aphelionz force-pushed the claude/zooko-pr-feedback-2a26d1 branch from f4f43f0 to 9d30ba9 Compare August 26, 2026 13:34

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 6 out of 7 changed files in this pull request and generated 1 comment.

Suppressed comments (1)

zebra/zebra-consensus/src/transaction/tests.rs:4221

  • The panic message is inverted: this branch runs when the mock input is not a PrevOut, but the message says it is.
    let (input, known_utxos) = uniquely_sourced(0xA4, input, known_utxos);
    let transparent::Input::PrevOut {
        outpoint, sequence, ..
    } = input
    else {
        panic!("mock input is a PrevOut");

Comment on lines +3697 to +3704
let transparent::Input::PrevOut {
outpoint,
unlock_script,
sequence,
} = input
else {
panic!("mock input is a PrevOut");
};
Copilot AI review requested due to automatic review settings August 26, 2026 13:37

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 6 out of 7 changed files in this pull request and generated no new comments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants