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

test: light-push/index.node.spec.ts: adjust metadata size to nwaku max size adjustment #1769

Merged
merged 1 commit into from
Dec 27, 2023

Conversation

Ivansete-status
Copy link
Contributor

Problem

The max message size reduction in nim-waku ( see this ) made the "Fails to push message with large meta" test fail because the nwaku didn't accept such a big message and therefore its response is different than the expected by the test.

Solution

In this PR, we are reducing the msg size sent in the "Fails to push message with large meta" test by the same amount of bytes that the max msg size is being reduced in nim-waku.

More details:
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

Notes

@Ivansete-status Ivansete-status requested a review from a team as a code owner December 22, 2023 10:03
… 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
@Ivansete-status Ivansete-status force-pushed the adjust-test-to-max-msg-size branch from abb33f1 to b7e78ca Compare December 22, 2023 10:04
Copy link

github-actions bot commented Dec 22, 2023

size-limit report 📦

Path Size Loading time (3g) Running time (snapdragon) Total time
Waku core 81.96 KB (0%) 1.7 s (0%) 179 ms (+2.79% 🔺) 1.9 s
Waku Simple Light Node 261.28 KB (0%) 5.3 s (0%) 619 ms (+29.28% 🔺) 5.9 s
ECIES encryption 31.94 KB (0%) 639 ms (0%) 219 ms (+94.08% 🔺) 858 ms
Symmetric encryption 31.93 KB (0%) 639 ms (0%) 215 ms (+1.06% 🔺) 854 ms
DNS discovery 123.17 KB (0%) 2.5 s (0%) 337 ms (+35.97% 🔺) 2.8 s
Privacy preserving protocols 119.3 KB (0%) 2.4 s (0%) 413 ms (-18.25% 🔽) 2.8 s
Light protocols 79.42 KB (0%) 1.6 s (0%) 227 ms (+14.99% 🔺) 1.9 s
History retrieval protocols 78.31 KB (0%) 1.6 s (0%) 257 ms (+22.11% 🔺) 1.9 s
Deterministic Message Hashing 5.92 KB (0%) 119 ms (0%) 37 ms (-23.96% 🔽) 156 ms

@adklempner
Copy link
Member

@Ivansete-status does it make sense to wait until the nwaku PR is merged and a new version is released? Then we can update the Docker image we use to run these tests in this PR. As of now, the test still passes without this change.

@Ivansete-status
Copy link
Contributor Author

@Ivansete-status does it make sense to wait until the nwaku PR is merged and a new version is released? Then we can update the Docker image we use to run these tests in this PR. As of now, the test still passes without this change.

Hey @adklempner !
I'm interested in making it the other way round: merge this js-waku PR so that the CI can pass properly on waku-org/nwaku#2298
Cheers

@adklempner adklempner merged commit 0d534e3 into master Dec 27, 2023
10 of 11 checks passed
@adklempner adklempner deleted the adjust-test-to-max-msg-size branch December 27, 2023 17:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants