Skip to content
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

Allow per-topic message size limits #209

Open
whyrusleeping opened this issue Oct 13, 2019 · 2 comments
Open

Allow per-topic message size limits #209

whyrusleeping opened this issue Oct 13, 2019 · 2 comments

Comments

@whyrusleeping
Copy link
Contributor

I'd like to be able to configure a maximum message size per topic. For example, filecoin transactions should never be larger than 32k, but block messages might be larger than that.

I don't want to have to read the entire object from the other peer into memory before i drop it, I should be able to detect the message is too big, and then either kill the connection, or discard the bytes as they come in.

@aschmahmann
Copy link
Contributor

Seems reasonable. While not necessary it seems like we should get #189 resolved first though.

@aschmahmann
Copy link
Contributor

An issue that @aarshkshah1992 brought up is that the PubSub protocol currently only has one stream per peer. This means that if topic A has a 1MB max and topic B has a 1KB max that the stream potentially has to read 1MB for every RPC that comes in, since it might be a topic A message.

There are a number of possible solutions, but here are a few:

  1. Have one stream per topic instead of peer
  2. Modify the PubSub wire protocol so that the topic arrives first and can be deserialized before the rest of the message
  3. Have the limits be configurable on a PubSub level instead of a topic level
  4. Have the limit be the max of the topic sizes
  5. Read the oversized message (e.g. topic size is 1k, but PubSub size is 1M and we read 1M) then if it's bigger than the topic size kill the connection and blacklist the peer

Since options 1 and 2 require protocol changes doing a combination of options 3 and 5 seems reasonable, although we may want to consider taking 1 and 2 into account for future PubSub protocol iterations.

@whyrusleeping @raulk what do you think?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants