Specify topology for n-ary chain tests (#4039)

* Refactor client, connection and channel for nary chain bootstrapping in order to use a specified topology

* Add changelog entry

* Apply suggestions from code review

Co-authored-by: Romain Ruetschi <[email protected]>
Signed-off-by: Luca Joss <[email protected]>

* Move parsing of string to TopologyType outside of bootstrap_topology method

* Add TwoDimHashMap struct instead of using HashMap<HashMap<>>

* Add option to override the topology for specific tests

* Add cyclic topology implementation

* Update changelog entry

* Fix codespell

* Refactor TwoDimHashMap to TwoDimMap and use BTreeMap instead of HashMap

* Use TwoDimMap iter() implementation for channels

* Use TwoDimMap iter() implementation for connections

* Use TwoDimMap iter() implementation for foreign_clients

* Remove unnecessary bool in TwoDimMap iterator

* Update method docstring

---------

Signed-off-by: Luca Joss <[email protected]>
Co-authored-by: Romain Ruetschi <[email protected]>

.changelog/unreleased/improvements/ibc-integration-test/4099-specify-topology-for-nary-tests.md

@@ -0,0 +1,4 @@
+- Refactored the test-framework bootstrapping for n-ary chain tests
+  to utilize the specified topology.
+    * Currently, only linear, cyclic and fully connected topologies are supported.
+  ([\#4038](https://github.com/informalsystems/hermes/issues/4038))

tools/integration-test/src/tests/forward/forward_hop_transfer.rs

@@ -53,7 +53,7 @@ impl NaryChannelTest<4> for IbcForwardHopTransferTest {
         chains: NaryConnectedChains<Handle, 4>,
         channels: NaryConnectedChannels<Handle, 4>,
     ) -> Result<(), Error> {
-        let connected_chains = chains.connected_chains_at::<0, 3>()?;
+        let connected_chains = chains.connected_chains_at::<0, 1>()?;
 
         let node_a = chains.full_node_at::<0>()?;
         let node_b = chains.full_node_at::<1>()?;
@@ -174,7 +174,7 @@ impl NaryChannelTest<4> for AtomicIbcForwardHopTransferTest {
         chains: NaryConnectedChains<Handle, 4>,
         channels: NaryConnectedChannels<Handle, 4>,
     ) -> Result<(), Error> {
-        let connected_chains = chains.connected_chains_at::<0, 3>()?;
+        let connected_chains = chains.connected_chains_at::<0, 1>()?;
 
         let node_a = chains.full_node_at::<0>()?;
         let node_b = chains.full_node_at::<1>()?;

tools/integration-test/src/tests/forward/forward_transfer.rs

@@ -73,7 +73,7 @@ impl NaryChannelTest<3> for IbcForwardTransferTest {
         chains: NaryConnectedChains<Handle, 3>,
         channels: NaryConnectedChannels<Handle, 3>,
     ) -> Result<(), Error> {
-        let connected_chains = chains.connected_chains_at::<0, 2>()?;
+        let connected_chains = chains.connected_chains_at::<0, 1>()?;
 
         let node_a = chains.full_node_at::<0>()?;
         let node_b = chains.full_node_at::<1>()?;
@@ -176,7 +176,7 @@ impl NaryChannelTest<3> for MisspelledMemoFieldsIbcForwardTransferTest {
         chains: NaryConnectedChains<Handle, 3>,
         channels: NaryConnectedChannels<Handle, 3>,
     ) -> Result<(), Error> {
-        let connected_chains = chains.connected_chains_at::<0, 2>()?;
+        let connected_chains = chains.connected_chains_at::<0, 1>()?;
 
         let node_a = chains.full_node_at::<0>()?;
         let node_b = chains.full_node_at::<1>()?;
@@ -422,7 +422,7 @@ impl NaryChannelTest<3> for MisspelledMemoContentIbcForwardTransferTest {
         chains: NaryConnectedChains<Handle, 3>,
         channels: NaryConnectedChannels<Handle, 3>,
     ) -> Result<(), Error> {
-        let connected_chains = chains.connected_chains_at::<0, 2>()?;
+        let connected_chains = chains.connected_chains_at::<0, 1>()?;
 
         let node_a = chains.full_node_at::<0>()?;
         let node_b = chains.full_node_at::<1>()?;

tools/integration-test/src/tests/ternary_transfer.rs

@@ -1,4 +1,5 @@
 use ibc_test_framework::prelude::*;
+use ibc_test_framework::types::topology::TopologyType;
 
 #[test]
 fn test_ternary_ibc_transfer() -> Result<(), Error> {
@@ -11,6 +12,10 @@ impl TestOverrides for TernaryIbcTransferTest {
     fn modify_relayer_config(&self, config: &mut Config) {
         config.mode.clients.misbehaviour = false;
     }
+
+    fn topology(&self) -> Option<TopologyType> {
+        Some(TopologyType::Cyclic)
+    }
 }
 
 impl PortsOverride<3> for TernaryIbcTransferTest {}

tools/test-framework/src/bootstrap/nary/chain.rs

@@ -4,29 +4,33 @@
 
 use ibc_relayer::chain::handle::ChainHandle;
 use ibc_relayer::config::Config;
-use ibc_relayer::foreign_client::ForeignClient;
 use ibc_relayer::registry::SharedRegistry;
 
 use crate::bootstrap::binary::chain::{
-    add_chain_config, add_keys_to_chain_handle, bootstrap_foreign_client, new_registry,
-    save_relayer_config,
+    add_chain_config, add_keys_to_chain_handle, new_registry, save_relayer_config,
 };
 use crate::error::{handle_generic_error, Error};
 use crate::relayer::driver::RelayerDriver;
 use crate::types::config::TestConfig;
 use crate::types::nary::chains::{DynamicConnectedChains, NaryConnectedChains};
 use crate::types::single::node::FullNode;
+use crate::types::topology::{bootstrap_topology, TopologyType};
 
 /**
   Bootstrap a fixed number of chains specified by `SIZE`.
 */
 pub fn boostrap_chains_with_nodes<const SIZE: usize>(
     test_config: &TestConfig,
     full_nodes: [FullNode; SIZE],
+    topology_override: Option<TopologyType>,
     config_modifier: impl FnOnce(&mut Config),
 ) -> Result<(RelayerDriver, NaryConnectedChains<impl ChainHandle, SIZE>), Error> {
-    let (relayer, chains) =
-        boostrap_chains_with_any_nodes(test_config, full_nodes.into(), config_modifier)?;
+    let (relayer, chains) = boostrap_chains_with_any_nodes(
+        test_config,
+        full_nodes.into(),
+        topology_override,
+        config_modifier,
+    )?;
 
     Ok((relayer, chains.try_into()?))
 }
@@ -38,22 +42,30 @@ pub fn boostrap_chains_with_nodes<const SIZE: usize>(
 pub fn boostrap_chains_with_self_connected_node<const SIZE: usize>(
     test_config: &TestConfig,
     full_node: FullNode,
+    topology_override: Option<TopologyType>,
     config_modifier: impl FnOnce(&mut Config),
 ) -> Result<(RelayerDriver, NaryConnectedChains<impl ChainHandle, SIZE>), Error> {
     let full_nodes = vec![full_node; SIZE];
-    let (relayer, chains) =
-        boostrap_chains_with_any_nodes(test_config, full_nodes, config_modifier)?;
+    let (relayer, chains) = boostrap_chains_with_any_nodes(
+        test_config,
+        full_nodes,
+        topology_override,
+        config_modifier,
+    )?;
 
     Ok((relayer, chains.try_into()?))
 }
 
 /**
    Bootstrap a dynamic number of chains, according to the number of full nodes
    in the `Vec<FullNode>`.
+   The topology will be retrieved and set in this method,
+   see [`crate::types::topology`] for more information.
 */
 pub fn boostrap_chains_with_any_nodes(
     test_config: &TestConfig,
     full_nodes: Vec<FullNode>,
+    topology_override: Option<TopologyType>,
     config_modifier: impl FnOnce(&mut Config),
 ) -> Result<(RelayerDriver, DynamicConnectedChains<impl ChainHandle>), Error> {
     let mut config = Config::default();
@@ -77,19 +89,24 @@ pub fn boostrap_chains_with_any_nodes(
         chain_handles.push(handle);
     }
 
-    let mut foreign_clients: Vec<Vec<ForeignClient<_, _>>> = Vec::new();
-
-    for handle_a in chain_handles.iter() {
-        let mut foreign_clients_b = Vec::new();
-
-        for handle_b in chain_handles.iter() {
-            let foreign_client = bootstrap_foreign_client(handle_a, handle_b, Default::default())?;
-
-            foreign_clients_b.push(foreign_client);
+    // Retrieve the topology or fallback to the Linear topology
+    let topology_type = if let Some(topology_type) = topology_override {
+        topology_type
+    } else {
+        let topology_str = std::env::var("TOPOLOGY").unwrap_or_else(|_| "linear".to_owned());
+        match topology_str.parse() {
+            Ok(topology_type) => topology_type,
+            Err(_) => {
+                tracing::warn!(
+                    "Failed to parse topology type `{topology_str}`. Will fallback to Linear topology"
+                );
+                TopologyType::Linear
+            }
         }
+    };
+    let topology = bootstrap_topology(topology_type);
 
-        foreign_clients.push(foreign_clients_b);
-    }
+    let foreign_clients = topology.create_topology(&chain_handles)?;
 
     let relayer = RelayerDriver {
         config_path,

tools/test-framework/src/bootstrap/nary/channel.rs

@@ -17,60 +17,46 @@ use crate::types::nary::chains::{DynamicConnectedChains, NaryConnectedChains};
 use crate::types::nary::channel::{ConnectedChannels, DynamicConnectedChannels};
 use crate::types::nary::connection::{ConnectedConnections, DynamicConnectedConnections};
 use crate::types::tagged::*;
-use crate::util::array::{assert_same_dimension, into_nested_vec};
+use crate::util::array::into_nested_vec;
+use crate::util::two_dim_hash_map::TwoDimMap;
 
 /**
    Bootstrap a dynamic number of channels based on the number of
    connections in `DynamicConnectedConnections`.
+   See [`crate::types::topology`] for more information.
 */
 pub fn bootstrap_channels_with_connections_dynamic<Handle: ChainHandle>(
     connections: DynamicConnectedConnections<Handle>,
-    chains: &Vec<Handle>,
     ports: &Vec<Vec<PortId>>,
     order: Ordering,
     bootstrap_with_random_ids: bool,
 ) -> Result<DynamicConnectedChannels<Handle>, Error> {
-    let size = chains.len();
-
-    assert_same_dimension(size, connections.connections())?;
-    assert_same_dimension(size, ports)?;
-
-    let mut channels: Vec<Vec<ConnectedChannel<Handle, Handle>>> = Vec::new();
-
-    for (i, connections_b) in connections.connections().iter().enumerate() {
-        let mut channels_b: Vec<ConnectedChannel<Handle, Handle>> = Vec::new();
-
-        for (j, connection) in connections_b.iter().enumerate() {
-            if i <= j {
-                let chain_a = &chains[i];
-                let chain_b = &chains[j];
-
-                let port_a = &ports[i][j];
-                let port_b = &ports[j][i];
-
-                let bootstrap_options = BootstrapChannelOptions::default()
-                    .order(order)
-                    .bootstrap_with_random_ids(bootstrap_with_random_ids);
-
-                let channel = bootstrap_channel_with_connection(
-                    chain_a,
-                    chain_b,
-                    connection.clone(),
-                    &DualTagged::new(port_a),
-                    &DualTagged::new(port_b),
-                    bootstrap_options,
-                )?;
-
-                channels_b.push(channel);
-            } else {
-                let counter_channel = &channels[j][i];
-                let channel = counter_channel.clone().flip();
-
-                channels_b.push(channel);
-            }
-        }
-
-        channels.push(channels_b);
+    let mut channels: TwoDimMap<ConnectedChannel<Handle, Handle>> = TwoDimMap::new();
+
+    for (src_chain, dst_chain, connection) in connections.connections().iter() {
+        let channel = if let Some(counterparty_channel) = channels.get((dst_chain, src_chain)) {
+            counterparty_channel.clone().flip()
+        } else {
+            // No channel is found, will create one
+            let chain_a = &connection.connection.a_chain();
+            let chain_b = &connection.connection.b_chain();
+            let port_a = ports[src_chain][dst_chain].clone();
+            let port_b = ports[dst_chain][src_chain].clone();
+
+            let bootstrap_options = BootstrapChannelOptions::default()
+                .order(order)
+                .bootstrap_with_random_ids(bootstrap_with_random_ids);
+
+            bootstrap_channel_with_connection(
+                chain_a,
+                chain_b,
+                connection.clone(),
+                &DualTagged::new(&port_a),
+                &DualTagged::new(&port_b),
+                bootstrap_options,
+            )?
+        };
+        channels.insert((src_chain, dst_chain), channel);
     }
 
     Ok(DynamicConnectedChannels::new(channels))
@@ -82,14 +68,12 @@ pub fn bootstrap_channels_with_connections_dynamic<Handle: ChainHandle>(
 */
 pub fn bootstrap_channels_with_connections<Handle: ChainHandle, const SIZE: usize>(
     connections: ConnectedConnections<Handle, SIZE>,
-    chains: [Handle; SIZE],
     ports: [[PortId; SIZE]; SIZE],
     order: Ordering,
     bootstrap_with_random_ids: bool,
 ) -> Result<ConnectedChannels<Handle, SIZE>, Error> {
     let channels = bootstrap_channels_with_connections_dynamic(
         connections.into(),
-        &chains.into(),
         &into_nested_vec(ports),
         order,
         bootstrap_with_random_ids,
@@ -118,7 +102,6 @@ pub fn bootstrap_channels_and_connections_dynamic<Handle: ChainHandle>(
 
     bootstrap_channels_with_connections_dynamic(
         connections,
-        chains.chain_handles(),
         ports,
         order,
         bootstrap_with_random_ids,

tools/test-framework/src/bootstrap/nary/connection.rs

@@ -3,6 +3,7 @@
 */
 
 use core::time::Duration;
+use eyre::eyre;
 use ibc_relayer::chain::handle::ChainHandle;
 use ibc_relayer::foreign_client::ForeignClient;
 
@@ -12,48 +13,44 @@ use crate::types::binary::connection::ConnectedConnection;
 use crate::types::binary::foreign_client::ForeignClientPair;
 use crate::types::nary::connection::{ConnectedConnections, DynamicConnectedConnections};
 use crate::types::nary::foreign_client::ForeignClientPairs;
-use crate::util::array::assert_same_dimension;
+use crate::util::two_dim_hash_map::TwoDimMap;
 
 /**
    Bootstrap a dynamic number of connections based on the
-   given foreign client NxN matrix.
+   given foreign clients.
+   See [`crate::types::topology`] for more information.
 */
 pub fn bootstrap_connections_dynamic<Handle: ChainHandle>(
-    foreign_clients: &Vec<Vec<ForeignClient<Handle, Handle>>>,
+    foreign_clients: &TwoDimMap<ForeignClient<Handle, Handle>>,
     connection_delay: Duration,
     bootstrap_with_random_ids: bool,
 ) -> Result<DynamicConnectedConnections<Handle>, Error> {
-    let size = foreign_clients.len();
+    let mut connections: TwoDimMap<ConnectedConnection<Handle, Handle>> = TwoDimMap::new();
 
-    assert_same_dimension(size, foreign_clients)?;
+    for (src_chain, dst_chain, foreign_client) in foreign_clients.iter() {
+        let connection = if let Some(counterparty_connection) =
+            connections.get((dst_chain, src_chain))
+        {
+            counterparty_connection.clone().flip()
+        } else {
+            // No connection is found, will create one
+            let client_a_to_b = foreign_client.clone();
+            let client_b_to_a = foreign_clients.get((dst_chain, src_chain)).ok_or_else(|| {
+                Error::generic(eyre!(
+                    "No client entry found from chain `{}` to `{}`",
+                    dst_chain,
+                    src_chain,
+                ))
+            })?;
+            let foreign_clients = ForeignClientPair::new(client_a_to_b, client_b_to_a.clone());
 
-    let mut connections: Vec<Vec<ConnectedConnection<Handle, Handle>>> = Vec::new();
+            let bootstrap_options = BootstrapConnectionOptions::default()
+                .connection_delay(connection_delay)
+                .bootstrap_with_random_ids(bootstrap_with_random_ids);
 
-    for (i, foreign_clients_b) in foreign_clients.iter().enumerate() {
-        let mut connections_b: Vec<ConnectedConnection<Handle, Handle>> = Vec::new();
-
-        for (j, foreign_client) in foreign_clients_b.iter().enumerate() {
-            if i <= j {
-                let counter_foreign_client = &foreign_clients[j][i];
-                let foreign_clients =
-                    ForeignClientPair::new(foreign_client.clone(), counter_foreign_client.clone());
-
-                let bootstrap_options = BootstrapConnectionOptions::default()
-                    .connection_delay(connection_delay)
-                    .bootstrap_with_random_ids(bootstrap_with_random_ids);
-
-                let connection = bootstrap_connection(&foreign_clients, bootstrap_options)?;
-
-                connections_b.push(connection);
-            } else {
-                let counter_connection = &connections[j][i];
-                let connection = counter_connection.clone().flip();
-
-                connections_b.push(connection);
-            }
-        }
-
-        connections.push(connections_b);
+            bootstrap_connection(&foreign_clients, bootstrap_options)?
+        };
+        connections.insert((src_chain, dst_chain), connection);
     }
 
     Ok(DynamicConnectedConnections::new(connections))

tools/test-framework/src/framework/binary/chain.rs

@@ -22,6 +22,7 @@ use crate::types::binary::chains::{ConnectedChains, DropChainHandle};
 use crate::types::config::TestConfig;
 use crate::types::env::write_env;
 use crate::types::single::node::FullNode;
+use crate::types::topology::TopologyType;
 use crate::util::suspend::hang_on_error;
 
 /**
@@ -113,6 +114,10 @@ pub trait RelayerConfigOverride {
     fn modify_relayer_config(&self, config: &mut Config);
 }
 
+pub trait TopologyOverride {
+    fn topology(&self) -> Option<TopologyType>;
+}
+
 /// An internal trait that can be implemented by test cases to override the
 /// settings for the foreign clients bootstrapped for the test.
 ///