-
Notifications
You must be signed in to change notification settings - Fork 42
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
feat: add 1MB restriction to LightPush and Relay #1351
Conversation
As next iteration I think we can split huge messages and send in chunks. Once chunks received we can figure out how to identify them and merge. |
size-limit report 📦
|
I would only apply the fix on relay and leave the responsibilty to the remote peer to decline messages over 1MB coming from light push. WDYT @alrevuelta ? That would allow the size limit for relay to be moved to the relay package as it's related to relay domain. Also I would not provide out of the box message splitting as I think different needs for large messages may require different strategies. |
So you mean that you would just enforce this limit on the receiver side and not sender? If so, I think this limit (and later on a lower one imho) should be enforced on both sides. So a message bigger than x is not published by the publisher and not gossiped by the subscriber. |
I am saying that we should only enforce this limit on the Waku Relay protocol and not the Waku Light Push protocol. When you say "Publisher", are you referring to a Relay publisher or Light Push requester? or both? |
I would enforce the 1mb limit in both, unless you have any strong reason for not doing it. I mean, if the network is going to reject it, why allowing >1mb in lightpush? |
@@ -49,6 +50,11 @@ class LightPush extends BaseProtocol implements ILightPush { | |||
const recipients: PeerId[] = []; | |||
|
|||
try { | |||
if (!isSizeValid(message.payload)) { | |||
log("Failed to send waku light push: message is bigger that 1MB"); | |||
return { recipients }; |
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.
Do we want to start return errors in this result?
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.
Yeah, good point. I'd still rather not throw, as hybrid let's put error codes there.
Problem
Users can abuse network and send huge messages.
Solution
Add same filter as in other implementations.
Notes