-
Notifications
You must be signed in to change notification settings - Fork 293
Gossipsub v1.3: Extensions control Message #684
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
I like the concept and the idea, I have some questions though. Thinking out-loud there is a bit of engineering complexity with this. The new control message MUST be sent as the first message and MUST only be sent once. As the protobuf allows these to be sent at any time, there is a bit of annoyance that we have to check its the first message, hasn't been sent a second time and reject all messages that may contain this extra control message. The reason I bring this up, is that in rust-libp2p, we already have all of this machinery for protocol-id. It can already only be sent once and we keep of track of each peer and what they support. Peers can send us a list of protocol-ids. Can't we have the same functionality where a peer supports say:
I guess the obvious downside is that multistream select chooses the "best" protocol-id to match on. So it would limit interactions between extensions, but a workaround would be choosing a different extension number that represents the interaction of multiple extensions, i.e extension 3 could mean support for extension 1 and 2. I only raise this, because I think this mentioned approach requires 0 engineering effort on our behalf and we dont have to add a control message. I'm not against adding this, just wanted to raise this as potentially a simpler possibility. |
Hey Age, thanks for the comments
The constraints should make it easier to implement. It is a bit more annoying to implement if you can expect an extensions message at any point. If the extensions message is sent a second time, you should disconnect as the peer is misbehaved. Making this be the first message and validating no future extension control messages arrive was <10 lines of code in the Go implementation.
Yes we could, but I think that's less ideal. For sake of argument, let's say we can encode extensions into the protocol ID in a way that extensions can be combined and we don't care about the encoding size. (We could for example give each extension a numeric ID, and attach them as a comma separated list after the If we rely purely on multistream semantics (no Identify to learn about a peers protocol list), this means we'd pay a round trip for each extension combination we'd want to support. If there are 3 extensions that can all be used independently, this would mean there could be 1 (all extensions) + 3 (only two) + 3 (only one) + 1 (no extensions)=8 round trips to negotiate the extensions with a single peer. This is pretty bad, so I won't dive further into this. If we use the Identify protocol (or similar) to first exchange a list of protocol IDs and maybe simply encode all supported extensions, instead of combinations. Then peers would know at the first multistream select round which protocol to offer, and the node would be fairly certain the other peer supports the offered extensions. This would work, but I'm not a fan of it for a couple reasons.
To address these issues I could imagine changes to core libp2p that:
This would be fine, but doesn't get us the 0 engineering effort we both would like. I'm not opposed to continue discussing using the protocol ID as maybe I've missed something. But I also want to hear more about what makes this proposal difficult to implement. |
We can put the extensions supported in the hello RPC, that should be backwards compatible (since it is protobuf, older clients can ignore). |
60fbe08
to
767eac2
Compare
I dont think this is difficult to implement. Just thought maybe we could get away with doing nothing by piggybacking the protocol-id. I've added a few features/things to rust-libp2p and I had some comments pushing back on new things saying that the protocol was getting too complex, so I guess now, i'm always looking for simplifications before adding more things. The complexity here is tiny (didn't mean to over-state it), seems like its necessary for what we're trying to do here. |
I think this is good. I still think we should stop adding version numbers in PR titles. It is confusing and versioning should not be done this way. Having said that, this would be good for 1.3, as it unlocks many other improvements. Finally, some nitpicking: |
Generally agree. I initially included it as a signal on how I think the various extensions should be prioritized. I've removed it to separate the versioning discussion.
What is the misunderstanding? I think the text is accurate. This is the same language as what is used in the HTTP/3 SETTINGS RFC (which I liberally copied from). That RFC states:
https://datatracker.ietf.org/doc/html/rfc9114#section-7.2.4-4 |
I see. You are right, it is hard to misunderstand (where the misunderstanding I wanted to avoid is to see this an an explicit ban of negotiating the use of extensions, which is clarified by the "however ... " part). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally, I think this is a good idea. It reduces frictions to propose new ideas or implement and deploy something not being agreed by all yet.
496cf55
to
e01116b
Compare
We have 2 implementations now: I think this is ready to merge. I'll merge this tomorrow unless there are objections. |
This lets us try more extensions and changes without requiring a new Gossipsub version. Hopefully it makes future extensions easier to iterate on, use, and deploy. More details in the doc
There is a lot of prior art here as many modern protocols have something like this, QUIC's transport parameters and H{3,2} SETTINGS frames to name a few.
Closes #687