-
Notifications
You must be signed in to change notification settings - Fork 56
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
11/WAKU2-RELAY: Large files not accepted #739
Comments
This is coming from here: https://github.com/status-im/nim-libp2p/blob/1681197d67e695e6adccccf56ce0e2d586317d67/libp2p/protocols/pubsub/pubsubpeer.nim#L122 I've opened a PR in libp2p to allow programs to tweak this number vacp2p/nim-libp2p#634 Note that this is not a good long term solution. With |
It's worth noting also that other gossipsub implementations will have similar limits, meaning that even if one node supports a higher value, it's not a given that the other does - one way of handling these kinds of issues is to gossip a URI of some kind that the end recipient can retrieve from elsewhere |
Agreed with all comments above. At this stage I think we should aim for feature parity with Waku v1.
So if we could get the limit raised to 600kb I understand it should be enough. Then, yes, ultimately images (and videos and other files) should not be transferred of the Waku network. |
this will be difficult to change, down the line, because old clients will work according to one rule while newer clients will want to have a lower limit - lowering the limit is a backwards-incompatible change. I'd look for creative solutions to this problem - for example chunking or other techniques - chunking doesn't make the bandwidth problem go away but it might have a more attractive compatibility profile. |
for waku, I suspect this can be lowered to 4 which is the "standard" libp2p mesh connectivity level. |
Agree that chunking would be a better approach that is worth exploring here. Should be doable at application layer without changing Waku relay / gossipsub limits. |
@oskarth so should we merge in libp2p? |
FYI, we are looking at the chunking approach at an application level which would mean modifying the Thoughts? |
@Menduist I'd say we hold off and explore chunking first. If there is a good way of doing it that works for Status protocol that'd be best. Agree it'd be better not to have it in, especially since it is a decision that is hard to reverse. The problem is really two: (1) chunking images/large messages (2) dealing with bridging. For the bridging, we might have to get creative here, for example by chunking up large messages w/o understanding content, then assembling them on client. |
next up: progressive jpegs - history repeats itself :) |
Uppon better reading of the spec (thanks @cskiraly), we've noticed that we should allow applications to tune the size |
So the 64kb is an arbitrary size? Or does it originate from another libp2p implementation? To go around this problem, @Szymx95 (from EthWorks, working on DappConnect Chat SDK) is going to implement a PoC in js-waku that would do the following: Define new fields on Waku v2 syntax = "proto3";
message WakuMessage {
bytes payload = 1;
string contentTopic = 2;
uint32 version = 3;
double timestamp = 4;
uint32 chunkSeq = 10; // sequence number of this chunk, only set if message is a chunk
uint32 totalChunk = 11; // total number of chunks, only set if message is a chunk
bytes payloadHash = 12; // SHA-256 has of the total payload, before chunkin
} Note: it was also proposed to have these new field in the Other alternative would be to add the fields in the For Waku v2 nodeOutboundIf an outbound Waku Message
For each chunk, send a new Waku Message with
Send all chunk messages. InboundIf an inbound Waku Message has all the new fields set then:
For Waku v1<>v2 bridgeFor messages received on v1 side over 50kb, do as per For chunk messages received on v2 side, do as per Let me know your thoughts, we can discussion in more details once the PoC is ready. |
AFAICT, yes. However, upon discussing with the nimbus team, they also need a 1mb default, and since this seems to be the norm around libp2p implementations, we'll also switch to a 1mb default |
Interesting, so I guess the problem took care of itself? :D
That seems to imply:
|
Merged in libp2p
All our points still holds, you may encounter a libp2p with smaller default (since the default is "hinted" in the spec, instead of enforced), and this is still bad in terms of bandwidth But that should hold for now :) If you have the possibility, add per-topic size validation, to at least stop relaying big messages where it does not make sense |
Agreed and thanks for the clarification. We will halt bridge compatible chunking on our side. Yes medial file chunking could be done in the future at application level. Cc @Szymx95 |
@jm-clius Does nim-waku v0.6 uses the nim-libp2p version with a limit of 1mb? |
Yup! We now proudly support larger message sizes |
The Status app currently sends images over Waku v1.
To attempt the same over Waku v2 as part of the DappConnect Chat SDK, we have tried sending messages of ~100kb. However, it fails.
The issue seem related to this error message:
Please find full logs below for a 500kb payload size:
Here are partial log for 100kb payload size (same error):
The text was updated successfully, but these errors were encountered: