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

Clarify ports / public ports in node HTTP getinfo #696

Merged
merged 7 commits into from
Aug 15, 2022

Conversation

pinheadmz
Copy link
Member

Closes #689

The issue is that the ports displayed in info are the public ports set by --public-port and --public-brontide-port not --port and --brontide-port. Public hosts and ports are what we advertise to peers when we are listening for inbound connections, this is the hostname that gets gossiped around the network. The other options set the actual host and port the server actually listens to 🤷‍♂️

Current and unchanged behavior of hsd

  • We will not advertise our own address unless --listen=true
  • On mainnet (only) we will not advertise our own address unless an address is specifically passed with --public-host
    • This behavior was added to hsd to prevent unreachable nodes from accidentally (user error) advertising

This PR adds some new fields to the pool object in the "get info" JSON. The existing data will now indicate the actual listening host/port and the public object will indicate the advertised data as set by the user. (See examples below).

This PR also refactors the node-http-test module to make more room for this coverage, and introduces a new p2p-test module which connects mainnet nodes together with different settings and monitors the ADDR packets transmitted.

ex. 1

hsd --memory=true --network=main

  "pool": {
    "host": "0.0.0.0",
    "port": 12038,
    "brontidePort": 44806,
    "identitykey": "ang7l54xdrg47duk3jax3f5blmgblfkkwq6t6fs7luyoawehh5jk4",
    "agent": "/hsd:3.0.1/",
    "services": "1",
    "outbound": 8,
    "inbound": 0,
    "public": {
      "listen": false,
      "host": null,
      "port": null,
      "brontidePort": null
    }
  },

ex. 2

hsd --memory=true --network=main --listen=true

  "pool": {
    "host": "0.0.0.0",
    "port": 12038,
    "brontidePort": 44806,
    "identitykey": "al47esnqoajntzsekilqjoq6fcyeq7uuzxr4d2j62ug34kydhbqbg",
    "agent": "/hsd:3.0.1/",
    "services": "1",
    "outbound": 8,
    "inbound": 0,
    "public": {
      "listen": true,
      "host": null,
      "port": null,
      "brontidePort": null
    }
  },

ex. 3

hsd --memory=true --network=main --listen=true --public-host=200.99.98.97 --public-port=12345 --public-brontide-port=54321

  "pool": {
    "host": "0.0.0.0",
    "port": 12038,
    "brontidePort": 44806,
    "identitykey": "anv6dprpxnyqjxgmljw2ys7p5yi7s4syg6vc5x2eptym3x7jaluha",
    "agent": "/hsd:3.0.1/",
    "services": "1",
    "outbound": 8,
    "inbound": 0,
    "public": {
      "listen": true,
      "host": "200.99.98.97",
      "port": 12345,
      "brontidePort": 54321
    }
  },

@coveralls
Copy link

coveralls commented Mar 5, 2022

Coverage Status

Coverage increased (+0.5%) to 67.748% when pulling 69e0759 on pinheadmz:ports-getinfo into bf614e0 on handshake-org:master.

Copy link
Member

@rithvikvibhu rithvikvibhu left a comment

Choose a reason for hiding this comment

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

LGTM. pool host and ports and the public host and ports are shown correctly.

Isn't there a public-brontide-port like how port has public-port?
https://hsd-dev.org/guides/config.html doesn't have it so I guess not, just checking.

Edit: just re-read the desc above and there is. So the config page needs to be edited to include --public-brontide-port

@rithvikvibhu
Copy link
Member

Also, how do you feel about defaulting public-port to port? Same for brontide-port.

@pinheadmz
Copy link
Member Author

Also, how do you feel about defaulting public-port to port? Same for brontide-port.

I think no, because they are used for separate things. port and publicPort are both set to the same network default. If a user wants to set both they should set both manually.

@pinheadmz pinheadmz merged commit 3fd74e0 into handshake-org:master Aug 15, 2022
@nodech nodech mentioned this pull request Jan 17, 2023
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.

HTTP api info returns incorrect ports
3 participants