Skip to content

deps!: update pkarr to v3 #3186

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 22 commits into from
Apr 30, 2025
Merged

deps!: update pkarr to v3 #3186

merged 22 commits into from
Apr 30, 2025

Conversation

Frando
Copy link
Member

@Frando Frando commented Feb 14, 2025

Description

Updates pkarr from v2 to v3. The pkarr client now supports both relays and DHT, so the DHT discovery code becomes a bit simpler.

Breaking Changes

  • pkarr::SignedPacket, as used as a parameter in iroh::dns::node_info::NodeInfo::to_pkarr_signed_packet and iroh::dns::node_info::NodeInfo::from_pkarr_signed_packet is now expecting pkarr at major version 3 instead of 2

Notes & open questions

Change checklist

  • Self-review.
  • Documentation updates following the style guide, if relevant.
  • Tests if relevant.
  • All breaking changes documented.

Copy link

github-actions bot commented Feb 14, 2025

Documentation for this PR has been generated and is available at: https://n0-computer.github.io/iroh/pr/3186/docs/iroh/

Last updated: 2025-04-30T10:14:29Z

Copy link

github-actions bot commented Feb 14, 2025

Netsim report & logs for this PR have been generated and is available at: LOGS
This report will remain available for 3 days.

Last updated for commit: 435ade7

@Nuhvi
Copy link
Contributor

Nuhvi commented Feb 14, 2025

@Frando The wasm error is what I get for updating to rand 0.9.0 which demands getrandom 0.3.0 which requires some massaging, even worse that sometimes you just have to do the 0.2.0 massaging as well if you have other dependencies that require it.

Long story short you need to add getrandom 0.3.0 with wasm_js feature and add this too https://github.com/pubky/pkarr/blob/main/pkarr/.cargo/config.toml

Edit: if you know a better way to do this, let me know, would love to learn.

@Nuhvi
Copy link
Contributor

Nuhvi commented Feb 14, 2025

I am not sure how would iroh-relay run in wasm, but anyway, I think the .cargo/config.toml should go in that crate.

@Frando Frando mentioned this pull request Feb 18, 2025
4 tasks
@Frando Frando changed the title deps: update pkarr to v3 deps!: update pkarr to v3 Feb 19, 2025
@Frando
Copy link
Member Author

Frando commented Feb 21, 2025

Note: While all tests pass this is not yet good to go. the PkarrClient in pkarr 3.4 has a bug in that it unconditionally DNS-resolves the default bootstrap URLs even if configured differently (i.e. without DHT or with different bootstrap URLs) because it does the resolving in a Default impl where this is not checked.

even when initializing a ClientBuilderwith no_default_networks or no_dht, the ClientBuilder::default which is called from Client::builder goes to Config::default which unconditionally calls Dht::builder, which at least in the most recent mainline release goes to Dhtbuilder::default and then mainline::rpc::Config::default which unconditionally calls bootstrap: to_socket_address(&DEFAULT_BOOTSTRAP_NODES) which (synchrounsly, blockingly!) DNS-resolves the default bootstrap URLs - even if the DHT is not used because no_default_networks is called
even if using the DHT, a sync-blocking call to to_socket_address for ~4 URLs is a bit of a nogo, it will block the tokio event loop for ~2s for me
but it definitely should not do that if the DHT is disabled in the pkarr client builder
(frando in discord)

So let's wait for a new release that fixes these issues.

@Nuhvi
Copy link
Contributor

Nuhvi commented Feb 21, 2025

@Frando published [email protected], and besides fixing the mainline issue, pkarr now re-exports mainline, so you can skip adding mainline as an explicit dependency.

However, Github CI has been failing for me for no reason, even when I rerun older commits that already passed, I seen no issues on mac or linux, so it should be fine, but I am not feeling too much confidence in CIs at the moment, so keep that in mind.

@Nuhvi
Copy link
Contributor

Nuhvi commented Mar 14, 2025

pkarr v3.6.0 have few bug fixes, but also enables setting timeout for requests to relays in wasm (after Reqwest published a new version).

@Frando
Copy link
Member Author

Frando commented Mar 14, 2025

Alright, I updated this PR:

  • Merged main
  • Update to pkarr v3.6
  • Remove explicit mainline dependency
  • Remove debug leftovers

Let's see what CI says, but I think this should be good to go now.

Requested a review from @rklaehn as the biggest changes are in the pkarr dht discovery impl. It became quite a bit simpler, as more of the logic to use both relays and the DHT is now handled by pkarr itself.

There's also some changes in the DNS server. To make sure that we don't break anything: @Arqu, can we testdrive this on staging or in a test environment somehow easily?

@Frando Frando marked this pull request as ready for review March 14, 2025 10:01
@Frando Frando requested a review from rklaehn March 14, 2025 10:10
@@ -101,13 +101,13 @@ mod tests {
30,
dns::rdata::RData::AAAA(Ipv6Addr::LOCALHOST.into()),
));
SignedPacket::from_packet(&keypair, &packet)?
SignedPacket::new(&keypair, &packet.answers, Timestamp::now())?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is not important here, but if there are other places where you have similar verbosity, reminder that the new SignedPacket::builder() api is much nicer.

@Arqu
Copy link
Collaborator

Arqu commented Mar 14, 2025

Alright, I updated this PR:

* Merged `main`

* Update to `pkarr v3.6`

* Remove explicit mainline dependency

* Remove debug leftovers

Let's see what CI says, but I think this should be good to go now.

Requested a review from @rklaehn as the biggest changes are in the pkarr dht discovery impl. It became quite a bit simpler, as more of the logic to use both relays and the DHT is now handled by pkarr itself.

There's also some changes in the DNS server. To make sure that we don't break anything: @Arqu, can we testdrive this on staging or in a test environment somehow easily?

Yes, we can set this up on the staging dns instance and do some tests.

tracing::info!("discovered node info from DHT {:?}", &node_info);
Some(Ok(DiscoveryItem::new(node_info, "mainline", None)))
tracing::info!("discovered node info {:?}", node_info);
Some(Ok(DiscoveryItem::new(node_info, "dht", None)))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this always dht or should we rename this to pkarr?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, it supports both relay and DHT, so changed to pkarr.

Copy link
Contributor

@rklaehn rklaehn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes seem like a straightforward API update.

I tried the dht_discovery example. It still seems to work.

Not sure if pkarr has gotten heavier with this update, but since it is an optional feature I guess we will have to live with it.

Did a quick cargo tree, and didn't see anything unusual. Just the usual rust madness.

@rklaehn rklaehn added this pull request to the merge queue Apr 30, 2025
Merged via the queue into main with commit 7b4bce8 Apr 30, 2025
26 checks passed
@github-project-automation github-project-automation bot moved this from 🏗 In progress to ✅ Done in iroh Apr 30, 2025
@matheus23 matheus23 deleted the deps/pkarr-v3 branch April 30, 2025 11:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

5 participants