feat: Nwaku Sync #2403

SionoiS · 2024-02-06T21:43:18Z

Description

This is the new Waku Sync protocol. It works by wrapping the Negentropy protocol which implements RBSR.
See this research post for more info.

At configurable intervals (A sync can also be requested ad-hoc), a sync request is sent to a peer chosen by the peer manager. After multiple requests and responses between the 2 peers, a callback containing the list of missing hashes is triggered. With the list of missing hashes, store is used to request the messages from the other node.

WIP spec -> https://github.com/waku-org/specs/blob/feat--waku-sync/standards/core/sync.md

Changes

Tracking

github-actions · 2024-02-06T21:47:59Z

You can find the image built from this PR at

quay.io/wakuorg/nwaku-pr:2403-rln-v2-false

Built from 21f4fd5

waku/node/waku_node.nim

jm-clius

I initially imagined writing our own wrapper protocol around the underlying bindings (rather than just passing the payload directly onto the wire). For now, this approach will lead us to faster POC and tests, so seems reasonable to me. Do you have any intuition for what our longer term approach will be?

waku/waku_sync/protocol.nim

waku/node/waku_node.nim

SionoiS · 2024-02-08T19:56:46Z

I initially imagined writing our own wrapper protocol around the underlying bindings (rather than just passing the payload directly onto the wire). For now, this approach will lead us to faster POC and tests, so seems reasonable to me. Do you have any intuition for what our longer term approach will be?

Negentropy protocol has only one option maxFrameSize that's it. I'm not sure what other information we might want to exchange justifying wrapping the protocol. I'll think about it...

SionoiS · 2024-03-07T16:42:18Z

Sorry I squashed everything and loss your work attribution @chaitanyaprem

chaitanyaprem · 2024-03-08T01:10:12Z

Sorry I squashed everything and loss your work attribution @chaitanyaprem

Ah..hope it won't be considered as no contributions from me for the past few weeks :)

Maybe you can mention me as Co-author in comments, like this https://docs.github.com/en/pull-requests/committing-changes-to-your-project/creating-and-editing-commits/creating-a-commit-with-multiple-authors

SionoiS · 2024-03-21T16:24:56Z

I notice that requests are received twice and that connections are not closed properly. I will investigate further.

github-actions · 2024-04-30T13:16:40Z

You can find the image built from this PR at

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

Built from 760d6b2

github-actions · 2024-04-30T13:20:12Z

You can find the image built from this PR at

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

Built from 760d6b2

SionoiS · 2024-05-03T20:35:10Z

I have stashed an update with a new pruning mechanism for after the state machine PR is merged.

Makefile

vendor/negentropy

waku/factory/external_config.nim

waku/node/waku_node.nim

waku/waku_sync/protocol.nim

jm-clius

Thanks for this. Really great effort and good work (also @chaitanyaprem). I do have a few comments below which I would appreciate your looking into. Approving, though, as I do not want to stand in the way of deployed experiments starting.

jm-clius · 2024-05-15T13:33:12Z

waku/waku_sync/raw_bindings.nim

+import std/[strutils], chronicles, std/options, stew/[results, byteutils], confutils
+import ../waku_core/message
+
+const negentropyPath =


Is there no way to compile a static library into our binary here?

If not, note that this will be a resource that also needs to be included in our Docker images (and, as Prem noted elsewhere, into nwaku-compose)

It would be possible, but as per recommendations noted in nim language bindings guide in status..I chose share lib.

waku/factory/external_config.nim

waku/waku_sync/raw_bindings.nim

waku/waku_sync/storage_manager.nim

jm-clius · 2024-05-15T13:57:07Z

waku/node/waku_node.nim

@@ -193,6 +195,125 @@ proc connectToNodes*(
  # NOTE Connects to the node without a give protocol, which automatically creates streams for relay
  await peer_manager.connectToNodes(node.peerManager, nodes, source = source)

+## Waku Sync
+
+proc mountWakuSync*(


Still a bit hard for me to see why this would not be better managed in a composite protocol WakuStoreSync :). I understand the objection to arbitrarily creating dependencies, but in this case we are defining a composite protocol that depends unidirectionally on wakuSync, a local archive and a storeClient. This is now simply implicit in the mount code, but imho can be better managed if the relationship is made explicit in a separate module. We should also expect this to become more complex in future, with e.g. sanity checks for local archive and negentropy configurations, penalising non-responsive/misbehaving store sync peers, etc. In fact, the interrelationship between all these elements to form Store Sync should likely be a specification on its own.

I kind of agree, but maybe for now we can leave it as is and refactor this once we start next phase of work which involves handling more scenarios etc?

waku/waku_sync/protocol.nim

waku/node/waku_node.nim

SionoiS · 2024-07-30T14:55:06Z

After discussing with @jm-clius I will have to add a SyncRange and a SyncInterval so that we can sync the last hour every 5m for example.

Ivansete-status

Great one! Congrats for it! 🙌
I've added a bunch of comments, some of them are pedantic and others are potentially more interesting :)
Very insightful PR

Makefile

waku/waku_sync.nim

waku/waku_sync/codec.nim

waku/waku_sync/protocol.nim

Ivansete-status · 2024-07-31T17:10:37Z

waku/waku_sync/protocol.nim

+        break
+
+      for (hash, timestamp) in elements:
+        self.storage.erase(timestamp, hash).isOkOr:


Yes you are right @SionoiS
We need a mechanism to measure the memory occupied by negentropy.

By using C++ we are at high risk of having memory leaks there, for example, this sounds like a candidate that we need to enhance: https://github.com/waku-org/negentropy/blob/311a21a22bdb6d80e5c4ba5e3d2f550e0062b2cb/cpp/negentropy_wrapper.cpp#L188

I agree @NagyZoltanPeter , we need to review the library as well :)

waku/node/waku_node.nim

gabrielmer

Woow, thanks so much! Amazing work!

Started reviewing and will be adding batches of comments/questions :)

waku/waku_sync/protocol.nim

waku/waku_core/time.nim

waku/waku_sync/storage_manager.nim

SionoiS · 2024-08-01T15:10:07Z

Let me know when I can rebase I don't want to mess your reviews.

NagyZoltanPeter

Highest level, pretty, neat and awesome feature. Thank you!!!
I will love to see it working.
Left some observations, mostly questions.

waku/waku_sync/protocol.nim

waku/waku_sync/session.nim

waku/waku_sync/common.nim

waku/waku_sync/protocol.nim

NagyZoltanPeter · 2024-08-01T14:17:20Z

waku/waku_sync/protocol.nim

+      tries = 3
+      while true:
+        (await callback(hashes, peer.peerId)).isOkOr:
+          error "transfer callback failed", error = $error


If we continously retry, why is this an error? If its an error why we think it will be ok at next try?
I'm asking because of operator point of view, when one can see repetitive errors in log, what he/she supposed to do against?

True maybe log error when done trying?

Yes. Is there threshold when we can consider giving up probing?

No other protocol has retry mechanism it's up to us the define good params. 3 tries was chosen arbitrarily.

NagyZoltanPeter · 2024-08-01T14:19:37Z

waku/waku_sync/protocol.nim

+      peer: RemotePeerInfo
+      tries = 3
+
+    while true:


Honestly I lost in the soooo many break statements....

It's called break dance. 🤣

Does Nim have labels for loop control break: myLoop or continue: myOtherLoop ? I don't think so?

Yes, it does not have, while using while true: and breaks seems to me controversial.
Once upon a time there was an article, Goto statement considered harmful, which is resonates with this, especially with two embeded while true:... reading it is not easy.

IDK how loops can be avoided. I'm open to suggestion.

NagyZoltanPeter · 2024-08-01T19:02:50Z

waku/waku_core/time.nim

@@ -33,3 +33,16 @@ template nanosecondTime*(collector: Gauge, body: untyped) =
    metrics.set(collector, nowInUnixFloat() - start)
  else:
    body
+
+# Unused yet
+#[ proc timestampInSeconds*(time: Timestamp): Timestamp =


If we think (suppose) the value will be an actual time value (not a diff), we don't need to convert it to string. Isn't that cheaper to just compare to some numeric value? Am I miss something? Btw why we expect the value may vary between milli/micro and nano secs?

I don't know. @chaitanyaprem wrote this part maybe he can answer.

NagyZoltanPeter · 2024-08-01T19:32:17Z

waku/waku_sync/protocol.nim

+    if hashes.len > 0 and self.transferCallBack.isSome():
+      let callback = self.transferCallBack.get()
+
+      (await callback(hashes, conn.peerId)).isOkOr:


I feel it, but still it can be an option to move out these refs out of this protocol into a separate callback setup place. That would eliminate the need for refs of WakuArchive and WakuStoreClient.
Now they need to be provided with new*.
In other words by separating setting up the callback from the protocol will move away the decision how that is implemented. Also enables mocking it for testing. WDYT?

waku/node/waku_node.nim

NagyZoltanPeter · 2024-08-01T19:48:18Z

Let me know when I can rebase I don't want to mess your reviews.

Left some comments, so from my side you can go with it.

Ivansete-status

Thanks for it 💯 ! I think we can have some more simplification :P

waku/waku_sync/protocol.nim

gabrielmer · 2024-08-07T14:57:42Z

waku/waku_sync/protocol.nim

+
+      peer
+
+  let connOpt = await self.peerManager.dialPeer(peer, WakuSyncCodec)


we should also make sure that we properly close the connections, would test and take a look that open connections don't grow indefinitely

Ivansete-status

LGTM! Thanks for the patience! 💯

Co-authored-by: Ivan FB <[email protected]> Co-authored-by: Prem Chaitanya Prathi <[email protected]>

We shouldn't assume it exists on the host. Introduced by: #2403 Signed-off-by: Jakub Sokołowski <[email protected]>

SionoiS self-assigned this Feb 6, 2024

SionoiS mentioned this pull request Feb 6, 2024

[Deliverable] Store v3 - store synchronisation waku-org/pm#132

Open

3 tasks

SionoiS added E:2.3: Basic distributed Store services See https://github.com/waku-org/pm/issues/64 for details and removed E:2.3: Basic distributed Store services See https://github.com/waku-org/pm/issues/64 for details labels Feb 6, 2024

SionoiS force-pushed the feat--nwaku-sync branch from 3635cfc to 6bc4e3a Compare February 7, 2024 14:15

alrevuelta reviewed Feb 7, 2024

View reviewed changes

waku/node/waku_node.nim Outdated Show resolved Hide resolved

jm-clius reviewed Feb 8, 2024

View reviewed changes

waku/waku_sync/protocol.nim Outdated Show resolved Hide resolved

waku/waku_sync/protocol.nim Outdated Show resolved Hide resolved

waku/node/waku_node.nim Outdated Show resolved Hide resolved

SionoiS mentioned this pull request Feb 13, 2024

feat: Integrate Negentropy RBSR library for Waku Sync #2426

Closed

chaitanyaprem force-pushed the feat--nwaku-sync branch from 2f42740 to 0b5a025 Compare March 7, 2024 05:06

SionoiS force-pushed the feat--nwaku-sync branch 2 times, most recently from 203c0fc to a31d630 Compare March 7, 2024 16:39

SionoiS force-pushed the feat--nwaku-sync branch from a31d630 to 9fca248 Compare March 8, 2024 13:16

chaitanyaprem force-pushed the feat--nwaku-sync branch from bb10593 to a6ea792 Compare March 19, 2024 08:48

SionoiS force-pushed the feat--nwaku-sync branch from 95c2c36 to d76b3d0 Compare May 3, 2024 15:29

SionoiS force-pushed the feat--nwaku-sync branch 2 times, most recently from 58d78d0 to 0ae1858 Compare May 14, 2024 13:03

SionoiS marked this pull request as ready for review May 14, 2024 15:45

SionoiS requested review from jm-clius, chaitanyaprem and Ivansete-status May 14, 2024 15:46

chaitanyaprem reviewed May 15, 2024

View reviewed changes

jm-clius approved these changes May 15, 2024

View reviewed changes

Ivansete-status reviewed Jul 31, 2024

View reviewed changes

gabrielmer reviewed Aug 1, 2024

View reviewed changes

NagyZoltanPeter reviewed Aug 1, 2024

View reviewed changes

SionoiS force-pushed the feat--nwaku-sync branch from 5c14b43 to 4c310dd Compare August 2, 2024 12:25

Ivansete-status reviewed Aug 7, 2024

View reviewed changes

waku/waku_sync/protocol.nim Show resolved Hide resolved

waku/waku_sync/protocol.nim Show resolved Hide resolved

waku/waku_sync/protocol.nim Show resolved Hide resolved

waku/waku_sync/protocol.nim Show resolved Hide resolved

waku/waku_sync/protocol.nim Show resolved Hide resolved

gabrielmer reviewed Aug 7, 2024

View reviewed changes

Ivansete-status approved these changes Aug 12, 2024

View reviewed changes

SionoiS force-pushed the feat--nwaku-sync branch 3 times, most recently from f987bb9 to 499c5e6 Compare August 13, 2024 11:23

SionoiS and others added 7 commits August 13, 2024 07:25

feat: Waku Sync Protocol

ee9c78f

Co-authored-by: Ivan FB <[email protected]> Co-authored-by: Prem Chaitanya Prathi <[email protected]>

feat: state machine (#2656)

ecfeb5d

feat: pruning storage mehcanism (#2673)

25a68e1

feat: message transfer mechanism & tests (#2688)

b8986c7

update docker files

09b1b5d

added ENR filed for sync & misc. fixes

5fafa17

adding new sync range param & fixes

97c050f

SionoiS force-pushed the feat--nwaku-sync branch from 499c5e6 to 97c050f Compare August 13, 2024 11:26

SionoiS merged commit 2cc86c5 into master Aug 13, 2024
8 of 9 checks passed

SionoiS deleted the feat--nwaku-sync branch August 13, 2024 11:27

chair28980 mentioned this pull request Aug 20, 2024

[Epic: nwaku] Telemetry: fleet logging #3080

Closed

6 tasks

jakubgs added a commit that referenced this pull request Aug 21, 2024

fix: copy libnegentropy.so from nim-build image

051b766

We shouldn't assume it exists on the host. Introduced by: #2403 Signed-off-by: Jakub Sokołowski <[email protected]>

jakubgs mentioned this pull request Aug 21, 2024

fix: copy libnegentropy.so from nim-build image #2991

Merged

jakubgs added a commit that referenced this pull request Aug 21, 2024

fix: copy libnegentropy.so from nim-build image

65518d8

We shouldn't assume it exists on the host. Introduced by: #2403 Signed-off-by: Jakub Sokołowski <[email protected]>

jakubgs added a commit that referenced this pull request Aug 21, 2024

fix: copy libnegentropy.so from nim-build image

c794a2a

We shouldn't assume it exists on the host. Introduced by: #2403 Signed-off-by: Jakub Sokołowski <[email protected]>

gabrielmer pushed a commit that referenced this pull request Aug 22, 2024

fix: copy libnegentropy.so from nim-build image (#2991)

2edc0bc

We shouldn't assume it exists on the host. Introduced by: #2403 Signed-off-by: Jakub Sokołowski <[email protected]>

gabrielmer pushed a commit that referenced this pull request Aug 22, 2024

fix: copy libnegentropy.so from nim-build image (#2991)

5a9703f

We shouldn't assume it exists on the host. Introduced by: #2403 Signed-off-by: Jakub Sokołowski <[email protected]>


		peer

		let connOpt = await self.peerManager.dialPeer(peer, WakuSyncCodec)

feat: Nwaku Sync #2403

feat: Nwaku Sync #2403

Conversation

SionoiS commented Feb 6, 2024 • edited Loading

Description

Changes

github-actions bot commented Feb 6, 2024 • edited Loading

jm-clius left a comment

Choose a reason for hiding this comment

SionoiS commented Feb 8, 2024

SionoiS commented Mar 7, 2024 • edited Loading

chaitanyaprem commented Mar 8, 2024

SionoiS commented Mar 21, 2024 • edited Loading

github-actions bot commented Apr 30, 2024 • edited Loading

github-actions bot commented Apr 30, 2024 • edited Loading

SionoiS commented May 3, 2024

jm-clius left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chaitanyaprem May 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SionoiS commented Jul 30, 2024

Ivansete-status left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gabrielmer left a comment

Choose a reason for hiding this comment

SionoiS commented Aug 1, 2024

NagyZoltanPeter left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SionoiS Aug 1, 2024 • edited Loading

Choose a reason for hiding this comment

NagyZoltanPeter Aug 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NagyZoltanPeter commented Aug 1, 2024 • edited Loading

Ivansete-status left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ivansete-status left a comment

Choose a reason for hiding this comment

SionoiS commented Feb 6, 2024 •

edited

Loading

github-actions bot commented Feb 6, 2024 •

edited

Loading

SionoiS commented Mar 7, 2024 •

edited

Loading

SionoiS commented Mar 21, 2024 •

edited

Loading

github-actions bot commented Apr 30, 2024 •

edited

Loading

github-actions bot commented Apr 30, 2024 •

edited

Loading

chaitanyaprem May 15, 2024 •

edited

Loading

SionoiS Aug 1, 2024 •

edited

Loading

NagyZoltanPeter Aug 1, 2024 •

edited

Loading

NagyZoltanPeter commented Aug 1, 2024 •

edited

Loading