feat(discovery): Phase 4 reservation voucher protocol surface (EXPERIMENTAL) - #240
Conversation
…elayURL handleAdminReserve was setting RelayURL to the relay's secp256k1/EVM address instead of the relay's API HTTPS URL (APIHTTPSAddr). The client-side voucher cache keys on APIHTTPSAddr, so vouchers issued with the wrong URL would never match a cached entry and the three-bucket partition would always demote the relay to bucket C. Fix: add relayAPIURL string parameter, plumb f.server.PortalURL() through from serveAdmin, and assert voucher.RelayURL == expectedAPIURL in the round-trip test.
handleAdminReserve received a relayAddress parameter that was never read inside the function body (the relay's secp256k1 address is not needed for voucher signing — only the private key + APIHTTPSAddr are). Remove the parameter and update both call sites and the test helper. cache_test.go's TestCacheConcurrent had a //nolint:errcheck pragma against Cache.Get, which returns (ReservationVoucher, bool) — no error; the pragma was stale from an earlier iteration of the API. Replace with a plain blank identifier discard.
![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: 0f3f7e0bd9
ℹ️ 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".
| func VerifyReservationVoucherFromDescriptor(v types.ReservationVoucher, desc types.RelayDescriptor) error { | ||
| return VerifyReservationVoucher(v, desc.Address) |
There was a problem hiding this comment.
VerifyReservationVoucherFromDescriptor only checks the recovered signer address and never verifies that v.RelayURL matches desc.APIHTTPSAddr, so a relay can sign a voucher that names a different relay URL and still pass verification. In flows that cache by RelayURL, this enables cross-relay voucher poisoning/misbinding (the voucher is cryptographically valid for the signer but semantically bound to the wrong relay endpoint).
Useful? React with 👍 / 👎.
| if stored, still := c.store[relayURL]; still && time.Now().After(stored.ExpiresAt) { | ||
| delete(c.store, relayURL) | ||
| } | ||
| c.mu.Unlock() | ||
| return types.ReservationVoucher{}, false |
There was a problem hiding this comment.
When Get sees an expired entry on the read path, it rechecks under the write lock, but it always returns (zero, false) even if another goroutine replaced that key with a fresh voucher before the lock was acquired. This produces false cache misses under concurrent refresh and can incorrectly demote reservation-capable relays despite a valid voucher being present.
Useful? React with 👍 / 👎.
1 participant
Phase 4 of the discovery rationalization
Builds on PR #239 (Phase 3 multi-hop diversity). Adds the reservation-voucher protocol surface mirroring libp2p Circuit Relay v2 semantics. Stacked PR: base =
discovery-phase-3-multihop-diversity.Plan-deferral caveat (READ THIS BEFORE ENABLING)
What landed (8 atomic commits)
5c7e9948feat(types): add ReservationVoucher type + RelayDescriptor.SupportsReservation flag—ReservationVoucher{ClientAddress, RelayURL, IssuedAt, ExpiresAt, Signature}withCanonicalBytes().RelayDescriptor.SupportsReservationis advisory (NOT inCanonicalBytes— legacy signed descriptors continue to verify).60843a99feat(auth): SignReservationVoucher + VerifyReservationVoucher— secp256k1 signing/verifying mirroring the descriptor pattern. Recoverable signatures (no out-of-band public key needed).26394ec0feat(discovery/voucher): in-memory voucher cache with expiry eviction—voucher.Cachewith sync.RWMutex-protected map;GetandHasevict expired entries on read.47b647bcfeat(discovery/voucher): selector wrapper with three-bucket partition—voucher.Voucherwraps an inner Selector. Algorithm partitions candidates into A (legacy bypass), B (cached preferred), C (uncached supports-reservation, demoted). Stable sort keeps MOLS order within each bucket.8fbfd980feat(relay-server): /admin/reserve handler with capacity budget— POST/admin/reserve(auth-gated undertypes.PathAdminPrefix); request{client_address, requested_duration_seconds}; response 200 with signed voucher, or 503 capacity-exhausted. Per-process atomic counter; default budget 100; tracks vouchers issued (not active-tunnel count).b40c897btest(loadtest): -reservation flag pre-seeds synthetic voucher cache—cmd/portal-loadtest -reservationmints a synthetic signing identity, signs all relay descriptors, pre-seeds the voucher cache, and wraps the selector chainvoucher.New(diversity.New(weighted.New(mols.New()))).6c947c75fix(relay-server): use APIHTTPSAddr (not secp256k1 addr) as voucher.RelayURL— semantic-correctness fix:voucher.RelayURLshould be the API URL clients connect to, not the relay's secp256k1 identity address. Pre-existing tests didn't catch this; new assertionvoucher.RelayURL != addressadded inadmin_test.go.0f3f7e0bchore(discovery): drop dead relayAddress param + stale nolint hint— cleanup-codebase post-review tidy: removes unusedrelayAddress stringparameter fromhandleAdminReserve(left over after feat: add WASM client web server and improve service worker #7 stopped using it) and replaces a stale//nolint:errcheckpragma incache_test.go(Cache.Get returns(T, bool), no error to check).Verification (local)
go build ./...exit 0go vet ./...exit 0make lintexit 0 (0 issues)go test -count=1 ./...exit 0 (273 tests pass across 25 packages, up from Phase 3's 243 in 24)go mod verifycleanPublic API additions
types.ReservationVouchertypes.RelayDescriptor.SupportsReservation boolauth.SignReservationVoucher/auth.VerifyReservationVouchervoucher.Cache(portal/discovery/voucher/)voucher.Voucherselector wrapper/admin/reserveHTTP routecmd/portal-loadtest -reservationNo public API breakage. All Phase 1-3 selectors and seams continue to work unchanged (
voucher.New(...)is opt-in via the SDK config, not wired by default).What this PR does NOT do (deliberate scope cap)
voucher.Voucherselector demotes uncached candidates but never calls/admin/reserveitself. A future phase would wire a background acquirer that polls the cache.Acceptance evidence
go run ./cmd/portal-loadtest -clients 100 -relays 5 -reservation(cache pre-populated; all relays SupportsReservation=true):mols+diversity+voucherThe synthetic load-test does NOT exercise
/admin/reserve(cache is pre-seeded). Production wiring would require:/admin/reservefor eachSupportsReservation=truerelay missing a fresh voucher.This is mechanism-only by design.
Atomic-commit gate
git log --oneline 5c37195d..HEADshows 35 commits across all 4 phases (22 Phase 2 + 5 Phase 3 + 8 Phase 4). Each with a single concern; no behavior+cleanup mixing. Phase 4 includes one explicitfix:commit (#7 — semantic correctness) and onechore:commit (#8 — cleanup-codebase post-review).Out of scope (entirely future)