Sync zcash v5.2.0 fixes

.config/nextest.toml

@@ -140,3 +140,79 @@ default-filter = 'package(zebrad) and test(=lightwalletd_test_suite)'
 [profile.rpc-z-getsubtreesbyindex-snapshot]
 slow-timeout = { period = "30m", terminate-after = 2 }
 default-filter = 'package(zebrad) and test(=fully_synced_rpc_z_getsubtreesbyindex_snapshot_test)'
+
+# --- Acceptance test scheduling (disabled) ---
+#
+# `ci-basic.yml` skips the acceptance binary, so this is commented out rather
+# than deleted, ready to restore. `[test-groups]` and the overrides naming a
+# group must be re-enabled together: nextest rejects an unresolvable name.
+
+# Caps how many tests from a group run at once, independently of the global
+# test-threads limit. Tuned for a 4-vCPU runner; raise it on larger machines.
+# [test-groups]
+# zebrad-acceptance = { max-threads = 2 }
+# Live Mainnet tests compete for the same limited set of usable peers when they
+# connect concurrently from one public IP.
+# serial-live-mainnet = { max-threads = 1 }
+
+# Overrides are matched top to bottom and the first match wins for each
+# individual setting, so narrower rules must come before broader ones.
+
+# Tests that must not share the machine with anything else.
+#
+# `threads-required = "num-test-threads"` makes a test reserve every test slot,
+# so nextest runs it alone. Group limits alone are not enough here: they cap how
+# many tests from the same group run together, but do not stop tests outside the
+# group from running alongside.
+#
+# `trusted_chain_sync_handles_forks_correctly` restarts a node against live
+# Mainnet and expects it to reach a chain tip within 200s. It passes when run on
+# its own and fails at ~250s when other zebrad processes compete for CPU and
+# network, so it is given the machine to itself.
+# [[profile.all-tests.overrides]]
+# filter = 'test(=trusted_chain_sync_handles_forks_correctly)'
+# threads-required = "num-test-threads"
+# retries = 0
+
+# These tests sync mainnet to genesis from live peers; they normally finish in
+# ~10-30s, but a slow/unreachable peer can stall them. Fail fast after 10m and
+# retry instead of hanging until the profile-wide cap.
+# [[profile.all-tests.overrides]]
+# filter = 'test(=sync_one_checkpoint_mainnet) or test(=restart_stop_at_height)'
+# test-group = 'serial-live-mainnet'
+# slow-timeout = { period = "5m", terminate-after = 2 }
+# retries = 2
+
+# `sync_large_checkpoints_mempool_mainnet` is `#[ignore]`d, so it only runs under
+# `--run-ignored`. It enforces its own 8m deadline, so the cap here is a hang backstop.
+# [[profile.all-tests.overrides]]
+# filter = 'test(=sync_large_checkpoints_mempool_mainnet)'
+# test-group = 'serial-live-mainnet'
+# slow-timeout = { period = "10m", terminate-after = 2 }
+# retries = 3
+
+# Known-flaky acceptance tests, retried by name.
+#
+# `disconnects_from_misbehaving_peers` starts two nodes on incompatible custom
+# testnets and asserts on peer state after a fixed delay, so it fails when the
+# nodes come up in an unlucky order. It also hangs rather than exits on failure,
+# hence the tighter cap.
+# [[profile.all-tests.overrides]]
+# filter = 'test(=disconnects_from_misbehaving_peers)'
+# retries = 2
+# slow-timeout = { period = "5m", terminate-after = 2 }
+
+# `non_blocking_logger` starts a node against live Mainnet and depends on
+# reaching a usable peer; the initial peer set is often mostly unreachable.
+# [[profile.all-tests.overrides]]
+# filter = 'test(=non_blocking_logger)'
+# retries = 2
+
+# Everything in the acceptance binary shares the concurrency group.
+#
+# These tests launch real zebrad processes with their own thread pools and
+# RocksDB instances, so running several at once on a small CI runner starves
+# them and trips their internal wall-clock deadlines.
+# [[profile.all-tests.overrides]]
+# filter = 'binary(acceptance)'
+# test-group = 'zebrad-acceptance'

Cargo.toml

@@ -29,7 +29,7 @@ edition = "2021"
 # `cargo release` settings
 
 [workspace.dependencies]
-incrementalmerkletree = { version = "0.8.1", features = ["legacy-api"] }
+incrementalmerkletree = { version = "0.8.2", features = ["legacy-api"] }
 # TODO: Remove the `temporary-zebra` feature once upstream `orchard` exposes the APIs
 # Zebra needs without this transition feature.
 orchard = { version = "0.14", features = ["zsa-issuance", "temporary-zebra"] }

deny.toml

@@ -206,9 +206,11 @@ allow-registry = ["https://github.com/rust-lang/crates.io-index"]
 # List of URLs for allowed Git repositories
 # TODO: Remove the URLs once the patch table in Cargo.toml is removed.
 allow-git = [
-    "https://github.com/zcash/librustzcash.git",
+    "https://github.com/zcash/sinsemilla",
+    "https://github.com/zcash/zcash_note_encryption",
 
     # TODO: remove these QED-it fork entries once the required changes are merged upstream.
+    "https://github.com/QED-it/halo2",
     "https://github.com/QED-it/librustzcash",
     "https://github.com/QED-it/orchard",
     "https://github.com/QED-it/sapling-crypto",

zebra-chain/src/transaction/serialize.rs

@@ -1341,11 +1341,11 @@ impl ZcashDeserialize for Transaction {
                 // Convert it to a NetworkUpgrade
                 let network_upgrade =
                     NetworkUpgrade::try_from(limited_reader.read_u32::<LittleEndian>()?)?;
-                // V6 transactions are only valid from NU5 onward, so reject
-                // transactions with pre-NU5 consensus branch IDs.
-                if network_upgrade < NetworkUpgrade::Nu5 {
+                // V6 transactions are only valid from NU7 onward, so reject
+                // transactions with pre-NU7 consensus branch IDs.
+                if network_upgrade < NetworkUpgrade::Nu7 {
                     return Err(SerializationError::Parse(
-                        "v6 transaction must have NU5 or later consensus branch ID",
+                        "v6 transaction must have NU7 or later consensus branch ID",
                     ));
                 }
                 // Denoted as `lock_time` in the spec.

zebra-chain/src/transaction/unmined/zip317/zsa.rs

@@ -6,14 +6,17 @@
 //! [ZIP-227]: https://zips.z.cash/zip-0227.html
 //! [ZIP-317]: https://zips.z.cash/zip-0317#fee-calculation
 
+use std::collections::HashSet;
+
 use zcash_primitives::transaction::fees::zip317::CREATION_COST;
 
 use crate::transaction::Transaction;
 
 /// Returns the number of ZIP-317 `logical_actions` contributed by ZIP-227 issuance in
 /// `transaction`: `nIssueNotes + CREATION_COST * nReferenceNotes`.
 ///
-/// `nReferenceNotes` is the number of Issue Actions whose first note is a [reference note].
+/// `nReferenceNotes` is the number of distinct Assets whose Issue Action in this transaction
+/// starts with a [reference note].
 /// It is used in place of `nAssetCreations`, which the current ZIP-227 defines in terms of the
 /// Global Issuance State and which therefore cannot be computed from the transaction alone:
 ///
@@ -32,8 +35,9 @@ use crate::transaction::Transaction;
 ///
 /// A reference note is required on the first issuance of an Asset and optional on subsequent
 /// issuance, so `nReferenceNotes >= nAssetCreations` and the fee can never be underpaid. It is
-/// higher only if an issuer voluntarily places a reference note first in a re-issuance action,
-/// which has no practical benefit and only raises their own fee.
+/// higher only if an issuer voluntarily places a reference note first when re-issuing an Asset,
+/// which has no practical benefit and only raises their own fee. Counting distinct Assets rather
+/// than Issue Actions keeps that overcount as small as the transaction data allows.
 ///
 /// [reference note]: https://zips.z.cash/zip-0227.html#reference-notes
 //
@@ -50,8 +54,12 @@ pub fn logical_actions(transaction: &Transaction) -> usize {
     let n_reference_notes = issue_bundle
         .actions()
         .iter()
-        .filter(|action| action.get_reference_note().is_some())
-        .count();
+        .filter_map(|action| action.get_reference_note())
+        // An Asset is created at most once, so count each Asset once however many Issue
+        // Actions in this transaction carry a reference note for it.
+        .map(|note| note.asset().to_bytes())
+        .collect::<HashSet<_>>()
+        .len();
 
     n_issue_notes + (CREATION_COST * n_reference_notes)
 }

zebra-consensus/src/primitives/halo2.rs

@@ -259,7 +259,11 @@ pub static VERIFIER_POST_NU6_2: Lazy<VerifierService> =
 #[cfg(all(zcash_unstable = "nu7", feature = "tx_v6"))]
 pub static VERIFIER_ZSA: Lazy<VerifierService> = Lazy::new(|| batch_verifier(&VERIFYING_KEY_ZSA));
 
-/// Returns the global Halo2 verifier for Orchard bundles in blocks at `network_upgrade`.
+/// Returns the global Halo2 verifier for `OrchardVanilla` bundles in blocks at `network_upgrade`.
+///
+/// Era and flavor are independent axes; this covers only the era. [`verifier_for_bundle`] composes
+/// flavor on top, sending `OrchardZSA` bundles to [`VERIFIER_ZSA`] instead. NU7 carries both
+/// flavors, because V5 transactions stay valid at NU7, so the upgrade alone cannot pick the key.
 ///
 /// The Orchard Action circuit — and therefore its verifying key — changed at NU6.2 (the fixed
 /// variable-base scalar-multiplication circuit; see GHSA-jfw5-j458-pfv6), and a proof produced
@@ -285,15 +289,7 @@ pub fn verifier_for(network_upgrade: NetworkUpgrade) -> &'static VerifierService
 
         // NU6.2 ships the fixed circuit, and every upgrade after it inherits that fixed circuit,
         // so all of them verify under the fixed key.
-        Nu6_2 => &VERIFIER_POST_NU6_2,
-
-        // Without ZSA V6 support, NU7 continues using the fixed post-NU6.2 Orchard circuit.
-        #[cfg(not(all(zcash_unstable = "nu7", feature = "tx_v6")))]
-        Nu7 => &VERIFIER_POST_NU6_2,
-
-        // With ZSA V6 support, NU7 uses the dedicated OrchardZSA circuit and verifying key.
-        #[cfg(all(zcash_unstable = "nu7", feature = "tx_v6"))]
-        Nu7 => &VERIFIER_ZSA,
+        Nu6_2 | Nu7 => &VERIFIER_POST_NU6_2,
 
         // `ZFuture` only exists under the `zcash_unstable = "zfuture"` cfg. It is a post-NU6.2
         // upgrade, so it inherits the fixed circuit and is bound to the fixed key here on purpose
@@ -304,6 +300,23 @@ pub fn verifier_for(network_upgrade: NetworkUpgrade) -> &'static VerifierService
     }
 }
 
+/// Returns the global Halo2 verifier holding the key for `bundle`'s circuit, in a block at
+/// `network_upgrade`.
+///
+/// This composes the two axes: flavor picks the circuit family, and for vanilla bundles
+/// [`verifier_for`] then picks the era. NU7 needs both, because a V5 transaction there carries a
+/// vanilla bundle while a V6 one carries a ZSA bundle.
+pub fn verifier_for_bundle(
+    bundle: &OrchardBundle<orchard::bundle::Authorized>,
+    network_upgrade: NetworkUpgrade,
+) -> &'static VerifierService {
+    match bundle {
+        OrchardBundle::OrchardVanilla(_) => verifier_for(network_upgrade),
+        #[cfg(all(zcash_unstable = "nu7", feature = "tx_v6"))]
+        OrchardBundle::OrchardZSA(_) => &VERIFIER_ZSA,
+    }
+}
+
 /// Halo2 proof verifier implementation
 ///
 /// This is the core implementation for the batch verification logic of the

zebra-consensus/src/primitives/halo2/tests.rs

@@ -10,7 +10,9 @@
 //!   * the same proof is **rejected** by the post-NU6.2 (fixed) key, so the verifier is not
 //!     "fail-open" — it does not accept whatever it is handed regardless of era; and
 //!   * [`verifier_for`] routes each network upgrade to the service holding the matching key,
-//!     with NU6.2 and every later upgrade going to the fixed-key verifier.
+//!     with NU6.2 and every later upgrade going to the fixed-key verifier, and never to the ZSA
+//!     verifier — flavor is composed on top by [`verifier_for_bundle`], which must send vanilla
+//!     bundles to the era verifier even at NU7.
 
 use std::sync::Arc;
 
@@ -25,8 +27,8 @@ use zebra_chain::{
 };
 
 use super::{
-    verifier_for, Item, VERIFIER_POST_NU6_2, VERIFIER_PRE_NU6_2, VERIFYING_KEY_POST_NU6_2,
-    VERIFYING_KEY_PRE_NU6_2,
+    verifier_for, verifier_for_bundle, Item, VERIFIER_POST_NU6_2, VERIFIER_PRE_NU6_2,
+    VERIFYING_KEY_POST_NU6_2, VERIFYING_KEY_PRE_NU6_2,
 };
 
 #[cfg(all(zcash_unstable = "nu7", feature = "tx_v6"))]
@@ -107,9 +109,6 @@ async fn verifier_for_routes_each_upgrade_to_the_correct_key() {
     let pre: &'static super::VerifierService = &VERIFIER_PRE_NU6_2;
     let post: &'static super::VerifierService = &VERIFIER_POST_NU6_2;
 
-    #[cfg(all(zcash_unstable = "nu7", feature = "tx_v6"))]
-    let zsa: &'static super::VerifierService = &VERIFIER_ZSA;
-
     // Everything before NU6.2 (including upgrades from before Orchard existed) routes to the
     // insecure key, which is the only key any pre-NU6.2 Orchard history verifies under.
     for nu in [
@@ -123,22 +122,67 @@ async fn verifier_for_routes_each_upgrade_to_the_correct_key() {
         );
     }
 
-    // NU6.2 and every later upgrade route to the fixed key, except NU7 in ZSA builds,
-    // which routes to the dedicated ZSA verifier. Nu7 still guards that "NU6.2 and later"
-    // does not silently fall back to the insecure verifier for future upgrades.
+    // NU6.2 and every later upgrade route to the fixed key. Nu7 is included because it carries
+    // vanilla bundles too, in V5 transactions.
     for nu in [NetworkUpgrade::Nu6_2, NetworkUpgrade::Nu7] {
-        #[cfg(all(zcash_unstable = "nu7", feature = "tx_v6"))]
-        if nu == NetworkUpgrade::Nu7 {
-            assert!(
-                std::ptr::eq(verifier_for(nu), zsa),
-                "{nu:?} must route to the ZSA verifier"
-            );
-            continue;
-        }
-
         assert!(
             std::ptr::eq(verifier_for(nu), post),
             "{nu:?} must route to the post-NU6.2 (fixed) verifier"
         );
     }
+
+    // The ZSA circuit is a separate axis, so `verifier_for` must never return its verifier.
+    #[cfg(all(zcash_unstable = "nu7", feature = "tx_v6"))]
+    {
+        let zsa: &'static super::VerifierService = &VERIFIER_ZSA;
+
+        assert!(
+            !std::ptr::eq(zsa, pre) && !std::ptr::eq(zsa, post),
+            "the ZSA verifier must not alias either era verifier"
+        );
+    }
+}
+
+/// A vanilla Orchard bundle at NU7 is routed by era, not to the ZSA verifier.
+///
+/// V5 transactions stay valid at NU7 and carry vanilla bundles, so routing on the upgrade alone —
+/// as the v5.2.0 merge briefly did — sends them to the OrchardZSA key, which cannot verify a
+/// vanilla proof.
+///
+/// The choice is asserted rather than the outcome, because both the right and the wrong key reject
+/// this pre-NU6.2 fixture. The ZSA direction is covered end to end by `check_orchard_zsa_workflow`.
+#[tokio::test(flavor = "multi_thread")]
+async fn vanilla_bundle_at_nu7_is_routed_by_era_not_flavor() {
+    let (bundle, _sighash) = pre_nu6_2_bundle_and_sighash();
+
+    assert!(
+        matches!(bundle, OrchardBundle::OrchardVanilla(_)),
+        "a V5 transaction's bundle must be the vanilla flavor"
+    );
+
+    assert!(
+        std::ptr::eq(
+            verifier_for_bundle(&bundle, NetworkUpgrade::Nu7),
+            &*VERIFIER_POST_NU6_2
+        ),
+        "a vanilla bundle at NU7 must route to the post-NU6.2 (fixed) verifier"
+    );
+
+    #[cfg(all(zcash_unstable = "nu7", feature = "tx_v6"))]
+    assert!(
+        !std::ptr::eq(
+            verifier_for_bundle(&bundle, NetworkUpgrade::Nu7),
+            &*VERIFIER_ZSA
+        ),
+        "a vanilla bundle must never route to the ZSA verifier"
+    );
+
+    // The era axis still applies below NU6.2, so historical blocks re-sync.
+    assert!(
+        std::ptr::eq(
+            verifier_for_bundle(&bundle, NetworkUpgrade::Nu5),
+            &*VERIFIER_PRE_NU6_2
+        ),
+        "a vanilla bundle at NU5 must route to the pre-NU6.2 (insecure) verifier"
+    );
 }

zebra-consensus/src/transaction.rs

@@ -1288,14 +1288,18 @@ where
 
     /// Verifies a transaction's Orchard shielded data.
     ///
-    /// `network_upgrade` is the network upgrade active at the verified transaction's block
-    /// height. It selects the Orchard verifier: the Orchard Action circuit (and its verifying
-    /// key) changed at NU6.2 to fix the variable-base scalar-multiplication bug
-    /// (GHSA-jfw5-j458-pfv6), so pre-NU6.2 bundles must be verified against the historical
-    /// (insecure) key and NU6.2+ bundles against the fixed key. A proof from one era does not
-    /// verify under the other era's key. [`primitives::halo2::verifier_for`] maps the upgrade to
-    /// the verifier holding the matching key; the two verifiers keep separate batches, so eras
-    /// are never mixed.
+    /// A proof only verifies under the key for the circuit that produced it, so the verifier is
+    /// selected on two independent axes:
+    ///
+    /// * flavor: `OrchardZSA` bundles use the ZSA circuit, whatever the upgrade.
+    /// * era, for `OrchardVanilla` bundles: the vanilla circuit changed at NU6.2 to fix
+    ///   GHSA-jfw5-j458-pfv6, so `network_upgrade` — the upgrade at the transaction's block
+    ///   height — selects the historical or the fixed key.
+    ///
+    /// [`primitives::halo2::verifier_for_bundle`] composes both.
+    ///
+    /// Both are needed: NU7 carries vanilla bundles (in V5 transactions, still valid at NU7) as
+    /// well as ZSA ones. Each verifier holds one key and batches separately, so circuits never mix.
     fn verify_orchard_bundle(
         bundle: Option<OrchardBundle<::orchard::bundle::Authorized>>,
         sighash: &SigHash,
@@ -1316,10 +1320,9 @@ where
             // Actions in one transaction. So we queue it for verification
             // only once instead of queuing it up for every Action description.
             //
-            // Route the bundle to the verifier for its circuit era: pre-NU6.2 bundles only
-            // verify under the insecure key, NU6.2+ bundles only under the fixed key.
+            // Route the bundle to the verifier holding the key for its circuit.
             async_checks.push(
-                primitives::halo2::verifier_for(network_upgrade)
+                primitives::halo2::verifier_for_bundle(&bundle, network_upgrade)
                     .clone()
                     .oneshot(primitives::halo2::Item::new(bundle, *sighash)),
             );