diff --git a/zebra-consensus/src/block.rs b/zebra-consensus/src/block.rs index ebeb3e44..ea3f979b 100644 --- a/zebra-consensus/src/block.rs +++ b/zebra-consensus/src/block.rs @@ -187,11 +187,6 @@ where .expect("must have coinbase transaction"); check::subsidy_is_valid(&block, network)?; - // Validate `nExpiryHeight` consensus rules - // TODO: check non-coinbase transaction expiry against the block height (#2387) - // check the maximum expiry height for non-coinbase transactions (#2387) - check::coinbase_expiry_height(&height, coinbase_tx, network)?; - // Now do the slower checks // Check compatibility with ZIP-212 shielded Sapling and Orchard coinbase output decryption diff --git a/zebra-consensus/src/block/check.rs b/zebra-consensus/src/block/check.rs index 94ff3cee..66826942 100644 --- a/zebra-consensus/src/block/check.rs +++ b/zebra-consensus/src/block/check.rs @@ -286,48 +286,3 @@ pub fn merkle_root_validity( Ok(()) } - -/// Returns `Ok(())` if the expiry height for the coinbase transaction is valid -/// according to specifications [7.1] and [ZIP-203]. -/// -/// [7.1]: https://zips.z.cash/protocol/protocol.pdf#txnencodingandconsensus -/// [ZIP-203]: https://zips.z.cash/zip-0203 -pub fn coinbase_expiry_height( - height: &Height, - coinbase: &transaction::Transaction, - network: Network, -) -> Result<(), BlockError> { - match NetworkUpgrade::Nu5.activation_height(network) { - // If Nu5 does not have a height, apply the pre-Nu5 rule. - None => validate_expiry_height_max(coinbase.expiry_height()), - Some(activation_height) => { - // Conesnsus rule: from NU5 activation, the nExpiryHeight field of a - // coinbase transaction MUST be set equal to the block height. - if *height >= activation_height { - match coinbase.expiry_height() { - None => Err(TransactionError::CoinbaseExpiration)?, - Some(expiry) => { - if expiry != *height { - return Err(TransactionError::CoinbaseExpiration)?; - } - } - } - return Ok(()); - } - // Consensus rule: [Overwinter to Canopy inclusive, pre-NU5] nExpiryHeight - // MUST be less than or equal to 499999999. - validate_expiry_height_max(coinbase.expiry_height()) - } - } -} - -/// Validate the consensus rule: nExpiryHeight MUST be less than or equal to 499999999. -fn validate_expiry_height_max(expiry_height: Option) -> Result<(), BlockError> { - if let Some(expiry) = expiry_height { - if expiry > Height::MAX_EXPIRY_HEIGHT { - return Err(TransactionError::CoinbaseExpiration)?; - } - } - - Ok(()) -} diff --git a/zebra-consensus/src/block/tests.rs b/zebra-consensus/src/block/tests.rs index e7a6d725..a88a1d17 100644 --- a/zebra-consensus/src/block/tests.rs +++ b/zebra-consensus/src/block/tests.rs @@ -719,16 +719,16 @@ fn legacy_sigops_count_for_historic_blocks() { } #[test] -fn coinbase_height_validation() -> Result<(), Report> { +fn transaction_expiration_height_validation() -> Result<(), Report> { zebra_test::init(); - coinbase_height_for_network(Network::Mainnet)?; - coinbase_height_for_network(Network::Testnet)?; + transaction_expiration_height_for_network(Network::Mainnet)?; + transaction_expiration_height_for_network(Network::Testnet)?; Ok(()) } -fn coinbase_height_for_network(network: Network) -> Result<(), Report> { +fn transaction_expiration_height_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(), @@ -737,13 +737,22 @@ fn coinbase_height_for_network(network: Network) -> Result<(), Report> { for (&height, block) in block_iter { let block = Block::zcash_deserialize(&block[..]).expect("block should deserialize"); - // Validate - let result = check::coinbase_expiry_height( - &Height(height), - block.transactions.get(0).unwrap(), - network, - ); - assert!(result.is_ok()); + for (n, transaction) in block.transactions.iter().enumerate() { + if n == 0 { + // coinbase + let result = transaction::check::coinbase_expiry_height( + &Height(height), + transaction, + network, + ); + assert!(result.is_ok()); + } else { + // non coinbase + let result = + transaction::check::non_coinbase_expiry_height(&Height(height), transaction); + assert!(result.is_ok()); + } + } } Ok(()) diff --git a/zebra-consensus/src/error.rs b/zebra-consensus/src/error.rs index 8690343a..a6e719dd 100644 --- a/zebra-consensus/src/error.rs +++ b/zebra-consensus/src/error.rs @@ -15,6 +15,9 @@ use crate::{block::MAX_BLOCK_SIGOPS, BoxError}; #[cfg(any(test, feature = "proptest-impl"))] use proptest_derive::Arbitrary; +/// Workaround for format string identifier rules. +const MAX_EXPIRY_HEIGHT: block::Height = block::Height::MAX_EXPIRY_HEIGHT; + #[derive(Error, Copy, Clone, Debug, PartialEq, Eq)] pub enum SubsidyError { #[error("no coinbase transaction in block")] @@ -70,8 +73,36 @@ pub enum TransactionError { #[cfg_attr(any(test, feature = "proptest-impl"), proptest(skip))] LockedUntilAfterBlockTime(DateTime), - #[error("coinbase expiration height is invalid")] - CoinbaseExpiration, + #[error( + "coinbase expiry {expiry_height:?} must be the same as the block {block_height:?} \ + after NU5 activation, failing transaction: {transaction_hash:?}" + )] + CoinbaseExpiryBlockHeight { + expiry_height: Option, + block_height: zebra_chain::block::Height, + transaction_hash: zebra_chain::transaction::Hash, + }, + + #[error( + "expiry {expiry_height:?} must be less than the maximum {MAX_EXPIRY_HEIGHT:?} \ + coinbase: {is_coinbase}, block: {block_height:?}, failing transaction: {transaction_hash:?}" + )] + MaximumExpiryHeight { + expiry_height: zebra_chain::block::Height, + is_coinbase: bool, + block_height: zebra_chain::block::Height, + transaction_hash: zebra_chain::transaction::Hash, + }, + + #[error( + "transaction must not be mined at a block {block_height:?} \ + greater than its expiry {expiry_height:?}, failing transaction {transaction_hash:?}" + )] + ExpiredTransaction { + expiry_height: zebra_chain::block::Height, + block_height: zebra_chain::block::Height, + transaction_hash: zebra_chain::transaction::Hash, + }, #[error("coinbase transaction failed subsidy validation")] #[cfg_attr(any(test, feature = "proptest-impl"), proptest(skip))] diff --git a/zebra-consensus/src/transaction.rs b/zebra-consensus/src/transaction.rs index 2b8ef9d4..47d88fd4 100644 --- a/zebra-consensus/src/transaction.rs +++ b/zebra-consensus/src/transaction.rs @@ -307,6 +307,13 @@ where check::coinbase_tx_no_prevout_joinsplit_spend(&tx)?; } + // Validate `nExpiryHeight` consensus rules + if tx.has_any_coinbase_inputs() { + check::coinbase_expiry_height(&req.height(), &tx, network)?; + } else { + check::non_coinbase_expiry_height(&req.height(), &tx)?; + } + // [Canopy onward]: `vpub_old` MUST be zero. // https://zips.z.cash/protocol/protocol.pdf#joinsplitdesc check::disabled_add_to_sprout_pool(&tx, req.height(), network)?; diff --git a/zebra-consensus/src/transaction/check.rs b/zebra-consensus/src/transaction/check.rs index bfecf49a..15d9ba6a 100644 --- a/zebra-consensus/src/transaction/check.rs +++ b/zebra-consensus/src/transaction/check.rs @@ -297,3 +297,112 @@ pub fn coinbase_outputs_are_decryptable( Ok(()) } + +/// Returns `Ok(())` if the expiry height for the coinbase transaction is valid +/// according to specifications [7.1] and [ZIP-203]. +/// +/// [7.1]: https://zips.z.cash/protocol/protocol.pdf#txnencodingandconsensus +/// [ZIP-203]: https://zips.z.cash/zip-0203 +pub fn coinbase_expiry_height( + block_height: &Height, + coinbase: &Transaction, + network: Network, +) -> Result<(), TransactionError> { + let expiry_height = coinbase.expiry_height(); + + match NetworkUpgrade::Nu5.activation_height(network) { + // If Nu5 does not have a height, apply the pre-Nu5 rule. + None => validate_expiry_height_max(expiry_height, true, block_height, coinbase), + Some(activation_height) => { + // Consensus rule: from NU5 activation, the nExpiryHeight field of a + // coinbase transaction MUST be set equal to the block 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(), + })?; + } + } + } + return Ok(()); + } + // Consensus rule: [Overwinter to Canopy inclusive, pre-NU5] nExpiryHeight + // MUST be less than or equal to 499999999. + validate_expiry_height_max(expiry_height, true, block_height, coinbase) + } + } +} + +/// Returns `Ok(())` if the expiry height for a non coinbase transaction is valid +/// according to specifications [7.1] and [ZIP-203]. +/// +/// [7.1]: https://zips.z.cash/protocol/protocol.pdf#txnencodingandconsensus +/// [ZIP-203]: https://zips.z.cash/zip-0203 +pub fn non_coinbase_expiry_height( + block_height: &Height, + transaction: &Transaction, +) -> Result<(), TransactionError> { + if transaction.is_overwintered() { + let expiry_height = transaction.expiry_height(); + + validate_expiry_height_max(expiry_height, false, block_height, transaction)?; + validate_expiry_height_mined(expiry_height, block_height, transaction)?; + } + Ok(()) +} + +/// Validate the consensus rule: nExpiryHeight MUST be less than or equal to 499999999. +/// +/// The remaining arguments are not used for validation, +/// they are only used to create errors. +fn validate_expiry_height_max( + expiry_height: Option, + is_coinbase: bool, + block_height: &Height, + transaction: &Transaction, +) -> Result<(), TransactionError> { + if let Some(expiry_height) = expiry_height { + if expiry_height > Height::MAX_EXPIRY_HEIGHT { + return Err(TransactionError::MaximumExpiryHeight { + expiry_height, + is_coinbase, + block_height: *block_height, + transaction_hash: transaction.hash(), + })?; + } + } + + Ok(()) +} + +/// Validate the consensus rule: If a transaction is not a coinbase transaction +/// and its nExpiryHeight field is nonzero, then it MUST NOT be mined at a block +/// height greater than its nExpiryHeight. +/// +/// The `transaction` is only used to create errors. +fn validate_expiry_height_mined( + expiry_height: Option, + block_height: &Height, + transaction: &Transaction, +) -> Result<(), TransactionError> { + if let Some(expiry_height) = expiry_height { + if *block_height > expiry_height { + return Err(TransactionError::ExpiredTransaction { + expiry_height, + block_height: *block_height, + transaction_hash: transaction.hash(), + })?; + } + } + + Ok(()) +} diff --git a/zebra-consensus/src/transaction/tests.rs b/zebra-consensus/src/transaction/tests.rs index 1ea4c3d5..29660e20 100644 --- a/zebra-consensus/src/transaction/tests.rs +++ b/zebra-consensus/src/transaction/tests.rs @@ -1,4 +1,5 @@ use std::{ + cmp::max, collections::HashMap, convert::{TryFrom, TryInto}, sync::Arc, @@ -301,6 +302,10 @@ async fn v5_transaction_is_accepted_after_nu5_activation_testnet() { async fn v5_transaction_is_accepted_after_nu5_activation_for_network(network: Network) { let nu5 = NetworkUpgrade::Nu5; + let nu5_activation_height = nu5 + .activation_height(network) + .expect("NU5 activation height is specified"); + let blocks = match network { Network::Mainnet => zebra_test::vectors::MAINNET_BLOCKS.iter(), Network::Testnet => zebra_test::vectors::TESTNET_BLOCKS.iter(), @@ -317,13 +322,16 @@ async fn v5_transaction_is_accepted_after_nu5_activation_for_network(network: Ne let expected_hash = transaction.unmined_id(); + let fake_block_height = max( + nu5_activation_height, + transaction.expiry_height().unwrap_or(nu5_activation_height), + ); + let result = verifier .oneshot(Request::Block { transaction: Arc::new(transaction), known_utxos: Arc::new(HashMap::new()), - height: nu5 - .activation_height(network) - .expect("NU5 activation height is specified"), + height: fake_block_height, time: chrono::MAX_DATETIME, }) .await;