diff --git a/defects/zebra-0001/TICKET.md b/defects/zebra-0001/TICKET.md new file mode 100644 index 000000000..81a5c144b --- /dev/null +++ b/defects/zebra-0001/TICKET.md @@ -0,0 +1,65 @@ +# zebra-0001: ZIP-317 block template `has_direct_dependencies` O(T×S) Vec scan + +**MOAD:** MOAD-0001 (CWE-407) +**Severity:** LOW-MEDIUM +**Speedup:** ~500x at T=1000 (pathological chain dependency topology) +**Language:** Rust +**File:** zebra-rpc/src/methods/types/get_block_template/zip317.rs:228 + +## Description + +`has_direct_dependencies()` is called for each dependent transaction during +block template construction (ZIP-317). It checks whether all of a candidate +transaction's dependencies are already in `selected_txs` by iterating the +entire `Vec` and counting how many entries appear in a +`HashSet`. + +```rust +fn has_direct_dependencies( + candidate_tx_deps: Option<&HashSet>, + selected_txs: &Vec, +) -> bool { + // ... + let mut num_available_deps = 0; + for tx in selected_txs { // O(S) per call + if deps.contains(&tx.transaction.id.mined_id()) { // O(1) HashSet lookup + num_available_deps += 1; + } + if num_available_deps == deps.len() { + return true; + } + } + false +} +``` + +In `checked_add_transaction_weighted_random`, each time a transaction is +selected and its dependents are evaluated, `has_direct_dependencies` iterates +all `S` already-selected transactions. As `S` grows with each selected tx, +the total work across a block template build is O(T×S_avg) = O(T²) for T +transactions in a deep dependency chain. + +A more efficient approach: maintain a `HashSet` of selected +tx IDs alongside the `Vec`. Then `has_direct_dependencies` reduces to O(D) +per check (D = number of direct dependencies, typically 1-3), giving O(T×D) +total. + +## Complexity + +- Current: O(T²) worst case for T txs in chain dependency topology +- Fixed: O(T×D) where D = average direct dependency count (typically 1-3) +- Speedup: ~T/D ≈ 333x at T=1000, D=3 + +## Context + +Block template construction runs once per block (~75 seconds on Zcash mainnet). +Zcash block size limits bound T to around 1000 transactions in practice. +This is not a hot path, so impact is LOW in typical operation, but a miner +constructing blocks from a deep-chained mempool would observe the worst case. + +## Fix + +Pass a `HashSet` of already-selected tx IDs alongside +`selected_txs`, built incrementally as transactions are added. Replace the +Vec scan in `has_direct_dependencies` with a single HashSet intersection +check: all deps must be present in `selected_tx_ids`. diff --git a/defects/zebra-0001/patch/zebra-0001-zip317-dep-check-hashset.patch b/defects/zebra-0001/patch/zebra-0001-zip317-dep-check-hashset.patch new file mode 100644 index 000000000..7a43572ad --- /dev/null +++ b/defects/zebra-0001/patch/zebra-0001-zip317-dep-check-hashset.patch @@ -0,0 +1,121 @@ +# UNDF: UNDF-PENDING +--- a/zebra-rpc/src/methods/types/get_block_template/zip317.rs ++++ b/zebra-rpc/src/methods/types/get_block_template/zip317.rs +@@ -91,9 +91,11 @@ pub fn select_mempool_transactions( + // Setup the transaction lists. + let (mut conventional_fee_txs, mut low_fee_txs): (Vec<_>, Vec<_>) = independent_mempool_txs + .into_values() + .partition(VerifiedUnminedTx::pays_conventional_fee); + + let mut selected_txs = Vec::new(); ++ // HashSet of selected tx IDs for O(1) dependency satisfaction checks. ++ let mut selected_tx_ids: HashSet = HashSet::new(); + + // Set up limit tracking + let mut remaining_block_bytes: usize = MAX_BLOCK_BYTES.try_into().expect("fits in memory"); +@@ -108,7 +110,7 @@ pub fn select_mempool_transactions( + while let Some(tx_weights) = conventional_fee_tx_weights { + conventional_fee_tx_weights = checked_add_transaction_weighted_random( + &mut conventional_fee_txs, + &mut dependent_mempool_txs, + tx_weights, + &mut selected_txs, ++ &mut selected_tx_ids, + &mempool_tx_deps, + &mut remaining_block_bytes, + &mut remaining_block_sigops, +@@ -122,7 +124,7 @@ pub fn select_mempool_transactions( + while let Some(tx_weights) = low_fee_tx_weights { + low_fee_tx_weights = checked_add_transaction_weighted_random( + &mut low_fee_txs, + &mut dependent_mempool_txs, + tx_weights, + &mut selected_txs, ++ &mut selected_tx_ids, + &mempool_tx_deps, + &mut remaining_block_bytes, + &mut remaining_block_sigops, +@@ -210,20 +212,16 @@ fn dependencies_depth( + /// Checks if every item in `candidate_tx_deps` is present in `selected_txs`. + /// +-/// Requires items in `selected_txs` to be unique to work correctly. +-fn has_direct_dependencies( ++/// Uses a pre-built `selected_tx_ids` HashSet for O(D) membership checks ++/// instead of scanning the entire Vec in O(S) per call. ++fn has_direct_dependencies( + candidate_tx_deps: Option<&HashSet>, +- selected_txs: &Vec, ++ selected_tx_ids: &HashSet, + ) -> bool { + let Some(deps) = candidate_tx_deps else { + return true; + }; + +- if selected_txs.len() < deps.len() { +- return false; +- } +- +- let mut num_available_deps = 0; +- for tx in selected_txs { +- #[cfg(test)] +- let (_, tx) = tx; +- if deps.contains(&tx.transaction.id.mined_id()) { +- num_available_deps += 1; +- } else { +- continue; +- } +- +- if num_available_deps == deps.len() { +- return true; +- } +- } +- +- false ++ // O(D) where D = number of direct dependencies (typically 1-3). ++ // Previously O(S) where S = number of already-selected transactions. ++ deps.iter().all(|dep_id| selected_tx_ids.contains(dep_id)) + } + + /// Chooses a random transaction from `txs` using the weighted index `tx_weights`, +@@ -275,6 +273,7 @@ fn checked_add_transaction_weighted_random( + candidate_txs: &mut Vec, + dependent_txs: &mut HashMap, + tx_weights: WeightedIndex, + selected_txs: &mut Vec, ++ selected_tx_ids: &mut HashSet, + mempool_tx_deps: &TransactionDependencies, + remaining_block_bytes: &mut usize, + remaining_block_sigops: &mut u32, +@@ -298,11 +297,14 @@ fn checked_add_transaction_weighted_random( + "all candidate transactions should be independent" + ); + ++ selected_tx_ids.insert(*selected_tx_id); ++ + #[cfg(not(test))] + selected_txs.push(candidate_tx); + + #[cfg(test)] + selected_txs.push((0, candidate_tx)); + + // Try adding any dependent transactions if all of their dependencies have been selected. + + let mut current_level_dependents = mempool_tx_deps.direct_dependents(selected_tx_id); +@@ -314,7 +316,7 @@ fn checked_add_transaction_weighted_random( + // the selected txs, which come from the mempool. If the tx also spends in-chain outputs, it won't + // be added. This behavior is not specified by consensus rules and can be changed at any time, + // meaning that such txs could be added. +- if has_direct_dependencies(tx_dependencies.get(dependent_tx_id), selected_txs) { ++ if has_direct_dependencies(tx_dependencies.get(dependent_tx_id), selected_tx_ids) { + let Some(candidate_tx) = dependent_txs.remove(dependent_tx_id) else { + continue; + }; +@@ -337,6 +339,8 @@ fn checked_add_transaction_weighted_random( + continue; + } + ++ selected_tx_ids.insert(*dependent_tx_id); ++ + #[cfg(not(test))] + selected_txs.push(candidate_tx); + diff --git a/defects/zebra-0001/test/test_zip317_dep_check.py b/defects/zebra-0001/test/test_zip317_dep_check.py new file mode 100644 index 000000000..f64895c40 --- /dev/null +++ b/defects/zebra-0001/test/test_zip317_dep_check.py @@ -0,0 +1,93 @@ +""" +zebra-0001: ZIP-317 has_direct_dependencies O(T*S) -> O(T*D) benchmark. + +Simulates the dependency satisfaction check in block template construction. +Measures the cost of the Vec-scan approach vs the HashSet approach. +""" + +import time +import random +import hashlib + +def make_tx_id(n): + return hashlib.sha256(str(n).encode()).digest() + +def simulate_vec_scan(tx_chain_len): + """Simulate the buggy Vec scan approach: O(S) per dep check.""" + selected_txs = [] # list of tx_ids (simulating Vec) + ops = 0 + + for i in range(tx_chain_len): + tx_id = make_tx_id(i) + if i > 0: + # has_direct_dependencies: iterate ALL selected_txs to find 1 dep + dep_id = make_tx_id(i - 1) + for sel in selected_txs: # O(S) + ops += 1 + if sel == dep_id: + break + selected_txs.append(tx_id) + + return ops + +def simulate_hashset_check(tx_chain_len): + """Simulate the fixed HashSet approach: O(D) per dep check.""" + selected_tx_ids = set() # HashSet + ops = 0 + + for i in range(tx_chain_len): + tx_id = make_tx_id(i) + if i > 0: + # has_direct_dependencies: check each dep against HashSet - O(1) + dep_id = make_tx_id(i - 1) + ops += 1 # single O(1) set lookup + _ = dep_id in selected_tx_ids + selected_tx_ids.add(tx_id) + + return ops + +def benchmark(tx_chain_len, label): + t0 = time.perf_counter() + vec_ops = simulate_vec_scan(tx_chain_len) + t1 = time.perf_counter() + set_ops = simulate_hashset_check(tx_chain_len) + t2 = time.perf_counter() + + vec_time = t1 - t0 + set_time = t2 - t1 + ratio = vec_ops / max(set_ops, 1) + + print(f"T={tx_chain_len:6d} ({label}): " + f"vec_ops={vec_ops:9,d} set_ops={set_ops:6,d} " + f"ratio={ratio:.1f}x " + f"vec_time={vec_time*1000:.2f}ms set_time={set_time*1000:.2f}ms") + return ratio + +def test_zip317_dep_check(): + print("\nzebra-0001: ZIP-317 dep satisfaction check benchmark") + print("=" * 70) + + ratios = [] + for t in [10, 50, 100, 500, 1000]: + r = benchmark(t, "chain deps") + ratios.append((t, r)) + + print() + print("Expected O(T²) for Vec scan vs O(T) for HashSet check.") + print() + + # Assert ratios scale with T (roughly T/2 ratio in chain topology) + t10, r10 = ratios[0] + t1000, r1000 = ratios[-1] + + assert r1000 > r10 * 5, ( + f"Ratio should grow significantly with T: " + f"r@T={t10} = {r10:.1f}x, r@T={t1000} = {r1000:.1f}x" + ) + assert r1000 > 100, ( + f"Expected >100x op-count ratio at T=1000, got {r1000:.1f}x" + ) + print(f"PASS: op-count ratio at T=1000 is {r1000:.1f}x (expected >100x)") + +if __name__ == "__main__": + test_zip317_dep_check()