refactor: Simplify coinbase expiry code so the consensus rule is clear (

#3408) * Fix some outdated TODO comments * refactor(coinbase expiry): Simplify the code so consensus rule is clear * Fix the formatting of an error message * Remove a redundant comment Co-authored-by: Marek <mail@marek.onl> Co-authored-by: Marek <mail@marek.onl>
ZcashFoundation · Jan 26, 2022 · 8e530b0 · 8e530b0
1 parent 8d297fd
commit 8e530b0
Show file tree

Hide file tree

Showing 2 changed files with 20 additions and 45 deletions.
diff --git a/zebra-chain/src/transaction/lock_time.rs b/zebra-chain/src/transaction/lock_time.rs
@@ -67,8 +67,7 @@ impl LockTime {
     /// Users should not construct lock times with timestamps lower than the
     /// value returned by this function.
     //
-    // When `Utc.timestamp` stabilises as a const function, we can make this a
-    // const function.
+    // TODO: replace Utc.timestamp with DateTime32 (#2211)
     pub fn min_lock_time_timestamp() -> LockTime {
         LockTime::Time(Utc.timestamp(Self::MIN_TIMESTAMP, 0))
     }
@@ -78,8 +77,7 @@ impl LockTime {
     /// Users should not construct lock times with timestamps greater than the
     /// value returned by this function.
     //
-    // When `Utc.timestamp` stabilises as a const function, we can make this a
-    // const function.
+    // TODO: replace Utc.timestamp with DateTime32 (#2211)
     pub fn max_lock_time_timestamp() -> LockTime {
         LockTime::Time(Utc.timestamp(Self::MAX_TIMESTAMP, 0))
     }

diff --git a/zebra-consensus/src/transaction/check.rs b/zebra-consensus/src/transaction/check.rs
@@ -294,57 +294,34 @@ pub fn coinbase_expiry_height(
 ) -> Result<(), TransactionError> {
     let expiry_height = coinbase.expiry_height();
 
-    match NetworkUpgrade::Nu5.activation_height(network) {
-        // # Consensus
-        //
-        // > [Overwinter to Canopy inclusive, pre-NU5] nExpiryHeight MUST be less than
-        // > or equal to 499999999.
-        //
-        // <https://zips.z.cash/protocol/protocol.pdf#txnconsensus>
-        //
-        // We check this rule here when no activation height is set.
-        None => validate_expiry_height_max(expiry_height, true, block_height, coinbase),
-
+    // TODO: replace `if let` with `expect` after NU5 mainnet activation
+    if let Some(nu5_activation_height) = NetworkUpgrade::Nu5.activation_height(network) {
         // # Consensus
         //
         // > [NU5 onward] The nExpiryHeight field of a coinbase transaction
         // > MUST be equal to its block height.
         //
         // <https://zips.z.cash/protocol/protocol.pdf#txnconsensus>
-        Some(activation_height) => {
-            if *block_height >= activation_height {
-                match expiry_height {
-                    None => Err(TransactionError::CoinbaseExpiryBlockHeight {
-                        expiry_height,
-                        block_height: *block_height,
-                        transaction_hash: coinbase.hash(),
-                    })?,
-                    Some(expiry) => {
-                        if expiry != *block_height {
-                            return Err(TransactionError::CoinbaseExpiryBlockHeight {
-                                expiry_height,
-                                block_height: *block_height,
-                                transaction_hash: coinbase.hash(),
-                            })?;
-                        }
-                    }
-                }
-
+        if *block_height >= nu5_activation_height {
+            if expiry_height != Some(*block_height) {
+                return Err(TransactionError::CoinbaseExpiryBlockHeight {
+                    expiry_height,
+                    block_height: *block_height,
+                    transaction_hash: coinbase.hash(),
+                });
+            } else {
                 return Ok(());
             }
-
-            // # Consensus
-            //
-            // > [Overwinter to Canopy inclusive, pre-NU5] nExpiryHeight MUST be less than
-            // > or equal to 499999999.
-            //
-            // <https://zips.z.cash/protocol/protocol.pdf#txnconsensus>
-            //
-            // We check this rule here when the NU5 activation height is set,
-            // but we haven't reached it yet.
-            validate_expiry_height_max(expiry_height, true, block_height, coinbase)
         }
     }
+
+    // # Consensus
+    //
+    // > [Overwinter to Canopy inclusive, pre-NU5] nExpiryHeight MUST be less than
+    // > or equal to 499999999.
+    //
+    // <https://zips.z.cash/protocol/protocol.pdf#txnconsensus>
+    validate_expiry_height_max(expiry_height, true, block_height, coinbase)
 }
 
 /// Returns `Ok(())` if the expiry height for a non coinbase transaction is