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

feat: relay msg size #963

Merged
merged 10 commits into from
Jan 3, 2024
Merged

feat: relay msg size #963

merged 10 commits into from
Jan 3, 2024

Conversation

chaitanyaprem
Copy link
Collaborator

@chaitanyaprem chaitanyaprem commented Dec 15, 2023

Description

Make relay maxMsgSize configurable and set default as per The waku network rfc https://rfc.vac.dev/spec/64/#message-size.

Follows waku-org/nwaku#2298

@status-im-auto
Copy link

status-im-auto commented Dec 15, 2023

Jenkins Builds

Click to see older builds (8)
Commit #️⃣ Finished (UTC) Duration Platform Result
✔️ 6280288 #1 2023-12-15 10:16:46 ~1 min nix-flake 📄log
✔️ b03a828 #2 2023-12-25 07:04:12 ~2 min nix-flake 📄log
✔️ c91d802 #3 2023-12-25 10:44:07 ~1 min nix-flake 📄log
✔️ 2720895 #4 2023-12-29 09:00:12 ~2 min nix-flake 📄log
✔️ 5186b98 #5 2023-12-29 10:27:34 ~1 min nix-flake 📄log
✔️ 23ad0f0 #6 2024-01-02 09:41:58 ~1 min nix-flake 📄log
✔️ 0da452d #7 2024-01-03 01:08:56 ~1 min nix-flake 📄log
✔️ 84d91e5 #8 2024-01-03 01:22:01 ~2 min nix-flake 📄log
Commit #️⃣ Finished (UTC) Duration Platform Result
✔️ ddddd03 #9 2024-01-03 01:24:06 ~1 min nix-flake 📄log
✔️ 91986df #10 2024-01-03 01:26:04 ~1 min nix-flake 📄log

Copy link
Member

@richard-ramos richard-ramos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should get this setting down to

if len(out) > pubsub.DefaultMaxMessageSize {

@richard-ramos
Copy link
Member

richard-ramos commented Dec 15, 2023

I just saw the PR in waku-org/nwaku#2298 , but there bytes instead of kilobytes are being used.

@Ivansete-status @chaitanyaprem , do discuss which unit to use :D i'm okay with any tbh, but let's make nwaku and go-waku use the same!

EDIT: ah! also the flag is different: max-num-bytes-msg-size

@Ivansete-status
Copy link

I just saw the PR in waku-org/nwaku#2298 , but there bytes instead of kilobytes are being used.

@Ivansete-status @chaitanyaprem , do discuss which unit to use :D i'm okay with any tbh, but let's make nwaku and go-waku use the same!

EDIT: ah! also the flag is different: max-num-bytes-msg-size

Good point!
imo, the best is to use "number of bytes" to avoid any possible confusion between different units (KB, KiB, MB, MiB, etc.)

@Ivansete-status
Copy link

Considering this comment and a DM chat I had with @richard-ramos , we will only accept the following units:
KiB, KB, and B because it doesn't make sense to consider messages bigger than 1MiB. Therefore, in order to set the limit in status fleets, we will need to set it as 1024KiB.

( cc @fryorcraken )

@chaitanyaprem
Copy link
Collaborator Author

chaitanyaprem commented Dec 25, 2023

I just saw the PR in waku-org/nwaku#2298 , but there bytes instead of kilobytes are being used.
@Ivansete-status @chaitanyaprem , do discuss which unit to use :D i'm okay with any tbh, but let's make nwaku and go-waku use the same!
EDIT: ah! also the flag is different: max-num-bytes-msg-size

Good point! imo, the best is to use "number of bytes" to avoid any possible confusion between different units (KB, KiB, MB, MiB, etc.)

I too agree with this. This would avoid chance of misconfiguring as there are many ways to configure.
But looks like now we support many config types, can we make this simpler by just supporting just KB config?

@Ivansete-status
Copy link

I just saw the PR in waku-org/nwaku#2298 , but there bytes instead of kilobytes are being used.
@Ivansete-status @chaitanyaprem , do discuss which unit to use :D i'm okay with any tbh, but let's make nwaku and go-waku use the same!
EDIT: ah! also the flag is different: max-num-bytes-msg-size

Good point! imo, the best is to use "number of bytes" to avoid any possible confusion between different units (KB, KiB, MB, MiB, etc.)

I too agree with this. This would avoid chance of misconfiguring as there are many ways to configure. But looks like now we support many config types, can we make this simpler by just supporting just KB config?

In nwaku we aim to support KiB, KB, and B. The logic isn't too complex:

check waku/common/utils/parse_size_units.nim from waku-org/nwaku#2298

@chaitanyaprem
Copy link
Collaborator Author

I just saw the PR in waku-org/nwaku#2298 , but there bytes instead of kilobytes are being used.
@Ivansete-status @chaitanyaprem , do discuss which unit to use :D i'm okay with any tbh, but let's make nwaku and go-waku use the same!
EDIT: ah! also the flag is different: max-num-bytes-msg-size

Good point! imo, the best is to use "number of bytes" to avoid any possible confusion between different units (KB, KiB, MB, MiB, etc.)

I too agree with this. This would avoid chance of misconfiguring as there are many ways to configure. But looks like now we support many config types, can we make this simpler by just supporting just KB config?

In nwaku we aim to support KiB, KB, and B. The logic isn't too complex:

check waku/common/utils/parse_size_units.nim from waku-org/nwaku#2298

Alright, found a util in golang that supports all kinds of options like Kib, KB, Mib, MB, B etc.
Don't need to write all parsing logic :D
https://github.com/dustin/go-humanize/tree/v1.0.1

@chaitanyaprem
Copy link
Collaborator Author

EDIT: ah! also the flag is different: max-num-bytes-msg-size

@Ivansete-status , maybe we should rename the flag to max-msg-size since we support various sizes now.

@chaitanyaprem
Copy link
Collaborator Author

chaitanyaprem commented Dec 29, 2023

@richard-ramos do note that when this version of go-waku is updated in status-go the argument node.WithMaxMsgSize has to be passed with 1MiB

CC @kaichaosun @plopezlpz

@Ivansete-status
Copy link

EDIT: ah! also the flag is different: max-num-bytes-msg-size

@Ivansete-status , maybe we should rename the flag to max-msg-size since we support various sizes now.

Good point! I'll rename it accordingly in nwaku .)

@Ivansete-status
Copy link

@richard-ramos do note that when this version of go-waku is updated in status-go the argument node.WithMaxMsgSize has to be passed with 1MB

CC @kaichaosun @plopezlpz

@chaitanyaprem - Maybe I'm wrong but, should it be 1MiB instead?

@chaitanyaprem
Copy link
Collaborator Author

@richard-ramos do note that when this version of go-waku is updated in status-go the argument node.WithMaxMsgSize has to be passed with 1MB
CC @kaichaosun @plopezlpz

@chaitanyaprem - Maybe I'm wrong but, should it be 1MiB instead?

Thanks for highlighting it, i have updated the comment now

Copy link
Contributor

@kaichaosun kaichaosun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@chaitanyaprem chaitanyaprem merged commit b5068b4 into master Jan 3, 2024
@chaitanyaprem chaitanyaprem deleted the feat/relay-msg-size branch January 3, 2024 01:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants