Repository navigation
feat: add canonical state-diff rewriter - #3
littlehand26 wants to merge 15 commits into
Conversation
Signed-off-by: lihe <he.li@debank.com>
Signed-off-by: lihe <he.li@debank.com>
Signed-off-by: lihe <he.li@debank.com>
Signed-off-by: lihe <he.li@debank.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| for key, newValue := range newValues.Values { | ||
| oldValue, ok := oldValues.Values[key] | ||
| switch { | ||
| case !ok: | ||
| stats.Added++ | ||
| case oldValue != newValue: | ||
| stats.Changed++ | ||
| } | ||
| } |
| for key := range oldValues.Values { | ||
| if _, ok := newValues.Values[key]; !ok { | ||
| stats.Removed++ | ||
| } | ||
| } |
littlehand26
left a comment
littlehand26
left a comment
There was a problem hiding this comment.
Review
Reviewed at 19b6aed4. Findings below are ordered by blast radius. Everything marked "verified" was checked against source, a real build, or production data — not inferred.
1. Blocking — height==2 parentRoot is wrong; a full plan aborts on its first processed height
cmd/statediff-rewriter/plan.go#L234-L235
if height == 2 {
return root, common.BytesToHash((storetypes.CommitInfo{}).Hash()), nil
}CommitInfo{}.Hash() special-cases the empty store set and returns sha256("") = 0xe3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855 (cosmos-sdk/store/types/commit_info.go:33-37).
The producer derives ParentHash differently: trace/api.go:183 calls BuildBlockStateDiff(parentHeader.Root, ...), and parentHeader.Root comes from rpc/types/utils.go:87 — Root: common.BytesToHash(header.AppHash), i.e. block H−1's Tendermint app_hash.
Verified: cronosmainnet_25-1's genesis app_hash is the empty string (fetched via /genesis_chunked). So block 1's header.AppHash is empty and common.BytesToHash([]byte{}) is the zero hash. The stored object for height 2 therefore carries ParentHash = 0x000…0, which can never equal 0xe3b0c442….
Result: plan.go#L197 returns root mismatch and runPlan terminates at height 2. Fail-closed, so no data is corrupted — but the plan can never complete.
Note the design doc has the same formula (§5.3.2, parentRoot(H) = H==2 ? CommitInfo{}.Hash() : …), so the doc needs the same correction: for this chain the height-2 parent root is the genesis app_hash, i.e. the zero hash.
Why no test caught it: archiveRoots takes a concrete *archiveReader, which is rocksdb-only, so the height↔CommitInfo↔S3-key mapping test required by §7.1 cannot be written at all. runPlan currently has zero test coverage. Suggest narrowing archiveRoots to a minimal interface and adding that mapping test.
Positive result from the same check: the key formula itself is correct. GET 25/4cc51d2e/0x9d565d6c3d91b141770ef3a71a2a956d1706ca030f7e35aed1b8318f7b18c98d/stateDiff (231 B) decodes to Hash = block 83908244's app_hash, ParentHash = block 83908243's app_hash, and an empty StorageDiff — confirming both the mapping and Bug B on the documented fixture block.
2. apply's independent-backup gate compares paths, not storage
cmd/statediff-rewriter/operations.go#L44-L54
The only independence test is EvalSymlinks(sourceDir) != EvalSymlinks(backupDir); proof.Kind is checked for non-emptiness only. A cp -r to a sibling directory on the same work EBS, plus a hand-written proof JSON, satisfies the gate — which is exactly the case §5.3.3 says must be rejected ("只有同机工作卷一份副本时拒绝 apply").
Directory buckets have no versioning, so the pack's OldBody is the only surviving copy of the old bytes. Suggest comparing syscall.Stat_t.Dev (the package already uses syscall.Statfs) and constraining Kind to an allowlist.
The existing negative test only covers Location == the plan directory, so the same-volume case is untested — and the passing test copies the plan into another t.TempDir() on the same filesystem.
3. Any failed PUT is reclassified as success when the target bytes are visible
cmd/statediff-rewriter/operations.go#L105-L111
Every PutObject error is funnelled through the indeterminate-PUT reclassification without inspecting it, so a definitive 412 (or an unexplained 409) is silently absorbed and counted in report.PUTs whenever the follow-up GET happens to show the target bytes. §5.3.3 requires the run to stop in that case.
Object content is still guaranteed by the post-write verification at operations.go:115-124, so this is a lost concurrency signal and an inaccurate PUT count rather than a correctness defect. Suggest branching on the error type.
4. Test gaps against §7.1
runPlanandrunVerifyhave no test coverage at all.- No equal-root pair (empty storage → skip; non-empty → hard fail).
- No chunk rollover, no cross-chunk ordinal continuity, no checkpoint resume from a non-zero frontier — i.e. the >512 MiB production path and the "apply 中断、checkpoint 恢复" case are never executed.
- No mainnet fixture (83908244 P1, or a P9 bridge-out block).
- No
plan → apply → verify → planidempotency test.
The dump-sealing tests are the strong part: 1–15 byte partial headers, declared-length overrun, bad zlib checksum, missing trailer, trailing garbage, and numeric-vs-lexicographic file ordering are all covered.
5. Build / CI
Verified by actually running things, not by reading:
- No dependency drift. Diffed against the real base
2ec3621b: go.mod / go.sum / gomod2nix.toml contain only the aws-sdk-go-v2 additions, the iavl + holiman/uint256 indirect→direct promotion (byte-identical versions), and the ethermint replace bump60189fd0e284 → e017d5ee7c8f. Zero hits for cosmos-sdk, cosmossdk.io/store, cometbft, go-ethereum, ibc-go, cronos-store. As a bonus, this also fixes a pre-existing inconsistency where basegomod2nix.tomlstill resolved ethermint tocrypto-org-chain/ethermintwhilego.modhad the Chaintable replace. - Build tags are right.
make -n build-statediff-rewriter COSMOS_BUILD_OPTIONS=rocksdbexpands to-tags "netgo objstore pebbledb mainnet ledger rocksdb grocksdb_clean_link nativebyteorder", sorocksDBBuild == true.COMMITreachesversion.Commitvia-X(confirmed with an override and in the built binary). - The image runs. Built
Dockerfile.statediff-rewriterverbatim on an amd64 host and ran it: full help, exit 0,lddshows no unresolved libraries.librocksdb.soneeds at mostGLIBC_2.34/GLIBCXX_3.4.30; ubuntu:22.04 provides 2.35 / 3.4.30. It works — but GLIBCXX has zero headroom: moving the builder base to gcc 13+ (e.g. a trixie-based golang image) will break the runtime withGLIBCXX_3.4.31 not found. - CI builds the image but never runs it (
push: false, nodocker run), and no job declaresneeds: build-statediff-rewriter; the repo has no branch protection, so it blocks nothing. Adding adocker run <image> --helpsmoke step is the highest-value change here, since it would cover both the build-tag guard and the runtime linkage. jlumbroso/free-disk-space@mainis a mutable third-party branch ref (lines 28 and 65).- Minor:
Makefile:68setsCGO_ENABLED=1as a plain make variable with noexport, so it never reaches thego buildenvironment — harmless, since cgo defaults on for native builds, but the line is a no-op. - Minor: CI stamps
GIT_COMMIT=${{ github.sha }}, which onpull_requestis the ephemeral merge commit. Harmless whilepush: false, but would record a non-existent commit if the job ever starts publishing.
6. Two premises worth revisiting
Multistream(false) is dead code. dump.go#L61-L63 — compress/zlib's reader has no Multistream method (only *gzip.Reader does), so the type assertion is statically false and the block never executes. The concatenated-stream case is in fact still caught, by the counter.n != stat.Size() check and the trailing-data Peek, so nothing is unprotected — but the code does not do what it reads as doing. Suggest deleting it or replacing it with a real check.
The cost model needs updating. Measured on this chain: every block carries an EIP-2935 storage write (0x0000F90827F1C53a10cb7A02335B175320002935) that the old S3 objects are missing — 1000/1000 over a contiguous sample. So the "语义相同不 PUT" saving that §5.3.5 leans on will save essentially nothing, and sizing should assume close to 84M PUTs and old+new bodies for nearly every object, not a small changed set.
7. Scale note (draft gates acknowledged)
The PR defers bounded concurrency, so this is context rather than a finding: plan and verify are currently fully serial S3 GETs, and each processed unit does an atomicWrite checkpoint (two fsyncs, ~7 syscalls) — runVerify's block loop does that for every height in [2,F]. At 84M heights both dominate wall-clock well before the network does, so even a pilot may not finish in the serial form. Persisting the checkpoint every N units rather than every unit would cost nothing in correctness given the resume logic is already idempotent.
Also worth noting: verify GETs each changed object twice per run — once in the pack loop for the exact-bytes check, then again in the block-height loop, which re-derives the key for every height with root != parent.
Refuted during review (raised, then disproved — recording so they don't get re-raised): missing S3 retry/backoff (the pinned SDK's default retry.NewStandard() covers it), unbounded new-body size (unreachable), sealed-plan enforcement being name-only (the manifest is written once, after all validation), pack hash TOCTOU (the write path is fail-closed on data a substituted pack does not control), fd leaks in mergeRootChunks (both functions run once per process and every error path exits), and the ethermint pin being unfetchable because it only lives on an unmerged PR branch (GitHub allows fetching by raw SHA; verified empirically).
Addendum — CI statusOf the 4 failing checks, only Two are worth more than a mechanical fix:
The other three failures are not from this PR:
|
Signed-off-by: lihe <he.li@debank.com>
Signed-off-by: lihe <he.li@debank.com>
| if err != nil { | ||
| return err | ||
| } | ||
| defer file.Close() |
| for label, diff := range map[string]dtypes.BlockStorageDiff{"old": oldDiff, "new": newDiff} { | ||
| if diff.Hash != root || diff.ParentHash != parent { | ||
| return fmt.Errorf("%s body root mismatch", label) | ||
| } | ||
| } |
| "io" | ||
| "os" | ||
| "path/filepath" | ||
| "reflect" |
| "io" | ||
| "os" | ||
| "path/filepath" | ||
| "runtime" |
| Files: len(ranges), GeneratedFiles: generatedFiles.Load(), ReusedFiles: reusedFiles.Load(), Records: records, | ||
| GeneratedRecords: generatedRecords.Load(), ReusedRecords: reusedRecords.Load(), | ||
| Concurrency: workerCount, CacheSize: options.CacheSize, ZlibLevel: options.ZlibLevel, | ||
| DurationSeconds: duration.Seconds(), HandledBlocksPerSecond: float64(records) / duration.Seconds(), |
| report.SemanticChanges = semanticChanges.Load() | ||
| report.DurationSeconds = time.Since(started).Seconds() | ||
| if report.DurationSeconds > 0 { | ||
| report.ObjectsPerSecond = float64(report.S3GETs) / report.DurationSeconds |
| go func() { | ||
| expected := firstSequence | ||
| var count uint64 | ||
| emit := func(sequence uint64, value Input) error { | ||
| if sequence != expected { | ||
| return fmt.Errorf("producer emitted sequence %d, want %d", sequence, expected) | ||
| } | ||
| select { | ||
| case tokens <- struct{}{}: | ||
| case <-pipelineCtx.Done(): | ||
| return pipelineCtx.Err() | ||
| } | ||
| select { | ||
| case jobs <- orderedTask[Input]{sequence: sequence, value: value}: | ||
| expected++ | ||
| count++ | ||
| return nil | ||
| case <-pipelineCtx.Done(): | ||
| <-tokens | ||
| return pipelineCtx.Err() | ||
| } | ||
| } | ||
| err := produce(pipelineCtx, emit) | ||
| producerDone <- producerSummary{count: count, err: err} | ||
| close(jobs) | ||
| if err != nil { | ||
| cancel() | ||
| } | ||
| }() |
| go func() { | ||
| defer workerGroup.Done() | ||
| for task := range jobs { | ||
| value, err := work(pipelineCtx, task.value) | ||
| result := orderedResult[Output]{sequence: task.sequence, value: value, err: err} | ||
| if err != nil { | ||
| // Deliver the causal error before cancellation makes peers exit. | ||
| results <- result | ||
| cancel() | ||
| return | ||
| } | ||
| select { | ||
| case results <- result: | ||
| case <-pipelineCtx.Done(): | ||
| return | ||
| } | ||
| } | ||
| }() |
| go func() { | ||
| workerGroup.Wait() | ||
| close(results) | ||
| }() |
Signed-off-by: lihe <he.li@debank.com>
Signed-off-by: lihe <he.li@debank.com>
| report.Conflicts = metrics.conflicts.Load() | ||
| report.DurationSeconds = time.Since(started).Seconds() | ||
| if report.DurationSeconds > 0 { | ||
| report.ObjectsPerSecond = float64(report.CompletedThisRun) / report.DurationSeconds |
Signed-off-by: lihe <he.li@debank.com>
Signed-off-by: lihe <he.li@debank.com>
Signed-off-by: lihe <he.li@debank.com>
| Files: len(legacyRanges), GeneratedFiles: generatedFiles.Load(), ReusedFiles: reusedFiles.Load(), Records: records, | ||
| GeneratedRecords: generatedRecords.Load(), ReusedRecords: reusedRecords.Load(), Concurrency: 1, | ||
| CacheSize: options.CacheSize, ZlibLevel: options.ZlibLevel, DurationSeconds: duration.Seconds(), | ||
| HandledBlocksPerSecond: float64(records) / duration.Seconds(), |
| GeneratedRecords: generatedRecords.Load(), ReusedRecords: reusedRecords.Load(), Concurrency: 1, | ||
| CacheSize: options.CacheSize, ZlibLevel: options.ZlibLevel, DurationSeconds: duration.Seconds(), | ||
| HandledBlocksPerSecond: float64(records) / duration.Seconds(), | ||
| GeneratedBlocksPerSecond: float64(generatedRecords.Load()) / duration.Seconds(), |
| GeneratedRecords: generatedRecords.Load(), ReusedRecords: reusedRecords.Load(), | ||
| Concurrency: workerCount, CacheSize: options.CacheSize, ZlibLevel: options.ZlibLevel, | ||
| DurationSeconds: duration.Seconds(), HandledBlocksPerSecond: float64(records) / duration.Seconds(), | ||
| GeneratedBlocksPerSecond: float64(generatedRecords.Load()) / duration.Seconds(), |
Signed-off-by: lihe <he.li@debank.com>
Signed-off-by: lihe <he.li@debank.com>
Signed-off-by: lihe <he.li@debank.com>
| go func() { | ||
| groupDone <- group.Wait() | ||
| close(results) | ||
| }() |
Signed-off-by: lihe <he.li@debank.com>
Problem
Historical Cronos
BlockStorageDiffobjects contain replay-derived storage that misses module writes. Bug B recovery needs a fail-closed replacement sourced from a frozen IAVL archive while changing onlyStorageDiff.Changes
r/o/nscans plus version-root traversal where historical roots are availabledump --stop-after-legacy; a later full dump validates and reuses every completed chunk before skipping the expensive legacy scan/sortPutObject If-Match: OldETagwith CRC32C; validate response CRC32C and ETag, with no normal pre-GET or post-GETverifyas an optional read-only diagnostic; it is not a production completion gateVerification
go test -mod=readonly -tags=objstore ./cmd/statediff-rewriter -count=1go test -mod=readonly -tags=objstore -race ./cmd/statediff-rewritergo vet -mod=readonly -tags=objstore ./cmd/statediff-rewritergo test -mod=readonly -tags=objstore ./...go mod verify1000 PUT / 0 GET, anomaly reconciliation, interruption, checkpoint, journal, and rollback testsDraft gates
This remains draft until the final-F archive pilots, real S3 scratch-prefix conditional-write test, storage sizing, sealed backup restore, and production write approval are complete. No production S3 mutation is authorized by this PR.
Known repository CI configuration failures are tracked separately: dependency review cannot run while Dependency graph is disabled, and integration tests cannot start without the Cachix credentials. Both reproduce on
mainand are unrelated to this diff.