perf(migration): merge-join the headstate source instead of point reads

[NazariiDenha]

---

[github-actions]

Claude encountered an error after 26s —— View job

---

I'll analyze this and get back to you.

[github-actions]

Review in progress

• Gather context (diff, changed files, CLAUDE.md)
• Review source.go (merge-join cursor logic)
• Review migrator.go (batch write logic)
• Review migrator_test.go
• Post findings

[github-actions]

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪

🏅 Score: 76

🧪 PR contains tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Iterator leak In pendingContracts, newCursor is called for each of the four buckets and appended to cursors only after a successful (err == nil) call. If a cursor's very first key is malformed, newCursor returns (c, c.err) with a non-nil error, causing the caller to set iterErr and return before appending c to the cursors slice. Since the deferred cleanup only closes iterators already in cursors, the just-opened db.Iterator for that bucket is never closed, leaking the handle. This triggers whenever a bucket's first scanned key fails the length check in cursor.set. } { c, err := newCursor(r, bucket) if err != nil { iterErr = err return } cursors = append(cursors, c) } Error masking In resolve, when heights.advanceTo(addr) returns false, the function immediately returns a generic "no deployment height for %s" error without joining driver.err, nonces.err, or heights.err. If the false result is actually caused by a malformed key encountered while advancing the heights cursor (rather than a genuinely missing entry), the more specific diagnostic error is silently discarded in favor of the misleading "missing height" message, making it harder to diagnose real data corruption. if !heights.advanceTo(addr) { return nil, fmt.Errorf("no deployment height for %s", &rec.addr) }

[codecov]

Codecov Report

❌ Patch coverage is 80.58252% with 20 lines in your changes missing coverage. Please review.
✅ Project coverage is 79.19%. Comparing base (ca4ebb2) to head (564f2ce).

Files with missing lines	Patch %	Lines
...ration/state/newstate/internal/headstate/source.go	80.26%	15 Missing ⚠️
...tion/state/newstate/internal/headstate/migrator.go	81.48%	5 Missing ⚠️

Additional details and impacted files

@@                        Coverage Diff                        @@
##           maksym/statehistory-migration    #4063      +/-   ##
=================================================================
- Coverage                          79.21%   79.19%   -0.02%     
=================================================================
  Files                                472      471       -1     
  Lines                              36089    36126      +37     
=================================================================
+ Hits                               28587    28610      +23     
- Misses                              7493     7507      +14     
  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.

[rodrodros]

@NazariiDenha when writing a PR as perf, please be sure to include why is it a perf

[EgeCaner]

Can be inlined, last step does not need to rotate the batch as well

[NazariiDenha]

last step does not create new batch now, renamed to flush()

[EgeCaner]

I'd still inline this. With the batch reopen moved out, flush is just Size → Write → Log, and the two call sites genuinely differ, the tail needs neither the reset nor a new batch, so the closure hides that rather than helping.

At exit, neither of the last two statements does anything useful. completed = 0 is dead, nothing reads it again. And counter.Log doesn't help either: it only prints when more than TimeLogRate has elapsed since the previous line, so on the final call it almost always returns silently, and the last batch's bytes and contract count are dropped along with the counter. Whether that batch ever shows up in the log is timing luck.

[EgeCaner]

Add a completion log before returning, with the elapsed time since the phase started, nothing currently records how long this takes

[EgeCaner]

If I'm not mistaken every deployed contract must have nonce 0 written to head state. see

[NazariiDenha]

Previous headstate migration had same check for non-existing nonce, so I left it here. I made only performance changes here

[EgeCaner]

It gives the impression that it might not exists, but if Im not mistaken it must exists. If that is the case we should make it required, either here or in the original PR

[github-actions]

Review in progress

• Gather context (diff, changed files, CLAUDE.md)
• Review source.go (merge-join cursor logic)
• Review migrator.go (batch write logic)
• Review migrator_test.go
• Review pipeline.go change
• Post findings

[github-actions]

Standalone PR Review

PR-Agent could not safely update the persistent review. This standalone result will not replace the canonical review.

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪

⚠️ Risk level: Medium

📂 Priority files migration/state/newstate/internal/headstate/source.go migration/state/newstate/internal/headstate/migrator.go

🏅 Score: 68

🧪 PR contains tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Panic risk resolve reads the deployment-height value and calls binary.BigEndian.Uint64(raw) without checking that raw is at least 8 bytes. If the ContractDeploymentHeight value is malformed/shorter than 8 bytes (e.g. due to DB corruption or a bug elsewhere), this will panic with an index-out-of-range error instead of returning a graceful error like the key-length validation already does for cursor keys. raw, err = heights.it.UncopiedValue() if err != nil { return rec, fmt.Errorf("reading deployment height for %s: %w", &rec.addr, err) } rec.height = binary.BigEndian.Uint64(raw) Deferred error detection Errors on the migrated (Contract bucket) cursor are only surfaced via errors.Join after the driver loop fully completes (line 155), unlike nonces/heights errors which are caught per-iteration inside resolve. If the Contract bucket has a malformed key partway through, migrated.advanceTo will keep returning false for every subsequent address (since c.addr becomes nil), causing every remaining contract to be needlessly re-resolved and re-written before the real error is finally reported at the end. Not data-corrupting, but wastes a full pass and delays failure detection. Uncertain how likely a malformed Contract key is in practice. for driver.addr != nil { addr := driver.addr if migrated.advanceTo(addr) { driver.next() continue } rec, err := resolve(driver, nonces, heights, addr) if err != nil { iterErr = err return } if !yield(rec) { return } driver.next() } iterErr = errors.Join(driver.err, nonces.err, heights.err, migrated.err)

[github-actions]

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

---

Review of PR #4063

• Gather context (diff, changed files, CLAUDE.md)
• Review source.go (merge-join cursor logic)
• Review migrator.go (batch write logic)
• Review migrator_test.go
• Review pipeline.go change
• Post findings

This PR replaces the pipelined point-read ingestor with a sequential merge-join over the four address-keyed buckets (ContractClassHash drives, ContractNonce/ContractDeploymentHeight/Contract are advanced in lockstep via cursor.advanceTo). The iterator-leak, error-masking, and WriteContract error-message issues raised by earlier automated passes have all been addressed in the latest commit (564f2ce) — newCursor now closes its iterator before returning an error, resolve surfaces heights.err instead of masking it with a generic "missing height" message, and errors are now in present-participle form ("writing contract %s: %w").

Verified the code builds (go build ./migration/...) and the merge-join / advanceTo logic against the test cases in migrator_test.go (empty DB, consolidation, skip-already-migrated, idempotent re-run, mid-run missing height).

Important

• migration/state/newstate/internal/headstate/source.go:136-155 — the migrated cursor's error is only surfaced in the final errors.Join after the driver loop finishes. Once a malformed Contract-bucket key sets migrated.err, migrated.addr stays nil for the rest of the pass, so migrated.advanceTo returns false for every subsequent address — every remaining contract gets needlessly re-resolved and re-written before the real error is reported at the end. Not data-corrupting (the migration is re-run safe), but it turns what should be an early exit into a full wasted pass. This was flagged in a previous review round and doesn't look addressed yet; suggest checking migrated.err right after advanceTo the same way heights.err is already checked in resolve.

Nit

• migration/state/newstate/internal/headstate/source.go:102 — binary.BigEndian.Uint64(raw) on the deployment-height value has no length check, so a value shorter than 8 bytes (DB corruption) would panic rather than return a graceful error like the key-length validation already does. Matches pre-existing behavior in core.GetContractDeploymentHeight, so not a regression from this PR, just worth noting given the file otherwise hardens against malformed keys.

Everything else looks solid: the advanceTo/lockstep design is correct for the "driven by ContractClassHash, others advance-or-skip" semantics, the batch/flush loop in migrator.go correctly checks ctx.Err() per iteration and flushes on both size threshold and completion, and wipeDeprecatedBuckets + the "skip already migrated" check make the migration safely re-runnable.

[github-actions]

Standalone PR Review

PR-Agent could not safely update the persistent review. This standalone result will not replace the canonical review.

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪

⚠️ Risk level: Medium

📂 Priority files migration/state/newstate/internal/headstate/source.go migration/state/newstate/internal/headstate/migrator.go

🏅 Score: 75

🧪 PR contains tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Delayed error propagation resolve() joins and returns driver.err, nonces.err, and heights.err, but not migrated.err. If the Contract bucket cursor hits a malformed key, migrated.advanceTo silently returns false (indistinguishable from "not migrated") and the error is only surfaced in iterErr after the entire driver loop finishes. This means every remaining address gets treated as unmigrated and re-written with stale data from the deprecated buckets before the corruption is finally reported, instead of aborting immediately when the corrupt key is detected. func resolve(driver, nonces, heights *cursor, addr []byte) (pendingContract, error) { rec := pendingContract{addr: felt.FromBytes[felt.Address](addr)} raw, err := driver.it.UncopiedValue() if err != nil { return rec, fmt.Errorf("reading class hash for %s: %w", &rec.addr, err) } rec.classHash.SetBytes(raw) // A missing nonce means the contract was never updated. if nonces.advanceTo(addr) { raw, err := nonces.it.UncopiedValue() if err != nil { return rec, fmt.Errorf("reading nonce for %s: %w", &rec.addr, err) } rec.nonce.SetBytes(raw) } if !heights.advanceTo(addr) { if heights.err != nil { return rec, heights.err } return rec, fmt.Errorf("no deployment height for %s", &rec.addr) } raw, err = heights.it.UncopiedValue() if err != nil { return rec, fmt.Errorf("reading deployment height for %s: %w", &rec.addr, err) } rec.height = binary.BigEndian.Uint64(raw) return rec, errors.Join(driver.err, nonces.err, heights.err) } // pendingContracts walks the deprecated buckets and Contract in lockstep. All // four are keyed by address, so one sequential pass replaces every point read. // ContractClassHash drives: it defines the contract set. func pendingContracts(r db.KeyValueReader) (iter.Seq[pendingContract], func() error) { var iterErr error seq := func(yield func(pendingContract) bool) { cursors := make([]*cursor, 0, 4) defer func() { for _, c := range cursors { c.it.Close() } }() for _, bucket := range []db.Bucket{ db.ContractClassHash, db.ContractNonce, db.ContractDeploymentHeight, db.Contract, } { c, err := newCursor(r, bucket) if err != nil { iterErr = err return } cursors = append(cursors, c) } driver, nonces, heights, migrated := cursors[0], cursors[1], cursors[2], cursors[3] for driver.addr != nil { addr := driver.addr if migrated.advanceTo(addr) { driver.next() continue } rec, err := resolve(driver, nonces, heights, addr) if err != nil { iterErr = err return } if !yield(rec) { return } driver.next() } iterErr = errors.Join(driver.err, nonces.err, heights.err, migrated.err)

[github-actions]

Important — deferred error detection on the migrated cursor still wastes a full pass on corruption.

migrated.err (set inside advanceTo → next → set when a Contract-bucket key is malformed) is only surfaced in the final errors.Join at line 155, after the driver loop has already run to completion. Once migrated.err is set, migrated.addr becomes nil permanently, so migrated.advanceTo(addr) (line 139) returns false for every remaining address — every remaining contract is treated as "not yet migrated," resolved, and written to the batch, before the corruption is finally reported and the whole migration is failed anyway.

This was already flagged in the previous automated review pass and doesn't look like it's been addressed. Not data-corrupting (writes are idempotent/re-runnable), but it turns a cheap early-exit into a full wasted pass whenever the Contract bucket has a malformed key. Consider checking migrated.err right after the advanceTo call (same pattern already used for heights.err at lines 93-95) so the failure surfaces immediately instead of after redoing the remaining work.

Fix this →

[github-actions]

Nit — potential panic on a malformed deployment-height value.

binary.BigEndian.Uint64(raw) will panic (index out of range) if raw is shorter than 8 bytes. cursor.set already validates that keys are the expected length before use, but there's no equivalent length check on the value read here (or in driver.it.UncopiedValue() / nonces.it.UncopiedValue() above). In the current codebase this mirrors the existing core.GetContractDeploymentHeight pattern, so it's not a regression, but since this migration already hardens against malformed keys, it might be worth doing the same for malformed values rather than trusting raw blindly — a corrupted/truncated value here would crash the migration instead of surfacing a clean error like the rest of this function.