Initial stab at solving #181 - see notes in PR

[Miserlou]

Okay, this is my first attempt at solving #181 - there is some discussion to be had.

The good news is that it seems no modifications are necessary for the client! ip can be supplied in getAnnounceOpts and the client will send it. Phew!

This PR contains modifications to the server to support this field successfully. It adds a new cmd option, --trust-ip to correspond to the already existing --trust-proxy.

The first question is: when should we force IPv4? See here: https://github.com/feross/bittorrent-tracker/blob/master/lib/server/parse-http.js#L36

Should we only force IPv4 if trust proxy is turned off? I don't understand the original reasoning, so it's hard to update it accordingly.

The second question is: What should we do about the compact format? I think that this is currently non-BEP compliant: https://github.com/feross/bittorrent-tracker/blob/master/server.js#L635 because of BEP23, which (I think) says we can represent non-IP (hostname) IPs as encoded strings in compact format, which this regular expression will currently filter out so these peers are never returned. So, we'll need a regular expression that will return ipv4s-or-valid-hostnames, but I'd like to defer to the wizards on that one.

Still, I think that this PR will work for supplied IPs that return properly when compact=0, and updates both the server and client documentation accordingly. We just need to discuss when to force IPv4 and how to return hostname peers in the compact format.

[yciabaud]

Thank you for your contribution @Miserlou !

First of all, IPv4 is not forced but we are using IPv4 when we have an IPv4-Mapped address on IPv6. For example, if we have a peer with an IP ::ffff:192.0.2.128, we are registering it under 192.0.2.128 to make it compatible with the IPv4 only peers.

Let me rewrite it for you, I agree it is not really readable:

// Proxy case
if (opts.trustProxy && req.headers['x-forwarded-for'] ) {
  params.ip = req.headers['x-forwarded-for'] 
} else {
  params.ip = req.connection.remoteAddress
    // Remove IPv4-Mapped address prefix from IPv6 address if present
    .replace(common.REMOVE_IPV4_MAPPED_IPV6_RE, '')
}

I suggest you rewrite this along with your option to make it more readable.

Then your remark regarding the compact format. I believe we are not supporting hostnames at all and some of the code has to be updated to handle it. Maybe we can file an issue for this...

I will review your PR in details ASAP but if I can suggest one improvement, could you make a test case ensuring the custom ip is present in the swarm after announcing?

[Miserlou]

Okay, I refactored the code as you suggested (hopefully I wasn't too verbose in the commenting, but it's certainly much more grokkable now) and added a specific test function for confirming a custom ip value in and out. The compact formatting problem still isn't solved.

[Miserlou]

Is there anything that I can do to speed up having this merged?

I need this and #157 merged for a project I'm working on, and I'd really like to be able to use the officially maintained v8.1 rather than my own branches!

[yciabaud]

I would like to have some thoughts from the other collaborators regarding the compact format. I dont really know how we can handle this, maybe we should retrieves the domains for both ipv6 and ipv4 peers.

In my opinion, we should drop the regexps and we should add a ipfamily property to the data structure to be able to select the family for both ips and domains.

What do you think @Miserlou ? Any chance @feross and @DiegoRBaquero can have a look at this?

[Miserlou]

I suppose that's fine, I just don't want it to fall through the cracks since I'm using this for a project I'm actively developing! Similarly, #157 has been stalled since September, I think it'd just take Feross or Diego half an hour to look through this stuff so we can keep moving forward.

[Miserlou]

Bump.

[DiegoRBaquero]

Every peer needs to be announcing at least every 15 minutes, the seedbox must do it, one time announce from CI is not enough. If it's about using hostname, still, it must be announced every 15 minutes to not get removed. I like the overall PR. As it's optional LGTM.

[github-actions]

Is this still relevant? If so, what is blocking it? Is there anything you can do to help move it forward?