Skip to content
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

rendezvous: Refresh on external address changes #4627

Closed
dariusc93 opened this issue Oct 11, 2023 · 1 comment · Fixed by #4629
Closed

rendezvous: Refresh on external address changes #4627

dariusc93 opened this issue Oct 11, 2023 · 1 comment · Fixed by #4629

Comments

@dariusc93
Copy link
Member

Description

Have the rendezvous behaviour refresh its registration upon any external address changes

Motivation

When there is ever a change in an external address, we would have to register to the namespace on the rendezvous node so that it can have the up-to-date address of the peer. This would could create some complexity and be redundant to do since it may require

a. A wrapper around rendezvous behaviour but track registered namespaces and monitor FromSwarm::ExternalAddrConfirmed and FromSwarm::ExternalAddrExpired for any changes to register to the existing namespaces.

b. To monitor for any changes to Swarm::external_addresses and register to any namespaces so the node can be up to date. This would require checking in a loop of sorts.

Although the spec is not specific on updating on every change, it does state

Peers can refresh their registrations at any time with a new `REGISTER` message

With this said, I do believe it would be appropriate to automate this upon external address changes.

Current Implementation

Current implementation does not register upon any changes to the external addresses, however it does have placeholders that would allow us to implement this easily due to it tracking the external addresses

fn on_swarm_event(&mut self, event: FromSwarm<Self::ConnectionHandler>) {
self.external_addresses.on_swarm_event(&event);
self.inner.on_swarm_event(event);
}

While we do track discovered peers, we dont explicitly track namespaces we are successfully registered to based on the implementation client implementation so its hard to determine if we want to reuse discovered_peers keys when refreshing or if we want to have a separate field to track the successful registration to a given node.

discovered_peers: HashMap<(PeerId, Namespace), Vec<Multiaddr>>,

Are you planning to do it yourself in a pull request ?

Maybe

@thomaseizinger
Copy link
Contributor

This makes sense.

While we do track discovered peers, we dont explicitly track namespaces we are successfully registered to based on the implementation client implementation so its hard to determine if we want to reuse discovered_peers keys when refreshing or if we want to have a separate field to track the successful registration to a given node.

I'd opt for a separate field. I am not even sure our own registration is part of the mentioned map.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants