-
Notifications
You must be signed in to change notification settings - Fork 1.1k
feat(relay): allow enabling and disabling of relay HOP advertisement #6154
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
base: master
Are you sure you want to change the base?
Changes from 7 commits
fd2cbb9
714c97b
2249d1c
5565f8b
67d3f69
90e6d23
cc2ae7d
e5eded1
5eb01cc
2d38e9c
985d463
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -26,7 +26,7 @@ use std::{ | |
collections::{hash_map, HashMap, HashSet, VecDeque}, | ||
num::NonZeroU32, | ||
ops::Add, | ||
task::{Context, Poll}, | ||
task::{Context, Poll, Waker}, | ||
time::Duration, | ||
}; | ||
|
||
|
@@ -260,6 +260,21 @@ pub struct Behaviour { | |
queued_actions: VecDeque<ToSwarm<Event, THandlerInEvent<Self>>>, | ||
|
||
external_addresses: ExternalAddresses, | ||
|
||
status: Status, | ||
|
||
auto_status_change: bool, | ||
|
||
waker: Option<Waker>, | ||
} | ||
|
||
#[derive(PartialEq, Copy, Clone, Debug)] | ||
pub enum Status { | ||
/// Enables advertisement of the STOP protocol | ||
Enable, | ||
|
||
/// Disables advertisement of the STOP protocol | ||
Disable, | ||
dariusc93 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
|
||
impl Behaviour { | ||
|
@@ -271,6 +286,78 @@ impl Behaviour { | |
circuits: Default::default(), | ||
queued_actions: Default::default(), | ||
external_addresses: Default::default(), | ||
status: Status::Disable, | ||
auto_status_change: true, | ||
Comment on lines
+292
to
+293
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we please document this default behavior somewhere? |
||
waker: None, | ||
} | ||
} | ||
|
||
pub fn set_status(&mut self, status: Option<Status>) { | ||
match status { | ||
Some(status) => { | ||
self.status = status; | ||
self.auto_status_change = false; | ||
self.reconfigure_relay_status(); | ||
dariusc93 marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
} | ||
None => { | ||
self.auto_status_change = true; | ||
self.determine_relay_status_from_external_address(); | ||
} | ||
} | ||
if let Some(waker) = self.waker.take() { | ||
waker.wake(); | ||
} | ||
} | ||
|
||
fn reconfigure_relay_status(&mut self) { | ||
if self.reservations.is_empty() { | ||
return; | ||
} | ||
|
||
for (peer_id, connections) in self.reservations.iter() { | ||
self.queued_actions | ||
.extend(connections.iter().map(|id| ToSwarm::NotifyHandler { | ||
peer_id: *peer_id, | ||
handler: NotifyHandler::One(*id), | ||
event: Either::Left(handler::In::SetStatus { | ||
status: self.status, | ||
}), | ||
})); | ||
} | ||
|
||
} | ||
|
||
fn determine_relay_status_from_external_address(&mut self) { | ||
let old = self.status; | ||
|
||
self.status = match (self.external_addresses.as_slice(), self.status) { | ||
([], Status::Enable) => { | ||
tracing::debug!("disabling protocol advertisment because we no longer have any confirmed external addresses"); | ||
Status::Disable | ||
} | ||
([], Status::Disable) => { | ||
// Previously disabled because of no external addresses. | ||
Status::Disable | ||
} | ||
(confirmed_external_addresses, Status::Disable) => { | ||
debug_assert!( | ||
!confirmed_external_addresses.is_empty(), | ||
"Previous match arm handled empty list" | ||
); | ||
tracing::debug!("advertising protcol because we are now externally reachable"); | ||
Status::Enable | ||
} | ||
(confirmed_external_addresses, Status::Enable) => { | ||
debug_assert!( | ||
!confirmed_external_addresses.is_empty(), | ||
"Previous match arm handled empty list" | ||
); | ||
|
||
Status::Enable | ||
} | ||
}; | ||
|
||
if self.status != old { | ||
self.reconfigure_relay_status(); | ||
} | ||
} | ||
|
||
|
@@ -337,6 +424,7 @@ impl NetworkBehaviour for Behaviour { | |
local_addr: local_addr.clone(), | ||
send_back_addr: remote_addr.clone(), | ||
}, | ||
self.status, | ||
))) | ||
} | ||
|
||
|
@@ -364,11 +452,16 @@ impl NetworkBehaviour for Behaviour { | |
role_override, | ||
port_use, | ||
}, | ||
self.status, | ||
))) | ||
} | ||
|
||
fn on_swarm_event(&mut self, event: FromSwarm) { | ||
self.external_addresses.on_swarm_event(&event); | ||
let changed = self.external_addresses.on_swarm_event(&event); | ||
|
||
if self.auto_status_change && changed { | ||
self.determine_relay_status_from_external_address(); | ||
} | ||
|
||
if let FromSwarm::ConnectionClosed(connection_closed) = event { | ||
self.on_connection_closed(connection_closed) | ||
|
@@ -718,11 +811,16 @@ impl NetworkBehaviour for Behaviour { | |
} | ||
|
||
#[tracing::instrument(level = "trace", name = "NetworkBehaviour::poll", skip(self))] | ||
fn poll(&mut self, _: &mut Context<'_>) -> Poll<ToSwarm<Self::ToSwarm, THandlerInEvent<Self>>> { | ||
fn poll( | ||
&mut self, | ||
cx: &mut Context<'_>, | ||
) -> Poll<ToSwarm<Self::ToSwarm, THandlerInEvent<Self>>> { | ||
if let Some(to_swarm) = self.queued_actions.pop_front() { | ||
return Poll::Ready(to_swarm); | ||
} | ||
|
||
self.waker = Some(cx.waker().clone()); | ||
|
||
Poll::Pending | ||
} | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is a breaking change. Previously, the HOP advertisement was always enabled, whereas now it depends on external addresses being present. The behavior in the end is probably the same (reservations only work if external addresses are present), but the error on the client side will be a different one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was a bit torn on if this would be considered breaking or not since if the relay message doesnt contain the addresses (which in our case, external addresses), it would error on the client side when attempting to establish a reservation, though in this case it would give a different error message compared to previously where if the relay didnt have an external address since it would not be advertising its protocol without it being enabled in some manner. I guess in a sense, it would be though since it would be a different error than what is expecting (probably should check and see the errors myself too)