Repository navigation
Conversation
Quest-Claim: b510eb5eacc2c9fc
… claim ends in a mixed ring Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed head: 3cbf67c
No actionable findings. Preserving the learned chain across lite-07, negotiating its trailing wire fields, and rejecting reflected or over-limit translated claims addresses the lost-path-information cause of mixed-version withdrawal loops. I traced initial announcements, chain-only updates, restarts, and withdrawal handling through the surrounding routing code and reviewed the new mesh regressions. No code changes suggested from this review.
Verification limits: GitHub-only static review; I did not run builds, tests, or quest tooling, or independently reproduce the reported seed runs. The current head's Check run is still in progress. PR state, full head SHA, and existing reviews were rechecked immediately before submission; no earlier GitHub reviews were present.
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
- Chain on the claim —
route::node::Claimgainschain: Hops;OwnClaims::applyseeds it from a route learned over an older wire, androuted::serveextends it with the origin node's hop when translating for lite-06. - Negotiated wire extension — SETUP parameter
0x9is offered by every lite-07 session;ANNOUNCE_START/UPDATE/RESTARTcarry a trailing Lite-06 Chain only to a peer that offered it, and only when non-empty, with an explicitly empty chain rejected. - Publisher side —
routed::Answerwaits for the peer's SETUP before its first claim, gates the chain on the offer, and emits anANNOUNCE_UPDATEwhen only the chain changes. - Reflection and bounds —
Subscriber::names_selfunifies the lite-06 hop and route-identity checks; lite-07 now drops a served claim whose translated chain names it or exceedsmax_hops. - Docs, draft, quests — the draft defines the parameter, the three trailing fields, the receive rule, and a changelog line; the relay cluster doc describes the translation;
quest/m1/mixed-version-ring.mdis deleted with its references removed or updated. - Tests — three mixed-version withdrawal tests cover the alternating ring, a rolling upgrade publishing at the lite-06 relay, and one publishing at a lite-07 relay.
I traced the chain end to end: a lite-06-learned route keeps its hop list through the route node, across lite-07 announce streams, and back out to lite-06 with the origin hop appended, so the originating relay now sees its route-identity hop and drops the claim. Chain changes propagate because Served equality includes the chain, and the trailing field never reaches a peer that did not offer 0x9, keeping previously released lite-07-wip peers and the JS port (which leaves the parameter unsent) decodable.
deepseek-v4.1-flash-1017 (free via Pullfrog for OSS) | 𝕏
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 16 minutes. View limit details
No actionable comments were generated in the recent review. WalkthroughThe change defines the Lite-06 Chain SETUP parameter and optional chain fields on Lite-07 announcements. Rust negotiates chain support, sends chains to peers that offer it, and retains received chains in routed claims. Relay code checks chains when serving claims. Integration tests cover withdrawals in mixed-version rings and rolling-upgrade topologies. Protocol and planning documents are also updated. Severity of issue fixed: Medium Merge Risk:
|
…n-ring # Conflicts: # quest/m1/mixed-version-ring.md # rs/moq-net/src/lite/publisher/routed.rs # rs/moq-net/src/lite/subscriber.rs # rs/moq-net/src/lite/subscriber/routed.rs
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed head: 5ee5a84
No actionable findings in the changes since my review of 3cbf67c, comparing each revision against its merge base to exclude imported main changes. The ranking correction in rs/moq-net/src/model/origin.rs:797–818 restores each lite-07 claim's one-identified-hop ordering without discarding the compatibility chain needed for reflection checks and translation. I checked the selection callers, regression test, and integration with the merged identity/draining changes. The direction is sound; no further code changes suggested.
Verification limits: GitHub-only static review; no builds, tests, interop runs, or quest tooling executed. Check is still in progress for this head. Open/non-draft state, full head SHA, and existing reviews were rechecked immediately before submission; no review of this head was already published.
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
- Merge integration — merged
mainand refit the chain work onto #8293's draining and route-ranking refactor;routed::servenow useswith_draininginstead of a localCost::DRAIN, and the route-hop reflection check is folded intonames_self. - Fuzz constructor —
fuzz::RouteNode::claimnow builds its claim with an emptychain, keeping the fuzz/bench path compiling against the newClaimfield. - Route ranking —
route_ordertreats any entry withnode: Some(_)as one identified hop, so the lite-06 chain a lite-07 claim carries no longer makes it anonymous or hop-length-ranked; a regression test covers the carried-chain case.
with_node is only set by the lite-07 receive path, which produced a single identified hop before this PR, so the ranking change restores the pre-chain behavior for every other route. The test gates the bug: without the change the carried claim ranks anonymous and loses before cost.
deepseek-v4.1-flash-1017 (free via Pullfrog for OSS) | 𝕏
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed head: 966a128
No actionable findings. Comparing with the previously reviewed 5ee5a84 and accounting for the main merge isolates the added empty chain in rs/moq-net/src/fuzz.rs:641. RouteRelay::claim creates native lite-07 claims for the benchmark fixture, so an empty compatibility chain is correct, matching the existing RouteNode fixture. The integration change is sound; no further code changes suggested.
Verification limits: GitHub-only static review; no builds, tests, interop runs, or quest tooling executed. Check remains in progress for this head. Open/non-draft state, full head SHA, and reviews were rechecked immediately before submission; no review of this head was already published.

Completes quest/m1/mixed-version-ring.md and deletes it.
Problem
After its publisher leaves, a broadcast never ends in a relay mesh that mixes moq-lite-06 and moq-lite-07 links. This includes a rolling upgrade, where one lite-06 relay links to three or more lite-07 relays. Subscribers see a Restart on every lap.
A route learned over lite-06 becomes the translating relay's own lite-07 claim, and its lite-06 hop chain was dropped there. When a lite-07 route was translated back out to lite-06, it carried only its origin node's hop. Once a claim crossed two lite-06/lite-07 boundaries, no relay ever saw its own hop again. In A-07-B-06-C-07-D-06-A, B and D kept re-originating each other's copy.
Approach
As the quest settled: a crate-private chain, carried through lite-07 by a temporary compatibility extension.
route::node::Claimgainschain: Hops. A claim the origin learned over an older wire keeps the hop chain it arrived with (OwnClaims::apply). A native claim's chain is empty. Thelearnedflag stays separate, because moq-transport still needs it after lite-08 deletes the chain.routed::Answer) waits for the peer's SETUP before its first claim. It sends an ANNOUNCE_UPDATE when only the chain changes.Subscriber::names_selfcheck, against either this node's lite-06 hop or its route identity's hop. Lite-06 applies it inchain(). Lite-07 applies it when it serves a claim, and also drops a claim whose translated chain would exceedmax_hops. That makesmax_hopsbound the whole path across both versions, and the old route-hop check becomes one case ofnames_self.Tests
New in
rs/moq-net/tests/integration/mesh_withdraw.rs, next tomixed_version_ring_withdraw_retracts. Each test announces at one relay, checks that every relay sees it start, withdraws it, and checks that every relay sees it end:alternating_version_ring_withdraw_retracts: A-07-B-06-C-07-D-06-A.rolling_upgrade_withdraw_retracts: the publisher is on the lite-06 relay, which links to each relay of a three-relay lite-07 full mesh.rolling_upgrade_from_lite07_withdraw_retracts: the same topology with the publisher on a lite-07 relay.Route identities are random, and selection hashes them with the path. So the helper publishes eight paths, each with its own tie-breaks, which makes the lite-07-publisher case a reliable gate (see Checks).
Impact
max_hops.Alternatives
The quest's decision rejected a cost or hop ceiling with no wire change, never forwarding a lite-06-learned claim to another lite-07 relay, and documenting mixed rings as unsupported. The choices below were made here:
learnedinto!chain.is_empty(): rejected, becauselearnedmust outlive the chain for moq-transport routes.Choices made without asking
Follow-ups
None. Merging main brought in moq-dev/moq#8293. Its route-hop reflection check in
chain()is now one case ofnames_self. Itswith_drainingreplaces the drain cost insubscriber/routed.rs.Ranking
A lite-07 claim's route now carries its lite-06 chain in its hops. So
route_orderranks an entry with a node as one identified hop, which is how every lite-07 claim ranked before. Without that, an unknown hop in the chain would lose to a costlier native claim at any cost, and a longer chain would lose a cost tie before the rendezvous hash. The test isa_lite07_claims_carried_chain_does_not_rank_it.Caused by
Checks
quest gate just checkandquest gate just test interop --allpass on 5ee5a84 after merging main.quest gate just checkpasses again on 966a128, which merges main at 208c23d and gives the fuzzClaimthat main added an empty chain, which CI'smoq-net/fuzzbuild needed. In one earlier run, the 9finish cpp -> *cases all failed together at a load average above 30. They passed when rerun on their own, and again in the full run. Its skips are the existing JS lite-07 quarantine that routes/js.md tracks.routed::Answer,alternating_version_ring_withdraw_retractsandrolling_upgrade_withdraw_retractshang until timeout, androlling_upgrade_from_lite07_withdraw_retractshangs under all 12 ofMOQ_SIM_SEED1 to 12. With the fix, all three pass under all 12 seeds.origin.rs's "announce called with a looping hop chain" debug assertion.Models
Size: L
Implementer: Claude Code, claude-opus-5-5, high effort
Implementer: Claude Code, claude-opus-5-5, low effort (iterate, S)
(Written by Claude Opus 5.5 with high)
Review
Rewritten by
quest reviewafter each round.01a1c1c5-e158-7ae2-9e63-4beaa13e43a8, round 3The only new branch change initializes native fuzz claims with an empty compatibility chain as intended; no new findings arose from code review, and builds and tests were not run.