fix: block start handling zero better

[dinhani-cw]

No description provided.

[github-actions]

PR Review 🔍

⏱️ Estimated effort to review [1-5]	3, because the PR involves changes across multiple files with modifications to core functionalities related to block handling and storage operations in a blockchain context. The changes are significant but not overly complex, requiring a good understanding of the existing system and the implications of the changes on block processing.
🧪 Relevant tests	No
⚡ Possible issues	Possible Bug: The removal of the block_number_to_start function and its replacement with read_block_number_to_resume_import might introduce issues if the new function does not handle all cases previously covered, especially edge cases around block zero handling.
🔒 Security concerns	No

Code feedback:

---

relevant file	src/eth/storage/stratus_storage.rs
suggestion	Consider adding error handling for potential failures in the read_block method calls within read_block_number_to_resume_import. This would ensure that the function does not fail silently or propagate incorrect states if block reads fail. [important]
relevant line	let zero = self.read_block(&BlockSelection::Number(BlockNumber::ZERO)).await?;

---

relevant file	src/bin/importer_offline.rs
suggestion	It might be beneficial to add a fallback or default case in the run function when determining block_start if storage.read_block_number_to_resume_import().await? fails, to ensure the importer can still function under unexpected conditions. [important]
relevant line	None => storage.read_block_number_to_resume_import().await?,

---

relevant file	src/bin/importer_online.rs
suggestion	Ensure that the new method read_block_number_to_resume_import is thoroughly tested, especially since it replaces several lines of logic that handle different states of block numbers. This is crucial to avoid regressions or bugs in block processing. [important]
relevant line	let number = storage.read_block_number_to_resume_import().await?;

---

relevant file	src/eth/storage/stratus_storage.rs
suggestion	Add detailed logging for each decision branch in read_block_number_to_resume_import to improve traceability and debugging capabilities, especially useful during the initial roll-out of these changes in a production environment. [medium]
relevant line	if zero.is_none() {

---

[github-actions]

PR Code Suggestions ✨

Category	Suggestion	Score
Possible issue	Improve error handling for reading the block number to resume import Replace the direct call to storage.read_block_number_to_resume_import().await? with a more robust error handling mechanism. This is important to ensure that the function does not fail silently in case of errors during the retrieval of the block number. src/bin/importer_offline.rs [56] -None => storage.read_block_number_to_resume_import().await?, +None => { + match storage.read_block_number_to_resume_import().await { + Ok(num) => num, + Err(e) => { + tracing::error!("Failed to read block number to resume import: {:?}", e); + return Err(e); + } + } +}, Suggestion importance[1-10]: 9 Why: The suggestion improves error handling, which is crucial for robustness. It ensures that any error during the retrieval of the block number is logged and handled properly, preventing silent failures.	9
Possible bug	Add validation for the retrieved block number to ensure it's correct Consider adding a check to ensure that the retrieved block number is valid and not an unexpected value. This can prevent issues where the system resumes from an incorrect block number. src/bin/importer_online.rs [91] let number = storage.read_block_number_to_resume_import().await?; +if number.is_invalid() { + tracing::error!("Invalid block number retrieved: {}", number); + return Err(anyhow::Error::new(std::io::Error::new(std::io::ErrorKind::Other, "Invalid block number"))); +} Suggestion importance[1-10]: 8 Why: Adding validation for the retrieved block number enhances the reliability of the system by preventing it from resuming from an incorrect block number, which could lead to potential issues.	8
Maintainability	Refactor the method to improve readability and maintainability by separating concerns Refactor the method read_block_number_to_resume_import to separate concerns and improve readability. Split the method into smaller, more focused methods. src/eth/storage/stratus_storage.rs [73-100] pub async fn read_block_number_to_resume_import(&self) -> anyhow::Result<BlockNumber> { - // if does not have the zero block present, should resume from zero - let zero = self.read_block(&BlockSelection::Number(BlockNumber::ZERO)).await?; - if zero.is_none() { - tracing::info!(number = %0, reason = %"block ZERO does not exist", "resume from ZERO"); - return Ok(BlockNumber::ZERO); + if let Ok(zero_block) = self.check_zero_block_presence().await { + return Ok(zero_block); } - // try to resume from active block number - let active_number = self.read_active_block_number().await?; - if let Some(active_number) = active_number { - tracing::info!(number = %active_number, reason = %"set in storage", "resume from ACTIVE"); - return Ok(active_number); + if let Ok(active_block) = self.check_active_block_presence().await { + return Ok(active_block); } - // fallback to last mined block number - let mined_number = self.read_mined_block_number().await?; - let mined_block = self.read_block(&BlockSelection::Number(mined_number)).await?; - match mined_block { - Some(_) => { - tracing::info!(number = %mined_number, reason = %"set in storage and block exist", "resume from MINED + 1"); - Ok(mined_number.next()) - } - None => { - tracing::info!(number = %mined_number, reason = %"set in storage but block does not exist", "resume from MINED"); - Ok(mined_number) - } - } + self.check_mined_block_presence().await } Suggestion importance[1-10]: 7 Why: The refactoring suggestion improves code readability and maintainability by separating concerns into smaller methods. However, it does not address any critical issues or bugs.	7
Enhancement	Add logging for determining the block number to stop the import Implement logging for the scenario where the block number to stop the import is determined. This will help in debugging and maintaining logs for the operations. src/bin/importer_offline.rs [241] match rpc_storage.read_max_block_number_in_range(BlockNumber::ZERO, BlockNumber::MAX).await { + Ok(max_num) => { + tracing::info!("Max block number to stop import: {}", max_num); + Ok(max_num) + } + Err(e) => { + tracing::error!("Failed to read max block number: {:?}", e); + Err(e) + } +} Suggestion importance[1-10]: 6 Why: Adding logging for determining the block number to stop the import is a useful enhancement for debugging and monitoring, but it is not as crucial as error handling or validation.	6