Fix Fast-Sync CLI parameters and behavior when full re-sync is required #6645
Conversation
This PR fixes the following two issues: 1. When `hopr_logs.db` is present, the Index DB is empty, but Fast-Sync is disabled, the node should sync fully from the RPC. However, it was mistakenly using the last processed block from the `hopr_logs.db` 2. The CLI parameters where incorrectly parsed: the `--noXYY` or `--disableXYZ` CLI args are count-based, and therefore the corresponding config feature should be enabled when these CLI args are equal to 0.
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request includes updates to several files in the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
hoprd/hoprd/src/cli.rs (1)
Line range hint
148-154: Consider renaming CLI flag for consistencyThe current CLI flag
--disableUnrealizedBalanceCheckdoesn't follow the same pattern as other similar flags like--noKeepLogsand--noFastSync. Consider renaming it to--noUnrealizedBalanceCheckfor consistency.#[arg( - long = "disableUnrealizedBalanceCheck", + long = "noUnrealizedBalanceCheck", env = "HOPRD_DISABLE_UNREALIZED_BALANCE_CHECK", help = "Disables checking of unrealized balance before validating unacknowledged tickets.", action = ArgAction::Count )] pub no_check_unrealized_balance: u8,hoprd/hoprd/src/config.rs (2)
272-274: Enhance CLI flag behavior documentationWhile the comment correctly explains that count-based flags equal to 0 mean "not specified", it could be more explicit about the meaning of non-zero values.
Consider updating the comment to:
- // The --no*/--disable* CLI flags are Count-based, therefore, if they equal to 0, - // it means they have not been specified on the CLI and thus the - // corresponding config value should be enabled. + // The --no*/--disable* CLI flags are Count-based: + // - Value of 0: Flag not specified, feature should be enabled + // - Value > 0: Flag specified, feature should be disabled
276-278: Consider adding validation for CLI flag count valuesWhile the implementation correctly handles the count-based flags, it might be beneficial to add validation to ensure the count values are within expected ranges (e.g., 0 or 1).
Consider adding validation before the assignments:
+ // Validate CLI flag counts + for (flag_name, count) in [ + ("no_check_unrealized_balance", cli_args.no_check_unrealized_balance), + ("no_fast_sync", cli_args.no_fast_sync), + ("no_keep_logs", cli_args.no_keep_logs), + ] { + if count > 1 { + warn!("CLI flag --{} was specified {} times. Any non-zero count disables the feature.", + flag_name, count); + } + } + cfg.hopr.chain.check_unrealized_balance = cli_args.no_check_unrealized_balance == 0; cfg.hopr.chain.fast_sync = cli_args.no_fast_sync == 0; cfg.hopr.chain.keep_logs = cli_args.no_keep_logs == 0;chain/indexer/src/block.rs (1)
Line range hint
500-1000: Consider adding edge case testsThe test suite is comprehensive but could benefit from additional test cases:
- Network partition scenarios during sync
- Database corruption recovery
- Memory pressure situations
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
chain/indexer/Cargo.toml(1 hunks)chain/indexer/src/block.rs(1 hunks)chain/indexer/src/config.rs(1 hunks)hoprd/hoprd/src/cli.rs(1 hunks)hoprd/hoprd/src/config.rs(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- chain/indexer/Cargo.toml
🔇 Additional comments (6)
chain/indexer/src/config.rs (2)
17-18: LGTM! Documentation improvement is clear and helpful.
The added documentation clearly specifies the default value, which improves configuration transparency.
19-20: Verify the default value's impact across the codebase.
The explicit default of true for fast_sync aligns well with the PR's objective to fix Fast-Sync behavior. However, let's verify there are no assumptions of different default values elsewhere in the codebase.
✅ Verification successful
Let me generate additional scripts to search for fast_sync usage patterns more thoroughly:
Default value of fast_sync is consistent with codebase usage
The codebase analysis shows that the default value of true for fast_sync is appropriate and consistent with the codebase:
- The default is explicitly overridden to
falseonly in test scenarios where specific behavior testing is needed - The configuration examples in
hoprd/example_cfg.yamland deployment configs explicitly set it totrue - The README documentation recommends setting
fast_synctotruefor optimal performance - The implementation in
block.rshandles both true/false cases appropriately with proper warning messages
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for any code that might assume a different default value for fast_sync
# Look for direct references to IndexerConfig::default()
rg -A 3 "IndexerConfig::default"
# Look for places where fast_sync is checked, to verify consistent behavior
ast-grep --pattern 'if $x.fast_sync { $$$ }'
ast-grep --pattern 'if !$x.fast_sync { $$$ }'
# Look for test cases that might assume different defaults
rg -l "test" | xargs rg "fast_sync.*false"
Length of output: 1451
Script:
#!/bin/bash
# Search for any usage of fast_sync in the codebase
rg -A 3 "fast_sync"
# Look for struct initialization patterns that might set fast_sync
ast-grep --pattern 'IndexerConfig { $$$, fast_sync: $_, $$$}'
# Look for any test modules that might configure fast_sync
rg -p "mod test" -A 10 | rg -A 5 "fast_sync"
Length of output: 4749
hoprd/hoprd/src/cli.rs (1)
153-153: LGTM! Field rename improves code consistency
The rename from check_unrealized_balance to no_check_unrealized_balance better aligns with the codebase's convention for disable flags and matches the existing clap attribute disableUnrealizedBalanceCheck.
hoprd/hoprd/src/config.rs (1)
266-270: LGTM: Consistent max_block_range update across networks
The implementation correctly ensures that the max_block_range setting is consistently applied across all networks in the configuration.
chain/indexer/src/block.rs (2)
151-158: LGTM! Clear and robust handling of fast-sync scenarios.
The code correctly handles all combinations of fast-sync configuration and index database state:
- When fast-sync is disabled but index is empty: Performs full re-sync
- When fast-sync is disabled and index exists: Continues normal sync
- Appropriate cleanup of database state is performed when needed
The logging messages are clear and informative, helping with debugging and monitoring.
Line range hint 1-500: Verify thread safety and resource management
The implementation uses atomic operations and proper error handling, but we should verify:
- Thread safety of concurrent database operations
- Resource cleanup on error paths
- Performance impact of full re-sync operations
This PR fixes the following two issues:
When
hopr_logs.dbis present, the Index DB is empty, but Fast-Sync is disabled, the node should sync fully from the RPC. However, it was mistakenly using the last processed block from thehopr_logs.dbThe CLI parameters where incorrectly parsed: the
--noXYYor--disableXYZCLI args are count-based, and therefore the corresponding config feature should be enabled when these CLI args are equal to 0.