Skip to content

feat(migration): statehistory migration - #3658

Open
MaksymMalicki wants to merge 13 commits into
mainfrom
maksym/statehistory-migration
Open

feat(migration): statehistory migration#3658
MaksymMalicki wants to merge 13 commits into
mainfrom
maksym/statehistory-migration

Conversation

@MaksymMalicki

@MaksymMalicki MaksymMalicki commented May 19, 2026

Copy link
Copy Markdown
Contributor

User description

Summary

Adds the statehistory migration: rewrites the three deprecated contract history layouts (class-hash, nonce, per-slot storage) so each entry stores the post-update value at its block instead of the pre-update value. Gated behind the existing --new-state flag. Depends on the headstate migration (consolidated Contract record) shipped in the sibling PR — the new layout reads contract.{ClassHash, Nonce} and the head storage trie as the "last value" source.

How it works

block │ old (pre-update) │ new (post-update)
──────┼──────────────────┼───────────────────
 100  │ —                │ V₀  ← explicit deploy (class-hash only)
 200  │ V₀               │ V₁
 500  │ V₁               │ V₂
 head │ V₂ (Contract)    │ (read from history)

Runs three sequential phases — class-hash, nonce, storage — each iterating the Contract bucket. Four worker goroutines (ingestorCount) per phase walk one contract's deprecated entries at a time, shift them into the new layout in shared db.Batches, and DeleteRange the deprecated rows in the same batch. One committer drains batches to disk; a semaphore caps in-flight batches at ingestorCount + 1.

  • Class-hash: the deprecated layout never wrote a deploy entry — the deploy-time hash lives only in the first replace's "pre-value". The migration inserts an explicit deploy_h entry on top of the shifted history, growing the count by one per replaced contract.
  • Nonce / storage: the first change entry's pre-value is 0 (the deploy default). Shift only; entry count per contract / per slot is unchanged.
  • Storage: the "last value for a slot" comes from the head storage trie, not the Contract record. The ingestor walks the deprecated history and the head trie in lockstep (both sorted by raw slot bytes); slots with no head leaf (zeroed out) resolve to felt.Zero.

What changes

  • New migration/statehistory/ package (migrator, three per-phase ingestors, shared baseIngestor, committer, counter, parse helpers, tests).
  • Registered in node/migration.go as an optional migration gated by cfg.NewState, running after the headstate migration.

Resume safety

  • Per-contract writes + deprecated-row deletion happen in the same batch; pebble batches commit atomically.
  • A contract whose history is large may span more than one batch. Each new entry's value is a pure function of the deprecated source, so re-running over a partially-rewritten contract overwrites with identical values and then deletes the (still-present) deprecated rows.
  • Contracts whose deprecated entries are already gone short-circuit on an empty iterator.
  • Phases run sequentially; a later phase only starts after the earlier phase completes.
  • Ctx cancellation returns (shouldRerun, ctx.Err()).

Alternatives considered

Two earlier attempts were benchmarked and dropped:

  1. Wipe + rewrite from state updates. Drop the deprecated history entirely and rebuild the new layout by replaying state updates block-by-block. Conceptually clean but the resulting writes touch every history bucket in a near-random order — pebble compaction has to merge many small per-block updates across overlapping key ranges, so compact pressure dominated runtime.

  2. Per-address instead of per-phase. Loop over contracts once and run all three phases (class-hash, nonce, storage) inside the same per-contract worker, finishing each contract before moving on. Saves two passes over the Contract bucket but interleaves writes to three different history buckets per contract — again scattered, again heavy on compaction.

The current per-phase approach writes are tightly clustered: one phase writes only one history bucket, in contract-address order, with deprecated DeleteRanges landing in the same batch as the new rows that replace them. Sequential, large, mostly-sorted writes — pebble's happy path. The two extra Contract-bucket scans are negligible compared to the compaction savings.


PR Type

Enhancement


Description

  • Rewrites contract history layouts to store post-update values

  • Adds newstate package encapsulating headstate and history phases

  • Extracts shared migration scaffolding into internal common package

  • Updates node migrations to use unified newstate migrator


File Walkthrough

Relevant files
Refactoring
8 files
committer.go
Added common Committer struct for shared pipeline writes 
+58/-0   
counter.go
Added a generic Counter for migration progress logging     
+67/-0   
ingestor.go
Introduced BaseIngestor struct for common pipeline processing tasks
+63/-0   
task.go
Added a common Task structure representing migration chunks
+9/-0     
ingestor.go
Migrated headstate ingestor to use the common BaseIngestor
+78/-0   
migrator.go
Updated headstate migrator to use common pipeline components
+9/-16   
committer.go
Deleted old headstate committer                                                   
+0/-49   
ingestor.go
Deleted old headstate ingestor                                                     
+0/-100 
Configuration changes
2 files
constants.go
Defined migration configuration constants like batch sizes
+14/-0   
migration.go
Replaced headstate migrator with unified newstate migrator
+2/-2     
Tests
4 files
counter_test.go
Added unit tests for the common Counter implementation     
+22/-6   
migrator_test.go
Updated import paths for headstate migrator tests               
+1/-1     
migrator_test.go
Added comprehensive test suite for contract history migrations
+762/-0 
migrator_test.go
Added tests for the unified multi-phase state migrator     
+266/-0 
Enhancement
6 files
class_hash_ingestor.go
Added ingestor to shift class hash history to post-update values
+164/-0 
migrator.go
Created migrator to manage history transformation phases sequentially
+170/-0 
nonce_ingestor.go
Added ingestor to shift nonce history to post-update values
+104/-0 
parse.go
Added helper utilities for parsing encoded history keys   
+28/-0   
storage_ingestor.go
Added ingestor for migrating per-slot storage history values
+221/-0 
migrator.go
Created a unified migrator coordinating headstate and history phases
+93/-0   
Additional files
1 files
counter.go +0/-54   

@MaksymMalicki
MaksymMalicki marked this pull request as draft May 19, 2026 11:20
@codecov

codecov Bot commented May 19, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 80.25641% with 77 lines in your changes missing coverage. Please review.
✅ Project coverage is 79.21%. Comparing base (121146c) to head (ca4ebb2).
⚠️ Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
...tate/newstate/internal/history/storage_ingestor.go 76.92% 21 Missing ⚠️
...e/newstate/internal/history/class_hash_ingestor.go 74.57% 15 Missing ⚠️
...ration/state/newstate/internal/history/migrator.go 75.47% 13 Missing ⚠️
.../state/newstate/internal/history/nonce_ingestor.go 81.08% 7 Missing ⚠️
...tion/state/newstate/internal/headstate/ingestor.go 78.57% 6 Missing ⚠️
migration/state/newstate/internal/history/parse.go 50.00% 6 Missing ⚠️
...gration/state/newstate/internal/common/ingestor.go 73.68% 5 Missing ⚠️
migration/state/newstate/migrator.go 93.54% 2 Missing ⚠️
...ration/state/newstate/internal/common/committer.go 95.23% 1 Missing ⚠️
node/migration.go 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3658      +/-   ##
==========================================
- Coverage   79.31%   79.21%   -0.11%     
==========================================
  Files         464      472       +8     
  Lines       35793    36089     +296     
==========================================
+ Hits        28389    28587     +198     
- Misses       7395     7493      +98     
  Partials        9        9              

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@MaksymMalicki
MaksymMalicki force-pushed the maksym/statehistory-migration branch from 5c0928e to 5a09f6c Compare May 19, 2026 22:36
@MaksymMalicki
MaksymMalicki marked this pull request as ready for review May 19, 2026 22:41
@MaksymMalicki
MaksymMalicki force-pushed the maksym/statehistory-migration branch from fa47934 to 05dd000 Compare May 20, 2026 12:56
Comment thread migration/statehistory/counter.go Outdated
elapsed := now.Sub(c.start).Seconds()
if elapsed > c.timeLogRate.Seconds() {
mbs := float64(c.size) / float64(db.Megabyte)
c.logger.Info(

@brbrr brbrr May 26, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

example log output:

statehistory/counter.go:39      write speed     {"phase": "class-hash", "MB": 96.00005149841309, "MB/s": 1.0337050014199167, "completedContracts": 1074100, "completedContracts/s": 11565.64527513286, "entries": 1378950, "entries/s": 14848.195281765626, "time": 92.86987232}

ideally, we should round to ~last 2 digits after .

Base automatically changed from maksym/headstate-migration to main May 28, 2026 09:39
@rodrodros
rodrodros force-pushed the maksym/statehistory-migration branch from 05dd000 to 5f59583 Compare May 28, 2026 09:44
@github-actions

github-actions Bot commented Jun 18, 2026

Copy link
Copy Markdown
Contributor

Claude finished @rodrodros's task in 4m 44s —— View job


Review: statehistory migration

Done — reviewed the full diff (migrator, three ingestors, shared common scaffolding, committer/counter, pipeline integration, and tests). This is a careful, well-documented PR and I found no correctness or concurrency blockers. Two nits posted inline.

What I verified

  • Concurrency / batching. The IngestorCount workers each touch a distinct Tasks[index] slot, Task is copied by value on the channel send (old batch goes to the committer, worker immediately rebinds to a fresh one), and Database is read-only — no shared mutable state, no data race. The semaphore capacity (IngestorCount+1) plus the unbuffered hand-off means a worker is down to 0 held batches before it GetBlockings a new one, and the committer always drains/Puts, so the worst case is 5 outstanding = capacity. No deadlock.
  • Resource cleanup. Every iterator (addressSeq, deprecated history, head storage trie) has a matching defer Close(). On error the committer keeps draining its input channel (the for range doesn't break on Run error), so all batches are Put back — no semaphore/goroutine leak.
  • Error handling. Source iterator errors are surfaced via errors.Join(sourceErr(), res.Err); malformed keys produce explicit errors; ctx cancellation returns (shouldRerun, ctx.Err()).
  • Storage lockstep. Walked the head-trie/deprecated-history lockstep for the tricky cases — missing leaf before/after a present one, fully-zeroed slots, interleaving. The head iterator only advances on the last entry of a slot that has a matching leaf, so it stays in sync regardless of zeroed-slot ordering. The prefix+251 leaf prefix correctly selects only leaves (Value always set ⇒ no nil-deref), and tests exercise all these paths.
  • Resume safety. New keys live in different buckets than the deprecated rows, so the same-batch DeleteRange never races the rewrites; re-running a partially-migrated contract overwrites with identical values then re-deletes. Cross-contract batch interleaving is handled by the empty-iterator short-circuit. Holds up.

Findings

nit

  • common/counter.goMB/MB/s are rounded to 2 dp but completedContracts/s, entries/s, and time are still full-precision (the bulk of the noise in @brbrr's earlier example log). Suggestion posted inline.
  • class_hash_ingestor.gowriteShiftedHistory re-counts EntryCount/CompletedAddrs on a resume pass (unlike the Has-guarded writeDeployOnly), so progress throughput can over-count after a restart. Metrics-only; correctness unaffected.

Both are non-blocking. Nice work on the per-phase design and the detailed doc comments.
· maksym/statehistory-migration

Comment on lines +52 to +60
fields = append(fields,
zap.Float64("MB", math.Round(mbs*cent)/cent),
zap.Float64("MB/s", math.Round(mbs/elapsed*cent)/cent),
zap.Uint64("completedContracts", c.completedAddrs),
zap.Float64("completedContracts/s", float64(c.completedAddrs)/elapsed),
zap.Uint64("entries", c.entryCount),
zap.Float64("entries/s", float64(c.entryCount)/elapsed),
zap.Float64("time", elapsed),
)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit (follow-up to @brbrr's earlier rounding request): MB and MB/s are now rounded to 2 decimals, but completedContracts/s, entries/s, and time are still emitted at full float precision — which is most of the noise in the example log line. Consider rounding them the same way:

Suggested change
fields = append(fields,
zap.Float64("MB", math.Round(mbs*cent)/cent),
zap.Float64("MB/s", math.Round(mbs/elapsed*cent)/cent),
zap.Uint64("completedContracts", c.completedAddrs),
zap.Float64("completedContracts/s", float64(c.completedAddrs)/elapsed),
zap.Uint64("entries", c.entryCount),
zap.Float64("entries/s", float64(c.entryCount)/elapsed),
zap.Float64("time", elapsed),
)
fields = append(fields,
zap.Float64("MB", math.Round(mbs*cent)/cent),
zap.Float64("MB/s", math.Round(mbs/elapsed*cent)/cent),
zap.Uint64("completedContracts", c.completedAddrs),
zap.Float64("completedContracts/s", math.Round(float64(c.completedAddrs)/elapsed*cent)/cent),
zap.Uint64("entries", c.entryCount),
zap.Float64("entries/s", math.Round(float64(c.entryCount)/elapsed*cent)/cent),
zap.Float64("time", math.Round(elapsed*cent)/cent),
)

Comment on lines +128 to +162
t.EntryCount++
if err := i.Flush(t, outputs); err != nil {
return err
}

for {
block, err := parseBlockKey(depIt.Key(), prefix)
if err != nil {
return fmt.Errorf("class-hash(%s): %w", addr, err)
}
hasNext := depIt.Next()
historyValue := *headClassHash
if hasNext {
rawValue, err := depIt.Value()
if err != nil {
return fmt.Errorf("class-hash(%s): %w", addr, err)
}
historyValue = felt.FromBytes[felt.Felt](rawValue)
}
if err := state.WriteClassHashHistory(t.Batch, addr, block, &historyValue); err != nil {
return err
}
t.EntryCount++
if err := i.Flush(t, outputs); err != nil {
return err
}
if !hasNext {
break
}
}

if err := t.Batch.DeleteRange(prefix, dbutils.UpperBound(prefix)); err != nil {
return fmt.Errorf("class-hash: DeleteRange deprecated(%s): %w", addr, err)
}
t.CompletedAddrs++

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit (metrics only): writeShiftedHistory unconditionally increments EntryCount/CompletedAddrs on every pass, whereas writeDeployOnly short-circuits via Has(deployKey) when the entry already exists. On a resume that re-runs an already-rewritten-but-not-yet-deleted contract, the shifted path re-counts every entry, so the reported entries/completedContracts throughput can over-count after a crash/restart. Correctness is unaffected (writes are idempotent); just noting the progress numbers aren't resume-exact here.

@NazariiDenha
NazariiDenha force-pushed the maksym/statehistory-migration branch from 4dda0e2 to 9fd197b Compare September 4, 2026 09:47
@github-actions

github-actions Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Review in progress

  • Gather context (PR description, prior comments, diff)
  • Read changed files in migration/state/common/, migration/state/history/, migration/state/headstate/, migration/state/newstate/, node/migration.go
  • Review for correctness, concurrency, resource leaks, error handling
  • Post inline comments for any findings
  • Post final summary

View branch

@github-actions

github-actions Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Claude finished @NazariiDenha's task in 7m 7s —— View job


Review: statehistory migration — re-review after move phases into internal package / state migration phases under one migration

  • Gather context (PR description, prior comments, diff)
  • Read changed files in migration/state/newstate/internal/{common,headstate,history}, migration/state/newstate/migrator.go, node/migration.go
  • Review for correctness, concurrency, resource leaks, error handling
  • Check whether prior nits (counter rounding, resume-count over-counting) were addressed
  • Post inline comments for any findings
  • Post final summary

This synchronize only contains a pure package reorg (migration/state/{common,headstate,history}migration/state/newstate/internal/{...}) plus two functional changes: a new newstate.Migrator orchestrator (migration/state/newstate/migrator.go) that runs headstate then history as phases of one migration.Migration, and the node/migration.go registration switching from two separate WithOptional entries to one. go build ./migration/... ./node/... and go vet ./migration/state/... pass; I could not run go test in this sandbox (tool approval declined for test execution) — the harness would need go test added to allowed commands to verify the suite here.

Blocker

  • node/migration.go:32 — combining headstate + history into a single newstate.Migrator registered at the same registry index that the standalone headstate.Migrator already occupies on main (PR feat(migration): headstate migration #3657, already merged) silently breaks upgrades. MigrationRunner tracks completion purely by positional bit in SchemaVersion (migration/runner.go, migration/version.go) — no name/hash identity check. Any node that already completed the headstate migration under the released code has that bit set; after upgrading to this build, the same bit now represents the combined headstate+history migrator, so the runner treats it as already done and the history phase inside it never runs. Deprecated class-hash/nonce/storage history entries are left in the old pre-update layout permanently, with no error surfaced. Two commits earlier in this same branch had this right — history.Migrator{} was registered as its own new bit (index 4), which is safe since a headstate-only DB never had that bit set. Suggest reverting to two separate registry entries (or otherwise giving the combined migrator a bit index that a headstate-only build could never have set). Posted inline with a fix-it link.

Nits (carried over from earlier review, still unaddressed — non-blocking)

  • migration/state/newstate/internal/common/counter.go:56-59MB/MB/s are rounded to 2dp but completedContracts/s, entries/s, and time are still logged at full float precision.
  • migration/state/newstate/internal/history/class_hash_ingestor.go:150 (writeShiftedHistory) — unconditionally re-increments EntryCount/CompletedAddrs on every pass, unlike the Has-guarded writeDeployOnly, so resumed/re-run passes can over-count progress metrics. Metrics-only, no correctness impact.
  • No test exists for migration/state/newstate/migrator.go (the phase orchestrator) or for registry-level compatibility across a pre-existing headstate-only SchemaVersion — this is exactly the gap that let the blocker above land undetected.

What still holds up from the prior full review

Concurrency (per-worker Tasks[index] isolation, semaphore capacity, no shared mutable state), iterator/resource cleanup (defer Close() everywhere), error wrapping, the storage history lockstep walk (head-trie vs. deprecated history, zeroed-slot handling), and resume-safety of the ingestors themselves (same-batch DeleteRange, idempotent overwrites) are all unchanged in this push and remain correct — the migrator_test.go table of scenarios (resume, cancellation, zeroed/interleaved slots, multi-address) still covers them well.
· Branch

Comment thread node/migration.go
@github-actions

github-actions Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

 Estimated effort to review: 3 🔵🔵🔵⚪⚪
🏅 Score: 85
🧪 PR contains tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Data Loss in Storage History

The resolveHistoryValue function expects the headIt iterator (head storage trie) and deprecatedHistoryIt to walk in perfect lockstep. However, if headIt contains a slot not present in deprecatedHistoryIt (e.g., due to prior history pruning or if the node was state-synced), headIt will fall behind the current slot. Because resolveHistoryValue only advances headIt on an exact match (bytes.Equal), it will never advance past the pruned/missing slot. Consequently, all subsequent slots in the deprecated history will fail the exact match check and incorrectly resolve their last history entry to felt.Zero instead of their true head value. You should advance headIt (e.g., using a for loop or .Seek()) whenever its current slot is lexicographically less than slot.Marshal().

func resolveHistoryValue(
	headIt db.Iterator,
	leafPrefix []byte,
	addr, slot *felt.Felt,
	hasSuccessor bool,
	successorSlot, successorValue felt.Felt,
	leafValid bool,
) (value felt.Felt, advanced bool, err error) {
	if hasSuccessor && successorSlot == *slot {
		return successorValue, false, nil
	}
	if !leafValid {
		return felt.Felt{}, false, nil
	}
	if !bytes.Equal(headIt.Key()[len(leafPrefix):], slot.Marshal()) {
		return felt.Felt{}, false, nil
	}
	raw, err := headIt.Value()
	if err != nil {
		return felt.Felt{}, false, fmt.Errorf(
			"storage: leaf(%s, slot=%s): %w", addr, slot, err,
		)
	}
	var node trie.Node
	if err := node.UnmarshalBinary(raw); err != nil {
		return felt.Felt{}, false, fmt.Errorf(
			"storage: decode leaf(%s, slot=%s): %w", addr, slot, err,
		)
	}
	return *node.Value, true, nil
}

@github-actions

github-actions Bot commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

Claude finished @NazariiDenha's task in 5m 15s —— View job


Review: statehistory migration — re-review after add test push

  • Gather context (PR description, prior comments, diff)
  • Check status of prior blocker (node/migration.go registry bit reuse)
  • Check status of reviewer-guide finding (storage_ingestor.go lockstep/pruned-slot concern)
  • Read changed files, review for correctness, concurrency, resource leaks, error handling
  • Post inline comments for findings
  • Post final summary

This push only adds migration/state/newstate/migrator_test.go (an end-to-end fixture test covering the phase-ordering happy path). Two previously-identified issues remain unresolved and are re-flagged inline:

Blocker

  • node/migration.go:32newstate.New() is still registered at the same registry position headstate.Migrator{} occupies on released main (PR feat(migration): headstate migration #3657). Since MigrationRunner tracks completion purely by positional bit in SchemaVersion with no identity check, any node that already completed the standalone headstate migration will have that bit set, and after upgrading, the history phase bundled inside newstate.Migrator will never run — silently leaving deprecated class-hash/nonce/storage history in the old layout forever. Flagged on 2026-09-04; unaddressed by the last three commits.
  • migration/state/newstate/internal/history/storage_ingestor.go:101-153 (confirms the automated PR-Agent "Data Loss in Storage History" focus area) — resolveHistoryValue assumes every head-storage-trie leaf has a matching entry in DeprecatedContractStorageHistory, walking both in lockstep and only advancing the head iterator on an exact slot match. That assumption breaks once history pruning (historyprunner/pruner, an independent --prune flag with no exclusion vs. --new-state) has deleted all deprecated entries for a slot whose last change predates the retention cutoff, while its head-trie leaf survives untouched. The head iterator then gets permanently stuck behind that orphaned leaf — every subsequent slot's final history entry for that contract silently resolves to felt.Zero instead of its true value. Traced through explicitly in the inline comment. This is reachable on any pruned node running the migration, not just a theoretical edge case.

Nits (carried over from earlier reviews, still unaddressed, non-blocking)

  • migration/state/newstate/internal/common/counter.go:56,58,59MB/MB/s are rounded to 2dp but completedContracts/s, entries/s, and time are still logged at full float precision.
  • migration/state/newstate/internal/history/class_hash_ingestor.go:150 (writeShiftedHistory) — unconditionally re-increments EntryCount/CompletedAddrs on every pass, unlike the Has-guarded writeDeployOnly, so resumed/re-run passes can over-count progress metrics. Metrics-only.

What still holds up

Concurrency (per-worker Tasks[index] isolation, semaphore capacity, no shared mutable state), iterator/resource cleanup (defer Close() everywhere), error wrapping, and resume-safety of the ingestors (same-batch DeleteRange, idempotent overwrites) are unchanged and remain correct for the non-pruned case. newstate/migrator_test.go's new fixture correctly exercises phase ordering (history depends on headstate's rewritten Contract records) and each layer's shift semantics end-to-end.
· Branch

Comment thread migration/state/newstate/internal/history/storage_ingestor.go
Comment thread node/migration.go
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants