Defining pubsub's max message size as a global const

[gabrielmer]

Moving the maximum message size value as a global constant so the value can be accessed and reused across the codebase

[codecov[bot]]

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (9bc5ec1) 83.13% compared to head (73cac43) 83.18%. Report is 1 commits behind head on unstable.

Additional details and impacted files

[](https://app.codecov.io/gh/status-im/nim-libp2p/pull/1019?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=status-im) ```diff @@ Coverage Diff @@ ## unstable #1019 +/- ## ============================================ + Coverage 83.13% 83.18% +0.05% ============================================ Files 91 91 Lines 15344 15349 +5 ============================================ + Hits 12756 12768 +12 + Misses 2588 2581 -7 ``` | [Files](https://app.codecov.io/gh/status-im/nim-libp2p/pull/1019?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=status-im) | Coverage Δ | | |---|---|---| | [libp2p/protocols/pubsub/pubsub.nim](https://app.codecov.io/gh/status-im/nim-libp2p/pull/1019?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=status-im#diff-bGlicDJwL3Byb3RvY29scy9wdWJzdWIvcHVic3ViLm5pbQ==) | `83.90% <ø> (ø)` | | ... and [4 files with indirect coverage changes](https://app.codecov.io/gh/status-im/nim-libp2p/pull/1019/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=status-im)

[diegomrsantos]

Could you please elaborate on "reused across the codebase"?

[gabrielmer]

Could you please elaborate on "reused across the codebase"?

Yes, in my specific case I want to check before sending a message if it's greater than the allowed limit. The idea is to avoid hardcoding 1024*1024 and referring to a global const, so if that value changes we don't introduce a bug.

[diegomrsantos]

This seems to be a parameter and 1024*1024 is the default value. Can you pass the value you want to use instead (that could be 1024*1024 too)?

[diegomrsantos]

@Ivansete-status thank you for elaborating on it, but I still don't understand why Waku needs to use Gossipsub default max message size in its codebase. Are you happy using whatever value it happens to have? Wouldn't it be better if Waku chose a value (which could be the same as the default) aligned with its needs and use it as the parameter value when creating the Gossipsub instance?

[diegomrsantos]

But I don't have a strong opinion on that, feel free to merge it.

[diegomrsantos]

Actually, we also have a maxMessageSize in pubsubpeer.nim file for the PubSubPeer type. I don't know the reason for the duplication, it might be important to analyze it before merging this PR.

[arnetheduck]

er, given this is a default for a runtime value, it doesn't make much sense checking against the constant - nwaku should be checking against the runtime value at the time of instantiation - if nwaku wants to use a constant, it should use its own constant both when instantiating libp2p and nwaku.

[arnetheduck]

more broadly, the thing nwaku needs to be wary of is if different nwaku nodes have different max message size values - for example when upgrading from one version to another - such a change generally must see a coordinated release / update

[gabrielmer]

er, given this is a default for a runtime value, it doesn't make much sense checking against the constant - nwaku should be checking against the runtime value at the time of instantiation - if nwaku wants to use a constant, it should use its own constant both when instantiating libp2p and nwaku.

Makes sense! Not sure why in nwaku we use the default value of nim-libp2p and not a value set by us in the first place. Is it by the assumption that the default value should be a reasonable and well-tested one? @Ivansete-status @jm-clius

[jm-clius]

Not sure why in nwaku we use the default value of nim-libp2p

We don't afaict. Where default values in nim-libp2p is considered reasonable we don't necessarily make this configurable or an explicit constant in Waku, but in the case of max message size it is (and should be) a Waku constant:

Constant here - https://github.com/waku-org/nwaku/blob/master/waku/waku_core/message/default_values.nim#L8 Used to initialise libp2p pubsub here - https://github.com/waku-org/nwaku/blob/e4e147bcbb71366b159d0f2dcfddfe85fa85b1af/waku/waku_relay/protocol.nim#L168

[gabrielmer]

We don't afaict. Where default values in nim-libp2p is considered reasonable we don't necessarily make this configurable or an explicit constant in Waku, but in the case of max message size it is (and should be) a Waku constant:

Constant here - https://github.com/waku-org/nwaku/blob/master/waku/waku_core/message/default_values.nim#L8 Used to initialise libp2p pubsub here - https://github.com/waku-org/nwaku/blob/e4e147bcbb71366b159d0f2dcfddfe85fa85b1af/waku/waku_relay/protocol.nim#L168

Ooh that's great! So it was a big misunderstanding then. For some reason we thought we used 2 different max values, libp2p's and Waku's. Closing this PR.

Thanks so much and apologies :))