refactor: Use discovery service instead of Endpoint::add_node_addr
#108
+266
−14
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
4 tasks
|
Documentation for this PR has been generated and is available at: https://n0-computer.github.io/iroh-gossip/pr/108/docs/iroh_gossip/ Last updated: 2025-10-21T11:26:32Z |
Simulation reportLast updated: 2025-10-21T11:27:52Z |
This was referenced Oct 16, 2025
Closed
| rng: &mut impl CryptoRng, | ||
| ) -> n0_snafu::Result<(NodeId, Router, Gossip, GossipSender, GossipReceiver)> { | ||
| let topic_id = TopicId::from([0u8; 32]); | ||
| let ep = Endpoint::builder() |
There was a problem hiding this comment.
This PR is still on the last released iroh, will fix in #110
There was a problem hiding this comment.
right, too many APIs 😅
| Some(Ok(_)) | ||
| )); | ||
|
|
||
| tokio::time::sleep(Duration::from_millis(200)).await; |
There was a problem hiding this comment.
you could use the paused tests to avoid making this actually wait
There was a problem hiding this comment.
The entries use SystemTime so I don't think this would work? I think time::pause() only works for Instants.
| loop { | ||
| n0_future::time::sleep(opts.evict_interval).await; | ||
| let Some(nodes) = nodes.upgrade() else { | ||
| break; |
There was a problem hiding this comment.
when would that happen? should maybe log sth as this means this task is dead?
There was a problem hiding this comment.
This only happens when the GossipDiscovery is dropped, and then it's fine to just break the task. I think it will depend on the drop order if this happens first or if the task is aborted. Fine to leave as is IMO.
Frando
force-pushed
the
Frando/discovery
branch
from
ccee482 to
d0d2ba2
Compare
dignifiedquire
approved these changes
Oct 21, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
iroh-gossip includes a node's addressing information in all Join and ForwardJoin messages (in the peer_data field). This is used by receives of the messages to be able to dial the sender directly, even when not using external discovery mechanisms.
We are using
Endpoint::add_node_addr_with_sourceto add the received addr info to the endpoint. In iroh 0.93,Endpoint::add_node_addrwas removed, andadd_node_addr_with_sourceshould have been removed as well, but was overlooked, and thus iroh-gossip 0.93 only works like previously right now because of an oversight in n0-computer/iroh#3338. We will removeadd_node_addr_with_sourcein 0.94 and maybe already deprecate sooner.This PR changes iroh-gossip to not use
Endpoint::add_node_addr_with_sourceanymore and instead get the received addressing info to the endpoint through a discoery service.The discovery service is fully internal atm: It is not exposed, only added to the endpoint and used from within the crate.
I first used a
StaticProviderbut that would keep the records forever, which is a memory leak for a long-running busy gossip node. So I switched to an impl that evicts entries after some time.Right now I hard-coded the retention to 5 minutes, evicting expired entries every 30s. Should this be configurable? Maybe yes, I think.
Breaking Changes
Notes & open questions
Should the retention settings be configurable? Could quite easily add it to the builder.
Previously, this "retention" was fully outside the control of either iroh-gossip or the user, as it was handled by iroh's magic socket internal pruning of unused entries from the node map. Would need to look up what intervals that are.
Change checklist