From 140a3eb721118356066b9414b4db25018c3efe05 Mon Sep 17 00:00:00 2001 From: Alfredo Garcia Date: Wed, 17 Nov 2021 15:15:12 -0300 Subject: [PATCH 01/11] validate consensus rule: negative fee not allowed --- zebra-chain/src/transaction.rs | 4 +- zebra-consensus/src/block/check.rs | 34 ++++++++++++-- zebra-consensus/src/block/tests.rs | 75 +++++++++++++++++++++++------- zebra-consensus/src/error.rs | 3 ++ 4 files changed, 93 insertions(+), 23 deletions(-) diff --git a/zebra-chain/src/transaction.rs b/zebra-chain/src/transaction.rs index 2f677c47208..d1a98c14867 100644 --- a/zebra-chain/src/transaction.rs +++ b/zebra-chain/src/transaction.rs @@ -1100,7 +1100,7 @@ impl Transaction { /// and added to sapling pool. /// /// https://zebra.zfnd.org/dev/rfcs/0012-value-pools.html#definitions - fn sapling_value_balance(&self) -> ValueBalance { + pub fn sapling_value_balance(&self) -> ValueBalance { let sapling_value_balance = match self { Transaction::V4 { sapling_shielded_data: Some(sapling_shielded_data), @@ -1167,7 +1167,7 @@ impl Transaction { /// and added to orchard pool. /// /// https://zebra.zfnd.org/dev/rfcs/0012-value-pools.html#definitions - fn orchard_value_balance(&self) -> ValueBalance { + pub fn orchard_value_balance(&self) -> ValueBalance { let orchard_value_balance = self .orchard_shielded_data() .map(|shielded_data| shielded_data.value_balance) diff --git a/zebra-consensus/src/block/check.rs b/zebra-consensus/src/block/check.rs index 01505ba7cbb..f8366d84772 100644 --- a/zebra-consensus/src/block/check.rs +++ b/zebra-consensus/src/block/check.rs @@ -4,6 +4,7 @@ use chrono::{DateTime, Utc}; use std::collections::HashSet; use zebra_chain::{ + amount::{Amount, Error as AmountError, NonNegative}, block::{Block, Hash, Header, Height}, parameters::{Network, NetworkUpgrade}, transaction, @@ -101,14 +102,41 @@ pub fn subsidy_is_valid(block: &Block, network: Network) -> Result<(), BlockErro let height = block.coinbase_height().ok_or(SubsidyError::NoCoinbase)?; let coinbase = block.transactions.get(0).ok_or(SubsidyError::NoCoinbase)?; + // Consensus rule: 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. + // https://zips.z.cash/protocol/protocol.pdf#txnencodingandconsensus + let transparent_value_balance: Amount = subsidy::general::output_amounts(coinbase) + .iter() + .sum::, AmountError>>() + .expect("the sum of all outputs wil lalways be positive") + .constrain() + .expect("positive value always fit in `NegativeAllowed`"); + let sapling_value_balance = coinbase.sapling_value_balance().sapling_amount(); + let orchard_value_balance = coinbase.orchard_value_balance().orchard_amount(); + + let block_subsidy: Amount = subsidy::general::block_subsidy(height, network) + .expect("a valid block subsidy for this height and network") + .constrain() + .expect("positive value always fit in `NegativeAllowed`"); + + // Consensus rule implementation: block fee must be at least zero. + let fees: Result, AmountError> = + (transparent_value_balance - sapling_value_balance - orchard_value_balance - block_subsidy) + .expect("should not overflow") + .constrain(); + + if fees.is_err() { + Err(SubsidyError::NegativeFees)? + } + + // Validate founders reward and funding streams let halving_div = subsidy::general::halving_divisor(height, network); let canopy_activation_height = NetworkUpgrade::Canopy .activation_height(network) .expect("Canopy activation height is known"); - // TODO: the sum of the coinbase transaction outputs must be less than or equal to the block subsidy plus transaction fees - - // Check founders reward and funding streams if height < SLOW_START_INTERVAL { unreachable!( "unsupported block height: callers should handle blocks below {:?}", diff --git a/zebra-consensus/src/block/tests.rs b/zebra-consensus/src/block/tests.rs index d5d6296c6ad..5a76ca754a1 100644 --- a/zebra-consensus/src/block/tests.rs +++ b/zebra-consensus/src/block/tests.rs @@ -7,7 +7,7 @@ use crate::{ use super::*; -use std::sync::Arc; +use std::{convert::TryFrom, sync::Arc}; use chrono::Utc; use color_eyre::eyre::{eyre, Report}; @@ -15,6 +15,7 @@ use once_cell::sync::Lazy; use tower::{buffer::Buffer, util::BoxService}; use zebra_chain::{ + amount::Amount, block::{self, Block, Height}, parameters::{Network, NetworkUpgrade}, serialization::{ZcashDeserialize, ZcashDeserializeInto}, @@ -193,7 +194,6 @@ fn difficulty_is_valid_for_network(network: Network) -> Result<(), Report> { #[test] fn difficulty_validation_failure() -> Result<(), Report> { zebra_test::init(); - use crate::error::*; // Get a block in the mainnet, and mangle its difficulty field let block = @@ -303,8 +303,6 @@ fn subsidy_is_valid_for_network(network: Network) -> Result<(), Report> { #[test] fn coinbase_validation_failure() -> Result<(), Report> { zebra_test::init(); - use crate::error::*; - let network = Network::Mainnet; // Get a block in the mainnet that is inside the founders reward period, @@ -376,9 +374,6 @@ fn coinbase_validation_failure() -> Result<(), Report> { #[test] fn founders_reward_validation_failure() -> Result<(), Report> { zebra_test::init(); - use crate::error::*; - use zebra_chain::transaction::Transaction; - let network = Network::Mainnet; // Get a block in the mainnet that is inside the founders reward period. @@ -390,12 +385,16 @@ fn founders_reward_validation_failure() -> Result<(), Report> { let tx = block .transactions .get(0) - .map(|transaction| Transaction::V3 { - inputs: transaction.inputs().to_vec(), - outputs: vec![transaction.outputs()[0].clone()], - lock_time: transaction.lock_time(), - expiry_height: Height(0), - joinsplit_data: None, + .map(|transaction| { + let mut output = transaction.outputs()[0].clone(); + output.value = Amount::try_from(i32::MAX).unwrap(); + Transaction::V3 { + inputs: transaction.inputs().to_vec(), + outputs: vec![output], + lock_time: transaction.lock_time(), + expiry_height: Height(0), + joinsplit_data: None, + } }) .unwrap(); @@ -448,9 +447,51 @@ fn funding_stream_validation_for_network(network: Network) -> Result<(), Report> #[test] fn funding_stream_validation_failure() -> Result<(), Report> { zebra_test::init(); - use crate::error::*; - use zebra_chain::transaction::Transaction; + let network = Network::Mainnet; + + // Get a block in the mainnet that is inside the funding stream period. + let block = + Arc::::zcash_deserialize(&zebra_test::vectors::BLOCK_MAINNET_1046400_BYTES[..]) + .expect("block should deserialize"); + + // Build the new transaction with modified coinbase outputs + let tx = block + .transactions + .get(0) + .map(|transaction| { + let mut output = transaction.outputs()[0].clone(); + output.value = Amount::try_from(i32::MAX).unwrap(); + Transaction::V4 { + inputs: transaction.inputs().to_vec(), + outputs: vec![output], + lock_time: transaction.lock_time(), + expiry_height: Height(0), + joinsplit_data: None, + sapling_shielded_data: None, + } + }) + .unwrap(); + + // Build new block + let transactions: Vec> = vec![Arc::new(tx)]; + let block = Block { + header: block.header, + transactions, + }; + // Validate it + let result = check::subsidy_is_valid(&block, network).unwrap_err(); + let expected = BlockError::Transaction(TransactionError::Subsidy( + SubsidyError::FundingStreamNotFound, + )); + assert_eq!(expected, result); + + Ok(()) +} + +#[test] +fn negative_fee_validation_failure() -> Result<(), Report> { + zebra_test::init(); let network = Network::Mainnet; // Get a block in the mainnet that is inside the funding stream period. @@ -481,9 +522,7 @@ fn funding_stream_validation_failure() -> Result<(), Report> { // Validate it let result = check::subsidy_is_valid(&block, network).unwrap_err(); - let expected = BlockError::Transaction(TransactionError::Subsidy( - SubsidyError::FundingStreamNotFound, - )); + let expected = BlockError::Transaction(TransactionError::Subsidy(SubsidyError::NegativeFees)); assert_eq!(expected, result); Ok(()) diff --git a/zebra-consensus/src/error.rs b/zebra-consensus/src/error.rs index 5e0edf034eb..c3f75f5cc82 100644 --- a/zebra-consensus/src/error.rs +++ b/zebra-consensus/src/error.rs @@ -24,6 +24,9 @@ pub enum SubsidyError { #[error("funding stream expected output not found")] FundingStreamNotFound, + + #[error("transaction fees must be positive")] + NegativeFees, } #[derive(Error, Clone, Debug, PartialEq, Eq)] From 48c41bbfa9d74c6bf8d29b42ae259bda7853336d Mon Sep 17 00:00:00 2001 From: Alfredo Garcia Date: Wed, 17 Nov 2021 15:15:57 -0300 Subject: [PATCH 02/11] fix a test TODO --- .../src/block/subsidy/funding_streams.rs | 2 +- zebra-consensus/src/block/subsidy/general.rs | 20 ++++++++++++++++--- 2 files changed, 18 insertions(+), 4 deletions(-) diff --git a/zebra-consensus/src/block/subsidy/funding_streams.rs b/zebra-consensus/src/block/subsidy/funding_streams.rs index 1e422ea9fb2..028539cd49b 100644 --- a/zebra-consensus/src/block/subsidy/funding_streams.rs +++ b/zebra-consensus/src/block/subsidy/funding_streams.rs @@ -53,7 +53,7 @@ pub fn funding_stream_values( /// as described in [protocol specification ยง7.10][7.10] /// /// [7.10]: https://zips.z.cash/protocol/protocol.pdf#fundingstreams -fn height_for_first_halving(network: Network) -> Height { +pub fn height_for_first_halving(network: Network) -> Height { // First halving on Mainnet is at Canopy // while in Testnet is at block constant height of `1_116_000` // https://zips.z.cash/protocol/protocol.pdf#zip214fundingstreams diff --git a/zebra-consensus/src/block/subsidy/general.rs b/zebra-consensus/src/block/subsidy/general.rs index 45aaf647092..02856ccb430 100644 --- a/zebra-consensus/src/block/subsidy/general.rs +++ b/zebra-consensus/src/block/subsidy/general.rs @@ -118,6 +118,11 @@ mod test { use super::*; use color_eyre::Report; + use crate::block::subsidy::{ + founders_reward::founders_reward, + funding_streams::{funding_stream_values, height_for_first_halving}, + }; + #[test] fn halving_test() -> Result<(), Report> { zebra_test::init(); @@ -307,8 +312,8 @@ mod test { } fn miner_subsidy_for_network(network: Network) -> Result<(), Report> { - use crate::block::subsidy::founders_reward::founders_reward; let blossom_height = Blossom.activation_height(network).unwrap(); + let first_halving_height = height_for_first_halving(network); // Miner reward before Blossom is 80% of the total block reward // 80*12.5/100 = 10 ZEC @@ -330,8 +335,17 @@ mod test { miner_subsidy(blossom_height, network, Some(founders_amount)) ); - // TODO: After first halving, miner will get 2.5 ZEC per mined block - // but we need funding streams code to get this number + // After first halving, miner will get 2.5 ZEC per mined block (not counting fees) + let funding_stream_values = funding_stream_values(first_halving_height, network)? + .iter() + .map(|row| row.1) + .sum::, Error>>() + .unwrap(); + + assert_eq!( + Amount::try_from(250_000_000), + miner_subsidy(first_halving_height, network, Some(funding_stream_values)) + ); // TODO: After second halving, there will be no funding streams, and // miners will get all the block reward From f6dd68a75e27e7f118c8f1993bd7e9ad440614ba Mon Sep 17 00:00:00 2001 From: Alfredo Garcia Date: Wed, 17 Nov 2021 16:30:12 -0300 Subject: [PATCH 03/11] fix imports --- zebra-consensus/src/block/tests.rs | 7 ------- 1 file changed, 7 deletions(-) diff --git a/zebra-consensus/src/block/tests.rs b/zebra-consensus/src/block/tests.rs index 8ff862672ca..9337bc0c56f 100644 --- a/zebra-consensus/src/block/tests.rs +++ b/zebra-consensus/src/block/tests.rs @@ -1,10 +1,5 @@ //! Tests for block verification -use crate::{ - parameters::{SLOW_START_INTERVAL, SLOW_START_SHIFT}, - script, -}; - use super::*; use std::{convert::TryFrom, sync::Arc}; @@ -34,8 +29,6 @@ use crate::{ script, transaction, }; -use super::*; - static VALID_BLOCK_TRANSCRIPT: Lazy< Vec<(Arc, Result)>, > = Lazy::new(|| { From 6332000e99fd33895e2438fbc7b5c24f510dd477 Mon Sep 17 00:00:00 2001 From: Alfredo Garcia Date: Thu, 18 Nov 2021 16:11:40 -0300 Subject: [PATCH 04/11] move import back --- zebra-consensus/src/block/tests.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/zebra-consensus/src/block/tests.rs b/zebra-consensus/src/block/tests.rs index 9337bc0c56f..d8de835772e 100644 --- a/zebra-consensus/src/block/tests.rs +++ b/zebra-consensus/src/block/tests.rs @@ -1,7 +1,5 @@ //! Tests for block verification -use super::*; - use std::{convert::TryFrom, sync::Arc}; use chrono::Utc; @@ -29,6 +27,8 @@ use crate::{ script, transaction, }; +use super::*; + static VALID_BLOCK_TRANSCRIPT: Lazy< Vec<(Arc, Result)>, > = Lazy::new(|| { From 73c0c70e4c167c2918fa0b34aacef52203170853 Mon Sep 17 00:00:00 2001 From: Alfredo Garcia Date: Thu, 18 Nov 2021 16:13:44 -0300 Subject: [PATCH 05/11] fix panic text --- zebra-consensus/src/block/check.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/zebra-consensus/src/block/check.rs b/zebra-consensus/src/block/check.rs index f8366d84772..62dd4d6f889 100644 --- a/zebra-consensus/src/block/check.rs +++ b/zebra-consensus/src/block/check.rs @@ -110,7 +110,7 @@ pub fn subsidy_is_valid(block: &Block, network: Network) -> Result<(), BlockErro let transparent_value_balance: Amount = subsidy::general::output_amounts(coinbase) .iter() .sum::, AmountError>>() - .expect("the sum of all outputs wil lalways be positive") + .expect("the sum of all outputs will always be positive") .constrain() .expect("positive value always fit in `NegativeAllowed`"); let sapling_value_balance = coinbase.sapling_value_balance().sapling_amount(); From 19f66779d9b24b8b13ec044aec48881e5f660023 Mon Sep 17 00:00:00 2001 From: Alfredo Garcia Date: Thu, 18 Nov 2021 16:38:20 -0300 Subject: [PATCH 06/11] join consensus rule check code --- zebra-consensus/src/block/check.rs | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/zebra-consensus/src/block/check.rs b/zebra-consensus/src/block/check.rs index 62dd4d6f889..5ee82d136ff 100644 --- a/zebra-consensus/src/block/check.rs +++ b/zebra-consensus/src/block/check.rs @@ -122,14 +122,10 @@ pub fn subsidy_is_valid(block: &Block, network: Network) -> Result<(), BlockErro .expect("positive value always fit in `NegativeAllowed`"); // Consensus rule implementation: block fee must be at least zero. - let fees: Result, AmountError> = - (transparent_value_balance - sapling_value_balance - orchard_value_balance - block_subsidy) - .expect("should not overflow") - .constrain(); - - if fees.is_err() { - Err(SubsidyError::NegativeFees)? - } + (transparent_value_balance - sapling_value_balance - orchard_value_balance - block_subsidy) + .expect("should not overflow") + .constrain::() + .map_err(|_| SubsidyError::NegativeFees)?; // Validate founders reward and funding streams let halving_div = subsidy::general::halving_divisor(height, network); From cd323b53bc48d9a6e0fc01a68bcb2db729c5ee49 Mon Sep 17 00:00:00 2001 From: Alfredo Garcia Date: Thu, 18 Nov 2021 18:10:35 -0300 Subject: [PATCH 07/11] match assertion better in tests --- zebra-consensus/src/block/tests.rs | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/zebra-consensus/src/block/tests.rs b/zebra-consensus/src/block/tests.rs index d8de835772e..6439a6c2521 100644 --- a/zebra-consensus/src/block/tests.rs +++ b/zebra-consensus/src/block/tests.rs @@ -409,10 +409,11 @@ fn founders_reward_validation_failure() -> Result<(), Report> { }; // Validate it - let result = check::subsidy_is_valid(&block, network).unwrap_err(); - let expected = BlockError::Transaction(TransactionError::Subsidy( + let result = check::subsidy_is_valid(&block, network); + let expected = Err(BlockError::Transaction(TransactionError::Subsidy( SubsidyError::FoundersRewardNotFound, - )); + ))); + assert_eq!(expected, result); Ok(()) @@ -483,10 +484,10 @@ fn funding_stream_validation_failure() -> Result<(), Report> { }; // Validate it - let result = check::subsidy_is_valid(&block, network).unwrap_err(); - let expected = BlockError::Transaction(TransactionError::Subsidy( + let result = check::subsidy_is_valid(&block, network); + let expected = Err(BlockError::Transaction(TransactionError::Subsidy( SubsidyError::FundingStreamNotFound, - )); + ))); assert_eq!(expected, result); Ok(()) @@ -524,8 +525,10 @@ fn negative_fee_validation_failure() -> Result<(), Report> { }; // Validate it - let result = check::subsidy_is_valid(&block, network).unwrap_err(); - let expected = BlockError::Transaction(TransactionError::Subsidy(SubsidyError::NegativeFees)); + let result = check::subsidy_is_valid(&block, network); + let expected = Err(BlockError::Transaction(TransactionError::Subsidy( + SubsidyError::NegativeFees, + ))); assert_eq!(expected, result); Ok(()) From 426ae23b07b79e44c3647ea4d51347cb1e47225c Mon Sep 17 00:00:00 2001 From: Alfredo Garcia Date: Tue, 23 Nov 2021 14:43:04 -0300 Subject: [PATCH 08/11] fix test --- zebra-consensus/src/block/tests.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/zebra-consensus/src/block/tests.rs b/zebra-consensus/src/block/tests.rs index 9d4b6bc618f..0cdd3cd8ca0 100644 --- a/zebra-consensus/src/block/tests.rs +++ b/zebra-consensus/src/block/tests.rs @@ -468,7 +468,7 @@ fn funding_stream_validation_failure() -> Result<(), Report> { Transaction::V4 { inputs: transaction.inputs().to_vec(), outputs: vec![output], - lock_time: transaction.lock_time(), + lock_time: transaction.lock_time().unwrap_or_else(LockTime::unlocked), expiry_height: Height(0), joinsplit_data: None, sapling_shielded_data: None, From fcdb77225d6ce4b0595c7baeaddce0fd5eff2545 Mon Sep 17 00:00:00 2001 From: Alfredo Garcia Date: Tue, 23 Nov 2021 19:22:34 -0300 Subject: [PATCH 09/11] fix consensus rule validation --- zebra-consensus/src/block.rs | 3 +- zebra-consensus/src/block/check.rs | 67 ++++++++++++++++++------------ zebra-consensus/src/block/tests.rs | 66 +++++++++++++++++------------ zebra-consensus/src/error.rs | 4 +- 4 files changed, 85 insertions(+), 55 deletions(-) diff --git a/zebra-consensus/src/block.rs b/zebra-consensus/src/block.rs index b912792ba8b..7288f9ec113 100644 --- a/zebra-consensus/src/block.rs +++ b/zebra-consensus/src/block.rs @@ -260,12 +260,13 @@ where } // TODO: check miner subsidy and miner fees (#1162) - let _block_miner_fees = + let block_miner_fees = block_miner_fees.map_err(|amount_error| BlockError::SummingMinerFees { height, hash, source: amount_error, })?; + check::miner_fees_are_valid(&block, network, block_miner_fees)?; // Finally, submit the block for contextual verification. let new_outputs = Arc::try_unwrap(known_utxos) diff --git a/zebra-consensus/src/block/check.rs b/zebra-consensus/src/block/check.rs index 613780101eb..69494f822fe 100644 --- a/zebra-consensus/src/block/check.rs +++ b/zebra-consensus/src/block/check.rs @@ -95,38 +95,13 @@ pub fn equihash_solution_is_valid(header: &Header) -> Result<(), equihash::Error header.solution.check(header) } -/// Returns `Ok(())` if the block subsidy and miner fees in `block` are valid for `network` +/// Returns `Ok(())` if the block subsidy in `block` is valid for `network` /// /// [3.9]: https://zips.z.cash/protocol/protocol.pdf#subsidyconcepts pub fn subsidy_is_valid(block: &Block, network: Network) -> Result<(), BlockError> { let height = block.coinbase_height().ok_or(SubsidyError::NoCoinbase)?; let coinbase = block.transactions.get(0).ok_or(SubsidyError::NoCoinbase)?; - // Consensus rule: 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. - // https://zips.z.cash/protocol/protocol.pdf#txnencodingandconsensus - let transparent_value_balance: Amount = subsidy::general::output_amounts(coinbase) - .iter() - .sum::, AmountError>>() - .expect("the sum of all outputs will always be positive") - .constrain() - .expect("positive value always fit in `NegativeAllowed`"); - let sapling_value_balance = coinbase.sapling_value_balance().sapling_amount(); - let orchard_value_balance = coinbase.orchard_value_balance().orchard_amount(); - - let block_subsidy: Amount = subsidy::general::block_subsidy(height, network) - .expect("a valid block subsidy for this height and network") - .constrain() - .expect("positive value always fit in `NegativeAllowed`"); - - // Consensus rule implementation: block fee must be at least zero. - (transparent_value_balance - sapling_value_balance - orchard_value_balance - block_subsidy) - .expect("should not overflow") - .constrain::() - .map_err(|_| SubsidyError::NegativeFees)?; - // Validate founders reward and funding streams let halving_div = subsidy::general::halving_divisor(height, network); let canopy_activation_height = NetworkUpgrade::Canopy @@ -185,6 +160,46 @@ pub fn subsidy_is_valid(block: &Block, network: Network) -> Result<(), BlockErro } } +/// Returns `Ok(())` if the miner fees consensus rule is valid. +/// +/// [7.1.2]: https://zips.z.cash/protocol/protocol.pdf#txnconsensus +pub fn miner_fees_are_valid( + block: &Block, + network: Network, + block_miner_fees: Amount, +) -> Result<(), BlockError> { + let height = block.coinbase_height().ok_or(SubsidyError::NoCoinbase)?; + let coinbase = block.transactions.get(0).ok_or(SubsidyError::NoCoinbase)?; + + let transparent_value_balance: Amount = subsidy::general::output_amounts(coinbase) + .iter() + .sum::, AmountError>>() + .expect("the sum of all outputs will always be positive") + .constrain() + .expect("positive value always fit in `NegativeAllowed`"); + let sapling_value_balance = coinbase.sapling_value_balance().sapling_amount(); + let orchard_value_balance = coinbase.orchard_value_balance().orchard_amount(); + + let block_subsidy = subsidy::general::block_subsidy(height, network) + .expect("a valid block subsidy for this height and network"); + + // Consensus rule: 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. + // https://zips.z.cash/protocol/protocol.pdf#txnconsensus + let left = (transparent_value_balance - sapling_value_balance - orchard_value_balance) + .expect("left side of the consensus rule equation should not overflow"); + let right = (block_subsidy + block_miner_fees) + .expect("right side of the consensus rule equation should not overflow"); + + if left > right { + return Err(SubsidyError::InvalidMinerFees)?; + } + + Ok(()) +} + /// Returns `Ok(())` if `header.time` is less than or equal to /// 2 hours in the future, according to the node's local clock (`now`). /// diff --git a/zebra-consensus/src/block/tests.rs b/zebra-consensus/src/block/tests.rs index 0cdd3cd8ca0..e7a6d7259f6 100644 --- a/zebra-consensus/src/block/tests.rs +++ b/zebra-consensus/src/block/tests.rs @@ -8,7 +8,7 @@ use once_cell::sync::Lazy; use tower::{buffer::Buffer, util::BoxService}; use zebra_chain::{ - amount::Amount, + amount::{Amount, MAX_MONEY}, block::{ self, tests::generate::{large_multi_transaction_block, large_single_transaction_block}, @@ -494,40 +494,54 @@ fn funding_stream_validation_failure() -> Result<(), Report> { } #[test] -fn negative_fee_validation_failure() -> Result<(), Report> { +fn miner_fees_validation_success() -> Result<(), Report> { + zebra_test::init(); + + miner_fees_validation_for_network(Network::Mainnet)?; + miner_fees_validation_for_network(Network::Testnet)?; + + Ok(()) +} + +fn miner_fees_validation_for_network(network: Network) -> Result<(), Report> { + let block_iter = match network { + Network::Mainnet => zebra_test::vectors::MAINNET_BLOCKS.iter(), + Network::Testnet => zebra_test::vectors::TESTNET_BLOCKS.iter(), + }; + + for (&height, block) in block_iter { + if Height(height) > SLOW_START_SHIFT { + let block = Block::zcash_deserialize(&block[..]).expect("block should deserialize"); + + // fake the miner fee to a big amount + let miner_fees = Amount::try_from(MAX_MONEY / 2).unwrap(); + + // Validate + let result = check::miner_fees_are_valid(&block, network, miner_fees); + assert!(result.is_ok()); + } + } + + Ok(()) +} + +#[test] +fn miner_fees_validation_failure() -> Result<(), Report> { zebra_test::init(); let network = Network::Mainnet; - // Get a block in the mainnet that is inside the funding stream period. let block = - Arc::::zcash_deserialize(&zebra_test::vectors::BLOCK_MAINNET_1046400_BYTES[..]) + Arc::::zcash_deserialize(&zebra_test::vectors::BLOCK_MAINNET_347499_BYTES[..]) .expect("block should deserialize"); - // Build the new transaction with modified coinbase outputs - let tx = block - .transactions - .get(0) - .map(|transaction| Transaction::V4 { - inputs: transaction.inputs().to_vec(), - outputs: vec![transaction.outputs()[0].clone()], - lock_time: transaction.lock_time().unwrap_or_else(LockTime::unlocked), - expiry_height: Height(0), - joinsplit_data: None, - sapling_shielded_data: None, - }) - .unwrap(); + // fake the miner fee to a small amount + let miner_fees = Amount::zero(); - // Build new block - let transactions: Vec> = vec![Arc::new(tx)]; - let block = Block { - header: block.header, - transactions, - }; + // Validate + let result = check::miner_fees_are_valid(&block, network, miner_fees); - // Validate it - let result = check::subsidy_is_valid(&block, network); let expected = Err(BlockError::Transaction(TransactionError::Subsidy( - SubsidyError::NegativeFees, + SubsidyError::InvalidMinerFees, ))); assert_eq!(expected, result); diff --git a/zebra-consensus/src/error.rs b/zebra-consensus/src/error.rs index 99c28093a09..f6c3f5109de 100644 --- a/zebra-consensus/src/error.rs +++ b/zebra-consensus/src/error.rs @@ -26,8 +26,8 @@ pub enum SubsidyError { #[error("funding stream expected output not found")] FundingStreamNotFound, - #[error("transaction fees must be positive")] - NegativeFees, + #[error("miner fees are invalid")] + InvalidMinerFees, } #[derive(Error, Clone, Debug, PartialEq, Eq)] From aaa75f5a85dd630d793f188970444a2bbed74388 Mon Sep 17 00:00:00 2001 From: Alfredo Garcia Date: Tue, 23 Nov 2021 21:25:22 -0300 Subject: [PATCH 10/11] remove panics --- zebra-consensus/src/block/check.rs | 7 +++---- zebra-consensus/src/error.rs | 3 +++ 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/zebra-consensus/src/block/check.rs b/zebra-consensus/src/block/check.rs index 69494f822fe..94ff3ceeacf 100644 --- a/zebra-consensus/src/block/check.rs +++ b/zebra-consensus/src/block/check.rs @@ -174,7 +174,7 @@ pub fn miner_fees_are_valid( let transparent_value_balance: Amount = subsidy::general::output_amounts(coinbase) .iter() .sum::, AmountError>>() - .expect("the sum of all outputs will always be positive") + .map_err(|_| SubsidyError::SumOverflow)? .constrain() .expect("positive value always fit in `NegativeAllowed`"); let sapling_value_balance = coinbase.sapling_value_balance().sapling_amount(); @@ -189,9 +189,8 @@ pub fn miner_fees_are_valid( // paid by transactions in this block. // https://zips.z.cash/protocol/protocol.pdf#txnconsensus let left = (transparent_value_balance - sapling_value_balance - orchard_value_balance) - .expect("left side of the consensus rule equation should not overflow"); - let right = (block_subsidy + block_miner_fees) - .expect("right side of the consensus rule equation should not overflow"); + .map_err(|_| SubsidyError::SumOverflow)?; + let right = (block_subsidy + block_miner_fees).map_err(|_| SubsidyError::SumOverflow)?; if left > right { return Err(SubsidyError::InvalidMinerFees)?; diff --git a/zebra-consensus/src/error.rs b/zebra-consensus/src/error.rs index f6c3f5109de..8690343a6c3 100644 --- a/zebra-consensus/src/error.rs +++ b/zebra-consensus/src/error.rs @@ -28,6 +28,9 @@ pub enum SubsidyError { #[error("miner fees are invalid")] InvalidMinerFees, + + #[error("a sum of amounts overflowed")] + SumOverflow, } #[derive(Error, Clone, Debug, PartialEq, Eq)] From 3c84b17e48e4e2e64628efe187d2082c751ffc4b Mon Sep 17 00:00:00 2001 From: teor Date: Wed, 24 Nov 2021 13:03:44 +1000 Subject: [PATCH 11/11] Delete a TODO --- zebra-consensus/src/block.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/zebra-consensus/src/block.rs b/zebra-consensus/src/block.rs index 7288f9ec113..ebeb3e44d49 100644 --- a/zebra-consensus/src/block.rs +++ b/zebra-consensus/src/block.rs @@ -259,7 +259,6 @@ where })?; } - // TODO: check miner subsidy and miner fees (#1162) let block_miner_fees = block_miner_fees.map_err(|amount_error| BlockError::SummingMinerFees { height,