Torrent v2 5/8: V2 handshake, hash exchange and bounded transport - #235
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e727802a9f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| init { | ||
| require(root.size == 32) | ||
| require(baseLayer in 0..63 && proofLayers in 0..63 && baseLayer + proofLayers <= 63) | ||
| require(length in 2..512 && length and (length - 1) == 0) |
There was a problem hiding this comment.
BEP 52 permits length to be any positive power of two up to 512, including 1. A peer requesting a single remaining hash therefore causes PeerHashSelector construction to throw during decoding, closing an otherwise valid v2 connection; local scheduling also cannot issue such a request. Allow length == 1 (the existing hashCount calculation already handles that case).
Useful? React with 👍 / 👎.
e727802 to
77e62d0
Compare
Test Results 652 files 652 suites 4m 59s ⏱️ Results for commit 77e62d0. |
) Four automated review findings from PRs #232, #235, #236 and #237 were left unresolved when those branches were consolidated and merged. Metadata cache (#237, P1): pending fetches were keyed by info hash alone, so a TRACKER_ONLY caller arriving while a PUBLIC fetch was in flight joined that operation and inherited discovery it had opted out of, along with its rejection of private metadata. The privacy branch lives inside the fetch lambda, which only runs for the caller that creates the deferred, so the guard could never fire for the joining caller. Pending work is now keyed by hash and privacy. Completed entries stay shared because the hash authenticates them. Piece scheduler (#236): every HAVE frame rebuilt the peer's full availability as a BooleanArray and made the scheduler copy and rescan it, so a peer sending duplicate announcements forced repeated allocation and a full rarity scan per message. HAVE now applies incrementally; only the one-time bitfield rebuilds. Hash wire (#235): BEP 52 permits a request length of any positive power of two up to 512, but length 1 was rejected during decoding, closing otherwise valid v2 connections. The existing hashCount calculation already covers that case: a lone base hash covers no proof layer, so uncles is proofLayers + 1, which matches the invariant peerHashProofHeight enforces. Metainfo (#232): an unconditional requireNotNull rejected documents that omit "piece layers", which BEP 52 allows when no file exceeds the piece length. An absent key now parses as an empty dictionary; validatePieceLayers still rejects omission whenever the parsed files require external layers. The symlink race reported on #233 is not addressed here. Closing it needs a no-follow open plus handle-identity verification, which okio's commonMain FileSystem does not expose; validateOwned still rejects symlinks, checks file identity and revalidates after writing. The oversized-file finding on the same PR was already fixed before merge and only left unresolved on GitHub. Each fix carries a regression test. The two behavioural ones were confirmed to fail without their fix: reverting the pending key to ignore privacy fails restrictedCallersNeverJoinPendingPublicDiscovery, and removing the duplicate guard fails announce_buildsAvailabilityIncrementallyAndIgnoresRepeatedIndexes. Full suites pass on a forced rerun: 639 torrent and 203 core JVM tests, plus the iOS simulator target compiling clean.
1 participant
Negotiates full-identity peer handshakes and authenticates bounded hash exchanges. Frame/request ownership, deadlines and explicit response handling constrain transport memory and malformed-peer behavior.
Part 5/8 of the consolidated stack for #162. Review and merge bottom-up. This PR targets
torrent-v2-stack-04-recovery; its diff contains only this capability group.Preservation and validation
Consolidates #186–#194. The consolidation snapshot matches original boundary
45e5b9ecexactly. Fresh CI then exposed a listener shutdown race: canceledaccept()could escape as a socket exception. Foundation commit2fdddd39fixes cancellation classification and adds a deterministic regression test. This inherited fix and its test are the only file differences from that original boundary; all other contents are preserved. Original branches and discussions are retained.Fresh CI runs on this replacement PR; historical checks and approvals are not transferred. The remaining production roadmap and release gates are still open.
Original PRs and frozen heads
c238102b1403f581e18b978cbd9c0a873b1f9cfec6f4ddf976cb3cc17e7c32e545e5b9ecReview carry-forward
These original discussions and their responses remain linked for review. Their unresolved status is preserved; consolidation does not imply reviewer approval.
PeerHashWire.kt; latest response.PeerHashWire.kt; latest response.PeerHashTransport.kt; latest response.PeerIdentityHandshake.kt; latest response.PeerBlockExchange.kt; latest response.Consolidated stack