From 04d15c2b8d807630726c8df99c873a3ea05e22e9 Mon Sep 17 00:00:00 2001 From: Daniel Urumov Date: Sat, 12 Sep 2026 21:36:38 +0300 Subject: [PATCH 1/2] fix(dht): decide a values peer's family by transport, not by blob length MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `decode_value_peers/1` tested `rem(size, @ipv6_peer_info_size) == 0` before the IPv4 stride. **18 is a multiple of 6**, so a `values` string packing three compact IPv4 peers has exactly the size of one compact IPv6 peer and was decoded as one — an IPv6 address invented out of three IPv4 ones. BEP 5 nominally puts one peer per string, but implementations pack them, so this is a shape we do meet. A phantom endpoint is not merely wasted work. It consumes a dial slot, can never connect, and its failure is recorded against the **IPv6** family in `DialStats` — which is what drives the per-family dial throttle. So the wrong guess teaches the engine that IPv6 does not work, on a host where IPv6 is the family that does (measured 23.5% yield against 0.9% on v4). That is the same failure shape as the `:add_peer_failed` misclassification fixed earlier: a local mistake counted as a network verdict, feeding a control loop. BEP 32 runs the IPv6 DHT as a separate DHT, which makes the transport the authority — a response that came back over IPv6 carries IPv6 peers. The family is already in hand at the call site (`apply_trusted_response/6` has the remote `ip`), so it is now passed down and consulted first. Length remains the tie-breaker where that family's own unit does not divide the string — a lone IPv4 peer answered over the v6 DHT, say — and callers that pass no family keep the previous length-only behaviour, so nothing else changes. Found while investigating why two torrents had **zero** IPv6 candidates out of 27 and 34 known endpoints. This is not that answer: the bug invents v6 endpoints rather than losing them, and both torrents' DHT shortlists were a balanced 4 v6 / 4 v4, so v6 nodes were being queried. Recorded so the real question stays open. +4 tests covering both readings of an 18-byte blob, the length fallback, and the unchanged no-family default. Co-authored-by: Cursor --- CHANGELOG.md | 10 ++++++ lib/elixir_torrent/dht.ex | 11 ++++-- lib/elixir_torrent/dht/krpc.ex | 63 ++++++++++++++++++++++------------ test/ipv6_dial_test.exs | 51 +++++++++++++++++++++++++++ 4 files changed, 111 insertions(+), 24 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 57ca70e..bc7ecfa 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -51,6 +51,16 @@ speed of 0 B/s reported on a torrent moving at 100 KB/s. oscillates, and a torrent slower than one piece per window reads 0 until its first piece lands. +- A packed `values` string in a `get_peers` response is no longer mistaken for an + IPv6 peer. **18 is a multiple of 6**, so a string carrying three compact IPv4 + peers has exactly the size of one compact IPv6 peer, and the decoder tested the + IPv6 stride first — inventing an IPv6 address out of three IPv4 ones. A phantom + endpoint is not harmless: it consumes a dial slot, can never connect, and its + failure is recorded against the **IPv6** family, which is what drives the + per-family dial throttle — so the guess teaches the engine that IPv6 does not + work. BEP 32 runs the IPv6 DHT as a separate DHT, which makes the transport the + authority: the family the response arrived on now decides, and length remains + the tie-breaker only where that family's own unit does not divide the string. - A disk error while serving a BEP 52 hash request returns an error instead of raising. `Merkle.leaf_range_response_from_disk/7` documents `{:error, term()}` and handles a failed `:file.open/2` that way, but the diff --git a/lib/elixir_torrent/dht.ex b/lib/elixir_torrent/dht.ex index 531d550..2eb146b 100644 --- a/lib/elixir_torrent/dht.ex +++ b/lib/elixir_torrent/dht.ex @@ -584,7 +584,9 @@ defmodule DHT do tables = RoutingTables.mark_good(state.routing_tables, contact, from_query: true) state = %{state | routing_tables: tables} - peers = KRPC.response_peers(response) + # The family the answer arrived on disambiguates a packed `values` string — + # see `KRPC.response_peers/2`. + peers = KRPC.response_peers(response, transport_family(ip)) nodes = response @@ -1499,10 +1501,13 @@ defmodule DHT do @spec socket_for_dest(t(), :inet.ip_address()) :: port() defp socket_for_dest(state, ip) do - family = if tuple_size(ip) == 8, do: :inet6, else: :inet - select_socket(state, family) + select_socket(state, transport_family(ip)) end + @spec transport_family(:inet.ip_address()) :: :inet | :inet6 + defp transport_family(ip) when tuple_size(ip) == 8, do: :inet6 + defp transport_family(_ip), do: :inet + @spec socket_family(t(), port()) :: :inet | :inet6 defp socket_family(%__MODULE__{socket_v6: socket_v6}, socket) when socket == socket_v6, do: :inet6 diff --git a/lib/elixir_torrent/dht/krpc.ex b/lib/elixir_torrent/dht/krpc.ex index e916bb2..691bd0f 100644 --- a/lib/elixir_torrent/dht/krpc.ex +++ b/lib/elixir_torrent/dht/krpc.ex @@ -310,38 +310,59 @@ defmodule DHT.KRPC do (v4 ++ v6) |> Enum.uniq_by(& &1.id) end - @doc "Decode compact peers from a get_peers response body." - @spec response_peers(response()) :: [Peer.t()] - def response_peers(%{values: values}) do + @doc """ + Decode compact peers from a get_peers response body. + + `family` is the address family the response arrived on. It is needed because + **18 is a multiple of 6**: BEP 5 nominally puts one compact peer in each + `values` string, but implementations pack several into one, so a string carrying + three IPv4 peers is indistinguishable by length alone from a single IPv6 peer. + BEP 32 runs the IPv6 DHT as a *separate* DHT, which makes the transport the + authority — a response that came back over IPv6 carries IPv6 peers. Length stays + the tie-breaker only when the transport's own unit does not divide the string, + and for callers that do not know the family. + """ + @spec response_peers(response(), :inet | :inet6 | nil) :: [Peer.t()] + def response_peers(response, family \\ nil) + + def response_peers(%{values: values}, family) do values |> normalize_values() - |> Enum.flat_map(&decode_value_peers/1) + |> Enum.flat_map(&decode_value_peers(&1, family)) |> Enum.uniq_by(&{&1.ip, &1.port}) end - def response_peers(_), do: [] + def response_peers(_, _), do: [] @spec normalize_values(term()) :: [binary()] defp normalize_values(values) when is_binary(values), do: [values] defp normalize_values(values) when is_list(values), do: Enum.filter(values, &is_binary/1) defp normalize_values(_), do: [] - @spec decode_value_peers(binary()) :: [Peer.t()] - defp decode_value_peers(blob) when is_binary(blob) do - size = byte_size(blob) - - cond do - size == 0 -> - [] - - rem(size, @ipv6_peer_info_size) == 0 -> - Compact.decode_ipv6_peers(blob) - - rem(size, @peer_info_size) == 0 -> - Compact.decode_peers(blob) - - true -> - [] + @spec decode_value_peers(binary(), :inet | :inet6 | nil) :: [Peer.t()] + defp decode_value_peers(blob, family) when is_binary(blob) do + case value_peer_family(byte_size(blob), family) do + :inet6 -> Compact.decode_ipv6_peers(blob) + :inet -> Compact.decode_peers(blob) + nil -> [] end end + + # The transport family goes first, so a packed 18-byte string arriving over IPv4 + # is read as the three IPv4 peers it is rather than one invented IPv6 address. + # A phantom endpoint is not harmless: it consumes a dial slot, can never connect, + # and its failure is recorded against the IPv6 family — which is what drives the + # per-family dial throttle, so guessing wrong here teaches the engine that IPv6 + # does not work. + @spec value_peer_family(non_neg_integer(), :inet | :inet6 | nil) :: :inet | :inet6 | nil + defp value_peer_family(0, _family), do: nil + + defp value_peer_family(size, :inet6) when rem(size, @ipv6_peer_info_size) == 0, do: :inet6 + defp value_peer_family(size, :inet) when rem(size, @peer_info_size) == 0, do: :inet + + # Family unknown, or known but its unit does not divide the string — a v4 peer + # answered over the v6 DHT, say. Fall back to length, preferring IPv6 as before. + defp value_peer_family(size, _family) when rem(size, @ipv6_peer_info_size) == 0, do: :inet6 + defp value_peer_family(size, _family) when rem(size, @peer_info_size) == 0, do: :inet + defp value_peer_family(_size, _family), do: nil end diff --git a/test/ipv6_dial_test.exs b/test/ipv6_dial_test.exs index 662e443..681a400 100644 --- a/test/ipv6_dial_test.exs +++ b/test/ipv6_dial_test.exs @@ -107,6 +107,57 @@ defmodule IPv6DialTest do assert length(peers) == 2 assert Enum.all?(peers, fn %Peer{ip: ip} -> tuple_size(ip) == 8 end) end + + # 18 is a multiple of 6, so a `values` string packing three IPv4 peers has + # exactly the size of one IPv6 peer. Length alone cannot tell them apart; BEP 32 + # runs the IPv6 DHT as a separate DHT, so the transport the answer arrived on is + # the authority. + test "a packed 18-byte blob over IPv4 is three IPv4 peers, not one IPv6 peer" do + blob = + Compact.encode_peer({1, 2, 3, 4}, 6881) <> + Compact.encode_peer({5, 6, 7, 8}, 6882) <> + Compact.encode_peer({9, 10, 11, 12}, 6883) + + assert byte_size(blob) == 18 + + peers = KRPC.response_peers(%{values: [blob]}, :inet) + + assert length(peers) == 3 + assert Enum.all?(peers, fn %Peer{ip: ip} -> tuple_size(ip) == 4 end) + assert %Peer{ip: {9, 10, 11, 12}, port: 6883} in peers + end + + test "the same 18-byte blob over IPv6 is one IPv6 peer" do + blob = + <<0x26, 0x02, 0x00, 0x2D, 0x40, 0x00, 0x00, 0x01, 0, 0, 0, 0, 0, 0, 0, 0x42, 0x1A, 0xE1>> + + peers = KRPC.response_peers(%{values: [blob]}, :inet6) + + assert length(peers) == 1 + assert Enum.all?(peers, fn %Peer{ip: ip} -> tuple_size(ip) == 8 end) + end + + test "falls back to length when the family's own unit does not divide the blob" do + # A single IPv4 peer answered over the IPv6 DHT: 6 is not a multiple of 18, so + # the length has to decide, and it says IPv4. + blob = Compact.encode_peer({1, 2, 3, 4}, 6881) + + assert KRPC.response_peers(%{values: [blob]}, :inet6) == [ + %Peer{ip: {1, 2, 3, 4}, port: 6881} + ] + end + + test "with no family the old length-only preference for IPv6 is unchanged" do + blob = + Compact.encode_peer({1, 2, 3, 4}, 6881) <> + Compact.encode_peer({5, 6, 7, 8}, 6882) <> + Compact.encode_peer({9, 10, 11, 12}, 6883) + + peers = KRPC.response_peers(%{values: [blob]}) + + assert length(peers) == 1 + assert Enum.all?(peers, fn %Peer{ip: ip} -> tuple_size(ip) == 8 end) + end end describe "DHT.cap_lookup_peers/2" do From 60b7c473aedb5232e7a60503eda41f3d094242d8 Mon Sep 17 00:00:00 2001 From: Daniel Urumov Date: Mon, 14 Sep 2026 08:58:28 +0300 Subject: [PATCH 2/2] chore: date 0.6.7 for the day it is actually released The section was written on the 12th; the tag goes out on the 14th, and a changelog that predates its own release is a small lie that ages badly. Co-authored-by: Cursor --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index bc7ecfa..0141fd8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,6 @@ # Changelog -## 0.6.7 - 2026-09-12 +## 0.6.7 - 2026-09-14 A measurement release. Every fix here is a case where the engine was doing something wrong *and the instruments said it was fine* — a tracker retried