[zero] perf(zebra-consensus): reuse verified transparent scripts and overlap UTXO lookups

[aphelionz]

Two changes to transaction verification, aimed at submitblock latency on consolidation-heavy blocks (~299KB, 1001-input P2SH transactions): block_spent_utxos now overlaps up to 64 state lookups instead of awaiting one round trip per input, and a new bounded cache (transaction/script_cache.rs) remembers successful transparent script verification, so a transaction verified at mempool admission is not re-verified in the block that mines it. zcashd has done this for years via its script and signature caches; zebra has had no reuse since #10494.

The cache key is (UnminedTxId, NetworkUpgrade, sha256d digest of the spent outputs in input order), which makes the CVE-2026-34377 twin shape (same txid, different authorizing data) a guaranteed miss; the full derivation lives in the module docs. A hit skips only the per-input script checks; inserts happen only after a fully successful verification, and spentness, fees, lock time, expiry, sigops, and shielded proofs rerun for every block.

Tests drive the production verifiers: an authorizing-data twin, poisoning at both insert sites, network-upgrade scoping, hit observation through a per-key counter, and a deterministic 1001-input wild-path test in which a regression to serial UTXO fetching deadlocks instead of passing slowly. A five-lens adversarial review ran pre-push and its findings are folded in. 145/145 under both cargo test and nextest; fmt and clippy -D warnings clean. Not here: shielded-proof reuse and upstreaming (ZF gate).

🤖 Generated with Claude Code

[Copilot]

Pull request overview

This PR reintroduces safe verification reuse in zebra-consensus by adding a transparent script verification cache keyed to prevent same-txid “twin” attacks, and by overlapping block-path UTXO lookups to reduce per-input state round-trip latency during block verification.

Changes:

• Add a process-wide transparent script verification cache keyed by (UnminedTxId, NetworkUpgrade, spent-outputs digest) and integrate it into mempool + block verification paths.
• Parallelize block spent-UTXO lookups using bounded concurrency (buffer_unordered(64)) while preserving input order for v5+ sighash semantics.
• Add extensive regression and safety tests covering cache key completeness, poisoning resistance, upgrade scoping, and the end-to-end “fat P2SH” path.

Reviewed changes

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

File	Description
zebra/zebra-consensus/src/transaction.rs	Adds bounded concurrent AwaitUtxo lookups for block verification and hooks transparent script cache checks/inserts into the async verification pipeline.
zebra/zebra-consensus/src/transaction/script_cache.rs	Introduces the process-wide transparent script verification cache (key, storage, eviction, metrics, and unit tests).
zebra/zebra-consensus/src/transaction/tests.rs	Adds targeted tests for CVE-shaped twins, failure non-recording, upgrade scoping, key-scoped removal behavior, and an end-to-end “wild path” regression test.

---

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

[Copilot]

Pull request overview

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

Suppressed comments (2)

zebra/zebra-consensus/src/transaction.rs:730

• The mempool-path cache insert happens before sigops and VerifiedUnminedTx::new(...) (which can still fail), so a transaction that is ultimately rejected by the mempool can still populate/evict entries in the process-global cache.

            if spent_mempool_outpoints.is_empty() && !tx.inputs().is_empty() {
                script_cache::verified_scripts().insert(script_cache::ScriptCacheKey::new(
                    tx_id,
                    nu,
                    cached_ffi_transaction.all_previous_outputs(),

zebra/zebra-consensus/src/transaction.rs:428

• The script-cache entry is inserted before later fallible checks (miner_fee and sigops), so a transaction that fails those checks can still populate/evict entries in the process-global cache, contradicting the “success-only” contract and enabling cache pollution via invalid transactions.

This issue also appears on line 726 of the same file.

            if !tx.is_coinbase() && !tx.inputs().is_empty() {
                script_cache::verified_scripts().insert(script_cache::ScriptCacheKey::new(
                    tx_id,
                    nu,
                    cached_ffi_transaction.all_previous_outputs(),

[zookoatshieldedlabs]

About the docs:

I found it a bit difficult to force myself to read through all the comments and commit messages because they are so copious and so Claudey. It would be helpful for me as a dumb old unaugmented human reviewer, and probably also for AI-powered reviewers and for future maintainers reading the code and old commits, if it was more succinct and non-redundant. The "Don't Repeat Yourself" principle applies to comments/docs/commit messages as well as to source code.

About the PR: It would be cool if the two optimizations in here were in separate commits/PRs so I could review each one separately, and each came with its own benchmarks so I could check the performance impact of each one separately.

About the design:

Cache key:

We do not need a separate digest of the spent outputs. UnminedTxId commits to each transparent input’s outpoint, and an outpoint uniquely identifies immutable previous-output contents, including its scriptPubKey and value. The checks returned by verify_transparent_inputs_and_outputs() depend on those contents, but not on the output’s current spent/unspent status; that contextual state check is performed separately.

I spent a long time trying to understand why the consensus branch ID is already folded into the v5 UnminedTxId but not the v4 ones. I'm still confused about it, and I figure if I'm confused about it, maybe at least some future code maintainers and auditors will also be confused about it. So then I hit on a nice simple solution: just never cache transactions with version earlier than 5. I'm pretty sure caching v4 transactions is not an important performance goal since those transactions are never produced by any current Zcash wallet implementation.

With those two pieces left out, then we can just use UnminedTxId as the cache key but only when it is the Witnessed enum variant. "Witnessed" means that it includes the authorization-data digest, which means its WtxId is the right value to use for this cache key. See the code below for an example of how to do that.

Replacement policy:

Instead of FIFO replacement, I'd recommend random-replacement. Random-replacement has better worst-case behavior:

Worst-case behavior can potentially be triggered adversarially in some cases. It is probably easier to use random-replacement than to figure out whether worst-case behavior could be triggered (accidentally or adversarially) in the current code, and to maintain that reasoning over future changes to the code. Another benefit of random-replacement is that it might be a little less code.

A good way to do random-replacement would be to use a siphash instance just for this purpose. It should be seeded by a seed unguessable to an adversary in production, but can be seeded with a fixed seed for reproducible testing and benchmarking.

Here's an implementation GPT-5.6-Sol and I just cooked up:

use siphasher::sip::SipHasher13;
use std::collections::HashSet;
use zebra_chain::transaction::WtxId;

struct RandomCache {
    capacity: usize,
    keys: HashSet<WtxId>,
    slots: Vec<WtxId>,
    siphasher: SipHasher13,
}

impl RandomCache {
    /// `capacity` is required to be > 0
    fn new(capacity: usize, seed: [u8; 16]) -> Self {
        assert!(capacity > 0, "cache capacity must be greater than zero");

        Self {
            capacity,
            keys: HashSet::with_capacity(capacity),
            slots: Vec::with_capacity(capacity),
            siphasher: SipHasher13::new_with_key(&seed),
        }
    }

    fn contains(&self, key: &WtxId) -> bool {
        debug_assert!(self.assert_invariants());

        self.keys.contains(key)
    }

    fn insert(&mut self, key: WtxId) {
        debug_assert!(self.assert_invariants());

        if !self.keys.contains(&key) {
            if self.slots.len() == self.capacity {
                let victim_index = self.victim_index(&key);
                let victim = std::mem::replace(&mut self.slots[victim_index], key);

                self.keys.remove(&victim);
            } else {
                self.slots.push(key);
            }

            self.keys.insert(key);
        }
    }

    fn victim_index(&self, key: &WtxId) -> usize {
        // SipHash guarantees that an adversary wouldn't be able to predict or control which key
        // gets evicted without knowing the seed.
        (self.siphasher.hash(&key.as_bytes()) % self.capacity as u64) as usize
    }

    // This takes expected O(N) time and O(N) temporary space. Do not call it except from inside
    // `debug_assert!()`.
    fn assert_invariants(&self) -> bool {
        self.slots.len() <= self.capacity
            && self.slots.len() == self.keys.len()
            && self.keys == self.slots.iter().copied().collect::<HashSet<_>>()
    }
}

(That depends on siphasher by Frank Denis: https://crates.io/crates/siphasher .)

About the code:

I'm confused about why populating the value in the cache is done in BlockTxVerifier::call() but checking for the value in the cache is done in verify_transparent_inputs_and_outputs(). Shouldn't those two things be right next to each other in the code? That would make it nice and easy for a reviewer to confirm that either one or the other always happens.

Hm, I'm confused about the async structure of this code. verify_transparent_inputs_and_outputs() doesn't actually verify them synchronously before it returns. Instead it constructs something that will verify them when it gets called and then returns that. (So maybe it should be named something like make_verifier_for_transparent_inputs_and_outputs()?)

Anyway, the code that I think would be easily audited would look like this in normal old sequential computation mode:

/// Returns true if and only if the transaction verifies.
fn synchronously_verify_transparent_inputs_and_outputs(
    tx: &Transaction,
    script_verifier: script::Verifier,
    cached_ffi_transaction: Arc<CachedFfiTransaction>,
) -> bool {
    if tx.is_coinbase() {
        // The script verifier only verifies PrevOut inputs and their corresponding UTXOs.
        // Coinbase transactions don't have any PrevOut inputs.
        return true;
    };

    if let UnminedTxId::Witnessed(wtx_id) = tx.unmined_id() {
        // For witnessed transactions, cache verifications:
        cache = script_cache::verified_scripts();
        if cache.contains(wtx_id) {
            return true;
        }

        if do_the_verification_synchronously_right_now_ill_wait(tx.inputs(), script_verifier, cached_ffi_transaction) {
            cache.insert(wtx_id);

            return true;
        } else {
            return false;
        }
    };

    // For legacy transactions, no caching:
    return do_the_verification_synchronously_right_now_ill_wait(tx.inputs(), script_verifier, cached_ffi_transaction);
}

I don't know how to accomplish the equivalent logic in inside-out async world, so that a reviewer (who understands async) can easily and locally verify that use of the cache is correct.

Okay, I haven't really looked at the "overlapping requests" part of this PR, just the caching part. Would you please submit a PR that just does the caching, and that comes with benchmarks, so I can review that separately from the overlapping-requests thing?