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: c238102bb8
ℹ️ 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 = 1 because one is a power of two, and this range is necessary for fetching an unpaired tail hash in layers with an odd number of entries. Rejecting it here means such valid requests cannot be encoded or decoded, preventing the complete hash layer from being exchanged.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
The current BEP 52 explicitly requires length to be at least two as well as a power of two (peer messages, lines 401–404): https://github.com/bittorrent/bittorrent.org/blob/master/beps/bep_0052.rst#L401-L404 . An odd tail is requested as an aligned range including canonical padding, which the proof verifier checks. Retaining the minimum of two; accepting one would contradict this protocol requirement.
| } | ||
|
|
||
| // The first log2(length)-1 proof layers are counted but omitted from the response. | ||
| val hashCount: Int get() = length + maxOf(0, proofLayers - length.countTrailingZeroBits() + 1) |
There was a problem hiding this comment.
The requested hashes derive log2(length) proof levels, not log2(length) - 1. For example, a request with length = 8 and proofLayers = 5 carries 8 base hashes plus only 2 proof hashes, but this calculation requires 11 hashes instead of 10; consequently valid peer responses are rejected and encoded responses contain an extra hash that other BEP 52 implementations will not expect.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
The current BEP 52 explicitly omits log2(length)-1 proof layers, while still counting them in proofLayers: https://github.com/bittorrent/bittorrent.org/blob/master/beps/bep_0052.rst#L422-L428 . The libtorrent RC_2_0 receive path also subtracts (merkle_num_layers(merkle_num_leafs(count)) - 1) when computing proof hashes: https://github.com/arvidn/libtorrent/blob/RC_2_0/src/bt_peer_connection.cpp#L1197-L1201 . Thus length=8/proofLayers=5 carries eight base hashes and three uncles. Retaining the existing calculation and its exact-size test.
|
Superseded by #235 (V2 handshake, hash exchange and bounded transport) in the consolidated eight-PR stack for #162. The replacement preserves this PR's code and subsequent fixes; its description links the original head and review discussions. The only additional consolidation fix is the CI-discovered listener cancellation regression, documented on the replacement. All eight replacements have passed their required CI checks. Closing without merging. This branch and its discussions remain available for reference. Review index: #162 (comment) |
1 participant
Adds bounded BEP 52 hash-request, hashes and hash-reject wire messages. The codec preserves full file roots and unsigned 32-bit indices, validates aligned power-of-two ranges, and calculates exact response sizes with the protocol's omitted proof layers. It adapts existing bounded unknown frames without enabling v2 semantics in the v1 runtime.
Stacked on #185; advances step 08 of #162. Requests are capped at BEP 52's recommended 512 hashes. Authenticated tree bounds, request correlation, resource admission, proof authentication, serving and live connection negotiation remain pending; decoded hashes do not authorize progress.
Validation: three focused JVM tests pass, covering exact big-endian wire vectors, unsigned index preservation, proof-layer omission, malformed coordinates and response sizes. Full torrent iOS simulator test-source compilation passes. This codec slice does not claim executed v2 interoperability.