Return backwards compatible configuration with 2.1.x versions #6655
Conversation
📝 WalkthroughWalkthroughThe pull request introduces modifications to the HOPR protocol's configuration and network handling. Key changes include the adjustment of the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (2)
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: 2
🧹 Outside diff range and nitpick comments (3)
transport/network/src/config.rs (1)
51-52: Standardize duration function naming.There's an inconsistency in the naming convention. The file uses both
duration_1_s()andduration_1_sec()for 1-second durations. Consider standardizing to use one naming pattern throughout the codebase.- #[serde(default = "duration_1_sec")] - #[default(duration_1_sec())] + #[serde(default = "duration_1_s")] + #[default(duration_1_s())]hoprd/hoprd/example_cfg.yaml (1)
212-212: Consider adding a configuration commentSince this configuration will be revisited in 3.0, consider adding a comment to document this as a temporary backwards compatibility measure.
+ # Note: Set to 1s for 2.1.x backwards compatibility. Will be revised in 3.0 ignore_timeframe: 1transport/p2p/src/swarm.rs (1)
51-54: Consider improving error handling transparency.The implementation is correct and maintains backwards compatibility. However, the error handling could be more transparent by distinguishing between missing variables and parse errors in logs.
Consider this alternative implementation:
- let num_streams = std::env::var("HOPR_INTERNAL_LIBP2P_YAMUX_MAX_NUM_STREAMS") - .and_then(|v| v.parse::<usize>().map_err(|_e| std::env::VarError::NotPresent)) - .unwrap_or(1024); + let num_streams = match std::env::var("HOPR_INTERNAL_LIBP2P_YAMUX_MAX_NUM_STREAMS") { + Ok(v) => match v.parse::<usize>() { + Ok(n) => n, + Err(e) => { + debug!("Failed to parse HOPR_INTERNAL_LIBP2P_YAMUX_MAX_NUM_STREAMS: {}, using default", e); + 1024 + } + }, + Err(_) => 1024, + };
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
hoprd/hoprd/example_cfg.yaml(1 hunks)transport/network/src/config.rs(2 hunks)transport/p2p/src/swarm.rs(1 hunks)
🔇 Additional comments (2)
hoprd/hoprd/example_cfg.yaml (1)
212-212: LGTM: Change aligns with backwards compatibility goal
The reduction of ignore_timeframe from 600s to 1s restores the 2.1.x behavior, allowing for more responsive node interactions. The potential impact of frequent retries is adequately controlled by the existing backoff and quality parameters.
transport/p2p/src/swarm.rs (1)
51-54: LGTM! Changes align with backwards compatibility goals.
The modifications to the Yamux configuration are well-contained and maintain backwards compatibility with 2.1.x versions. The default value of 1024 streams matches the Mplex configuration, ensuring consistent behavior across protocols.
The configuration created a behavior different from 2.1.x which unnecessarily complicated the node interactions. The configuration will be revisited in 3.0 to set proper new values.
@ausias-armesto FYI.