Repository navigation
feat(pullsync): content-aware chunk checksum for divergent SOC sync (SWIP-101) - #5544
martinconic wants to merge 26 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.
…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.
| const ( | ||
| protocolName = "pullsync" | ||
| protocolVersion = "1.4.0" | ||
| protocolVersion = "2.0.0" |
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.
There was a problem hiding this comment.
We could even keep the protocol version 1.4.0, by adapting the proto message for Chunk:
message Chunk {
bytes Address = 1;
bytes BatchID = 2;
bytes StampHash = 3;
bytes Sum = 4;
}where len(Sum) == 0 can identify the legacy peer, and select the old batchID/stampHash check.
| // 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) { |
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 { |
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
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>
| storage.ErrDivergentChunkRejected, | ||
| ) | ||
| } | ||
| } else if bytes.Compare(oldStampIndex.StampHash, stampHash) < 0 { |
There was a problem hiding this comment.
remove this branch, test case and document the parent Put method
| return | ||
| } | ||
|
|
||
| func (r *Reserve) evaluateSOCDivergence( |
There was a problem hiding this comment.
please document in a nutshell what this method does
| BatchID: oldStampIndex.BatchID, | ||
| StampHash: oldStampIndex.StampHash, | ||
| entrySum := sum | ||
| if has && !incomingWins { |
There was a problem hiding this comment.
if this is true, why do we need to do anything instead of returning early here?
There was a problem hiding this comment.
We can’t return early because the incoming chunk brings a new, valid stamp (different batch or index) for this SOC address. Returning early would break arrival-order convergence:
If V1 arrives before V2: both stamps end up in the reserve, with V1 updated to V2's payload via refreshSiblingSums.
If V2 arrives before V1: returning early would drop V1's stamp completely, causing the node to diverge from nodes that received them in reverse order.
So we must still record the stamp in the reserve, but index it with storedSum (the winning payload on disk) so pullsync offers match what we actually serve.
Equal-timestamp collisions with distinct stamp hashes now take the last write, and the duplicated stamp-pick / metadata-delete paths are one helper each. Co-authored-by: Cursor <cursoragent@cursor.com>
…um backfill Four defects found by adversarial tests around the SWIP-101 chunk sum: reserve/putSOC: a single owner chunk that loses the cross-stamp tie-break still gets an entry for its stamp, but the chunkstore reference count was not bumped. Evicting the winning batch then deleted the payload from under the losing entry, leaving an undeliverable reserve entry. The losing path now Puts the stored chunk (reference count only). The stored chunk is read once and errors from the read and the sum computation are returned instead of falling back to the incoming sum. reserve/resolveSOCDivergence: an entry whose ChunkSumItem was missing or stale (interrupted migration or repair) made the reserve reject its own chunk: CACs failed with "not a single owner chunk", identical SOCs with ErrDivergentChunkRejected. Pullsync then re-wanted the entry forever, and the CAC error stalled the puller interval. Both cases now repair the sum row via restoreEntrySum instead of running the tie-break. migration/step_08: removals were batched per page, so two removals at one address both read the committed reference count and leaked the payload. Each removal now runs in its own transaction; backfills stay batched. migration/step_08: entries recorded with a zero stamp hash left their stampindex and chunkstamp rows behind, since RemoveChunkWithItem resolves the stamp by hash. The orphaned slot then rejected both a re-put of the same chunk and a newer chunk in that slot. removeEntry now deletes stamp rows of the entry's batch that no live entry references. Tests: put_cases_test.go covers every Put branch with state assertions; adversarial_test.go adds order-permutation, randomized convergence, sum-repair and concurrency tests (known divergences are reported and fail only with RESERVE_STRICT_CONVERGENCE=1); pullsync_test.go covers the divergent-rejection, malformed-sum and stale-sum paths; step_08_corner_test.go runs the migration on the real sharky+leveldb stack, which the in-memory storage cannot exercise for reference counts or batched writes.
…duplicate binIDs in step_08
|
Findings fixed in the last 8 commits |
|
Hi, here is the context behind each of those points:
|
| if offer.Chunks[i] == nil { | ||
| return 0, 0, fmt.Errorf("nil chunk at index %d in offer from peer %s", i, peer) | ||
| s.logger.Debug("syncer got nil chunk on offer", "peer_address", peer, "index", i) | ||
| continue |
There was a problem hiding this comment.
if no error is returned, maybe metrics that count skipped could be beneficial to check offer. Also, at the end of the method we could still write the error and return top most?
| func (r *Reserve) resolveStampIndexCollision( | ||
| ctx context.Context, s transaction.Store, | ||
| chunk swarm.Chunk, oldStampIndex *stampindex.Item, | ||
| sum, stampHash []byte, bin uint8, |
| s, err := soc.FromChunk(ch) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("soc from chunk: %w", err) | ||
| } |
There was a problem hiding this comment.
This is already called in the soc.Valid(ch)
| storage.ErrDivergentChunkRejected, | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
Instead of last write wins, would it be better that we have smaller hash wins, which is deterministic for all, something like this:
} else if bytes.Compare(oldStampIndex.StampHash, stampHash) < 0 {
return false, fmt.Errorf("stamp collision chunk %s lost stamp-hash tie-break: %w", chunk.Address(), storage.ErrDivergentChunkRejected)
}There was a problem hiding this comment.
I do not remember exactly why, but we decided do not compare stamp hashes. I remember there was long discussion on calls with Elad also. May be @martinconic remembers better?
| backfilled, removed := 0, 0 | ||
| lastReported := 0 | ||
| lastID := "" | ||
| seenBinIDs := make(map[uint8]map[uint64]swarm.Address) |
There was a problem hiding this comment.
If swarm address is not necesary to be print out in the log, by using struct{}, you could save over 300MB of heap memory...
…-sum # Conflicts: # pkg/storer/migration/all_steps.go # pkg/storer/migration/export_test.go
A same-slot re-stamp of the same address with an equal timestamp but a different stamp hash replaced the stored entry. Such a stamp can be derived by anyone from an existing one through secp256k1 signature malleability, which allowed rolling a single owner chunk back to an older payload and re-indexing any chunk under a fresh bin ID without owning a batch. Reject it as before; the unreachable equal-hash branch is dropped. Also: - refresh sibling sums with a context that is not canceled with the caller, since the replacing payload is already committed - propagate iteration and stamp hash errors from highestTimestampStamp instead of treating a failed read as no stored stamps - stop logging put errors before returning them
ChunkSumFromParts detected single owner chunks with soc.Valid, so every content addressed chunk paid a failed signature recovery, and storeReserveEntries classified the chunk a second time. Callers already know the type, so it is passed in. Pullsync classifies a delivered chunk once and reuses the type after stamp validation. resolveSOCDivergence now reports whether it replaced the payload, so repairing a missing sum row no longer triggers a sibling sum refresh. A CAC reserve put drops from about 105us to 41us and a new SOC put from about 360us to 130us.
When two reserve entries shared a (bin, binID) key and the survivor was backfilled in an earlier migration page, removing the duplicate deleted the survivor's ChunkBinItem and ChunkSumItem, leaving it counted in the reserve but invisible to pullsync and the sampler. deleteChunkBinItem now only removes the item if it belongs to the entry being removed.
Pin the checksum layout with vectors computed independently of the implementation: a content addressed chunk and two single owner chunks that share an address but wrap different payloads. Without them a change to the input order or the truncation passed every test, and other clients had nothing to check their sums against. Also remove the second FuzzChunkBinItemUnmarshal, which shadowed the one in items_fuzz_test.go and made fuzzing the package fail.
The pushsync checks were wrapped in an unbounded retry loop, which turned intermittent failures into passes and delayed consistent ones until the timeout. Run them once, as on master.
Two nodes backed by real reserves hold different payloads of one single owner chunk, under one shared stamp and under stamps of two batches. After syncing in both directions, in either order, both serve the winning payload, index only its sums, and a second round puts nothing. The mock reserve cannot hold divergent payloads, so until now no test covered the offered sums, the want decision and the reserve's retention rules together. goleveldb's mpoolDrain is ignored by the leak check as in the other packages that open a store.
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 a mismatch is rejected as unsolicited. Offer entries shrink from 96 to 48 bytes.When one SOC address holds several payloads, the reserve keeps one of them deterministically, whatever the arrival order:
When a SOC payload is replaced, the sums of the entries under the other stamps are refreshed, 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 in either direction, which needs a coordinated release. See the open question below.Migration
step_09): backfills sums for existing reserve entries, paginated and without loading stamps. It removes dangling, unreadable, untyped and duplicate entries, plus leftovers in the legacy format, instead of failing startup.removeRogueChunks(v2.8.3). Run on a node upgrading from v2.8.2 or older, it would decodeChunkBinItems in the pre-Sum format with the new decoder and fail.step_09re-verifies every reserve entry against the chunkstore, so it covers that step.db repair-reserverebuilds the sum index alongside the chunk bin items.Merge blocker
.github/workflows/beekeeper.ymlpinsBEEKEEPER_BRANCHtofeat/pullsync-chuns-convergence, because thesocmatrixcheck only exists there (beekeeper PR 611). The pin goes back tomasteronce that PR is merged.Open questions for reviewers
2.0.0, we could bump to1.5.0and keepBatchID = 2andStampHash = 3next to a newSum = 4. Old nodes could then still pull from new ones, as with every earlier pullsync bump. The cost is larger offers until a later release drops the old fields. (Suggested by @gacevicljubisa in review.)1.4.0are not offered again, and only divergence created after the upgrade converges. Resetting the pullsync intervals once would heal the existing network, at the cost of one full offer pass per neighbor.Known limitations
RESERVE_STRICT_CONVERGENCE=1.bee db repair-reserve.Testing
(bin, binID)across a page boundary.ci-socmatrixruns the retention rules end to end on a local cluster.