feat: Add re-registration on expiry for p2p node #685
Conversation
aidan46
added
ready for review
| rendezvous::server::Event::RegistrationExpired(registration), | ||
| )) => { | ||
| tracing::info!( | ||
| "Registration for peer {} expired in namespace {}", |
There was a problem hiding this comment.
What do we do when the registration expires? What does it mean for the node to be registered?
There was a problem hiding this comment.
When the registration expires the registration node re-registers automatically.
A node being registered means that they have identified themselves with the bootstrap node and shared their Registration with them. This Registration holds information like their Peer ID and multiaddr.
aidan46
added
ready for review
| /// TTL of the p2p registration in seconds | ||
| #[serde(default = "default_registration_ttl")] | ||
| #[arg(long, default_value_t = DEFAULT_REGISTRATION_TTL)] | ||
| pub(crate) registration_ttl: u64, |
There was a problem hiding this comment.
I think it would be better if we use Duration here. That way the reader can immediately see how the duration is parsed
There was a problem hiding this comment.
For the tick it makes sense but we re-use this value for the register call in the swarm which expects a Option<u64>
There was a problem hiding this comment.
It's ok as is, but for reference, you can always just .as_secs
aidan46
added
ready for review
| registration.namespace | ||
| ); | ||
| } | ||
| other => tracing::debug!("Encountered event: {other:?}"), |
There was a problem hiding this comment.
Won't this catch the previous ConnectionEstablished and ConnectionClosed events?
| // Poll tick every TTL to re-register. | ||
| // First tick completes immediately. | ||
| _ = register_tick.tick() => { | ||
| tracing::info!("Registering with p2p node"); |
There was a problem hiding this comment.
I think @cernicc knows a bit more than me on this, but I think these logs should be contained inside an #[instrument] or a span to keep context
| return Err(P2PError::RegistrationFailed(rendezvous_node)); | ||
| } | ||
| other => tracing::debug!("Encountered event: {other:?}"), | ||
| } | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
A docstring would be appreciated
| @@ -158,11 +164,10 @@ pub async fn run_register_node( | |||
| }, | |||
| _ = token.cancelled() => { | |||
| tracing::info!("P2P node has been stopped by the cancellation token..."); | |||
| tracker.close(); | |||
| tracker.wait().await; | |||
There was a problem hiding this comment.
What is the reason for using TaskTracker here?
There was a problem hiding this comment.
Good catch, I don't even see what task this is tracking. I think this might be a leftover from an experiment?
Description
Add a TTL for the registration node and auto re-register when the registration expires. The re-registration is achieved using an Interval that ticks at expiry.
Something that does not seem right to me is when re-registering, the port of the node registering changes, see the logs below. I have tried unregistering first but this messes up the connection. I have also tried adding a delay but this does not solve it either. I have looked into the code of the rendezvous server register event and the Registrations add method, these seem to indicate that the old registration does get booted.
The above issue has been fixed by adding the external address only once. Adding the external address every time we register makes it change.
Logs from bootstrap node
INFO full: Peer 12D3KooWEs4rKaEAmcYpnjiPf834DTMfefeKe3SfFydepwANGWrx registered for namespace 'polka-storage' for 5 seconds Peer Addresses: [/ip4/127.0.0.1/tcp/54401] INFO full: Registration for peer 12D3KooWEs4rKaEAmcYpnjiPf834DTMfefeKe3SfFydepwANGWrx expired in namespace polka-storage INFO full: Connected to 12D3KooWEs4rKaEAmcYpnjiPf834DTMfefeKe3SfFydepwANGWrx INFO full: Peer 12D3KooWEs4rKaEAmcYpnjiPf834DTMfefeKe3SfFydepwANGWrx registered for namespace 'polka-storage' for 5 seconds Peer Addresses: [/ip4/127.0.0.1/tcp/54402, /ip4/127.0.0.1/tcp/54401]Logs from discovery node
INFO discover: New peer entered with addresses [ /ip4/127.0.0.1/tcp/54838, ] peer_id=12D3KooWEs4rKaEAmcYpnjiPf834DTMfefeKe3SfFydepwANGWrx INFO discover: Peer updated peer_id=12D3KooWEs4rKaEAmcYpnjiPf834DTMfefeKe3SfFydepwANGWrx address=/ip4/127.0.0.1/tcp/54841 INFO discover: Peer updated peer_id=12D3KooWEs4rKaEAmcYpnjiPf834DTMfefeKe3SfFydepwANGWrx address=/ip4/127.0.0.1/tcp/54842