Skip to content

fix #147 by adding support for skipping STUN #156

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

Closed
wants to merge 1 commit into from

Conversation

lithp
Copy link
Contributor

@lithp lithp commented Oct 26, 2021

No description provided.

@lithp lithp requested review from njgheorghita and ogenev October 26, 2021 01:57
@lithp lithp force-pushed the lithp/fix-peertest branch from a65a5db to ad3490a Compare October 26, 2021 01:57
@lithp lithp changed the title fix #155 by adding support for skipping STUN fix #147 by adding support for skipping STUN Oct 26, 2021
@lithp lithp force-pushed the lithp/fix-peertest branch from ad3490a to 21c9142 Compare October 26, 2021 02:01
@lithp
Copy link
Contributor Author

lithp commented Oct 26, 2021

I'm not sure which PR should make it in, this one is smaller than #155 but usage is less intuitive, adding it as an alternative but no worries if you prefer #155!

Copy link
Member

@ogenev ogenev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My personal preference is #155 😇 because It is also fixing the issue for the target node and I think adding a flag оn whether we want to use a STUN server or not solves the problem on a more fundamental level.

@njgheorghita
Copy link
Collaborator

I'm about 50/50 between these two solutions. I'd agree #155 is more comprehensive. But, it's hard for me to gauge whether or not that much comprehension will be useful or unnecessary bloat down the road? But, it seems to me that for whatever reason, we might at some point want the ability to skip stunned external ip's (maybe when dealing with testnets - especially if we end up on k8s at some point?) so I'm leaning towards #155

@njgheorghita
Copy link
Collaborator

@lithp Any final thoughts/objections? Otherwise let's move ahead with #155

@lithp
Copy link
Contributor Author

lithp commented Oct 26, 2021

I'm happy with #155 , let's merge that one!

@lithp lithp closed this Oct 26, 2021
@lithp lithp deleted the lithp/fix-peertest branch October 26, 2021 22:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants