From cb60164b9fc5c6856387aa798237a75cc46dd346 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marin=20Ver=C5=A1i=C4=87?= Date: Tue, 12 Dec 2023 19:45:43 +0300 Subject: [PATCH] [fix] #4140: Fix registration of new peer MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Marin Veršić --- cli/src/main.rs | 3 +- client/tests/integration/connected_peers.rs | 126 ++++++++++++++------ client/tests/integration/mod.rs | 5 +- client/tests/integration/offline_peers.rs | 58 ++++++++- client/tests/integration/restart_peer.rs | 2 +- core/src/block.rs | 37 +++--- core/src/gossiper.rs | 1 - core/src/sumeragi/main_loop.rs | 23 ++-- core/src/sumeragi/mod.rs | 6 +- core/src/wsv.rs | 6 - core/test_network/src/lib.rs | 2 +- docker-compose.single.yml | 1 + docker-compose.stable.single.yml | 2 +- docker-compose.stable.yml | 8 +- docker-compose.yml | 4 + p2p/src/network.rs | 2 +- telemetry/src/metrics.rs | 8 +- 17 files changed, 195 insertions(+), 99 deletions(-) diff --git a/cli/src/main.rs b/cli/src/main.rs index 16629ea7ea7..b2e4d118d94 100644 --- a/cli/src/main.rs +++ b/cli/src/main.rs @@ -141,7 +141,8 @@ async fn main() -> Result<(), color_eyre::Report> { "Hyperledgerいろは2にようこそ！(translation) Welcome to Hyperledger Iroha!" ); - assert!(args.submit_genesis || config.sumeragi.trusted_peers.peers.len() > 1, + let trusted_peers = &config.sumeragi.trusted_peers.peers; + assert!(args.submit_genesis || trusted_peers.len() > 1 || trusted_peers.len() == 1 && !trusted_peers.contains(&config.sumeragi.peer_id), "Only peer in network, yet required to receive genesis topology. This is a configuration error." ); diff --git a/client/tests/integration/connected_peers.rs b/client/tests/integration/connected_peers.rs index 0ad808b20c2..10c2f36acd1 100644 --- a/client/tests/integration/connected_peers.rs +++ b/client/tests/integration/connected_peers.rs @@ -1,15 +1,15 @@ use std::thread; use eyre::{Context, Result}; -use iroha_client::{ - client::Client, - data_model::{ - parameter::{default::MAX_TRANSACTIONS_IN_BLOCK, ParametersBuilder}, - peer::Peer as DataModelPeer, - prelude::*, - }, +use iroha_client::{client::Client, data_model::peer::Peer as DataModelPeer}; +use iroha_data_model::{ + isi::{RegisterExpr, UnregisterExpr}, + IdBox, }; +use iroha_primitives::unique_vec; +use rand::{seq::SliceRandom, thread_rng, Rng}; use test_network::*; +use tokio::runtime::Runtime; use super::Configuration; @@ -24,11 +24,50 @@ fn connected_peers_with_f_1_0_1() -> Result<()> { connected_peers_with_f(1, Some(11_000)) } +#[test] +fn register_new_peer() -> Result<()> { + let (_rt, network, _) = ::start_test_with_runtime(4, Some(11_040)); + wait_for_genesis_committed(&network.clients(), 0); + let pipeline_time = Configuration::pipeline_time(); + + let mut peer_clients: Vec<_> = Network::peers(&network) + .zip(Network::clients(&network)) + .collect(); + + check_status(&peer_clients, 1); + + // Start new peer + let mut configuration = Configuration::test(); + configuration.sumeragi.trusted_peers.peers = + unique_vec![peer_clients.choose(&mut thread_rng()).unwrap().0.id.clone()]; + let rt = Runtime::test(); + let new_peer = rt.block_on( + PeerBuilder::new() + .with_configuration(configuration.clone()) + .with_into_genesis(WithGenesis::None) + .with_port(10_000) + .start(), + ); + + let register_peer = RegisterExpr::new(DataModelPeer::new(new_peer.id.clone())); + peer_clients + .choose(&mut thread_rng()) + .unwrap() + .1 + .submit_blocking(register_peer)?; + peer_clients.push((&new_peer, Client::test(&new_peer.api_address))); + thread::sleep(pipeline_time * 2); // Wait for some time to allow peers to connect + + check_status(&peer_clients, 2); + + Ok(()) +} + /// Test the number of connected peers, changing the number of faults tolerated down and up fn connected_peers_with_f(faults: u64, start_port: Option) -> Result<()> { let n_peers = 3 * faults + 1; - let (_rt, network, client) = ::start_test_with_runtime( + let (_rt, network, _) = ::start_test_with_runtime( (n_peers) .try_into() .wrap_err("`faults` argument `u64` value too high, cannot convert to `u32`")?, @@ -37,40 +76,51 @@ fn connected_peers_with_f(faults: u64, start_port: Option) -> Result<()> { wait_for_genesis_committed(&network.clients(), 0); let pipeline_time = Configuration::pipeline_time(); - client.submit_blocking( - ParametersBuilder::new() - .add_parameter(MAX_TRANSACTIONS_IN_BLOCK, 1u32)? - .into_set_parameters(), - )?; + let mut peer_clients: Vec<_> = Network::peers(&network) + .zip(Network::clients(&network)) + .collect(); - // Confirm all peers connected - let mut status = client.get_status()?; - assert_eq!(status.peers, n_peers - 1); - assert_eq!(status.blocks, 2); + check_status(&peer_clients, 1); - // Unregister a peer: committed with f = `faults` - // then `status.peers` decrements - let peer = network.peers.values().last().unwrap(); - let peer_client = Client::test(&peer.api_address); - let unregister_peer = UnregisterExpr::new(IdBox::PeerId(peer.id.clone())); - client.submit_blocking(unregister_peer)?; + // Unregister a peer: committed with f = `faults` then `status.peers` decrements + let removed_peer_idx = rand::thread_rng().gen_range(0..peer_clients.len()); + let (removed_peer, _) = &peer_clients[removed_peer_idx]; + let unregister_peer = UnregisterExpr::new(IdBox::PeerId(removed_peer.id.clone())); + peer_clients + .choose(&mut thread_rng()) + .unwrap() + .1 + .submit_blocking(unregister_peer)?; thread::sleep(pipeline_time * 2); // Wait for some time to allow peers to connect - status = client.get_status()?; - assert_eq!(status.peers, n_peers - 2); - assert_eq!(status.blocks, 3); - status = peer_client.get_status()?; + let (removed_peer, removed_peer_client) = peer_clients.remove(removed_peer_idx); + + check_status(&peer_clients, 2); + let status = removed_peer_client.get_status()?; + assert_eq!(status.blocks, 2); assert_eq!(status.peers, 0); - // Re-register the peer: committed with f = `faults` - 1 then - // `status.peers` increments - let register_peer = RegisterExpr::new(DataModelPeer::new(peer.id.clone())); - client.submit_blocking(register_peer)?; - thread::sleep(pipeline_time * 4); // Wait for some time to allow peers to connect - status = client.get_status()?; - assert_eq!(status.peers, n_peers - 1); - assert_eq!(status.blocks, 4); - status = peer_client.get_status()?; - assert_eq!(status.peers, n_peers - 1); - assert_eq!(status.blocks, 4); + // Re-register the peer: committed with f = `faults` - 1 then `status.peers` increments + let register_peer = RegisterExpr::new(DataModelPeer::new(removed_peer.id.clone())); + peer_clients + .choose(&mut thread_rng()) + .unwrap() + .1 + .submit_blocking(register_peer)?; + peer_clients.insert(removed_peer_idx, (removed_peer, removed_peer_client)); + thread::sleep(pipeline_time * 2); // Wait for some time to allow peers to connect + + check_status(&peer_clients, 3); + Ok(()) } + +fn check_status(peer_clients: &[(&Peer, Client)], expected_blocks: u64) { + let n_peers = peer_clients.len() as u64; + + for (_, peer_client) in peer_clients { + let status = peer_client.get_status().unwrap(); + + assert_eq!(status.peers, n_peers - 1); + assert_eq!(status.blocks, expected_blocks); + } +} diff --git a/client/tests/integration/mod.rs b/client/tests/integration/mod.rs index 5bb44460bb0..a7b0bc1bbdf 100644 --- a/client/tests/integration/mod.rs +++ b/client/tests/integration/mod.rs @@ -1,7 +1,4 @@ -pub use iroha_config::{ - base::proxy::Builder, - iroha::{Configuration, ConfigurationProxy}, -}; +pub use iroha_config::iroha::Configuration; mod add_account; mod add_domain; diff --git a/client/tests/integration/offline_peers.rs b/client/tests/integration/offline_peers.rs index 86838146000..2f357afee81 100644 --- a/client/tests/integration/offline_peers.rs +++ b/client/tests/integration/offline_peers.rs @@ -1,14 +1,19 @@ -use eyre::Result; +use eyre::{Context, Result}; use iroha_client::{ - client::{self, QueryResult}, + client::{self, Client, QueryResult}, data_model::{ parameter::{default::MAX_TRANSACTIONS_IN_BLOCK, ParametersBuilder}, + peer::Peer as DataModelPeer, prelude::*, }, }; +use iroha_crypto::KeyPair; +use iroha_data_model::peer::PeerId; use test_network::*; use tokio::runtime::Runtime; +use super::Configuration; + #[test] fn genesis_block_is_committed_with_some_offline_peers() -> Result<()> { // Given @@ -43,3 +48,52 @@ fn genesis_block_is_committed_with_some_offline_peers() -> Result<()> { assert_eq!(AssetValue::Quantity(alice_has_roses), *asset.value()); Ok(()) } + +#[test] +fn register_offline_peer() -> Result<()> { + let n_peers = 3 * 1 + 1; + + let (_rt, network, client) = ::start_test_with_runtime( + (n_peers) + .try_into() + .wrap_err("`faults` argument `u64` value too high, cannot convert to `u32`")?, + Some(11_080), + ); + wait_for_genesis_committed(&network.clients(), 0); + let pipeline_time = Configuration::pipeline_time(); + + let peer_clients: Vec<_> = network + .peers + .values() + .chain(core::iter::once(&network.genesis)) + .map(|peer| Client::test(&peer.api_address)) + .collect(); + + check_status(&peer_clients, 1); + + let address = "128.0.0.2:8085".parse()?; + let key_pair = KeyPair::generate().unwrap(); + let public_key = key_pair.public_key().clone(); + let peer_id = PeerId::new(&address, &public_key); + let register_peer = RegisterExpr::new(DataModelPeer::new(peer_id)); + + // Wait for some time to allow peers to connect + client.submit_blocking(register_peer)?; + std::thread::sleep(pipeline_time * 2); + + // Make sure status hasn't change + check_status(&peer_clients, 2); + + Ok(()) +} + +fn check_status(peer_clients: &[Client], expected_blocks: u64) { + let n_peers = peer_clients.len() as u64; + + for peer_client in peer_clients { + let status = peer_client.get_status().unwrap(); + + assert_eq!(status.peers, n_peers - 1); + assert_eq!(status.blocks, expected_blocks); + } +} diff --git a/client/tests/integration/restart_peer.rs b/client/tests/integration/restart_peer.rs index cfe153e3c9d..02b7e101f3a 100644 --- a/client/tests/integration/restart_peer.rs +++ b/client/tests/integration/restart_peer.rs @@ -17,7 +17,7 @@ fn restarted_peer_should_have_the_same_asset_amount() -> Result<()> { let temp_dir = Arc::new(TempDir::new()?); let mut configuration = Configuration::test(); - let mut peer = ::new().with_port(10_000).build()?; + let mut peer = PeerBuilder::new().with_port(10_000).build()?; configuration.sumeragi.trusted_peers.peers = unique_vec![peer.id.clone()]; let account_id = AccountId::from_str("alice@wonderland").unwrap(); diff --git a/core/src/block.rs b/core/src/block.rs index 9322d16400d..6c6d90361cf 100644 --- a/core/src/block.rs +++ b/core/src/block.rs @@ -267,27 +267,28 @@ mod valid { topology: &Topology, wsv: &mut WorldStateView, ) -> Result { - let actual_commit_topology = &block.payload().commit_topology; - let expected_commit_topology = &topology.ordered_peers; - - if actual_commit_topology != expected_commit_topology { - let actual_commit_topology = actual_commit_topology.clone(); - - return Err(( - block, - BlockValidationError::TopologyMismatch { - expected: expected_commit_topology.clone(), - actual: actual_commit_topology, - }, - )); - } + if !block.payload().header.is_genesis() { + let actual_commit_topology = &block.payload().commit_topology; + let expected_commit_topology = &topology.ordered_peers; + + if actual_commit_topology != expected_commit_topology { + let actual_commit_topology = actual_commit_topology.clone(); + + return Err(( + block, + BlockValidationError::TopologyMismatch { + expected: expected_commit_topology.clone(), + actual: actual_commit_topology, + }, + )); + } - if !block.payload().header.is_genesis() - && topology + if topology .filter_signatures_by_roles(&[Role::Leader], block.signatures()) .is_empty() - { - return Err((block, SignatureVerificationError::LeaderMissing.into())); + { + return Err((block, SignatureVerificationError::LeaderMissing.into())); + } } let expected_block_height = wsv.height() + 1; diff --git a/core/src/gossiper.rs b/core/src/gossiper.rs index 5856dfd4a1b..365ebb7ac7a 100644 --- a/core/src/gossiper.rs +++ b/core/src/gossiper.rs @@ -100,7 +100,6 @@ impl TransactionGossiper { .n_random_transactions(self.gossip_batch_size, &self.wsv); if txs.is_empty() { - iroha_logger::debug!("Nothing to gossip"); return; } diff --git a/core/src/sumeragi/main_loop.rs b/core/src/sumeragi/main_loop.rs index 441c0946b2d..f7abbe2df36 100644 --- a/core/src/sumeragi/main_loop.rs +++ b/core/src/sumeragi/main_loop.rs @@ -186,7 +186,7 @@ impl Sumeragi { &mut self, shutdown_receiver: &mut tokio::sync::oneshot::Receiver<()>, ) -> Result<(), EarlyReturn> { - trace!("Listen for genesis"); + info!(addr = %self.peer_id.address, "Listen for genesis"); loop { std::thread::sleep(Duration::from_millis(50)); @@ -223,6 +223,8 @@ impl Sumeragi { } }; + new_wsv.world_mut().trusted_peers_ids = + block.payload().commit_topology.clone(); self.commit_block(block, new_wsv); return Err(EarlyReturn::GenesisBlockReceivedAndCommitted); } @@ -295,7 +297,7 @@ impl Sumeragi { info!( addr=%self.peer_id.address, role=%self.current_topology.role(&self.peer_id), - block_height=%self.wsv.height(), + block_height=%block.payload().header.height, block_hash=%block.hash(), "{}", Strategy::LOG_MESSAGE, ); @@ -313,11 +315,8 @@ impl Sumeragi { // Parameters are updated before updating public copy of sumeragi self.update_params(); - let new_topology = Topology::recreate_topology( - block.as_ref(), - 0, - self.wsv.peers_ids().iter().cloned().collect(), - ); + let new_topology = + Topology::recreate_topology(block.as_ref(), 0, self.wsv.peers().cloned().collect()); let events = block.produce_events(); // https://github.com/hyperledger/iroha/issues/3396 @@ -801,10 +800,10 @@ pub(crate) fn run( }; span.exit(); - trace!( - me=%sumeragi.peer_id.public_key, + info!( + addr=%sumeragi.peer_id.address, role_in_next_round=%sumeragi.current_topology.role(&sumeragi.peer_id), - "Finished sumeragi init.", + "Sumeragi initialized", ); let mut voting_block = None; @@ -1125,7 +1124,7 @@ fn handle_block_sync( let last_committed_block = new_wsv .latest_block_ref() .expect("Not in genesis round so must have at least genesis block"); - let new_peers = new_wsv.peers_ids().clone(); + let new_peers = new_wsv.peers().cloned().collect(); let view_change_index = block.payload().header().view_change_index; Topology::recreate_topology(&last_committed_block, view_change_index, new_peers) }; @@ -1145,7 +1144,7 @@ fn handle_block_sync( let last_committed_block = new_wsv .latest_block_ref() .expect("Not in genesis round so must have at least genesis block"); - let new_peers = new_wsv.peers_ids().clone(); + let new_peers = new_wsv.peers().cloned().collect(); let view_change_index = block.payload().header().view_change_index; Topology::recreate_topology(&last_committed_block, view_change_index, new_peers) }; diff --git a/core/src/sumeragi/mod.rs b/core/src/sumeragi/mod.rs index b6f3c7391f0..8ed08194753 100644 --- a/core/src/sumeragi/mod.rs +++ b/core/src/sumeragi/mod.rs @@ -261,11 +261,7 @@ impl SumeragiHandle { "Sumeragi could not load block that was reported as present. \ Please check that the block storage was not disconnected.", ); - Topology::recreate_topology( - &block_ref, - 0, - wsv.peers_ids().iter().cloned().collect(), - ) + Topology::recreate_topology(&block_ref, 0, wsv.peers().cloned().collect()) } }; diff --git a/core/src/wsv.rs b/core/src/wsv.rs index 13a899845e9..32153e8ca3b 100644 --- a/core/src/wsv.rs +++ b/core/src/wsv.rs @@ -858,12 +858,6 @@ impl WorldStateView { &mut self.world } - /// Returns reference for trusted peer ids - #[inline] - pub fn peers_ids(&self) -> &PeersIds { - &self.world.trusted_peers_ids - } - /// Return an iterator over blockchain block hashes starting with the block of the given `height` pub fn block_hashes_from_height(&self, height: usize) -> Vec> { self.block_hashes diff --git a/core/test_network/src/lib.rs b/core/test_network/src/lib.rs index 96c4210fd08..b60e13f68d6 100644 --- a/core/test_network/src/lib.rs +++ b/core/test_network/src/lib.rs @@ -556,7 +556,7 @@ impl PeerBuilder { /// Set Iroha configuration #[must_use] pub fn with_configuration(mut self, configuration: Configuration) -> Self { - self.configuration.replace(configuration); + self.configuration = Some(configuration); self } diff --git a/docker-compose.single.yml b/docker-compose.single.yml index d46667110c2..454f92ff312 100644 --- a/docker-compose.single.yml +++ b/docker-compose.single.yml @@ -3,6 +3,7 @@ services: iroha0: build: . image: iroha2:lts + platform: linux/amd64 environment: TORII_P2P_ADDR: iroha0:1337 TORII_API_URL: iroha0:8080 diff --git a/docker-compose.stable.single.yml b/docker-compose.stable.single.yml index e2c07b8e2fc..8950aea4a3e 100644 --- a/docker-compose.stable.single.yml +++ b/docker-compose.stable.single.yml @@ -3,6 +3,7 @@ services: iroha0: build: . image: iroha2:stable + platform: linux/amd64 environment: TORII_P2P_ADDR: iroha0:1337 TORII_API_URL: iroha0:8080 @@ -18,7 +19,6 @@ services: ports: - "1337:1337" - "8080:8080" - - "8180:8180" init: true command: iroha --submit-genesis volumes: diff --git a/docker-compose.stable.yml b/docker-compose.stable.yml index e57463504fc..3ac16ce8505 100644 --- a/docker-compose.stable.yml +++ b/docker-compose.stable.yml @@ -2,6 +2,7 @@ version: "3.8" services: iroha0: image: hyperledger/iroha2:stable + platform: linux/amd64 environment: TORII_P2P_ADDR: iroha0:1337 TORII_API_URL: iroha0:8080 @@ -17,7 +18,6 @@ services: ports: - "1337:1337" - "8080:8080" - - "8180:8180" volumes: - './configs/peer/stable:/config' init: true @@ -25,6 +25,7 @@ services: iroha1: image: hyperledger/iroha2:stable + platform: linux/amd64 environment: TORII_P2P_ADDR: iroha1:1338 TORII_API_URL: iroha1:8081 @@ -39,13 +40,13 @@ services: ports: - "1338:1338" - "8081:8081" - - "8181:8181" volumes: - './configs/peer/stable:/config' init: true iroha2: image: hyperledger/iroha2:stable + platform: linux/amd64 environment: TORII_P2P_ADDR: iroha2:1339 TORII_API_URL: iroha2:8082 @@ -60,13 +61,13 @@ services: ports: - "1339:1339" - "8082:8082" - - "8182:8182" volumes: - './configs/peer/stable:/config' init: true iroha3: image: hyperledger/iroha2:stable + platform: linux/amd64 environment: TORII_P2P_ADDR: iroha3:1340 TORII_API_URL: iroha3:8083 @@ -81,7 +82,6 @@ services: ports: - "1340:1340" - "8083:8083" - - "8183:8183" volumes: - './configs/peer/stable:/config' init: true diff --git a/docker-compose.yml b/docker-compose.yml index e781607ee9a..d24025c2fb3 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -2,6 +2,7 @@ version: "3.8" services: iroha0: image: hyperledger/iroha2:lts + platform: linux/amd64 environment: TORII_P2P_ADDR: iroha0:1337 TORII_API_URL: iroha0:8080 @@ -25,6 +26,7 @@ services: iroha1: image: hyperledger/iroha2:lts + platform: linux/amd64 environment: TORII_P2P_ADDR: iroha1:1338 TORII_API_URL: iroha1:8081 @@ -46,6 +48,7 @@ services: iroha2: image: hyperledger/iroha2:lts + platform: linux/amd64 environment: TORII_P2P_ADDR: iroha2:1339 TORII_API_URL: iroha2:8082 @@ -67,6 +70,7 @@ services: iroha3: image: hyperledger/iroha2:lts + platform: linux/amd64 environment: TORII_P2P_ADDR: iroha3:1340 TORII_API_URL: iroha3:8083 diff --git a/p2p/src/network.rs b/p2p/src/network.rs index 751eb779d3d..51b97d661e2 100644 --- a/p2p/src/network.rs +++ b/p2p/src/network.rs @@ -366,7 +366,7 @@ impl NetworkBase { }: Connected, ) { if !self.current_topology.contains_key(&peer_id) { - iroha_logger::warn!(topology=?self.current_topology, "Peer not present in topology is trying to connect"); + iroha_logger::warn!(%peer_id, topology=?self.current_topology, "Peer not present in topology is trying to connect"); return; } diff --git a/telemetry/src/metrics.rs b/telemetry/src/metrics.rs index 1043cbb9954..ad4f7744750 100644 --- a/telemetry/src/metrics.rs +++ b/telemetry/src/metrics.rs @@ -35,10 +35,10 @@ impl Encode for Uptime { /// Response body for GET status request #[derive(Clone, Copy, Debug, Default, Deserialize, Serialize, Encode)] pub struct Status { - /// Number of connected peers, except for the reporting peer itself + /// Number of currently connected peers excluding the reporting peer #[codec(compact)] pub peers: u64, - /// Number of committed blocks + /// Number of committed blocks (blockchain height) #[codec(compact)] pub blocks: u64, /// Number of accepted transactions @@ -77,9 +77,9 @@ impl> From<&T> for Status { pub struct Metrics { /// Total number of transactions pub txs: IntCounterVec, - /// Current block height + /// Number of committed blocks (blockchain height) pub block_height: IntCounter, - /// Total number of currently connected peers + /// Number of currently connected peers excluding the reporting peer pub connected_peers: GenericGauge, /// Uptime of the network, starting from commit of the genesis block pub uptime_since_genesis_ms: GenericGauge,