fix(sync): record snap progress per backend

[batrr]

Changes

If a flat node stops during healing after snap sync, after restart it comes back as a patricia node.

Snap sync stores its range progress in the state DB, whatever the backend is:

// ProgressTracker, injected with [KeyFilter(DbNames.State)] IDb db
_db.PutSpan(ACC_PROGRESS_KEY, ValueKeccak.MaxValue.Bytes, WriteFlags.DisableWAL);

and the same DB is what decides whether flat is active:

// FlatStateActivationPolicy.DecideBackend -> patricia
if (patriciaStateDb.Value.GetAllKeys().Any())

The fix stores the progress per backend: patricia keeps it in the state DB as before, flat stores nothing because it wipes its DB on every run.

Types of changes

What types of changes does your code introduce?

• Bugfix (a non-breaking change that fixes an issue)
• New feature (a non-breaking change that adds functionality)
• Breaking change (a change that causes existing functionality not to work as expected)
• Optimization
• Refactoring
• Documentation update
• Build-related changes
• Other: Description

[claude]

Claude finished @batrr's task in 9m 41s —— View job

---

Review: fix(sync): record snap progress per backend

• Gather context (base diff via HEAD~2, matches the PR's 163/-57)
• Review ISnapTrieFactory / PatriciaSnapTrieFactory / FlatSnapTrieFactory
• Review ProgressTracker and SnapSyncRunner changes
• Verify DI wiring and all call sites
• Review tests
• Post review

Verdict: no Critical, High, or Medium findings. Looks correct and mergeable. 4 Low notes, all posted inline; the last three are optional.

What I verified

The diagnosis holds and the fix is the right shape. Some things I checked rather than assumed:

• The fix is structural, not just behavioural. FlatSnapTrieFactory now has no state-IDb dependency at all, so the flat backend cannot write into the patricia state DB — that's stronger than a test. PatriciaSnapTrieFactory keeps the exact same key ("AccountProgressKey"), same value (MaxValue), same DisableWAL + Flush, and the same read semantics (Length == 32 && == MaxValue), so existing patricia DBs read back unchanged. The old else branch that logged partial progress was dead — only MaxValue/absent are ever stored — so nothing was lost by dropping it. No references to the old ProgressTracker.ACC_PROGRESS_KEY remain anywhere.
• DI still resolves the decider, not the raw patricia factory. SynchronizerModule (which registers ISnapTrieFactory → PatriciaSnapTrieFactory) is loaded via NetworkModule before WorldStateDbDeciderModule in NethermindModule, so the policy-based registration still wins last — the fix does take effect on flat nodes. The new [KeyFilter(DbNames.State)] IDb resolves because the container DSL applies attribute filtering (same mechanism SnapProvider's [KeyFilter(DbNames.Code)] already relies on), and TestSynchronizerModule loads DbModule, so the container-based SnapProviderTests keep resolving.
• No new resolution cycle or premature initialization. ProgressTracker's new ISnapTrieFactory dependency forces the backend decision at tracker construction, but SnapProvider — the only thing that resolves the tracker — already depended on ISnapTrieFactory, so nothing resolves earlier than before.
• Moving the load out of the constructor is safe today. See the inline note on SnapSyncRunner.cs:22: the dispatcher is the only path to IsFinished, and it runs after LoadProgress(). Also fine when Sync.SnapSync=false or when StateSyncRunner early-returns on an existing full state — the snap feed is never dispatched in either case, so the unloaded tracker is never queried.

Findings

#	Severity	Where	Note
1	Low	ISnapTrieFactory.cs:20-21	New members are abstract; default impls (=> false / { }) would match the documented semantics, drop the flat/test boilerplate, and not break out-of-tree implementers. Trade-off discussed inline. Also prefer XML docs over // for member contracts.
2	Low	SnapSyncRunner.cs:22	Load-before-dispatch is now a temporal invariant rather than a structural one. Not currently reachable as a bug; a once-guarded lazy load inside ProgressTracker would restore the guarantee and avoid the new public LoadProgress() + ctor dependency.
3	Low	SnapProvider.cs:325	Extra MemDb allocated per refreshed-account response for a slot that is never read or written; a static readonly instance suffices.
4	Low	ProgressTracker.cs:188	The flag is re-recorded (Put + Flush) on every idle IsFinished(), including right after a restart that just read it back. Pre-existing shape, cheap to make write-once now.

One question on the upgrade path

The fix stops the new writes, but nothing removes an AccountProgressKey that a pre-fix flat run already left in the patricia state DB. Such a node still trips FlatStateActivationPolicy.DecideBackend's patriciaStateDb.Value.GetAllKeys().Any() check on its next restart and flips to patricia once more. For a node that already flipped and started writing patricia trie nodes there's nothing to recover, so this may well be intended — worth a line in the PR description either way, or a Remove in the policy if affected nodes are meant to heal on upgrade.

Tests

Coverage is good and well targeted: the drain→mark path, the skip and no-skip branches of LoadProgress, cross-instance read-back for patricia, and non-persistence for flat. Pinning the on-disk key as a literal in PatriciaSnapTrieFactoryTests with a comment explaining why it's duplicated is exactly right for a format-compatibility test. SnapSyncRunnerTests asserting order through the factory stub is a bit indirect (IsRangePhaseFinished recorded as "LoadProgress"), but it does pin the ordering that matters.

One note: FlatSnapTrieFactoryTests.RangePhase_NeverCarriesIntoTheNextRun asserts a constant, so it becomes a test of the interface default if you take finding #1.

Caveat: builds and dotnet test are blocked in this environment (no network/build permission), so the above is static analysis — I did not compile or run the suite. Everything I could check by reading (call sites, DI ordering, keyed-DB availability in every container that resolves these types) is consistent.
• branch fix/snap-progress-per-backend

[claude]

Claude finished @batrr's task in 4m 29s —— View job

---

Review: fix(sync): record snap progress per backend

• Gather context — full diff vs origin/master
• Re-check the four prior Low findings against the author's responses
• Review correctness / edge cases / regressions
• Review DI wiring and call sites
• Review tests
• Post review

Verdict: no Critical, High, or Medium findings. Mergeable. Re-reviewed after address review comments + the master merge; all four earlier Low notes are now either fixed or answered with reasoning I accept.

Prior findings, resolved

#	Where	Status
1	ISnapTrieFactory defaults	Closed — your argument is the right one. The existing EnsureInitialize() { } / FinalizeSync() { } defaults describe a backend that keeps its store, so defaulting IsRangePhaseFinished() to false would tell exactly that kind of backend it has no progress. Forcing each backend to state its answer is correct here.
2	SnapSyncRunner load ordering	Closed. Keeping the call next to EnsureInitialize() in the one place that owns the run lifecycle is a fair trade, and the failure mode really is re-requesting fetched ranges, not skipping data. Re-verified on the merged tree: SimpleDispatcher<SnapSyncBatch> is constructed only in Synchronizer.cs:451 and consumed only by SnapSyncRunner, and ISnapProvider.IsSnapGetRangesFinished()/CanSync() have no production caller outside the tracker/provider pair — so nothing can observe the tracker before LoadProgress().
3	SnapProvider.cs:325 allocation	Fixed, and NullDb.Instance is the better call than my suggestion. Verified: NullDb.Get returns null (no throw), so IsRangePhaseFinished() would be safely false; Set throws but is unreachable — SnapProviderHelper.VerifyAccountRange only calls CreateStateTree(), and grep confirms the two new members are called from ProgressTracker alone. Your point about the catch (Exception) in VerifyRefreshedAccount swallowing it either way is also correct.
4	ProgressTracker.cs:188 repeated write	Acceptable as-is on closer reading. MarkRangePhaseFinished() is only reached when IsSnapGetRangesFinished() is true — which requires all four active-request counters to be zero — and that same call then returns true and ends the dispatcher. So it's ~one Put+Flush per run, not per idle poll. Withdrawing the suggestion.

What I verified this pass

• The diagnosis is complete for the reported symptom. I checked what else could leave a key in the patricia state DB during a flat snap sync + heal: ITreeSyncStore resolves to FlatTreeSyncStore on a flat node (so PatriciaTreeSyncStore never writes), and StateBoundaryStore only ever Removes from the state DB. The progress key really does look like the sole writer that trips FlatStateActivationPolicy's patriciaStateDb.Value.GetAllKeys().Any().
• Format compatibility is preserved. Same key bytes ("AccountProgressKey"u8), same value (MaxValue), same DisableWAL + Flush, same read predicate (Length: 32 && == MaxValue). The dropped else branch was dead — only MaxValue/absent were ever stored. No references to the removed ProgressTracker.ACC_PROGRESS_KEY remain.
• No eager DB touch on flat nodes. WorldStateDbDeciderModule.cs:53 resolves the patricia factory through Func<PatriciaSnapTrieFactory>, so the new [KeyFilter(DbNames.State)] IDb parameter is never resolved when flat wins — the added dependency can't open the patricia DB on a flat node.
• Every construction site has a keyed State IDb. All five sites checked: Synchronizer.cs:444 and PruningTrieStoreModule.cs:107 (prod, DbModule present), StateSyncFeedTestsBase.cs:137 (registers it explicitly), TestSynchronizerModule (loads DbModule), and the direct new in SnapProvider. WithAttributeFiltering() is applied by the container DSL, which is the same mechanism ProgressTracker's old [KeyFilter(DbNames.State)] relied on.
• LoadProgress() is idempotent across repeated runs. It early-returns unless the flag is set, and the clearing branch only runs when phase 1 genuinely completed, so a second SnapSyncRunner.Run (or a mid-phase-2 restart) can't discard legitimately re-queued partitions. It's also unreachable when Sync.SnapSync=false, which is fine — nothing queries the tracker on that path.

Remaining notes (both optional)

Low — upgrade path for already-affected nodes. Still worth a line in the PR description. A node that hit this bug on a pre-fix build has an AccountProgressKey sitting in its patricia state DB; on the fixed build DecideBackend still sees GetAllKeys().Any() == true and picks patricia. If it already flipped once and wrote patricia trie nodes there's nothing to recover and staying patricia is the right answer — but if it stopped before that first flip, this PR alone doesn't heal it. Either state that explicitly, or Remove the key in the policy. Scope call; I'd lean toward just documenting it.

Low — trivia at ProgressTracker.cs:464: _logger.Info($"Snap - State Ranges (Phase 1) is finished.") has no interpolation holes, so the $ is redundant. Carried over verbatim from the old GetSyncProgress, so leaving it is also defensible.

Tests

Good coverage, well targeted: drain→mark, both branches of LoadProgress, cross-instance read-back for patricia, non-persistence for flat. Pinning the on-disk key as a literal in PatriciaSnapTrieFactoryTests with a comment explaining why it's duplicated is exactly the right instinct for a format-compatibility test. FlatSnapTrieFactoryTests.RangePhase_NeverCarriesIntoTheNextRun is close to a tautology, but since you kept the interface members abstract it does pin the flat contract, so it earns its place.

Caveat: dotnet build / dotnet test are not permitted in this environment, so the above is static analysis — I did not compile or run the suite. Everything checkable by reading (call sites, DI ordering, keyed-DB availability in every container that resolves these types, NullDb semantics) is consistent.
• branch fix/snap-progress-per-backend

[flcl42]

Found 2 medium-severity issues in src/Nethermind/Nethermind.State.Flat/Sync/Snap/FlatSnapTrieFactory.cs and src/Nethermind/Nethermind.Synchronization/SnapSync/ISnapTrieFactory.cs.

[batrr]

@claude re review

[claude]

Claude finished @batrr's task in 8m 47s —— View job

---

Re-review: fix(sync): record snap progress per backend

• Gather context — full diff 2edb7ab...HEAD (includes address review comments + the master merge + remove range phase methods from ISnapTrieFactory and FlatSnapTrieFactory)
• Re-check every prior finding
• Review correctness / edge cases / regressions
• Review DI wiring and call sites
• Review tests
• Post review

Verdict: no Critical, High, or Medium findings. Mergeable. Third pass; every earlier finding is now fixed or answered with reasoning I accept. One new Low, posted inline.

What changed since the last pass

The interface members gained default bodies and FlatSnapTrieFactory dropped its explicit overrides:

bool IsRangePhaseFinished() => false;
void MarkRangePhaseFinished() { }

I checked this against both objections it was resolving, and it holds:

• flcl42's binary/source-compat concern is addressed. Out-of-tree ISnapTrieFactory implementers keep compiling and keep working.
• Your original objection is also satisfied, which is the non-obvious part. You argued a false default would tell a store-keeping backend it has no progress. It would — but that is the fail-safe direction: false means "request the ranges again", never "skip ranges over data we don't have". MarkRangePhaseFinished() { } likewise fails toward re-doing work. A backend that forgets to override loses resume, not correctness. Making the members abstract would have traded that for a compile break, which is the worse side of the trade here.
• Flat is provably correct under the default. FlatSnapTrieFactory.EnsureInitialize() calls persistence.Clear() unconditionally (FlatSnapTrieFactory.cs:24-28) — flat wipes on every run, with no conditional-resume path — so false is not merely safe, it's the accurate answer.
• The substitutes still intercept. IsRangePhaseFinished() being a default interface member does not break Substitute.For<ISnapTrieFactory>() / .Returns(...): EnsureInitialize/FinalizeSync were already DIMs and SnapSyncRunnerTests already asserts they are recorded on master, so Castle's DIM interception is demonstrably working in this project.

Prior findings — final status

#	Where	Status
1	ISnapTrieFactory defaults	Fixed (defaults added; reasoning above).
2	SnapSyncRunner load ordering	Closed, re-verified on this tree. ISnapProvider has exactly one production consumer (SnapSyncFeed), which touches IsFinished only from PrepareRequest — dispatcher-driven. ISnapSyncRunner.Run has one call site (StateSyncRunner.cs:52), once per process. Nothing can observe the tracker before LoadProgress().
3	SnapProvider.cs:325 allocation	Fixed with NullDb.Instance. Verified NullDb.Get returns null rather than throwing (NullDb.cs:24), so IsRangePhaseFinished() is safely false; Set throws but is unreachable.
4	ProgressTracker.cs:188 repeated write	Withdrawn. IsSnapGetRangesFinished() requires all four active-request counters at zero, and returning true ends the dispatcher — ~one Put+Flush per run.
5	Upgrade path (flcl42 + mine)	Acknowledged with rationale; correct, and I can now confirm no regression — see below.

On the upgrade path, precisely

You said such a node "still works correctly, just on the wrong backend." That's right, and here is the exact mechanism, which is worth having on the record: the stale AccountProgressKey makes DecideBackend pick patricia, and then PatriciaSnapTrieFactory.IsRangePhaseFinished() reads true against a patricia state DB that holds only that key and no trie nodes — so LoadProgress() clears every account-range partition and snap phase 1 is skipped entirely. The node then downloads the whole trie through the phase-2 state-sync feed. Slow, but correct.

The part that settles it: this is byte-for-byte the pre-PR behaviour for that node — the old ProgressTracker read the same key from the same DB in its constructor and skipped phase 1 the same way. So the PR neither causes nor worsens it; it only fails to heal a pre-existing on-disk state. No objection to leaving it.

What I verified this pass

• No new eager resolution or DI cycle. ProgressTracker's new ISnapTrieFactory dependency has only two consumers (SnapProvider, SnapSyncRunner), both of which already depended on ISnapTrieFactory. WorldStateDbDeciderModule.cs:53 resolves patricia through Func<PatriciaSnapTrieFactory>, so the new [KeyFilter(DbNames.State)] IDb parameter is never resolved on a flat node — the added dependency cannot open the patricia DB there.
• Keyed-DB availability at every site. WithAttributeFiltering() is applied by CommonNethermindConfig() (ContainerBuilderExtensions.cs:490-493), which the AddSingleton DSL routes through, so [KeyFilter] resolves for PatriciaSnapTrieFactory wherever it's registered. All five construction sites checked: Synchronizer.cs:444, PruningTrieStoreModule.cs:107, WorldStateDbDeciderModule.cs:53, StateSyncFeedTestsBase.cs:140 (registers AddKeyedSingleton<IDb>(DbNames.State, …) at line 137), and the direct new in SnapProvider.
• Format compatibility. Same key bytes, same value, same DisableWAL + Flush, same read predicate, and the same [KeyFilter(DbNames.State)] IDb the old tracker used — so existing patricia DBs read back unchanged. The dropped else branch was dead (only MaxValue/absent were ever stored). No reference to the removed ProgressTracker.ACC_PROGRESS_KEY survives.
• SnapSyncRunner's two constructors still bind unambiguously. Both now take three parameters, but Func<CancellationToken, Task> is not a registered service, so only the SimpleDispatcher overload is bindable — the same tie that already existed at two parameters.
• Usings and API surface. Nethermind.Core is still needed in ProgressTracker (BlockHeader), System.Linq still used (line 550). New test file: BytesToArray() is an instance method on Hash256 (Nethermind.Core.Crypto), NodeStorage is in Nethermind.Trie, Assert.EnterMultipleScope() is used elsewhere in the repo — all imports present.

Remaining Low notes

1. FlatSnapTrieFactoryTests.cs:73 — posted inline. The test now exercises only the interface default; still useful as a guard, but the intent isn't visible without a comment.
2. ISnapTrieFactory.cs:18-20 — the contract for these members is in // comments; repo rules prefer XML <summary>/<remarks> for member-level docs. The content is good (it already spells out the non-persisting default), just the wrong tag.
3. ProgressTracker.cs:464 — _logger.Info($"Snap - State Ranges (Phase 1) is finished.") has no interpolation holes, so the $ is redundant. Carried over verbatim from the old GetSyncProgress, so leaving it is defensible.

Tests

Coverage is right on target: drain→mark, both branches of LoadProgress, cross-instance read-back for patricia, non-persistence for flat, and ordering through SnapSyncRunner. Pinning the on-disk key as a literal in PatriciaSnapTrieFactoryTests with a comment explaining why it's duplicated is exactly the right instinct for a format-compatibility test.

Caveat: dotnet build / dotnet test are not permitted in this environment, so this is static analysis — I did not compile or run the suite. Everything checkable by reading (call sites, DI ordering, keyed-DB availability, NullDb semantics, namespace/import resolution for the new test file) is consistent.
• branch fix/snap-progress-per-backend