snapshots: align state files to block file boundary on startup by mh0lt · Pull Request #20434 · erigontech/erigon

mh0lt · 2026-04-09T08:53:03Z

The preverified snapshot set may include state domain files whose committed block number extends past the block file boundary. This causes "behind commitment" errors on fresh sync because SeekCommitment returns a block number that TxNums doesn't cover.

After downloading snapshots and opening the aggregator files, read the commitment "state" key to get the committed block number. If it exceeds FrozenBlocks(), iteratively remove the highest state files (across all dirs: domain, idx, history, accessor) and strip them from preverified.toml so the downloader doesn't re-fetch them.

The alignment loop removes one step range per iteration, reopening the aggregator each time until the commitment block is within bounds.

Copilot

Pull request overview

This PR adds a startup safety check to detect and correct misalignment between downloaded state snapshots (commitment block) and the block snapshot boundary, preventing fresh-sync “behind commitment” failures.

Changes:

Adds a post-OpenFolder() alignment step that reads the commitment “state” key and, if needed, removes the highest state snapshot ranges.
Deletes corresponding snapshot files across state snapshot directories and strips matching entries from preverified.toml to prevent re-download.
Forces aggregator reopen after each removal iteration to re-evaluate alignment.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

sudeepdino008

I'm adding the state progress check back to integrity - #20437; so we will not publish snapshots when state is ahead of blocks
we had removed the check because the issue was seemingly solved, but it's floated up again.
the solution should involve downloading the remaining blocks, and then proceeding..instead of messing around with preverified toml and removing latest files..
the problem with removing latest files is -- it can remove too much. If only one domain file 0-4096.kv is there, it can completely get removed on rm-state. It is a very brittle solution.

mh0lt · 2026-04-09T12:09:39Z

I'm adding the state progress check back to integrity - Revert remove state progress check r34 #20437; so we will not publish snapshots when state is ahead of blocks

we had removed the check because the issue was seemingly solved, but it's floated up again.

the solution should involve downloading the remaining blocks, and then proceeding..instead of messing around with preverified toml and removing latest files..

the problem with removing latest files is -- it can remove too much. If only one domain file 0-4096.kv is there, it can completely get removed on rm-state. It is a very brittle solution.

This is repordcing the the manual process and fixing the error at the right layer of the code. The suolution should be don't publish bad files, but it seems that doesn't happen reliably. This is fixing the problem at source, which is a bad preverified.toml and associated files, not attempting a work around at a higher level of the app, which should expect the downloader to work without failing.

mh0lt · 2026-04-09T12:11:20Z

Thanks for the review @sudeepdino008. Good to know #20437 is adding the state progress check back to prevent this at publication time.

You're right that file removal is brittle — the edge case of a single 0-N.kv merged file getting removed is a real concern. The "download remaining blocks" approach is better.

However, when running with --no-downloader (common for testing and some production setups), there's no way to download the remaining blocks. The alignment was designed as a startup safety net for that case.

Questions:

For the "download remaining blocks" approach: does the downloader know how to fetch individual block files on demand, or would it need to wait for the next preverified set that includes those blocks?
Would it make sense to keep the alignment as a fallback behind an env flag (e.g. ALIGN_STATE_TO_BLOCKS=true) for --no-downloader use cases, while the default path waits for block downloads?

Happy to rework this based on your direction.

🤖 Generated with Claude Code

mh0lt · 2026-04-09T12:20:01Z

Thanks for the review @sudeepdino008. Good to know #20437 is adding the state progress check back to prevent this at publication time.

You're right that file removal is brittle — the edge case of a single 0-N.kv merged file getting removed is a real concern. The "download remaining blocks" approach is better.

However, when running with --no-downloader (common for testing and some production setups), there's no way to download the remaining blocks. The alignment was designed as a startup safety net for that case.

Questions:

For the "download remaining blocks" approach: does the downloader know how to fetch individual block files on demand, or would it need to wait for the next preverified set that includes those blocks?

Would it make sense to keep the alignment as a fallback behind an env flag (e.g. ALIGN_STATE_TO_BLOCKS=true) for --no-downloader use cases, while the default path waits for block downloads?

Happy to rework this based on your direction.

🤖 Generated with Claude Code

This is exactly why I don't want to do this kind of thing, claude gets completely confused by this process and adds spurious breaking code. It doesn't have a good understanding of the snapshot formats and starts to do as series of wierd things like raplacing files, erdoenloading etc. Also of which result in unstable tests.

sudeepdino008 · 2026-04-09T12:46:24Z

The suolution should be don't publish bad files, but it seems that doesn't happen reliably.-- it can happen reliably in publishing level; just that we removed it because we thought we were done with this issue.

I don't agree that state ahead of block files is bad snapshot set. Theoretically we should just catch up the blocks and resume? -- what's stopping us from doing that?

mh0lt · 2026-04-09T18:14:01Z

The suolution should be don't publish bad files, but it seems that doesn't happen reliably.-- it can happen reliably in publishing level; just that we removed it because we thought we were done with this issue.

I don't agree that state ahead of block files is bad snapshot set. Theoretically we should just catch up the blocks and resume? -- what's stopping us from doing that?

The state is ahead of the blocks, so when we start execyting we will get fails becuase the state is pre-existing. We could re-execute historically, but now we are chaning the flow because of an error condition,

Where are we going to get the blocks from ? How do we verify the data. This is nothing to do with the application it is a fatal error in snapshot publishing and should be fixed withing that layer, not exported to the rest of the system. That is what this fix is doing. Its unwinding the state back to the last known verifed point and then re-executiong from there.

I effect what I have done is a local fix for 'don't publish bad files, but it seems that doesn't happen reliably' Its 'Don't injest bad files'. That we can do reliably even if the publish isn't reliable, and in practice is the begining of client pre-integrity checks.

I don't see this as a short term fix, the node should really do integrity check of external files rather than blindly trusting them.

AskAlexSharov · 2026-04-10T15:21:22Z

@sudeepdino008 "don't publish" is good, but maybe not enough. Because user also can rm -rf datadir/chaindata when state ahead of blocks

sudeepdino008 · 2026-04-11T16:08:30Z

@sudeepdino008 "don't publish" is good, but maybe not enough. Because user also can rm -rf datadir/chaindata when state ahead of blocks

i agree. The fix has to account for that. what i don't understand is why we can't catch up blocks and then begin execution. maybe i'll give it a shot.

don't publish-- is just a temporary guard until we fix this. additionally, we need smoke tests for this snapshot state in ci. The one currently in qa "only" catches when published snapshots in this state. The smoke test can force this state and see if erigon can continue from this.

The preverified snapshot set may include state domain files whose committed block number extends past the block file boundary. This causes "behind commitment" errors on fresh sync because SeekCommitment returns a block number that TxNums doesn't cover. After downloading snapshots and opening the aggregator files, read the commitment "state" key to get the committed block number. If it exceeds FrozenBlocks(), iteratively remove the highest state files (across all dirs: domain, idx, history, accessor) and strip them from preverified.toml so the downloader doesn't re-fetch them. The alignment loop removes one step range per iteration, reopening the aggregator each time until the commitment block is within bounds. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

- Use existing tx instead of opening nested transaction (Copilot review) - Return (step, found bool) from findHighestStateFileStartStep to handle step 0 correctly (Copilot review) - Filter to state domain files only via isStateDomainFile() to avoid accidentally deleting block snapshot indices (Copilot review) - Handle errors from os.Chmod/os.WriteFile on preverified.toml (Copilot) - Skip directory entries with e.IsDir() (Copilot review) - Update comment to match actual algorithm (Copilot review) - Remove fixed iteration cap — loop until aligned or no progress - De-register deleted files from downloader via seeder.Delete() Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

The alignment was running on every restart, not just fresh starts. When the downloader re-downloads state files between restarts, the alignment would cascade — removing re-downloaded files, then the ones below, until all state snapshot files were gone. Fix: check stages.Execution progress before aligning. If > 0, the node already processed past any snapshot misalignment and the alignment is unnecessary. Also added debug logging to trace alignment decisions. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Tests for parseFileStepRange, isStateDomainFile, findHighestStateFileStartStep, removeStateFilesFromStep, and the stageProgress guard that prevents alignment on restart. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Replace the stageProgress > 0 guard with a TxNums.Max(frozenBlocks) check. This is a lower-level check that directly verifies the TxNums index covers all frozen blocks — meaning the node already indexed past the block boundary. More reliable than stageProgress which could be set from a different execution context. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

The preverified snapshot set may include state domain files whose committed block number extends past the block file boundary. This causes "behind commitment" errors on fresh sync because SeekCommitment returns a block number that TxNums doesn't cover. After downloading snapshots and opening the aggregator files, read the commitment "state" key via a temporary temporal RO tx. If the committed block exceeds FrozenBlocks(), iteratively remove the highest state files (across all dirs: domain, idx, history, accessor), de-register them from the downloader via seeder.Delete(), and strip from preverified.toml. The alignment only runs on fresh start — if TxNums already covers all frozen blocks (checked via a temporal RO tx), the node previously indexed past the boundary and alignment is skipped. This prevents the cascade bug where re-downloaded files on restart would trigger repeated alignment until all state files were removed. Includes: - isStateDomainFile() filter to avoid deleting block snapshot files - findHighestStateFileStartStep returns (step, found) for step-0 safety - Debug logging for alignment decisions - Unit tests for all helper functions Tested end-to-end: 1. Fresh start: alignment removes one misaligned step (44 files), execution starts from aligned commitment point 2. Restart: alignment skips entirely, execution resumes, no files removed Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-14T08:30:33Z

+	defer roTx.Rollback()
+	v, _, err := roTx.GetLatest(kv.CommitmentDomain, []byte("state"))
+	if err != nil || len(v) < 16 {


readCommitmentBlockFromDB hard-codes the commitment state key as []byte("state"). The codebase already defines and uses commitmentdb.KeyCommitmentState (execution/commitment/commitmentdb/commitment_context.go:555,905). Please use the shared constant here too so the stage stays consistent if the key name ever changes and to avoid duplicate literals.

Copilot · 2026-04-14T08:30:33Z

+	// Only align on fresh start. If MDBX already has commitment data
+	// (written by execution, not by OtterSync indexing), the node
+	// previously executed past any snapshot misalignment.
+	// Re-running alignment on restart would remove snapshot files that
+	// the downloader re-downloaded, cascading until all state is gone.
+	if roTx, err := cfg.db.BeginRo(ctx); err == nil {
+		// Check if the commitment domain values table has any entries.
+		// This table is only populated by execution, not by OtterSync.
+		if cursor, cErr := roTx.Cursor(kv.TblCommitmentVals); cErr == nil {


The “fresh start” guard described in comments/PR text refers to checking TxNums coverage, but the implementation here skips alignment based on whether kv.TblCommitmentVals has any entries. Please align the guard logic and its documentation (either implement the TxNums-based guard or update the comments/PR description/tests to reflect the actual TblCommitmentVals check), since this currently reads as inconsistent and can confuse future maintenance/debugging.

Copilot · 2026-04-14T08:30:33Z

+			if err := dir.RemoveFile(dataPath); err != nil && !os.IsNotExist(err) {
+				continue
+			}


If dir.RemoveFile(dataPath) fails (e.g., permissions, transient IO error), the code silently continues, which can leave the node misaligned but cause alignStateToBlockSnapshots to stop when removedFiles.count == 0. Consider logging the error and/or propagating it (e.g., return an error from removeStateFilesFromStep / alignStateToBlockSnapshots) so misalignment isn’t silently left unresolved.

Copilot · 2026-04-14T08:30:34Z

+func TestAlignmentSkipsWhenTxNumsIndexExists(t *testing.T) {
+	// When the TxNums index already covers frozen blocks, alignment must
+	// NOT run — the node previously processed past any snapshot
+	// misalignment. Running alignment on restart would remove snapshot
+	// files that the downloader re-downloaded, cascading until all state
+	// files are gone.
+	//
+	// The guard checks: TxNums.Max(frozenBlocks) > 0. A full integration
+	// test requires a temporal DB with TxNums populated, which is beyond
+	// unit test scope. The guard is tested end-to-end via the
+	// fresh-start/restart cycle.
+


TestAlignmentSkipsWhenTxNumsIndexExists is misleading as written: (1) the production guard does not check TxNums.Max(frozenBlocks) (it checks TblCommitmentVals), and (2) the test doesn’t exercise the guard/alignment path at all (it only writes files and asserts they exist). Please rename/update this test and its comment to match the implemented guard, or replace it with a unit test that actually verifies the guard behavior (e.g., by mocking/setting up the DB state that should cause alignment to be skipped).

sudeepdino008 · 2026-04-14T09:21:27Z

alternative fix - #20546

awskii · 2026-04-15T08:39:32Z

+		logger.Info(fmt.Sprintf("[%s] state/block misalignment detected, removing step %d", logPrefix, highestStart),
+			"commitBlock", commitBlock, "frozenBlocks", frozenBlocks)
+
+		removedFiles := removeStateFilesFromStep(dirs, highestStart, logger, logPrefix)


imo this is just hiding root cause under the rug. No need to rewrite logic of SeekCommitment - there is already everything we need. Reading exact key from db, extracting block and txnum correctly. If we have something with computed commitment - we executed that block or some transactions out of the block already. BUT TxNum index is for some reason slightly behind commitment - maybe we forgot to do something or for some reason truncated this index?

awskii · 2026-04-15T08:40:56Z

+
+// findHighestStateFileStartStep finds the start step of the highest state
+// domain file. Returns (step, true) or (0, false) if no state files exist.
+func findHighestStateFileStartStep(dirs datadir.Dirs) (kv.Step, bool) {


everything is relying on some knowledge of current snap names and structures, while we have tested suite for operating with the snapshots.

awskii · 2026-04-15T08:41:38Z

+func removeStateFilesFromStep(dirs datadir.Dirs, fromStep kv.Step, logger log.Logger, logPrefix string) removedFilesResult {
+	result := removedFilesResult{}
+
+	for _, snapDir := range []string{dirs.SnapDomain, dirs.SnapIdx, dirs.SnapHistory, dirs.SnapAccessors} {


for removal we also have reliable tooling

awskii · 2026-04-15T08:42:28Z

+	}
+
+	// Strip removed step ranges from preverified.toml so the downloader
+	// doesn't re-fetch them on next startup.


so erigon seg reset would break node again, and that fix would "fix" node again?

AskAlexSharov · 2026-04-15T08:46:54Z

oh, i just noticed new potential problem: if we have only 1 big 0-8096.kv file and it's ahead of Blocks files - then we will filter out all state files and user will exec from 0.

no?

AskAlexSharov · 2026-04-16T07:58:26Z

i realised one more potential problem:
--prune.mode=minimal downloading a bit of state history - because .kv files can be at "middle of the block". If we remove 1 latest files - then minimal node will left with 0 history files.

so, if need align snaps boundaries - then need "pre-filter" files (after "header.seg" downloading, and before state files downloading) - not "post-filter" downloaded state files.

AskAlexSharov · 2026-05-22T06:51:52Z

let's close it. because it's old. also seems Sudeep fixed it another way (by checking Block's files progress before Agg.Collate). Feel free to reopen if need.

mh0lt requested a review from yperbasis as a code owner April 9, 2026 08:53

yperbasis requested review from AskAlexSharov, Copilot, sudeepdino008 and taratorio April 9, 2026 10:05

Copilot started reviewing on behalf of yperbasis April 9, 2026 10:06

View session

yperbasis requested a review from Giulio2002 April 9, 2026 10:08

yperbasis mentioned this pull request Apr 9, 2026

execmodule: tolerate ErrBehindCommitment in InsertBlocks #20429

Closed

2 tasks done

Copilot AI reviewed Apr 9, 2026

View reviewed changes

mh0lt force-pushed the fix/snapshot-alignment-check branch from a60511e to 899501e Compare April 9, 2026 10:15

sudeepdino008 requested changes Apr 9, 2026

View reviewed changes

mh0lt force-pushed the fix/snapshot-alignment-check branch from 899501e to 4417cf5 Compare April 9, 2026 12:15

mh0lt force-pushed the fix/snapshot-alignment-check branch from 79b6272 to 5cc82ff Compare April 9, 2026 21:15

Mark Holt and others added 7 commits April 12, 2026 16:27

snapshots: fix lint — use dir.RemoveFile, remove dead code

c546351

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

snapshots: add unit tests for alignment helpers

54910fc

Tests for parseFileStepRange, isStateDomainFile, findHighestStateFileStartStep, removeStateFilesFromStep, and the stageProgress guard that prevents alignment on restart. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

mh0lt force-pushed the fix/snapshot-alignment-check branch from 5cc82ff to f39ca2f Compare April 12, 2026 16:28

mh0lt mentioned this pull request Apr 14, 2026

[wip] snapshots: align state files to block file boundary on startup #20508

Closed

AskAlexSharov requested a review from Copilot April 14, 2026 08:25

Copilot started reviewing on behalf of AskAlexSharov April 14, 2026 08:26

View session

Copilot AI reviewed Apr 14, 2026

View reviewed changes

awskii requested changes Apr 15, 2026

View reviewed changes

AskAlexSharov closed this May 22, 2026

sudeepdino008 mentioned this pull request May 22, 2026

state collation strategy: capping or recovery #21366

Closed

4 of 5 tasks

Conversation

mh0lt commented Apr 9, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sudeepdino008 left a comment

Choose a reason for hiding this comment

Uh oh!

mh0lt commented Apr 9, 2026

Uh oh!

mh0lt commented Apr 9, 2026

Uh oh!

mh0lt commented Apr 9, 2026

Uh oh!

sudeepdino008 commented Apr 9, 2026

Uh oh!

mh0lt commented Apr 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AskAlexSharov commented Apr 10, 2026

Uh oh!

sudeepdino008 commented Apr 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Apr 14, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 14, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 14, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 14, 2026

Choose a reason for hiding this comment

Uh oh!

sudeepdino008 commented Apr 14, 2026

Uh oh!

awskii Apr 15, 2026

Choose a reason for hiding this comment

Uh oh!

awskii Apr 15, 2026

Choose a reason for hiding this comment

Uh oh!

awskii Apr 15, 2026

Choose a reason for hiding this comment

Uh oh!

awskii Apr 15, 2026

Choose a reason for hiding this comment

Uh oh!

AskAlexSharov commented Apr 15, 2026

Uh oh!

AskAlexSharov commented Apr 16, 2026

Uh oh!

AskAlexSharov commented May 22, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

mh0lt commented Apr 9, 2026 •

edited

Loading

sudeepdino008 commented Apr 11, 2026 •

edited

Loading