feat(pullsync): content-aware chunk checksum for divergent SOC sync (SWIP-101) - #5544
Open
martinconic wants to merge 5 commits into
Open
feat(pullsync): content-aware chunk checksum for divergent SOC sync (SWIP-101)#5544martinconic wants to merge 5 commits into
martinconic wants to merge 5 commits into
Conversation
… (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.
martinconic
marked this pull request as draft
July 24, 2026 07:30
…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.
sbackend123
marked this pull request as ready for review
August 11, 2026 08:47
acud
reviewed
Aug 11, 2026
| const ( | ||
| protocolName = "pullsync" | ||
| protocolVersion = "1.4.0" | ||
| protocolVersion = "2.0.0" |
Contributor
There was a problem hiding this comment.
with the change from @janos we can just backward support the old version through the different semver (but we'd need to duplicate the protocol code for this). let's discuss this.
| // otherwise fail every full iteration of the namespace: the reserve sampler, | ||
| // pullsync bin subscriptions and the reserve repairer. They have no companion | ||
| // ChunkSumItem to remove. Returns the number of entries deleted. | ||
| func RemoveMalformedChunkBinItems(ctx context.Context, st transaction.Storage) (int, error) { |
Contributor
There was a problem hiding this comment.
what is this about? :) i think this would be the first appearance of a self-authored storage wipe mechanism. why do we need such a thing? and why does a migration not cover what we need it to cover?
| // rather than the caller's chunk, so concurrent replacements converge on the | ||
| // content that committed last. Entries whose sum already matches, including | ||
| // the caller's own, are left untouched. | ||
| func (r *Reserve) refreshSiblingSums(ctx context.Context, addr swarm.Address) error { |
Contributor
There was a problem hiding this comment.
iff when a chunk changes on the disk we need to bump the binIDs for the indices relating to that chunk
| if err != nil { | ||
| return err | ||
| } | ||
| if socReplaced { |
Contributor
There was a problem hiding this comment.
not 100% about this either. @sbackend123 last time i heard you were saying you're at the point of have a putSoc and putCac which i think is generally a good direction (although the review diff might be quite large).
Contributor
Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: Calin Martinconi <martinconic@gmail.com> Co-authored-by: Akrem Chabchoub <121046693+akrem-chabchoub@users.noreply.github.com> Co-authored-by: Not Darko <93942788+darkobas2@users.noreply.github.com> Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> Co-authored-by: Ljubiša Gačević <35105035+gacevicljubisa@users.noreply.github.com> Co-authored-by: Janoš Guljaš <janos@users.noreply.github.com> Co-authored-by: acud <12988138+acud@users.noreply.github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
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
3 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.
Problem
Two valid single owner chunks can share the same address, batch and stamp while wrapping different content. The pullsync want-check (
ReserveHas(addr, batchID, stampHash)) is blind to content, so a node holding one variant never requests the other — neighborhoods holding divergent SOCs can never converge, and their reserve samples produce different commitments in the redistribution game.Change (SWIP-101)
Offers now carry a 16-byte checksum per chunk instead of batch ID + stamp hash:
keccak256(batchID ‖ stampHash)[:16]keccak256(batchID ‖ stampHash ‖ wrappedCacAddress)[:16]The receiver wants a chunk iff
!Has(address, sum), answered in O(1) by a new existence index (ChunkSumItem). Sums are precomputed at write time and stored onChunkBinItem; on delivery the sum is recomputed and mismatches are rejected as unsolicited. Replacing a SOC payload refreshes the sums of co-resident entries under other stamps, keeping the index consistent with deliverable content.Breaking change
Pullsync protocol version
1.4.0→2.0.0. Old and new nodes cannot pullsync with each other; needs a coordinated release.Migration
step_08backfills sums for existing reserve entries (paginated, no stamp loads) and removes dangling or legacy-format leftovers instead of failing startup.db repair-reserverebuilds the sum index alongside the chunk bin items.Scope note
This makes divergence visible and syncable. Deterministic retention (which chunk a neighborhood converges on) is a follow-up on
feat/pullsync-soc-convergence. Divergent SOCs under different batches occupy different stamp indices and are not reconciled here.Testing
Unit tests cover the divergent-SOC sum property, sum-index lockstep with the reserve, sibling-sum refresh on both replacement paths, and the migration against genuine pre-migration records (old 106-byte serialization, dangling entries, orphans, zero stamp hash).