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

chore: delivery monitor for store v3 reliability protocol #2977

Merged
merged 9 commits into from
Aug 27, 2024

Conversation

Ivansete-status
Copy link
Collaborator

@Ivansete-status Ivansete-status commented Aug 15, 2024

Description

This PR reinforces the efforts in the proper messages delivery, considering both sending and receiving.
Changes that try to replicate similar changes that are currently implemented in status-go/go-waku.

Notice that I haven't tested it yet. The idea is to start testing, and adjusting its implementation, once we integrate nwaku in status-go.

Changes

  • Start suggesting a persisting mechanism so that we can persist not-sent messages locally. Notice that this is not fully implemented yet as I consider that might be the app's responsibility.
  • New parameter to enable/disable reliability. Disabled by default.
  • New delivery monitor module.
  • Callback approach so that we can give feedback to the app regarding the messages' delivery.
  • Minor cleanup in store protocols (this is a bit aside from the original PR's purpose.)

Issue

closes #2819

Better structure of delivery monitor and related modules

store protocol: simple cleanup of unused const

use of observer observable pattern to inform delivery_monitor about subscription state

waku_store resume does not need to get an explicit peerId as query param

waku_sync protocol needs some peerId as peerId is optional in query proc

send_monitor becomes a publish observer of lightpush and relay

deliver monitor add more protection against possible crash and better logs
Copy link

github-actions bot commented Aug 15, 2024

You can find the image built from this PR at

quay.io/wakuorg/nwaku-pr:2977

Built from 381b15c

Copy link
Contributor

@gabrielmer gabrielmer left a comment

Choose a reason for hiding this comment

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

Wooow, beautiful PR! 😍 😍

Added some small questions, but looks great! Looking forward to see it working in practice!

waku/waku_store/client.nim Show resolved Hide resolved
waku/waku_relay/protocol.nim Outdated Show resolved Hide resolved
waku/node/delivery_monitor/send_monitor.nim Outdated Show resolved Hide resolved
waku/node/delivery_monitor/send_monitor.nim Show resolved Hide resolved
waku/node/delivery_monitor/recv_monitor.nim Outdated Show resolved Hide resolved
Ivansete-status and others added 4 commits August 23, 2024 17:03
Co-authored-by: gabrielmer <101006718+gabrielmer@users.noreply.github.com>
Copy link
Contributor

@SionoiS SionoiS left a comment

Choose a reason for hiding this comment

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

LGTM Nice work!

If only Sync was better implemented it could have been useful for this feature. Without proper storage though it's difficult to use.

As for re-sending messages can it still work with RLN? If verifying that a msg is missing take too much time the epoch would change and the proof would end up invalid no? That would also happen sometimes at the epoch boundary even if verifying is fast.

@Ivansete-status
Copy link
Collaborator Author

As for re-sending messages can it still work with RLN? If verifying that a msg is missing take too much time the epoch would change and the proof would end up invalid no? That would also happen sometimes at the epoch boundary even if verifying is fast.

ah yes, good point! This is something we will need to fine-tune in the future when RLN is applied

@Ivansete-status Ivansete-status merged commit 0f68274 into master Aug 27, 2024
9 of 11 checks passed
@Ivansete-status Ivansete-status deleted the feature/store-v3-reliability-protocol branch August 27, 2024 14:49
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.

feat: enhance reliability thanks to StoreV3
3 participants