Enable multiple local Stratus instances

[gabriel-aranha-cw]

This PR adds configurable GRPC address and RocksDB path prefix runtime flags, and fixes the local execution when running multiple instances of Stratus.

Steps to execute multiple instances:

RUST_LOG=info cargo run --features=metrics,rocks,dev --bin run-with-importer -- --enable-test-accounts --candidate-peers="http://0.0.0.0:3002;3779,http://0.0.0.0:3001;3778" -a=0.0.0.0:3000 --grpc-server-address=0.0.0.0:3777
RUST_LOG=info cargo run --features=metrics,rocks,dev --bin run-with-importer -- --enable-test-accounts --candidate-peers="http://0.0.0.0:3000;3777,http://0.0.0.0:3002;3779" -a=0.0.0.0:3001 --grpc-server-address=0.0.0.0:3778 --rocks-path-prefix=tmp1
RUST_LOG=info cargo run --features=metrics,rocks,dev --bin run-with-importer -- --enable-test-accounts --candidate-peers="http://0.0.0.0:3000;3777,http://0.0.0.0:3001;3778" -a=0.0.0.0:3002 --grpc-server-address=0.0.0.0:3779 --rocks-path-prefix=tmp2

[github-actions]

PR Review 🔍

⏱️ Estimated effort to review [1-5]	3, because the PR involves multiple changes across different files including configuration and core consensus logic. Understanding the impact of these changes on the system's behavior, especially in a consensus algorithm, requires a deep understanding of the existing architecture and the introduced modifications.
🧪 Relevant tests	No
⚡ Possible issues	Possible Bug: The use of config.clone() multiple times within the same function call in Consensus::new could be inefficient and error-prone. Consider using references or restructuring how configuration data is passed to functions.
🔒 Security concerns	No

Code feedback:

---

relevant file	src/bin/run_with_importer.rs
suggestion	Consider using structured logging for better traceability and debugging. For example, instead of using tracing::info!("No chain url found");, use tracing::info!(message = "No chain url found", http_url = %http_url, ws_url = %ws_url); to provide more context in the logs. [important]
relevant line	return Err(anyhow!("No chain url found"));

---

relevant file	src/eth/consensus/mod.rs
suggestion	Ensure that the consensus module's server initialization logs the exact address it's starting on, including the port. Modify the log statement to include the port dynamically from the configuration. For example, change to tracing::info!("Starting append entry service at address: {}", consensus.grpc_address);. [important]
relevant line	tracing::info!("Starting append entry service at address: {}", consensus.grpc_address);

---

relevant file	src/eth/consensus/mod.rs
suggestion	To avoid potential runtime panics and ensure cleaner error handling, replace unwrap() calls with proper error handling in discover_my_address. For instance, use ? or match statements to handle errors gracefully. [important]
relevant line	let my_ip = socket.local_addr().ok().map(|addr| addr.ip().to_string()).unwrap();

---

relevant file	src/eth/consensus/mod.rs
suggestion	Instead of hardcoding the channel size, consider making it configurable or at least defined as a constant at the beginning of your module. This makes it easier to adjust and maintain. For example, define const CHANNEL_SIZE: usize = 32; and use it as mpsc::channel::<Block>(CHANNEL_SIZE);. [medium]
relevant line	let (sender, receiver) = mpsc::channel::(32);

---

[github-actions]

PR Code Suggestions ✨

Category	Suggestion	Score
Possible bug	Add error handling for UDP socket operations to prevent unwrapping errors Implement error handling for UdpSocket::bind and socket.connect to manage network errors gracefully. src/eth/consensus/mod.rs [191-192] -let socket = UdpSocket::bind("0.0.0.0:0").unwrap(); -socket.connect("8.8.8.8:80").ok().unwrap(); +let socket = UdpSocket::bind("0.0.0.0:0").map_err(|e| return Err(anyhow!("Failed to bind UDP socket: {}", e)))?; +socket.connect("8.8.8.8:80").map_err(|e| return Err(anyhow!("Failed to connect: {}", e)))?; Suggestion importance[1-10]: 9 Why: Adding error handling for UDP socket operations is crucial to prevent unwrapping errors, which enhances the stability and reliability of the code.	9
	Replace unwrap with error handling for retrieving the local IP address Avoid using unwrap directly on socket.local_addr() to handle potential errors more gracefully. src/eth/consensus/mod.rs [193] -let my_ip = socket.local_addr().ok().map(|addr| addr.ip().to_string()).unwrap(); +let my_ip = socket.local_addr().map_err(|e| return Err(anyhow!("Failed to get local address: {}", e)))?.ip().to_string(); Suggestion importance[1-10]: 9 Why: Replacing unwrap with proper error handling for retrieving the local IP address is important to avoid potential crashes and improve error management.	9
Possible issue	Add error handling for cloning the config to prevent runtime panics Consider handling potential errors from config.clone() method calls to avoid panics in runtime. Use pattern matching or error handling methods like map_err or unwrap_or_else. src/bin/run_with_importer.rs [25] -let consensus = Consensus::new(Arc::clone(&storage), config.clone().candidate_peers.clone(), Some(config.clone()), config.address, config.grpc_server_address).await; +let candidate_peers = config.clone().candidate_peers.map_err(|e| return Err(anyhow!("Failed to clone candidate_peers: {}", e)))?; +let config_clone = config.clone().map_err(|e| return Err(anyhow!("Failed to clone config: {}", e)))?; +let consensus = Consensus::new(Arc::clone(&storage), candidate_peers, Some(config_clone), config.address, config.grpc_server_address).await; Suggestion importance[1-10]: 8 Why: Adding error handling for cloning the config is a good practice to prevent potential runtime panics, which improves the robustness of the code.	8
Enhancement	Use environment variables for UDP connection settings to increase configurability Replace the hardcoded IP and port in UdpSocket::connect with configurable values to enhance flexibility and avoid potential network issues in different environments. src/eth/consensus/mod.rs [192] -socket.connect("8.8.8.8:80").ok().unwrap(); +let connect_ip = env::var("UDP_CONNECT_IP").unwrap_or("8.8.8.8".to_string()); +let connect_port = env::var("UDP_CONNECT_PORT").unwrap_or("80".to_string()); +socket.connect(format!("{}:{}", connect_ip, connect_port)).ok().unwrap(); Suggestion importance[1-10]: 7 Why: Using environment variables for UDP connection settings increases configurability and flexibility, which is beneficial for different deployment environments.	7

[github-actions]

PR Reviewer Guide 🔍

⏱️ Estimated effort to review [1-5]	4
🧪 Relevant tests	No
🔒 Security concerns	No
⚡ Key issues to review	Logging and Monitoring: The PR includes logging for various operations, especially in the consensus and peer discovery modules. However, it's important to ensure that all critical paths and error conditions are logged appropriately to facilitate debugging and monitoring.
	Async and Blocking Tasks: The PR does not explicitly address the separation of async and blocking tasks. Ensure that any potentially blocking operations (like I/O operations with RocksDB) are handled in a way that does not block the async runtime.
	Use of spawn_named: The PR does not use spawn_named for spawning new tasks. It is recommended to use named tasks for better traceability and debugging.

[github-actions]

PR Code Suggestions ✨

Category	Suggestion	Score
Possible bug	Replace unwrap calls with error handling to prevent potential runtime panics Replace the direct unwrap calls with proper error handling to prevent potential panics in production code. src/eth/consensus/mod.rs [209-211] -let socket = UdpSocket::bind("0.0.0.0:0").unwrap(); -socket.connect("8.8.8.8:80").ok().unwrap(); -let my_ip = socket.local_addr().ok().map(|addr| addr.ip().to_string()).unwrap(); +let socket = UdpSocket::bind("0.0.0.0:0")?; +socket.connect("8.8.8.8:80")?; +let my_ip = socket.local_addr()?.ip().to_string(); Suggestion importance[1-10]: 9 Why: This suggestion addresses a potential bug by replacing unwrap calls with proper error handling, which is crucial for preventing runtime panics and improving code robustness.	9
	Replace unwrap calls with error handling in database initialization to improve robustness Handle potential errors from RocksDb::new instead of unwrapping directly, to prevent panics and allow error propagation. src/eth/storage/rocks/rocks_state.rs [65-72] -accounts: RocksDb::new(&format!("{}./data/accounts.rocksdb", path_prefix), DbConfig::Default).unwrap(), -accounts_history: RocksDb::new(&format!("{}./data/accounts_history.rocksdb", path_prefix), DbConfig::FastWriteSST).unwrap(), -account_slots: RocksDb::new(&format!("{}./data/account_slots.rocksdb", path_prefix), DbConfig::Default).unwrap(), -account_slots_history: RocksDb::new(&format!("{}./data/account_slots_history.rocksdb", path_prefix), DbConfig::FastWriteSST).unwrap(), -transactions: RocksDb::new(&format!("{}./data/transactions.rocksdb", path_prefix), DbConfig::LargeSSTFiles).unwrap(), -blocks_by_number: RocksDb::new(&format!("{}./data/blocks_by_number.rocksdb", path_prefix), DbConfig::LargeSSTFiles).unwrap(), -blocks_by_hash: RocksDb::new(&format!("{}./data/blocks_by_hash.rocksdb", path_prefix), DbConfig::LargeSSTFiles).unwrap(), -logs: RocksDb::new(&format!("{}./data/logs.rocksdb", path_prefix), DbConfig::LargeSSTFiles).unwrap(), +accounts: RocksDb::new(&format!("{}./data/accounts.rocksdb", path_prefix), DbConfig::Default)?, +accounts_history: RocksDb::new(&format!("{}./data/accounts_history.rocksdb", path_prefix), DbConfig::FastWriteSST)?, +account_slots: RocksDb::new(&format!("{}./data/account_slots.rocksdb", path_prefix), DbConfig::Default)?, +account_slots_history: RocksDb::new(&format!("{}./data/account_slots_history.rocksdb", path_prefix), DbConfig::FastWriteSST)?, +transactions: RocksDb::new(&format!("{}./data/transactions.rocksdb", path_prefix), DbConfig::LargeSSTFiles)?, +blocks_by_number: RocksDb::new(&format!("{}./data/blocks_by_number.rocksdb", path_prefix), DbConfig::LargeSSTFiles)?, +blocks_by_hash: RocksDb::new(&format!("{}./data/blocks_by_hash.rocksdb", path_prefix), DbConfig::LargeSSTFiles)?, +logs: RocksDb::new(&format!("{}./data/logs.rocksdb", path_prefix), DbConfig::LargeSSTFiles)?, Suggestion importance[1-10]: 9 Why: This suggestion improves the robustness of the code by handling potential errors during database initialization, preventing possible panics and allowing for better error propagation.	9
	Improve error handling by propagating errors instead of using unwrap() Consider handling the potential error from the Consensus::new method instead of using unwrap(). This will prevent the program from panicking at runtime if the initialization fails. You can use ? to propagate the error. src/main.rs [23-29] let consensus = Consensus::new( Arc::clone(&storage), config.clone().candidate_peers.clone(), None, config.address, config.grpc_server_address, ) -.await; // for now, we force None to initiate with the current node being the leader +.await?; // for now, we force None to initiate with the current node being the leader Suggestion importance[1-10]: 9 Why: This suggestion improves error handling by propagating errors instead of causing a potential panic with unwrap(). This is crucial for maintaining the stability and reliability of the application.	9
Performance	Reduce multiple cloning of the same object to improve efficiency Consider using a single config.clone() call and storing it in a variable to avoid multiple cloning operations, which can be inefficient. src/bin/run_with_importer.rs [25-30] +let cloned_config = config.clone(); let consensus = Consensus::new( Arc::clone(&storage), - config.clone().candidate_peers.clone(), - Some(config.clone()), + cloned_config.candidate_peers.clone(), + Some(cloned_config), config.address, config.grpc_server_address, ) Suggestion importance[1-10]: 7 Why: This suggestion improves performance by reducing redundant cloning operations. While it is a minor optimization, it enhances code efficiency and readability.	7
Enhancement	Use a realistic path for rocks_path_prefix to better simulate actual operational conditions in tests Instead of initializing rocks_path_prefix with an empty string, consider using a meaningful default value or configuring this through test setup to reflect realistic usage scenarios. tests/test_import_external_snapshot_rocksdb.rs [21-22] -let rocks_path_prefix: Option<String> = Some(String::new()); +let rocks_path_prefix: Option<String> = Some("path/to/rocksdb".to_string()); let rocks = RocksPermanentStorage::new(rocks_path_prefix).await.unwrap(); Suggestion importance[1-10]: 7 Why: Using a realistic path for rocks_path_prefix in tests can better simulate actual operational conditions, improving the reliability and relevance of the tests. However, this is a minor enhancement compared to critical error handling improvements.	7
Maintainability	Use descriptive variable names to enhance code clarity Use a more descriptive variable name than _scheme to improve code readability and maintainability. src/eth/consensus/mod.rs [103-106] -let (_scheme, address_part) = if let Some(address) = s.strip_prefix("http://") { +let (scheme, address_part) = if let Some(address) = s.strip_prefix("http://") { ("http://", address) } else if let Some(address) = s.strip_prefix("https://") { ("https://", address) } Suggestion importance[1-10]: 6 Why: This suggestion enhances code readability and maintainability by using a more descriptive variable name. While it is a minor improvement, it contributes to better code clarity.	6