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: message transfer mechanism #2688

Merged
merged 6 commits into from
May 14, 2024
Merged

feat: message transfer mechanism #2688

merged 6 commits into from
May 14, 2024

Conversation

SionoiS
Copy link
Contributor

@SionoiS SionoiS commented May 9, 2024

Add a callback to query the store of the other node for the missing messages and then add them to our own.

Copy link

github-actions bot commented May 9, 2024

You can find the image built from this PR at

quay.io/wakuorg/nwaku-pr:2688-rln-v1

Built from 6dedc86

Copy link

github-actions bot commented May 9, 2024

You can find the image built from this PR at

quay.io/wakuorg/nwaku-pr:2688-rln-v2

Built from 6dedc86

@SionoiS SionoiS requested a review from jm-clius May 9, 2024 14:43
@SionoiS SionoiS marked this pull request as ready for review May 9, 2024 14:43
Copy link
Contributor

@jm-clius jm-clius left a comment

Choose a reason for hiding this comment

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

LGTM in terms of functionality, though I would urge abstracting the coordination logic away from the node module.

@@ -243,8 +243,49 @@ proc mountWakuSync*(node: WakuNode): Result[void, string] =

return ok((elements, cursor))

#TODO add sync callback and options
node.wakuSync = WakuSync.new(peerManager = node.peerManager, pruneCB = some(prune))
let transfer: TransferCallback = proc(
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not a huge fan of us extending the waku_node module with (more) protocol integration logic. To me it seems like we're mounting a new type of protocol - mountWakuStoreSync() if you will. WakuStoreSync contains a reference to the local wakuStoreClient, wakuArchive and wakuSync and handles the coordination logic between these layers. I imagine this coordination will even be extended in future (e.g. with configuration checks to ensure the archive and sync stores are reasonably dimensioned). WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't like it either but a "component" (like WakuStoreSync) referencing other components is a BIG no-no in my book and architecturally we don't have hierarchies of components in nwaku and that's a good thing!

I think we have to tolerate a waku_node like this until we have proper channels implemented in chronos then we can move away from callbacks, get rid of waku_node and move to a channel based architecture a la Go or Rust (multi-threading for "free" when chronos impl. it would be another benefit). The fact that waku_node cannot be implemented in Rust should tell you that it's a 🦶 🔫. The only reason waku_node can exist is because of GC magic and multi-threading not being designed for in nwaku.

I believe that whatever functionality we want from WakuStoreSync can be achieved by coordinating components and proper configuration.

@SionoiS
Copy link
Contributor Author

SionoiS commented May 10, 2024

Basic Store Sync E2E tests done!

@SionoiS SionoiS merged commit 58d78d0 into feat--nwaku-sync May 14, 2024
9 of 15 checks passed
@SionoiS SionoiS deleted the transfer branch May 14, 2024 13:00
SionoiS added a commit that referenced this pull request Jun 3, 2024
SionoiS added a commit that referenced this pull request Jun 7, 2024
SionoiS added a commit that referenced this pull request Jul 2, 2024
SionoiS added a commit that referenced this pull request Jul 9, 2024
SionoiS added a commit that referenced this pull request Aug 2, 2024
SionoiS added a commit that referenced this pull request Aug 13, 2024
* feat: Waku Sync Protocol

* feat: state machine (#2656)

* feat: pruning storage mehcanism (#2673)

* feat: message transfer mechanism & tests (#2688)

* update docker files

* added ENR filed for sync & misc. fixes

* adding new sync range param & fixes

---------

Co-authored-by: Ivan FB <[email protected]>
Co-authored-by: Prem Chaitanya Prathi <[email protected]>
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