feat: new behavior for network bootstrapping #1257
Conversation
cylewitruk
added
sbtc signer binary
|
@matteojug I ended up doing quite a bit of refactoring, sorry :( So this probably needs a fresh look. |
| let mut addr = Multiaddr::empty(); | ||
| for part in self.iter() { | ||
| if matches!(part, Protocol::P2p(_)) { | ||
| break; | ||
| } | ||
| addr.push(part.clone()); | ||
| } | ||
| addr |
There was a problem hiding this comment.
If it's it guaranteed that p2p is the last part (as assumed by the check above), why not self.clone() + pop()?
| let multiaddr = Multiaddr::empty() | ||
| .with(Protocol::Ip4(IP4_LOOPBACK)) | ||
| .with(Protocol::Tcp(8080)) | ||
| .with(Protocol::P2p(PeerId::random())); | ||
| let expected = Multiaddr::empty() | ||
| .with(Protocol::Ip4(IP4_LOOPBACK)) | ||
| .with(Protocol::Tcp(8080)); | ||
| assert_eq!( | ||
| multiaddr.without_p2p_protocol(), | ||
| expected, | ||
| "ip4 + tcp + p2p" | ||
| ); |
There was a problem hiding this comment.
isn't this one the same as the one above?
| let multiaddr = Multiaddr::empty() | ||
| .with(Protocol::Ip4(IP4_LOOPBACK)) | ||
| .with(Protocol::Tcp(8080)) | ||
| .with(Protocol::P2p(PeerId::random())); | ||
| let expected = Multiaddr::empty() | ||
| .with(Protocol::Ip4(IP4_LOOPBACK)) | ||
| .with(Protocol::Tcp(8080)); | ||
| assert_eq!( | ||
| multiaddr.without_p2p_protocol(), | ||
| expected, | ||
| "ip4 + tcp + p2p" | ||
| ); |
There was a problem hiding this comment.
Also this one seems the same as the one above
| pub fn new(local_peer_id: PeerId) -> Self { | ||
| Self { | ||
| local_peer_id, | ||
| seed_addresses: Default::default(), | ||
| bootstrap_interval: Duration::from_secs(60), | ||
| initial_delay: Duration::from_secs(0), |
| } | ||
|
|
||
| /// Events emitted by the bootstrapping behavior. | ||
| #[allow(dead_code)] |
There was a problem hiding this comment.
nit: do we need the #[allow(dead_code)] here?
| } | ||
|
|
||
| /// Gets the [`PeerId`]s of all connected peers. | ||
| pub fn connected_peers(&self) -> Vec<(PeerId, Multiaddr)> { |
There was a problem hiding this comment.
Now that this is a vec, we no longer need to collect (let peers = swarm.connected_peers().collect::<Vec<_>>();) in libp2p event loop.
But I'm wondering if we really want this, eg we usually count the number of PeerId in logs, while with this now we may have duplicated (with different addresses). OTOH, it may be useful to have the addrs info. Maybe we should still return a map?
| // that this is a seed peer dial that we've initiated, so we publish | ||
| // the `DialingSeed` event. | ||
| if self.pending_connections.contains(&connection_id) { | ||
| tracing::debug!(%connection_id, addresses = ?addresses, peer_id = ?maybe_peer, "attempting to dial seed peer"); |
There was a problem hiding this comment.
nit?
| tracing::debug!(%connection_id, addresses = ?addresses, peer_id = ?maybe_peer, "attempting to dial seed peer"); | |
| tracing::debug!(%connection_id, ?addresses, peer_id = ?maybe_peer, "attempting to dial seed peer"); |
| ) -> std::task::Poll<libp2p::swarm::ToSwarm<Self::ToSwarm, libp2p::swarm::THandlerInEvent<Self>>> | ||
| { | ||
| // If we have any pending events, we return them immediately. | ||
| if let Some(event) = self.pending_events.pop_front() { |
There was a problem hiding this comment.
nit: use self.next_pending_event()? It may end up uglier, but at least we are using the same interface
| // If we've attempted to bootstrap recently, we wait until the interval has passed. | ||
| if let Some(last_bootstrap) = self.last_attempted_at { | ||
| if last_bootstrap.elapsed() < self.config.bootstrap_interval { | ||
| return self.next_pending_event(); | ||
| } | ||
| } |
There was a problem hiding this comment.
Sanity check: we are going to get poll'd even if there's no swarm activity, right? I'm thinking if it could happen that we need to bootstrap again, but we consume all the events so if we don't get poll'd anymore we would be stuck.
| if first_attempt.elapsed() < self.config.initial_delay { | ||
| return self.next_pending_event(); | ||
| } | ||
| } else if self.config.initial_delay > Duration::from_secs(0) { |
Description
Closes: #1133
Changes
Testing Information
Existing tests pass and I've tested this quite a bit in
devenv. I'll add some actual tests for the behavior, but PR'ing this now for preview.Checklist: