From baaa78d4531760e8018c3ebd2bfebd70620877a1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marin=20Ver=C5=A1i=C4=87?= Date: Tue, 30 Nov 2021 11:44:54 +0100 Subject: [PATCH] fix review comments MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Marin Veršić --- core/src/kura.rs | 6 ++- core/src/smartcontracts/isi/query.rs | 2 +- core/src/tx.rs | 3 ++ core/src/wsv.rs | 53 ++++++++++--------- core/test_network/tests/sumeragi_with_mock.rs | 2 +- data_model/src/lib.rs | 27 +++++++--- 6 files changed, 57 insertions(+), 36 deletions(-) diff --git a/core/src/kura.rs b/core/src/kura.rs index 30794bc03c1..c5f7df98b9c 100644 --- a/core/src/kura.rs +++ b/core/src/kura.rs @@ -181,7 +181,7 @@ impl Actor for KuraWithIO { } Err(error) => { iroha_logger::error!(%error, "Initialization of kura failed"); - panic!("Init failed"); + panic!("Kura initialization failed"); } } } @@ -245,7 +245,9 @@ impl KuraWithIO { match self.block_store.write(&block).await { Ok(hash) => { self.merkle_tree = self.merkle_tree.add(hash); - self.wsv.apply(block).await; + if let Err(error) = self.wsv.apply(block).await { + iroha_logger::warn!(%error, "Failed to execute transaction on WSV"); + } self.broker.issue_send(UpdateNetworkTopology).await; self.broker.issue_send(ContinueSync).await; Ok(hash) diff --git a/core/src/smartcontracts/isi/query.rs b/core/src/smartcontracts/isi/query.rs index 94b95af9e56..af16da73966 100644 --- a/core/src/smartcontracts/isi/query.rs +++ b/core/src/smartcontracts/isi/query.rs @@ -297,7 +297,7 @@ mod tests { .sign(ALICE_KEYS.clone()) .expect("Failed to sign blocks.") .commit(); - wsv.apply(vcb).await; + wsv.apply(vcb).await?; let wrong_hash = Hash::new(&[2_u8]); let not_found = FindTransactionByHash::new(wrong_hash).execute(&wsv); diff --git a/core/src/tx.rs b/core/src/tx.rs index c1fb3f02ab9..cc4c8f8bff6 100644 --- a/core/src/tx.rs +++ b/core/src/tx.rs @@ -313,16 +313,19 @@ impl AcceptedTransaction { } impl IsInBlockchain for VersionedAcceptedTransaction { + #[inline] fn is_in_blockchain(&self, wsv: &WorldStateView) -> bool { wsv.has_transaction(&self.hash()) } } impl IsInBlockchain for VersionedValidTransaction { + #[inline] fn is_in_blockchain(&self, wsv: &WorldStateView) -> bool { wsv.has_transaction(&self.hash()) } } impl IsInBlockchain for VersionedRejectedTransaction { + #[inline] fn is_in_blockchain(&self, wsv: &WorldStateView) -> bool { wsv.has_transaction(&self.hash()) } diff --git a/core/src/wsv.rs b/core/src/wsv.rs index 23b15f74083..2f853130cf9 100644 --- a/core/src/wsv.rs +++ b/core/src/wsv.rs @@ -68,6 +68,7 @@ pub struct WorldStateView { } impl Default for WorldStateView { + #[inline] fn default() -> Self { Self::new(W::default()) } @@ -75,6 +76,7 @@ impl Default for WorldStateView { impl World { /// Creates an empty `World`. + #[inline] pub fn new() -> Self { Self::default() } @@ -121,7 +123,11 @@ impl WorldStateView { #[iroha_futures::telemetry_future] pub async fn init(&self, blocks: Vec) { for block in blocks { - self.apply(block).await + #[allow(clippy::panic)] + if let Err(error) = self.apply(block).await { + iroha_logger::error!(%error, "Initialization of WSV failed"); + panic!("WSV initialization failed"); + } } } @@ -151,39 +157,36 @@ impl WorldStateView { account.permission_tokens.clone() } - /// Apply instructions to the `WorldStateView`. + /// Apply `CommittedBlock` with changes in form of **Iroha Special Instructions** to `self`. /// /// # Errors - /// Can fail if execution of instructions fail(should be fine after validation) - fn proceed(&self, tx: &VersionedValidTransaction) -> Result<()> { - let tx = tx.as_inner_v1(); - - for instruction in &tx.payload.instructions { - instruction - .clone() - .execute(tx.payload.account_id.clone(), self)?; - } - // XXX: Should it just return `()`? - Ok(()) - } - - /// Apply `CommittedBlock` with changes in form of **Iroha Special Instructions** to `self`. + /// Can fail if execution of instruction fails(should be fine after validation) #[iroha_futures::telemetry_future] #[iroha_logger::log(skip(self, block))] - pub async fn apply(&self, block: VersionedCommittedBlock) { + pub async fn apply(&self, block: VersionedCommittedBlock) -> Result<()> { for tx in &block.as_inner_v1().transactions { - if let Err(error) = self.proceed(tx) { - iroha_logger::warn!(%error, "Failed to proceed transaction on WSV"); - } + let account_id = &tx.payload().account_id; + tx.as_inner_v1() + .payload + .instructions + .iter() + .cloned() + .try_for_each(|instruction| instruction.execute(account_id.clone(), self))?; + self.transactions.insert(tx.hash()); // Yeild control cooperatively to the task scheduler. // The transaction processing is a long CPU intensive task, so this should be included here. task::yield_now().await; } - for tx in &block.as_inner_v1().rejected_transactions { - self.transactions.insert(tx.hash()); - } + block + .as_inner_v1() + .rejected_transactions + .iter() + .for_each(|tx| { + self.transactions.insert(tx.hash()); + }); self.blocks.push(block); + Ok(()) } /// Hash of latest block @@ -500,12 +503,14 @@ impl WorldStateView { impl Deref for World { type Target = World; + #[inline] fn deref(&self) -> &Self::Target { self } } impl DerefMut for World { + #[inline] fn deref_mut(&mut self) -> &mut Self::Target { self } @@ -573,7 +578,7 @@ mod tests { } let block: VersionedCommittedBlock = block.clone().into(); block_hashes.push(block.hash()); - wsv.apply(block).await; + wsv.apply(block).await.unwrap(); } assert_eq!( diff --git a/core/test_network/tests/sumeragi_with_mock.rs b/core/test_network/tests/sumeragi_with_mock.rs index d592a352ef0..81a987016ea 100644 --- a/core/test_network/tests/sumeragi_with_mock.rs +++ b/core/test_network/tests/sumeragi_with_mock.rs @@ -123,7 +123,7 @@ pub mod utils { async fn handle(&mut self, StoreBlock(block): StoreBlock) -> Self::Result { self.broker.issue_send(Stored(block.hash())).await; - self.wsv.apply(block).await; + self.wsv.apply(block).await.unwrap(); self.broker.issue_send(UpdateNetworkTopology).await; self.broker.issue_send(ContinueSync).await; } diff --git a/data_model/src/lib.rs b/data_model/src/lib.rs index 95897cb5453..564de99ece1 100644 --- a/data_model/src/lib.rs +++ b/data_model/src/lib.rs @@ -2029,8 +2029,10 @@ pub mod transaction { && self.metadata == other.metadata } + /// Checks if number of instructions in payload exceeds maximum + /// /// # Errors - /// Asserts specific instruction number of instruction constraint + /// Fails if instruction length exceeds maximum instruction number pub fn check_instruction_len(&self, max_instruction_number: u64) -> Result<()> { if self .instructions @@ -2155,6 +2157,7 @@ pub mod transaction { impl VersionedValidTransaction { /// Same as [`as_v1`](`VersionedValidTransaction::as_v1()`) but also does conversion + #[inline] pub const fn as_inner_v1(&self) -> &ValidTransaction { match self { Self::V1(v1) => &v1.0, @@ -2180,8 +2183,10 @@ pub mod transaction { self.as_inner_v1().hash().transmute() } + /// Checks if number of instructions in payload exceeds maximum + /// /// # Errors - /// Asserts specific instruction number of instruction in transaction constraint + /// Fails if instruction length exceeds maximum instruction number pub fn check_instruction_len(&self, max_instruction_len: u64) -> Result<()> { self.as_inner_v1() .check_instruction_len(max_instruction_len) @@ -2201,15 +2206,17 @@ pub mod transaction { )] #[derive(Clone, Debug, Io, Encode, Decode, IntoSchema)] pub struct ValidTransaction { - /// [`ValidTransaction`]'s payload. + /// The [`Transaction`]'s payload. pub payload: Payload, - /// [`ValidTransaction`]'s [`Signature`]s. + /// [`Transaction`]'s [`Signature`]s. pub signatures: SignaturesOf, } impl ValidTransaction { + /// Checks if number of instructions in payload exceeds maximum + /// /// # Errors - /// Asserts specific instruction number of instruction in transaction constraint + /// Fails if instruction length exceeds maximum instruction number pub fn check_instruction_len(&self, max_instruction_len: u64) -> Result<()> { self.payload.check_instruction_len(max_instruction_len) } @@ -2255,8 +2262,10 @@ pub mod transaction { } } + /// Checks if number of instructions in payload exceeds maximum + /// /// # Errors - /// Asserts specific instruction number of instruction in transaction constraint + /// Fails if instruction length exceeds maximum instruction number pub fn check_instruction_len(&self, max_instruction_len: u64) -> Result<()> { self.as_inner_v1() .check_instruction_len(max_instruction_len) @@ -2317,7 +2326,7 @@ pub mod transaction { Clone, Debug, Io, Encode, Decode, Serialize, Deserialize, Eq, PartialEq, IntoSchema, )] pub struct RejectedTransaction { - /// [`Transaction`]'s payload. + /// The [`Transaction`]'s payload. pub payload: Payload, /// [`Transaction`]'s [`Signature`]s. pub signatures: SignaturesOf, @@ -2326,8 +2335,10 @@ pub mod transaction { } impl RejectedTransaction { + /// Checks if number of instructions in payload exceeds maximum + /// /// # Errors - /// Asserts specific instruction number of instruction in transaction constraint + /// Fails if instruction length exceeds maximum instruction number pub fn check_instruction_len(&self, max_instruction_len: u64) -> Result<()> { self.payload.check_instruction_len(max_instruction_len) }