Review comments

core/tests/loadnext/Cargo.toml

@@ -24,6 +24,7 @@ anyhow = "1.0"
 rand = { version = "0.7", features = ["small_rng"] }
 envy = "0.4"
 hex = "0.4"
+static_assertions = "1.1"
 
 [dev-dependencies]
 zksync_test_account = { path = "../test_account", version = "1.0" }

core/tests/loadnext/src/account_pool.rs

@@ -9,7 +9,10 @@ use zksync::{
 use zksync_eth_signer::PrivateKeySigner;
 use zksync_types::{tx::PackedEthSignature, Address, H256};
 
-use crate::{config::LoadtestConfig, rng::LoadtestRng};
+use crate::{
+    config::LoadtestConfig,
+    rng::{LoadtestRng, Random},
+};
 
 /// Thread-safe pool of the addresses of accounts used in the loadtest.
 #[derive(Debug, Clone)]
@@ -41,16 +44,14 @@ pub struct AccountCredentials {
     pub address: Address,
 }
 
-impl AccountCredentials {
-    /// Generates random credentials.
-    pub fn rand(rng: &mut LoadtestRng) -> Self {
+impl Random for AccountCredentials {
+    fn random(rng: &mut LoadtestRng) -> Self {
         let eth_pk = H256::random_using(rng);
         let address = pk_to_address(&eth_pk);
 
         Self { eth_pk, address }
     }
 }
-
 /// Type that contains the data required for the test wallet to operate.
 #[derive(Debug)]
 pub struct TestWallet {
@@ -88,14 +89,14 @@ impl AccountPool {
 
         // Perform a health check: check whether zkSync server is alive.
         let mut server_alive = false;
-        for _ in 0u64..3 {
+        for _ in 0usize..3 {
             if let Ok(Ok(_)) = timeout(Duration::from_secs(3), provider.contract_address()).await {
                 server_alive = true;
                 break;
             }
         }
         if !server_alive {
-            anyhow::bail!("zkSync server does not respond. Please check RPC addres and whether server is launched");
+            anyhow::bail!("zkSync server does not respond. Please check RPC address and whether server is launched");
         }
 
         let mut rng = LoadtestRng::new_generic(config.seed.clone());
@@ -118,7 +119,7 @@ impl AccountPool {
         let mut addresses = Vec::with_capacity(config.accounts_amount);
 
         for _ in 0..config.accounts_amount {
-            let eth_credentials = AccountCredentials::rand(&mut rng);
+            let eth_credentials = AccountCredentials::random(&mut rng);
             let zksync_pk = private_key_from_seed(eth_credentials.eth_pk.as_bytes())
                 .expect("Can't generate the zkSync private key");
             let wallet_credentials = WalletCredentials::<PrivateKeySigner>::from_pk(

core/tests/loadnext/src/all.rs

@@ -0,0 +1,9 @@
+/// Trait that allows accessing all the possible variants of a sequence.
+pub trait All: Sized {
+    fn all() -> &'static [Self];
+}
+
+/// Trait that extends `All` trait with the corresponding expected probability.
+pub trait AllWeighted: Sized {
+    fn all_weighted() -> &'static [(Self, f32)];
+}

core/tests/loadnext/src/command/mod.rs

@@ -1,7 +1,13 @@
 use rand::Rng;
+use static_assertions::const_assert;
+
 use zksync_types::Address;
 
-use crate::{account_pool::AddressPool, rng::LoadtestRng};
+use crate::{
+    account_pool::AddressPool,
+    constants::MAX_BATCH_SIZE,
+    rng::{LoadtestRng, Random},
+};
 
 pub use self::{
     api_command::ApiRequestCommand,
@@ -29,19 +35,21 @@ enum CommandType {
     ApiRequest,
 }
 
-impl CommandType {
+impl Random for CommandType {
     fn random(rng: &mut LoadtestRng) -> Self {
         // Chances of a certain event generation.
         // You must maintain the sum of these constants to be equal to 1.0f32.
         const SINGLE_TX_CHANCE: f32 = 0.7;
         const BATCH_CHANCE: f32 = 0.3;
         // We don't generate API requests at the moment.
+        #[allow(dead_code)] // I't used to count total chance.
         const API_REQUEST_CHANCE: f32 = 0.0;
 
+        #[allow(dead_code)] // I't used in the const assertion.
         const CHANCES_SUM: f32 = SINGLE_TX_CHANCE + BATCH_CHANCE + API_REQUEST_CHANCE;
-        assert!(
-            (CHANCES_SUM - 1.0f32).abs() <= f32::EPSILON,
-            "Sum of chances is not equal to 1.0"
+        // Unfortunately. f64::abs()` is not yet a `const` function.
+        const_assert!(
+            -f32::EPSILON <= (CHANCES_SUM - 1.0f32) && (CHANCES_SUM - 1.0f32) <= f32::EPSILON
         );
         let chance = rng.gen_range(0.0f32, 1.0f32);
 
@@ -56,15 +64,13 @@ impl CommandType {
 }
 
 impl Command {
-    pub const MAX_BATCH_SIZE: usize = 20;
-
     pub fn random(rng: &mut LoadtestRng, own_address: Address, addresses: &AddressPool) -> Self {
         match CommandType::random(rng) {
             CommandType::SingleTx => Self::SingleTx(TxCommand::random(rng, own_address, addresses)),
             CommandType::Batch => {
                 // TODO: For some reason, batches of size 1 are being rejected because of nonce mistmatch.
                 // It may be either bug in loadtest or server code, thus it should be investigated.
-                let batch_size = rng.gen_range(2, Self::MAX_BATCH_SIZE + 1);
+                let batch_size = rng.gen_range(2, MAX_BATCH_SIZE + 1);
                 let mut batch_command: Vec<_> = (0..batch_size)
                     .map(|_| TxCommand::random_batchable(rng, own_address, addresses))
                     .collect();

core/tests/loadnext/src/command/tx_command.rs

@@ -1,9 +1,13 @@
 use num::BigUint;
-use rand::{seq::SliceRandom, Rng};
+use rand::Rng;
 
 use zksync_types::Address;
 
-use crate::{account_pool::AddressPool, rng::LoadtestRng};
+use crate::{
+    account_pool::AddressPool,
+    all::{All, AllWeighted},
+    rng::{LoadtestRng, WeightedRandom},
+};
 
 /// Type of transaction. It doesn't copy the zkSync operation list, because
 /// it divides some transactions in subcategories (e.g. to new account / to existing account; to self / to other; etc)/
@@ -18,39 +22,71 @@ pub enum TxType {
     ChangePubKey,
 }
 
-impl TxType {
-    /// Generates a random transaction type. Not all the variants have the equal chance to be generated;
-    /// specifically transfers are made more likely.
-    pub fn random(rng: &mut LoadtestRng) -> Self {
+impl All for TxType {
+    fn all() -> &'static [Self] {
+        &[
+            Self::Deposit,
+            Self::TransferToNew,
+            Self::TransferToExisting,
+            Self::WithdrawToSelf,
+            Self::WithdrawToOther,
+            Self::FullExit,
+            Self::ChangePubKey,
+        ]
+    }
+}
+
+impl AllWeighted for TxType {
+    fn all_weighted() -> &'static [(Self, f32)] {
         // All available options together with their weight.
         // `TransferToNew` and `TransferToExisting` the most likely options.
-        const DEFAULT_WEIGHT: usize = 1;
-        const HIGH_WEIGHT: usize = 3;
-        let options = vec![
+        const DEFAULT_WEIGHT: f32 = 1.0;
+        const HIGH_WEIGHT: f32 = 3.0;
+        &[
             (Self::Deposit, DEFAULT_WEIGHT),
             (Self::TransferToNew, HIGH_WEIGHT),
             (Self::TransferToExisting, HIGH_WEIGHT),
             (Self::WithdrawToSelf, DEFAULT_WEIGHT),
             (Self::WithdrawToOther, DEFAULT_WEIGHT),
             (Self::FullExit, DEFAULT_WEIGHT),
             (Self::ChangePubKey, DEFAULT_WEIGHT),
-        ];
-
-        // Now we can get weighted element by simply choosing the random value from the vector.
-        options.choose_weighted(rng, |item| item.1).unwrap().0
+        ]
     }
+}
 
+impl TxType {
     /// Generates a random transaction type that can be a part of the batch.
     pub fn random_batchable(rng: &mut LoadtestRng) -> Self {
         loop {
             let output = Self::random(rng);
 
-            // Priority ops and ChangePubKey cannot be inserted into the batch.
-            if !matches!(output, Self::Deposit | Self::FullExit) {
+            // Priority ops cannot be inserted into the batch.
+            if output.is_batchable() {
                 return output;
             }
         }
     }
+
+    /// Checks whether `TxType` can be used as a part of the batch.
+    fn is_batchable(self) -> bool {
+        !matches!(self, Self::Deposit | Self::FullExit)
+    }
+
+    fn is_withdrawal(self) -> bool {
+        matches!(self, Self::WithdrawToOther | Self::WithdrawToSelf)
+    }
+
+    fn is_change_pubkey(self) -> bool {
+        matches!(self, Self::ChangePubKey)
+    }
+
+    fn is_priority(self) -> bool {
+        matches!(self, Self::Deposit | Self::FullExit)
+    }
+
+    fn is_target_self(self) -> bool {
+        matches!(self, Self::WithdrawToSelf | Self::FullExit)
+    }
 }
 
 /// Modifier to be applied to the transaction in order to make it incorrect.
@@ -71,48 +107,74 @@ pub enum IncorrectnessModifier {
     None,
 }
 
-/// Expected outcome of transaction:
-/// Since we may create erroneous transactions on purpose,
-/// we may expect different outcomes for each transaction.
-#[derive(Debug, Copy, Clone, PartialEq)]
-pub enum ExpectedOutcome {
-    /// Transactions was successfully executed.
-    TxSucceed,
-    /// Transaction sending should fail.
-    ApiRequestFailed,
-    /// Transaction should be accepted, but rejected at the
-    /// time of execution.
-    TxRejected,
-}
-
 impl IncorrectnessModifier {
-    pub fn random(rng: &mut LoadtestRng) -> Self {
-        if Self::no_modifier(rng) {
-            return Self::None;
-        }
-
-        let options = &[
+    // Have to implement this as a const function, since const functions in traits are not stabilized yet.
+    const fn const_all() -> &'static [Self] {
+        &[
             Self::ZeroFee,
             Self::IncorrectZkSyncSignature,
             Self::IncorrectEthSignature,
             Self::NonExistentToken,
             Self::TooBigAmount,
             Self::NotPackableAmount,
             Self::NotPackableFeeAmount,
-        ];
+            Self::None,
+        ]
+    }
+}
 
-        options.choose(rng).copied().unwrap()
+impl All for IncorrectnessModifier {
+    fn all() -> &'static [Self] {
+        Self::const_all()
     }
+}
 
-    fn no_modifier(rng: &mut LoadtestRng) -> bool {
-        // 90% of transactions should be correct.
-        const NO_MODIFIER_PROBABILITY: f32 = 0.9f32;
+impl AllWeighted for IncorrectnessModifier {
+    fn all_weighted() -> &'static [(Self, f32)] {
+        const VARIANT_AMOUNTS: f32 = IncorrectnessModifier::const_all().len() as f32;
+        // No modifier is 9 times probable than all the other variants in sum.
+        // In other words, 90% probability of no modifier.
+        const NONE_PROBABILITY: f32 = (VARIANT_AMOUNTS - 1.0) * 9.0;
+        const DEFAULT_PROBABILITY: f32 = 1.0f32;
+
+        &[
+            (Self::ZeroFee, DEFAULT_PROBABILITY),
+            (Self::IncorrectZkSyncSignature, DEFAULT_PROBABILITY),
+            (Self::IncorrectEthSignature, DEFAULT_PROBABILITY),
+            (Self::NonExistentToken, DEFAULT_PROBABILITY),
+            (Self::TooBigAmount, DEFAULT_PROBABILITY),
+            (Self::NotPackableAmount, DEFAULT_PROBABILITY),
+            (Self::NotPackableFeeAmount, DEFAULT_PROBABILITY),
+            (Self::None, NONE_PROBABILITY),
+        ]
+    }
+}
 
-        let chance = rng.gen_range(0f32, 1f32);
+impl IncorrectnessModifier {
+    fn affects_amount(self) -> bool {
+        matches!(self, Self::TooBigAmount | Self::NotPackableAmount)
+    }
 
-        chance <= NO_MODIFIER_PROBABILITY
+    fn is_not_packable_amount(self) -> bool {
+        matches!(self, Self::NotPackableAmount)
     }
+}
+
+/// Expected outcome of transaction:
+/// Since we may create erroneous transactions on purpose,
+/// we may expect different outcomes for each transaction.
+#[derive(Debug, Copy, Clone, PartialEq)]
+pub enum ExpectedOutcome {
+    /// Transactions was successfully executed.
+    TxSucceed,
+    /// Transaction sending should fail.
+    ApiRequestFailed,
+    /// Transaction should be accepted, but rejected at the
+    /// time of execution.
+    TxRejected,
+}
 
+impl IncorrectnessModifier {
     pub fn expected_outcome(self) -> ExpectedOutcome {
         match self {
             Self::None => ExpectedOutcome::TxSucceed,
@@ -188,29 +250,19 @@ impl TxCommand {
             command.to = Address::random();
         }
 
-        // Check whether we should use a self as an target.
-        if matches!(
-            command.command_type,
-            TxType::WithdrawToSelf | TxType::FullExit
-        ) {
+        // Check whether we should use a self as a target.
+        if command.command_type.is_target_self() {
             command.to = own_address;
         }
 
         // Transactions that have no amount field.
-        let no_amount_field = matches!(command.command_type, TxType::ChangePubKey)
-            && matches!(
-                command.modifier,
-                IncorrectnessModifier::TooBigAmount | IncorrectnessModifier::NotPackableAmount
-            );
+        let no_amount_field =
+            command.command_type.is_change_pubkey() && command.modifier.affects_amount();
         // It doesn't make sense to fail contract-based functions.
-        let incorrect_priority_op =
-            matches!(command.command_type, TxType::Deposit | TxType::FullExit);
+        let incorrect_priority_op = command.command_type.is_priority();
         // Amount doesn't have to be packable for withdrawals.
-        let unpackable_withdrawal = matches!(
-            command.command_type,
-            TxType::WithdrawToOther | TxType::WithdrawToSelf
-        ) && command.modifier
-            == IncorrectnessModifier::NotPackableAmount;
+        let unpackable_withdrawal =
+            command.command_type.is_withdrawal() && command.modifier.is_not_packable_amount();
 
         // Check whether generator modifier does not make sense.
         if no_amount_field || incorrect_priority_op || unpackable_withdrawal {

core/tests/loadnext/src/config.rs

@@ -62,7 +62,7 @@ impl Default for LoadtestConfig {
             eth_network: "localhost".into(),
             master_wallet_pk: "74d8b3a188f7260f67698eb44da07397a298df5427df681ef68c45b34b61f998"
                 .into(),
-            accounts_amount: 20,
+            accounts_amount: 80,
             operations_per_account: 40,
             main_token: "DAI".into(),
             seed: None,

core/tests/loadnext/src/constants.rs

@@ -10,3 +10,7 @@ pub const COMMIT_TIMEOUT: Duration = Duration::from_secs(600);
 /// We don't want to overload the server with too many requests; given the fact that blocks are expected to be created
 /// every couple of seconds, chosen value seems to be adequate to provide the result in one or two calls at average.
 pub const POLLING_INTERVAL: Duration = Duration::from_secs(3);
+
+// TODO (ZKS-623): This value is not the greatest batch size zkSync supports.
+// However, choosing the bigger value (e.g. 40) causes server to fail with error "Error communicating core server".
+pub const MAX_BATCH_SIZE: usize = 20;