From 977ca2f4fbde41918e1833baf23d2905ca188d2a Mon Sep 17 00:00:00 2001 From: Arya Date: Wed, 16 Oct 2024 20:50:15 -0400 Subject: [PATCH 1/5] replaces some potential panics with error in block verifier checks --- zebra-consensus/src/block/check.rs | 15 ++- .../src/block/subsidy/funding_streams.rs | 1 + zebra-consensus/src/block/subsidy/general.rs | 104 ------------------ zebra-consensus/src/error.rs | 3 + 4 files changed, 14 insertions(+), 109 deletions(-) diff --git a/zebra-consensus/src/block/check.rs b/zebra-consensus/src/block/check.rs index 189cfdc8493..c973ae5118e 100644 --- a/zebra-consensus/src/block/check.rs +++ b/zebra-consensus/src/block/check.rs @@ -191,7 +191,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 // @@ -208,10 +209,14 @@ pub fn subsidy_is_valid( 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) diff --git a/zebra-consensus/src/block/subsidy/funding_streams.rs b/zebra-consensus/src/block/subsidy/funding_streams.rs index ce0bbf792ad..f1551a224e2 100644 --- 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 index d871751da34..56de345dd7a 100644 --- 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> .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 { - 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::::zero(), - lockbox_input_value(&network, Height::MIN) - ); - - let expected_lockbox_value: Amount = 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 = 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/error.rs b/zebra-consensus/src/error.rs index 91dfbf0ce3a..8fe14c62d52 100644 --- 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 for BlockError { From 31f7dfb757c50e54d4fea9624258e4ba1125b1b6 Mon Sep 17 00:00:00 2001 From: Arya Date: Fri, 18 Oct 2024 16:55:27 -0400 Subject: [PATCH 2/5] fixes outstanding documentation issues listed as examples in audit report --- zebra-chain/src/parameters/network/subsidy.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/zebra-chain/src/parameters/network/subsidy.rs b/zebra-chain/src/parameters/network/subsidy.rs index cb043f6b05c..3f7018cdcab 100644 --- a/zebra-chain/src/parameters/network/subsidy.rs +++ b/zebra-chain/src/parameters/network/subsidy.rs @@ -72,11 +72,11 @@ pub enum FundingStreamReceiver { 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)] From 3c017a794543cd28a40698b8f7f4f98ac163c529 Mon Sep 17 00:00:00 2001 From: Arya Date: Fri, 18 Oct 2024 16:58:56 -0400 Subject: [PATCH 3/5] fixes remaining TODOs in `parameters::network::subsidy` module. --- zebra-chain/src/parameters/network/subsidy.rs | 23 +++++++------------ 1 file changed, 8 insertions(+), 15 deletions(-) diff --git a/zebra-chain/src/parameters/network/subsidy.rs b/zebra-chain/src/parameters/network/subsidy.rs index 3f7018cdcab..df3d8c966f4 100644 --- a/zebra-chain/src/parameters/network/subsidy.rs +++ b/zebra-chain/src/parameters/network/subsidy.rs @@ -65,8 +65,8 @@ 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, } @@ -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 From 46d585ec8ee6c59262fcec6d1c3bef71398acd12 Mon Sep 17 00:00:00 2001 From: Arya Date: Fri, 18 Oct 2024 17:25:14 -0400 Subject: [PATCH 4/5] 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. --- zebra-consensus/src/block.rs | 2 +- zebra-consensus/src/block/check.rs | 34 ++++++++++++------------------ zebra-consensus/src/block/tests.rs | 6 +++--- zebra-consensus/src/checkpoint.rs | 2 +- 4 files changed, 18 insertions(+), 26 deletions(-) diff --git a/zebra-consensus/src/block.rs b/zebra-consensus/src/block.rs index 2b0013a5a3d..611aea2ceba 100644 --- 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 index c973ae5118e..24ef2ba2ed1 100644 --- 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 - 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!( @@ -205,7 +199,7 @@ 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; } @@ -255,25 +249,23 @@ 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 @@ -281,9 +273,9 @@ pub fn miner_fees_are_valid( // // > [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/tests.rs b/zebra-consensus/src/block/tests.rs index d8f74bf7b2f..e6eb6f2c4b9 100644 --- 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 = 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 index dfdf915b5be..039ea6e33e3 100644 --- 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 { From 4aa1b2ae24bd1cd85ba28f198cbd044f27d57dfc Mon Sep 17 00:00:00 2001 From: Arya Date: Mon, 21 Oct 2024 15:13:15 -0400 Subject: [PATCH 5/5] updates snapshot --- .../get_block_subsidy_future_nu6_height@nu6testnet_10.snap | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/zebra-rpc/src/methods/tests/snapshot/snapshots/get_block_subsidy_future_nu6_height@nu6testnet_10.snap b/zebra-rpc/src/methods/tests/snapshot/snapshots/get_block_subsidy_future_nu6_height@nu6testnet_10.snap index aba4f4d5a38..e06d32f5b32 100644 --- a/zebra-rpc/src/methods/tests/snapshot/snapshots/get_block_subsidy_future_nu6_height@nu6testnet_10.snap +++ b/zebra-rpc/src/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 }