feat: pullsync soc convergence refactoring - #5550
Draft
martinconic wants to merge 19 commits into
Draft
Conversation
Two valid single owner chunks can share an address, batch and stamp while wrapping different content. The stamp signs the chunk address, so both carry an identical stamp and produce an identical stamp hash. The content-blind existence check at the top of reserve.Put therefore treated the second chunk as already stored and dropped it, leaving each node holding whichever chunk reached it first. Neighborhoods never converged, and the reserve sampler computed different commitments from the same address. Make the check content-aware via the pullsync sum, and settle the divergence here rather than in the protocol: the chunk wrapping the lower CAC address wins. The rule depends only on the two payloads, so every node reaches the same answer regardless of arrival order. On a win the chunk is replaced in place, reusing the stamp index and stamp entries, which are identical for both. The bin ID is bumped so peers that already synced past the old one are offered the replacement. The reserve size is unchanged: one chunk goes in, one comes out. Pullsync treats a lost tie-break as an expected outcome rather than a sync error, since the node already holds the chunk the neighborhood converges on. Divergent chunks under different batches are not covered: they occupy different stamp indices, so no tie-break fires.
… (SWIP-101) Review fixes for the pullsync chunk checksum change, addressing upgrade safety, repair integration and sum index consistency. The critical fix concerns deleteChunkBinItem: it read the stored record before deleting it, but during the Sum backfill migration and the sharky recovery that runs before it, on-disk ChunkBinItems still carry the pre-Sum serialization and fail to unmarshal. The migration's own removal branch for dangling entries then failed the whole migration, leaving the node unable to start, with the repair command blocked behind the same migration. An undecodable record now gets a key-only delete; it predates the sum index, so no companion entry exists. Replacing a single owner chunk's payload now refreshes the divergence checksums of co-resident entries under other stamps. The payload is stored once per address while index entries exist per stamp, so a replacement used to leave sibling sums advertising content the node no longer holds. The refresh runs after the put transaction under each sibling's batch lock, recomputing from the committed payload so concurrent replacements converge. Migration and repair hardening: step_08 pages through the reserve index in fixed windows instead of loading it whole, removes legacy entries with an unset stamp hash, derives sums from the batch ID and stamp hash already on the index item so stamps are never loaded, and sweeps orphaned pre-Sum ChunkBinItems by raw key so no old-format record survives to break later iterations. The reserve repairer now rebuilds the sum index alongside the chunk bin items, sweeps stale sum entries, and deletes chunk bin items key-only so undecodable values cannot wedge it. Also: ChunkBinItem.Marshal validates the sum length, received offers are rejected on a malformed sum length after the zero-address skip, and pullsync.pb.go is properly regenerated with gogo/protobuf v1.3.2 so the embedded descriptor matches the proto definition. The migration is covered by a new test seeding genuine pre-Sum records, including the dangling-entry case, and the sibling refresh by a test covering both replacement paths; both fail against the unfixed code.
…variant (SWIP-101) Extend the chunk checksum test coverage to the properties that phase 1 guarantees on its own, independent of the follow-up retention work. An end-to-end pullsync test drives two syncers over a recorded stream with two single owner chunks sharing an address, batch and stamp while wrapping different content: the divergent chunk must be wanted and delivered, while identical content must not be requested. The former is precisely the case the content-blind want-check used to skip. Fuzz targets cover the surfaces that parse untrusted or hand-encoded bytes: ChunkSum, which pullsync recomputes on delivered chunks before their validity is checked, the ChunkBinItem codec, and the raw chunkBin key parser. Writing the key parser round-trip property surfaced that bin values at or above swarm.MaxBins would not survive the rune-encoded ID construction, so ParseChunkBinID now rejects them instead of misinterpreting malformed keys. A randomized operation test (overlapping SOC puts across batches with timestamp replacements, CAC puts and batch evictions) repeatedly checks the invariant the want-decision depends on: the chunk sum index is exactly the set of live (address, sum) pairs, and every stored sum matches the payload currently held in the chunkstore.
Two valid single owner chunks can share an address, batch and stamp while wrapping different content. The stamp signs the chunk address, so both carry an identical stamp and produce an identical stamp hash. The content-blind existence check at the top of reserve.Put therefore treated the second chunk as already stored and dropped it, leaving each node holding whichever chunk reached it first. Neighborhoods never converged, and the reserve sampler computed different commitments from the same address. Make the check content-aware via the pullsync sum, and settle the divergence here rather than in the protocol: the chunk wrapping the lower CAC address wins. The rule depends only on the two payloads, so every node reaches the same answer regardless of arrival order. On a win the chunk is replaced in place, reusing the stamp index and stamp entries, which are identical for both. The bin ID is bumped so peers that already synced past the old one are offered the replacement. The reserve size is unchanged: one chunk goes in, one comes out. Pullsync treats a lost tie-break as an expected outcome rather than a sync error, since the node already holds the chunk the neighborhood converges on. Divergent chunks under different batches are not covered: they occupy different stamp indices, so no tie-break fires.
…nto feat/pullsync-soc-convergence # Conflicts: # pkg/storage/storage_test.go # pkg/storer/internal/reserve/reserve.go # pkg/storer/internal/reserve/reserve_test.go
…SWIP-101) Different nodes receive the same chunks in different orders, so for any set of conflicting chunks every arrival order must leave the reserve in the same final state; an order-dependent outcome means neighborhoods that can never agree. The harness drives conflict sets through every permutation against a fresh reserve and compares canonical state fingerprints (bin IDs excluded: they are order-dependent by design), asserting the sum index invariants on every run as a side effect. Converging on this branch: the equal-timestamp CAC tie-break, the identical-stamp divergent SOC resolution and timestamp ordering. Five constellations are order-dependent, all falling through the same gap: at equal stamp timestamps the tie-break fires only for a content addressed incoming chunk with a different address, and every other case drops into an unconditional replace. These are marked unresolved in the case table: they log the divergence without failing the suite, and setting RESERVE_STRICT_CONVERGENCE=1 turns them into failures, which gives the reserve.Put refactor a concrete target. A case marked unresolved that starts converging fails loudly so the marker is removed and the table stays honest. Testing methodology and findings are documented alongside the refactor notes for reuse.
…nto feat/pullsync-soc-convergence
…and SOC convergence
martinconic
changed the title
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
2 participants
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.
Checklist
Description
Open API Spec Version Changes (if applicable)
Motivation and Context (Optional)
Related Issue (Optional)
Screenshots (if appropriate):
AI Disclosure