engine/fetch/chunk: reject peer block indices that overflow signed range geometry (fixes #52)

[Coldwings]

Fix: peer/engine uint64 block indices overflow signed range geometry (#52)

What

The peer wire (GET /peer/v1/block/<chunkHex>/<blockIndex>) carries the block
index as an unrestricted uint64, but relay geometry is signed int64
arithmetic. engine.PeerSource/PeerStreamSource converted the wire value
with int64(req.Key.Block) (engine.go, stream.go ×2) and fetch.FetchBlock
derived the byte window with start := blockIndex * blockSize. Indices that
cannot be represented in signed range geometry wrapped instead of failing.

Triage (verification of the issue's claims against 84aac4e)

Authentic — every cited mechanism reproduced on the exact named commit
(84aac4e, then current main; the cited lines match):

• internal/peer/peer.go:131-146 — parseBlockPath accepts any uint64 via
  strconv.ParseUint(..., 64) with no geometry check. Confirmed.
• The three int64(req.Key.Block) conversion sites (engine.go:453,
  stream.go:108, stream.go:145). Confirmed.
• HTTPFetcher.Fetch omits the Range header when start < 0. Confirmed.

Executable repro (httptest origin counting requests, since deleted):

wire index	int64	computed start (BlockSize 4096, size 4500)	old behavior
2^63	MinInt64	wraps to 0	fetched bytes=0-4095 — wrong block served, no error
2^64-1 (MaxUint64)	-1	-4096	no Range header → whole-object GET (4500 bytes for one block)
MaxInt64	—	wraps negative	no Range header → whole-object GET
2^52	—	wraps to 0	fetched the wrong block, no error

The issue is understated: besides the full-object amplification it
describes, the wrapped-to-0 shape silently serves and caches block 0's bytes
under a garbage chunk key. On the engine path the old code also computed a
negative expected block length (blockLen returning
-9223372036854775708). Classification: implementation defect (protocol
boundary / integer overflow / transfer amplification) — within the trust model
peers are trusted, but malformed wire geometry must fail cheaply, per the #15
precedent the issue cites.

Fixes

Two layers, so range arithmetic provably cannot wrap:

1. chunk.Config.MaxBlockIndex() (new): the largest block index whose
   whole window [i*BlockSize, i*BlockSize+BlockSize-1] fits in int64,
   i.e. (MaxInt64-BlockSize+1)/BlockSize. Golden-tested against
   arbitrary-precision values.
2. Engine relay boundary: PeerSource and PeerStreamSource decline
   req.Key.Block > uint64(cfg.MaxBlockIndex()) before cache lookup,
   relay selection, size probe, or origin I/O. Declined (held=false), not
   errored: the malformed index is the requester's fault, and a 500 would
   charge this healthy relay on the requester's circuit breaker. A legitimate
   same-config peer can never send such an index (a real block of a real
   object always satisfies the bound).
3. fetch.FetchBlock arithmetic guard: rejects blockSize <= 0,
   blockIndex < 0, and blockIndex > (MaxInt64-blockSize+1)/blockSize
   before invoking the fetcher — defense in depth for any future caller that
   skips the boundary check. (fetch deliberately does not import chunk, so it
   states the same bound locally, cross-referenced in both comments.)

Docs updated per template: docs/chunk.md §3.2, docs/fetch.md §3.4,
docs/engine.md §3.5/§3.6/§4, docs/peer.md §2, plus test-list rows.

Regression tests

All verified to fail on the pre-fix code (sources temporarily reverted,
tests kept):

• internal/engine/issue52_test.go
  • TestPeerSourceRejectsMalformedBlockIndex — 2^63, 2^64-1, and the first
    window-overflowing index (576460752303423488 @ BlockSize 16): cheap
    decline, zero origin requests (not even the size probe), no cache
    pollution; legitimate control still relays. Old code: served 16 wrong
    bytes with held=true for 2^63; pulled the whole 100-byte object for
    2^64-1.
  • TestPeerStreamSourceRejectsMalformedBlockIndex — same on the
    cut-through path: nothing streamed, sizer not called, zero origin
    requests; legitimate control still streams.
• internal/fetch/issue52_test.go
  • TestFetchBlockRejectsOverflowGeometry — 7 malformed geometries × size
    known/unknown: error before the (capture) fetcher is invoked.
  • TestFetchBlockMaxInt64Window — the largest representable index reaches
    the fetcher with the exact golden window
    [9223372036854771712, 9223372036854775807]; ordinary tail clamping
    unaffected.
• internal/chunk/issue52_test.go
  • TestMaxBlockIndexGolden — the bound pinned for BlockSize
    1/3/16/4096/4MiB against independently computed values; the bound's window
    fits int64 and the next index does not.

Verification

gofmt -l internal/        # clean
go vet ./...              # clean
go test ./... -race -count=1   # all 13 packages ok
go test ./... -cover -count=1  # all ok; chunk 98.1%, fetch 91.7%, engine 86.9%

Fixes #52

[Coldwings]

@copilot Please review this fix for issue #52 (uint64 peer block indices overflowing signed range geometry).

Focus points:

1. The bound chunk.Config.MaxBlockIndex() = (MaxInt64-BlockSize+1)/BlockSize — is the window [i*BlockSize, i*BlockSize+BlockSize-1] provably non-overflowing at and below the bound, and overflowing above it (for any BlockSize >= 1)?
2. Engine relay boundary: relayableBlockIndex declines in both PeerSource and PeerStreamSource before cache lookup / size probe / origin I/O. Are there any other paths where a peer-supplied req.Key.Block reaches signed geometry (e.g. relayFromParent, hedging, blockLen) without passing the guard?
3. fetch.FetchBlock guard: division-by-zero safety, and whether rejecting blockSize <= 0 / negative index changes any legitimate existing caller (engine client path derives indices from clamped offsets).
4. Decline-vs-error semantics: a malformed index returns held=false (404) rather than 500, to avoid charging a healthy relay on the requester's circuit breaker — sound?
5. Regression tests: do internal/{engine,fetch,chunk}/issue52_test.go genuinely pin the fixed behavior (they were verified to fail on pre-fix code)?

Do NOT push commits to this branch; leave review comments only.

[Copilot]

@copilot Please review this fix for issue #52 (uint64 peer block indices overflowing signed range geometry).

Focus points:...

1. Yes. For any BlockSize >= 1, i <= (MaxInt64-BlockSize+1)/BlockSize is exactly equivalent to i*BlockSize <= MaxInt64-BlockSize+1, so both start=i*BlockSize and end=start+BlockSize-1 stay within int64. For i = bound+1, start > MaxInt64-BlockSize+1, so end > MaxInt64 and the window is not representable.

2. I re-checked all req.Key.Block uses in internal/engine: peer-supplied values enter through PeerSource and PeerStreamSource, and both guard first (relayableBlockIndex) before any cache lookup, size probe, relay, block(...), fetch.FetchBlock(...), or blockLen(...). relayFromParent also only runs after that guard.

3. FetchBlock guard is division-safe because blockSize <= 0 is checked before using the divisor. Existing legitimate callers are unchanged: production callsite is engine.block(...) with validated chunk.Config (BlockSize > 0) and non-negative block indices derived from clamped geometry.

4. Decline-vs-error is sound here. Returning held=false maps to 404 (request issue) and avoids penalizing a healthy relay via the requester-side breaker for malformed peer input.

5. Yes — these regressions pin the fix behavior and boundary values:

• internal/chunk/issue52_test.go validates the bound with independent golden values and next-index non-fit.
• internal/fetch/issue52_test.go proves malformed geometry errors before any fetcher call, and pins the max valid window.
• internal/engine/issue52_test.go proves malformed peer indices are cheap-declined with zero origin calls on both buffered and streaming peer paths, with legit control cases still working.

I also reran the targeted tests locally (internal/chunk, internal/fetch, and internal/engine issue52 cases), all passing.