gossipsub: Topic Streams Extension - #729
Conversation
| If there are multiple streams for a single topic, the receiver SHOULD process | ||
| them in the order the streams were opened by the initiator. The receiver | ||
| SHOULD limit the number of concurrent topic streams for the same topic to 3 and | ||
| downscore peers that open more. Initiators SHOULD limit the number of | ||
| concurrent topic streams to 1 per topic. The initiator MUST close the old | ||
| stream before writing on a new stream for a given topic. |
There was a problem hiding this comment.
it's a bit confusing, why would there be 3 streams?
Is this because of subscribe, unsubscribe, subscribe, unsubscribe kind of pattern?
There was a problem hiding this comment.
right, or if the implementation opens a new stream per message. Because there is a potential for packets to be reordered or delayed, the sender should avoid concurrent streams that could appear to the receiver as multiple concurrent streams open at the same time.
Add a Shadow scenario that publishes a 1MiB message then a 1KiB message on different topics, and a log-timestamp check that the small message is not head-of-line blocked. Wire the go binary to the topic-streams pubsub fork (libp2p/specs#729) with WithTopicStreams enabled.
| If a receiver receives a message from a peer that violates a MUST condition, | ||
| the receiver MUST reset the connection to the peer and send error code | ||
| `0xd52505` when the transport allows it. This code signifies a Topic Streams |
There was a problem hiding this comment.
the message violates the condition, no? not the peer
| If a receiver receives a message from a peer that violates a MUST condition, | |
| the receiver MUST reset the connection to the peer and send error code | |
| `0xd52505` when the transport allows it. This code signifies a Topic Streams | |
| If a peer receives a message that violates a MUST condition, | |
| the receiver MUST reset the connection to the peer and send error code | |
| `0xd52505` when the transport allows it. This code signifies a Topic Streams |
| Protocol Violation error, and is derived from the first 3 bytes of the sha256 | ||
| hashsum of the string `gossipsub-topic-streams`. (i.e. `echo -n | ||
| "gossipsub-topic-streams" | sha256sum | head -c 6`) |
There was a problem hiding this comment.
Is this usual for specs to do? Otherwise, I see no point to explain where it comes from (maybe for curiosity in an appendix?
There was a problem hiding this comment.
Historically no, but that's partly because we didn't have great support for error codes. Now we have more support for error codes (at least in QUIC and Yamux transports), and it is helpful for debugging to know why a peer disconnected from you. Did you do something wrong? Are they saturated? Or are they going offline?
|
|
||
| ## Topic Streams | ||
|
|
||
| A peer opens a bidirectional stream for each topic that it wishes to send |
There was a problem hiding this comment.
| A peer opens a bidirectional stream for each topic that it wishes to send | |
| A peer MUST open a bidirectional stream for each topic that it wishes to send |
| The responder of the bidirectional stream MUST NOT write on the stream after | ||
| protocol negotiation completes. |
There was a problem hiding this comment.
So it is only bidirectional to accomodate the initial handshake? If so this should be stated explicitly
There was a problem hiding this comment.
The bidirectional nature is to be consistent with how Gossipsub uses streams. Unidirectional streams are only supported with QUIC.
Also, as you point out, bidirectional streams are needed for the multistream select flow. but nothing else in this protocol requires it.
Any suggested text? Happy to add it!
There was a problem hiding this comment.
Maybe:
The responder of the bidirectional stream MUST NOT write on the stream after protocol negotiation completes. The stream is only created bidirectional for the initial handshake.
|
|
||
| If there are multiple streams for a single topic, the receiver SHOULD process | ||
| them in the order the streams were opened by the initiator. The receiver | ||
| SHOULD limit the number of concurrent topic streams for the same topic to 3 and |
There was a problem hiding this comment.
I wonder if it'd be better to use variables here for this, as well as the stream reset timeout or other constants, and then add a table below with recommended default values for these.
| downscore peers that open more. Initiators SHOULD limit the number of | ||
| concurrent topic streams to 1 per topic. The initiator MUST close the old |
There was a problem hiding this comment.
Initiators SHOULD limit the number of concurrent topic streams to 1 per topic
Should we include an explanation about what to do when there are multiple concurrent topic streams for a single topic? Because I can't see how that would work exactly. If it makes no sense whatsoever, then maybe we can change that SHOULD to a MUST?
There was a problem hiding this comment.
Because of packet reordering, the receiver cannot know if the sender purposefully or accidentally had two or more concurrent topic streams for a given topic. If we make this a MUST, implementations may incorrectly mark a peer as misbehaving just because of packet reordering.
The idea here is that senders should not have more than one concurrent topic stream, this should limit, in practice, the number of concurrent streams a receiver could view.
There was a problem hiding this comment.
I see! This is tricky to word indeed. How about:
The initiator MUST close the existing topic stream for a topic before it opens a new one. Because the transport can reorder packets, the receiver can get a new stream before the close of the old one. This is not a violation. The receiver MUST accept up to 3 open streams for a topic and SHOULD process them in the sequence opened. If there are more than 3, the receiver SHOULD reset the excess streams and downscore the peer.
| If the receiver receives a topic stream for a topic it is not subscribed to and | ||
| has not recently published partial messages to (via fanout), it SHOULD | ||
| downscore the peer. The receiver MUST NOT downscore a peer for opening a topic |
There was a problem hiding this comment.
it SHOULD downscore the peer
Is this true for other spec violations here as well? Maybe we should include this sentence in other places as well
There was a problem hiding this comment.
Yes, I think it would be a good addition. Please open a separate PR and tag me for review!
| downscore the peer. The receiver MUST NOT downscore a peer for opening a topic | ||
| stream for a topic the receiver recently unsubscribed from, as the peer may not | ||
| have received the unsubscribe message before opening the topic stream. |
There was a problem hiding this comment.
nit
| downscore the peer. The receiver MUST NOT downscore a peer for opening a topic | |
| stream for a topic the receiver recently unsubscribed from, as the peer may not | |
| have received the unsubscribe message before opening the topic stream. | |
| downscore the peer. | |
| **Note:** The receiver MUST NOT downscore a peer for opening a topic | |
| stream for a topic the receiver recently unsubscribed from, as the peer may not | |
| have received the unsubscribe message before opening the topic stream. |
| `TopicRPC` messages MUST NOT be empty. They MUST contain either a partial or | ||
| publish message. The data length of the application message MUST be non zero. |
There was a problem hiding this comment.
The data length of the application message MUST be non zero
Is this restriction mandatory? because pubsub does allow empty data.
https://github.com/libp2p/specs/blob/master/pubsub/README.md#the-message
There was a problem hiding this comment.
It is when this extension is negotiated. This is intentional as it is likely incorrect or malicious behavior to have an empty data field.
| A topic stream is created when a node publishes a topic message to a peer. It is | ||
| closed when either the peer unsubscribes from the topic, or the publisher will | ||
| no longer publish on the topic. Either side may close the stream. |
There was a problem hiding this comment.
| A topic stream is created when a node publishes a topic message to a peer. It is | |
| closed when either the peer unsubscribes from the topic, or the publisher will | |
| no longer publish on the topic. Either side may close the stream. | |
| A topic stream is created when a node publishes a topic message to a peer. It is | |
| closed when the peer unsubscribes or the sender no longer intends to send | |
| messages on that topic. Either side may close the stream. |
Co-authored-by: Gabriel Cruz <8129788+gmelodie@users.noreply.github.com>
When receiving more than 3 concurrent streams
Attribute MUST-condition violations to the message, require opening a bidirectional stream per published topic, and specify concurrent-stream reordering: close-before-open, accept up to 3, excess streams are not a violation until that limit. Co-authored-by: Gabriel Cruz <8129788+gmelodie@users.noreply.github.com>
Gossipsub v1.3 uses a single stream per direction for all RPCs. This introduces some problems: unnecessary head of line blocking between messages (especially problematic when a large message in one topic delays small messages in another topic) and topic name overhead on each message.
The Topic Streams Extension addresses these problems. It moves topic scoped application messages to separate long-lived streams.