Decide a DHT values peer's family by transport, not by blob length - #23
Merged
Merged
Conversation
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
`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 <cursoragent@cursor.com>
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 <cursoragent@cursor.com>
daniboybye
force-pushed
the
fix/dht-values-transport-family
branch
from
September 14, 2026 05:58
12792ea to
60b7c47
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacked on #21 (whose branch now also carries the merged #22), so the diff here is one commit. Merge #21 first and this retargets to
mastercleanly.Found while investigating why two torrents had zero IPv6 candidates out of 27 and 34 known endpoints, on a host where IPv6 is the family that works (measured 23.5% dial yield against 0.9% on IPv4).
The defect
decode_value_peers/1tested the IPv6 stride before the IPv4 one:18 is a multiple of 6. A
valuesstring 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 actually meet.Why it matters more than "one bad endpoint"
A phantom endpoint consumes a dial slot and can never connect. Worse, 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 only family with a usable success rate.That is the same failure shape as the
:add_peer_failedmisclassification fixed in #21: a local mistake counted as a network verdict, feeding a control loop that then makes a worse decision.The fix
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/6has the remoteip— so it is passed down and consulted first.Length remains the tie-breaker where the 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 outside this path changes.
This is not the answer to the v6=0 question
Worth being explicit, because it would be easy to read this PR as closing that investigation. It does not: this bug invents IPv6 endpoints rather than losing them. What I established while looking:
select_socket/2correctly routes IPv6 destinations to itadded6peers6The instrument that would settle it already exists —
[dht] get_peers ipv6_peers=N total=Minapply_trusted_response/6— but it isLogger.debug, which a production release purges at compile time. That is precisely why the question has stayed open. TheELIXIR_TORRENT_DEBUG_BUILD=1switch added in the WebUI repo keeps those call sites, so the next step is a diagnostic build rather than more code reading.Verification
All 12 gates of
.github/workflows/build-and-publish.ymljobbuild, run locally in CI's order:mix testmix dialyzermix credo --allmix format --check-formattedmix compile --warnings-as-errors(dev + test)mix sobelow,mix hex.auditmix deps.get --check-locked(dev + test),mix deps.unlock --check-unusedscripts/compile-property-deps.sh+4 tests covering both readings of an 18-byte blob, the length fallback, and the unchanged no-family default. The changelog entry joins the unreleased 0.6.7 section.
Not reproducible locally, so not claimed green: the Trivy filesystem scan, the Codecov upload, and the SARIF upload to code scanning. CI does not trigger automatically on a PR whose base is not
master, so the workflow was dispatched manually against this branch.