fix: calculate blocks to fetch correctly

[dinhani-cw]

No description provided.

[github-actions]

PR Review 🔍

⏱️ Estimated effort to review [1-5]	2, because the changes are localized to specific functions within two files and involve straightforward logic adjustments and logging level changes. The logic changes are not complex but require careful attention to ensure correctness.
🧪 Relevant tests	No
⚡ Possible issues	Off-by-one Error: The change in blocks_to_fetch calculation includes an additional block (+1). This might be intended to include the current block in the fetch operation, but it should be verified against the intended behavior to avoid fetching an extra or incorrect block.
🔒 Security concerns	No

Code feedback:

---

relevant file	src/bin/importer_online.rs
suggestion	Consider adding a comment explaining why +1 is added in the calculation of blocks_to_fetch. This will help maintain clarity and prevent future errors during maintenance. [important]
relevant line	let mut blocks_to_fetch = rpc_current_number.saturating_sub(number.as_u64()) + 1;

---

relevant file	src/bin/importer_online.rs
suggestion	Ensure that the tasks vector is properly cleared or managed if an error occurs during the block fetching loop to prevent memory leaks or unintended behavior. [medium]
relevant line	let mut tasks = Vec::with_capacity(blocks_to_fetch as usize);

---

relevant file	src/eth/block_miner.rs
suggestion	Verify the change of logging level from debug to info in the context of performance implications, especially if this logging is in a critical performance path. [medium]
relevant line	tracing::info!(number = %block.number(), transactions_len = %block.transactions.len(), "commiting block");

---

relevant file	src/bin/importer_online.rs
suggestion	Consider using a constant for the maximum number of blocks to fetch (1_000) to improve maintainability and configurability of the code. [medium]
relevant line	blocks_to_fetch = min(blocks_to_fetch, 1_000); # avoid spawning millions of tasks (not parallelism), at least until we know it is safe

---

[github-actions]

PR Code Suggestions ✨

Category	Suggestion	Score
Possible bug	Add a check for None before proceeding with the timeout operation on the subscription Consider adding a check to ensure that sub.next() does not return None before proceeding with the timeout operation. This will prevent potential runtime errors if the subscription unexpectedly ends. src/bin/importer_online.rs [189] -let resubscribe = match timeout(TIMEOUT_NEW_HEADS, sub.next()).await { +let next_sub = sub.next(); +if next_sub.is_none() { + tracing::error!("Subscription unexpectedly ended."); + return; +} +let resubscribe = match timeout(TIMEOUT_NEW_HEADS, next_sub).await { Suggestion importance[1-10]: 9 Why: This suggestion addresses a potential runtime error by adding a check for None before proceeding with the timeout operation. This is a significant improvement for robustness and error handling.	9
Possible issue	Verify the change of log level from debug to info to ensure it aligns with the intended logging policy The log level has been changed from debug to info. Ensure that this change is intentional and consistent with your logging policy, especially considering the verbosity and sensitivity of the information being logged. src/eth/block_miner.rs [209] -tracing::info!(number = %block.number(), transactions_len = %block.transactions.len(), "commiting block"); +// If the change was intentional, no action needed. Otherwise, revert to debug level: +tracing::debug!(number = %block.number(), transactions_len = %block.transactions.len(), "commiting block"); Suggestion importance[1-10]: 8 Why: Verifying the log level change is important to ensure it aligns with the logging policy. This suggestion is crucial for maintaining consistent logging practices and avoiding unnecessary verbosity.	8
Maintainability	Extract the calculation of blocks_to_fetch into a separate function for better code organization To improve the readability and maintainability of the code, consider extracting the calculation of blocks_to_fetch into a separate function. This function can handle the logic of saturating subtraction and limiting the number of blocks. src/bin/importer_online.rs [262-263] -let mut blocks_to_fetch = rpc_current_number.saturating_sub(number.as_u64()) + 1; -blocks_to_fetch = min(blocks_to_fetch, 1_000); +let mut blocks_to_fetch = calculate_blocks_to_fetch(rpc_current_number, number); +// New function +fn calculate_blocks_to_fetch(current_number: u64, number: u64) -> u64 { + let blocks = current_number.saturating_sub(number) + 1; + min(blocks, 1_000) +} + Suggestion importance[1-10]: 7 Why: Extracting the calculation into a separate function improves code readability and maintainability. While this is a good practice, it is not crucial for functionality, hence a moderate score.	7
Enhancement	Use a more descriptive name for the vector holding fetch tasks Consider using a more descriptive variable name than tasks for the vector that holds tasks for fetching blocks and receipts. A name like fetch_tasks would provide clearer context. src/bin/importer_online.rs [265] -let mut tasks = Vec::with_capacity(blocks_to_fetch as usize); +let mut fetch_tasks = Vec::with_capacity(blocks_to_fetch as usize); Suggestion importance[1-10]: 6 Why: Using a more descriptive variable name improves code readability and maintainability. This is a minor enhancement but contributes positively to code clarity.	6