feat(iroh): Gossip client

[rklaehn]

Description

This makes gossip available in iroh. Using iroh-gossip directly, while not horrible, is a bit verbose.

Breaking Changes

Not sure. It exports a few more things, and adds things. But in theory it should not modify existing things.

There should be none, and the semver test seems to agree...

Notes & open questions

• There can be some scenarios where this can cause trouble. E.g. when subscribing and then unsubscribing to a topic that is also used for doc sync.

This ^ is taken care of, since docs no longer use subscribe_all!

How to deal with missed messages due to slow reader:

I think that relying on guaranteed delivery for gossip is not a good idea in any case. so maybe just sending a Lagged but then continuing is best? Forcing the client to resubscribe could be a bit annoying.

Change checklist

• Self-review.
• Documentation updates if relevant.
• Tests if relevant.
• All breaking changes documented.

[rklaehn]

I think it is safe to just ignore any messages that are not for us. See b88398e

Obviously you can mess with things by subscribing to a namespace that is currently being synced, but doing so accidentally is highly unlikely. And you can also mess with other stuff by using the low level blobs api, so 🤷 .

[rklaehn]

It seems that due to the fact that the rpc protocol is public, we can not ever do an additional rpc request without breaking compat.

--- failure constructible_struct_adds_field: externally-constructible struct adds field ---

Description:
A pub struct constructible with a struct literal has a new pub field. Existing struct literals must be updated to include the new field.
        ref: https://doc.rust-lang.org/reference/expressions/struct-expr.html
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.31.0/src/lints/constructible_struct_adds_field.ron

Failed in:
  field Iroh.gossip in /home/runner/work/iroh/iroh/iroh/src/client.rs:44

--- failure enum_variant_added: enum variant added on exhaustive enum ---

Description:
A publicly-visible enum without #[non_exhaustive] has a new variant.
        ref: https://doc.rust-lang.org/cargo/reference/semver.html#enum-variant-new
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.31.0/src/lints/enum_variant_added.ron

[dignifiedquire]

we need to mark the rpc enums as non exhaustive, that should solve this

[dignifiedquire]

longterm I wish the rpc types where all private anyway

[dignifiedquire]

It seems that the sync engine thinks that it "owns" gossip, so it uses subscribe_all

@Frando can we change this? seems unfortunate to subscribe to everything by default

[Frando]

It seems that the sync engine thinks that it "owns" gossip, so it uses subscribe_all

@Frando can we change this? seems unfortunate to subscribe to everything by default

I think we can, yes. What is our primitive of choice these days to merge a changing list of streams?

with futures and thus its merge combinators phased out in iroh - maybe https://docs.rs/futures-concurrency/latest/futures_concurrency/stream/trait.Merge.html ?

[dignifiedquire]

• https://docs.rs/futures-lite/latest/futures_lite/stream/trait.StreamExt.html#method.race

[Frando]

I did not yet give the new gossip dispatcher module a real review, but my first thought is: What about we move the gossip_dispatcher from iroh to iroh-gossip? And make it the main interface. Because then we don't need broadcast channels at all anymore. And save one channel with its buffers. The sync engine would then use the gossip dispatcher, as would the RPC handlers. And if we get things right, less code - because the sync engine also dances around the different state (but your dispatcher has the nicer impl with the state enum).

[Frando]

#2265 changes the sync engine usage of gossip to not use subscribe_all (based on this branch).
However as outlined in the previous comment I think I'd prefer to move the new gossip_dispatcher module to iroh_gossip, and make both the node's gossip client and the node's sync engine use that dispatcher.

[rklaehn]

@Frando so the basic idea of the gossip dispatcher is just to have a thing that wraps the gossip and does some management so that it is less fragile to use. It does the join/unjoin etc. for you and deals with laggy subscribers.

If you agree with / can live with the public interface (which is just the subscribe function), we could move something with that signature into iroh-gossip as a higher level interface and then I would use it here, and you could update docs to use it (or not...).

The interface is basically this:

pub fn subscribe(
    &self,
    msg: GossipSubscribeRequest,
    updates: UpdateStream,
) -> impl Stream<Item = RpcResult<GossipSubscribeResponse>> {

where UpdateStream is just a stream of either broadcast or broadcast_neighbours:

/// Send a gossip message
#[derive(Serialize, Deserialize, Debug)]
pub enum GossipSubscribeUpdate {
    /// Broadcast a message to all nodes in the swarm
    Broadcast(Bytes),
    /// Broadcast a message to all direct neighbors
    BroadcastNeighbors(Bytes),
}

There is a hardcoded constant somewhere about how much you are allowed to lag before you are thrown out. We could make that configurable as well.

[Frando]

I think the API is fine. And still very much in favor of having only one and not 2 high level dispatch interfaces to gossip.

[rklaehn]

Ok, I will clean up the dispatcher a bit and make sure it is independent of the rpc protocol, in preparation for moving it into iroh-gossip.

[rklaehn]

Moved the dispatcher to iroh-gossip and moved the types into the dispatcher file.

I reexported some of the dispatcher types instead of making newtypes for them in the rpc protocol. Not 100% sure about this, but making so many single element newtypes is a drag...

One question is if the dispatcher should present a simplified interface for creation. Currently it takes a Gossip. But I don't really see how this can be made really nice. Take an endpoint and an options object?

[rklaehn]

This is missing adapting the docs stuff to the new gossip dispatcher. Other than that I think it is good to go.

[rklaehn]

@Frando I have added a test. It seems to work, now that I have added the ability to tell the endpoint about node addrs (see #2433 ).

I think this kind of test is nice, because it forces us to make the client API sufficiently rich that you can do tests with it. Also it is much less likely to fluke out than a full cli type tests.

But - do you think this test for gossip will reliably work? I am connecting two nodes and then send a msg from one to the next. Can there be a situation where I send the message too early and it gets lost?

[Frando]

But - do you think this test for gossip will reliably work? I am connecting two nodes and then send a msg from one to the next. Can there be a situation where I send the message too early and it gets lost?

Yes, Gossip::join is awaited here, and that waits for at least one neighbor to come up before completing.

So the remaining TODO is to merge the two dispatchers (in dispatcher.rs and net.rs) so that messages flow only over a single channel and not two. We should do this, but IMO it can also happen in a followup.