fix(docs): Fix NU6-related documentation (#8949)

* replaces some potential panics with error in block verifier checks * fixes outstanding documentation issues listed as examples in audit report * fixes remaining TODOs in `parameters::network::subsidy` module. * Addresses other TODOs added during NU6 implementation, fixes a TODO in `subsidy_is_valid()` about using the network slow start interval parameters when PoW is disabled. * updates snapshot
ZcashFoundation · Oct 22, 2024 · 46c6b6e · 46c6b6e
1 parent b1ffc89
commit 46c6b6e
Show file tree

Hide file tree

Showing 9 changed files with 47 additions and 157 deletions.
diff --git a/zebra-chain/src/parameters/network/subsidy.rs b/zebra-chain/src/parameters/network/subsidy.rs
@@ -65,18 +65,18 @@ pub enum FundingStreamReceiver {
 
     /// The Major Grants (Zcash Community Grants) funding stream.
     MajorGrants,
-    /// The deferred pool contribution.
-    // TODO: Add link to lockbox stream ZIP
+
+    /// The deferred pool contribution, see [ZIP-1015](https://zips.z.cash/zip-1015) for more details.
     Deferred,
 }
 
 impl FundingStreamReceiver {
     /// Returns a human-readable name and a specification URL for the receiver, as described in
-    /// [ZIP-1014] and [`zcashd`] before NU6. After NU6, the specification is in the [ZIP-lockbox].
+    /// [ZIP-1014] and [`zcashd`] before NU6. After NU6, the specification is in the [ZIP-1015].
     ///
     /// [ZIP-1014]: https://zips.z.cash/zip-1014#abstract
     /// [`zcashd`]: https://github.com/zcash/zcash/blob/3f09cfa00a3c90336580a127e0096d99e25a38d6/src/consensus/funding.cpp#L13-L32
-    /// [ZIP-lockbox]: https://zips.z.cash/draft-nuttycom-funding-allocation#alternative-2-hybrid-deferred-dev-fund-transitioning-to-a-non-direct-funding-model
+    /// [ZIP-1015]: https://zips.z.cash/zip-1015
     pub fn info(&self, is_nu6: bool) -> (&'static str, &'static str) {
         if is_nu6 {
             (
@@ -112,10 +112,10 @@ pub const FUNDING_STREAM_RECEIVER_DENOMINATOR: u64 = 100;
 /// [ZIP-214]: https://zips.z.cash/zip-0214
 pub const FUNDING_STREAM_SPECIFICATION: &str = "https://zips.z.cash/zip-0214";
 
-/// The specification for post-NU6 funding stream and lockbox receivers, a URL that links to [ZIP-lockbox].
+/// The specification for post-NU6 funding stream and lockbox receivers, a URL that links to [ZIP-1015].
 ///
-/// [ZIP-lockbox]: https://zips.z.cash/draft-nuttycom-funding-allocation#alternative-2-hybrid-deferred-dev-fund-transitioning-to-a-non-direct-funding-model
-pub const LOCKBOX_SPECIFICATION: &str = "https://zips.z.cash/draft-nuttycom-funding-allocation#alternative-2-hybrid-deferred-dev-fund-transitioning-to-a-non-direct-funding-model";
+/// [ZIP-1015]: https://zips.z.cash/zip-1015
+pub const LOCKBOX_SPECIFICATION: &str = "https://zips.z.cash/zip-1015";
 
 /// Funding stream recipients and height ranges.
 #[derive(Deserialize, Clone, Debug, Eq, PartialEq)]
@@ -225,10 +225,8 @@ lazy_static! {
         .collect(),
     };
 
-    /// The post-NU6 funding streams for Mainnet
-    // TODO: Add a reference to lockbox stream ZIP, this is currently based on https://zips.z.cash/draft-nuttycom-funding-allocation
+    /// The post-NU6 funding streams for Mainnet as described in [ZIP-1015](https://zips.z.cash/zip-1015).
     pub static ref POST_NU6_FUNDING_STREAMS_MAINNET: FundingStreams = FundingStreams {
-        // TODO: Adjust this height range and recipient list once a proposal is selected
         height_range: POST_NU6_FUNDING_STREAM_START_RANGE_MAINNET,
         recipients: [
             (
@@ -266,11 +264,8 @@ lazy_static! {
         .collect(),
     };
 
-    /// The post-NU6 funding streams for Testnet
-    // TODO: Add a reference to lockbox stream ZIP, this is currently based on the number of blocks between the
-    //       start and end heights for Mainnet in https://zips.z.cash/draft-nuttycom-funding-allocation
+    /// The post-NU6 funding streams for Testnet as described in [ZIP-1015](https://zips.z.cash/zip-1015).
     pub static ref POST_NU6_FUNDING_STREAMS_TESTNET: FundingStreams = FundingStreams {
-        // TODO: Adjust this height range and recipient list once a proposal is selected
         height_range: POST_NU6_FUNDING_STREAM_START_RANGE_TESTNET,
         recipients: [
             (
@@ -279,7 +274,6 @@ lazy_static! {
             ),
             (
                 FundingStreamReceiver::MajorGrants,
-                // TODO: Update these addresses
                 FundingStreamRecipient::new(8, POST_NU6_FUNDING_STREAM_FPF_ADDRESSES_TESTNET),
             ),
         ]
@@ -288,15 +282,14 @@ lazy_static! {
     };
 }
 
-/// The start height of post-NU6 funding streams on Mainnet
-// TODO: Add a reference to lockbox stream ZIP, this is currently based on https://zips.z.cash/draft-nuttycom-funding-allocation
+/// The start height of post-NU6 funding streams on Mainnet as described in [ZIP-1015](https://zips.z.cash/zip-1015).
 const POST_NU6_FUNDING_STREAM_START_HEIGHT_MAINNET: u32 = 2_726_400;
 
-/// The start height of post-NU6 funding streams on Testnet
-// TODO: Add a reference to lockbox stream ZIP, this is currently based on https://zips.z.cash/draft-nuttycom-funding-allocation
+/// The start height of post-NU6 funding streams on Testnet as described in [ZIP-1015](https://zips.z.cash/zip-1015).
 const POST_NU6_FUNDING_STREAM_START_HEIGHT_TESTNET: u32 = 2_976_000;
 
-/// The number of blocks contained in the post-NU6 funding streams height ranges on Mainnet or Testnet.
+/// The number of blocks contained in the post-NU6 funding streams height ranges on Mainnet or Testnet, as specified
+/// in [ZIP-1015](https://zips.z.cash/zip-1015).
 const POST_NU6_FUNDING_STREAM_NUM_BLOCKS: u32 = 420_000;
 
 /// The post-NU6 funding stream height range on Mainnet

diff --git a/zebra-consensus/src/block.rs b/zebra-consensus/src/block.rs
@@ -284,7 +284,7 @@ where
                 })?;
             }
 
-            // TODO: Add link to lockbox stream ZIP
+            // See [ZIP-1015](https://zips.z.cash/zip-1015).
             let expected_deferred_amount = subsidy::funding_streams::funding_stream_values(
                 height,
                 &network,

diff --git a/zebra-consensus/src/block/check.rs b/zebra-consensus/src/block/check.rs
@@ -162,13 +162,7 @@ pub fn subsidy_is_valid(
         .activation_height(network)
         .expect("Canopy activation height is known");
 
-    // TODO: Add this as a field on `testnet::Parameters` instead of checking `disable_pow()`, this is 0 for Regtest in zcashd,
-    //       see <https://github.com/zcash/zcash/blob/master/src/chainparams.cpp#L640>
-    let slow_start_interval = if network.disable_pow() {
-        Height(0)
-    } else {
-        network.slow_start_interval()
-    };
+    let slow_start_interval = network.slow_start_interval();
 
     if height < slow_start_interval {
         unreachable!(
@@ -191,7 +185,8 @@ pub fn subsidy_is_valid(
             network,
             expected_block_subsidy,
         )
-        .expect("We always expect a funding stream hashmap response even if empty");
+        // we always expect a funding stream hashmap response even if empty
+        .map_err(|err| BlockError::Other(err.to_string()))?;
 
         // # Consensus
         //
@@ -204,14 +199,18 @@ pub fn subsidy_is_valid(
         for (receiver, expected_amount) in funding_streams {
             if receiver == FundingStreamReceiver::Deferred {
                 // The deferred pool contribution is checked in `miner_fees_are_valid()`
-                // TODO: Add link to lockbox stream ZIP
+                // See [ZIP-1015](https://zips.z.cash/zip-1015) for more details.
                 continue;
             }
 
-            let address = subsidy::funding_streams::funding_stream_address(
-                height, network, receiver,
-            )
-            .expect("funding stream receivers other than the deferred pool must have an address");
+            let address =
+                subsidy::funding_streams::funding_stream_address(height, network, receiver)
+                    // funding stream receivers other than the deferred pool must have an address
+                    .ok_or_else(|| {
+                        BlockError::Other(format!(
+                            "missing funding stream address at height {height:?}"
+                        ))
+                    })?;
 
             let has_expected_output =
                 subsidy::funding_streams::filter_outputs_by_address(coinbase, address)
@@ -250,35 +249,33 @@ pub fn miner_fees_are_valid(
     let sapling_value_balance = coinbase_tx.sapling_value_balance().sapling_amount();
     let orchard_value_balance = coinbase_tx.orchard_value_balance().orchard_amount();
 
-    // TODO: Update the quote below once its been updated for NU6.
-    //
     // # Consensus
     //
-    // > The total value in zatoshi of transparent outputs from a coinbase transaction,
-    // > minus vbalanceSapling, minus vbalanceOrchard, MUST NOT be greater than the value
-    // > in zatoshi of block subsidy plus the transaction fees paid by transactions in this block.
+    // > - define the total output value of its coinbase transaction to be the total value in zatoshi of its transparent
+    // >   outputs, minus vbalanceSapling, minus vbalanceOrchard, plus totalDeferredOutput(height);
+    // > – define the total input value of its coinbase transaction to be the value in zatoshi of the block subsidy,
+    // >   plus the transaction fees paid by transactions in the block.
     //
     // https://zips.z.cash/protocol/protocol.pdf#txnconsensus
     //
     // The expected lockbox funding stream output of the coinbase transaction is also subtracted
     // from the block subsidy value plus the transaction fees paid by transactions in this block.
-    let left = (transparent_value_balance - sapling_value_balance - orchard_value_balance)
-        .map_err(|_| SubsidyError::SumOverflow)?;
-    let right = (expected_block_subsidy + block_miner_fees - expected_deferred_amount)
-        .map_err(|_| SubsidyError::SumOverflow)?;
+    let total_output_value = (transparent_value_balance - sapling_value_balance - orchard_value_balance
+        + expected_deferred_amount.constrain().expect("valid Amount with NonNegative constraint should be valid with NegativeAllowed constraint"))
+    .map_err(|_| SubsidyError::SumOverflow)?;
+    let total_input_value =
+        (expected_block_subsidy + block_miner_fees).map_err(|_| SubsidyError::SumOverflow)?;
 
-    // TODO: Updadte the quotes below if the final phrasing changes in the spec for NU6.
-    //
     // # Consensus
     //
     // > [Pre-NU6] The total output of a coinbase transaction MUST NOT be greater than its total
     // input.
     //
     // > [NU6 onward] The total output of a coinbase transaction MUST be equal to its total input.
     if if NetworkUpgrade::current(network, height) < NetworkUpgrade::Nu6 {
-        left > right
+        total_output_value > total_input_value
     } else {
-        left != right
+        total_output_value != total_input_value
     } {
         Err(SubsidyError::InvalidMinerFees)?
     };

diff --git a/zebra-consensus/src/block/subsidy/funding_streams.rs b/zebra-consensus/src/block/subsidy/funding_streams.rs
@@ -43,6 +43,7 @@ pub fn funding_stream_values(
             }
         }
     }
+
     Ok(results)
 }
 

diff --git a/zebra-consensus/src/block/subsidy/general.rs b/zebra-consensus/src/block/subsidy/general.rs
@@ -120,47 +120,10 @@ pub fn output_amounts(transaction: &Transaction) -> HashSet<Amount<NonNegative>>
         .collect()
 }
 
-/// Lockbox funding stream total input value for a block height.
-///
-/// Assumes a constant funding stream amount per block.
-// TODO: Cache the lockbox value balance in zebra-state (will be required for tracking lockbox
-//       value balance after the Zcash Sustainability Fund ZIPs or after a ZIP for spending from the deferred pool)
-#[allow(dead_code)]
-fn lockbox_input_value(network: &Network, height: Height) -> Amount<NonNegative> {
-    let Some(nu6_activation_height) = Nu6.activation_height(network) else {
-        return Amount::zero();
-    };
-
-    let expected_block_subsidy = block_subsidy(nu6_activation_height, network)
-        .expect("block at NU6 activation height must have valid expected subsidy");
-    let &deferred_amount_per_block =
-        funding_stream_values(nu6_activation_height, network, expected_block_subsidy)
-            .expect("we always expect a funding stream hashmap response even if empty")
-            .get(&FundingStreamReceiver::Deferred)
-            .expect("we expect a lockbox funding stream after NU5");
-
-    let post_nu6_funding_stream_height_range = network.post_nu6_funding_streams().height_range();
-
-    // `min(height, last_height_with_deferred_pool_contribution) - (nu6_activation_height - 1)`,
-    // We decrement NU6 activation height since it's an inclusive lower bound.
-    // Funding stream height range end bound is not incremented since it's an exclusive end bound
-    let num_blocks_with_lockbox_output = (height.0 + 1)
-        .min(post_nu6_funding_stream_height_range.end.0)
-        .checked_sub(post_nu6_funding_stream_height_range.start.0)
-        .unwrap_or_default();
-
-    (deferred_amount_per_block * num_blocks_with_lockbox_output.into())
-        .expect("lockbox input value should fit in Amount")
-}
-
 #[cfg(test)]
 mod test {
     use super::*;
     use color_eyre::Report;
-    use zebra_chain::parameters::testnet::{
-        self, ConfiguredActivationHeights, ConfiguredFundingStreamRecipient,
-        ConfiguredFundingStreams,
-    };
 
     #[test]
     fn halving_test() -> Result<(), Report> {
@@ -436,73 +399,6 @@ mod test {
         Ok(())
     }
 
-    #[test]
-    fn check_lockbox_input_value() -> Result<(), Report> {
-        let _init_guard = zebra_test::init();
-
-        let network = testnet::Parameters::build()
-            .with_activation_heights(ConfiguredActivationHeights {
-                blossom: Some(Blossom.activation_height(&Network::Mainnet).unwrap().0),
-                nu6: Some(POST_NU6_FUNDING_STREAMS_MAINNET.height_range().start.0),
-                ..Default::default()
-            })
-            .with_post_nu6_funding_streams(ConfiguredFundingStreams {
-                // Start checking funding streams from block height 1
-                height_range: Some(POST_NU6_FUNDING_STREAMS_MAINNET.height_range().clone()),
-                // Use default post-NU6 recipients
-                recipients: Some(
-                    POST_NU6_FUNDING_STREAMS_TESTNET
-                        .recipients()
-                        .iter()
-                        .map(|(&receiver, recipient)| ConfiguredFundingStreamRecipient {
-                            receiver,
-                            numerator: recipient.numerator(),
-                            addresses: Some(
-                                recipient
-                                    .addresses()
-                                    .iter()
-                                    .map(|addr| addr.to_string())
-                                    .collect(),
-                            ),
-                        })
-                        .collect(),
-                ),
-            })
-            .to_network();
-
-        let nu6_height = Nu6.activation_height(&network).unwrap();
-        let post_nu6_funding_streams = network.post_nu6_funding_streams();
-        let height_range = post_nu6_funding_streams.height_range();
-
-        let last_funding_stream_height = post_nu6_funding_streams
-            .height_range()
-            .end
-            .previous()
-            .expect("the previous height should be valid");
-
-        assert_eq!(
-            Amount::<NonNegative>::zero(),
-            lockbox_input_value(&network, Height::MIN)
-        );
-
-        let expected_lockbox_value: Amount<NonNegative> = Amount::try_from(18_750_000)?;
-        assert_eq!(
-            expected_lockbox_value,
-            lockbox_input_value(&network, nu6_height)
-        );
-
-        let num_blocks_total = height_range.end.0 - height_range.start.0;
-        let expected_input_per_block: Amount<NonNegative> = Amount::try_from(18_750_000)?;
-        let expected_lockbox_value = (expected_input_per_block * num_blocks_total.into())?;
-
-        assert_eq!(
-            expected_lockbox_value,
-            lockbox_input_value(&network, last_funding_stream_height)
-        );
-
-        Ok(())
-    }
-
     #[test]
     fn check_height_for_num_halvings() {
         for network in Network::iter() {

diff --git a/zebra-consensus/src/block/tests.rs b/zebra-consensus/src/block/tests.rs
@@ -515,7 +515,7 @@ fn miner_fees_validation_for_network(network: Network) -> Result<(), Report> {
 
             let expected_block_subsidy = block_subsidy(height, &network)?;
 
-            // TODO: Add link to lockbox stream ZIP
+            // See [ZIP-1015](https://zips.z.cash/zip-1015).
             let expected_deferred_amount = subsidy::funding_streams::funding_stream_values(
                 height,
                 &network,
@@ -549,8 +549,8 @@ fn miner_fees_validation_failure() -> Result<(), Report> {
         .expect("block should deserialize");
     let height = block.coinbase_height().expect("valid coinbase height");
     let expected_block_subsidy = block_subsidy(height, &network)?;
-    // TODO: Add link to lockbox stream ZIP
-    let expected_deferred_amount =
+    // See [ZIP-1015](https://zips.z.cash/zip-1015).
+    let expected_deferred_amount: Amount<zebra_chain::amount::NonNegative> =
         subsidy::funding_streams::funding_stream_values(height, &network, expected_block_subsidy)
             .expect("we always expect a funding stream hashmap response even if empty")
             .remove(&FundingStreamReceiver::Deferred)

diff --git a/zebra-consensus/src/checkpoint.rs b/zebra-consensus/src/checkpoint.rs
@@ -611,7 +611,7 @@ where
         // We can't get the block subsidy for blocks with heights in the slow start interval, so we
         // omit the calculation of the expected deferred amount.
         let expected_deferred_amount = if height > self.network.slow_start_interval() {
-            // TODO: Add link to lockbox stream ZIP
+            // See [ZIP-1015](https://zips.z.cash/zip-1015).
             funding_stream_values(height, &self.network, block_subsidy(height, &self.network)?)?
                 .remove(&FundingStreamReceiver::Deferred)
         } else {

diff --git a/zebra-consensus/src/error.rs b/zebra-consensus/src/error.rs
@@ -345,6 +345,9 @@ pub enum BlockError {
         hash: zebra_chain::block::Hash,
         source: amount::Error,
     },
+
+    #[error("unexpected error occurred: {0}")]
+    Other(String),
 }
 
 impl From<SubsidyError> for BlockError {

diff --git a/...c/methods/tests/snapshot/snapshots/get_block_subsidy_future_nu6_height@nu6testnet_10.snap b/...c/methods/tests/snapshot/snapshots/get_block_subsidy_future_nu6_height@nu6testnet_10.snap
@@ -6,7 +6,7 @@ expression: get_block_subsidy
   "fundingstreams": [
     {
       "recipient": "Zcash Community Grants NU6",
-      "specification": "https://zips.z.cash/draft-nuttycom-funding-allocation#alternative-2-hybrid-deferred-dev-fund-transitioning-to-a-non-direct-funding-model",
+      "specification": "https://zips.z.cash/zip-1015",
       "value": 0.125,
       "valueZat": 12500000,
       "address": "t2HifwjUj9uyxr9bknR8LFuQbc98c3vkXtu"
@@ -15,7 +15,7 @@ expression: get_block_subsidy
   "lockboxstreams": [
     {
       "recipient": "Lockbox NU6",
-      "specification": "https://zips.z.cash/draft-nuttycom-funding-allocation#alternative-2-hybrid-deferred-dev-fund-transitioning-to-a-non-direct-funding-model",
+      "specification": "https://zips.z.cash/zip-1015",
       "value": 0.1875,
       "valueZat": 18750000
     }