From b1c682402ca7507be7a384725ba84e5cded62c6e Mon Sep 17 00:00:00 2001 From: Marek Date: Wed, 15 Jan 2025 12:57:29 +0100 Subject: [PATCH 1/3] Avoid tx conversion in coinbase tx --- zebra-consensus/src/error.rs | 3 + zebra-consensus/src/transaction/check.rs | 13 ++++ zebra-consensus/src/transaction/tests.rs | 87 +++++++++--------------- 3 files changed, 49 insertions(+), 54 deletions(-) diff --git a/zebra-consensus/src/error.rs b/zebra-consensus/src/error.rs index ac7e339eb55..caf68bad225 100644 --- a/zebra-consensus/src/error.rs +++ b/zebra-consensus/src/error.rs @@ -83,6 +83,9 @@ pub enum TransactionError { #[error("non-coinbase transactions MUST NOT have coinbase inputs")] NonCoinbaseHasCoinbaseInput, + #[error("the tx is not coinbase, but it should be")] + NotCoinbase, + #[error("transaction is locked until after block height {}", _0.0)] LockedUntilAfterBlockHeight(block::Height), diff --git a/zebra-consensus/src/transaction/check.rs b/zebra-consensus/src/transaction/check.rs index b7338bbdadd..2987e78cdd9 100644 --- a/zebra-consensus/src/transaction/check.rs +++ b/zebra-consensus/src/transaction/check.rs @@ -330,6 +330,14 @@ pub fn coinbase_outputs_are_decryptable( network: &Network, height: Height, ) -> Result<(), TransactionError> { + // Do quick checks first so we can avoid an expensive tx conversion + // in `zcash_note_encryption::decrypts_successfully`. + + // The consensus rule only applies to coinbase txs with shielded outputs. + if !transaction.has_shielded_outputs() { + return Ok(()); + } + // The consensus rule only applies to Heartwood onward. if height < NetworkUpgrade::Heartwood @@ -339,6 +347,11 @@ pub fn coinbase_outputs_are_decryptable( return Ok(()); } + // The passed tx should have been be a coinbase tx. + if !transaction.is_coinbase() { + return Err(TransactionError::NotCoinbase); + } + if !zcash_note_encryption::decrypts_successfully(transaction, network, height) { return Err(TransactionError::CoinbaseOutputsNotDecryptable); } diff --git a/zebra-consensus/src/transaction/tests.rs b/zebra-consensus/src/transaction/tests.rs index 044c8b01842..024be84dbd1 100644 --- a/zebra-consensus/src/transaction/tests.rs +++ b/zebra-consensus/src/transaction/tests.rs @@ -3293,65 +3293,44 @@ fn add_to_sprout_pool_after_nu() { fn coinbase_outputs_are_decryptable_for_historical_blocks() -> Result<(), Report> { let _init_guard = zebra_test::init(); - for network in Network::iter() { - coinbase_outputs_are_decryptable_for_historical_blocks_for_network(network)?; - } + for net in Network::iter() { + let mut tested_coinbase_txs = false; + let mut tested_non_coinbase_txs = false; - Ok(()) -} + for (height, block) in net.block_iter() { + let block = block.zcash_deserialize_into::().expect("block"); + let height = Height(*height); + let is_heartwood = height >= NetworkUpgrade::Heartwood.activation_height(&net).unwrap(); + let tx = block.transactions.first().expect("coinbase transaction"); -fn coinbase_outputs_are_decryptable_for_historical_blocks_for_network( - network: Network, -) -> Result<(), Report> { - let block_iter = network.block_iter(); - - let mut tested_coinbase_txs = 0; - let mut tested_non_coinbase_txs = 0; - - for (height, block) in block_iter { - let block = block - .zcash_deserialize_into::() - .expect("block is structurally valid"); - let height = Height(*height); - let heartwood_onward = height - >= NetworkUpgrade::Heartwood - .activation_height(&network) - .unwrap(); - let coinbase_tx = block - .transactions - .first() - .expect("must have coinbase transaction"); - - // Check if the coinbase outputs are decryptable with an all-zero key. - if heartwood_onward - && (coinbase_tx.sapling_outputs().count() > 0 - || coinbase_tx.orchard_actions().count() > 0) - { - // We are only truly decrypting something if it's Heartwood-onward - // and there are relevant outputs. - tested_coinbase_txs += 1; - } - check::coinbase_outputs_are_decryptable(coinbase_tx, &network, height) - .expect("coinbase outputs must be decryptable with an all-zero key"); - - // For remaining transactions, check if existing outputs are NOT decryptable - // with an all-zero key, if applicable. - for tx in block.transactions.iter().skip(1) { - let has_outputs = tx.sapling_outputs().count() > 0 || tx.orchard_actions().count() > 0; - if has_outputs && heartwood_onward { - tested_non_coinbase_txs += 1; - check::coinbase_outputs_are_decryptable(tx, &network, height).expect_err( - "decrypting a non-coinbase output with an all-zero key should fail", - ); - } else { - check::coinbase_outputs_are_decryptable(tx, &network, height) - .expect("a transaction without outputs, or pre-Heartwood, must be considered 'decryptable'"); + if tx.has_shielded_outputs() && is_heartwood { + tested_coinbase_txs = true; + } + + // Check if the coinbase shielded outputs are decryptable with an all-zero key. + check::coinbase_outputs_are_decryptable(tx, &net, height) + .expect("coinbase shielded outputs must be decryptable with an all-zero key"); + + // For remaining transactions, check if existing outputs are NOT decryptable + // with an all-zero key, if applicable. + for tx in block.transactions.iter().skip(1) { + if tx.has_shielded_outputs() && is_heartwood { + tested_non_coinbase_txs = true; + check::coinbase_outputs_are_decryptable(tx, &net, height).expect_err( + "non-coinbase shielded outputs must not be decryptable with an all-zero key", + ); + } else { + check::coinbase_outputs_are_decryptable(tx, &net, height).expect( + "a tx without shielded outputs, or pre-Heartwood, must be considered \ + 'decryptable'", + ); + } } } - } - assert!(tested_coinbase_txs > 0, "ensure it was actually tested"); - assert!(tested_non_coinbase_txs > 0, "ensure it was actually tested"); + assert!(tested_coinbase_txs); + assert!(tested_non_coinbase_txs); + } Ok(()) } From d25a7457404931039363f451d84fae0443a46048 Mon Sep 17 00:00:00 2001 From: Marek Date: Wed, 15 Jan 2025 17:26:34 +0100 Subject: [PATCH 2/3] Update a section number --- zebra-consensus/src/transaction/check.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/zebra-consensus/src/transaction/check.rs b/zebra-consensus/src/transaction/check.rs index 2987e78cdd9..3e12578cfe6 100644 --- a/zebra-consensus/src/transaction/check.rs +++ b/zebra-consensus/src/transaction/check.rs @@ -309,7 +309,7 @@ where /// # Consensus /// /// > [Heartwood onward] All Sapling and Orchard outputs in coinbase transactions MUST decrypt to a note -/// > plaintext, i.e. the procedure in § 4.19.3 ‘Decryption using a Full Viewing Key ( Sapling and Orchard )’ on p. 67 +/// > plaintext, i.e. the procedure in § 4.20.3 ‘Decryption using a Full Viewing Key (Sapling and Orchard)’ /// > does not return ⊥, using a sequence of 32 zero bytes as the outgoing viewing key. (This implies that before /// > Canopy activation, Sapling outputs of a coinbase transaction MUST have note plaintext lead byte equal to /// > 0x01.) From be36ae99dd6fb06cc1aef6d2cbf8756f68ea4c03 Mon Sep 17 00:00:00 2001 From: Marek Date: Wed, 15 Jan 2025 17:27:45 +0100 Subject: [PATCH 3/3] Refactor the test for decrypting coinbase outputs --- zebra-consensus/src/transaction/tests.rs | 100 +++++++++++++++++------ 1 file changed, 76 insertions(+), 24 deletions(-) diff --git a/zebra-consensus/src/transaction/tests.rs b/zebra-consensus/src/transaction/tests.rs index 024be84dbd1..597f6b9335f 100644 --- a/zebra-consensus/src/transaction/tests.rs +++ b/zebra-consensus/src/transaction/tests.rs @@ -3289,47 +3289,99 @@ fn add_to_sprout_pool_after_nu() { ); } +/// Checks that Heartwood onward, all Sapling and Orchard outputs in coinbase txs decrypt to a note +/// plaintext, i.e. the procedure in § 4.20.3 ‘Decryption using a Full Viewing Key (Sapling and +/// Orchard )’ does not return ⊥, using a sequence of 32 zero bytes as the outgoing viewing key. We +/// will refer to such a sequence as the _zero key_. #[test] -fn coinbase_outputs_are_decryptable_for_historical_blocks() -> Result<(), Report> { +fn coinbase_outputs_are_decryptable() -> Result<(), Report> { let _init_guard = zebra_test::init(); for net in Network::iter() { - let mut tested_coinbase_txs = false; - let mut tested_non_coinbase_txs = false; + let mut tested_post_heartwood_shielded_coinbase_tx = false; + let mut tested_pre_heartwood_shielded_coinbase_tx = false; + + let mut tested_post_heartwood_unshielded_coinbase_tx = false; + let mut tested_pre_heartwood_unshielded_coinbase_tx = false; + + let mut tested_post_heartwood_shielded_non_coinbase_tx = false; + let mut tested_pre_heartwood_shielded_non_coinbase_tx = false; + + let mut tested_post_heartwood_unshielded_non_coinbase_tx = false; + let mut tested_pre_heartwood_unshielded_non_coinbase_tx = false; for (height, block) in net.block_iter() { let block = block.zcash_deserialize_into::().expect("block"); let height = Height(*height); let is_heartwood = height >= NetworkUpgrade::Heartwood.activation_height(&net).unwrap(); - let tx = block.transactions.first().expect("coinbase transaction"); + let coinbase = block.transactions.first().expect("coinbase transaction"); - if tx.has_shielded_outputs() && is_heartwood { - tested_coinbase_txs = true; + if coinbase.has_shielded_outputs() && is_heartwood { + tested_post_heartwood_shielded_coinbase_tx = true; + check::coinbase_outputs_are_decryptable(coinbase, &net, height).expect( + "post-Heartwood shielded coinbase outputs must be decryptable with the zero key", + ); } - // Check if the coinbase shielded outputs are decryptable with an all-zero key. - check::coinbase_outputs_are_decryptable(tx, &net, height) - .expect("coinbase shielded outputs must be decryptable with an all-zero key"); - - // For remaining transactions, check if existing outputs are NOT decryptable - // with an all-zero key, if applicable. - for tx in block.transactions.iter().skip(1) { - if tx.has_shielded_outputs() && is_heartwood { - tested_non_coinbase_txs = true; - check::coinbase_outputs_are_decryptable(tx, &net, height).expect_err( - "non-coinbase shielded outputs must not be decryptable with an all-zero key", - ); - } else { - check::coinbase_outputs_are_decryptable(tx, &net, height).expect( - "a tx without shielded outputs, or pre-Heartwood, must be considered \ - 'decryptable'", + if coinbase.has_shielded_outputs() && !is_heartwood { + tested_pre_heartwood_shielded_coinbase_tx = true; + check::coinbase_outputs_are_decryptable(coinbase, &net, height) + .expect("the consensus rule does not apply to pre-Heartwood txs"); + } + + if !coinbase.has_shielded_outputs() && is_heartwood { + tested_post_heartwood_unshielded_coinbase_tx = true; + check::coinbase_outputs_are_decryptable(coinbase, &net, height) + .expect("the consensus rule does not apply to txs with no shielded outputs"); + } + + if !coinbase.has_shielded_outputs() && !is_heartwood { + tested_pre_heartwood_unshielded_coinbase_tx = true; + check::coinbase_outputs_are_decryptable(coinbase, &net, height) + .expect("the consensus rule does not apply to pre-Heartwood txs"); + } + + // For non-coinbase txs, check if existing outputs are NOT decryptable with an all-zero + // key, if applicable. + for non_coinbase in block.transactions.iter().skip(1) { + if non_coinbase.has_shielded_outputs() && is_heartwood { + tested_post_heartwood_shielded_non_coinbase_tx = true; + assert_eq!( + check::coinbase_outputs_are_decryptable(non_coinbase, &net, height), + Err(TransactionError::NotCoinbase) + ) + } + + if non_coinbase.has_shielded_outputs() && !is_heartwood { + tested_pre_heartwood_shielded_non_coinbase_tx = true; + check::coinbase_outputs_are_decryptable(non_coinbase, &net, height) + .expect("the consensus rule does not apply to pre-Heartwood txs"); + } + + if !non_coinbase.has_shielded_outputs() && is_heartwood { + tested_post_heartwood_unshielded_non_coinbase_tx = true; + check::coinbase_outputs_are_decryptable(non_coinbase, &net, height).expect( + "the consensus rule does not apply to txs with no shielded outputs", ); } + + if !non_coinbase.has_shielded_outputs() && !is_heartwood { + tested_pre_heartwood_unshielded_non_coinbase_tx = true; + check::coinbase_outputs_are_decryptable(non_coinbase, &net, height) + .expect("the consensus rule does not apply to pre-Heartwood txs"); + } } } - assert!(tested_coinbase_txs); - assert!(tested_non_coinbase_txs); + assert!(tested_post_heartwood_shielded_coinbase_tx); + // We have no pre-Heartwood shielded coinbase txs. + assert!(!tested_pre_heartwood_shielded_coinbase_tx); + assert!(tested_post_heartwood_unshielded_coinbase_tx); + assert!(tested_pre_heartwood_unshielded_coinbase_tx); + assert!(tested_post_heartwood_shielded_non_coinbase_tx); + assert!(tested_pre_heartwood_shielded_non_coinbase_tx); + assert!(tested_post_heartwood_unshielded_non_coinbase_tx); + assert!(tested_pre_heartwood_unshielded_non_coinbase_tx); } Ok(())