fix: add timeout for DNS failures#6972
fix: add timeout for DNS failures#6972stringhandler merged 7 commits intotari-project:developmentfrom
Conversation
WalkthroughThis update introduces several improvements to the peer-to-peer (P2P) networking layer. The DNS resolver configuration is enhanced by adding a one-second timeout and enabling the "native-certs" feature. The default DNS seed hosts are expanded to include IPv4, IPv6, and Tor variants. DNS resolution and name server connection attempts now have explicit five-second timeouts with improved logging for successes, errors, and timeouts. Additionally, the parsing logic for seed peer strings is simplified to expect a single address per entry, and error logging is made more precise. Several asynchronous DNS client connection methods were converted to synchronous calls, simplifying control flow in multiple components. Changes
Sequence Diagram(s)sequenceDiagram
participant App
participant P2pInitializer
participant DNSResolver
participant Logger
App->>P2pInitializer: Start initialization
P2pInitializer->>DNSResolver: Resolve DNS seed (with 5s timeout)
DNSResolver-->>P2pInitializer: Return result or timeout
P2pInitializer->>Logger: Log elapsed time and result
P2pInitializer->>DNSResolver: Connect to DNS name server (returns future, no await)
DNSResolver-->>P2pInitializer: Return connection result or error
P2pInitializer->>Logger: Log connection status
P2pInitializer->>App: Continue initialization
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (4)
🔇 Additional comments (4)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 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
CodeRabbit Configuration File (
|
base_layer/p2p/src/initialization.rs
Outdated
| let res = match (dns_seeds_use_dnssec, dns == &DnsNameServer::System) { | ||
| (true, false) => DnsSeedResolver::connect_secure(dns.clone()).await, | ||
| (_, _) => DnsSeedResolver::connect(dns.clone()).await, | ||
| (true, false) => timeout(Duration::from_secs(5), DnsSeedResolver::connect_secure(dns.clone())).await, |
There was a problem hiding this comment.
connect and connect_secure are mistakenly marked as async functions and don't do IO, so they could be simplified
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
base_layer/p2p/src/dns/client.rs (1)
79-84: Make the 1‑second resolver timeout configurableHard–coding a 1 second timeout (
opts.timeout = Duration::from_secs(1)) improves failure‑detection on healthy networks, but it risks spurious timeouts on slower or high‑latency links (e.g. satellite or congested mobile networks).Consider exposing the value via configuration (or at least a
const) so that operators can tune it without recompiling.base_layer/p2p/src/config.rs (1)
77-91: Avoid repeatedNetwork::get_current_or_user_setting_or_default()callsYou call
Network::get_current_or_user_setting_or_default()three times in a row.
Compute it once to reduce overhead and improve readability:- dns_seeds: vec![ - format!( - "seeds.{}.tari.com", - Network::get_current_or_user_setting_or_default().as_key_str() - ), - format!( - "ip4.seeds.{}.tari.com", - Network::get_current_or_user_setting_or_default().as_key_str() - ), - format!( - "ip6.seeds.{}.tari.com", - Network::get_current_or_user_setting_or_default().as_key_str() - ), - ] + dns_seeds: { + let net = Network::get_current_or_user_setting_or_default().as_key_str(); + vec![ + format!("seeds.{net}.tari.com"), + format!("ip4.seeds.{net}.tari.com"), + format!("ip6.seeds.{net}.tari.com"), + ] + }base_layer/p2p/src/initialization.rs (2)
584-599: Consider removing commented-out codeThe old DNS connection handling code is commented out but should be removed if the new implementation is stable and tested.
- // match res { - // Ok(val) => { - // trace!(target: LOG_TARGET, "Found DNS client at '{}'", dns); - // return Ok(val); - // }, - // Err(err) => { - // warn!( - // target: LOG_TARGET, - // "DNS entry '{}' did not respond, trying the next one. You can edit 'dns_seed_name_servers' in - // \ the config file. (Error: {})", - // dns, - // err.to_string(), - // ); - // dns_errors.push(err.to_string()) - // }, - // }
514-527: Consider making timeout durations configurableThe 5-second timeout values are hardcoded in both DNS seed resolution and name server connection logic. Consider making these configurable in the P2pConfig to allow users to adjust based on their network conditions.
// Add to P2pConfig struct (in config.rs) + /// Timeout in seconds for DNS seed resolution + pub dns_seed_resolution_timeout_secs: u64, + /// Timeout in seconds for DNS name server connection + pub dns_name_server_connection_timeout_secs: u64, // Then use it in the code - let seeds_res = match timeout(Duration::from_secs(5), resolver.resolve(addr)).await { + let seeds_res = match timeout(Duration::from_secs(config.dns_seed_resolution_timeout_secs), resolver.resolve(addr)).await { // And similarly for the name server connection - (true, false) => timeout(Duration::from_secs(5), DnsSeedResolver::connect_secure(dns.clone())).await, - (_, _) => timeout(Duration::from_secs(5), DnsSeedResolver::connect(dns.clone())).await, + (true, false) => timeout(Duration::from_secs(config.dns_name_server_connection_timeout_secs), DnsSeedResolver::connect_secure(dns.clone())).await, + (_, _) => timeout(Duration::from_secs(config.dns_name_server_connection_timeout_secs), DnsSeedResolver::connect(dns.clone())).await,Also applies to: 569-583
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
base_layer/p2p/Cargo.toml(1 hunks)base_layer/p2p/src/config.rs(1 hunks)base_layer/p2p/src/dns/client.rs(1 hunks)base_layer/p2p/src/initialization.rs(3 hunks)base_layer/p2p/src/peer_seeds.rs(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
base_layer/p2p/src/config.rs (3)
common/src/configuration/network.rs (1)
get_current_or_user_setting_or_default(60-71)common/src/configuration/dns_name_server_list.rs (1)
from_str(34-42)common/src/configuration/name_server.rs (1)
from_str(68-87)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: test (mainnet, stagenet)
- GitHub Check: ci
🔇 Additional comments (6)
base_layer/p2p/Cargo.toml (1)
38-43: 👍 Enablingnative-certsis a sensible defaultIncluding the
native-certsfeature lets Hickory use the OS trust store, avoiding manual CA bundles. No further action required.base_layer/p2p/src/config.rs (1)
96-97: DNSSEC default toggled totrue– verify operational impactTurning DNSSEC on by default is a security win, but make sure:
- All default and documented public DNS servers in
dns_seed_name_serverssupport DNSSEC validation.- Nodes behind restrictive firewalls still resolve seeds successfully.
base_layer/p2p/src/peer_seeds.rs (1)
82-89: Nice: log only failed parsesChanging
inspect()➜inspect_err()stops logging successful parses, keeping logs clean.base_layer/p2p/src/initialization.rs (3)
79-82: Good addition of timeout importThe addition of
tokio::time::timeoutis appropriate for the implementation of timeout handling in DNS operations.
514-527: Excellent timeout implementation for DNS seed resolutionAdding a 5-second timeout for DNS seed resolution is a great improvement that prevents the application from hanging indefinitely when DNS servers are unresponsive. The additional logging provides valuable context about resolution timing.
569-583: Well-implemented DNS name server connection timeoutThe timeout implementation for DNS name server connections follows the same robust pattern as the seed resolution. This change properly logs timeouts separately from other connection errors, improving observability and debugging.
Test Results (CI) 2 files 77 suites 38m 41s ⏱️ For more details on these failures, see this check. Results for commit 0ec06ab. ♻️ This comment has been updated with latest results. |
base_layer/p2p/src/config.rs
Outdated
| ) | ||
| .expect("string is valid"), | ||
| dns_seeds_use_dnssec: false, | ||
| dns_seeds_use_dnssec: true, |
There was a problem hiding this comment.
DNSSec is not enabled on tari.com, is it worth enabling by default?
| ), | ||
| format!( | ||
| "ip6.seeds.{}.tari.com", | ||
| Network::get_current_or_user_setting_or_default().as_key_str() |
There was a problem hiding this comment.
we can also add tor.seeds.{}.tari.com, if we want to pull as many nodes?
stringhandler
merged commit 6dfff45
into
tari-project:development
4 participants
Summary by CodeRabbit
New Features
Improvements
Bug Fixes