hoprd/hoprd/src/main.rs (1)

163-166: LGTM! Consider adding monitoring and documentation.

The change from thread stack size to max blocking threads configuration should help resolve the stack overflow crash by providing more threads for blocking operations. The increase to 1024 threads (from default 512) aligns with the throughput requirements discussed in the past review comments.

Recommendations:

1. Monitor thread usage and system resources to ensure the increased thread count doesn't cause resource pressure.
2. Consider adding a comment explaining why 1024 was chosen as the optimal value.

Add a comment explaining the rationale:

 let res = tokio::runtime::Builder::new_multi_thread()
     .enable_all()
+    // Increased from default 512 to handle more concurrent blocking operations
+    // and prevent stack overflow crashes in the indexer hot-path
     .max_blocking_threads(1024)
     .build()

chain/rpc/src/indexer.rs (5)

Line range hint 73-83: Enhance error context when fetching logs fails

The current error logging provides the block range but could include additional context such as the specific error code or message to aid in debugging.

Consider updating the error log to include the error code:

 error!(
     from = ?subrange.get_from_block(),
     to = ?subrange.get_to_block(),
+    error_code = ?e.code(),
     error = %e,
     "failed to fetch logs in block subrange"
 );

---

Line range hint 101-114: Avoid panicking when the start block number exceeds the latest block number

Panicking within the try_stream_logs method can cause the entire application to crash unexpectedly. It's recommended to handle this scenario by returning a structured error so that the calling code can decide how to handle it gracefully.

Replace the panic with an error return:

- panic!("indexer start block number {from_block} is greater than the chain latest block number {latest_block} (diff {past_diff}) =>
- possible causes: chain reorg, RPC provider out of sync, corrupted DB =>
- possible solutions: change the RPC provider, reinitialize the DB");
+ return Err(RpcError::IndexerStartBlockTooHigh {
+     from_block,
+     latest_block,
+     past_diff,
+     message: "Possible causes include chain reorg, RPC provider out of sync, or corrupted DB. Possible solutions: change the RPC provider or reinitialize the DB.".to_string(),
+ });

Ensure that RpcError includes a variant IndexerStartBlockTooHigh to accommodate this error.

---

Line range hint 155-167: Handle unrecoverable RPC errors without panicking

Using panic! after exceeding MAX_LOOP_FAILURES can abruptly terminate the application. Instead, return an error to allow higher-level code to handle the situation appropriately.

Modify the code to return an error:

- panic!("!!! Cannot advance the chain indexing due to unrecoverable RPC errors.
-
- The RPC provider does not seem to be working correctly.
-
- The last encountered error was: {e}");
+ return Err(RpcError::UnrecoverableRpcError {
+     message: "Cannot advance the chain indexing due to unrecoverable RPC errors. The RPC provider may not be working correctly.".to_string(),
+     last_error: Box::new(e),
+ });

Update RpcError to include an UnrecoverableRpcError variant if it doesn't already exist.

---

Line range hint 200-202: Check for potential integer overflow when calculating delay

In the line where the delay is calculated:

futures_timer::Delay::new(past_diff as u32 * self.cfg.expected_block_time / 3).await;

Casting past_diff to u32 might cause an overflow if past_diff exceeds u32::MAX. This can happen if the block difference is large.

Consider using u64 for the calculation to prevent overflow:

- futures_timer::Delay::new(past_diff as u32 * self.cfg.expected_block_time / 3).await;
+ futures_timer::Delay::new(Duration::from_secs(past_diff * self.cfg.expected_block_time.as_secs() / 3)).await;

---

Line range hint 235-237: Use tokio::time::sleep instead of futures_timer::Delay

The futures_timer crate is no longer maintained. It's recommended to use tokio::time::sleep for compatibility and maintenance purposes.

Update the delay implementation:

- futures_timer::Delay::new(self.cfg.expected_block_time).await;
+ tokio::time::sleep(self.cfg.expected_block_time).await;

Ensure that the tokio features are enabled and that the rest of the codebase is compatible with tokio.

chain/indexer/src/block.rs (3)

341-346: Refactor to eliminate the boolean flag fetch_checksum_from_db

Passing a boolean flag like fetch_checksum_from_db to control function behavior can make the code harder to read and maintain. Consider refactoring by splitting process_block into two separate functions or using a strategy pattern to handle the different behaviors. This would improve code clarity and maintainability.

---

361-370: Simplify checksum handling to reduce complexity

The conditional logic based on fetch_checksum_from_db adds complexity to the checksum handling. Consider unifying the approach to obtaining last_log_checksum, possibly always fetching it from the database or restructuring the code to avoid the need for the flag altogether. This would simplify the code and reduce potential errors.

---

744-867: Refactor the test function to remove code duplication

The test_indexer_fast_sync_full_with_resume function contains duplicated code for "Run 1" and "Run 2". Extract common setup and assertions into helper functions or utilize test parameterization to reduce duplication. This refactoring will enhance readability and make the tests easier to maintain.

Configuration used: .coderabbit.yaml
Review profile: CHILL

#!/bin/bash
# Search for all calls to update_logs_checksums
rg -A 2 "update_logs_checksums\(\)" 

# Search for implementations to ensure they return Hash
ast-grep --pattern 'impl $_ for $_ {
  $$$
  async fn update_logs_checksums(&self) -> Result<Hash> {
    $$$
  }
}'

#!/bin/bash
# Search for trait implementations to ensure they've been updated
ast-grep --pattern 'impl $_ for $_ {
  $$$
  async fn get_logs_block_numbers(
    $$$
    processed: Option<bool>
  ) $$$
}'

#!/bin/bash
# Let's try a different approach to find implementations
# First, find the trait definition
echo "=== Searching for trait definition ==="
rg "trait.*get_logs_block_numbers" -A 5

echo -e "\n=== Searching for implementations ==="
# Search for impl blocks containing get_logs_block_numbers
rg "impl.*\{" -A 20 | rg -A 20 "get_logs_block_numbers"

#!/bin/bash
# Let's try a simpler search approach to find any mentions of get_logs_block_numbers
echo "=== Searching for get_logs_block_numbers mentions ==="
rg "get_logs_block_numbers" -B 2 -A 10

echo -e "\n=== Searching for files containing get_logs_block_numbers ==="
fd . -t f -x grep -l "get_logs_block_numbers" {}

#!/bin/bash
# Description: Find all calls to `get_logs_block_numbers` and check if they include the `processed` parameter.

rg 'get_logs_block_numbers\(' --type rust -A 2

chain/rpc/src/client.rs (2)

489-490: Consider using debug log level instead of info in SurfRequestor::new

The log statement info!(?cfg, "creating surf client"); provides useful information during debugging but may be too verbose at the info level in production environments. Consider changing it to debug! to reduce log verbosity during normal operation.

---

556-557: Consider using debug log level instead of info in ReqwestRequestor::new

The log statement info!(?cfg, "creating reqwest client"); provides useful debugging information but might be too verbose at the info level for production use. Changing it to debug! can help keep the logs cleaner while still providing necessary details during development.

Configuration used: .coderabbit.yaml
Review profile: CHILL

db/sql/src/logs.rs (1)

173-187: Improved query performance in get_logs_block_numbers

The changes to use LogStatus table directly with DISTINCT clause and optional processing status filter improve database performance by:

• Reducing JOIN operations by querying LogStatus directly
• Preventing duplicate block numbers
• Adding flexibility to filter by processing status

Consider adding an index on (processed, block_number) columns if not already present, to further optimize this query pattern.

Configuration used: .coderabbit.yaml
Review profile: CHILL

Learnt from: NumberFour8
PR: hoprnet/hoprnet#6142
File: hopr/hopr-lib/tests/chain_integration_tests.rs:40-56
Timestamp: 2024-11-12T10:56:01.095Z
Learning: The use of `unwrap` is acceptable in test scenarios within the `hoprnet` project, as it helps directly identify issues when tests fail.

chain/indexer/src/block.rs (2)

149-154: Consider adding more detailed logging for fast sync resume

The implementation correctly handles fast sync resume, but could benefit from additional logging to help diagnose issues.

-info!("Fast sync is enabled, but the index database is not empty. Fast sync will continue on existing unprocessed logs.");
+info!(
+    unprocessed_logs = ?log_block_numbers.len(),
+    "Fast sync is enabled, but the index database is not empty. Fast sync will continue on existing unprocessed logs."
+);

---

738-860: LGTM! Consider adding content verification

The test implementation thoroughly covers fast sync resume functionality. Consider adding assertions to verify the content of processed logs, not just their count.

 assert_eq!(db.get_logs_block_numbers(None, None, Some(true)).await?.len(), 4);
 assert_eq!(db.get_logs_block_numbers(None, None, Some(false)).await?.len(), 0);
+
+// Verify the content of processed logs
+let processed_logs = db.get_logs(None, None).await?;
+assert!(processed_logs.iter().all(|log| log.processed == Some(true)), "All logs should be processed");
+assert!(processed_logs.iter().any(|log| log.block_number == 3), "Should contain log from block 3");
+assert!(processed_logs.iter().any(|log| log.block_number == 4), "Should contain log from block 4");

Configuration used: .coderabbit.yaml
Review profile: CHILL

Learnt from: tolbrino
PR: hoprnet/hoprnet#6675
File: chain/indexer/src/block.rs:319-320
Timestamp: 2024-11-27T17:22:40.429Z
Learning: In the Rust file `chain/indexer/src/block.rs`, within the `process_block_by_id` function, if there is a block number mismatch in logs retrieved from the database, we use `panic!` to handle this unrecoverable error.