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: connection with ICE now gets target port from signalling node #624

Merged
merged 2 commits into from
Nov 14, 2023

Conversation

tegefaulkes
Copy link
Contributor

@tegefaulkes tegefaulkes commented Nov 9, 2023

Description

This PR tries to address the issue with #605. It's hard to say what the exact problem is but I'm assuming the forward connection isn't getting the most up to date information.

To attempt to fix this, this PR does the following.

  1. The NodesConnectionSignalInitial returns the host and port of the target node from the perspective of the signalling node.
  2. When doing ICE, the nodesConnectionSignalInitial is called returns the new address. The new port is then used when attempting the connection.

Issues Fixed

Tasks

Final checklist

  • Domain specific tests
  • Full tests
  • Updated inline-comment documentation
  • Lint fixed
  • Squash and rebased
  • Sanity check the final build

@tegefaulkes tegefaulkes self-assigned this Nov 9, 2023
@tegefaulkes
Copy link
Contributor Author

I made a PR for this to review the changes. We need to coordinate the design between this and the decentralised signalling process that @CMCDragonkai is working on.

@ghost
Copy link

ghost commented Nov 9, 2023

👇 Click on the image for a new way to code review

Review these changes using an interactive CodeSee Map

Legend

CodeSee Map legend

@tegefaulkes tegefaulkes marked this pull request as ready for review November 9, 2023 22:32
@CMCDragonkai
Copy link
Member

CMCDragonkai commented Nov 9, 2023

image

If the connection worked from my end, it has to work from your end. The "strict nat" idea does not make sense here, because it's always a simultaneous interaction between both nodes.

The first step is to prove empirically if simultaneous interaction is happening. We know node 1 was able to connect to node 2. But node 2 was not able to connect to node 1 when it was initiating. So dump the logs for sending packets to each other.

@CMCDragonkai
Copy link
Member

Also we cannot wait until we get a response from the signaller until we start a direct connection. We should start a direct connection, and also ask the signaller, and get a result if it is different, cancel the direct connection and start a new direct connection.

@CMCDragonkai
Copy link
Member

Also during our initial test, it shouldn't be possible that our local node graph is already out of date, because we got that information about node 2, already from the same signaller.

@CMCDragonkai
Copy link
Member

CMCDragonkai commented Nov 9, 2023

So I don't think the current PR fixes the problem. I suspect there's an issue with the signalling logic, we need logs here to show that there's simultaneous holepunching occurring with the correct ports.

@CMCDragonkai
Copy link
Member

Consider this experiment, what if you initiated signalling and held off from direct connections. And then waited a long enough time for the other side to be attempting a direct connection, then start your own direct connection. Logically one would argue this is the equivalent of when the other side is just connecting to you via ICE.

@tegefaulkes
Copy link
Contributor Author

After some extra testing I've determined that #614 also needs to be addressed in this PR to fix the issue.

@tegefaulkes
Copy link
Contributor Author

I'm going to merge this now and move on to testing it. If the problem is resolved I'll close #605.

@tegefaulkes tegefaulkes merged commit 1be5054 into staging Nov 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

NodesListConnections handler is randomly mapping IP addresses as IPv6 mapped IPv4
2 participants