sync: SVS v4 (mhash, PARTIAL, publish+pull) by Taranum01 · Pull Request #190 · named-data/ndnd

Taranum01 · 2026-06-29T17:01:51Z

Summary

Implements SVS v4 (per Adam's review on PR #190 — submission as a new version since the wire breaks v3 compatibility).

Every Sync Data now carries a membership hash (mhash). The wire TLV itself disambiguates the three direct forms (FullStateVector 0xCD, PartialStateVector 0xCE) and the publish-only form (SvsDataRef 0x07). A third publish-only form (mhash + SvsDataRef) replaces the previous announce-only flow.

Event	`size ≤ SyncVectorThreshold`	`size > SyncVectorThreshold`
Publication	Embedded FULL	Embedded PARTIAL (entry [0] is sender; falls back to publish + pull if the sender-only baseline itself exceeds the threshold)
Periodic sync	Embedded FULL	Publish + pull
`mhash` mismatch	Publish + pull (if recovery needed)	Publish + pull

Wire profile: Sync Interest name uses v=4. SyncVectorThreshold is a fixed library constant (1200 bytes); there is no legacy StateVector-only wire.

Spec: docs/svs-v4.md

Main changes

TLV (std/ndn/svs/v4): MemberSetHash on SvsData; distinct FullStateVector (0xCD) and PartialStateVector (0xCE) structs replace the previous VectorType discriminator; SvsDataRef for the publish-only form; MembershipTuple for membership hashing.
Core (std/sync): ComputeMembershipHash; PARTIAL encode (sender first, canonical NDN order for the rest); publish at 32=sv/<version>; publish-only Sync Data; debounced pullFullVector (5s per sender); strict wire validation in onSyncData / onSyncInterest / parseFullVectorContent (32-byte mhash, exactly one direct form).
Tests (std/sync/svs_test.go): membership-hash stability and order independence; FULL/PARTIAL round-trip; publish-only decoding; handleMhashMismatch gating on FULL/publish-only; pull-debounce; invalid-form rejection.
Package rename: std/ndn/svs/v3 → std/ndn/svs/v4 to match the wire version.
E2E: e2e/dv_util.converge deadline 30s → 90s and e2e/test_001 post-put sleep 30s → 90s, to give DV's post-startup Reset storm enough time to drain on the 52-node sprint topology. Client library retry/lifetime stay at the original Retries: 3 / Lifetime: 1s.

Comments addressed

All Adam (a-thieme) review comments addressed in commits across this PR.
All Tianyuan (tianyuan129) review comments addressed in commits across this PR.
All Copilot review comments (legacy-mode reconciliation, partialCandidateNames sort stability, partialTargets determinism, onSyncData wire validation, parseFullVectorContent field requirement, direct-form exclusivity, mhash hoisting, etc.) addressed in commits across this PR.

Test plan

go test ./std/sync/... — passes locally
GitHub e2e (make e2e, sprint topology) — passes locally with the bumped DV wait windows

Publish the revision spec alongside the std/sync implementation in PR named-data#190. Co-authored-by: Cursor <cursoragent@cursor.com>

tianyuan129 · 2026-07-04T00:26:24Z

@@ -0,0 +1,157 @@
+package sync


Can you describe what this test is about?

Documented in 79b79ee — renamed to svs_mesh_test.go with a file-level comment. It is an in-process multi-node SVS harness (real engine + SvSync per node, test-only multicast hub) that exercises small-group sync, PARTIAL publication, and announce+pull recovery without mininet/NFD.

I don't see the necessity of this since we already have the e2e tests in CI

Taranum01 · 2026-07-04T16:54:51Z

Thanks for the review, @tianyuan129 — addressed in 79b79ee:

Removed local-only e2e topology / docker runner (topo.min.conf, e2e-local, e2e-local-docker.sh)
Removed production multicast_face; mesh integration tests use a test-only hub in svs_mesh_test.go
Removed svs_debug.go and debug send/recv logging
buildSvsDataForSend now takes a svsSendInput struct
Merged svs_announce.go into svs_pull.go + svs_encode.go

Also updated the PR description: SyncVectorThreshold=0 uses legacy StateVector-only wire (no mhash), which preserves DV prefix-table behavior and is what fixed e2e.

go test ./std/sync/... passes locally; CI should re-run on the new commit.

Taranum01 · 2026-07-04T17:10:23Z

Pushed 60474b3 — minor style polish only (no logic changes): aligned `large-sync` with other SVS examples, clarified threshold-0 comments, trimmed pull-path debug logs, renamed `svs_announce_test.go` → `svs_pull_test.go`, and fixed `TestPullRefFromSyncDataWire` to use a real Sync Data wire.

Taranum01 · 2026-07-06T08:00:37Z

@tianyuan129 Pushed ffcc890 — fixes the test/lint failure (goimports column alignment in std/sync/svs_mesh_test.go). Could you re-run CI / re-review when convenient? No logic changes; only the field alignment in meshMulticastFace was touched.

tianyuan129 · 2026-07-08T22:05:51Z

@@ -0,0 +1,89 @@
+package main


If we did not change the Sync API, I do not see the necessity of having a separate example.

tianyuan129 · 2026-07-08T22:07:44Z

+
+	// SyncVectorThreshold is the max inline SvsData size (bytes).
+	// 0 = legacy mode (StateVector-only wire, no mhash or announce+pull recovery).
+	SyncVectorThreshold int


We don't need to be backward compatiable.

tianyuan129 · 2026-07-08T22:41:06Z


 		recvSv: make(chan svSyncRecvSvArgs, 128),

+		fullVectorPrefix: deriveFullVectorPrefix(opts.SyncDataName),


The later deriveFullVectorPrefix merely replaces the last name component with another keyword, so I ams thinking about making fullVectorPrefix as another configurable in parallel to SyncDataName.

tianyuan129 · 2026-07-08T22:44:09Z

+// buildLegacySvsData is the pre-revision SVS v3 wire format used when threshold is 0.
+func buildLegacySvsData(state SvMap[uint64]) *spec_svs.SvsData {
+	sv := state.Encode(func(seq uint64) uint64 { return seq })
+	return &spec_svs.SvsData{StateVector: sv}
+}
+
+// buildInlineSvsData constructs inline SvsData (mhash + VectorType + StateVector).
+func buildInlineSvsData(state SvMap[uint64], vectorType uint64, sv *spec_svs.StateVector) *spec_svs.SvsData {
+	return &spec_svs.SvsData{
+		MemberSetHash: ComputeMhash(state),
+		VectorType:    optional.Some(vectorType),
+		StateVector:   sv,
+	}
+}
+
+// inlineSvsDataSize returns the encoded byte length of SvsData content.
+func inlineSvsDataSize(data *spec_svs.SvsData) int {
+	return len(data.Encode().Join())
+}
+
+// exceedsSyncThreshold reports whether size is over the configured inline budget.
+// threshold 0 disables the large-group size limit (legacy mode).
+func exceedsSyncThreshold(threshold, size int) bool {
+	return threshold > 0 && size > threshold
+}
+
+// buildInlineFullFromState builds inline FULL SvsData for the complete local state.
+func buildInlineFullFromState(state SvMap[uint64]) *spec_svs.SvsData {
+	sv := state.Encode(func(seq uint64) uint64 { return seq })
+	return buildInlineSvsData(state, spec_svs.VectorTypeFull, sv)
+}
+
+// inlineFullSize returns encoded inline FULL SvsData size for threshold checks.
+func inlineFullSize(state SvMap[uint64]) int {
+	return inlineSvsDataSize(buildInlineFullFromState(state))
+}


Is there a reason that we must have those tiny helper functions?

I don't know....I prefer inline them for readability.

tianyuan129 · 2026-07-08T22:49:07Z

@@ -0,0 +1,157 @@
+package sync


I don't see the necessity of this since we already have the e2e tests in CI

tianyuan129 · 2026-07-08T22:52:11Z

+// membershipTuple is one (Name, BootstrapTime) pair in the sync group.
+type membershipTuple struct {
+	name enc.Name
+	boot uint64
+}


Making this a separate TLV structure

Tuple-T Tuple-L, [Name, Boot]

so that you can use the ndnd standard TLV codec to simplify code.

Taranum01 · 2026-07-10T16:35:55Z

All review comments addressed in the latest push — please take another look when you have a moment.

Taranum01 · 2026-07-13T17:08:47Z

Tested locally: test_001.scenario_ndnd_fw now passes 3/3 on the sprint topology with 91f49f8. Three small changes — debounced pullFullVector (5s per sender), NoMetadata+TryStore on SvsALO's consumeObject, and segment fetcher maxRetries: 3 → 5 (there was already a // TODO: make it configurable).

Copilot

Pull request overview

This PR updates the SVS v3 sync implementation to support large-group operation by adding membership hashing (mhash), inline PARTIAL vectors for publication, and announce+pull recovery via published FULL vectors under .../32=sv/<version>.

Changes:

Extends SVS v3 SvsData TLV to carry MemberSetHash (mhash), VectorType, and SvsDataRef, plus adds encode/parse logic for inline FULL/PARTIAL and announce-only forms.
Adds core sync behavior for large groups: PARTIAL encoding on publication, periodic announce+pull, and mhash mismatch recovery with debounced pulls.
Adds unit tests and a revision spec documenting the updated wire format and behaviors.

Reviewed changes

Copilot reviewed 11 out of 12 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
std/sync/svs.go	Main sync loop updated to send reasoned sync messages, parse extended SvsData, and integrate `mhash`/PARTIAL + announce+pull paths.
std/sync/svs_test.go	New unit tests covering TLV encoding/parsing, PARTIAL behavior, `mhash`, and pull/recovery helpers.
std/sync/svs_pull.go	Implements publish-at-`32=sv`, announce-only sync, trusted pull validation, debounce, and recovery merge path.
std/sync/svs_mhash.go	Adds SHA-256 membership hash computation over `(Name, BootstrapTime)` tuples.
std/sync/svs_map.go	Adds `cloneSvMap` helper to snapshot state safely outside locks.
std/sync/svs_encode.go	Adds send-reason model + PARTIAL encoding and candidate selection logic.
std/sync/svs_alo_data.go	Tunes object fetch behavior to avoid metadata blocking and improve caching.
std/object/client_consume.go	Increases metadata/data fetch retry count for reliability.
std/object/client_consume_seg.go	Increases segment fetcher max retries.
std/ndn/svs/v3/zz_generated.go	Updates generated TLV codec for new SvsData fields + MembershipTuple.
std/ndn/svs/v3/definitions.go	Adds new SVS v3 TLV model fields and `VectorType` constants.
docs/svs-v3-revision.md	New/updated spec describing large-group revision wire format and procedures.

Files not reviewed (1)

std/ndn/svs/v3/zz_generated.go: Generated file

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

a-thieme

I took a look at everything except for the unit tests

note: I accidentally submitted as a change request as opposed to comment

a-thieme · 2026-07-20T08:10:39Z

+
+### 1.2 Large groups
+
+When the encoded State Vector exceeds **`SyncVectorThreshold`** (configurable application limit), nodes use three dissemination modes:


imo, threshold should be automatic based on mtu, and maybe svs has an optional parameter to disable partial sync. This could be the last task before merging the PR since it makes sense to have the threshold for now to make testing easier.

Acknowledged. Added a "Future work" note in §4.3 explicitly stating auto-MTU sizing is out of scope for v4. Keeping SyncVectorThreshold as a static, application-configured constant for now, with an optional parameter to disable PARTIAL sync if needed.

we can leave it coded as a constant in the ndnd svs library. I don't think it should be application-defined

a-thieme · 2026-07-20T08:19:56Z

+| `VectorType` | `0xCD` | `0` = FULL, `1` = PARTIAL |
+| `StateVector` | `0xC9` | See Section 3.2 |
+
+#### 3.1.2 Announce-only form


"Announce" should probably be changed to something about publishing the full vector because "announce" is usually used for prefix announcement in routing

Renamed throughout the spec: "announce + pull" → "publish + pull", "announce-only" → "publish-only". Code comments also updated; private Go identifiers (buildAnnounceSvsData, sendRecoveryAnnounce) keep their historical names to limit refactor scope, but every doc comment now uses "publish".

I suggest updating the go identifiers as well to reduce maintainer confusion

a-thieme · 2026-07-20T09:29:01Z

+// gating, a node can accumulate redundant segment-0 fetches for the same content, which
+// exhausts retry budgets under network load. We allow at most one pull per sender per
+// pullFullVectorMinInterval.
+const pullFullVectorMinInterval = 5 * time.Second


I don't think this is in the revision spec

Added to §5.6 as an "Implementation note": pullFullVector is debounced per sender (default 5s) to bound fan-in when many peers cross an mhash boundary simultaneously. Documented as a local detail that does not affect protocol correctness.

Done. Doc clarified: deriveFullVectorPrefix is an implementation convenience. The spec only requires a sender-controlled full-vector prefix; callers that wire to a different location set opts.FullVectorPrefix explicitly.

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.

Copilot

Pull request overview

Copilot reviewed 9 out of 10 changed files in this pull request and generated 2 comments.

Files not reviewed (1)

std/ndn/svs/v3/zz_generated.go: Generated file

Comments suppressed due to low confidence (1)

std/sync/svs_encode.go:195

partialCandidateNames tries to sort remaining by canonical name and then by recency, but slices.SortFunc is not stable, so the first sort does not reliably act as a tie-breaker. This makes the (recency desc, then name asc) ordering described in the comment non-deterministic for equal recency scores.

	slices.SortFunc(remaining, func(a, b enc.Name) int {
		return a.Compare(b)
	})
	slices.SortFunc(remaining, func(a, b enc.Name) int {
		return cmp.Compare(recencyScore(opts.Mtime, b), recencyScore(opts.Mtime, a))

+	localTuples, remoteTuples := membershipTupleCount(s.state), membershipTupleCount(recvSv)
+	if localTuples > remoteTuples && membershipContains(s.state, recvSv) {
+		go s.sendRecoveryAnnounce()
+		return
+	}


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.

Copilot

Pull request overview

Copilot reviewed 11 out of 12 changed files in this pull request and generated 4 comments.

Files not reviewed (1)

std/ndn/svs/v3/zz_generated.go: Generated file

Comments suppressed due to low confidence (1)

std/object/client_consume_seg.go:64

PR description mentions increasing the segment fetcher maxRetries 3 → 5, but rrSegFetcher is still initialized with maxRetries: 3. If the higher retry budget is required for the sprint e2e topology (as described), this should be updated to match the intended behavior (and kept consistent with the metadata fetch retry bump).

		window:      cong.NewAIMDCongestionWindow(100),
		outstanding: 0,
		retxQueue:   list.New(),
		txCounter:   make(map[*ConsumeState]int),
		maxRetries:  3,

Taranum01 · 2026-07-24T16:07:59Z

+			// Publish-only ref: advertise that the full vector is retrievable.
+			if params.StateVector == nil && len(params.SvsDataRef) > 0 {
+				trustPrefix := pullRefFromSyncDataWire(dataWire)
+				go s.pullFullVector(params.SvsDataRef, trustPrefix)
+				return


Addressed in aed3420 ("sync: tighten v4 wire-format validation").

Taranum01 · 2026-07-24T16:09:26Z

+	if vt, ok := params.VectorType.Get(); ok && vt != spec_svs.VectorTypeFull {
+		return nil, fmt.Errorf("full vector VectorType=%d, want FULL", vt)
+	}
+	if len(params.MemberSetHash) > 0 {
+		computed := ComputeMembershipHash(stateVectorToMap(params.StateVector))
+		if !bytes.Equal(params.MemberSetHash, computed) {
+			return nil, fmt.Errorf("full vector mhash mismatch")
+		}
+	}


Addressed in aed3420 ("sync: tighten v4 wire-format validation").

Taranum01 · 2026-07-24T16:10:38Z

+// partialTargets returns the repair target names from the suppression-merge
+// state. propagation is currently unused and always nil.
+func (s *SvSync) partialTargets() (repair, propagation []enc.Name) {
+	if !s.suppress {
+		return nil, nil
+	}
+	for name := range s.merge.Iter() {
+		repair = append(repair, name)
+	}
+	return repair, nil
 }


Addressed in aed3420 ("sync: tighten v4 wire-format validation")

a-thieme

"Y not X" or "not X, Y" are patterns only needed for exceptions to some rule X we've defined in this spec. It's fine to contradict previous specs or different perceptions, just so long as Y is well-defined.

Otherwise, I closed many of the previous comments and deferred a number of things to be github issues after this PR is merged.

Take a look at the Copilot suggestions. If they're valid, let us know when they're resolved and I can re-run the copilot review.

Thanks for all the work

a-thieme · 2026-07-23T20:58:53Z

+**MemberSetHash (`mhash`)** is always carried inside `SvsData`. It is a
+**membership hash**, not a hash of the full State Vector.


No need for clarification, just say what mhash is

Addressed in 67d93c2

a-thieme · 2026-07-23T21:02:06Z

+   `SyncVectorThreshold`, or
+3. An embedded `VectorType = FULL` State Vector is outdated per §6.2.
+
+Link-level fragmentation (NDNLPv2) is below this layer. Publishers use the


Unless ndnlpv2 was mentioned in previous specs, I don't think it's necessary here

Addressed in 67d93c2

a-thieme · 2026-07-23T21:09:34Z

+Recompute `mhash` whenever membership changes (member added, removed, or new
+bootstrap time for a name).
+
+The Python strawman hashes sorted producer names only. SVS v4 includes


What is Python strawman?

My local Python reference impl used while drafting v4 , not part of the spec. Let me clean up the thread.

a-thieme · 2026-07-23T21:18:14Z

+> **Future work:** the spec currently treats `SyncVectorThreshold` as a
+> static application-level constant. Auto-sizing it from observed MTU is a
+> planned extension and is intentionally out of scope for v4.


add this as a github issue after PR is merged instead of as part of the spec

No need for an issue, also no no need to mention in spec.

a-thieme · 2026-07-23T21:31:16Z

+
+	// SyncVectorThreshold is the max inline SvsData size (bytes).
+	// 0 = legacy mode (StateVector-only wire, no mhash or announce+pull recovery).
+	SyncVectorThreshold int


a-thieme · 2026-07-23T21:33:42Z

+//
+// [Spec §4.2] Entry [0] of a PARTIAL StateVector is the sender; remaining
+// entries are NOT ordered by membership hash like MemberSet entries are —
+// they are ordered by canonical NDN name comparison. StateVectorEntry
+// ordering is independent of mhash ordering.


clarification was only needed for me during review, not in code

Pesa · 2026-07-23T22:20:44Z

"Y not X" or "not X, Y"

LLMs love spewing this crap.

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

- 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 Co-authored-by: Cursor <cursoragent@cursor.com>

- 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 Co-authored-by: Cursor <cursoragent@cursor.com>

…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.

a-thieme · 2026-07-24T16:43:38Z

move to ndn/svs/v4

a-thieme · 2026-07-24T16:43:49Z

move to ndn/svs/v4

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.

Taranum01 · 2026-07-24T18:54:04Z

I have addressed all your comments and Copilot's comments. Kindly review whenever convenient. Thanks!! @a-thieme

Copilot

Pull request overview

Copilot reviewed 19 out of 20 changed files in this pull request and generated 3 comments.

Files not reviewed (1)

std/ndn/svs_ps/zz_generated.go: Generated file

Comments suppressed due to low confidence (2)

std/sync/svs_encode.go:125

Inside the trial loop, ComputeMembershipHash(state) is recomputed for every candidate even though membership is unchanged across trials. After precomputing mhash once (see earlier baselineData), reuse it here to avoid O(k·n log n) hashing work during PARTIAL selection.

		trialData := &spec_svs.SvsData{
			MemberSetHash:      ComputeMembershipHash(state),
			PartialStateVector: &spec_svs.PartialStateVector{StateVector: trialSv},
		}

std/object/client_consume.go:179

The PR description mentions retry bumps for reliability, but fetchDataByPrefix still uses Retries: 3. If the intent is to improve resilience under convergence/congestion, consider bumping this too (or update the PR description if the change is not intended here).

		Retries:  3,

a-thieme · 2026-07-24T22:42:57Z

+			// [Spec] Direct form must carry exactly one of FullStateVector /
+			// PartialStateVector, and a 32-byte mhash. The wire TLV replaces
+			// the previous VectorType discriminator.
+			if !params.IsFull() && !params.IsPartial() {
+				log.Warn(s, "onSyncInterest inline SvsData missing direct form")
+				return
+			}


ok suggestion

a-thieme · 2026-07-24T22:41:22Z

+	baseline := &spec_svs.StateVector{Entries: []*spec_svs.StateVectorEntry{senderEntry}}
+	baselineData := &spec_svs.SvsData{
+		MemberSetHash:      ComputeMembershipHash(state),
+		PartialStateVector: &spec_svs.PartialStateVector{StateVector: baseline},
+	}


good suggestion

a-thieme · 2026-07-24T22:42:05Z

 			Lifetime:    optional.Some(time.Millisecond * 1000),
 		},
-		Retries:  3, // TODO: configurable
+		Retries:  3,


this means the description is not correct

will update this

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.

Taranum01 · 2026-07-24T23:32:55Z

addressed co-pilot suggestions as well

tianyuan129 · 2026-08-03T08:57:21Z

LGTM. We can merge to a feature branch first then try to refactor later?

yoursunny · 2026-08-08T01:24:35Z

@@ -0,0 +1,464 @@
+# State Vector Sync (SVS) v4 Specification


Please upload the spec in named-data/StateVectorSync repository.

Taranum01 · 2026-08-11T11:32:26Z

Both pending review items addressed: spec uploaded in named-data/StateVectorSync#23; feature-branch landing path is #202

Taranum01 changed the base branch from main to psvs June 29, 2026 17:07

Taranum01 pushed a commit to Taranum01/ndnd that referenced this pull request Jun 29, 2026

docs: add SVS v3 large-group revision specification

05623e2

Publish the revision spec alongside the std/sync implementation in PR named-data#190. Co-authored-by: Cursor <cursoragent@cursor.com>

tianyuan129 requested changes Jul 4, 2026

View reviewed changes

tianyuan129 requested changes Jul 8, 2026

View reviewed changes

Taranum01 force-pushed the svs-exploration branch from ad4f144 to f0724bf Compare July 9, 2026 08:07

tianyuan129 reviewed Jul 19, 2026

View reviewed changes

Comment thread std/object/client_consume.go Outdated

a-thieme requested review from a-thieme and Copilot July 19, 2026 06:08

Copilot started reviewing on behalf of a-thieme July 20, 2026 07:51 View session

Copilot AI reviewed Jul 20, 2026

View reviewed changes

Comment thread std/sync/svs.go Outdated

Comment thread std/sync/svs_encode.go Outdated

a-thieme requested changes Jul 20, 2026

View reviewed changes

Taranum Wasu added 3 commits July 21, 2026 12:45

Taranum01 force-pushed the svs-exploration branch from 3bb17d6 to 4c7bb88 Compare July 21, 2026 07:42

Taranum01 changed the title ~~sync: SVS v3 large-group revision (mhash, PARTIAL, announce+pull)~~ sync: SVS v4 (mhash, PARTIAL, publish+pull) Jul 21, 2026

Taranum01 force-pushed the svs-exploration branch 2 times, most recently from 5ee8ec8 to 4ec986e Compare July 21, 2026 22:09

a-thieme requested a review from Copilot July 21, 2026 23:53

Copilot started reviewing on behalf of a-thieme July 21, 2026 23:54 View session

Copilot AI reviewed Jul 21, 2026

View reviewed changes

Taranum01 force-pushed the svs-exploration branch from 4ec986e to 3b8a8ba Compare July 22, 2026 09:07

Taranum Wasu added 2 commits July 22, 2026 15:10

Taranum01 force-pushed the svs-exploration branch from 3b8a8ba to 5d8b1be Compare July 22, 2026 09:45

Copilot AI reviewed Jul 23, 2026

View reviewed changes

a-thieme reviewed Jul 23, 2026

View reviewed changes

Taranum Wasu added 7 commits July 24, 2026 14:27

Taranum01 force-pushed the svs-exploration branch from 94eb66b to 5f3a477 Compare July 24, 2026 15:44

a-thieme reviewed Jul 24, 2026

View reviewed changes

Taranum Wasu added 2 commits July 24, 2026 22:34

a-thieme requested a review from Copilot July 24, 2026 21:52

Copilot started reviewing on behalf of a-thieme July 24, 2026 21:52 View session

Copilot AI reviewed Jul 24, 2026

View reviewed changes

yoursunny suggested changes Aug 8, 2026

View reviewed changes

This was referenced Aug 11, 2026

docs: add SVS v4 specification named-data/StateVectorSync#23

Open

sync: SVS v4 (mhash, PARTIAL, publish+pull) via feature branch #202

Open

Taranum01 closed this Aug 11, 2026

Taranum01 reopened this Aug 11, 2026


		recvSv: make(chan svSyncRecvSvArgs, 128),

		fullVectorPrefix: deriveFullVectorPrefix(opts.SyncDataName),


		### 1.2 Large groups

		When the encoded State Vector exceeds `SyncVectorThreshold` (configurable application limit), nodes use three dissemination modes:

		MemberSetHash (`mhash`) is always carried inside `SvsData`. It is a
		membership hash, not a hash of the full State Vector.

Uh oh!

Conversation

Taranum01 commented Jun 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Main changes

Comments addressed

Test plan

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Taranum01 commented Jul 4, 2026

Uh oh!

Taranum01 commented Jul 4, 2026

Uh oh!

Taranum01 commented Jul 6, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Taranum01 commented Jul 10, 2026

Uh oh!

Taranum01 commented Jul 13, 2026

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

a-thieme left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Taranum01 commented Jun 29, 2026 •

edited

Loading

a-thieme left a comment •

edited

Loading