Skip to content

Commit

Permalink
refactor!: move transaction error out of block payload
Browse files Browse the repository at this point in the history 
Signed-off-by: Marin Veršić <[email protected]>
  • Loading branch information
@mversic
mversic committed Oct 8, 2024
1 parent 0729420 commit 9c9186e
Show file tree
Hide file tree
Showing 19 changed files with 366 additions and 456 deletions.
9 changes: 4 additions & 5 deletions crates/iroha/benches/tps/utils.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)]
Expand Down
18 changes: 7 additions & 11 deletions crates/iroha/tests/integration/tx_history.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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(())
}
4 changes: 1 addition & 3 deletions crates/iroha_core/benches/blocks/common.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -56,9 +56,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
}
Expand Down
157 changes: 66 additions & 91 deletions crates/iroha_core/src/block.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,14 +4,14 @@
//! 2. If a block is received, i.e. deserialized:
//! `SignedBlock` -> `ValidBlock` -> `CommittedBlock`
//! [`Block`]s are organised into a linear sequence over time (also known as the block chain).
use std::{error::Error as _, time::Duration};
use std::{collections::BTreeMap, error::Error as _, time::Duration};

use iroha_crypto::{HashOf, KeyPair, MerkleTree};
use iroha_data_model::{
block::*,
events::prelude::*,
peer::PeerId,
transaction::{error::TransactionRejectionReason, prelude::*},
transaction::{error::TransactionRejectionReason, SignedTransaction},
};
use thiserror::Error;

Expand Down Expand Up @@ -118,7 +118,6 @@ pub struct BlockBuilder<B>(B);
mod pending {
use std::time::SystemTime;

use iroha_data_model::transaction::CommittedTransaction;
use nonzero_ext::nonzero;

use super::*;
Expand Down Expand Up @@ -156,14 +155,14 @@ mod pending {
fn make_header(
prev_block: Option<&SignedBlock>,
view_change_index: usize,
transactions: &[CommittedTransaction],
transactions: &[SignedTransaction],
) -> BlockHeader {
let prev_block_time =
prev_block.map_or(Duration::ZERO, |block| block.header().creation_time());

let latest_txn_time = transactions
.iter()
.map(|tx| tx.as_ref().creation_time())
.map(SignedTransaction::creation_time)
.max()
.expect("INTERNAL BUG: Block empty");

Expand Down Expand Up @@ -194,7 +193,7 @@ mod pending {
prev_block_hash: prev_block.map(SignedBlock::hash),
transactions_hash: transactions
.iter()
.map(|value| value.as_ref().hash())
.map(SignedTransaction::hash)
.collect::<MerkleTree<_>>()
.hash()
.expect("INTERNAL BUG: Empty block created"),
Expand All @@ -211,27 +210,31 @@ mod pending {
fn categorize_transactions(
transactions: Vec<AcceptedTransaction>,
state_block: &mut StateBlock<'_>,
) -> Vec<CommittedTransaction> {
transactions
) -> (
Vec<SignedTransaction>,
BTreeMap<usize, TransactionRejectionReason>,
) {
let mut errors = BTreeMap::new();

let transactions = transactions
.into_iter()
.map(|tx| match state_block.validate(tx) {
Ok(tx) => CommittedTransaction {
value: tx,
error: None,
},
.enumerate()
.map(|(idx, tx)| match state_block.validate(tx) {
Ok(tx) => tx,
Err((tx, error)) => {
iroha_logger::warn!(
reason = %error,
caused_by = ?error.source(),
"Transaction validation failed",
);
CommittedTransaction {
value: tx,
error: Some(Box::new(error)),
}
errors.insert(idx, error);

tx
}
})
.collect()
.collect();

(transactions, errors)
}

/// Chain the block with existing blockchain.
Expand All @@ -242,16 +245,19 @@ mod pending {
view_change_index: usize,
state: &mut StateBlock<'_>,
) -> BlockBuilder<Chained> {
let transactions = Self::categorize_transactions(self.0.transactions, state);

BlockBuilder(Chained(BlockPayload {
header: Self::make_header(
state.latest_block().as_deref(),
view_change_index,
&transactions,
),
transactions,
}))
let (transactions, errors) = Self::categorize_transactions(self.0.transactions, state);

BlockBuilder(Chained(
BlockPayload {
header: Self::make_header(
state.latest_block().as_deref(),
view_change_index,
&transactions,
),
transactions,
},
errors,
))
}
}
}
Expand All @@ -261,12 +267,17 @@ mod chained {

/// When a [`Pending`] block is chained with the blockchain it becomes [`Chained`] block.
#[derive(Debug, Clone)]
pub struct Chained(pub(super) BlockPayload);
pub struct Chained(
pub(super) BlockPayload,
pub(super) BTreeMap<usize, TransactionRejectionReason>,
);

impl BlockBuilder<Chained> {
/// Sign this block and get [`SignedBlock`].
pub fn sign(self, private_key: &PrivateKey) -> WithEvents<ValidBlock> {
WithEvents::new(ValidBlock(self.0 .0.sign(private_key)))
let mut block = self.0 .0.sign(private_key);
block.categorize_transactions(self.0 .1);
WithEvents::new(ValidBlock(block))
}
}
}
Expand Down Expand Up @@ -566,7 +577,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)
}) {
Expand All @@ -593,7 +604,8 @@ mod valid {
.transactions()
// TODO: Unnecessary clone?
.cloned()
.try_for_each(|CommittedTransaction { value, error }| {
.enumerate()
.try_for_each(|(idx, value)| {
let tx = if is_genesis {
AcceptedTransaction::accept_genesis(
value,
Expand All @@ -610,7 +622,7 @@ mod valid {
)
}?;

if error.is_some() {
if block.error(idx).is_some() {
match state_block.validate(tx) {
Err(rejected_transaction) => Ok(rejected_transaction),
Ok(_) => Err(TransactionValidationError::RejectedIsValid),
Expand Down Expand Up @@ -776,22 +788,22 @@ mod valid {
leader_private_key: &PrivateKey,
f: impl FnOnce(&mut BlockPayload),
) -> Self {
use nonzero_ext::nonzero;
let (transactions, errors) = (Vec::new(), BTreeMap::new());

let mut payload = BlockPayload {
header: BlockHeader {
height: nonzero!(2_u64),
height: nonzero_ext::nonzero!(2_u64),
prev_block_hash: None,
transactions_hash: HashOf::from_untyped_unchecked(Hash::prehashed(
[1; Hash::LENGTH],
)),
creation_time_ms: 0,
view_change_index: 0,
},
transactions: Vec::new(),
transactions,
};
f(&mut payload);
BlockBuilder(Chained(payload))
BlockBuilder(Chained(payload, errors))
.sign(leader_private_key)
.unpack(|_| {})
}
Expand Down Expand Up @@ -825,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);
}
}
Expand Down Expand Up @@ -1048,15 +1060,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().into()),
);

TransactionEvent {
block_height: Some(block_height),
hash: tx.as_ref().hash(),
hash: tx.hash(),
status,
}
});
Expand Down Expand Up @@ -1167,23 +1180,8 @@ mod tests {
.sign(alice_keypair.private_key())
.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]
Expand Down Expand Up @@ -1247,23 +1245,10 @@ mod tests {
.sign(alice_keypair.private_key())
.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]
Expand Down Expand Up @@ -1311,27 +1296,17 @@ mod tests {
.sign(alice_keypair.private_key())
.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."
);
}
Expand Down
11 changes: 2 additions & 9 deletions crates/iroha_core/src/metrics.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Loading

0 comments on commit 9c9186e

Please sign in to comment.