feat: rendezvous refactor #1183
Conversation
|
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? |
|
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. |
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. |
|
@SionoiS is the PR's purpose to make those hardcoded values configurable? |
Yes and also allow the selection of specific peers for |
|
Anything else I can do or suggestions for this PR @diegomrsantos @lchenut ? Waku's rendezvous work is blocked by this. cc @jm-clius |
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. |
|
@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.
But overall, this PR looks great. |
|
Thanks for the comments. I will make the changes. |
libp2p/protocols/rendezvous.nim
Outdated
| @@ -690,6 +690,15 @@ proc new*( | |||
| minDuration = MinimumDuration, | |||
| maxDuration = MaximumDuration, | |||
| ): T = | |||
| if minDuration < 1.minutes: | |||
| raiseAssert("TTL too short: 1 minute minimum") | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Good catch, you're right, raiseAssert is not the good call here... I did my last review a bit too fast (or too early).
| 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(), |
There was a problem hiding this comment.
@lchenut just realized this. Do you remember why is this commented?
There was a problem hiding this comment.
I don't. I guess it's an artifact of an old implementation. Because it's not really useful afaik
|
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. |
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.