fix(consensus): Verify consensus branch ID in SIGHASH precomputation (#…

…9139) * Add `has_foo` fns to `Transaction` * Add V5 SIGHASH test based on consensus branch ID * Guard `skip_checks` by test features * Enable `proptest-impl` for `zebrad` in tests * Simplify conditional compilation * Enable `proptest-impl` in scanner's dev deps * Fix conditional compilation in `zebra-chain` tests * Add error types for `zebra-chain` * `impl TryFrom<u32> for NetworkUpgrade` * `impl TryFrom<ConsensusBranchId> for BranchId` * Rm `fn from_branch_id() -> Option<NetworkUpgrade>` * Check consensus branch ID in SIGHASH computation * Simplify tx deserialization * Rm `impl TryFrom<&Trans> for zp_tx::Trans` * Update tests * Update tests * Add docs for `to_librustzcash` * Update docs for `PrecomputedTxData::new` * Document the SIGHASH consensus rules we missed * Update docs for script validation * Fix script verification tests In a previous commit, I erroneously edited the tests so that they'd expect `Ok`s instead of `Err`s. This commit fixes that. * Fix spelling * Impl `NetworkUpgrade::iter()` * Refactor `Network_upgrade::next_upgrade` * Impl `NetworkUpgrade::previous_upgrade` * Impl `Transaction::hash_shielded_data` * Don't make `NETWORK_UPGRADES_IN_ORDER` `pub` * Test `Transaction::sighash` with cons branch ids * Extend the `consensus_branch_id` test * Derive `Debug` for `SigHasher` * Remove the beautiful test for tx verifier * Remove the "skip check" functionality * Revert the compilation adjustments * Apply suggestions from code review Co-authored-by: Arya <[email protected]> * Fix docs * Clarify panic conditions in docs * remove duplicated verification Co-authored-by: Arya <[email protected]> --------- Co-authored-by: Arya <[email protected]> Co-authored-by: Alfredo Garcia <[email protected]>
ZcashFoundation · Feb 1, 2025 · fcf5565 · fcf5565
1 parent 09538d4
commit fcf5565
Show file tree

Hide file tree

Showing 30 changed files with 354 additions and 301 deletions.
diff --git a/book/src/dev/rfcs/0003-inventory-tracking.md b/book/src/dev/rfcs/0003-inventory-tracking.md
@@ -192,7 +192,7 @@ specific inventory request is ready, because until we get the request, we
 can't determine which peers might be required to process it.
 
 We could attempt to ensure that the peer set would be ready to process a
-specific inventory request would be to pre-emptively "reserve" a peer as soon
+specific inventory request would be to preemptively "reserve" a peer as soon
 as it advertises an inventory item. But this doesn't actually work to ensure
 readiness, because a peer could advertise two inventory items, and only be
 able to service one request at a time. It also potentially locks the peer

diff --git a/book/src/dev/rfcs/0006-contextual-difficulty.md b/book/src/dev/rfcs/0006-contextual-difficulty.md
@@ -753,10 +753,10 @@ would be a security issue.
 # Future possibilities
 [future-possibilities]: #future-possibilities
 
-## Re-using the relevant chain API in other contextual checks
+## Reusing the relevant chain API in other contextual checks
 [relevant-chain-api-reuse]: #relevant-chain-api-reuse
 
-The relevant chain iterator can be re-used to implement other contextual
+The relevant chain iterator can be reused to implement other contextual
 validation checks.
 
 For example, responding to peer requests for block locators, which means

diff --git a/book/src/dev/rfcs/drafts/0005-treestate.md b/book/src/dev/rfcs/drafts/0005-treestate.md
@@ -126,7 +126,7 @@ finished validating everything that can be validated without the context of
 their anchor's finalization state.
 
 So for each transaction, for both `Spend` descriptions and `JoinSplit`s, we can
-pre-emptively try to do our consensus check by looking up the anchors in our
+preemptively try to do our consensus check by looking up the anchors in our
 finalized set first. For `Spend`s, we then trigger the remaining validation and
 when that finishes we are full done with those. For `JoinSplit`s, the anchor
 state check may pass early if it's a previous block Sprout `NoteCommitment` tree

diff --git a/zebra-chain/src/error.rs b/zebra-chain/src/error.rs
@@ -1,6 +1,10 @@
 //! Errors that can occur inside any `zebra-chain` submodule.
+
+use std::io;
 use thiserror::Error;
 
+// TODO: Move all these enums into a common enum at the bottom.
+
 /// Errors related to random bytes generation.
 #[derive(Error, Copy, Clone, Debug, PartialEq, Eq)]
 pub enum RandError {
@@ -51,3 +55,19 @@ pub enum AddressError {
     #[error("Randomness did not hash into the Jubjub group for producing a new diversifier")]
     DiversifierGenerationFailure,
 }
+
+/// `zebra-chain`'s errors
+#[derive(Error, Debug)]
+pub enum Error {
+    /// Invalid consensus branch ID.
+    #[error("invalid consensus branch id")]
+    InvalidConsensusBranchId,
+
+    /// Zebra's type could not be converted to its librustzcash equivalent.
+    #[error("Zebra's type could not be converted to its librustzcash equivalent: ")]
+    Conversion(#[from] io::Error),
+
+    /// The transaction is missing a network upgrade.
+    #[error("the transaction is missing a network upgrade")]
+    MissingNetworkUpgrade,
+}
diff --git a/zebra-chain/src/lib.rs b/zebra-chain/src/lib.rs
@@ -41,6 +41,8 @@ pub mod transparent;
 pub mod value_balance;
 pub mod work;
 
+pub use error::Error;
+
 #[cfg(any(test, feature = "proptest-impl"))]
 pub use block::LedgerState;
 

diff --git a/zebra-chain/src/parameters/network/testnet.rs b/zebra-chain/src/parameters/network/testnet.rs
@@ -7,7 +7,7 @@ use crate::{
         constants::{magics, SLOW_START_INTERVAL, SLOW_START_SHIFT},
         network_upgrade::TESTNET_ACTIVATION_HEIGHTS,
         subsidy::{funding_stream_address_period, FUNDING_STREAM_RECEIVER_DENOMINATOR},
-        Network, NetworkKind, NetworkUpgrade, NETWORK_UPGRADES_IN_ORDER,
+        Network, NetworkKind, NetworkUpgrade,
     },
     work::difficulty::{ExpandedDifficulty, U256},
 };
@@ -369,7 +369,7 @@ impl ParametersBuilder {
 
         // Check that the provided network upgrade activation heights are in the same order by height as the default testnet activation heights
         let mut activation_heights_iter = activation_heights.iter();
-        for expected_network_upgrade in NETWORK_UPGRADES_IN_ORDER {
+        for expected_network_upgrade in NetworkUpgrade::iter() {
             if !network_upgrades.contains(&expected_network_upgrade) {
                 continue;
             } else if let Some((&height, &network_upgrade)) = activation_heights_iter.next() {
@@ -381,7 +381,7 @@ impl ParametersBuilder {
 
                 assert!(
                     network_upgrade == expected_network_upgrade,
-                    "network upgrades must be activated in order, the correct order is {NETWORK_UPGRADES_IN_ORDER:?}"
+                    "network upgrades must be activated in order specified by the protocol"
                 );
             }
         }

diff --git a/zebra-chain/src/parameters/network/tests/vectors.rs b/zebra-chain/src/parameters/network/tests/vectors.rs
@@ -15,8 +15,7 @@ use crate::{
             self, ConfiguredActivationHeights, ConfiguredFundingStreamRecipient,
             ConfiguredFundingStreams, MAX_NETWORK_NAME_LENGTH, RESERVED_NETWORK_NAMES,
         },
-        Network, NetworkUpgrade, MAINNET_ACTIVATION_HEIGHTS, NETWORK_UPGRADES_IN_ORDER,
-        TESTNET_ACTIVATION_HEIGHTS,
+        Network, NetworkUpgrade, MAINNET_ACTIVATION_HEIGHTS, TESTNET_ACTIVATION_HEIGHTS,
     },
 };
 
@@ -124,7 +123,7 @@ fn activates_network_upgrades_correctly() {
         "activation height for all networks after Genesis and BeforeOverwinter should match NU5 activation height"
     );
 
-    for nu in NETWORK_UPGRADES_IN_ORDER.into_iter().skip(1) {
+    for nu in NetworkUpgrade::iter().skip(1) {
         let activation_height = nu
             .activation_height(&network)
             .expect("must return an activation height");
@@ -286,8 +285,8 @@ fn check_full_activation_list() {
         })
         .to_network();
 
-    // We expect the first 8 network upgrades to be included, up to NU5
-    let expected_network_upgrades = &NETWORK_UPGRADES_IN_ORDER[..8];
+    // We expect the first 8 network upgrades to be included, up to and including NU5
+    let expected_network_upgrades = NetworkUpgrade::iter().take(8);
     let full_activation_list_network_upgrades: Vec<_> = network
         .full_activation_list()
         .into_iter()
@@ -296,7 +295,7 @@ fn check_full_activation_list() {
 
     for expected_network_upgrade in expected_network_upgrades {
         assert!(
-            full_activation_list_network_upgrades.contains(expected_network_upgrade),
+            full_activation_list_network_upgrades.contains(&expected_network_upgrade),
             "full activation list should contain expected network upgrade"
         );
     }

diff --git a/zebra-chain/src/parameters/network_upgrade.rs b/zebra-chain/src/parameters/network_upgrade.rs
@@ -15,7 +15,7 @@ use hex::{FromHex, ToHex};
 use proptest_derive::Arbitrary;
 
 /// A list of network upgrades in the order that they must be activated.
-pub const NETWORK_UPGRADES_IN_ORDER: [NetworkUpgrade; 9] = [
+const NETWORK_UPGRADES_IN_ORDER: [NetworkUpgrade; 9] = [
     Genesis,
     BeforeOverwinter,
     Overwinter,
@@ -63,6 +63,18 @@ pub enum NetworkUpgrade {
     Nu6,
 }
 
+impl TryFrom<u32> for NetworkUpgrade {
+    type Error = crate::Error;
+
+    fn try_from(branch_id: u32) -> Result<Self, Self::Error> {
+        CONSENSUS_BRANCH_IDS
+            .iter()
+            .find(|id| id.1 == ConsensusBranchId(branch_id))
+            .map(|nu| nu.0)
+            .ok_or(Self::Error::InvalidConsensusBranchId)
+    }
+}
+
 impl fmt::Display for NetworkUpgrade {
     fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
         // Same as the debug representation for now
@@ -204,6 +216,15 @@ impl fmt::Display for ConsensusBranchId {
     }
 }
 
+impl TryFrom<ConsensusBranchId> for zcash_primitives::consensus::BranchId {
+    type Error = crate::Error;
+
+    fn try_from(id: ConsensusBranchId) -> Result<Self, Self::Error> {
+        zcash_primitives::consensus::BranchId::try_from(u32::from(id))
+            .map_err(|_| Self::Error::InvalidConsensusBranchId)
+    }
+}
+
 /// Network Upgrade Consensus Branch Ids.
 ///
 /// Branch ids are the same for mainnet and testnet. If there is a testnet
@@ -327,19 +348,14 @@ impl NetworkUpgrade {
             .expect("every height has a current network upgrade")
     }
 
-    /// Returns the next expected network upgrade after this network upgrade
+    /// Returns the next expected network upgrade after this network upgrade.
     pub fn next_upgrade(self) -> Option<Self> {
-        match self {
-            Genesis => Some(BeforeOverwinter),
-            BeforeOverwinter => Some(Overwinter),
-            Overwinter => Some(Sapling),
-            Sapling => Some(Blossom),
-            Blossom => Some(Heartwood),
-            Heartwood => Some(Canopy),
-            Canopy => Some(Nu5),
-            Nu5 => Some(Nu6),
-            Nu6 => None,
-        }
+        Self::iter().skip_while(|&nu| self != nu).nth(1)
+    }
+
+    /// Returns the previous network upgrade before this network upgrade.
+    pub fn previous_upgrade(self) -> Option<Self> {
+        Self::iter().rev().skip_while(|&nu| self != nu).nth(1)
     }
 
     /// Returns the next network upgrade for `network` and `height`.
@@ -518,12 +534,9 @@ impl NetworkUpgrade {
         NetworkUpgrade::current(network, height).averaging_window_timespan()
     }
 
-    /// Returns the NetworkUpgrade given an u32 as ConsensusBranchId
-    pub fn from_branch_id(branch_id: u32) -> Option<NetworkUpgrade> {
-        CONSENSUS_BRANCH_IDS
-            .iter()
-            .find(|id| id.1 == ConsensusBranchId(branch_id))
-            .map(|nu| nu.0)
+    /// Returns an iterator over [`NetworkUpgrade`] variants.
+    pub fn iter() -> impl DoubleEndedIterator<Item = NetworkUpgrade> {
+        NETWORK_UPGRADES_IN_ORDER.into_iter()
     }
 }
 

diff --git a/zebra-chain/src/primitives/zcash_note_encryption.rs b/zebra-chain/src/primitives/zcash_note_encryption.rs
@@ -4,38 +4,29 @@
 use crate::{
     block::Height,
     parameters::{Network, NetworkUpgrade},
-    primitives::zcash_primitives::convert_tx_to_librustzcash,
     transaction::Transaction,
 };
 
 /// Returns true if all Sapling or Orchard outputs, if any, decrypt successfully with
 /// an all-zeroes outgoing viewing key.
-///
-/// # Panics
-///
-/// If passed a network/height without matching consensus branch ID (pre-Overwinter),
-/// since `librustzcash` won't be able to parse it.
-pub fn decrypts_successfully(transaction: &Transaction, network: &Network, height: Height) -> bool {
-    let network_upgrade = NetworkUpgrade::current(network, height);
-    let alt_tx = convert_tx_to_librustzcash(
-        transaction,
-        network_upgrade
-            .branch_id()
-            .expect("should have a branch ID"),
-    )
-    .expect("zcash_primitives and Zebra transaction formats must be compatible");
+pub fn decrypts_successfully(tx: &Transaction, network: &Network, height: Height) -> bool {
+    let nu = NetworkUpgrade::current(network, height);
+
+    let Ok(tx) = tx.to_librustzcash(nu) else {
+        return false;
+    };
 
     let null_sapling_ovk = sapling_crypto::keys::OutgoingViewingKey([0u8; 32]);
 
     // Note that, since this function is used to validate coinbase transactions, we can ignore
     // the "grace period" mentioned in ZIP-212.
-    let zip_212_enforcement = if network_upgrade >= NetworkUpgrade::Canopy {
+    let zip_212_enforcement = if nu >= NetworkUpgrade::Canopy {
         sapling_crypto::note_encryption::Zip212Enforcement::On
     } else {
         sapling_crypto::note_encryption::Zip212Enforcement::Off
     };
 
-    if let Some(bundle) = alt_tx.sapling_bundle() {
+    if let Some(bundle) = tx.sapling_bundle() {
         for output in bundle.shielded_outputs().iter() {
             let recovery = sapling_crypto::note_encryption::try_sapling_output_recovery(
                 &null_sapling_ovk,
@@ -48,7 +39,7 @@ pub fn decrypts_successfully(transaction: &Transaction, network: &Network, heigh
         }
     }
 
-    if let Some(bundle) = alt_tx.orchard_bundle() {
+    if let Some(bundle) = tx.orchard_bundle() {
         for act in bundle.actions() {
             if zcash_note_encryption::try_output_recovery_with_ovk(
                 &orchard::note_encryption::OrchardDomain::for_action(act),