refactor: remove `AddrInfo` #3024

dignifiedquire · 2024-12-09T20:48:45Z

Description

Simplify type structures

TODO

fix tests
resolve ticket breakage

Breaking Changes

remove iroh_base::node_addr::AddrInfo
remove iroh_base::node_addr::AddrInfoOptions
introduce ticket feature for iroh_base, to use iroh_base::ticket

Notes & open questions

Change checklist

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

iroh/src/discovery.rs

rklaehn

As I have discussed with @flub some time ago, I am not a fan of getting rid of AddrInfo entirely. I think keeping AddrInfo just for discovery would give you the same benefits for the most common use cases while still making implementing and working with discovery more pleasant.

iroh/src/discovery.rs

flub

Looking good! 🚀

flub · 2024-12-10T09:05:56Z

iroh-base/src/node_addr.rs

@@ -92,17 +93,31 @@ impl NodeAddr {
    ///
    /// [discovery]: https://docs.rs/iroh/*/iroh/index.html#node-discovery
    pub fn apply_options(&mut self, opts: AddrInfoOptions) {
-        self.info.apply_options(opts);
+        match opts {
+            AddrInfoOptions::Id => {


I think AddrInfoOptions should probably be renamed to NodeAddrOptions

yeah, I will look at the fate of that again

removed, this was really not pulling its wait anymore, can be easily achieved through other means now

flub · 2024-12-10T09:07:01Z

iroh/Cargo.toml

@@ -177,7 +177,7 @@ name = "key"
 harness = false

 [features]
-default = ["metrics", "discovery-pkarr-dht"]
+default = ["metrics", "discovery-pkarr-dht", "discovery-local-network"]


Did you mean to change this default?

no this is just for easier debugging

iroh/src/discovery.rs

github-actions · 2024-12-10T10:14:58Z

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: d01f95e

dignifiedquire · 2024-12-10T11:43:29Z

@flub @rklaehn I have implemented internal legacy structs to keep the serialization stable

github-actions · 2024-12-10T11:53:21Z

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

Last updated: 2024-12-11T17:42:16Z

flub

I guess remaining is:

doing something about the AddrInfoOptions.
reverting the cargo features defaults

iroh-base/src/ticket/blob.rs

another attempt

Cleanup some changes from #3024 - Cleans up some `is_empty` logic and naming - fixes DNS publishing to only publish the relay url if one is available, and not the IP addresses Manual testing confirms, this fixes a regression of connections flip flopping between mixed and direct Closes #3081

link2xt · 2025-02-04T18:04:52Z

iroh-base/src/node_addr.rs

@@ -41,31 +41,34 @@ pub use crate::relay_url::RelayUrl;
 pub struct NodeAddr {
    /// The node's identifier.
    pub node_id: NodeId,
-    /// Addressing information to connect to [`Self::node_id`].
-    pub info: AddrInfo,


The change should have been probably marked as breaking because NodeAddr is serializable and Delta Chat serialized it into JSON and into QR codes then, so upgrading broke QR code compatibility: chatmail/core#6518

Ugh, sorry about that. We handled this for users who were serialising tickets: those are backwards compatible.

Should we add backwards compatibility via a custom Deserialize implementation in a future release?

No need to add anything, it's good enough. We have a test now to make sure it does not break again.

We ended up adding workaround anyway :)
https://github.com/deltachat/deltachat-core-rust/blob/302aa5a5f750d2d24211bff8e61eb2610470fca3/src/qr.rs#L264-L269
(it's fine, we have tests)

dignifiedquire added this to the v0.30.0 milestone Dec 9, 2024

rklaehn reviewed Dec 10, 2024

View reviewed changes

iroh/src/discovery.rs Show resolved Hide resolved

rklaehn reviewed Dec 10, 2024

View reviewed changes

iroh/src/discovery.rs Show resolved Hide resolved

flub reviewed Dec 10, 2024

View reviewed changes

dignifiedquire force-pushed the refactor-remove-addrinfo branch from 8a1ce36 to fda9400 Compare December 10, 2024 10:09

dignifiedquire force-pushed the refactor-remove-addrinfo branch from fda9400 to 1796746 Compare December 10, 2024 11:42

dignifiedquire marked this pull request as ready for review December 10, 2024 11:44

flub reviewed Dec 11, 2024

View reviewed changes

iroh-base/src/ticket/blob.rs Show resolved Hide resolved

dignifiedquire added 5 commits December 11, 2024 16:00

refactor: remove AddrInfo

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.

Learn about vigilant mode

bee17fe

another attempt

bring back is_empty

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

GPG key ID: 4AEE18F83AFDEB23
Expired

Verified
Learn about vigilant mode

33c4580

implement legacy format for tickets

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.

Learn about vigilant mode

15754ae

fixup docs

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.

Learn about vigilant mode

c91d69e

happy clippy

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

GPG key ID: 4AEE18F83AFDEB23
Expired

Verified
Learn about vigilant mode

5c45c6a

dignifiedquire force-pushed the refactor-remove-addrinfo branch from 2dcc6ab to 5c45c6a Compare December 11, 2024 15:00

dignifiedquire self-assigned this Dec 11, 2024

dignifiedquire added 2 commits December 11, 2024 18:38

remove AddrInfoOptions

8270355

fixup: undo default feature change

3e6cfe7

flub approved these changes Dec 11, 2024

View reviewed changes

dignifiedquire added this pull request to the merge queue Dec 11, 2024

Merged via the queue into main with commit 6a988a5 Dec 11, 2024
26 of 27 checks passed

zh522130 mentioned this pull request Dec 30, 2024

After the #3024 refactor, the hole punching success rate is very low. #3081

Closed

dignifiedquire mentioned this pull request Dec 30, 2024

fix: correctly set publishing details for pkarr records #3082

Merged

link2xt mentioned this pull request Feb 4, 2025

Setting up second device is incompatible between v1.155.2 and v1.155.1 chatmail/core#6518

Closed

link2xt reviewed Feb 4, 2025

View reviewed changes

dignifiedquire deleted the refactor-remove-addrinfo branch February 5, 2025 13:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: remove `AddrInfo` #3024

refactor: remove `AddrInfo` #3024

dignifiedquire commented Dec 9, 2024 •

edited

Loading

rklaehn left a comment

flub left a comment

flub Dec 10, 2024

dignifiedquire Dec 10, 2024 •

edited

Loading

dignifiedquire Dec 11, 2024

flub Dec 10, 2024

dignifiedquire Dec 10, 2024

github-actions bot commented Dec 10, 2024 •

edited

Loading

dignifiedquire commented Dec 10, 2024

github-actions bot commented Dec 10, 2024 •

edited

Loading

flub left a comment

link2xt Feb 4, 2025

flub Feb 5, 2025

matheus23 Feb 5, 2025

link2xt Feb 5, 2025

link2xt Feb 10, 2025 •

edited

Loading

refactor: remove AddrInfo #3024

refactor: remove AddrInfo #3024

Conversation

dignifiedquire commented Dec 9, 2024 • edited Loading

Description

Breaking Changes

Notes & open questions

Change checklist

rklaehn left a comment

Choose a reason for hiding this comment

flub left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dignifiedquire Dec 10, 2024 • edited Loading

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

github-actions bot commented Dec 10, 2024 • edited Loading

dignifiedquire commented Dec 10, 2024

github-actions bot commented Dec 10, 2024 • edited Loading

flub 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

link2xt Feb 10, 2025 • edited Loading

Choose a reason for hiding this comment

refactor: remove `AddrInfo` #3024

refactor: remove `AddrInfo` #3024

dignifiedquire commented Dec 9, 2024 •

edited

Loading

dignifiedquire Dec 10, 2024 •

edited

Loading

github-actions bot commented Dec 10, 2024 •

edited

Loading

github-actions bot commented Dec 10, 2024 •

edited

Loading

link2xt Feb 10, 2025 •

edited

Loading