From 36965ee52aed57a5c8e09da5bcaf2b02c295226b Mon Sep 17 00:00:00 2001 From: Daniil Polyakov Date: Thu, 19 May 2022 23:23:23 +0300 Subject: [PATCH 1/6] Fix logging problem Signed-off-by: Daniil Polyakov --- cli/src/lib.rs | 21 ++++---- client/tests/integration/restart_peer.rs | 18 +++++-- core/src/genesis.rs | 11 ++++- core/src/sumeragi/fault.rs | 4 +- core/test_network/src/lib.rs | 61 +++++++++--------------- data_model/src/transaction.rs | 10 ++-- 6 files changed, 69 insertions(+), 56 deletions(-) diff --git a/cli/src/lib.rs b/cli/src/lib.rs index 7dba5377870..7a9ec95dac9 100644 --- a/cli/src/lib.rs +++ b/cli/src/lib.rs @@ -96,11 +96,15 @@ where /// To make `Iroha` peer work all actors should be started first. /// After that moment it you can start it with listening to torii events. /// + /// # Side effect + /// - Prints welcome message in the log + /// /// # Errors /// - Reading genesis from disk /// - Reading telemetry configs /// - telemetry setup - /// - Initialisation of [`Sumeragi`] + /// - Initialization of [`Sumeragi`] + #[allow(clippy::non_ascii_literal)] pub async fn new( args: &Arguments, instruction_validator: IsInstructionAllowedBoxed, @@ -113,6 +117,10 @@ where }; config.load_environment()?; + let telemetry = iroha_logger::init(&config.logger)?; + iroha_logger::info!("Hyperledgerいろは2にようこそ！"); + iroha_logger::info!("(translation) Welcome to Hyperledger Iroha 2!"); + let genesis = G::from_configuration( args.submit_genesis, RawGenesisBlock::from_path(&args.genesis_path)?, @@ -127,6 +135,7 @@ where instruction_validator, query_validator, broker, + telemetry, ) .await } @@ -137,22 +146,18 @@ where /// - Reading telemetry configs /// - telemetry setup /// - Initialization of [`Sumeragi`] - #[allow(clippy::non_ascii_literal)] pub async fn with_genesis( genesis: Option, config: Configuration, instruction_validator: IsInstructionAllowedBoxed, query_validator: IsQueryAllowedBoxed, broker: Broker, + telemetry: Option, ) -> Result { if !config.disable_panic_terminal_colors { - if let Err(e) = color_eyre::install() { - iroha_logger::error!("Tried to install eyre_hook twice: {:?}", e); - } + // Ignoring error if `iroha_logger` has already installed colors + let _color_res = color_eyre::install(); } - let telemetry = iroha_logger::init(&config.logger)?; - iroha_logger::info!("Hyperledgerいろは2にようこそ！"); - iroha_logger::info!("(translation) Welcome to Hyperledger Iroha 2!"); let listen_addr = config.torii.p2p_addr.clone(); iroha_logger::info!(%listen_addr, "Starting peer"); let network = IrohaNetwork::new( diff --git a/client/tests/integration/restart_peer.rs b/client/tests/integration/restart_peer.rs index 43cc179ce95..07c359aafbf 100644 --- a/client/tests/integration/restart_peer.rs +++ b/client/tests/integration/restart_peer.rs @@ -4,7 +4,7 @@ use std::{str::FromStr, thread, time::Duration}; use eyre::Result; use iroha_client::client::{self, Client}; -use iroha_core::prelude::*; +use iroha_core::{genesis::GenesisNetwork, prelude::*}; use iroha_data_model::prelude::*; use tempfile::TempDir; use test_network::{Peer as TestPeer, *}; @@ -24,7 +24,13 @@ fn restarted_peer_should_have_the_same_asset_amount() -> Result<()> { // Given let rt = Runtime::test(); - rt.block_on(peer.start_with_config_permissions_dir(configuration.clone(), AllowAll, &temp_dir)); + rt.block_on(peer.start_with_config_permissions_dir( + configuration.clone(), + GenesisNetwork::test(true), + AllowAll, + AllowAll, + &temp_dir, + )); let mut iroha_client = Client::test(&peer.api_address, &peer.telemetry_address); wait_for_genesis_committed(&vec![iroha_client.clone()], 0); @@ -60,7 +66,13 @@ fn restarted_peer_should_have_the_same_asset_amount() -> Result<()> { thread::sleep(Duration::from_millis(2000)); let rt = Runtime::test(); - rt.block_on(peer.start_with_config_permissions_dir(configuration, AllowAll, &temp_dir)); + rt.block_on(peer.start_with_config_permissions_dir( + configuration, + GenesisNetwork::test(true), + AllowAll, + AllowAll, + &temp_dir, + )); let account_asset = iroha_client .poll_request(client::asset::by_account_id(account_id), |assets| { diff --git a/core/src/genesis.rs b/core/src/genesis.rs index 592a6a22e07..fce59bd3691 100644 --- a/core/src/genesis.rs +++ b/core/src/genesis.rs @@ -196,7 +196,14 @@ impl GenesisNetworkTrait for GenesisNetwork { raw_transaction.sign_and_accept(genesis_key_pair, tx_limits) }) - .filter_map(Result::ok) + .enumerate() + .filter_map(|(i, res)| { + res.map_err(|error| { + let error_msg = format!("{error:#}"); + iroha_logger::error!(error = %error_msg, "Genesis transaction #{i} didn't succeed") + }) + .ok() + }) .collect(), wait_for_peers_retry_count: genesis_config.wait_for_peers_retry_count, wait_for_peers_retry_period_ms: genesis_config.wait_for_peers_retry_period_ms, @@ -276,7 +283,7 @@ impl GenesisTransaction { /// Convert `GenesisTransaction` into `AcceptedTransaction` with signature /// /// # Errors - /// Fails if signing fails + /// Fails if signing or accepting fails pub fn sign_and_accept( &self, genesis_key_pair: KeyPair, diff --git a/core/src/sumeragi/fault.rs b/core/src/sumeragi/fault.rs index b9fd8b3e1e8..f05fb285780 100644 --- a/core/src/sumeragi/fault.rs +++ b/core/src/sumeragi/fault.rs @@ -466,7 +466,9 @@ impl ctx: &mut Context, ) -> Result<()> { if transactions.is_empty() { - Err(eyre!("Genesis transactions set is empty.")) + Err(eyre!( + "Genesis transactions set is empty or all of the transactions are incorrect" + )) } else if genesis_topology.leader() != &self.peer_id { Err(eyre!( "Incorrect network topology this peer should be {:?} but is {:?}", diff --git a/core/test_network/src/lib.rs b/core/test_network/src/lib.rs index ac15a42e0f3..67bb5063d9e 100644 --- a/core/test_network/src/lib.rs +++ b/core/test_network/src/lib.rs @@ -435,10 +435,15 @@ where /// - Block store path not readable /// - [`Iroha::start_as_task`] failed or produced empty job handle. /// - `receiver` fails to produce a message. + /// + /// # TODO + /// Use *Builder* pattern (#2238) pub async fn start_with_config_permissions_dir( &mut self, configuration: Configuration, - permissions: impl Into> + Send + 'static, + genesis: Option, + instruction_validator: impl Into> + Send + 'static, + query_validator: impl Into> + Send + 'static, temp_dir: &TempDir, ) { let mut configuration = self.get_config(configuration); @@ -453,15 +458,18 @@ where telemetry_addr = %self.telemetry_address ); let broker = self.broker.clone(); + let telemetry = + iroha_logger::init(&configuration.logger).expect("Failed to initialize telemetry"); let (sender, receiver) = std::sync::mpsc::sync_channel(1); let handle = task::spawn( async move { let mut iroha = >::with_genesis( - G::test(true), + genesis, configuration, - permissions.into(), - AllowAll.into(), + instruction_validator.into(), + query_validator.into(), broker, + telemetry, ) .await .expect("Failed to start iroha"); @@ -473,6 +481,7 @@ where ); self.iroha = Some(receiver.recv().unwrap()); + time::sleep(Duration::from_millis(300)).await; self.shutdown = Some(handle); } @@ -480,6 +489,7 @@ where /// /// # Panics /// - [`TempDir::new`] failed. + /// - Initializing [telemetry](iroha_logger::init()) failed /// - [`Iroha::with_genesis`] failed. /// - Failed to send [`Iroha`] via sender. /// - [`Iroha::start_as_task`] failed or produced empty job handle. @@ -491,41 +501,14 @@ where query_validator: impl Into> + Send + 'static, ) { let temp_dir = TempDir::new().expect("Failed to create temp dir."); - let mut configuration = self.get_config(configuration); - configuration - .kura - .block_store_path(temp_dir.path()) - .expect("Guaranteed to exist"); - let info_span = iroha_logger::info_span!( - "test-peer", - p2p_addr = %self.p2p_address, - api_addr = %self.api_address, - telemetry_addr = %self.telemetry_address - ); - let broker = self.broker.clone(); - let (sender, receiver) = std::sync::mpsc::sync_channel(1); - let join_handle = tokio::spawn( - async move { - let _temp_dir = temp_dir; - let mut iroha = >::with_genesis( - genesis, - configuration, - instruction_validator.into(), - query_validator.into(), - broker, - ) - .await - .expect("Failed to start Iroha"); - let job_handle = iroha.start_as_task().unwrap(); - sender.send(iroha).unwrap(); - job_handle.await.unwrap().unwrap(); - } - .instrument(info_span), - ); - - self.iroha = Some(receiver.recv().unwrap()); - time::sleep(Duration::from_millis(300)).await; - self.shutdown = Some(join_handle); + self.start_with_config_permissions_dir( + configuration, + genesis, + instruction_validator, + query_validator, + &temp_dir, + ) + .await; } /// Start peer with config diff --git a/data_model/src/transaction.rs b/data_model/src/transaction.rs index 7159fc37fba..790d9415ac4 100644 --- a/data_model/src/transaction.rs +++ b/data_model/src/transaction.rs @@ -67,14 +67,18 @@ pub trait Txn { let instruction_count: usize = instructions.iter().map(Instruction::len).sum(); if instruction_count as u64 > limits.max_instruction_number { - return Err(TransactionLimitError(String::from( - "Too many instructions in payload", + return Err(TransactionLimitError(format!( + "Too many instructions in payload, max number is {}", + limits.max_instruction_number ))); } } Executable::Wasm(WasmSmartContract { raw_data }) => { if raw_data.len() as u64 > limits.max_wasm_size_bytes { - return Err(TransactionLimitError(String::from("wasm binary too large"))); + return Err(TransactionLimitError(format!( + "Wasm binary too large, max size is {}", + limits.max_wasm_size_bytes + ))); } } } From 3335aa0c8eeecedd7f982459c9cd1e98c87a6279 Mon Sep 17 00:00:00 2001 From: Daniil Polyakov Date: Fri, 20 May 2022 00:08:15 +0300 Subject: [PATCH 2/6] Fix error message format in test Signed-off-by: Daniil Polyakov --- core/src/tx.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/core/src/tx.rs b/core/src/tx.rs index 2d29f91b8b4..a437fdfbf47 100644 --- a/core/src/tx.rs +++ b/core/src/tx.rs @@ -439,7 +439,10 @@ mod tests { ); assert_eq!( chain.next().unwrap().to_string(), - "Too many instructions in payload" + format!( + "Too many instructions in payload, max number is {}", + tx_limits.max_instruction_number + ) ); } } From a451e401ff0745cc5adb04dbffd39e946cc4bfc0 Mon Sep 17 00:00:00 2001 From: Daniil Polyakov Date: Fri, 20 May 2022 11:10:19 +0300 Subject: [PATCH 3/6] Fix temp dir lifetime bug Signed-off-by: Daniil Polyakov --- client/tests/integration/restart_peer.rs | 12 ++++++------ core/test_network/src/lib.rs | 10 ++++++---- 2 files changed, 12 insertions(+), 10 deletions(-) diff --git a/client/tests/integration/restart_peer.rs b/client/tests/integration/restart_peer.rs index 07c359aafbf..b82f0ef0b7c 100644 --- a/client/tests/integration/restart_peer.rs +++ b/client/tests/integration/restart_peer.rs @@ -1,6 +1,6 @@ #![allow(clippy::restriction)] -use std::{str::FromStr, thread, time::Duration}; +use std::{str::FromStr, sync::Arc, thread, time::Duration}; use eyre::Result; use iroha_client::client::{self, Client}; @@ -14,7 +14,7 @@ use super::Configuration; #[test] fn restarted_peer_should_have_the_same_asset_amount() -> Result<()> { - let temp_dir = TempDir::new()?; + let temp_dir = Arc::new(TempDir::new()?); let mut configuration = Configuration::test(); let mut peer = ::new()?; @@ -29,7 +29,7 @@ fn restarted_peer_should_have_the_same_asset_amount() -> Result<()> { GenesisNetwork::test(true), AllowAll, AllowAll, - &temp_dir, + Arc::clone(&temp_dir), )); let mut iroha_client = Client::test(&peer.api_address, &peer.telemetry_address); wait_for_genesis_committed(&vec![iroha_client.clone()], 0); @@ -40,7 +40,7 @@ fn restarted_peer_should_have_the_same_asset_amount() -> Result<()> { RegisterBox::new(AssetDefinition::quantity(asset_definition_id.clone()).build()); iroha_client.submit(create_asset)?; thread::sleep(pipeline_time * 2); - //When + // When let quantity: u32 = 200; let mint_asset = MintBox::new( Value::U32(quantity), @@ -52,7 +52,7 @@ fn restarted_peer_should_have_the_same_asset_amount() -> Result<()> { iroha_client.submit(mint_asset)?; thread::sleep(pipeline_time * 2); - //Then + // Then let asset = iroha_client .request(client::asset::by_account_id(account_id.clone()))? .into_iter() @@ -71,7 +71,7 @@ fn restarted_peer_should_have_the_same_asset_amount() -> Result<()> { GenesisNetwork::test(true), AllowAll, AllowAll, - &temp_dir, + temp_dir, )); let account_asset = iroha_client diff --git a/core/test_network/src/lib.rs b/core/test_network/src/lib.rs index 67bb5063d9e..c5239fa30d4 100644 --- a/core/test_network/src/lib.rs +++ b/core/test_network/src/lib.rs @@ -3,7 +3,7 @@ #![allow(clippy::restriction, clippy::future_not_send)] use core::{fmt::Debug, str::FromStr as _, time::Duration}; -use std::{collections::HashMap, thread}; +use std::{collections::HashMap, sync::Arc, thread}; use eyre::{Error, Result}; use futures::{prelude::*, stream::FuturesUnordered}; @@ -444,7 +444,7 @@ where genesis: Option, instruction_validator: impl Into> + Send + 'static, query_validator: impl Into> + Send + 'static, - temp_dir: &TempDir, + temp_dir: Arc, ) { let mut configuration = self.get_config(configuration); configuration @@ -463,6 +463,8 @@ where let (sender, receiver) = std::sync::mpsc::sync_channel(1); let handle = task::spawn( async move { + // Prevent temporary directory deleting + let _temp_dir = Arc::clone(&temp_dir); let mut iroha = >::with_genesis( genesis, configuration, @@ -500,13 +502,13 @@ where instruction_validator: impl Into> + Send + 'static, query_validator: impl Into> + Send + 'static, ) { - let temp_dir = TempDir::new().expect("Failed to create temp dir."); + let temp_dir = Arc::new(TempDir::new().expect("Failed to create temp dir.")); self.start_with_config_permissions_dir( configuration, genesis, instruction_validator, query_validator, - &temp_dir, + temp_dir, ) .await; } From 781830c0dda1ffeff8a161bdb238b9db0a1f03da Mon Sep 17 00:00:00 2001 From: Daniil Polyakov Date: Fri, 20 May 2022 11:33:06 +0300 Subject: [PATCH 4/6] Fix remarks from @appetrosyan Signed-off-by: Daniil Polyakov --- cli/src/lib.rs | 6 ++++-- core/src/genesis.rs | 2 +- core/src/sumeragi/fault.rs | 2 +- data_model/src/transaction.rs | 27 +++++++++++++++++++-------- 4 files changed, 25 insertions(+), 12 deletions(-) diff --git a/cli/src/lib.rs b/cli/src/lib.rs index 7a9ec95dac9..1251bb9e2e6 100644 --- a/cli/src/lib.rs +++ b/cli/src/lib.rs @@ -155,8 +155,10 @@ where telemetry: Option, ) -> Result { if !config.disable_panic_terminal_colors { - // Ignoring error if `iroha_logger` has already installed colors - let _color_res = color_eyre::install(); + if let Err(e) = color_eyre::install() { + let error_message = format!("{e:#}"); + iroha_logger::error!(error = %error_message, "Tried to install eyre_hook twice",); + } } let listen_addr = config.torii.p2p_addr.clone(); iroha_logger::info!(%listen_addr, "Starting peer"); diff --git a/core/src/genesis.rs b/core/src/genesis.rs index fce59bd3691..9d9874762ca 100644 --- a/core/src/genesis.rs +++ b/core/src/genesis.rs @@ -200,7 +200,7 @@ impl GenesisNetworkTrait for GenesisNetwork { .filter_map(|(i, res)| { res.map_err(|error| { let error_msg = format!("{error:#}"); - iroha_logger::error!(error = %error_msg, "Genesis transaction #{i} didn't succeed") + iroha_logger::error!(error = %error_msg, "Genesis transaction #{i} failed") }) .ok() }) diff --git a/core/src/sumeragi/fault.rs b/core/src/sumeragi/fault.rs index f05fb285780..2750baaceae 100644 --- a/core/src/sumeragi/fault.rs +++ b/core/src/sumeragi/fault.rs @@ -467,7 +467,7 @@ impl ) -> Result<()> { if transactions.is_empty() { Err(eyre!( - "Genesis transactions set is empty or all of the transactions are incorrect" + "Genesis transactions set contains no valid transactions" )) } else if genesis_topology.leader() != &self.peer_id { Err(eyre!( diff --git a/data_model/src/transaction.rs b/data_model/src/transaction.rs index 790d9415ac4..051e23d9519 100644 --- a/data_model/src/transaction.rs +++ b/data_model/src/transaction.rs @@ -61,23 +61,34 @@ pub trait Txn { /// /// Fails if number of instructions or wasm size exceeds maximum #[inline] + #[allow(clippy::expect_used)] fn check_limits(&self, limits: &TransactionLimits) -> Result<(), TransactionLimitError> { match &self.payload().instructions { Executable::Instructions(instructions) => { - let instruction_count: usize = instructions.iter().map(Instruction::len).sum(); - - if instruction_count as u64 > limits.max_instruction_number { + let instruction_count: u64 = instructions + .iter() + .map(Instruction::len) + .sum::() + .try_into() + .expect("`usize` should always fit in `u64`"); + + if instruction_count > limits.max_instruction_number { return Err(TransactionLimitError(format!( - "Too many instructions in payload, max number is {}", - limits.max_instruction_number + "Too many instructions in payload, max number is {}, but got {}", + limits.max_instruction_number, instruction_count ))); } } Executable::Wasm(WasmSmartContract { raw_data }) => { - if raw_data.len() as u64 > limits.max_wasm_size_bytes { + let len: u64 = raw_data + .len() + .try_into() + .expect("`usize` should always fit in `u64`"); + + if len > limits.max_wasm_size_bytes { return Err(TransactionLimitError(format!( - "Wasm binary too large, max size is {}", - limits.max_wasm_size_bytes + "Wasm binary too large, max size is {}, but got {}", + limits.max_wasm_size_bytes, len ))); } } From 94c2de45c8878fec33c9158df61d504711efefc2 Mon Sep 17 00:00:00 2001 From: Daniil Polyakov Date: Fri, 20 May 2022 14:05:49 +0300 Subject: [PATCH 5/6] Fix string check in test one more time Signed-off-by: Daniil Polyakov --- core/src/tx.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/core/src/tx.rs b/core/src/tx.rs index a437fdfbf47..5e2d72d6ba7 100644 --- a/core/src/tx.rs +++ b/core/src/tx.rs @@ -440,8 +440,9 @@ mod tests { assert_eq!( chain.next().unwrap().to_string(), format!( - "Too many instructions in payload, max number is {}", - tx_limits.max_instruction_number + "Too many instructions in payload, max number is {}, but got {}", + tx_limits.max_instruction_number, + DEFAULT_MAX_INSTRUCTION_NUMBER + 1 ) ); } From c206266721c15d25fd2f8e9dbdabe7da7abdf0eb Mon Sep 17 00:00:00 2001 From: Daniil Polyakov Date: Fri, 20 May 2022 14:07:25 +0300 Subject: [PATCH 6/6] Wrap `QueryResult` in box Signed-off-by: Daniil Polyakov --- cli/src/torii/tests.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cli/src/torii/tests.rs b/cli/src/torii/tests.rs index 82e4767234c..091d161934b 100644 --- a/cli/src/torii/tests.rs +++ b/cli/src/torii/tests.rs @@ -195,7 +195,7 @@ impl From> for QueryResponseBody if StatusCode::OK == src.status() { let body = VersionedQueryResult::decode_versioned(src.body()) .expect("The response body failed to be decoded to VersionedQueryResult even though the status is Ok 200"); - Self::Ok(body) + Self::Ok(Box::new(body)) } else { let body = query::Error::decode(&mut src.body().as_ref()) .expect("The response body failed to be decoded to query::Error even though the status is not Ok 200"); @@ -213,7 +213,7 @@ struct QueryResponseTest { #[allow(variant_size_differences)] enum QueryResponseBody { - Ok(VersionedQueryResult), + Ok(Box), Err(query::Error), }