radicle-dev / radicle-link

The second iteration of the Radicle code collaboration protocol.
Other
424 stars 39 forks source link

disocvery: Rework mechanism to not interfere with network stack #723

Open xla opened 3 years ago

xla commented 3 years ago

It has been shown that the current discovery design is not ideal and can interfere with the memberships protocol of handling active and passive sets see https://github.com/radicle-dev/radicle-link/pull/629#discussion_r617574089 https://github.com/radicle-dev/radicle-link/pull/696#discussion_r662311259

kim commented 3 years ago

The formulation as a stream is not entirely wrong, but somewhat misleading. The consumption of that stream is wrong, though. Consider:

After the initial bootstrapping, new peers are discovered via the membership protocol. In case we run out of reachable peers, we'd like to re-initiate the bootstrapping process (ie. reconnect). For this to work, we would either need to call some sort of reset on the datastructure containing the bootstrap nodes -- or keep using a stream, which, in the case of a static list, would simply cycle. The stream abstraction can also encapsulate dynamic discovery (eg. (m)DNS) more naturally (it wouldn't need to be Sync, I think).

There currently is a trait Discovery, which we never actually make useful use of:

pub trait Discovery {
    type Addr;
    type Stream: futures::Stream<Item = (PeerId, Vec<Self::Addr>)> + Send;

    fn discover(self) -> Self::Stream;
}

We could reformulate this as:

pub trait Discovery {
    type Addr;
    type Stream: futures::Stream<Item = (PeerId, Vec<Self::Addr>)> + Send;

    fn discover(&self) -> Self::Stream;
}

such that the protocol stack calls discover whenever it wants a (lazy) list of peers to connect to.

I'm not sure this would make things clearer, though, because ultimately this is isomorphic to just passing the stream.

tl;dr The only required change is to pull from the disco stream only when needed, which allows the Static discovery to become infinitary.