Implement Append Entries to Log

[gabriel-aranha-cw]

This PR implements saving Block and Transaction Executions to the Append Entries Log.

to do:

• implement append block
• desired behavior when error on saving log entry

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit a2d7a2d)

⏱️ Estimated effort to review [1-5]	4
🧪 Relevant tests	Yes
🔒 Security concerns	No
⚡ Key issues to review	Logging and Monitoring: Ensure that all operations related to tokio::spawn and tokio::spawn_blocking are properly logged and monitored. The PR should use spawn_named or spawn_blocking_named for better traceability.
	Error Handling: The use of expect in places where unwrap is used can provide more context in case of an error, enhancing the robustness of the code.
	Tracing Fields: Ensure that dynamic fields in tracing events are replaced with structured tracing fields to maintain consistency and reliability in log messages.
	Initialization Logging: The initialization of core components like Consensus and AppendLogEntriesStorage should be logged with all relevant configurations to ensure transparency and traceability.
	Cloning: There are multiple instances of cloning, especially in configurations and paths. It's advisable to check if references can be used to optimize performance.

[github-actions]

PR Code Suggestions ✨

Category	Suggestion	Score
Error handling	Add error handling for retrieving the last index from log storage Implement error handling for the unwrap() calls when retrieving the last index from log_storage. This will prevent potential panics in runtime due to None values. src/eth/consensus/mod.rs [419] -let last_index = consensus.log_storage.get_last_index().unwrap_or(0); +let last_index = consensus.log_storage.get_last_index().unwrap_or_else(|| { + tracing::error!("Failed to retrieve last index from log storage"); + 0 // Default to 0 if unable to retrieve last index +}) Suggestion importance[1-10]: 9 Why: Adding error handling for the unwrap() calls significantly enhances the robustness of the code by preventing potential runtime panics.	9
	Implement error handling for deleting entries from log storage Replace the expect calls with proper error handling to avoid panics when deleting entries from log_storage. This will enhance the robustness of the code. src/eth/consensus/mod.rs [431] -consensus.log_storage.delete_entries_from(transaction_entry.index).expect("Failed to delete existing transaction entries"); +if let Err(e) = consensus.log_storage.delete_entries_from(transaction_entry.index) { + tracing::error!("Failed to delete existing transaction entries: {:?}", e); +} Suggestion importance[1-10]: 9 Why: This suggestion improves error handling, making the code more robust by avoiding panics and properly logging errors.	9
Maintainability	Replace hardcoded path with a configurable path for log storage initialization Replace the hardcoded string "log_storage" with a configurable path or variable to avoid hardcoding paths in the codebase. This will make the code more flexible and maintainable. src/eth/consensus/mod.rs [194] -log_storage: Arc::new(AppendLogEntriesStorage::new("log_storage").unwrap()) +log_storage: Arc::new(AppendLogEntriesStorage::new(config.log_storage_path).unwrap()) Suggestion importance[1-10]: 8 Why: This suggestion improves maintainability by avoiding hardcoded paths, making the code more flexible and easier to configure for different environments.	8
Reliability	Add retry mechanism for failed broadcasts to ensure reliability Consider implementing a retry mechanism or alternative handling when broadcasting fails, to ensure important log entries are not lost due to transient issues. src/eth/consensus/mod.rs [444] if consensus.broadcast_sender.send(transaction_entry_data).is_err() { - tracing::error!("failed to broadcast transaction"); + tracing::warn!("Failed to broadcast transaction, retrying..."); + // Implement retry logic or alternative handling here } Suggestion importance[1-10]: 7 Why: Implementing a retry mechanism for failed broadcasts can improve reliability, but the suggestion is less critical compared to error handling improvements.	7

[github-actions]

Persistent review updated to latest commit a2d7a2d

[github-actions]

PR Code Suggestions ✨

Category	Suggestion	Score
Possible issue	Replace assert! with error handling to prevent program termination Replace the assert! with a more graceful error handling mechanism. Using assert! will cause the program to panic and terminate if the condition is not met, which is not ideal for production code. Instead, return an error to allow the caller to handle it appropriately. src/eth/consensus/append_log_entries_storage.rs [23] -assert!(!prefix.is_empty(), "given prefix for RocksDB is empty, try not providing the flag"); +if prefix.is_empty() { + return Err(anyhow::anyhow!("given prefix for RocksDB is empty, try not providing the flag")); +} Suggestion importance[1-10]: 9 Why: This suggestion addresses a potential runtime panic by replacing assert! with proper error handling, which is crucial for production code stability.	9
	Handle potential invalid or None path scenarios safely Consider handling the case where config.storage.perm_storage.rocks_path_prefix.clone() might return a None or invalid path, to prevent runtime errors when the path is used. src/main.rs [25] -config.storage.perm_storage.rocks_path_prefix.clone() +config.storage.perm_storage.rocks_path_prefix.clone().unwrap_or_else(|| String::from("/default/rocksdb/path")) Suggestion importance[1-10]: 9 Why: This suggestion addresses a potential runtime issue by ensuring that the path is valid, which is crucial for preventing errors during execution.	9
Possible bug	Handle potential None case when converting path to string Consider handling the case where tmpdir_log_entries.path().to_str() returns None. This can happen if the path contains invalid Unicode characters. You can handle this by either defaulting to a predefined path or by propagating the error. src/eth/consensus/tests/factories.rs [107] -let tmpdir_log_entries_path = tmpdir_log_entries.path().to_str().map(|s| s.to_owned()); +let tmpdir_log_entries_path = tmpdir_log_entries.path().to_str().map(|s| s.to_owned()).unwrap_or_else(|| String::from("/default/path")); Suggestion importance[1-10]: 9 Why: This suggestion addresses a potential bug where the path conversion could fail, leading to a None value. Handling this case improves the robustness of the code.	9
	Implement error handling for DB operations to enhance robustness Replace the unwrap() calls with proper error handling to prevent potential runtime panics if an error occurs during database operations. src/eth/consensus/append_log_entries_storage.rs [36-37] let db = DB::open(&opts, path).context("Failed to open RocksDB")?; -Ok(Self { db }) +db.map(|db| Self { db }).context("Failed to create AppendLogEntriesStorage with the provided DB") Suggestion importance[1-10]: 8 Why: This suggestion enhances robustness by replacing unwrap() with proper error handling, preventing potential runtime panics.	8
Enhancement	Use PathBuf for path handling to enhance safety and correctness Consider using PathBuf instead of String for the path parameter in the new function to handle paths more robustly and avoid potential issues with path manipulation. src/eth/consensus/append_log_entries_storage.rs [17] -pub fn new(path: Option<String>) -> Result<Self> +pub fn new(path: Option<PathBuf>) -> Result<Self> Suggestion importance[1-10]: 7 Why: Using PathBuf for path handling is a good practice in Rust, enhancing safety and correctness, though it is a minor improvement.	7
Best practice	Verify and ensure appropriate usage of Arc for thread safety Ensure thread safety by using Arc for tmpdir_log_entries if it's shared across threads, or consider reducing overhead by not using Arc if it's not necessary. src/eth/consensus/tests/factories.rs [99] -let (_log_entries_storage, tmpdir_log_entries) = StratusStorage::mock_new_rocksdb(); +let (_log_entries_storage, tmpdir_log_entries) = StratusStorage::mock_new_rocksdb(); // Ensure proper usage of Arc if shared across threads Suggestion importance[1-10]: 7 Why: This suggestion promotes best practices for thread safety, which is important but may not be immediately critical depending on the context of tmpdir_log_entries.	7
Maintainability	Refactor path logic into a separate function to improve modularity Refactor the new function to separate concerns by extracting the path manipulation logic into a separate function. This will improve readability and maintainability. src/eth/consensus/append_log_entries_storage.rs [21-34] -let path = if let Some(prefix) = path { - ... - path -} else { - ... - "data/log-entries-rocksdb".to_string() -}; +let path = determine_path(path); +... +fn determine_path(path: Option<String>) -> String { + if let Some(prefix) = path { + ... + format!("{prefix}-log-entries-rocksdb") + } else { + ... + "data/log-entries-rocksdb".to_string() + } +} Suggestion importance[1-10]: 6 Why: This refactoring improves code readability and maintainability by separating concerns, but it does not address any critical issues.	6
Performance	Optimize atomic operation by using a weaker ordering if applicable Replace Ordering::SeqCst with Ordering::Relaxed if the order of operations is not critical in this context, as SeqCst might be unnecessarily strong and impact performance. src/eth/consensus/tests/factories.rs [150] -consensus.current_term.store(term, Ordering::SeqCst); +consensus.current_term.store(term, Ordering::Relaxed); Suggestion importance[1-10]: 6 Why: This suggestion could improve performance by using a weaker memory ordering, but it requires careful consideration of the context to ensure correctness.	6