refactor!: streamline discovery and node info types

[Frando]

Description

Adds a NodeData struct that contains the information about a node that can be published in discovery services. This struct is both used in iroh_relay::dns::NodeInfo and in iroh::discovery::Discovery. Also, iroh::discovery::DiscoveryItem has no public fields anymore and uses NodeInfo internally to again reduce duplication. With these changes, the fields published about a node in discovery now are only defined in a single place.

This PR also serves these prupsoes:

• We want to add some "user defined data" to discovery (see feat(iroh): publish and resolve user-defined data in discovery #3176). This would add a third argument to Discovery::publish; with this PR instead we can just add it as an optional field to NodeData.
• It makes it possible to potentially add data to discovery after 1.0. The fields of NodeData are private, so we can add fields, and setter/getter methods, without this being a semver-breaking change.

A few other places are changed for consistency: StaticProvider now works with NodeInfo instead of NodeAddr (NodeInfo can be easily converted into NodeAddr). DnsProvider::lookup_node_by_id etc. now return NodeInfo instead of NodeAddr. A few method names are adjusted for consistency. NodeInfo is constructed in a builder-style fashion instead of passing all fields to new.

Breaking Changes

• iroh::discovery::Discovery::publish now takes data: &NodeData as its single argument. iroh::discovery::NodeData is a re-export of iroh_relay::dns::node_info::NodeData, and contains relay URL and direct addresses. See docs for NodeData for details.
• iroh::discovery::DiscoveryItem no longer has any public fields. There are now getters for the contained data, and constructors for createing a DiscoveryItem from a NodeInfo.
• iroh_relay::dns::node_info::NodeInfo is changed.
  • NodeInfo::new now has a single NodeId argument. Use NodeInfo::with_direct_addresses and NodeInfo::with_relay_url to set addresses and relay URL. Alternatively, use NodeInfo::from_parts and pass a NodeData struct.
  • NodeInfo now has two public fields node_id: NodeId and data: NodeData, and setter and getter methods for the relay URL and direct addresses.
• iroh::discovery::pkarr::PkarrPublisher::update_addr_info now takes a NodeData argument

Notes & open questions

Change checklist

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

[github-actions]

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

Last updated: 2025-02-18T14:34:00Z

[github-actions]

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: 1b2fda4

[ramfox]

Okay, my previous comment about the NodeInfo API comes from a bit of confusion on my part.

I very much agree with your goal of structuring this in a way where we can make future changes without breaking the 1.0 API.

What are your thoughts on how the current code in this PR is going to affect DiscoveryItem? Will the user defined data be a separate field? If so, we would still be breaking the discovery API each time we add anything to Discovery, since it would need to be reported in the DiscoveryItem.

DiscoveryItem currently contains a NodeAddr. NodeAddr currently contains node_id, relay_url, and direct_addresses, this is everything in the NodeInfo, only missing the future user defined data field.

So what if instead:

• NodeInfo is the opaque struct (rather than NodeData), with getters and setters and no public fields
• Discovery::publish now takes a NodeInfo (but we would likely tell discovery implementers to ignore the node_id field. this is the "orange" flag for this proposal, in my opinion)
• DiscoveryItem now contains a NodeInfo, rather than a NodeAddr, and we add a node_addr method in DiscoveryItem to smooth this transition.

I guess another option would be:

• NodeData is the opaque struct, with getters and setters and no public fields
• Discovery::publish takes a NodeData
• DiscoveryItem has a node_id field and a data field (w/ an item.node_addr() field to smooth the transition)

The reason I prefer the first option is we have one less public struct to maintain, though we would need to add documentation to the Discovery::publish trait that you should likely not be using the node_id field in NodeInfo for anything.

I appreciate the push to make the initial RFC more robust!

Thoughts?

[Frando]

Thanks for the review!

I agree that DiscoveryItem should reuse the NodeData or NodeInfo, so that there really is only one struct that contains all the info about a node that we support in discovery.

I'm less sure about publish taking NodeInfo which contains a node id. I think it is a bit of a bad API to tell people to ignore a field. So my gut feeling is to keep NodeData. What we could do is to have NodeInfo deref to NodeData, so that all methods on NodeData are available directly on NodeInfo as well, maybe I'll try this out next to see how it feels.

[Frando]

I pushed a commit that implements the following:

• NodeInfo has a NodeId and a NodeData. It derefs to NodeData, so methods from there are available directly on NodeInfo. It can be converted from and into a NodeAddr.
• NodeData has relay url and direct addresses (and soon user data). All fields are private, and there's getter and setter methods for all fields.
• DiscoveryItem now also has all fields private, and just stores a NodeInfo, the provenance and last_updated (which me might want to remove, but that's for another PR). No direct data fields anymore! It also derefs to NodeData, so you can just call item.relay_url() etc.

Also see generated docs: DiscoveryItem, NodeInfo, NodeData (actual doc comments will need another pass, but the structure is quite OK now I think?)

I'm quite happy with this, and it made #3176 even simpler.

Let me know what you think

[Frando]

I pushed another commit that adapts the StaticProvider to also use NodeInfowhere appropriate, so that adding more things to NodeInfo (i.e. UserData with #3176 ) just works.

[Frando]

I pushed a change that adds the builder-style API for NodeInfo that @ramfox suggested above (NodeInfo::new(node_id).with_relay_url(relay_url) - it makes the code using NodeInfo much nicer indeed.

[ramfox]

Woo! Looks good! Needs a rebase.

edit: looks like it doesn't need a rebase, not sure why github was showing me that before.