feat(migration): trie migration - #3659
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## maksym/statehistory-migration #3659 +/- ##
=================================================================
- Coverage 79.11% 79.03% -0.09%
=================================================================
Files 471 478 +7
Lines 36091 36559 +468
=================================================================
+ Hits 28555 28895 +340
- Misses 7527 7655 +128
Partials 9 9 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
b23a1b7 to
3640393
Compare
f6ba2c8 to
4aac6ce
Compare
|
This pull request is stale because it has been open 35 days with no activity. Remove stale label or comment or this will be closed in 14 days. |
df52a01 to
91cf2e3
Compare
|
Claude finished @NazariiDenha's task in 5m 16s —— View job Review of PR #3659 (trie migration)Todo list
SummaryReviewed the DFS trie migrator (enumerate → traverse → hash → commit, with a parallel edge-hashing pool for large tries), the resumability/wipe logic in Important
Nit / observation
Everything else in the trie package (codec, root-edge handling for non-empty deprecated root paths, resumability check via Branch: |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| func (i *ingestor) migrateTrie(t *task, desc TrieDesc, outputs chan<- task) error { | ||
| if desc.NodeCount == 0 { | ||
| return nil | ||
| } | ||
| parallelDispatch := desc.NodeCount >= SmallTrieThreshold | ||
| prefix := deprecatedTriePrefix(desc) | ||
| sched := newHashScheduler(desc.HashFn, parallelDispatch, desc.TrieBucket, desc.Owner, i.pool) | ||
|
|
||
| rootHash, err := i.traverse(t, outputs, prefix, *desc.RootPath, sched) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| if err := sched.sync(t.batch); err != nil { | ||
| return err | ||
| } | ||
| if desc.RootPath.Len() > 0 { | ||
| if err := writeRootEdgeNode(desc.RootPath, rootHash, sched, t.batch); err != nil { | ||
| return err | ||
| } | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Potential panic: orphaned hashWorkerPool.submit goroutine races with pool.close() when a large trie's traversal errors out.
For tries at/above SmallTrieThreshold, hashScheduler.fire (hashworker.go:71-84) submits a batch to the shared hashWorkerPool and only drains it (drainInFlight) on the next fire call or in sched.sync (hashworker.go:106-129). sync is only reached here on the success path of traverse — if traverse returns an error (line 184-187), migrateTrie returns immediately and sched.sync is never called, so any in-flight batch is abandoned with its done channel never consumed.
That abandoned batch corresponds to a still-running background goroutine spawned inside hashWorkerPool.submit (hashpool.go:46-58), which is independently sending chunks to p.work and doing wg.Wait(). Meanwhile the error propagates up through the pipeline, runMigration returns early, and its defer pool.close() (trie.go:118) closes p.work — with no synchronization against that orphaned goroutine. If the orphaned goroutine hasn't finished its sends yet, close(p.work) races with p.work <- hashWork{...}, which panics with "send on closed channel".
This isn't limited to ctx cancellation — any error during traversal of a large (parallel-dispatch) trie (e.g. a transient batch Put/db error) hits the same path, since traverse's error return skips sync() unconditionally.
Notably, SmallTrieThreshold is 100_000 nodes, so none of the current tests (which use tiny fixture tries) exercise this code path at all — consistent with hashworker.go/hashpool.go showing the lowest patch coverage in this PR (36% / 32% per Codecov).
Suggest draining any in-flight batch before returning an error from migrateTrie/traverse (e.g. defer-driven drain, or have pool.close() wait for outstanding submissions via a sync.WaitGroup tracked by the pool itself), so shutdown/error paths can't race with close(p.work).
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
91cf2e3 to
1df3dd8
Compare
|
Claude finished @NazariiDenha's task in 3m 20s —— View job Review of PR #3659 (trie migration)Todo list
SummaryThis diff ( Important
Nit / observation
|
| // | ||
| // In-flight batches flush at target size; cancellation is observed at | ||
| // every flush and every channel send. | ||
| func (i *ingestor) migrateTrie(t *task, desc TrieDesc, outputs chan<- task) error { |
There was a problem hiding this comment.
Carried over from the previous review: still-unresolved panic risk (send on closed channel) when a large trie's traversal errors out.
migrateTrie only calls sched.sync(t.batch) on the success path (line 188). If i.traverse returns an error at line 184-187 — which happens on ctx cancellation (via flush's select) or any other read/decode error — the function returns immediately, and sched.sync is skipped.
For a trie at/above SmallTrieThreshold (parallel dispatch), hashScheduler.fire (hashworker.go:71-84) may already have an in-flight batch submitted to the shared hashWorkerPool via pool.submit (hashpool.go:41-59). That submission spawns a background goroutine that sends chunks to p.work and then does wg.Wait(); it is only drained by drainInFlight, which is reached from sync(). Skipping sync() on error abandons that goroutine — it's not tied to ctx at all, so it keeps running independently.
Meanwhile the error propagates up through pipeline.New (migration/pipeline/pipeline.go:65-68), which calls r.cancel(); runMigration (trie.go:108-165) returns shouldRerun, err, and its defer pool.close() (trie.go:118) calls close(p.work). If the orphaned goroutine hasn't finished sending its remaining chunks yet, that send races with the close and panics with "send on closed channel".
This is unchanged since it was flagged in the last review pass (then at migration/trie/ingestor.go) — this commit only moved the file to migration/state/newstate/internal/trie/ (git show 1df3dd83c is a pure rename + 2-line wiring change in migrator.go), the logic itself is untouched. Still no test exercises SmallTrieThreshold/parallel dispatch (grep -n parallel trie_test.go finds nothing), consistent with the low patch coverage Codecov reported for hashworker.go/hashpool.go.
Suggest having hashWorkerPool track outstanding submit calls itself (e.g. its own sync.WaitGroup that close() waits on before closing p.work), so an error/cancellation path in migrateTrie can't race with pool shutdown regardless of whether sync() gets called.
3 participants
User description
Summary
One-shot migration that converts every deprecated Starknet trie on disk into the equivalent
trie2layout. After this migration runs, the new state package can read directly from the new buckets and the deprecated buckets are wiped.Bucket mapping:
ClassesTrieClassTrieStateTrieContractTrieContractContractStorageContractTrieStorageFormat differences
Three things change between the two layouts: how nodes are keyed on disk, how each node's value is encoded, and how path compression is expressed.
On-disk keying
Both layouts share a common prefix; only the suffix differs:
common (both) suffix
───────────── ─────────────────────────────────────────
bucket [|| owner] → path-length-byte || path-bytes (deprecated)
→ nodeType-byte || path-length-byte || path-bytes (new)
owneris present only for storage tries. The new layout's extranodeTypebyte splits leaves from internal nodes into two index slices within the same bucket —trie2state lookups use this to short-circuit between leaf reads and internal-node traversals.Node encoding
Both layouts are raw byte streams (no length prefixes, no varints — field-element widths are fixed).
Deprecated: nodes are self-contained. Internal binary nodes embed the compressed paths to their children inline:
leaf value
binary value || left-child-path || right-child-path
[|| left-hash || right-hash, optional cache, ignored here]
valueis the node's own Starknet trie hash, or the stored value when the node is a leaf. The trailing hash pair was a denormalised cache; the migrator does not read it (hashes are recomputed from scratch — see below).New: every node has an explicit type tag and path compression lives in dedicated edge nodes:
value value
binary 0x01 || left-edge-hash || right-edge-hash
edge 0x02 || child-hash || encoded-path-segment
Path compression — the key structural change
The deprecated format compresses paths inside the parent binary node (via its embedded child-path fields). The new format moves compression into dedicated edge nodes sitting between binary nodes and their children:
deprecated: binary ──────── child-path ────────► child
new: binary ──► edge ──► child
One consequence: the deprecated root marker (a single entry at the bare bucket prefix recording the root's path) disappears. Whatever the deprecated root embedded becomes either a direct binary/leaf at the empty path or, when the deprecated root path is itself non-empty, an edge node at the empty path that points "down" to the real root.
Traversal
DFS is a natural fit here. In the new layout a binary node's payload is
left-edge-hash || right-edge-hash— so before we can write the parent, we need both children's hashes. A bottom-up walk reads each deprecated node exactly once and produces the child hash that the parent needs at the moment the parent is encoded; no separate hashing pass, no intermediate caches sized to the trie. Going top-down would force us to either re-read every child later or hold every visited node in memory until its subtree is hashed.For each trie:
ContractStoragekeyed bybucket || owner, so the enumerator splits them into per-owner descriptors. Each descriptor records the root path (from the bare-prefix marker entry) and the node count.Pipeline:
IngestorCountworker goroutines pull descriptors from the enumeration source; a single committer flushes filled batches to disk. A semaphore caps in-flight batches atIngestorCount * 2. Every flush and every channel send observesctx.Done; on cancelMigratereturns theshouldRerunsentinel and the migration runner re-invokes on the next process start.After the full pipeline finishes, the three deprecated buckets are wiped via
DeleteRange. The wipe is gated on full success — a crashed mid-migration leaves the deprecated source intact, so partially migrated tries either have a new-format root (skipped on the next pass) or don't (re-migrated from scratch).Alternative considered: reverse-iteration BFS
An earlier attempt used reverse-iteration BFS over the deprecated bucket — iterating keys from longest-path to shortest so leaves are processed first, then their parents, then their parents, and so on. Each level's hashes are buffered until the next level up consumes them.
In practice it performed comparably to DFS on wall-clock time but with substantially higher peak memory — the buffer of "hashes waiting for their parent" grows roughly with the widest level of the trie, which for full or near-full subtrees is most of the leaves. DFS keeps only the current root-to-leaf path in flight, which is bounded by the trie depth (≤ 251), so memory stays flat regardless of trie size. Same correctness, worse memory profile, no speed win — dropped.
Hashing
Starknet trie hashes for the new layout:
leaf value
binary hashFn(left-edge-hash, right-edge-hash)
edge hashFn(child-hash, path-segment-as-felt) + segment-length
A zero-length edge short-circuits to the bare
child-hash— the convention for absent edges. Class tries hash with Poseidon; contract and storage tries with Pedersen.Performance: for small tries (below
SmallTrieThresholdnodes) every edge hash is computed inline. Above the threshold, edge-hash jobs are batched (parallelHashBatchSize) and dispatched to a fixed-size worker pool for parallel computation. The scheduler preserves the original job order so the persisted bytes are byte-identical to a natively-builttrie2— verified end-to-end in the tests by comparing the migrated DB against one built directly throughtrie2.Trie.Update(seeTestMigrationEndToEnd).PR Type
Enhancement
Description
Migrate Starknet tries to
trie2layout.ClassesTrie,StateTrie, andContractStorage.Build parallel hash and write pipeline.
Add state migration orchestrator and safeguards.
Register new migration phase.
File Walkthrough
1 files
Encode and decode node blobs, paths, and edge hashes1 files
Write database batches and handle concurrency semaphores1 files
Track and log migration progress and speed metrics2 files
Worker pool for parallel trie edge hash computationSchedule and buffer parallel or synchronous hash jobs2 files
Traverse deprecated tries and stream nodes to database batchesMain migration orchestrator and trie bucket enumerator pipeline1 files
Tests for trie migration, idempotency, and context cancellation1 files
Register trie migration phase in the main state migrator