feat: rendezvous refactor #1183

SionoiS · 2024-08-28T19:47:19Z

Hello!

This PR aim to refactor rendezvous code so that it is easier to impl. Waku rdv strategy. The hardcoded min and max TTL were out of range with what we needed and specifying which peers to interact with is also needed since Waku deals with peers on multiple separate shards.

I tried to keep the changes to a minimum, specifically I did not change the name of any public procs which result in less than descriptive names in some cases. I also wanted to return results instead of raising exceptions but didn't. Would it be acceptable to do so?

Please advise on best practices, thank you.

diegomrsantos · 2024-08-28T19:57:15Z

Could you please add to the PR description a high-level overview of the changes and if possible how they make the protocol easier to use?

lchenut · 2024-08-30T11:28:11Z

The reason why min and max TTL are hardcoded is because those limits are set by the specifications: https://github.com/libp2p/specs/blob/master/rendezvous/README.md?plain=1#L270-L273.
In theory, you shouldn't have to change those.

SionoiS · 2024-08-30T12:16:58Z

The reason why min and max TTL are hardcoded is because those limits are set by the specifications: https://github.com/libp2p/specs/blob/master/rendezvous/README.md?plain=1#L270-L273. In theory, you shouldn't have to change those.

I understand but those are only recommendations. In the context of Waku we decided to go with more "lively" registrations lifecycle hence the shorter TTL. I know we are bending the intended purpose of rendezvous a little but we are not breaking the core spec.

libp2p/protocols/rendezvous.nim

diegomrsantos · 2024-08-30T12:48:48Z

@SionoiS is the PR's purpose to make those hardcoded values configurable?

SionoiS · 2024-08-30T14:31:30Z

@SionoiS is the PR's purpose to make those hardcoded values configurable?

Yes and also allow the selection of specific peers for advertise and request

SionoiS · 2024-09-16T12:42:21Z

Anything else I can do or suggestions for this PR @diegomrsantos @lchenut ?

Waku's rendezvous work is blocked by this.

cc @jm-clius

diegomrsantos · 2024-09-19T14:38:11Z

I also wanted to return results instead of raising exceptions but didn't. Would it be acceptable to do so?

The use of exceptions and results isn't consistent in the project, we still need to define a new strategy for new code. I would stick with what is implemented instead of changing it when it comes to existing code.

libp2p/protocols/rendezvous.nim

lchenut · 2024-09-19T14:51:23Z

@SionoiS I rechecked and the only thing that annoys me with this PR is that you do not verify anything about the minimum and the maximum ttl.

Users can set a minimum bigger than the maximum, which will cause a weird behavior.
While I get why you could use a shorter TTL, using one bigger than 72 hours seems a bit excessive, I would limit the maximum.

But overall, this PR looks great.

libp2p/protocols/rendezvous.nim

SionoiS · 2024-09-19T19:05:48Z

Thanks for the comments. I will make the changes.

lchenut

LGTM!

diegomrsantos · 2024-09-20T10:12:07Z

libp2p/protocols/rendezvous.nim

@@ -690,6 +690,15 @@ proc new*(
    minDuration = MinimumDuration,
    maxDuration = MaximumDuration,
 ): T =
+  if minDuration < 1.minutes:
+    raiseAssert("TTL too short: 1 minute minimum")


I'm not sure raising a Defect here is appropriate. Defect types (like those raised by raiseAssert) are meant to indicate programming errors - situations that should never occur if the code is correct. They are not intended for handling invalid input or runtime errors that can occur during normal operation. The invalid minDuration and maxDuration values represent incorrect usage of the new procedure by the caller, possibly due to user input or misconfiguration. These are not programming errors but runtime issues that should be handled gracefully. By raising an Exception, you allow the caller of the procedure to catch and handle the exception appropriately.

Good catch, you're right, raiseAssert is not the good call here... I did my last review a bit too fast (or too early).

diegomrsantos

Please address the defect concern and this PR should also include tests for the features added.

diegomrsantos · 2024-09-20T11:48:53Z

libp2p/protocols/rendezvous.nim

+  let
+    minTTL = minDuration.seconds.uint64
+    maxTTL = maxDuration.seconds.uint64
+
  let rdv = T(
    rng: rng,
    salt: string.fromBytes(generateBytes(rng[], 8)),
    registered: initOffsettedSeq[RegisteredData](1),
    defaultDT: Moment.now() - 1.days,
    #registerEvent: newAsyncEvent(),


@lchenut just realized this. Do you remember why is this commented?

I don't. I guess it's an artifact of an old implementation. Because it's not really useful afaik

SionoiS · 2024-09-20T12:06:37Z

The first thing I did was raise exceptions but I forgot about the pragma and was confused it didn't work so I tried something else. We good now.

I also added a test for misconfig.

diegomrsantos · 2024-09-23T12:51:33Z

specifying which peers to interact with is also needed since Waku deals with peers on multiple separate shards.

Thank you for the changes. Could you please elaborate more on this feature?

SionoiS · 2024-09-23T16:01:13Z

specifying which peers to interact with is also needed since Waku deals with peers on multiple separate shards.

Thank you for the changes. Could you please elaborate more on this feature?

Waku rendezvous strategy will be to register at rdv points for multiple namespaces specific to GossipSub topics that a node uses. Without a way to choose specific peers to register at, it was not possible to impl. this strategy.

diegomrsantos · 2024-09-23T18:07:12Z

libp2p/protocols/rendezvous.nim

-) {.async, base.} =
-  let sprBuff = rdv.switch.peerInfo.signedPeerRecord.encode().valueOr:
-    raise newException(RendezVousError, "Wrong Signed Peer Record")
+proc batchAdvertise*(


I have just realized this has been changed from a method to a proc, could you please explain why?

The advertise method now call a public batchAdvertise proc so that the interface still work like before but you can also use the proc with the new parameters.

@lchenut does it make sense to pass the peers as a parameter here instead of using the seq inside the rdv instance?

There's really nothing against it. In theory, the rendezvous protocol manage this with the peer event handler on joined/left, but if you want to use your own, why not. It wasn't something we needed at the beginning (because we don't really use it outside testing), but if waku needs it, it's a good thing to implement.

Ideally, it would be good to add docs explaining the use of those procs and testing covering them. I don't know if different names for them are a good idea either.

@lchenut it'd be good to add to the issue an evaluation of the use of methods and procs.

I'm not sure I understand your last point

Some things are implemented as procs and some as methods, why?

I did a little checking. The Rendezvous protocol inherits from LPProtocol, so it should have a start and stop method, which it does. But advertise is also a method, which is wrong, it should be proc.

This explains the difference and when to use one or the other https://matthiashager.com/proc-method-nim

diegomrsantos

Thank you, it looks good.

SionoiS · 2024-09-24T19:32:01Z

I changed the names but I'll let someone else do the docs since there's none yet.

added test wrt subscribe and unsubscribe added tests/pubsub/testgossipinternal2 file linters feat: rendezvous refactor (#1183) Hello! This PR aim to refactor rendezvous code so that it is easier to impl. Waku rdv strategy. The hardcoded min and max TTL were out of range with what we needed and specifying which peers to interact with is also needed since Waku deals with peers on multiple separate shards. I tried to keep the changes to a minimum, specifically I did not change the name of any public procs which result in less than descriptive names in some cases. I also wanted to return results instead of raising exceptions but didn't. Would it be acceptable to do so? Please advise on best practices, thank you. --------- Co-authored-by: Ludovic Chenut <[email protected]> refactor and suite name refactor chore(ci): Enable S3 caching for interop (#1193) - Adds our S3 bucket for caching docker images as Protocol Labs shut down their shared one. - Remove the free disk space workaround that prevented the jobs from failing for using too much space for the images. --------- Co-authored-by: diegomrsantos <[email protected]> PR review comment changes

rendezvous refactor

d643f95

SionoiS requested review from diegomrsantos and lchenut August 28, 2024 19:47

SionoiS self-assigned this Aug 28, 2024

SionoiS mentioned this pull request Aug 29, 2024

feat: waku rendezvous wrapper waku-org/nwaku#2962

Draft

3 tasks

diegomrsantos reviewed Aug 30, 2024

View reviewed changes

libp2p/protocols/rendezvous.nim Outdated Show resolved Hide resolved

diegomrsantos reviewed Aug 30, 2024

View reviewed changes

libp2p/protocols/rendezvous.nim Outdated Show resolved Hide resolved

added back consts & renaming

3b8d520

SionoiS mentioned this pull request Sep 10, 2024

[Deliverable] Usage of rendezvous waku-org/pm#199

Open

Merge branch 'master' into feat--rendezvous-refactor

c6e79d6

diegomrsantos reviewed Sep 19, 2024

View reviewed changes

libp2p/protocols/rendezvous.nim Outdated Show resolved Hide resolved

diegomrsantos reviewed Sep 19, 2024

View reviewed changes

libp2p/protocols/rendezvous.nim Show resolved Hide resolved

added ttl limits & better error msg

771da4e

lchenut approved these changes Sep 20, 2024

View reviewed changes

diegomrsantos reviewed Sep 20, 2024

View reviewed changes

diegomrsantos suggested changes Sep 20, 2024

View reviewed changes

diegomrsantos reviewed Sep 20, 2024

View reviewed changes

tests & raising exceptions

8aa5c43

SionoiS requested review from diegomrsantos and lchenut September 23, 2024 12:26

diegomrsantos reviewed Sep 23, 2024

View reviewed changes

diegomrsantos approved these changes Sep 24, 2024

View reviewed changes

proc names

9baffa5

diegomrsantos approved these changes Sep 25, 2024

View reviewed changes

diegomrsantos added this pull request to the merge queue Sep 25, 2024

Merged via the queue into master with commit d389d96 Sep 25, 2024
14 checks passed

diegomrsantos deleted the feat--rendezvous-refactor branch September 25, 2024 10:10

lchenut mentioned this pull request Sep 27, 2024

docs: public rendezvous procedure #1203

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: rendezvous refactor #1183

feat: rendezvous refactor #1183

SionoiS commented Aug 28, 2024 •

edited

Loading

diegomrsantos commented Aug 28, 2024

lchenut commented Aug 30, 2024

SionoiS commented Aug 30, 2024

diegomrsantos commented Aug 30, 2024

SionoiS commented Aug 30, 2024

SionoiS commented Sep 16, 2024

diegomrsantos commented Sep 19, 2024

lchenut commented Sep 19, 2024 •

edited

Loading

SionoiS commented Sep 19, 2024

lchenut left a comment

diegomrsantos Sep 20, 2024 •

edited

Loading

lchenut Sep 20, 2024

diegomrsantos left a comment

diegomrsantos Sep 20, 2024

lchenut Sep 20, 2024

SionoiS commented Sep 20, 2024 •

edited

Loading

diegomrsantos commented Sep 23, 2024

SionoiS commented Sep 23, 2024

diegomrsantos Sep 23, 2024

SionoiS Sep 23, 2024

diegomrsantos Sep 23, 2024

lchenut Sep 24, 2024 •

edited

Loading

diegomrsantos Sep 24, 2024

diegomrsantos Sep 25, 2024

lchenut Sep 25, 2024

diegomrsantos Sep 25, 2024

lchenut Sep 27, 2024

diegomrsantos Sep 27, 2024

diegomrsantos left a comment

SionoiS commented Sep 24, 2024

feat: rendezvous refactor #1183

feat: rendezvous refactor #1183

Conversation

SionoiS commented Aug 28, 2024 • edited Loading

diegomrsantos commented Aug 28, 2024

lchenut commented Aug 30, 2024

SionoiS commented Aug 30, 2024

diegomrsantos commented Aug 30, 2024

SionoiS commented Aug 30, 2024

SionoiS commented Sep 16, 2024

diegomrsantos commented Sep 19, 2024

lchenut commented Sep 19, 2024 • edited Loading

SionoiS commented Sep 19, 2024

lchenut left a comment

Choose a reason for hiding this comment

diegomrsantos Sep 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

diegomrsantos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SionoiS commented Sep 20, 2024 • edited Loading

diegomrsantos commented Sep 23, 2024

SionoiS commented Sep 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lchenut Sep 24, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

diegomrsantos left a comment

Choose a reason for hiding this comment

SionoiS commented Sep 24, 2024

SionoiS commented Aug 28, 2024 •

edited

Loading

lchenut commented Sep 19, 2024 •

edited

Loading

diegomrsantos Sep 20, 2024 •

edited

Loading

SionoiS commented Sep 20, 2024 •

edited

Loading

lchenut Sep 24, 2024 •

edited

Loading