cmd/integration: add history duplicates to find redundant history values - #22450
cmd/integration: add history duplicates to find redundant history values#22450sudeepdino008 wants to merge 9 commits into
history duplicates to find redundant history values#22450Conversation
…alues Adds an `integration history duplicates` subcommand that scans domain history for consecutive entries that repeat the previous value for the same key — redundant rows an as-of read collapses away. By default it scans every domain present in the datadir (so commitment and rcache are included only when their history was enabled), or a single domain via --domain. For each domain it reports total entries, distinct keys, keys carrying at least one duplicate, the duplicate-pair count with its share of entries, and a few example keys. The consecutive-equal counting is factored into a small pure type (histDupScan) with unit-test coverage.
There was a problem hiding this comment.
Pull request overview
Adds a new integration history duplicates diagnostic command to scan per-domain history for consecutive duplicate values per key (redundant history rows), with a small pure scanner type (histDupScan) and unit tests.
Changes:
- Add
history duplicatessubcommand with--from/--to/--domain/--samplesflags and per-domain summary output. - Implement
histDupScanto count duplicate pairs, keys with duplicates, and collect sample keys. - Add
TestHistDupScancovering mixed/no-duplicate/empty/sample-limit cases.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| cmd/integration/commands/state_history.go | Adds duplicate-history scanner, domain iteration, and history duplicates CLI command. |
| cmd/integration/commands/state_history_test.go | Adds unit tests for the new histDupScan logic. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
AskAlexSharov
left a comment
There was a problem hiding this comment.
Correctness notes only (skipping style/altitude). The first one invalidates the numbers in the PR description, the rest make a failed scan look like a clean datadir.
One more that can't be anchored inline because the line is unchanged: openHistory at cmd/integration/commands/state_history.go:101 calls history.Scan(ctx, ...) and drops the returned error. Scan can fail in scanDirs, openFolder or GetStateIndicesSalt (missing salt file, unreadable snapshot dir). When it does, you get a History with an empty visible-file set, HistoryDump yields nothing, and duplicates prints no history entries (disabled or empty) for that domain plus the clean summary. errcheck won't catch it — .golangci.yml excludes errcheck's not checked message.
|
|
||
| func (s *histDupScan) observe(key, val []byte) { | ||
| s.Entries++ | ||
| if s.havePrev && bytes.Equal(key, s.prevKey) { |
There was a problem hiding this comment.
The scan assumes HistoryDump streams entries grouped by key, but it doesn't. HistoryDump (db/state/history.go:1463) loops over .ef files first and keys second:
for _, item := range ht.iit.files { // outer: per file
... // inner: per key within that file
}So on any datadir with more than one history file per domain — i.e. every real one — the same key reappears once per file, with millions of other keys in between.
Two consequences:
- False negatives. A genuine redundant pair whose two entries straddle a file boundary (last txNum of
accounts.0-1024.ef, first txNum ofaccounts.1024-2048.ef) never hitsbytes.Equal(key, s.prevKey), so it is not counted. - Inflated counts.
DistinctKeyscounts key runs, not distinct keys — a key present in 50 files bumps it 50 times.curDupalso resets per run, so the same key can be pushed intoSampleKeysand counted inKeysWithDuponce per file.
The keysWithDup=0 for accounts/storage/code/commitment in the PR description is exactly what this false negative looks like. rcache reports non-zero only because it has a single key, so nothing ever intervenes.
Making this order-independent needs either a per-key merge across files, or sorting/accumulating outside the dump callback.
There was a problem hiding this comment.
Fixed. Entries now go through an ETL sort on key||txNum before the duplicate scan, so "the previous entry for this key" means the previous one chain-wide rather than within one .ef file. HistoryDump still walks files outer, but the callback only collects; the scan runs in Collector.Load order.
That fixes both consequences: a pair straddling a file boundary is counted, and DistinctKeys/KeysWithDup count keys rather than per-file runs. The keysWithDup=0 numbers in the description need re-running.
Pinned by TestHistDupSorter_FileOrderIndependent, which feeds key A at tx 1 and 3 with key B at tx 2 and 4 in between — i.e. A split across two files with the duplicate straddling the boundary — and asserts distinctKeys=2, dupPairs=1. On the old order that read distinctKeys=4, dupPairs=0. The test also asserts a shuffled feed produces an identical report.
ETL is used as a sort scratch-pad (Load(nil, "")), which also keeps an empty value — a deletion marker — from being suppressed; TestHistDupSorter_KeepsEmptyValues covers that.
| } | ||
|
|
||
| scan := &histDupScan{sampleLimit: dupSamples} | ||
| if err := roTx.HistoryDump( |
There was a problem hiding this comment.
HistoryDump only reads frozen .ef/.v files — db/state/history.go:1455 short-circuits with if len(ht.iit.files) == 0 { return nil } and the body iterates ht.iit.files only. Nothing touches TblAccountHistoryVals / TblCommitmentHistoryVals, so history still in MDBX is never scanned.
On a dev/chiado datadir, or any node whose aggregator hasn't built files yet, every domain hits scan.Entries == 0, prints no history entries (disabled or empty) — which is a wrong diagnosis, the history exists — and the run ends with no consecutive duplicate history values found, exit 0.
There was a problem hiding this comment.
Partly fixed — the wrong diagnosis and the false clean verdict are gone; the DB-resident scan itself is not implemented.
scanDomainDuplicates now checks len(roTx.Files()) == 0 and returns errHistoryNotInFiles. That domain prints history not collated into files yet, so it was not scanned instead of no history entries (disabled or empty), and the run ends with scan incomplete: [...] have history only in the DB and a non-zero exit rather than the clean line.
Actually scanning TblAccountHistoryVals / TblCommitmentHistoryVals and merging that with the file entries is a feature rather than a fix, so I left it out — say the word and I will add it, but it needs an RwDB open and a merge against the file stream, which is a different shape of change from the rest of this.
| fromTxNum, | ||
| toTxNum, | ||
| nil, | ||
| func(key []byte, _ uint64, val []byte) { scan.observe(key, val) }, |
There was a problem hiding this comment.
The callback drops txNum, but HistoryDump filters --from/--to only at whole-file granularity (db/state/history.go:1464-1469: it skips a file when item.endTxNum <= fromTxNum and breaks when item.startTxNum >= toTxNum, then dumps every entry of a partially overlapping file).
With stepSize=1562500 and a merged file covering steps 0-1024, --from=1000 --to=1001 reports entries and dupPairs for the whole 0-1024 range, and counts duplicate pairs across the requested boundary. The txNum needed to filter exactly is already handed to the callback here.
There was a problem hiding this comment.
Fixed. The callback now filters on the txNum it is handed (txNum < fromTxNum || txNum >= toTxNum), so a partially overlapping file no longer contributes entries or duplicate pairs outside the requested range. The int bounds passed to HistoryDump are kept as a coarse per-file pre-filter.
| } | ||
|
|
||
| func scanDomainDuplicates(ctx context.Context, dirs datadir.Dirs, name string, logger log.Logger) (*histDupScan, error) { | ||
| history, settings, err := openHistory(ctx, dirs, name, toStep, logger) |
There was a problem hiding this comment.
toStep defaults to 1e18 and is passed unclamped, so openHistory computes scanToStep*settings.StepSize (line 101) and wraps uint64 — 1e18 * 1562500 mod 2^64 = 5436724579849469952. That garbage bound goes into History.Scan.
It's harmless only by accident: Scan currently ignores its toTxNum argument (db/state/history.go:376, the commented-out reCalcVisibleFiles(toTxNum) TODO), and BeginFilesRoForDebug uses dirtyFilesEndTxNumMinimax(). If that TODO is ever restored, --to becomes a wrapped arbitrary limit — and for some stepSize values the wrapped product lands below the real end txNum, silently truncating the file set and under-reporting duplicates.
Worth clamping here, next to the maxStepForInt guard a few lines down that already handles the same class of overflow for the int conversion.
There was a problem hiding this comment.
Fixed. Step bounds go through stepToTxNum, which saturates at MaxUint64 instead of wrapping, and it is applied in openHistory before History.Scan — the site that was taking the wrapped product. TestStepToTxNum_SaturatesInsteadOfWrapping pins 1e18 * 1_562_500.
print and distribution had the same unclamped int(toStep)*int(stepSize) on their HistoryDump bounds; both now go through stepDumpBounds.
| for _, name := range names { | ||
| scan, err := scanDomainDuplicates(ctx, dirs, name, logger) | ||
| if err != nil { | ||
| logger.Warn("skipping domain", "domain", name, "err", err) |
There was a problem hiding this comment.
Every per-domain error is demoted to a warning and skipped, and the run still ends at line 381 with no consecutive duplicate history values found and exit 0. A total failure and a clean datadir are indistinguishable.
--domain=account (typo, singular) makes kv.String2Domain error, logs one warning, and prints the clean verdict. Same for --samples=-1, --to=5 --from=10, and for a mid-scan no .vi file found after 100M entries were already processed — partial results are dropped, verdict still clean.
Run should be RunE so the exit code reflects it; at minimum track a failure flag and print scan incomplete instead of the clean line.
There was a problem hiding this comment.
Fixed. duplicatesCmd is RunE. A domain scan failure is now returned rather than logged and skipped, --domain is validated up front via kv.String2Domain, and --samples/--to >= --from are checked before any datadir work. The clean verdict prints only when every domain was scanned and none had duplicates; an unscanned domain yields scan incomplete and a non-zero exit.
| names = []string{historyDomain} | ||
| } | ||
|
|
||
| ctx := cmd.Context() |
There was a problem hiding this comment.
ctx only reaches openHistory; neither HistoryDump nor scan.observe checks ctx.Err(). On the datadir from the PR description (148M commitment + 59M accounts entries) Ctrl-C cancels the context but the scan keeps running and keeps datadir.MustFlock() held, blocking any other erigon process on that datadir until it finishes.
The sibling diagnostics in db/integrity poll ctx.Done() inside their scan loops (e.g. rcache_no_duplicates.go:126); the callback here can do the same and return a sentinel error to stop the dump.
There was a problem hiding this comment.
Fixed. HistoryDump's callback signature is now func(key []byte, txNum uint64, val []byte) error and the error is propagated, so the scan returns ctx.Err() and the dump stops. The ETL load loop checks ctx.Err() too and passes Quit: ctx.Done(). Ctrl-C now releases the MustFlock promptly instead of running out the whole scan.
All three HistoryDump call sites are in this file, so the signature change was contained.
…nd fail loudly HistoryDump walks .ef files outer and keys inner, so a key reappears once per file with every other key in between. Comparing against the previous entry in that order missed every duplicate pair straddling a file boundary and counted one key once per file — the keysWithDup=0 in the PR description is what that false negative looks like. Entries now go through an ETL sort on key||txNum, so 'the previous entry for this key' means the previous one chain-wide. Also: - exact --from/--to filtering per entry; HistoryDump filters whole files only, so a partially overlapping file reported entries outside the range. - --to defaults to 1e18 steps, which wrapped uint64 when multiplied by the step size and fed a garbage bound to History.Scan; step bounds now saturate. - openHistory dropped History.Scan's error, which yielded an empty file set and a clean verdict for an unreadable snapshot dir. - the scan honours ctx, so Ctrl-C releases the datadir flock instead of running to completion; HistoryDump's callback can now fail. - a domain whose history is still in the DB is reported as unscanned rather than 'disabled or empty', and the command is RunE: a bad --domain, a mid-scan failure or an unscanned domain no longer exits 0 behind a clean verdict.
|
Addressed the review comments and pushed to the branch (2a6395a); replied on each thread. The one that changes the reported numbers: the scan compared each entry against the previous one in Also fixed: exact per-entry Left undone deliberately: history still in MDBX is not scanned. It is no longer misreported as
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
Suppressed comments (2)
cmd/integration/commands/state_history.go:405
- This treats every domain without history files as “history only in the DB,” but that also includes domains whose history is not enabled at all. Commitment and RCache history are disabled by default, so an ordinary datadir will add them to
unscannedand the default all-domain command will fail instead of scanning only domains present on disk as described. Distinguish an absent/disabled history from actual DB-resident history (for example by checking the on-disk history tables/config), and skip only the former.
if len(roTx.Files()) == 0 {
return nil, errHistoryNotInFiles
cmd/integration/commands/state_history.go:498
- An incomplete scan returns success whenever any earlier domain has duplicates, because this branch runs before the
unscannedcheck. That makes the command silently accept missing domains and contradicts the requirement that every domain be accounted for. Checkunscannedfirst, then reportwithDup.
if len(withDup) > 0 {
fmt.Printf("domains with duplicate history values: %v\n", withDup)
return nil
}
if len(unscanned) > 0 {
return fmt.Errorf("scan incomplete: %v have history only in the DB", unscanned)
}
| func (s *histDupSorter) add(key []byte, txNum uint64, val []byte) error { | ||
| binary.BigEndian.PutUint64(s.txNumBuf[:], txNum) | ||
| return s.collector.Collect(append(bytes.Clone(key), s.txNumBuf[:]...), val) |
Resolve db/state/history.go: keep the error-returning HistoryDump callback from this branch, plus main's doc comment.
What
Adds
integration history duplicates— a diagnostic that scans domain history for consecutive entries repeating the previous value for the same key.HistoryDumpyields entries grouped by key and ordered by txNum, so a consecutive-equal value is a redundant row that an as-of read collapses away.By default it scans every domain present in the datadir, so
commitmentandrcacheare included only when their history was actually enabled (detected by presence of entries, not by static schema flags — which don't reflect on-disk state in a standalone invocation). A single domain can be selected with--domain, and--from/--tobound the step range.Per domain it reports: total entries, distinct keys, keys carrying ≥1 duplicate, the duplicate-pair count with its share of entries, and a few example keys.
Why
While comparing serial vs parallel execution state, redundant history rows showed up as a source of non-determinism in file contents (identical state roots, differing history files). This tool makes it easy to audit any datadir for that class of redundancy across all domains at once.
Example
Tests
The consecutive-equal counting is factored into a small pure type (
histDupScan);TestHistDupScancovers the mixed, no-duplicate, empty, and sample-limit cases. Build +golangci-lintclean; verified end-to-end against a real archive datadir (output above).Read-only diagnostic; no production code paths touched.