chore(waku): Enable message missing verification flag #21005
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Jenkins BuildsClick to see older builds (13)
|
qfrank
approved these changes
Aug 12, 2024
There was a problem hiding this comment.
I saw the comments:
https://github.com/status-im/status-go/pull/5570/files#r1690794119
https://github.com/status-im/status-go/pull/5570/files#r1692345230
I'm wondering if we should enable WakuV2EnableStoreConfirmationForMessagesSent by default now? because it's disabled by default and also disabled for mobile V1 User @ilmotta
@qfrank, I'm not sure what's best, but we have a PR from @cammellos #20715 (comment) that was merged last month and from the explanation it looks like enabling |
ilmotta
force-pushed
the
ilmotta-20879/enable-waku-message-verification-flag
branch
2 times, most recently
from
48124b4 to
7f4d40d
Compare
10% of end-end tests have passedFailed tests (46)Click to expandClass TestWalletMultipleDevice:
Class TestCommunityMultipleDeviceMerged:
Class TestActivityMultipleDevicePRTwo:
Class TestCommunityOneDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestActivityMultipleDevicePR:
Class TestDeepLinksOneDevice:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestActivityCenterContactRequestMultipleDevicePR:
Passed tests (5)Click to expandClass TestWalletOneDevice:
Class TestCommunityOneDeviceMerged:
Class TestDeepLinksOneDevice:
|
|
@churik I think we better postpone this PR for 2.30.1. cc @richard-ramos |
ilmotta
force-pushed
the
ilmotta-20879/enable-waku-message-verification-flag
branch
from
7f4d40d to
b0852f5
Compare
43% of end-end tests have passedFailed tests (4)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestWalletMultipleDevice:
Class TestWalletOneDevice:
Passed tests (3)Click to expandClass TestCommunityMultipleDeviceMerged:
Class TestCommunityOneDeviceMerged:
|
|
3 tests are failed because of SauceLabs (they are really unstable lately), 1 because of #21151 |
0% of end-end tests have passedFailed tests (51)Click to expandClass TestCommunityMultipleDeviceMergedTwo:
Class TestDeepLinksOneDevice:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestWalletMultipleDevice:
Class TestWalletOneDevice:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityOneDeviceMerged:
Class TestCommunityMultipleDeviceMerged:
Class TestActivityMultipleDevicePRTwo:
Class TestActivityMultipleDevicePR:
Class TestActivityCenterContactRequestMultipleDevicePR:
|
ilmotta
force-pushed
the
ilmotta-20879/enable-waku-message-verification-flag
branch
from
b0852f5 to
7541d5a
Compare
|
@ilmotta it is not compatible with current desktop - so when mobile device send contact request, the desktop user doesn't recieve it. CR was sent on 16:45, logs of sender (mobile): logs (4).zip All is working with nightly11/09 (CR arrives immidiately on desktop) |
@churik, I tried now with the public Desktop 2.30 beta and with this PR's build and the CR sent from mobile arrived immediately on desktop and the CR acceptance confirmation arrived 1min later on mobile. Then I tested with a new profile and the CR sending & confirmation arrived instantly. Other chat messages also are working fine. With these test cases, I think this is enough for me to think the PR is working as expected when waku is friendly and that the PR is compatible with desktop without any further changes on their end. When appropriate, could you re-check this PR? |
88% of end-end tests have passedFailed tests (6)Click to expandClass TestWalletMultipleDevice:
Class TestActivityMultipleDevicePRTwo:
Class TestCommunityOneDeviceMerged:
Passed tests (45)Click to expandClass TestActivityMultipleDevicePR:
Class TestWalletOneDevice:
Class TestCommunityOneDeviceMerged:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityMultipleDeviceMerged:
Class TestDeepLinksOneDevice:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestActivityCenterContactRequestMultipleDevicePR:
|
|
Posting the results here:
The bottom line:
I leave desicion on to merge it or not to you, as frankly I don't see a lot of improvements and I'm not sure what from the above requires more investigation in particular case. |
|
Most excellent analysis @churik 👏🏼 I don't want to merge if there's no measurable benefit, but maybe there's a theoretical benefit that's hard to reproduce. @richard-ramos, based on the results above #21005 (comment), what's your recommendation in terms of enabling the flag |
|
IMO should be merged. Desktop has been using it for a while and has not reported issues with it either, and the benefits while not immediatly tangible, should still lead to less missing messages as it will constantly attempt to missing messages from storenodes. |
Thank you @richard-ramos, I didn't know Desktop was using this flag for a while. With this added evidence, I'll rebase and merge @churik |
ilmotta
force-pushed
the
ilmotta-20879/enable-waku-message-verification-flag
branch
from
7541d5a to
a520320
Compare
ilmotta
deleted the
ilmotta-20879/enable-waku-message-verification-flag
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
As recommended here and here, we set the Waku config flag
WakuV2EnableMissingMessageVerificationto true. The flag can't be toggled in the UI because there's no supporting endpoint in status-go at the moment.Steps to test
In order for the flag to be used you have to create a new Profile. If you login with an existing one, it will be the same as if the flag was disabled (as in
develop).It would be good to get this PR in 2.30 as it can improve the UX, but only if we deem it safe.
Areas worth testing that may be impacted:
status: ready