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...

libtorrent/src/torrent_peer.cpp

Lines 98 to 102 in 9aada93

	static const std::uint8_t v6mask[][8] = {
	{ 0xff, 0xff, 0xff, 0xff, 0x55, 0x55, 0x55, 0x55 },
	{ 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0x55, 0x55 },
	{ 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff }
	};

[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).

libtorrent/src/torrent_peer.cpp

Lines 59 to 60 in 9aada93

	// 1. if the IP addresses are identical, hash the ports in 16 bit network-order
	// binary representation, ordered lowest first.

libtorrent/src/torrent_peer.cpp

Lines 88 to 94 in 9aada93

	std::array<char, 4> buf;
	auto ptr = buf.data();
	aux::write_uint16(e1.port(), ptr);
	aux::write_uint16(e2.port(), ptr);
	std::uint32_t p;
	std::memcpy(&p, buf.data(), 4);
	ret = crc32c_32(p);

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:

libtorrent/include/libtorrent/io.hpp

Lines 78 to 99 in 9aada93

	template <class T, class In, class OutIt>
	typename std::enable_if<(std::is_integral<In>::value
	&& !std::is_same<In, bool>::value)
	|| std::is_enum<In>::value, void>::type
	write_impl(In data, OutIt& start)
	{
	// Note: the test for [OutItT==void] below is necessary because
	// in C++11 std::back_insert_iterator::value_type is void.
	// This could change in C++17 or above
	using OutItT = typename std::iterator_traits<OutIt>::value_type;
	using Byte = typename std::conditional<
	std::is_same<OutItT, void>::value, char, OutItT>::type;
	static_assert(sizeof(Byte) == 1, "wrong iterator or pointer type");
	T val = static_cast<T>(data);
	TORRENT_ASSERT(data == static_cast<In>(val));
	for (int i = int(sizeof(T)) - 1; i >= 0; --i)
	{
	*start = static_cast<Byte>((val >> (i * 8)) & 0xff);
	++start;
	}
	}

libtorrent/include/libtorrent/io.hpp

Lines 157 to 159 in 9aada93

	template <class T, class OutIt>
	void write_uint16(T val, OutIt& start)
	{ write_impl<std::uint16_t>(val, start); }

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.

libtorrent/src/torrent_peer.cpp

Lines 88 to 91 in 9aada93

	std::array<char, 4> buf;
	auto ptr = buf.data();
	aux::write_uint16(e1.port(), ptr);
	aux::write_uint16(e2.port(), ptr);

---

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 #7743