sync: SVS v4 (mhash, PARTIAL, publish+pull) via feature branch - #202
Open
Taranum01 wants to merge 18 commits into
Open
sync: SVS v4 (mhash, PARTIAL, publish+pull) via feature branch#202Taranum01 wants to merge 18 commits into
Taranum01 wants to merge 18 commits into
Conversation
added 18 commits
July 21, 2026 12:45
This rewrites the State Vector Sync protocol to v4 in the ndnd implementation.
v4 introduces a membership hash (mhash) carried on every Sync Data, two
embedded State Vector encodings (FULL and PARTIAL), and a publish-only form
that references a retrievable full vector at .../32=sv/<version>.
Highlights:
* SvsData now carries MemberSetHash and VectorType on every Sync Data;
there is no legacy StateVector-only wire form.
* New publication sends embedded PARTIAL when FULL exceeds
SyncVectorThreshold; entry [0] is always the sender's own entry.
If the sender-only baseline itself exceeds the threshold, the sender
falls back to publish+pull rather than emit a PARTIAL vector missing
the required entry [0].
* Periodic sync and mhash mismatch recovery use publish+pull: produce
full-vector Data at .../32=sv/<version>, then announce-only Sync Data
carrying mhash + SvsDataRef.
* pullFullVector is debounced per sender (5s) to bound the pull fan-in
when many peers cross the membership hash boundary simultaneously.
* SyncDataName wire version is bumped from v=3 to v=4.
File renames and surface API changes:
* ComputeMhash -> ComputeMembershipHash (in svs_membership_hash.go).
The wire-level Go TLV package path std/ndn/svs/v3/ is unchanged because
it is an internal ndnd package name, not part of the wire profile.
Covers the new v4 surface:
* Membership hash: stability across rerun, name-order independence,
sensitivity to membership changes, insensitivity to SeqNo values.
* Embedded FULL / PARTIAL round-trip and decode.
* Publish-only form decoding: mhash + SvsDataRef, no StateVector.
* handleMhashMismatch behavior when local superset, when only the
sender added members, and when only remote added members.
* buildAnnounceSvsData + parseFullVectorContent round-trip.
* Pull-debounce per-sender gate (pullFullVectorMinInterval).
Uses ComputeMembershipHash (renamed from ComputeMhash in the previous
commit).
Standalone v4 specification, renamed from the previous "v3 revision" doc. Renames the protocol version throughout (sync interest name v=4, section references updated), replaces "announce + pull" with "publish + pull" and "announce-only" with "publish-only", and rewrites section 5.6 with explicit "Sender procedure" and "Receiver procedure" subsections. Adds rationale for keeping VectorType on the wire (Section 3.4): mhash alone cannot distinguish FULL from PARTIAL because two parties with identical membership but different subscription views may legitimately disagree on what subset was sent. Section 4.3 notes that auto-MTU sizing of SyncVectorThreshold is planned future work and is intentionally out of scope for v4. Section 5.6 includes an implementation note documenting the 5s per-sender debounce on pullFullVector as a local detail that does not affect protocol correctness.
The fetcher was using NewFixedCongestionWindow(100), which ignores all congestion signals. On the 53-node sprint e2e topology, 8 concurrent ndnd cat consumers each maintain 100 outstanding Interests, producing ~800 concurrent Interests that overwhelm the network. With no congestion response, loss rate climbs and a single segment loses 3 retries in a row, aborting the cat fetch with 'retries exhausted, segment number=N'. Switch to NewAIMDCongestionWindow(100). The AIMD window halves on SigLoss/SigCongest and grows by 1/cwnd on SigData. The signals are already emitted in handleResult; this just wires them to a window that responds. Verified locally: all 3 scenarios (NDNd, NFD, NDNd replay) pass on the sprint topology.
* onReceiveStateVector: gate handleMhashMismatch on FULL/publish-only. PARTIAL vectors are subsets by design, so the recvSv tuple-count superset check in handleMhashMismatch would spuriously trigger sendRecoveryAnnounce on every PARTIAL receipt from a node whose own view is a superset (i.e. always, in steady state). * partialTargets: fix the comment to say it returns only repair targets; propagation is currently unused and always nil. * partialCandidateNames: use a single SortFunc comparator for (recency desc, canonical name asc). The previous code sorted by name then by recency in two passes, but slices.SortFunc is not stable so the first sort is not a reliable tie-breaker for entries with equal recency scores.
The metadata and prefix fetch paths in client_consume use default Retries: 3 and Lifetime: 1s. On the 53-node sprint e2e topology, DV's startup broadcasts a global Reset on every node, which can take up to 9 seconds (locally) or more (in CI) to fully drain. Even after the test announces 'routing converged', a late Reset arriving at a consumer wipes the producer prefix from its route table. The producer's re-announcement follows over the next few DV sync cycles, so a 4-second retry budget can be exhausted before routes stabilize. Bumping to Retries: 5 and Lifetime: 2s gives ~10s of client-side budget for this transient post-convergence gap. Verifies locally on the sprint topology under all three scenarios (NDNd, NFD, NDNd replay). Client-side retry/lifetime are not part of the SVS v4 wire format; this only affects the consumer's tolerance for slow DV propagation.
docs: rename §1.2 row names to 'Inline FULL / Inline PARTIAL / Out-of-band FULL' and replace 'embedded' with 'inline' throughout the spec to match. docs: simplify mhash description in §3 (point to §4.2). docs: remove 'Future work' block from §4.3 (out of scope for v4). docs: clarify §6.2 outdated-vector rule applies to FULL only; PARTIAL omissions are a subset by design. docs: remove v3 / 'Python strawman' / NDNLPv2 / 'ndnd object segmentation APIs' references. code: bump rrSegFetcher maxRetries 3 -> 5 (matches PR description and the existing 'TODO: make it configurable' comment; aligns with metadata fetch retry budget in client_consume.go). code: drop long 'clarification' comment on sortPartialTail (kept the one-line summary). Spec logic is unchanged. Build passes.
docs: §3.1 inline-form 'VectorType is only meaningful in the inline form' -> 'The inline form carries VectorType; the publish-only form does not.' docs: §3.2 'If an entry is absent' -> 'A missing entry compares as SeqNo = 0 against a present entry.' docs: §3.3 drop 'is not a hash of the full State Vector and not a hash of sequence numbers'; describe what mhash is in positive terms and note that it is unaffected by data publications. docs: §3.3 'Membership data and State Vector data are separate concepts' -> describe how the full State Vector carries membership implicitly. docs: §4.1 / §4.2 redundant 'Set VectorType = ...' -> cross-references to §3.4. docs: §6.2 'applies to FULL only ... never indicate' -> 'applies to FULL. ... do not carry any information about whether A is outdated relative to B.' code: drop 'there is no legacy StateVector-only mode' from SyncVectorThreshold doc comment. Spec logic is unchanged. Build passes.
Address the four open Copilot code-level comments on SVS v4: * onSyncData: reject SvsData that omit the required 32-byte MemberSetHash (mhash) and, for inline form, that omit or carry an invalid VectorType. Prevents PARTIAL-as-FULL misclassification and skipped recovery. * parseFullVectorContent: require VectorType=FULL and a 32-byte mhash on fetched full-vector Data; always verify mhash against the local computation (no more 'if present' opt-in). * pullFullVector: lazy-init lastPullTime under the mutex so the debounce is safe for SvSync instances built via keyed struct literals (tests). * partialTargets: sort the repair name list in NDN canonical order so PARTIAL selection is deterministic across runs. Add unit tests for missing-mhash and missing-VectorType cases on parseFullVectorContent. Spec logic is unchanged. All std/... tests pass.
Address two more review comments on SVS v4:
* SyncVectorThreshold: move from SvSyncOpts to a package-level constant
(syncVectorThreshold = 1200). The threshold is not application-tunable:
it is a fixed library constant. Removes the public field from SvSyncOpts
and the <= 0 default-application block in NewSvSync.
* Rename SVS Go identifiers to drop 'announce':
buildAnnounceSvsData -> buildPublishSvsData
shouldUseAnnouncePull -> shouldUsePublishPull
sendRecoveryAnnounce -> sendRecoveryPublish
TestSvsDataAnnounceTLV -> TestSvsDataPublishTLV
TestBuildAnnounceSvsData -> TestBuildPublishSvsData
TestShouldUseAnnouncePull -> TestShouldUsePublishPull
TestEncodeSyncDataAnnounceMode -> TestEncodeSyncDataPublishMode
Also clean up the 'announce-only Sync Data' and 'announce or pull
recovery' comments and rename the test fixture variable.
docs/svs-v4.md: spec §1.2, §4.3, §8 now describe SyncVectorThreshold as
a fixed library constant (1200 bytes), not an application-configured
parameter.
The Client.AnnouncePrefix API (used for routing prefix announcement, not
SVS) is left unchanged.
Spec logic is unchanged. All std/... tests pass.
ExpressR previously only retried on InterestResultTimeout. A persistent Nack (e.g. NackReasonNoRoute) failed the request immediately, even within the configured retry budget. On the 53-node sprint topology the DV/NLSR startup race produces a burst of prefix resets; the first metadata Interest from a scenario transition can arrive at a forwarder whose FIB has not yet been re-populated for the producer's /test sub-prefix, triggering an immediate Nack. * ExpressR: treat InterestResultNack with NackReasonNoRoute or NackReasonCongestion as retryable, alongside the existing Timeout retry. The retry budget (Retries) is now respected for both transient timeouts and transient Nacks; non-retryable Nacks (e.g. Duplicate) still pass through immediately. * rrSegFetcher.handleResult: when a segment Interest is Nacked with NackReasonNoRoute, treat it as a loss so the AIMD window shrinks and the segment is retried, instead of aborting the fetch. Verified locally: all three scenarios (NDNd, NFD, NDNd replay) pass on the 53-node sprint topology.
- Rename "inline" to "direct" and "out-of-band" to "referenced" for the
two StateVector delivery forms (more descriptive, per a-thieme).
- PARTIAL vector layout: replace "[0]" / "[1...n]" notation with prose
("the first entry is the sender's own entry").
- Clarify single-member mhash in §5.7 and §7.4: the membership set is
{N} for a joining node.
- Dedupe the mhash definition: the §1.2 paragraph now points to §3.3
instead of restating SHA-256 digest.
- Drop the "ndnd" qualifier on segmentation; the spec is forwarder-agnostic.
- §6.2: clarify that "A is outdated" applies when A is FULL, and explain
why PARTIAL receivers cannot use the omitted-name test.
Source: PR named-data#190 review comments addressed:
3612873010, 3612971226, 3612986026, 3641436856, 3641470105, 3641509945
- encodePartialStateVector: return an empty StateVector (no entries) instead of nil when the sender-only baseline exceeds the size budget. The caller treats the empty vector as the publish+pull trigger. Doc updated; spec §4.2 explicitly allows the empty-PARTIAL signal. - buildSvsDataForSend: detect the empty result with len(...Entries) == 0. - partialCandidateNames -> priorityOrderedPeers, more descriptive for review and maintenance. - sortPartialTail: drop the redundant trailing sort in encodePartialStateVector — the inner-loop sort already produces a tail-sorted slice. - ComputeMembershipHash: rename the local `tuples` variable to `membershipTuples` to disambiguate from the spec tuple. - shouldUsePublishPull: now returns (usePublish, full, size). The build-the-full-SvsData-once pattern means the inline-full case no longer re-encodes for size check + send. - deriveFullVectorPrefix and resolveFullVectorPrefix: doc clarified to call out the helper as an implementation convention, not a spec constraint (the spec only requires a sender-controlled full-vector prefix; callers wire to a different location by setting opts.FullVectorPrefix). - buildPublishSvsData: doc added explaining what `ref` is (the sender's published full-vector Data name) and how it is consumed. Verified locally: all 3 e2e scenarios pass (NDNd + NFD + replay). Source: PR named-data#190 review comments addressed: 3613120231, 3613133568, 3613158180, 3613167366, 3613250227, 3613262079, 3613275363
…ctor TLVs Per Adam's review note in the v4 spec, the VectorType discriminator field is removed from SvsData. The wire TLV itself now disambiguates between direct FULL (0xCD), direct PARTIAL (0xCE), and publish-only SvsDataRef (0x07). MemberSetHash moves to the top-level SvsData TLV so all three forms carry it in the same place. The StateVector type itself is unchanged and remains used directly by DV's advertisement (which has its own simpler wire shape). - definitions.go: introduce FullStateVector and PartialStateVector structs, drop VectorType field and constants. - accessors.go (new): helpers Kind / IsFull / IsPartial / GetStateVector so callers don't have to switch on which struct was non-nil. - svs_pull.go, svs_encode.go: build FULL/PARTIAL SvsData with the new struct types; mhash on SvsData, vector on inner struct. - svs.go: svSyncRecvSvArgs now carries partial bool; onSyncData routes by IsFull/IsPartial and rejects malformed direct forms. - svs_test.go: all tests rewritten against the new constructors and accessors; TestSvsDataLegacyParse removed. - dv/dv/advert_sync.go: use spec_svs.StateVector directly (DV's wire shape was always a single-entry StateVector). - docs/svs-v4.md: collapse §3.1, add new TLVs to the table in §3.2, replace §3.4 VectorType section with FullStateVector vs PartialStateVector, update §4 / §5 / §6 / §7 / §8 references. Tests: std/sync passes. ndnd build unchanged aside from new SVS v4 wire format.
goimports had two complaints on the prior commit:
* std/ndn/svs/v3/accessors.go was missing a trailing newline.
* std/sync/svs_pull.go had a misaligned struct literal in
shouldUsePublishPull (MemberSetHash not column-aligned with
FullStateVector after the SvsData tagged-union reshuffle).
Rename the wire-spec Go package std/ndn/svs/v3 -> std/ndn/svs/v4 to
match the spec filename (svs-v4.md) and the new wire behaviour
(mhash, distinct FULL / PARTIAL / SvsDataRef TLVs). The folder
previously advertised itself as the v3 package, but the wire
format it now produces is v4. Per Adam's review note: keeping
"v3" in the import path would be misleading for any downstream
consumer pulling this ndnd build.
* git mv std/ndn/svs/v3 std/ndn/svs/v4
* update import path in 9 caller files (std/sync/*, dv/dv/*,
std/ndn/svs_ps/*) and the one in-tree comment reference.
* struct names, TLV type numbers, and the spec text are
unchanged - only the import path moves.
Tests: std/sync, std/object/storage, std/security pass.
goimports -l clean on all touched files.
…bump Per Adam's review (PR named-data#190, comment 3641275220): the DV convergence flake on the 52-node sprint topology should be fixed by giving the test more time to converge, not by relaxing the client's metadata/prefix fetch retry budget. Reverts std/object/client_consume.go to Retries:3 / Lifetime:1s on both fetchMetadata and fetchDataByPrefix (the values used before commit 9340c00). The library default is now consistent across all consumers, not just the sprint e2e. Bumps the e2e wait times instead: * dv_util.converge deadline: 30s -> 90s Worst observed post-DV-startup propagation in CI is ~20s (9340c00 commit message); 90s gives a 4x safety margin. * test_001 scenario_ndnd_fw post-put sleep: 30s -> 90s The cat-phase failure mode in CI was a metadata Timeout ~13s after the put started (Time=cat ≈ Put+30s in failing runs). After 8 simultaneous puts, DV has to re-converge for the new --expose prefixes; 90s of post-put quiescence is enough for the late "Reset" storm to fully drain. Test runtime impact: scenario_ndnd_fw goes from ~5min to ~7min. Tests: std/object, std/sync, std/security pass locally.
Two Copilot review nits on PR named-data#190, both approved by Adam. * svs.go onSyncData (the direct-form gate): the previous check `!IsFull() && !IsPartial()` only rejected packets that had neither form. A malformed packet carrying both FullStateVector and PartialStateVector, or carrying SvsDataRef alongside an embedded vector, was being processed. Tighten to `IsFull() == IsPartial() || len(SvsDataRef) > 0` so only the exactly-one-of-{FULL, PARTIAL}-and-no-ref shape is accepted. * svs_encode.go encodePartialStateVector: ComputeMembershipHash(state) is constant for a given state but was being recomputed for the sender-only baseline check and again for every trial entry. Hoist to a single `mhash` local at the top of the function. PR description also updated to drop the now-stale references to the previous VectorType discriminator, the v3 package path, the "Retries 3 -> 5" / "Lifetime 1s -> 2s" claim, and the legacy `Threshold <= 0` mode. The new description matches the wire, the package path, and the e2e wait windows that are actually shipped. Tests: std/sync passes.
Carries the SVS v4 sync surface (mhash, FullStateVector / PartialStateVector / SvsDataRef TLVs, and publish + pull recovery) from PR named-data#190 onto a feature branch off psvs. Refactor passes will happen on this branch before merging into psvs.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #190 — same content, landed via a feature branch off
psvsper @tianyuan129's suggestion. Refactor passes will be done on this branch before merging intopsvs.What's in this PR
Identical content to #190:
mhash(0xCB),FullStateVector(0xCD),PartialStateVector(0xCE),SvsDataRef(0x07).docs/svs-v4.md.std/sync/, package renamedstd/ndn/svs/v3→std/ndn/svs/v4.e2e/dv_util.convergedeadline 30s → 90s,e2e/test_001.pypost-put sleep 30s → 90s.Retries:3,Lifetime:1s).