IPv6 canonical priority implemented differently from BEP-40

[tearfur]

Please provide the following information

libtorrent version (or branch): RC_2_0

platform/architecture: N/A

compiler and compiler version: N/A

libtorrent's current canonical peer priority calculation for IPv6 only uses 3 masks, but according to BEP-40, there should be 10 possible masks to choose from.

I wonder what's the history on this. Was this intentional? Or libtorrent's implemention is simply outdated.

For an IPv6 address, the mask should be derived in the same way, beginning with FFFF:FFFF:FFFF:5555:5555:5555:5555:5555. If the IP addresses are in the same /48, the mask to be used should be FFFF:FFFF:FFFF:FF55:5555:5555:5555:5555. If the IP addresses are in the same /56, the mask to be used should be FFFF:FFFF:FFFF:FFFF:5555:5555:5555:5555, etc...

https://github.com/arvidn/libtorrent/blob/9aada93c982a2f0f76b129857bec3f885a37c437/src/torrent_peer.cpp#L98-L102

[tearfur]

Another confusing thing is, according to the comments, if the ports are used for generating the CRC32-C checksum, they should be in network byte order. However, the actual code uses the host byte order (aux::write_uint16() preserves the original endianness, and tcp::endpoint::port() returns in host byte order).

https://github.com/arvidn/libtorrent/blob/9aada93c982a2f0f76b129857bec3f885a37c437/src/torrent_peer.cpp#L59-L60

https://github.com/arvidn/libtorrent/blob/9aada93c982a2f0f76b129857bec3f885a37c437/src/torrent_peer.cpp#L88-L94

P.S. None of these detail about endianness are mentioned at all in BEP-40...

[arvidn]

write_uint16() (which is a wrapper around write_impl()) prints the bytes in network byte order. See the implementation here. It shifts right by 8 and masks with 0xff for the first byte, and shifts by 0 and masks by 0xff for the second byte.

[tearfur]

Correct me if I'm wrong, but instead of the implementation you linked to, I think the canonical priority code is using this implementation instead, which preserves endianness:

https://github.com/arvidn/libtorrent/blob/9aada93c982a2f0f76b129857bec3f885a37c437/include/libtorrent/io.hpp#L78-L99

https://github.com/arvidn/libtorrent/blob/9aada93c982a2f0f76b129857bec3f885a37c437/include/libtorrent/io.hpp#L157-L159

You passed a char* into aux::write_uint16(), AFAICT that won't match with the void write_uint16(T val, span<Byte>& view) signature that wraps around the write_impl() implementation you linked.

https://github.com/arvidn/libtorrent/blob/9aada93c982a2f0f76b129857bec3f885a37c437/src/torrent_peer.cpp#L88-L91

---

Also any comment about the IPv6 masks?

Edit: I realise I might be sounding like I just wanted to nitpick. So to clarify, what I want to know is, which version should other implementations be following? IMO if the BEP is going to stay the same, then all implementations, including libtorrent, should be following it.

[arvidn]

I will investigate the logic and get back to you. I agree that libttorrent does not appear to implement the BEP correctly.

Regarding the endianness, both the pointer and span versions of write_impl() produces a big endian representation. The both loop over the value, shifting down the most significant byte first.

[tearfur]

I will investigate the logic and get back to you. I agree that libttorrent does not appear to implement the BEP correctly.

Please do, thank you very much for your work.

Regarding the endianness, both the pointer and span versions of write_impl() produces a big endian representation. The both loop over the value, shifting down the most significant byte first.

My bad, turns out I'm wrong after all, so thanks for being patient with me. Embarrassingly, before typing my previous comment, I even copied into the code into godbolt to test, and somehow still thought it outputs little endian...

To set records straight, https://godbolt.org/z/d1v7ofvMq proves that aux::write_uint16() is working correctly here.

[arvidn]

thanks for the report! It's addressed here https://github.com/arvidn/libtorrent/pull/7743