Remove support for multi-topic messages #388
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
raulk
approved these changes
Oct 8, 2020
| @@ -0,0 +1,12 @@ | |||
| syntax = "proto2"; | |||
|
|
|||
| package compat.pb; | |||
There was a problem hiding this comment.
Nice! This is a great technique we should keep using across the board on protobuf changes ;-)
| if err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| if newMessage.GetTopic() != topic2 { |
There was a problem hiding this comment.
Cool, this verifies that indeed the last topic in the list survives.
| From: []byte(attacker.ID()), | ||
| Seqno: []byte{byte(i + 1)}, | ||
| Data: []byte("some data" + strconv.Itoa(i)), | ||
| Topic: &mytopic, |
There was a problem hiding this comment.
I think we should remove the optional trait from the protobuf file for the topic. It is compulsory (AFAIK) and it would remove this annoying indirection, and speed things up slightly.
There was a problem hiding this comment.
Agreed. But it's a bit of work, so let's do it in a future pr.
| @@ -18,7 +18,7 @@ message Message { | |||
| optional bytes from = 1; | |||
| optional bytes data = 2; | |||
| optional bytes seqno = 3; | |||
| repeated string topicIDs = 4; | |||
| optional string topic = 4; | |||
There was a problem hiding this comment.
Don't need to do here, but we should drop the optional here.
| // check against the mesh delivery window -- if the validated time is passed as 0, then | ||
| // the message was received before we finished validation and thus falls within the mesh | ||
| // delivery window. | ||
| if !validated.IsZero() && time.Since(validated) > tparams.MeshMessageDeliveriesWindow { |
| @@ -300,471 +293,3 @@ func TestValidateAssortedOptions(t *testing.T) { | |||
| } | |||
| } | |||
| } | |||
|
|
|||
| func TestValidateMultitopic(t *testing.T) { | |||
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This was a fringe / half-baked feature in the wire protocol that has no direct users or API exposure.
Not only we didn't provide an API to do multi-topic publishing, but also the recent introduction of the topic handle abstraction created impedance mismatch.
So this feature adds bloat with no visible gain, and this PR proposes simplification by removal.
Migration path
There is no visible consequence for go-libp2p-pubsub users. At the wire level, due to the way that Protobuf serialises repeated fields, this causes no breakage. If direct wire protocol users (e.g. other libp2p implementations) were using the variadic topics field, this patch will make Protobuf pick the last topic in the list.