From 416d789687a3c7d239e4ef27072161d53bfbb750 Mon Sep 17 00:00:00 2001 From: Alfredo Garcia Date: Sat, 26 Jun 2021 14:14:03 -0300 Subject: [PATCH 1/3] update the has_inputs_and_outputs() to new rules --- zebra-chain/src/transaction.rs | 10 ++++++++ zebra-consensus/src/transaction/check.rs | 25 +++++++++++++++----- zebra-consensus/src/transaction/tests.rs | 30 ++++++++++++++++++++++-- 3 files changed, 57 insertions(+), 8 deletions(-) diff --git a/zebra-chain/src/transaction.rs b/zebra-chain/src/transaction.rs index d4b086631b8..5f0f7a420f2 100644 --- a/zebra-chain/src/transaction.rs +++ b/zebra-chain/src/transaction.rs @@ -483,4 +483,14 @@ impl Transaction { .map(orchard::ShieldedData::nullifiers) .flatten() } + + /// Access the [`orchard::Flags`] in this transaction, if there is any, + /// regardless of version. + pub fn orchard_flags(&self) -> Option { + if let Some(orchard_shielded_data) = self.orchard_shielded_data() { + Some(orchard_shielded_data.flags) + } else { + None + } + } } diff --git a/zebra-consensus/src/transaction/check.rs b/zebra-consensus/src/transaction/check.rs index 4ee98a07fd0..c2ed1d27450 100644 --- a/zebra-consensus/src/transaction/check.rs +++ b/zebra-consensus/src/transaction/check.rs @@ -13,12 +13,14 @@ use crate::error::TransactionError; /// Checks that the transaction has inputs and outputs. /// /// For `Transaction::V4`: -/// * at least one of `tx_in_count`, `nSpendsSapling`, and `nJoinSplit` MUST be non-zero. -/// * at least one of `tx_out_count`, `nOutputsSapling`, and `nJoinSplit` MUST be non-zero. +/// * At least one of `tx_in_count`, `nSpendsSapling`, and `nJoinSplit` MUST be non-zero. +/// * At least one of `tx_out_count`, `nOutputsSapling`, and `nJoinSplit` MUST be non-zero. /// /// For `Transaction::V5`: -/// * at least one of `tx_in_count`, `nSpendsSapling`, and `nActionsOrchard` MUST be non-zero. -/// * at least one of `tx_out_count`, `nOutputsSapling`, and `nActionsOrchard` MUST be non-zero. +/// * This condition must hold: `tx_in_count` > 0 or `nSpendsSapling` > 0 or +/// (`nActionsOrchard` > 0 and `enableSpendsOrchard` = 1) +/// * This condition must hold: `tx_out_count` > 0 or `nOutputsSapling` > 0 or +/// (`nActionsOrchard` > 0 and `enableOutputsOrchard` = 1) /// /// This check counts both `Coinbase` and `PrevOut` transparent inputs. /// @@ -30,10 +32,21 @@ pub fn has_inputs_and_outputs(tx: &Transaction) -> Result<(), TransactionError> let n_spends_sapling = tx.sapling_spends_per_anchor().count(); let n_outputs_sapling = tx.sapling_outputs().count(); let n_actions_orchard = tx.orchard_actions().count(); + let flags_orchard = tx.orchard_flags().unwrap_or(Flags::empty()); - if tx_in_count + n_spends_sapling + n_joinsplit + n_actions_orchard == 0 { + if tx_in_count + + n_spends_sapling + + n_joinsplit + + (n_actions_orchard > 0 && flags_orchard.contains(Flags::ENABLE_SPENDS)) as usize + == 0 + { Err(TransactionError::NoInputs) - } else if tx_out_count + n_outputs_sapling + n_joinsplit + n_actions_orchard == 0 { + } else if tx_out_count + + n_outputs_sapling + + n_joinsplit + + (n_actions_orchard > 0 && flags_orchard.contains(Flags::ENABLE_OUTPUTS)) as usize + == 0 + { Err(TransactionError::NoOutputs) } else { Ok(()) diff --git a/zebra-consensus/src/transaction/tests.rs b/zebra-consensus/src/transaction/tests.rs index 9f2922a9e20..2d57819f776 100644 --- a/zebra-consensus/src/transaction/tests.rs +++ b/zebra-consensus/src/transaction/tests.rs @@ -86,8 +86,34 @@ fn fake_v5_transaction_with_orchard_actions_has_inputs_and_outputs() { // guaranteed structurally by `orchard::ShieldedData`) insert_fake_orchard_shielded_data(&mut transaction); - // If a transaction has at least one Orchard shielded action, it should be considered to have - // inputs and/or outputs + // The check will fail if it haves no flags + assert_eq!( + check::has_inputs_and_outputs(&transaction), + Err(TransactionError::NoInputs) + ); + + // If we add ENABLE_SPENDS flag it will pass the inputs check but fails with the outputs + let shielded_data = insert_fake_orchard_shielded_data(&mut transaction); + shielded_data.flags = orchard::Flags::ENABLE_SPENDS; + + assert_eq!( + check::has_inputs_and_outputs(&transaction), + Err(TransactionError::NoOutputs) + ); + + // If we add ENABLE_OUTPUTS flag it will pass the outputs check but fails with the inputs + let shielded_data = insert_fake_orchard_shielded_data(&mut transaction); + shielded_data.flags = orchard::Flags::ENABLE_OUTPUTS; + + assert_eq!( + check::has_inputs_and_outputs(&transaction), + Err(TransactionError::NoInputs) + ); + + // Finally make it valid by adding both required flags + let shielded_data = insert_fake_orchard_shielded_data(&mut transaction); + shielded_data.flags = orchard::Flags::ENABLE_SPENDS | orchard::Flags::ENABLE_OUTPUTS; + assert!(check::has_inputs_and_outputs(&transaction).is_ok()); } From 3f331e225779564268ad1fe97245757a07400d4b Mon Sep 17 00:00:00 2001 From: Alfredo Garcia Date: Sat, 26 Jun 2021 16:19:59 -0300 Subject: [PATCH 2/3] apply clippy suggestions --- zebra-chain/src/transaction.rs | 7 ++----- zebra-consensus/src/transaction/check.rs | 2 +- 2 files changed, 3 insertions(+), 6 deletions(-) diff --git a/zebra-chain/src/transaction.rs b/zebra-chain/src/transaction.rs index 5f0f7a420f2..b86b990edb8 100644 --- a/zebra-chain/src/transaction.rs +++ b/zebra-chain/src/transaction.rs @@ -487,10 +487,7 @@ impl Transaction { /// Access the [`orchard::Flags`] in this transaction, if there is any, /// regardless of version. pub fn orchard_flags(&self) -> Option { - if let Some(orchard_shielded_data) = self.orchard_shielded_data() { - Some(orchard_shielded_data.flags) - } else { - None - } + self.orchard_shielded_data() + .map(|orchard_shielded_data| orchard_shielded_data.flags) } } diff --git a/zebra-consensus/src/transaction/check.rs b/zebra-consensus/src/transaction/check.rs index c2ed1d27450..74ec554af94 100644 --- a/zebra-consensus/src/transaction/check.rs +++ b/zebra-consensus/src/transaction/check.rs @@ -32,7 +32,7 @@ pub fn has_inputs_and_outputs(tx: &Transaction) -> Result<(), TransactionError> let n_spends_sapling = tx.sapling_spends_per_anchor().count(); let n_outputs_sapling = tx.sapling_outputs().count(); let n_actions_orchard = tx.orchard_actions().count(); - let flags_orchard = tx.orchard_flags().unwrap_or(Flags::empty()); + let flags_orchard = tx.orchard_flags().unwrap_or_else(Flags::empty); if tx_in_count + n_spends_sapling From 05019a9d4d5b7a18517489029f9a9a19171b76c4 Mon Sep 17 00:00:00 2001 From: Alfredo Garcia Date: Mon, 28 Jun 2021 12:32:38 -0300 Subject: [PATCH 3/3] add some TODOs --- zebra-consensus/src/transaction/check.rs | 1 + zebra-consensus/src/transaction/tests.rs | 3 ++- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/zebra-consensus/src/transaction/check.rs b/zebra-consensus/src/transaction/check.rs index 74ec554af94..dcf8a575fba 100644 --- a/zebra-consensus/src/transaction/check.rs +++ b/zebra-consensus/src/transaction/check.rs @@ -34,6 +34,7 @@ pub fn has_inputs_and_outputs(tx: &Transaction) -> Result<(), TransactionError> let n_actions_orchard = tx.orchard_actions().count(); let flags_orchard = tx.orchard_flags().unwrap_or_else(Flags::empty); + // TODO: Improve the code to express the spec rules better #2410. if tx_in_count + n_spends_sapling + n_joinsplit diff --git a/zebra-consensus/src/transaction/tests.rs b/zebra-consensus/src/transaction/tests.rs index 2d57819f776..13787119806 100644 --- a/zebra-consensus/src/transaction/tests.rs +++ b/zebra-consensus/src/transaction/tests.rs @@ -86,13 +86,14 @@ fn fake_v5_transaction_with_orchard_actions_has_inputs_and_outputs() { // guaranteed structurally by `orchard::ShieldedData`) insert_fake_orchard_shielded_data(&mut transaction); - // The check will fail if it haves no flags + // The check will fail if the transaction has no flags assert_eq!( check::has_inputs_and_outputs(&transaction), Err(TransactionError::NoInputs) ); // If we add ENABLE_SPENDS flag it will pass the inputs check but fails with the outputs + // TODO: Avoid new calls to `insert_fake_orchard_shielded_data` for each check #2409. let shielded_data = insert_fake_orchard_shielded_data(&mut transaction); shielded_data.flags = orchard::Flags::ENABLE_SPENDS;