fix(p2p/dns): greatly simplify DNS TXT lookup#6922
fix(p2p/dns): greatly simplify DNS TXT lookup#6922SWvheerden merged 5 commits intotari-project:developmentfrom
Conversation
WalkthroughThe pull request removes the obsolete dependency Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant DNS as DnsClient
participant Resolver as TokioResolver
App->>DNS: connect_secure(name_server)
DNS->>Resolver: connect_dnssec(name_server)
Resolver-->>DNS: Connection established
DNS->>Resolver: lookup_txt(name)
Resolver-->>DNS: TXT record response
DNS-->>App: Return DNS response
sequenceDiagram
participant Pulse as TariPulseService
participant DnsCli as DnsClient
Pulse->>DnsCli: get_dns_client()
DnsCli->>DnsCli: query_txt(name)
DnsCli-->>Pulse: Return TXT result
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms (6)
🔇 Additional comments (7)
🪧 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
base_layer/p2p/Cargo.toml (1)
62-62: Update comment to reflect current versionThe cargo-machete comment is outdated as it still references alpha.2 while the code now uses alpha.4.
-ignored = ["hickory-proto"] #fix hickory-proto version to same alpha.2 +ignored = ["hickory-proto"] #fix hickory-proto version to same alpha.4base_layer/core/Cargo.toml (1)
132-132: Update comment to reflect current versionThe cargo-machete comment is outdated as it still references alpha.2 while the code now uses alpha.4.
-ignored = ["hickory-proto"] #fix hickory-proto version to same alpha.2 +ignored = ["hickory-proto"] #fix hickory-proto version to same alpha.4base_layer/core/src/base_node/tari_pulse_service/mod.rs (1)
180-189: Parsing DNS TXT records for checkpoints.This uses
split_once(':')and discards invalid entries silently. If you need more insights on malformed records, consider logging parse failures.base_layer/p2p/src/dns/client.rs (1)
50-71:query_txtparsing may benefit from multi-chunk handling.Currently, only the first length byte is stripped. DNS TXT records can contain multiple segments. If you anticipate multi-part TXT records, consider handling them accordingly.
📜 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 (13)
base_layer/core/Cargo.toml(1 hunks)base_layer/core/src/base_node/tari_pulse_service/20326.rsa(0 hunks)base_layer/core/src/base_node/tari_pulse_service/38696.rsa(0 hunks)base_layer/core/src/base_node/tari_pulse_service/mod.rs(4 hunks)base_layer/p2p/Cargo.toml(1 hunks)base_layer/p2p/src/auto_update/dns.rs(3 hunks)base_layer/p2p/src/dns/client.rs(2 hunks)base_layer/p2p/src/dns/error.rs(1 hunks)base_layer/p2p/src/dns/mock.rs(0 hunks)base_layer/p2p/src/dns/mod.rs(0 hunks)base_layer/p2p/src/dns/roots/mod.rs(0 hunks)base_layer/p2p/src/lib.rs(1 hunks)base_layer/p2p/src/peer_seeds.rs(2 hunks)
💤 Files with no reviewable changes (5)
- base_layer/core/src/base_node/tari_pulse_service/38696.rsa
- base_layer/p2p/src/dns/mod.rs
- base_layer/core/src/base_node/tari_pulse_service/20326.rsa
- base_layer/p2p/src/dns/roots/mod.rs
- base_layer/p2p/src/dns/mock.rs
🧰 Additional context used
🧬 Code Definitions (2)
base_layer/p2p/src/auto_update/dns.rs (3)
common/src/configuration/name_server.rs (2)
dns(125-125)from_str(68-87)base_layer/p2p/src/dns/client.rs (1)
connect_secure(40-43)base_layer/p2p/src/auto_update/mod.rs (2)
version(171-173)hash(162-164)
base_layer/core/src/base_node/tari_pulse_service/mod.rs (5)
applications/minotari_node/src/config.rs (1)
network(75-77)applications/minotari_node/src/builder.rs (2)
network(148-150)config(89-91)base_layer/core/src/base_node/service/service.rs (1)
new(108-128)base_layer/p2p/src/dns/client.rs (2)
connect(45-48)connect(104-123)base_layer/p2p/src/auto_update/dns.rs (1)
connect(47-56)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: test (nextnet, nextnet)
- GitHub Check: test (testnet, esmeralda)
- GitHub Check: cargo check with stable
- GitHub Check: Cucumber tests / FFI
- GitHub Check: Cucumber tests / Base Layer
🔇 Additional comments (19)
base_layer/p2p/src/lib.rs (1)
42-42: API visibility change is appropriateMaking the DNS module public aligns with the PR objective of simplifying DNS handling. This change allows the module to be accessed directly from outside the p2p crate, supporting better reusability of the DNS functionality.
base_layer/p2p/Cargo.toml (1)
42-43: Dependency upgrade and feature changes look goodThe changes correctly implement the PR objective:
- Upgrading hickory-resolver and hickory-proto to version 0.25.0-alpha.4
- Switching from dns-over-rustls to more feature-rich options (tokio-runtime, dns-over-tls, dns-over-native-tls)
- Removing the hickory-client dependency
This will provide better abstraction and reduce maintenance burden from frequent breaking changes.
base_layer/core/Cargo.toml (1)
99-99: Dependency update looks goodCorrectly removed hickory-client dependency and upgraded hickory-proto to version 0.25.0-alpha.4, which aligns with the PR objective of simplifying DNS TXT lookup.
base_layer/p2p/src/dns/error.rs (2)
25-26: Import statement cleanup is appropriateThe simplified import statement aligns with the removal of hickory-client dependency and reflects the more streamlined approach to error handling.
29-30: Error handling simplification looks goodProperly removed the ClientError variant and updated the ProtoError variant to use the fully qualified path from hickory-proto. The error message has been simplified which maintains clarity while reducing verbosity.
base_layer/p2p/src/peer_seeds.rs (2)
41-41: Use ofDnsClientimport looks correct.The new import aligns well with the secure connection calls below.
55-55: Verify security implications of removing custom trust anchors.This call to
DnsClient::connect_secureno longer takes a trust anchor parameter. Please confirm that default or system trust anchors meet your security requirements.base_layer/p2p/src/auto_update/dns.rs (2)
36-36: NewDnsClientimport usage is consistent.The added
use crate::dns::DnsClient;import is properly referenced below.
50-50: Confirm removal of trust anchor from secure connection.Previously, the secure connection also used a custom trust anchor. Now it strictly relies on system defaults. Ensure that this aligns with your overall security posture.
base_layer/core/src/base_node/tari_pulse_service/mod.rs (5)
23-37: Imports updated for new DNS approach.All newly added imports (e.g.,
tari_common::DnsNameServer,tari_p2p::dns::DnsClient) and references toMissedTickBehaviorintegrate cleanly with the existing code.
55-62: Returning static DNS checkpoint domains.Switching from inferred or dynamically constructed DNS names to fixed string literals simplifies maintenance. However, please confirm these domain strings are correct for each network.
67-67:dns_nameis now a static string reference.This choice is simpler than building up a
Stringor similar structure at runtime. No issues found.
74-74: Minimal overhead in cloning config, but acceptable.Cloning
configonly to access its network is minor. No immediate concerns.
83-86: Direct system DNS usage inget_dns_client.Falling back to
DnsNameServer::Systemis straightforward. Just confirm you don’t require DNSSEC or custom trust anchors here.base_layer/p2p/src/dns/client.rs (5)
23-29: New hickory resolver imports.Imports from
hickory_protoandhickory_resolverreflect the updated library usage. No concerns.
35-37: Refactor to a singleDnsClientstruct.Consolidating multiple variants into a single struct design clarifies usage across the codebase.
40-43:connect_secureno longer takes a trust anchor.Confirm that relying on system or default trust anchors still meets your DNSSEC requirements.
77-101: Client connection logic consolidated.Both
connect_dnssecandconnectare straightforward. Thetrust_negative_responsesis set tofalse, which may be appropriate, though confirm negative response handling is desired.
127-129: Newlookup_txtwrapper is clean.This minimal wrapper over
resolver.txt_lookupis consistent with the new design.
sdbondi
force-pushed
the
simplify-hickory-usage
branch
2 times, most recently
from
0d74fa9 to
da7d544
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
base_layer/core/src/base_node/tari_pulse_service/mod.rs (1)
74-74: Replaced manual DNS name assignment with theget_network_dns_namehelper.
This cleanly retrieves the DNS endpoint.You can avoid cloning the entire config if only
config.networkis required:- let dns_name = get_network_dns_name(config.clone().network); + let dns_name = get_network_dns_name(config.network);
📜 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 (14)
.license.ignore(0 hunks)base_layer/core/Cargo.toml(1 hunks)base_layer/core/src/base_node/tari_pulse_service/20326.rsa(0 hunks)base_layer/core/src/base_node/tari_pulse_service/38696.rsa(0 hunks)base_layer/core/src/base_node/tari_pulse_service/mod.rs(4 hunks)base_layer/p2p/Cargo.toml(1 hunks)base_layer/p2p/src/auto_update/dns.rs(3 hunks)base_layer/p2p/src/dns/client.rs(2 hunks)base_layer/p2p/src/dns/error.rs(1 hunks)base_layer/p2p/src/dns/mock.rs(0 hunks)base_layer/p2p/src/dns/mod.rs(0 hunks)base_layer/p2p/src/dns/roots/mod.rs(0 hunks)base_layer/p2p/src/lib.rs(1 hunks)base_layer/p2p/src/peer_seeds.rs(2 hunks)
💤 Files with no reviewable changes (6)
- base_layer/p2p/src/dns/mod.rs
- base_layer/core/src/base_node/tari_pulse_service/20326.rsa
- .license.ignore
- base_layer/core/src/base_node/tari_pulse_service/38696.rsa
- base_layer/p2p/src/dns/roots/mod.rs
- base_layer/p2p/src/dns/mock.rs
🚧 Files skipped from review as they are similar to previous changes (4)
- base_layer/core/Cargo.toml
- base_layer/p2p/src/lib.rs
- base_layer/p2p/src/dns/error.rs
- base_layer/p2p/Cargo.toml
🧰 Additional context used
🧬 Code Definitions (3)
base_layer/p2p/src/auto_update/dns.rs (4)
common/src/configuration/name_server.rs (2)
dns(125-125)from_str(68-87)base_layer/p2p/src/peer_seeds.rs (2)
connect_secure(54-57)from_str(111-125)base_layer/p2p/src/dns/client.rs (1)
connect_secure(44-47)base_layer/p2p/src/auto_update/mod.rs (2)
version(171-173)hash(162-164)
base_layer/p2p/src/peer_seeds.rs (2)
common/src/configuration/name_server.rs (1)
dns(125-125)base_layer/p2p/src/dns/client.rs (1)
connect_secure(44-47)
base_layer/core/src/base_node/tari_pulse_service/mod.rs (3)
base_layer/p2p/src/initialization.rs (1)
future(514-536)base_layer/p2p/src/dns/client.rs (2)
connect(49-52)connect(108-127)base_layer/p2p/src/auto_update/dns.rs (1)
connect(47-56)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: test (testnet, esmeralda)
- GitHub Check: test (mainnet, stagenet)
- GitHub Check: ci
- GitHub Check: cargo check with stable
🔇 Additional comments (18)
base_layer/p2p/src/peer_seeds.rs (2)
41-41: ImportingDnsClientdirectly fromcrate::dnsis clean and consistent.
No issues here.
55-55: Removal of trust anchor parameter inconnect_secureimproves simplicity.
This change aligns with the updated DNS client API and reduces complexity.base_layer/p2p/src/auto_update/dns.rs (2)
36-36: ImportingDnsClientcentralizes DNS functionality.
This is a straightforward import addition.
50-50: Simplifyingconnect_securecall by removing trust anchor parameter.
Matches the refactor in the DNS client and keeps code DRY.base_layer/core/src/base_node/tari_pulse_service/mod.rs (5)
23-23: New imports for DNS and async utilities.
These additions (e.g.,DnsNameServer,DnsClient, plus time-related imports) are correct and consistent with the rest of the module.Also applies to: 28-29, 33-33
55-62: Refactoringget_network_dns_nameto return a static string reference.
Returning&'static strinstead of creating a dynamicNameobject simplifies the code. Ensure you don’t need any dynamic DNS naming in the future.
67-67: Adopting a static string fordns_name.
Aligns with the revised function signature inget_network_dns_name.
83-84: Switchingget_dns_clientto returnDnsClient.
This matches the new DNS client approach and removes unneeded trust anchor handling.
180-186: Streamlined TXT record retrieval usingquery_txtandsplit_once.
The approach is readable and concise, returning a collection of(height, hash)tuples. Consider additional checks if multiple colons are expected or if data could contain more than one delimiter.base_layer/p2p/src/dns/client.rs (9)
23-33: Import changes align well with the PR objectiveThe updated imports show the transition from
hickory-clienttohickory-resolverandhickory-proto, which aligns with the PR's goal of simplifying DNS lookup by using a higher-level abstraction. The specific imports likeTokioResolverandTxtLookupindicate a more targeted approach to DNS resolution.
38-41: Good simplification from enum to structConverting
DnsClientfrom an enum with multiple variants to a simple struct with a single client field is a good architectural decision. This reduces complexity and eliminates the need for enum pattern matching throughout the codebase.
44-47: API simplification by removing trust_anchor parameterRemoving the
trust_anchorparameter fromconnect_securesimplifies the API and reduces maintenance burden. This change aligns with the PR objective of reducing the impact of breaking changes in the underlying libraries.
54-56: Simplified query interfaceThe implementation now directly uses
lookup_txtwhich provides a cleaner and more direct approach to TXT record lookups compared to the previous implementation.
80-83: Removed generic type parameter from Client structThe removal of the generic type parameter from the
Clientstruct simplifies the implementation. Using the concreteTokioResolverdirectly makes the code more straightforward and easier to understand.
86-106: Improved DNSSEC connection implementationThe new implementation of
connect_dnssechandling both system and custom DNS configurations is well-structured and comprehensive. The code now:
- Uses the system configuration when
DnsNameServer::Systemis specified- Properly configures TLS for custom DNS servers
- Handles error cases appropriately, such as missing DNS names for DNSSEC
This is a good improvement over the previous implementation.
108-127: Consistent implementation between connect methodsThe
connectmethod follows the same pattern asconnect_dnssec, maintaining consistency in the codebase. The only difference is usingProtocol::default()instead ofProtocol::Tls, which is appropriate for non-secure connections.
130-135: Streamlined lookup implementationThe new
lookup_txtmethod directly leverages theTokioResolver's built-in functionality, which simplifies the code significantly. This reduces the potential for bugs and makes future maintenance easier.
1-136:Details
✅ Verification successful
Verify the functionality with existing consumers
The refactoring looks good overall. Since this is a significant API change, it would be beneficial to verify that all consumers of this API have been updated accordingly.
🏁 Script executed:
#!/bin/bash # Find all places where the DnsClient is imported and used echo "Searching for DnsClient usage in the codebase..." rg -A 2 "use .*DnsClient" | grep -v "dns/client.rs" echo "Searching for connect_secure calls that might still use trust_anchor..." rg "connect_secure.*trust_anchor" echo "Searching for places that might still use DnsClient as an enum..." rg "DnsClient::(Secure|Normal|Mock)" echo "Checking imports of Client struct..." rg -A 2 "use .*Client" | grep -v "dns/client.rs"Length of output: 75544
Action: Confirm Updated API Usage Across Consumers
I’ve verified that consumers of the
DnsClientAPI now align with the refactored changes. Searches across the repository show that:
- Files such as
base_layer/p2p/src/peer_seeds.rs,base_layer/p2p/src/auto_update/dns.rs, andbase_layer/core/src/base_node/tari_pulse_service/mod.rsimport and useDnsClientas expected.- There are no remaining calls to
connect_secureusingtrust_anchor.- No legacy enum-like usage of
DnsClientremains.Based on these findings, the changes appear to be consistent with consumer expectations. However, please ensure that integration tests are run to validate runtime behavior.
Test Results (CI) 3 files 129 suites 47m 21s ⏱️ Results for commit cfd4eef. ♻️ This comment has been updated with latest results. |
Test Results (Integration tests)36 tests 36 ✅ 15m 51s ⏱️ Results for commit cfd4eef. ♻️ This comment has been updated with latest results. |
sdbondi
force-pushed
the
simplify-hickory-usage
branch
2 times, most recently
from
1ae1283 to
428c88a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
base_layer/p2p/src/dns/client.rs (1)
109-129: Consider refactoring duplicated configuration code!The implementation for standard DNS connection looks good. However, there's some duplication between this method and
connect_dnssecin how theNameServerConfigis created.Consider extracting the common configuration code to a private helper method to reduce duplication:
impl Client { pub async fn connect_dnssec(name_server: DnsNameServer) -> Result<Self, DnsClientError> { let resolver = match name_server { DnsNameServer::System => TokioResolver::from_system_conf(TokioConnectionProvider::default())?, DnsNameServer::Custom { addr, dns_name } => { let dns_name = dns_name.ok_or(DnsClientError::DnsNameRequiredForDnsSec)?; - let mut conf = ResolverConfig::new(); - conf.add_name_server(NameServerConfig { - socket_addr: addr, - protocol: Protocol::Tls, - tls_dns_name: Some(dns_name), - http_endpoint: None, - trust_negative_responses: false, - bind_addr: None, - tls_config: None, - }); - let opts = ResolverOpts::default(); - TokioResolver::tokio(conf, opts) + Self::create_resolver(addr, Some(dns_name), Protocol::Tls) }, }; Ok(Self { resolver }) } pub async fn connect(name_server: DnsNameServer) -> Result<Self, DnsClientError> { let resolver = match name_server { DnsNameServer::System => TokioResolver::from_system_conf(TokioConnectionProvider::default())?, DnsNameServer::Custom { addr, dns_name } => { - let mut conf = ResolverConfig::new(); - conf.add_name_server(NameServerConfig { - socket_addr: addr, - protocol: Protocol::default(), - tls_dns_name: dns_name, - http_endpoint: None, - trust_negative_responses: false, - bind_addr: None, - tls_config: None, - }); - let opts = ResolverOpts::default(); - TokioResolver::tokio(conf, opts) + Self::create_resolver(addr, dns_name, Protocol::default()) }, }; Ok(Self { resolver }) } + fn create_resolver( + socket_addr: std::net::SocketAddr, + tls_dns_name: Option<String>, + protocol: Protocol + ) -> TokioResolver { + let mut conf = ResolverConfig::new(); + conf.add_name_server(NameServerConfig { + socket_addr, + protocol, + tls_dns_name, + http_endpoint: None, + trust_negative_responses: false, + bind_addr: None, + tls_config: None, + }); + let opts = ResolverOpts::default(); + TokioResolver::tokio(conf, opts) + } }
📜 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 (14)
.license.ignore(0 hunks)base_layer/core/Cargo.toml(1 hunks)base_layer/core/src/base_node/tari_pulse_service/20326.rsa(0 hunks)base_layer/core/src/base_node/tari_pulse_service/38696.rsa(0 hunks)base_layer/core/src/base_node/tari_pulse_service/mod.rs(4 hunks)base_layer/p2p/Cargo.toml(2 hunks)base_layer/p2p/src/auto_update/dns.rs(3 hunks)base_layer/p2p/src/dns/client.rs(2 hunks)base_layer/p2p/src/dns/error.rs(1 hunks)base_layer/p2p/src/dns/mock.rs(0 hunks)base_layer/p2p/src/dns/mod.rs(0 hunks)base_layer/p2p/src/dns/roots/mod.rs(0 hunks)base_layer/p2p/src/lib.rs(1 hunks)base_layer/p2p/src/peer_seeds.rs(2 hunks)
💤 Files with no reviewable changes (6)
- base_layer/core/src/base_node/tari_pulse_service/38696.rsa
- base_layer/core/src/base_node/tari_pulse_service/20326.rsa
- .license.ignore
- base_layer/p2p/src/dns/mod.rs
- base_layer/p2p/src/dns/roots/mod.rs
- base_layer/p2p/src/dns/mock.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- base_layer/p2p/src/lib.rs
- base_layer/core/Cargo.toml
- base_layer/core/src/base_node/tari_pulse_service/mod.rs
🧰 Additional context used
🧬 Code Definitions (1)
base_layer/p2p/src/peer_seeds.rs (2)
common/src/configuration/name_server.rs (1)
dns(125-125)base_layer/p2p/src/dns/client.rs (1)
connect_secure(44-47)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: test (nextnet, nextnet)
- GitHub Check: test (testnet, esmeralda)
- GitHub Check: test (mainnet, stagenet)
- GitHub Check: cargo check with stable
- GitHub Check: ci
🔇 Additional comments (17)
base_layer/p2p/src/dns/error.rs (2)
25-25: Updated resolver dependency reflects project simplification.The import change from the previous resolver to
hickory_resolver::ResolveErroraligns with the PR objective to simplify DNS lookup by utilizing the hickory resolver.
29-30: Simplified error handling with updated hickory library.The error message and type have been updated to reflect the change from using
hickory-clientto usinghickory-protodirectly. This is part of the simplification effort mentioned in the PR objectives.base_layer/p2p/Cargo.toml (2)
36-38: Updated hickory dependencies as per PR objectives.The dependency updates align perfectly with the PR objective to upgrade the hickory library to version
0.25.0-alpha.4. The addition of specific features forhickory-resolver(tokio-runtime, dns-over-tls, dns-over-rustls) supports the goal of simplifying DNS TXT lookup code.
57-57: Cleaned up cargo-machete metadata.Removing
hickory-protofrom the ignored list in cargo-machete metadata is appropriate since the dependency is now correctly used in the codebase.base_layer/p2p/src/peer_seeds.rs (2)
41-41: Simplified module imports.Updated import to use the local DNS client implementation, which aligns with the overall code simplification objective.
54-56: Simplified DNS client connection API.Removed the
default_trust_anchor()parameter from theconnect_securemethod call, which streamlines the connection logic and aligns with the PR objective to simplify DNS TXT lookup using the hickory resolver.base_layer/p2p/src/auto_update/dns.rs (2)
36-36: Simplified module imports.Updated import to use the local DNS client implementation, consistent with changes in other files.
50-50: Simplified DNS client connection API.Removed the
default_trust_anchor()parameter from theconnect_securemethod call, maintaining consistency with similar changes in the peer_seeds.rs file.base_layer/p2p/src/dns/client.rs (9)
23-33: Import updates look good!The imports have been updated to properly reflect the transition from using hickory-client to hickory-resolver. This aligns with the PR objective of simplifying DNS TXT lookup by utilizing the hickory resolver directly.
38-41: Simplified design from enum to struct!The change from an enum-based
DnsClientwith multiple variants to a simple struct with aClientfield is a good architectural simplification. This design is cleaner, more maintainable, and aligns with the PR objective to simplify DNS lookups.
44-47: Improved API by removing unnecessary parameter!Removing the
trust_anchorparameter fromconnect_securesimplifies the API while maintaining functionality. The trust anchor handling is now likely handled internally by the hickory-resolver, which is a good abstraction.
49-52: Updated implementation is consistent with the new design!The
connectmethod has been properly updated to match the new struct-based design, maintaining consistent API while simplifying implementation.
54-56: Method now leverages the simplified client!The
query_txtmethod now properly delegates to the newlookup_txtmethod. This maintains the external API while benefiting from the internal simplification.
57-77: Processing logic preserved for compatibility!The logic for processing TXT records is preserved, ensuring backward compatibility while working with the new underlying resolver. Good attention to detail in maintaining the same behavior.
80-83: Simplified Client struct by removing generic parameter!The removal of the generic type parameter from the
Clientstruct simplifies the implementation and likely makes it more straightforward to use. This is in line with the simplification goal.
86-107: Clear implementation of DNSSEC connection!The
connect_dnssecmethod now directly creates aTokioResolverbased on the providedname_server, which simplifies the code. The handling of both system and custom DNS configurations is implemented clearly and effectively.I see proper error handling for the case when a DNS name is missing for custom servers with DNSSEC.
132-137: Clean and direct implementation of TXT lookup!The
lookup_txtmethod provides a clear and direct way to query TXT records using the hickory resolver. This simpler implementation replaces the more complex manual query construction from the previous version.
| #[test] | ||
| fn it_parses_update_spec_string() { | ||
| let update_spec = UpdateSpec::from_str("base-node:linux-x64:1.0.0:bada55").unwrap(); | ||
| assert_eq!(update_spec.application, ApplicationType::BaseNode); | ||
| assert_eq!(update_spec.arch, "linux-x64"); | ||
| assert_eq!(update_spec.version.to_string(), "1.0.0"); | ||
| assert_eq!(update_spec.hash, [0xBA, 0xDA, 0x55]); | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
❓ Verification inconclusive
Consider adding more test coverage for DNS client integration.
While the basic parsing test is maintained, it appears that several other test cases might have been removed. Consider adding tests for the new hickory-resolver integration to ensure the simplification doesn't introduce regressions.
🏁 Script executed:
#!/bin/bash
# Check if there were previously more tests in this file
git log -p -- base_layer/p2p/src/auto_update/dns.rs | grep -A 3 -B 3 "fn it_" | grep -v "it_parses_update_spec_string" | head -n 20Length of output: 0
Action Required: Enhance Test Coverage for DNS Client Integration
After reviewing the git commit history for base_layer/p2p/src/auto_update/dns.rs, there’s no evidence of previously existing tests besides the maintained basic parsing test (it_parses_update_spec_string). It appears that tests covering the new hickory-resolver integration or other DNS client functionalities are missing. Please consider adding tests that verify:
- The correct integration and resolution behavior for hickory-resolver.
- Additional DNS client edge cases to catch potential regressions.
• File: base_layer/p2p/src/auto_update/dns.rs
• Current Test: it_parses_update_spec_string
• Suggestion: Add integration tests for hickory-resolver and expanded DNS client behaviors.
sdbondi
force-pushed
the
simplify-hickory-usage
branch
from
428c88a to
9acfa77
Compare
sdbondi
force-pushed
the
simplify-hickory-usage
branch
from
9acfa77 to
223ea0b
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
base_layer/p2p/src/auto_update/dns.rs (1)
209-215: Basic test coverage is good—consider adding negative or edge-case tests.This straightforward test validates parsing a single well-formed record. Adding tests for invalid parts, extra parts, or empty fields can help catch parsing errors early.
📜 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 (14)
.license.ignore(0 hunks)base_layer/core/Cargo.toml(1 hunks)base_layer/core/src/base_node/tari_pulse_service/20326.rsa(0 hunks)base_layer/core/src/base_node/tari_pulse_service/38696.rsa(0 hunks)base_layer/core/src/base_node/tari_pulse_service/mod.rs(4 hunks)base_layer/p2p/Cargo.toml(2 hunks)base_layer/p2p/src/auto_update/dns.rs(3 hunks)base_layer/p2p/src/dns/client.rs(1 hunks)base_layer/p2p/src/dns/error.rs(1 hunks)base_layer/p2p/src/dns/mock.rs(0 hunks)base_layer/p2p/src/dns/mod.rs(0 hunks)base_layer/p2p/src/dns/roots/mod.rs(0 hunks)base_layer/p2p/src/lib.rs(1 hunks)base_layer/p2p/src/peer_seeds.rs(2 hunks)
💤 Files with no reviewable changes (6)
- base_layer/core/src/base_node/tari_pulse_service/20326.rsa
- .license.ignore
- base_layer/core/src/base_node/tari_pulse_service/38696.rsa
- base_layer/p2p/src/dns/mod.rs
- base_layer/p2p/src/dns/roots/mod.rs
- base_layer/p2p/src/dns/mock.rs
🚧 Files skipped from review as they are similar to previous changes (4)
- base_layer/p2p/src/dns/error.rs
- base_layer/p2p/src/lib.rs
- base_layer/p2p/src/peer_seeds.rs
- base_layer/core/src/base_node/tari_pulse_service/mod.rs
🧰 Additional context used
🧬 Code Definitions (2)
base_layer/p2p/src/auto_update/dns.rs (4)
common/src/configuration/name_server.rs (2)
dns(125-125)from_str(68-87)base_layer/p2p/src/peer_seeds.rs (2)
connect_secure(54-57)from_str(111-125)base_layer/p2p/src/dns/client.rs (1)
connect_secure(46-53)base_layer/p2p/src/auto_update/mod.rs (2)
version(171-173)hash(162-164)
base_layer/p2p/src/dns/client.rs (4)
common/src/configuration/name_server.rs (1)
DnsNameServer(128-128)base_layer/p2p/src/peer_seeds.rs (3)
connect_secure(54-57)connect(63-66)new(90-92)base_layer/p2p/src/auto_update/mod.rs (1)
default(80-91)base_layer/p2p/src/auto_update/dns.rs (1)
connect(47-56)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: test (testnet, esmeralda)
- GitHub Check: test (nextnet, nextnet)
- GitHub Check: test (mainnet, stagenet)
- GitHub Check: ci
- GitHub Check: cargo check with stable
- GitHub Check: Cucumber tests / FFI
- GitHub Check: Cucumber tests / Base Layer
🔇 Additional comments (13)
base_layer/core/Cargo.toml (1)
99-99: Dependency version alignment looks good.Upgrading
hickory-prototo=0.25.0-alpha.4is consistent with the rest of the PR. No immediate issues found here.base_layer/p2p/Cargo.toml (2)
37-38: Dependency updates are aligned with the resolver-based approach.Switching to
hickory-resolver = "=0.25.0-alpha.4"with TLS support and upgradinghickory-prototo the same version appears consistent with the removal ofhickory-client. Ensure these features have been tested to confirm secure DNS lookups work as expected.
56-56: Removal of the cargo-machete ignore list is appropriate.Clearing the ignore list for
hickory-protonow that it’s updated makes sense, allowing consistent version handling within the workspace.base_layer/p2p/src/auto_update/dns.rs (2)
36-36: New DNS client import is correct.Importing
DnsClientfrom the local crate aligns with the simplified resolver-based architecture.
50-50: Use of the new secure connection method.The call to
DnsClient::connect_secure(name_server)omits a trust anchor, matching the updated client API. Ensure no missed validations are required in your environment.base_layer/p2p/src/dns/client.rs (8)
23-27: Introduction ofhickory_protoimports is consistent.These imports match the usage of
IntoName,xfer::Protocol, and related features in the new resolver-based approach.
28-33: Resolver imports look correct.Bringing in
hickory_resolverand its config modules aligns with the new single client strategy.
38-38: Defined log target is clear.Using a dedicated log target (
tari::p2p::dns::client) aids in isolating DNS debug logs.
41-42: Unified struct-based client implementation is simpler and more maintainable.Replacing the enum-based approach with a single
DnsClientstruct clarifies the DNS workflow.
46-52: Secure connect approach is streamlined.Removing the trust anchor parameter reduces complexity. Verify any custom TLS requirements before provisioning in production.
55-61: Non-secure connect remains straightforward.This method cleanly mirrors the secure connect approach without TLS overhead.
64-81: Centralized resolver creation.
create_resolverconsolidates config for normal or TLS-based DNS usage. This promotes consistency and reduces duplication.
83-118: TXT query logic might need multi-chunk handling checks.The code takes the first length byte as the claims for the entire record. Some DNS servers provide multiple segments per TXT record. Verify or document acceptance criteria for multi-chunk scenarios.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
supply-chain/config.toml (1)
921-928: Verify Removal ofhickory-clientExemption
As described in the PR objectives and AI summary, the obsoletehickory-clientexemption has been removed from the configuration. Although no block for[[exemptions.hickory-client]]appears in the current file, please confirm that this removal does not affect any tools or dependencies still expecting it.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
supply-chain/config.toml(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: test (nextnet, nextnet)
- GitHub Check: test (testnet, esmeralda)
- GitHub Check: ci
- GitHub Check: cargo check with stable
🔇 Additional comments (2)
supply-chain/config.toml (2)
929-932: Updatehickory-protoVersion
The exemption forhickory-protohas been updated to version0.25.0-alpha.4, which is in line with the PR objectives. This update helps keep the dependency aligned with the latest changes, simplifying maintenance and ensuring compatibility.
932-935: Updatehickory-resolverVersion
Similarly, thehickory-resolverexemption now reflects version0.25.0-alpha.4. This consolidated version upgrade should reduce the impact of hickory updates on the codebase and maintain a smooth integration with the Tari pulse and P2P DNS modules.
* development: fix(p2p/dns): greatly simplify DNS TXT lookup (tari-project#6922) feat: handle outbound pipeline interlock due to connection cleanup (tari-project#6921) chore(ci): build docker with metrics (tari-project#6916)
* development: fix(p2p/dns): greatly simplify DNS TXT lookup (tari-project#6922) feat: handle outbound pipeline interlock due to connection cleanup (tari-project#6921) chore(ci): build docker with metrics (tari-project#6916)
* development: fix(p2p/dns): greatly simplify DNS TXT lookup (tari-project#6922) feat: handle outbound pipeline interlock due to connection cleanup (tari-project#6921) chore(ci): build docker with metrics (tari-project#6916)
* development: fix(p2p/dns): greatly simplify DNS TXT lookup (tari-project#6922) feat: handle outbound pipeline interlock due to connection cleanup (tari-project#6921) chore(ci): build docker with metrics (tari-project#6916)
* development: fix(p2p/dns): greatly simplify DNS TXT lookup (tari-project#6922) feat: handle outbound pipeline interlock due to connection cleanup (tari-project#6921) chore(ci): build docker with metrics (tari-project#6916)
4 participants
Description
0.25.0-alpha.4(from alpha.2)Motivation and Context
Hickory-client includes many breaking changes with practically every update (
0.25.0-alpha.2 -> 4is no exception), and this is a burden to maintain.The
hickory-resolveris a higher-level crate that abstracts away (hopefully most) breaking changes.The main reason for this PR is that I am updating the feature-dan2 branch to latest development, this led to the need to update our libp2p fork both to update tari-crypto and because libp2p also uses hickory and their version conflicts with the base layer version
0.25.0-alpha.2. I have updated to0.25.0-alpha.4(what libp2p currently uses) instead of0.25.1for this reason. Future hickory upgrades should be simple to implement.How Has This Been Tested?
Peer seed unit test that queries real DNS
What process can a PR reviewer use to test or verify this change?
Autoupdate, peer seeds and tari pulse should work as before
Breaking Changes
Summary by CodeRabbit
Summary by CodeRabbit
Overall, these improvements enhance performance and reliability while reducing complexity.