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

Allow the node to auto-forward its listnening port using UPnP #1575

Open
wants to merge 2 commits into
base: testnet2-2022
Choose a base branch
from

Conversation

ljedrz
Copy link
Collaborator

@ljedrz ljedrz commented Jan 18, 2022

The testnet2 equivalent of https://github.com/AleoHQ/snarkOS/pull/1218; the only difference is that the external address is not registered, but it can be extended with that functionality if need be.

Supersedes https://github.com/AleoHQ/snarkOS/pull/1218.
Closes https://github.com/AleoHQ/snarkOS/issues/423.

@ljedrz
Copy link
Collaborator Author

ljedrz commented Jan 18, 2022

The CI error is unrelated:

running 1 test
test helpers::block_requests::tests::test_block_requests_case_2cbb ... FAILED

failures:

---- helpers::block_requests::tests::test_block_requests_case_2cbb stdout ----
thread 'helpers::block_requests::tests::test_block_requests_case_2cbb' panicked at 'assertion failed: `(left == right)`
  left: `AbortAndDisconnect(TwoCBA, "exceeded fork range")`,
 right: `Proceed(TwoCBB, BlockRequestHandlerProceed { start_block_height: 904, end_block_height: 1153, ledger_is_on_fork: true })`', src/helpers/block_requests.rs:572:13

niklaslong
niklaslong previously approved these changes Jan 18, 2022
Copy link
Collaborator

@niklaslong niklaslong left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@ljedrz
Copy link
Collaborator Author

ljedrz commented Jan 28, 2022

Rebased.

@howardwu
Copy link
Member

I think this is worth including. Question: is there a way to harmonize this feature with the design, so that we adhere to an implicit design for using upnp (as opposed to having the user explicitly set a upnp flag)?

For example, is there a concept of using this first, then defaulting to basic tcp/ip if it does not hold?

@ljedrz
Copy link
Collaborator Author

ljedrz commented Feb 14, 2022

There's a reason why I proposed this functionality to be opt-in rather than being automatic; if for whatever reason someone is changing their listening port on a regular basic, or if they are using internal nodes that are communicating with the network only via a single proxy exposed to it, or if there are any other reasons why someone wouldn't want their port to be opened automatically (e.g. a complex network configuration), it would potentially break their setups and/or eventually leave them with many open ports that they might not be aware about.

That being said, this functionality only requires the user to run with the --upnp flag once, after which they can forget about it even being there.

@ljedrz ljedrz changed the base branch from testnet2 to testnet3 March 1, 2022 10:09
@ljedrz
Copy link
Collaborator Author

ljedrz commented Mar 1, 2022

Rebased against testnet3.

@aleo-svc-exp
Copy link

Admin: Please respond with "build this" in order to build this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request network A network-related issue testnet2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature] Use UPnP to open ports
4 participants