Some issues spotted (#145)

This PR lists some issues and suggested fixes for them (feel free to
pick them up separately and close this PR):

In addition to issues outlined in
#117 (comment),
here are some issues spotted when writing
https://github.com/paritytech/disabling-e2e-tests:
- runtime genesis patch is applied incorrectly (extra `/genesis` pointer
shouldn't be added)
- malus accepts
[subcommands](https://github.com/paritytech/polkadot-sdk/blob/4c0e0e071355c1048d75fba538c96c35ac743547/polkadot/zombienet_tests/functional/0008-dispute-old-finalized.toml#L25),
having a command with spaces not supported, so I added a subcommand
support
- some types need to be exported in order to be able to reuse
setup/helper functions across multiple tests
- test cleanup doesn't always work (zombie polkadot processes - not
fixed here)

---------

Co-authored-by: Javier Viola <[email protected]>

crates/configuration/src/relaychain.rs

@@ -33,7 +33,7 @@ pub struct RelaychainConfig {
     max_nominations: Option<u8>,
     #[serde(skip_serializing_if = "std::vec::Vec::is_empty", default)]
     nodes: Vec<NodeConfig>,
-    genesis_overrides: Option<serde_json::Value>,
+    runtime_genesis_patch: Option<serde_json::Value>,
     command: Option<Command>,
 }
 
@@ -89,8 +89,8 @@ impl RelaychainConfig {
     }
 
     /// The genesis overrides as a JSON value.
-    pub fn genesis_overrides(&self) -> Option<&serde_json::Value> {
-        self.genesis_overrides.as_ref()
+    pub fn runtime_genesis_patch(&self) -> Option<&serde_json::Value> {
+        self.runtime_genesis_patch.as_ref()
     }
 
     /// The nodes of the relay chain.
@@ -133,7 +133,7 @@ impl Default for RelaychainConfigBuilder<Initial> {
                 command: None,
                 random_nominators_count: None,
                 max_nominations: None,
-                genesis_overrides: None,
+                runtime_genesis_patch: None,
                 nodes: vec![],
             },
             validation_context: Default::default(),
@@ -338,7 +338,7 @@ impl RelaychainConfigBuilder<WithChain> {
     pub fn with_genesis_overrides(self, genesis_overrides: impl Into<serde_json::Value>) -> Self {
         Self::transition(
             RelaychainConfig {
-                genesis_overrides: Some(genesis_overrides.into()),
+                runtime_genesis_patch: Some(genesis_overrides.into()),
                 ..self.config
             },
             self.validation_context,

crates/configuration/src/shared/node.rs

@@ -65,6 +65,7 @@ pub struct NodeConfig {
     name: String,
     pub(crate) image: Option<Image>,
     pub(crate) command: Option<Command>,
+    pub(crate) subcommand: Option<Command>,
     #[serde(default)]
     args: Vec<Arg>,
     #[serde(alias = "validator", default = "default_as_true")]
@@ -112,6 +113,12 @@ impl Serialize for NodeConfig {
             state.serialize_field("command", &self.command)?;
         }
 
+        if self.subcommand.is_none() {
+            state.skip_field("subcommand")?;
+        } else {
+            state.serialize_field("subcommand", &self.subcommand)?;
+        }
+
         if self.args.is_empty() || self.args == self.chain_context.default_args {
             state.skip_field("args")?;
         } else {
@@ -174,6 +181,11 @@ impl NodeConfig {
         self.command.as_ref()
     }
 
+    /// Subcommand to run the node.
+    pub fn subcommand(&self) -> Option<&Command> {
+        self.subcommand.as_ref()
+    }
+
     /// Arguments to use for node.
     pub fn args(&self) -> Vec<&Arg> {
         self.args.iter().collect()
@@ -265,6 +277,7 @@ impl Default for NodeConfigBuilder<Initial> {
                 name: "".into(),
                 image: None,
                 command: None,
+                subcommand: None,
                 args: vec![],
                 is_validator: true,
                 is_invulnerable: true,
@@ -371,6 +384,29 @@ impl NodeConfigBuilder<Buildable> {
         }
     }
 
+    /// Set the subcommand that will be executed to launch the node.
+    pub fn with_subcommand<T>(self, subcommand: T) -> Self
+    where
+        T: TryInto<Command>,
+        T::Error: Error + Send + Sync + 'static,
+    {
+        match subcommand.try_into() {
+            Ok(subcommand) => Self::transition(
+                NodeConfig {
+                    subcommand: Some(subcommand),
+                    ..self.config
+                },
+                self.validation_context,
+                self.errors,
+            ),
+            Err(error) => Self::transition(
+                self.config,
+                self.validation_context,
+                merge_errors(self.errors, FieldError::Command(error.into()).into()),
+            ),
+        }
+    }
+
     /// Set the image that will be used for the node (only podman/k8s). Override the default.
     pub fn with_image<T>(self, image: T) -> Self
     where

crates/examples/examples/0001-simple.toml

@@ -1,6 +1,11 @@
 [settings]
 timeout = 1000
 
+[relaychain.runtime_genesis_patch.configuration.config]
+max_validators_per_core = 1
+needed_approvals = 2
+group_rotation_frequency = 10
+
 [relaychain]
 default_image = "{{ZOMBIENET_INTEGRATION_TEST_IMAGE}}"
 chain = "rococo-local"
@@ -18,7 +23,7 @@ command = "polkadot"
 id = 100
 addToGenesis = false
 
-  [parachains.collator]
+  [[parachains.collators]]
   name = "collator01"
   image = "{{COL_IMAGE}}"
   command = "adder-collator"
@@ -27,4 +32,4 @@ addToGenesis = false
 [types.Header]
 number = "u64"
 parent_hash = "Hash"
-post_state = "Hash"
+post_state = "Hash"

crates/examples/examples/simple_network_example.rs

@@ -12,7 +12,7 @@ async fn main() -> Result<(), Box<dyn std::error::Error>> {
     println!("🚀🚀🚀🚀 network deployed");
 
     let client = network
-        .get_node("alice")?
+        .get_node("collator01")?
         .client::<subxt::PolkadotConfig>()
         .await?;
     let mut blocks = client.blocks().subscribe_finalized().await?.take(3);

crates/orchestrator/src/generators/chain_spec.rs

@@ -306,7 +306,7 @@ impl ChainSpec {
             if let Some(overrides) = &para.genesis_overrides {
                 if let Some(genesis) = chain_spec_json.pointer_mut(&format!("{}/genesis", pointer))
                 {
-                    override_genesis(genesis, overrides);
+                    merge(genesis, overrides);
                 }
             }
 
@@ -389,10 +389,9 @@ impl ChainSpec {
                 .map_err(GeneratorError::ChainSpecGeneration)?;
 
             // make genesis overrides first.
-            if let Some(overrides) = &relaychain.genesis_overrides {
-                if let Some(genesis) = chain_spec_json.pointer_mut(&format!("{}/genesis", pointer))
-                {
-                    override_genesis(genesis, overrides);
+            if let Some(overrides) = &relaychain.runtime_genesis_patch {
+                if let Some(patch_section) = chain_spec_json.pointer_mut(&pointer) {
+                    merge(patch_section, overrides);
                 }
             }
 
@@ -604,10 +603,10 @@ fn get_runtime_config_pointer(chain_spec_json: &serde_json::Value) -> Result<Str
     Err("Can not find the runtime pointer".into())
 }
 
-// Override `genesis` key if present
-fn override_genesis(genesis: &mut serde_json::Value, overrides: &serde_json::Value) {
+// Merge `patch_section` with `overrides`.
+fn merge(patch_section: &mut serde_json::Value, overrides: &serde_json::Value) {
     if let (Some(genesis_obj), Some(overrides_obj)) =
-        (genesis.as_object_mut(), overrides.as_object())
+        (patch_section.as_object_mut(), overrides.as_object())
     {
         for overrides_key in overrides_obj.keys() {
             // we only want to override keys present in the genesis object
@@ -617,7 +616,7 @@ fn override_genesis(genesis: &mut serde_json::Value, overrides: &serde_json::Val
                     (serde_json::Value::Object(_), Some(overrides_value))
                         if overrides_value.is_object() =>
                     {
-                        override_genesis(genesis_value, overrides_value);
+                        merge(genesis_value, overrides_value);
                     },
                     // override if genesis value not an object
                     (_, Some(overrides_value)) => {

crates/orchestrator/src/generators/command.rs

@@ -329,6 +329,10 @@ pub fn generate_for_node(
 
     final_args.append(&mut tmp_args);
 
+    if let Some(ref subcommand) = node.subcommand {
+        final_args.insert(1, subcommand.as_str().to_string());
+    }
+
     if options.use_wrapper {
         ("/cfg/zombie-wrapper.sh".to_string(), final_args)
     } else {

crates/orchestrator/src/network_spec/node.rs

@@ -26,6 +26,7 @@ macro_rules! impl_from_for_add_node_opts {
                 Self {
                     image: value.image,
                     command: value.command,
+                    subcommand: value.subcommand,
                     args: value.args,
                     env: value.env,
                     is_validator: value.is_validator,
@@ -62,6 +63,9 @@ pub struct NodeSpec {
     /// Command to run the node. Override the default.
     pub(crate) command: Command,
 
+    /// Optional subcommand for the node.
+    pub(crate) subcommand: Option<Command>,
+
     /// Arguments to use for node. Appended to default.
     pub(crate) args: Vec<Arg>,
 
@@ -125,6 +129,8 @@ impl NodeSpec {
             ));
         };
 
+        let subcommand = node_config.subcommand().cloned();
+
         // If `args` is set at `node` level use them
         // otherwise use the default_args (can be empty).
         let args: Vec<Arg> = if node_config.args().is_empty() {
@@ -150,6 +156,7 @@ impl NodeSpec {
             peer_id,
             image,
             command,
+            subcommand,
             args,
             is_validator: node_config.is_validator(),
             is_invulnerable: node_config.is_invulnerable(),
@@ -197,6 +204,8 @@ impl NodeSpec {
             ));
         };
 
+        let subcommand = options.subcommand.clone();
+
         // If `args` is set at `node` level use them
         // otherwise use the default_args (can be empty).
         let args: Vec<Arg> = if options.args.is_empty() {
@@ -226,6 +235,7 @@ impl NodeSpec {
             peer_id,
             image,
             command,
+            subcommand,
             args,
             is_validator: options.is_validator,
             is_invulnerable: false,

crates/orchestrator/src/network_spec/relaychain.rs

@@ -44,7 +44,7 @@ pub struct RelaychainSpec {
     pub(crate) max_nominations: u8,
 
     /// Genesis overrides as JSON value.
-    pub(crate) genesis_overrides: Option<serde_json::Value>,
+    pub(crate) runtime_genesis_patch: Option<serde_json::Value>,
 
     /// Nodes to run.
     pub(crate) nodes: Vec<NodeSpec>,
@@ -110,7 +110,7 @@ impl RelaychainSpec {
             chain_spec,
             random_nominators_count: config.random_nominators_count().unwrap_or(0),
             max_nominations: config.max_nominations().unwrap_or(24),
-            genesis_overrides: config.genesis_overrides().cloned(),
+            runtime_genesis_patch: config.runtime_genesis_patch().cloned(),
             nodes,
         })
     }

crates/orchestrator/src/shared/macros.rs

@@ -6,6 +6,8 @@ macro_rules! create_add_options {
             pub image: Option<Image>,
             /// Command to run the node
             pub command: Option<Command>,
+            /// Subcommand for the node
+            pub subcommand: Option<Command>,
             /// Arguments to pass to the node
             pub args: Vec<Arg>,
             /// Env vars to set

crates/sdk/src/lib.rs

@@ -1,11 +1,13 @@
 use async_trait::async_trait;
 pub use configuration::{NetworkConfig, NetworkConfigBuilder, RegistrationStrategy};
 pub use orchestrator::{
-    errors::OrchestratorError, network::Network, AddCollatorOptions, AddNodeOptions, Orchestrator,
-    PjsResult,
+    errors::OrchestratorError,
+    network::{node::NetworkNode, Network},
+    AddCollatorOptions, AddNodeOptions, Orchestrator, PjsResult,
 };
 use provider::NativeProvider;
-use support::{fs::local::LocalFileSystem, process::os::OsProcessManager};
+pub use support::fs::local::LocalFileSystem;
+use support::process::os::OsProcessManager;
 
 #[async_trait]
 pub trait NetworkConfigExt {