refactor!: move transaction error out of block payload

Signed-off-by: Marin Veršić <[email protected]>

crates/iroha/benches/tps/utils.rs

@@ -110,15 +110,14 @@ impl Config {
             .view();
         let mut blocks =
             state_view.all_blocks(NonZeroUsize::new(blocks_out_of_measure as usize + 1).unwrap());
-        let (txs_accepted, txs_rejected) = (0..self.blocks)
+        let (txs_rejected, txs_accepted) = (0..self.blocks)
             .map(|_| {
                 let block = blocks
                     .next()
                     .expect("The block is not yet in state. Need more sleep?");
-                (
-                    block.transactions().filter(|tx| tx.error.is_none()).count(),
-                    block.transactions().filter(|tx| tx.error.is_some()).count(),
-                )
+
+                let rejected = block.errors().count();
+                (rejected, block.transactions().count() - rejected)
             })
             .fold((0, 0), |acc, pair| (acc.0 + pair.0, acc.1 + pair.1));
         #[allow(clippy::float_arithmetic, clippy::cast_precision_loss)]

crates/iroha/tests/integration/tx_history.rs

@@ -50,7 +50,7 @@ fn client_has_rejected_and_acepted_txs_should_return_tx_history() -> Result<()>
 
     let transactions = client
         .query(transaction::all())
-        .filter_with(|tx| tx.transaction.value.authority.eq(account_id.clone()))
+        .filter_with(|tx| tx.value.authority.eq(account_id.clone()))
         .with_pagination(Pagination {
             limit: Some(nonzero!(50_u64)),
             offset: 1,
@@ -59,15 +59,11 @@ fn client_has_rejected_and_acepted_txs_should_return_tx_history() -> Result<()>
     assert_eq!(transactions.len(), 50);
 
     let mut prev_creation_time = core::time::Duration::from_millis(0);
-    transactions
-        .iter()
-        .map(AsRef::as_ref)
-        .map(AsRef::as_ref)
-        .for_each(|tx| {
-            assert_eq!(tx.authority(), &account_id);
-            //check sorted
-            assert!(tx.creation_time() >= prev_creation_time);
-            prev_creation_time = tx.creation_time();
-        });
+    transactions.iter().map(AsRef::as_ref).for_each(|tx| {
+        assert_eq!(tx.authority(), &account_id);
+        //check sorted
+        assert!(tx.creation_time() >= prev_creation_time);
+        prev_creation_time = tx.creation_time();
+    });
     Ok(())
 }

crates/iroha_core/benches/blocks/common.rs

@@ -58,9 +58,7 @@ pub fn create_block(
     .unwrap();
 
     // Verify that transactions are valid
-    for tx in block.as_ref().transactions() {
-        assert_eq!(tx.error, None);
-    }
+    assert_eq!(block.as_ref().errors().count(), 0);
 
     block
 }

crates/iroha_core/src/block.rs

@@ -11,7 +11,7 @@ use iroha_data_model::{
     block::*,
     events::prelude::*,
     peer::PeerId,
-    transaction::{error::TransactionRejectionReason, prelude::*},
+    transaction::{error::TransactionRejectionReason, SignedTransaction},
 };
 use thiserror::Error;
 
@@ -562,7 +562,7 @@ mod valid {
             if block.transactions().any(|tx| {
                 state
                     .transactions()
-                    .get(&tx.as_ref().hash())
+                    .get(&tx.hash())
                     // In case of soft-fork transaction is check if it was added at the same height as candidate block
                     .is_some_and(|height| height.get() < expected_block_height)
             }) {
@@ -585,7 +585,6 @@ mod valid {
 
             let errors = block
                 .transactions()
-                .map(AsRef::as_ref)
                 // FIXME: Redundant clone
                 .cloned()
                 .enumerate()
@@ -838,7 +837,7 @@ mod valid {
 
         let transactions = block.payload().transactions.as_slice();
         for transaction in transactions {
-            if transaction.value.authority() != genesis_account {
+            if transaction.authority() != genesis_account {
                 return Err(InvalidGenesisError::UnexpectedAuthority);
             }
         }
@@ -1074,15 +1073,16 @@ mod event {
         fn produce_events(&self) -> impl Iterator<Item = PipelineEventBox> {
             let block_height = self.as_ref().header().height;
 
-            let tx_events = self.as_ref().transactions().map(move |tx| {
-                let status = tx.error.as_ref().map_or_else(
+            let block = self.as_ref();
+            let tx_events = block.transactions().enumerate().map(move |(idx, tx)| {
+                let status = block.error(idx).map_or_else(
                     || TransactionStatus::Approved,
-                    |error| TransactionStatus::Rejected(error.clone()),
+                    |error| TransactionStatus::Rejected(Box::new(error.clone())),
                 );
 
                 TransactionEvent {
                     block_height: Some(block_height),
-                    hash: tx.as_ref().hash(),
+                    hash: tx.hash(),
                     status,
                 }
             });
@@ -1195,23 +1195,8 @@ mod tests {
             .categorize(&mut state_block)
             .unpack(|_| {});
 
-        // The first transaction should be confirmed
-        assert!(valid_block
-            .as_ref()
-            .transactions()
-            .next()
-            .unwrap()
-            .error
-            .is_none());
-
-        // The second transaction should be rejected
-        assert!(valid_block
-            .as_ref()
-            .transactions()
-            .nth(1)
-            .unwrap()
-            .error
-            .is_some());
+        // The 1st transaction should be confirmed and the 2nd rejected
+        assert_eq!(*valid_block.as_ref().errors().next().unwrap().0, 1);
     }
 
     #[tokio::test]
@@ -1277,23 +1262,10 @@ mod tests {
             .categorize(&mut state_block)
             .unpack(|_| {});
 
-        // The first transaction should fail
-        assert!(valid_block
-            .as_ref()
-            .transactions()
-            .next()
-            .unwrap()
-            .error
-            .is_some());
-
-        // The third transaction should succeed
-        assert!(valid_block
-            .as_ref()
-            .transactions()
-            .nth(2)
-            .unwrap()
-            .error
-            .is_none());
+        // The 1st transaction should fail and 2nd succeed
+        let mut errors = valid_block.as_ref().errors();
+        assert_eq!(0, *errors.next().unwrap().0);
+        assert!(errors.next().is_none());
     }
 
     #[tokio::test]
@@ -1343,27 +1315,17 @@ mod tests {
             .categorize(&mut state_block)
             .unpack(|_| {});
 
-        // The first transaction should be rejected
-        assert!(
-            valid_block
-                .as_ref()
-                .transactions()
-                .next()
-                .unwrap()
-                .error
-                .is_some(),
+        let mut errors = valid_block.as_ref().errors();
+        // The 1st transaction should be rejected
+        assert_eq!(
+            0,
+            *errors.next().unwrap().0,
             "The first transaction should be rejected, as it contains `Fail`."
         );
 
         // The second transaction should be accepted
         assert!(
-            valid_block
-                .as_ref()
-                .transactions()
-                .nth(1)
-                .unwrap()
-                .error
-                .is_none(),
+            errors.next().is_none(),
             "The second transaction should be accepted."
         );
     }

crates/iroha_core/src/metrics.rs

@@ -75,15 +75,8 @@ impl MetricsReporter {
                     break;
                 };
                 block_index += 1;
-                let mut block_txs_accepted = 0;
-                let mut block_txs_rejected = 0;
-                for tx in block.transactions() {
-                    if tx.error.is_none() {
-                        block_txs_accepted += 1;
-                    } else {
-                        block_txs_rejected += 1;
-                    }
-                }
+                let block_txs_rejected = block.errors().count() as u64;
+                let block_txs_accepted = block.transactions().count() as u64 - block_txs_rejected;
 
                 self.metrics
                     .txs

crates/iroha_core/src/smartcontracts/isi/query.rs

@@ -6,9 +6,9 @@ use eyre::Result;
 use iroha_data_model::{
     prelude::*,
     query::{
-        error::QueryExecutionFail as Error, parameters::QueryParams, QueryBox, QueryOutputBatchBox,
-        QueryRequest, QueryRequestWithAuthority, QueryResponse, SingularQueryBox,
-        SingularQueryOutputBox,
+        error::QueryExecutionFail as Error, parameters::QueryParams, CommittedTransaction,
+        QueryBox, QueryOutputBatchBox, QueryRequest, QueryRequestWithAuthority, QueryResponse,
+        SingularQueryBox, SingularQueryOutputBox,
     },
 };
 
@@ -68,7 +68,7 @@ impl SortableQueryOutput for RoleId {
     }
 }
 
-impl SortableQueryOutput for TransactionQueryOutput {
+impl SortableQueryOutput for CommittedTransaction {
     fn get_metadata_sorting_key(&self, _key: &Name) -> Option<JsonValue> {
         None
     }
@@ -571,15 +571,11 @@ mod tests {
 
         assert_eq!(txs.len() as u64, num_blocks * 2);
         assert_eq!(
-            txs.iter()
-                .filter(|txn| txn.transaction.error.is_some())
-                .count() as u64,
+            txs.iter().filter(|txn| txn.error.is_some()).count() as u64,
             num_blocks
         );
         assert_eq!(
-            txs.iter()
-                .filter(|txn| txn.transaction.error.is_none())
-                .count() as u64,
+            txs.iter().filter(|txn| txn.error.is_none()).count() as u64,
             num_blocks
         );
 
@@ -632,9 +628,7 @@ mod tests {
 
         let not_found = FindTransactions::new()
             .execute(
-                TransactionQueryOutputPredicateBox::build(|tx| {
-                    tx.transaction.value.hash.eq(wrong_hash)
-                }),
+                CommittedTransactionPredicateBox::build(|tx| tx.value.hash.eq(wrong_hash)),
                 &state_view,
             )
             .expect("Query execution should not fail")
@@ -643,20 +637,17 @@ mod tests {
 
         let found_accepted = FindTransactions::new()
             .execute(
-                TransactionQueryOutputPredicateBox::build(|tx| {
-                    tx.transaction.value.hash.eq(va_tx.as_ref().hash())
+                CommittedTransactionPredicateBox::build(|tx| {
+                    tx.value.hash.eq(va_tx.as_ref().hash())
                 }),
                 &state_view,
             )
             .expect("Query execution should not fail")
             .next()
             .expect("Query should return a transaction");
 
-        if found_accepted.transaction.error.is_none() {
-            assert_eq!(
-                va_tx.as_ref().hash(),
-                found_accepted.as_ref().as_ref().hash()
-            )
+        if found_accepted.error.is_none() {
+            assert_eq!(va_tx.as_ref().hash(), found_accepted.as_ref().hash())
         }
         Ok(())
     }

crates/iroha_core/src/smartcontracts/isi/tx.rs

@@ -9,12 +9,10 @@ use iroha_data_model::{
     prelude::*,
     query::{
         error::QueryExecutionFail,
-        predicate::{
-            predicate_atoms::block::TransactionQueryOutputPredicateBox, CompoundPredicate,
-        },
-        TransactionQueryOutput,
+        predicate::{predicate_atoms::block::CommittedTransactionPredicateBox, CompoundPredicate},
+        CommittedTransaction,
     },
-    transaction::CommittedTransaction,
+    transaction::error::TransactionRejectionReason,
 };
 use iroha_telemetry::metrics;
 use nonzero_ext::nonzero;
@@ -51,28 +49,36 @@ impl BlockTransactionRef {
         self.0.hash()
     }
 
-    fn value(&self) -> CommittedTransaction {
-        self.0
-            .transactions()
-            .nth(self.1)
-            .expect("The transaction is not found")
-            .clone()
+    fn value(&self) -> (SignedTransaction, Option<TransactionRejectionReason>) {
+        (
+            self.0
+                .transactions()
+                .nth(self.1)
+                .expect("INTERNAL BUG: The transaction is not found")
+                .clone(),
+            self.0.error(self.1).cloned(),
+        )
     }
 }
 
 impl ValidQuery for FindTransactions {
     #[metrics(+"find_transactions")]
     fn execute(
         self,
-        filter: CompoundPredicate<TransactionQueryOutputPredicateBox>,
+        filter: CompoundPredicate<CommittedTransactionPredicateBox>,
         state_ro: &impl StateReadOnly,
     ) -> Result<impl Iterator<Item = Self::Item>, QueryExecutionFail> {
         Ok(state_ro
             .all_blocks(nonzero!(1_usize))
             .flat_map(BlockTransactionIter::new)
-            .map(|tx| TransactionQueryOutput {
-                block_hash: tx.block_hash(),
-                transaction: tx.value(),
+            .map(|tx| {
+                let (value, error) = tx.value();
+
+                CommittedTransaction {
+                    block_hash: tx.block_hash(),
+                    value,
+                    error,
+                }
             })
             .filter(move |tx| filter.applies(tx)))
     }

crates/iroha_core/src/state.rs

@@ -1381,15 +1381,14 @@ impl<'state> StateBlock<'state> {
     /// # Errors
     /// Fails if transaction instruction execution fails
     fn execute_transactions(&mut self, block: &CommittedBlock) -> Result<()> {
+        let block = block.as_ref();
+
         // TODO: Should this block panic instead?
-        for tx in block.as_ref().transactions() {
-            if tx.error.is_none() {
+        for (idx, tx) in block.transactions().enumerate() {
+            if block.error(idx).is_none() {
                 // Execute every tx in it's own transaction
                 let mut transaction = self.transaction();
-                transaction.process_executable(
-                    tx.as_ref().instructions(),
-                    tx.as_ref().authority().clone(),
-                )?;
+                transaction.process_executable(tx.instructions(), tx.authority().clone())?;
                 transaction.apply();
             }
         }
@@ -1421,7 +1420,6 @@ impl<'state> StateBlock<'state> {
         block
             .as_ref()
             .transactions()
-            .map(|tx| &tx.value)
             .map(SignedTransaction::hash)
             .for_each(|tx_hash| {
                 self.transactions.insert(tx_hash, block_height);