chore(p2p): dnsaddr recursive resolution #2204

rymnc · 2024-09-16T14:40:40Z

Linked Issues/PRs

fixes feat(p2p): Handle dnsaddr resolution #2202

Description

creates a new module dnsaddr_resolution which handles recursive dnsaddr resolution to add to the DHT without suffix matching. This way we can connect to all peers behind a domain without specifying the exact PeerId, like /dnsaddr/bootstrap.libp2p.io.
asyncifies some of the methods related to mounting the p2p service
resultifies some of the methods related to mounting the p2p service

Checklist

Breaking changes are clearly marked as such in the PR description and changelog
New behavior is reflected in tests
The specification matches the implemented behavior (link update PR if changes are needed)

Before requesting review

I have reviewed the code myself
I have created follow-up issues caused by this PR and linked them here

After merging, notify other teams

[Add or remove entries as needed]

Rust SDK
Sway compiler
Platform documentation (for out-of-organization contributors, the person merging the PR will do this)
Someone else?

xgreenx · 2024-09-16T21:54:36Z

crates/services/p2p/Cargo.toml

@@ -50,6 +50,7 @@ thiserror = "1.0.47"
 tokio = { workspace = true, features = ["sync"] }
 tracing = { workspace = true }
 void = "1"
+hickory-resolver = "0.25.0-alpha.2"


Can we use resolver from libp2p?

do you mean the same version as libp2p or to use their resolver?

their resolver is in libp2p-dns (https://docs.rs/libp2p-dns/latest/libp2p_dns/tokio/type.Transport.html) and doesn't export any helper functions to resolve dnsaddr's.

I see, they use the same library to do it. Yeah. we need to use the same version to not bloat the Cargo.lock

addressed in 4f4b662

xgreenx · 2024-09-17T11:39:57Z

crates/services/p2p/src/discovery/discovery_config.rs

+    let mut dnsaddr_multiaddrs = vec![];
+
+    for dnsaddr in dnsaddr_urls {
+        let multiaddrs = dns_resolver.lookup_dnsaddr(dnsaddr.as_ref()).await?;


What if dnsaddr will have another dnsaddr, will it work?

yup, resolution is recursive. the test case handles that :)

xgreenx · 2024-09-17T11:40:25Z

crates/services/p2p/src/discovery/dnsaddr_resolution.rs

+const MAX_DNS_LOOKUPS: usize = 10;
+
+#[async_trait::async_trait]
+pub trait DnsLookup {


Why do we need trait?

xgreenx · 2024-09-17T11:42:53Z

crates/services/p2p/src/discovery/discovery_config.rs

@@ -97,7 +102,7 @@ impl Config {
        self
    }

-    pub fn finish(self) -> Behaviour {
+    pub async fn finish(self) -> anyhow::Result<Behaviour> {


Just curious, have you considered implementation that doesn't require async during construction? Maybe it is possible to resolved addresses in the fn start?

do you mean here -

fuel-core/crates/services/p2p/src/p2p_service.rs

Line 238 in 1d1560f

pub async fn start(&mut self) -> anyhow::Result<()> {

?

addressed in 5b4671f

netrome

I hate to block this PR, but there are two pretty substantial issues I can see so far that I would like to have addressed.

I don't think we should use .now_or_never() to assert that config.finish().await is ready in build_behavior_fn. It is not clear to me that this future will always be ready when this is invoked.
The DNS resolution tests should not be dependent on the host machines local DNS cache or network connection. Instead I'd suggest creating a port for this, which would allow us to create more tests for different DNS configurations.

Moreover, if a port is introduced to allow the DNS lookup logic to be tested I'd be interested in seeing a test case covering recursive lookups to make sure they work as expected.

netrome · 2024-09-17T12:07:38Z

crates/services/p2p/src/discovery.rs

+            async move { config.finish().await }
+                .now_or_never()
+                .unwrap()
+                .unwrap()


I don't see how we can guarantee this future to be immediately ready. What happens if the DnsResolver::new().await? is pending for example? As I read this, we'd panic in that scenario. That feels quite brittle to me, in which case it would feel safer to just use the synchronous resolver instead.

My preferred option though would be to allow build_behavior_fn to return a function which returns a future, so we can return this future without having to panic if it isn't ready here.

deprecated in 5b4671f

Nice, thank you!

netrome · 2024-09-17T12:13:36Z

crates/services/p2p/src/discovery/discovery_config.rs

+    let dnsaddr_urls = multiaddrs
+        .iter()
+        .filter_map(|node| {
+            if let Protocol::Dnsaddr(multiaddr) = node.iter().next()? {
+                Some(multiaddr.clone())
+            } else {
+                None
+            }
+        })
+        .collect::<Vec<_>>();


Nit: This reads a bit funky to me. Wouldn't the singular of multiaddrs be multiaddr rather than node? I'd suggest this naming instead:

Suggested change

let dnsaddr_urls = multiaddrs

.iter()

.filter_map(|node| {

if let Protocol::Dnsaddr(multiaddr) = node.iter().next()? {

Some(multiaddr.clone())

} else {

None

}

})

.collect::<Vec<_>>();

let dnsaddr_urls = multiaddrs

.iter()

.filter_map(|multiaddr| {

if let Protocol::Dnsaddr(dnsaddr_url) = multiaddr.iter().next()? {

Some(dnsaddr_url.clone())

} else {

None

}

})

.collect::<Vec<_>>();

addressed in 0482f80

netrome · 2024-09-17T12:18:34Z

crates/services/p2p/src/discovery/dnsaddr_resolution.rs

+/// This limit is for preventing malicious or misconfigured DNS records from causing infinite recursion.
+const MAX_DNS_LOOKUPS: usize = 10;
+
+#[async_trait::async_trait]


Async traits have been stabilized since rust 1.75, so I'd prefer not to add this declaration for new traits. Instead just have the functions return impl Future<Output = ...> and use async fn in the implementations.

removed the trait

netrome · 2024-09-17T12:22:08Z

crates/services/p2p/src/discovery/dnsaddr_resolution.rs

+    ) -> Pin<
+        Box<dyn std::future::Future<Output = anyhow::Result<Vec<Multiaddr>>> + Send + 'a>,
+    > {


Do we need to do the Pin<Box<...>> here? That feels like an artifact of async_trait which should preferably be removed (as per my other suggestion), or if you want to keep the async_trait, this should happen in the trait implementation and not in this private helper method.

recursion needs us to pin the future

Ah right, makes sense 👍

netrome · 2024-09-17T12:27:32Z

crates/services/p2p/src/discovery/dnsaddr_resolution.rs

+        // given
+        let resolver = DnsResolver::new().await.unwrap();
+        // when
+        let multiaddrs = resolver
+            .lookup_dnsaddr("bootstrap.libp2p.io")
+            .await
+            .unwrap();
+        // then
+        assert!(!multiaddrs.is_empty());


Do we assume here that any machine running this test will always be able to look up the "bootstrap.libp2p.io" address in their environment?

For example, what happens if I'm flushing my local DNS cache and disconnect from the internet before running this test? As I read this, the test case would fail in that scenario.

I think we need to introduce a a port to manage the DNS lookup to make this logic testable.

I think introduction of the port will make this tests useless, since it will just test how port itself works. Plus I'm not sure how hard is it I think the introduction of the port will make these tests useless since they will just test how the port itself works. Plus I'm not sure how hard it is to write your own DNS resolver.

I'm okay with the idea that this test fails without a connection or local DNS resolution.

Having a test with real DNS proves that it works in a real environment. The libp2p library tests it in the same way=)

While I see your point, I have to respectfully disagree. I think the port will help us make the tests even more useful. Right now we can't assert much more than !multiaddrs.is_empty(), because there's no way for us to control which DNS records we get when we look up bootstrap.libp2p.io. With the port, we for example can test that our logic parses the TXT records correctly.

As for another example, there's currently no way to know from this test if this test is doing a recursive lookup or just a plain single lookup - and this can vary depending on how libp2p.io configures their records. With the trait we can write multiple test cases to test and make clear assertions about different scenarios.

I don't think it would be too hard to put a trait between our code and the TokioAsyncResolver, because as far as I can see, we're only calling TokioAsyncResolver::txt_lookup so we only need to mock one method for the tests.

Okay, I will try to put that in another way=)

The main functionality that I would like to see tested is that we can resolve the real addresses from ipfs(or fuel testnet/mainnet). So, I still want to see the test that uses real dnsaddr and verifies that TokioAsyncResolver works as expected.

The functionality of resolve_recursive and how this function works are not so important to me.
The approach with the port for internal resolver can help to test the behaviour of the resolve_recursive function in different use cases. And it looks like the right call to do it.

It is up to @rymn. I'm okay with doing that in a separate PR just dedicated to better test coverage of the resolve_recursive since the main feature is implemented and we have an integration test that covers this feature=)

how about this? I have some dnsaddr's on my personal domain anyway, we can just reuse those with recursion and assert strictly about the multiaddrs this function spits out?

Sure, I'd prefer if we test against fuel domains but I'm okay with any domain within our control so if you use your personal domain for now and create a follow-up to set up fuel domains that would be my preferred option. Any owner of a hard-coded domain in our tests will have the power to block our CI temporarily so I'd rather have it be someone in the team.

I'd still find the port solution more readable, since this scenario is only verifiable by manually doing DNS lookups to check which records exists on the host machine.

addressed in 8c77c1d

i won't touch the records, i promise 😂

Hahah pinky promise 😂 Nice comment also, super helpful thank you! 🙏

netrome

I'll re-review tonight, but my main concerns have been addressed. Thank you!

Concerns has been addressed, but I need to re-review before approving.

rymnc · 2024-09-17T14:42:27Z

crates/services/p2p/src/dnsaddr_resolution.rs

-            .lookup_dnsaddr("bootstrap.libp2p.io")
-            .await
-            .unwrap();
+        // run a `dig +short txt rymnc.com` to get the TXT records


err, this should be _dnsaddr.rymnc.com. patching.

xgreenx

LGTM=)

crates/services/p2p/src/p2p_service.rs

xgreenx · 2024-09-17T14:49:48Z

crates/services/p2p/src/dnsaddr_resolution.rs

+        // notice that it contains -
+        // `dnsaddr=/dnsaddr/zone-1.rymnc.com/tcp/4001/p2p/QmNnooDu7bfjPFoTZYxMNLWUQJyrVwtbZg5gBMjTezGAJN`
+        // which is a recursive call
+        let multiaddrs = resolver.lookup_dnsaddr("rymnc.com").await.unwrap();


Maybe it is better to use main net dnsaddr=)

addressed in 92f5f22

Co-authored-by: Green Baneling <XgreenX9999@gmail.com>

mchristopher · 2024-09-17T20:00:49Z

crates/services/p2p/src/dnsaddr_resolution.rs

+        // when
+        // run a `dig +short txt _dnsaddr.mainnet.fuel.network` to get the TXT records
+        let multiaddrs = resolver
+            .lookup_dnsaddr("mainnet.fuel.network")


Can we switch this to core-test.fuellabs.net? The records on the mainnet.fuel.network record can and will change over time, but I just setup the core-test DNS record to be static.

Thanks for setting this up. I created this follow-up issue to use the core-test.fuellabs.net hostname in the test, since this PR got auto-merged after my approval.

netrome

Looks good to me 👍 It would be nice to use the test records Mike suggested, but that can be done as a follow-up if you want to get this merged promptly for release.

netrome · 2024-09-17T20:16:00Z

crates/services/p2p/src/lib.rs

@@ -5,6 +5,7 @@ pub mod behavior;
 pub mod codecs;
 pub mod config;
 pub mod discovery;
+mod dnsaddr_resolution;


Nit: I'd move this declaration to a separate block than the public modules.

@rymnc

## Version v0.36.0 ### Added - [2135](#2135): Added metrics logging for number of blocks served over the p2p req/res protocol. - [2151](#2151): Added limitations on gas used during dry_run in API. - [2188](#2188): Added the new variant `V2` for the `ConsensusParameters` which contains the new `block_transaction_size_limit` parameter. - [2163](#2163): Added runnable task for fetching block committer data. - [2204](#2204): Added `dnsaddr` resolution for TLD without suffixes. ### Changed #### Breaking - [2199](#2199): Applying several breaking changes to the WASM interface from backlog: - Get the module to execute WASM byte code from the storage first, an fallback to the built-in version in the case of the `FUEL_ALWAYS_USE_WASM`. - Added `host_v1` with a new `peek_next_txs_size` method, that accepts `tx_number_limit` and `size_limit`. - Added new variant of the return type to pass the validation result. It removes block serialization and deserialization and should improve performance. - Added a V1 execution result type that uses `JSONError` instead of postcard serialized error. It adds flexibility of how variants of the error can be managed. More information about it in FuelLabs/fuel-vm#797. The change also moves `TooManyOutputs` error to the top. It shows that `JSONError` works as expected. - [2145](#2145): feat: Introduce time port in PoA service. - [2155](#2155): Added trait declaration for block committer data - [2142](#2142): Added benchmarks for varied forms of db lookups to assist in optimizations. - [2158](#2158): Log the public address of the signing key, if it is specified - [2188](#2188): Upgraded the `fuel-vm` to `0.57.0`. More information in the [release](https://github.com/FuelLabs/fuel-vm/releases/tag/v0.57.0). ## What's Changed * chore(p2p_service): add metrics for number of blocks requested over p2p req/res protocol by @rymnc in #2135 * Weekly `cargo update` by @github-actions in #2149 * Debug V1 algorightm and use more realistic values in gas price analysis by @MitchTurner in #2129 * feat(gas_price_service): include trait declaration for block committer data by @rymnc in #2155 * Convert gas price analysis tool to CLI by @MitchTurner in #2156 * chore: add benchmarks for varied forms of lookups by @rymnc in #2142 * Add label nochangelog on weekly cargo update by @AurelienFT in #2152 * Log consensus-key signer address if specified by @acerone85 in #2158 * chore(rocks_db): move ShallowTempDir to benches crate by @rymnc in #2168 * chore(benches): conditional dropping of databases in benchmarks by @rymnc in #2170 * feat: Introduce time port in PoA service by @netrome in #2145 * Get DA costs from predefined data by @MitchTurner in #2157 * chore(shallow_temp_dir): panic if not panicking by @rymnc in #2172 * chore: Add initial CODEOWNERS file by @netrome in #2179 * Weekly `cargo update` by @github-actions in #2177 * fix(db_lookup_times): rework core logic of benchmark by @rymnc in #2159 * Add verification on transaction dry_run that they don't spend more than block gas limit by @AurelienFT in #2151 * bug: fix algorithm overflow issues by @MitchTurner in #2173 * feat(gas_price_service): create runnable task for expensive background polling for da metadata by @rymnc in #2163 * Weekly `cargo update` by @github-actions in #2197 * Fix bug with gas price factor in V1 algorithm by @MitchTurner in #2201 * Applying several breaking changes to the WASM interface from backlog by @xgreenx in #2199 * chore(p2p): dnsaddr recursive resolution by @rymnc in #2204 ## New Contributors * @acerone85 made their first contribution in #2158 **Full Changelog**: v0.35.0...v0.36.0

rymnc added 2 commits September 16, 2024 20:08

chore(p2p): dnsaddr recursive resolution

1d1560f

fix: unwrap

599b49e

xgreenx reviewed Sep 16, 2024

View reviewed changes

fix: use same version of hickory resolver as from libp2p-dns

4f4b662

rymnc self-assigned this Sep 17, 2024

rymnc added 3 commits September 17, 2024 14:11

Merge branch 'master' into fix/p2p

2fde308

fix: changelog

903dfac

fix: unwrapping in test helpers

13c57f2

rymnc marked this pull request as ready for review September 17, 2024 10:15

rymnc requested review from Dentosal and MitchTurner as code owners September 17, 2024 10:15

rymnc requested review from xgreenx and a team September 17, 2024 10:15

xgreenx reviewed Sep 17, 2024

View reviewed changes

netrome self-requested a review September 17, 2024 11:53

netrome previously requested changes Sep 17, 2024

View reviewed changes

rymnc added 3 commits September 17, 2024 19:34

fix: move things around, remove trait

82ec625

fix: remove async where not required

5b4671f

fix: naming

0482f80

netrome self-requested a review September 17, 2024 14:20

netrome reviewed Sep 17, 2024

View reviewed changes

chore: deterministic dnsaddr

8c77c1d

rymnc commented Sep 17, 2024

View reviewed changes

fix: comment

0504c1a

xgreenx reviewed Sep 17, 2024

View reviewed changes

xgreenx and others added 4 commits September 17, 2024 16:50

Merge branch 'master' into fix/p2p

dcc3367

chore: cleanup result

c273cba

Co-authored-by: Green Baneling <XgreenX9999@gmail.com>

Merge branch 'master' into fix/p2p

689b822

fix: use mainnet urls

92f5f22

xgreenx previously approved these changes Sep 17, 2024

View reviewed changes

fix: clippy

cfd5503

rymnc dismissed xgreenx’s stale review via cfd5503 September 17, 2024 16:02

xgreenx approved these changes Sep 17, 2024

View reviewed changes

xgreenx requested a review from netrome September 17, 2024 16:29

xgreenx enabled auto-merge (squash) September 17, 2024 16:30

mchristopher reviewed Sep 17, 2024

View reviewed changes

netrome approved these changes Sep 17, 2024

View reviewed changes

xgreenx merged commit 20f3091 into master Sep 17, 2024
33 of 35 checks passed

xgreenx deleted the fix/p2p branch September 17, 2024 20:21

netrome mentioned this pull request Sep 17, 2024

Use core-test.fuellabs.net as hostname in DNS lookup test #2212

Closed

xgreenx mentioned this pull request Sep 17, 2024

Release v0.36.0 #2213

Merged

chore(p2p): dnsaddr recursive resolution #2204

chore(p2p): dnsaddr recursive resolution #2204

Conversation

rymnc commented Sep 16, 2024 • edited Loading

Linked Issues/PRs

Description

Checklist

Before requesting review

After merging, notify other teams

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

netrome left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

netrome left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xgreenx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

netrome left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rymnc commented Sep 16, 2024 •

edited

Loading