chore: test peer connection management

[Ivansete-status]

Description

Enhance reconnect logic and fix reconnect tests.
The important point is that it is necessary to set a port different than zero in tests.

Changes

Issue

closes

• bug: Peer Reconnection not working? #2592

[github-actions]

You can find the image built from this PR at

quay.io/wakuorg/nwaku-pr:3049

Built from 7b25036

[gabrielmer]

Looks amazing! Thanks so much!

Added a small question :)

[gabrielmer]

Not sure I understand why this is true. Shouldn't they be theoretically equal, but in practice any of them be longer than the other one?

Or in other words, why reconnectDuration + backoffPeriod.seconds.float can't be greater than reconnectDurationWithBackoffPeriod?

[Ivansete-status]

Not sure I understand why this is true. Shouldn't they be theoretically equal, but in practice any of them be longer than the other one?

Or in other words, why reconnectDuration + backoffPeriod.seconds.float can't be greater than reconnectDurationWithBackoffPeriod?

Actually, in both cases we are doing the same operation but, in when the backoff time assigned is != 0, then we add this extra block:

    if backoffTime > ZeroDuration:
      debug "Backing off before reconnect",
        peerId = peerInfo.peerId, backoffTime = backoffTime
      # We disconnected recently and still need to wait for a backoff period before connecting
      await sleepAsync(backoffTime)

... where we are adding an extra backoffTime plus the time spent in the debug call plus the if statement. Hence, the > condition should happen.

[gabrielmer]

Mmm I think that this can be problematic, the only difference is the if statement, the log and the time it takes to return from the sleep after the time has passed.

But for example, if the call to await pm.connectToNodes(@[peerInfo]) in the case without the backoff for some reason takes longer than in the run with the backoff, it can happen that the statement won't be true.

So given that it can be false in certain cases and that I don't think that the check adds much, I would personally remove it. But if not we can keep it and remove it if we see that it gives problems :)

[Ivansete-status]

Morning! ah yes , makes sense. For now I just simplified the test in 772131e
Thanks!