-
Notifications
You must be signed in to change notification settings - Fork 57
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
chore: message.nim - set max message size to 150KiB according to spec #2298
Conversation
You can find the image built from this PR at
Built from b966524 |
This needs to be configurable so that the Status fleet can retain the 1MB message size for now (note they already have plans to reduce their message size). |
|
This PR may contain changes to configuration options of one of the apps. If you are introducing a breaking change (i.e. the set of options in latest release would no longer be applicable) make sure the original option is preserved with a deprecation note for 2 following releases before it is actually removed. Please also make sure the label |
Hey @fryorcraken ! I've added the configurable parameter
|
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.
LGTM, thanks so much!
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.
lgtm. Note that some non TWN deployments would need to be updated.
Thanks for that. How do we ensure that we don't forget to change the config when upgrading Status fleet? nitpick: could it accept |
Good point! We've created this issue to tackle that: #2305
I'll check it asap 👌 |
Using KiB instead of KB because that seems more aligned with the actual default defined in nim-libp2p (1024 * 1024) Spec details: https://rfc.vac.dev/spec/64/#message-size
8498a56
to
79ae7ba
Compare
… adjustment The "Fails to push message with large meta" test used 10 ** 6 when `nwaku` node had MaxWakuMessageSize == 1MiB ( 1*2^20 .) `nwaku` establishes the max lightpush msg size as `const MaxRpcSize* = MaxWakuMessageSize + 64 * 1024` see: https://github.com/waku-org/nwaku/blob/07beea02095035f4f4c234ec2dec1f365e6955b8/waku/waku_lightpush/rpc_codec.nim#L15 In the PR waku-org/nwaku#2298 we reduced the MaxWakuMessageSize from 1MiB to 150KiB. Therefore, the 105024 number comes from substracting ( 1*2^20 - 150*2^10 ) to the original 10^6 that this test had when MaxWakuMessageSize == 1*2^20
… adjustment The "Fails to push message with large meta" test used 10 ** 6 when `nwaku` node had MaxWakuMessageSize == 1MiB ( 1*2^20 .) `nwaku` establishes the max lightpush msg size as `const MaxRpcSize* = MaxWakuMessageSize + 64 * 1024` see: https://github.com/waku-org/nwaku/blob/07beea02095035f4f4c234ec2dec1f365e6955b8/waku/waku_lightpush/rpc_codec.nim#L15 In the PR waku-org/nwaku#2298 we reduced the MaxWakuMessageSize from 1MiB to 150KiB. Therefore, the 105024 number comes from substracting ( 1*2^20 - 150*2^10 ) to the original 10^6 that this test had when MaxWakuMessageSize == 1*2^20
… adjustment The "Fails to push message with large meta" test used 10 ** 6 when `nwaku` node had MaxWakuMessageSize == 1MiB ( 1*2^20 .) `nwaku` establishes the max lightpush msg size as `const MaxRpcSize* = MaxWakuMessageSize + 64 * 1024` see: https://github.com/waku-org/nwaku/blob/07beea02095035f4f4c234ec2dec1f365e6955b8/waku/waku_lightpush/rpc_codec.nim#L15 In the PR waku-org/nwaku#2298 we reduced the MaxWakuMessageSize from 1MiB to 150KiB. Therefore, the 105024 number comes from substracting ( 1*2^20 - 150*2^10 ) to the original 10^6 that this test had when MaxWakuMessageSize == 1*2^20
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.
LGTM!
msg6 = fakeWakuMessage(contentTopic=contentTopic, payload=getByteSequence(3*1024*1024 + 1023*1024 + 970)) # 4MiB - 54B -> Out of Max Size | ||
msg4 = fakeWakuMessage(contentTopic=contentTopic, payload=getByteSequence(MaxRpcSize - 1024)) # Max Size (Inclusive Limit) | ||
msg5 = fakeWakuMessage(contentTopic=contentTopic, payload=getByteSequence(MaxRpcSize)) # Max Size (Exclusive Limit) | ||
msg6 = fakeWakuMessage(contentTopic=contentTopic, payload=getByteSequence(MaxRpcSize)) # Out of Max Size |
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.
This one is duplicated
## https://rfc.vac.dev/spec/64/#message-size | ||
MaxWakuMessageSize* = 150 * 1024 # Remember that 1 MiB is the PubSub default | ||
|
||
DefaultMaxWakuMessageSizeStr* = "150KiB" |
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.
I wouldn't have two sources of truth (MaxWakuMessageSize
and DefaultMaxWakuMessageSizeStr
). How about we use the str
one as source of truth, and define the other in terms of it by calling parseMsgSize(DefaultMaxWakuMessageSizeStr)
?
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.
LGTM apart two little comment you may consider. Thank you.
## Parses size strings such as "1.2 KiB" or "3Kb" and returns the equivalent number of bytes | ||
## if the parse task goes well. If not, it returns an error describing the problem. | ||
|
||
const RegexDef = """\s*(\d+([\,\.]\d*)?)\s*([Kk]{0,1}[i]?[Bb]{1})""" |
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.
I'm not pretty sure if it would not be suitable to apply a more restrictive pattern?
Like is someone mistakenly use it "1o24 KiB" it still accepts but results in 241024. Or "l50kb" is translated to 501000. I would clearly drop trailing white space or put it under a group that should be empty at the end.
Also I may change the middle whitespace to accept zero or only 1 of it.
|
||
const | ||
## https://rfc.vac.dev/spec/64/#message-size | ||
DefaultMaxWakuMessageSizeStr* = "150KiB" # Remember that 1 MiB is the PubSub default |
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.
DefaultMaxWakuMessageSizeStr* = "150KiB" # Remember that 1 MiB is the PubSub default | |
DefaultMaxWakuMessageSizeStr* = "150KiB" # Remember that 1024 KiB is the PubSub default |
As MiB is not supported we might not refer it anywhere.
We have some test with 1MB messages. Pretty sure @fbarbu15 or the @waku-org/js-waku-developers is already on it. |
In
Indeed as @fryorcraken suggested, some 1MB tests are failing with |
Description
Change to align the actual message limit with the one written in the spec: https://rfc.vac.dev/spec/64/#message-size
Notice that the spect states 150KB but we are setting the maximum to 150KiB because that sounds more aligned with the actual default value defined within nim-libp2p (1024 * 1024)
Issue
closes #2297