Harden disk SMT failover recovery

[jait91]

This PR hardens the disk-backed SMT HA path for crash/failover recovery.

Summary:

• Adds durable proposal recovery so restarted leaders can resume/finalize certified proposals without requiring an active in-memory round.
• Fixes disk SMT startup replay across finalized block gaps and avoids false root mismatches when the live tip advances during replay.
• Simplifies the disk precollector to the one-shot handoff path and removes dormant async prepare-ahead code.
• Skips redundant Mongo SMT-node writes in disk mode while keeping aggregator records as the durable recovery source.
• Tightens finalized/proof visibility and adds regression coverage for replay, recovery, duplicate/failover paths, and disk SMT persistence.

Operational note

This branch changes the disk-SMT durable storage model and Mongo indexes. It is intended to run from a clean local state unless a migration/backfill is added separately.

[gemini-code-assist]

Code Review

This pull request introduces significant changes to the BFT client and round management logic to support durable proposals, including improved recovery mechanisms, better error handling for root mismatches, and optimized SMT persistence. The reviewer identified a potential issue where an invalid durable proposal could wedge the system upon restart, and provided an actionable suggestion to abandon the proposal in such cases. I have kept this feedback as it highlights a critical improvement opportunity for system resilience.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

[Copilot]

Pull request overview

This PR hardens disk-backed Sparse Merkle Tree (SMT) high-availability recovery by shifting durable replay/recovery sources toward Mongo “aggregator_records + blocks” (proposal-scoped), tightening finality visibility semantics, and adding metrics + regression tests to protect failover/crash paths.

Changes:

• Reworks Mongo storage model/indexes for aggregator records (hashed stateId, {blockNumber, proposalId}) and removes block_records as a durable mapping source.
• Updates SMT root hashing semantics (raw root hex by default) while keeping legacy path proof compatibility via a test-only helper.
• Improves disk SMT persistence / replay / recovery paths and adds extensive regression coverage + new proof build latency metrics.

Reviewed changes

Copilot reviewed 55 out of 55 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
pkg/api/merkle_tree_path_verify_test.go	Adjusts proof-root assertions to align with updated root encoding behavior.
internal/storage/mongodb/test_helpers_test.go	Adds Mongo testcontainer helper for mongodb storage tests.
internal/storage/mongodb/index_test.go	Updates expected Mongo index set for aggregator records after storage model change.
internal/storage/mongodb/connection.go	Removes BlockRecordsStorage wiring and index creation.
internal/storage/mongodb/block.go	Adds block status/proposal fields, status migration, and “any finalization” retrieval helpers.
internal/storage/mongodb/block_records.go	Deletes Mongo block_records storage implementation.
internal/storage/mongodb/aggregator_record.go	Adds proposal-aware/finality-aware record queries; changes indexes to hashed + compound.
internal/storage/mongodb/aggregator_record_test.go	Updates/expands tests for proposal IDs, finality visibility, and duplicate handling.
internal/storage/interfaces/interfaces.go	Removes BlockRecordsStorage interface and storage accessor.
internal/smt/smt.go	Changes root hash getters to return raw roots; keeps legacy path support note.
internal/smt/smt_test.go	Updates golden roots to raw hex; adjusts legacy path root expectations.
internal/smt/smt_debug_test.go	Updates debug test to use legacy path root helper.
internal/smt/legacy_path_test.go	Adds helper to compute legacy (imprinted) root hex for legacy path tests.
internal/smt/inclusion_cert_test.go	Updates golden vectors / assertions to raw root hex semantics.
internal/smt/golden_vectors_test.go	Updates golden root vectors to raw root hex semantics.
internal/smt/disk/rocksstore/store.go	Refactors proof response key construction via a shared prefix helper.
internal/smt/disk/persist/tree.go	Improves snapshot forking over uncommitted overlays; reduces stats overhead; releases buffers on close.
internal/smt/disk/persist/tree_test.go	Updates tests to match append-only node semantics and new fork behavior.
internal/smt/backend/disk_backend.go	Refactors locking (commit stats RWMutex) and adds per-commit timing reporting.
internal/smt/backend/backend.go	Introduces CommitTimingProvider / CommitTiming for commit timing reporting.
internal/service/service.go	Adds inclusion-cert build timing metrics; hardens nil checks in proof path.
internal/service/service_test.go	Updates tests to match AggregatorRecord model changes (removes FinalizedAt).
internal/service/proof_diagnostics.go	Replaces proof path atomic logging with histogram-based inclusion-cert timing metrics.
internal/service/aggregate_test.go	Updates test model-to-API mapping expectations after record model change.
internal/round/smt_persistence_integration_test.go	Ensures durable proposal is stored before finalization in restart workflow test.
internal/round/round_process_regression_test.go	Adds regression for stale certification handling + finality visibility expectations.
internal/round/round_manager.go	Adds proposal IDs to rounds; makes mini-batch size configurable; refactors precollector lifecycle.
internal/round/precollector.go	Simplifies precollector to one-shot handoff with optional record staging + stats.
internal/round/precollection_test.go	Updates tests for new precollector behavior and adds staging + validation coverage.
internal/round/parent_round_manager.go	Adds activation context management and async StartNextRound behavior for parent mode.
internal/round/finalize_duplicate_test.go	Shifts duplicate recovery to durable proposal model; adds proposal ID semantics tests.
internal/round/disk_smt_startup.go	Replays disk SMT using finalized blocks + aggregator records (not block_records); improves replay correctness.
internal/round/disk_smt_startup_test.go	Updates disk startup tests to use aggregator-record replay model and cover “latest advances during replay”.
internal/round/disk_ha_failover_integration_rocksdb_test.go	Updates HA failover test to use aggregator records (proposal scoped) instead of block_records.
internal/round/disk_bft_integration_rocksdb_test.go	Updates BFT integration tests to validate persisted aggregator-record “block records” behavior.
internal/models/finality_status.go	Adds explicit finality status constants and normalization helpers.
internal/models/block.go	Adds Status and ProposalID fields to blocks and BSON conversions.
internal/models/block_record.go	Removes BlockRecords model.
internal/models/aggregator_record.go	Adds ProposalID, removes FinalizedAt, adjusts BSON serialization defaults.
internal/models/aggregator_record_test.go	Updates serialization test for AggregatorRecord model changes.
internal/metrics/metrics.go	Adds SMTInclusionCertBuildDuration histogram metric.
internal/ha/block_syncer.go	Switches replay to aggregator records and blocks; adds helper for next finalized block lookup.
internal/ha/block_syncer_test.go	Updates block syncer tests to store proposal-scoped aggregator records instead of block_records.
internal/config/config.go	Adds COLLECT_MINI_BATCH_SIZE configuration with env default + validation.
internal/config/config_test.go	Adds config validation and env override tests for collect mini-batch size.
internal/bft/client.go	Adds durable proposal resume/finalize/abandon flows; strengthens UC sequence checks and root validation.
internal/bft/client_stub.go	Makes stub lifecycle cancelable and goroutine-safe; avoids starting rounds after Stop.
internal/bft/client_stub_test.go	Adds tests for new durable proposal behaviors, UC sequencing, and local-root validation.
docs/disk-backed-smt-performance.md	Documents disk-backed SMT E2E performance checkpoints and config.
cmd/performance-test/types.go	Extends perf log parsing fields and ensures JSON-RPC params are encoded before sending.

---

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

[MastaP]

Review: Harden disk SMT failover recovery

Reviewed via a multi-agent pass (subsystem mapping → dimension finders → adversarial verification) plus a manual re-trace of the core paths. Findings below are deduped against the existing Copilot/Gemini comments — those are cross-referenced in a table at the bottom rather than repeated.

Recommendation: changes requested. Two issues can hard-wedge a node, and the migration gaps silently corrupt or hide data on any non-clean upgrade. Design is otherwise coherent (double root-check before finalize, finalizationMu over the commit+finalize window, proposalId-joined proof visibility).

---

🔴 Critical / blocking

1. Memory/Mongo backend: a crash mid-FinalizeBlock permanently wedges startup — internal/round/batch_processor.go:651–685, internal/storage/mongodb/block.go (GetUnfinalized), internal/round/round_manager.go:1189
In the default memory backend, storeDataParallel() writes block N's smt_nodes (:651) and SetFinalizedWithCertificate finalizes the block (:677) non-atomically; the block was written earlier as status="proposed". If the process dies in between:

• GetUnfinalized no longer returns proposed blocks (new finalizingBlockFilter), so RecoverUnfinalizedBlock does not recover block N.
• restoreSmtFromStorage rebuilds the root from all smt_nodes (GetChunked, no finalization filter) → root = N's root, but GetLatest() = N‑1 → the :1189 verification FATALs.

→ Startup fails on every subsequent boot until manual DB surgery. This is a regression (old GetUnfinalized matched all finalized:false blocks) and it triggers on clean state — precisely the crash-during-finalize scenario this PR is meant to harden.
Fix: make the smt_nodes write + finalize atomic (single txn), or restore only nodes belonging to finalized blocks, or reconcile a proposed block whose nodes are already persisted at startup.

2. Upgrade-in-place silently drops re-proposed records (legacy unique stateId_1 index never dropped) — internal/storage/mongodb/aggregator_record.go:354 (CreateIndexes)
CreateIndexes adds stateId_hashed + {blockNumber, proposalId} but never DropIndex the old unique stateId_1. The new model deliberately re-writes the same stateId under different proposalIds (propose → abandon → re-propose). On an upgraded DB the surviving unique index rejects re-proposed records, and StoreBatch swallows duplicate-key errors → records silently not inserted → recovery loads an incomplete leaf set → recomputed root ≠ certified root → finalization fails/wedges. The "clean state only" caveat is the only guardrail and nothing enforces it.
Fix: idempotently drop the legacy indexes in a migration, or hard-fail startup if a unique stateId index is detected.

---

🟠 High

3. proposalId is never backfilled → historical data breaks after upgrade — internal/storage/mongodb/aggregator_record.go, internal/ha/block_syncer.go:274 (GetByBlockNumber facet already raised by Copilot)
Old finalized blocks/records have no proposalId; {proposalId: ""} does not match field-absent docs in Mongo. Two impacts beyond Copilot's note:

• GetByBlockNumber returns zero records for every pre-migration block → get_block_records / totalCommitments read empty (while single getByStateIDFromFinalizedBlock still works via $expr $eq — an inconsistent split).
• HA disk failover replay (updateSMTForBlock) loads 0 leaves for a non-empty historical block → root mismatch → ErrDiskSMTDiverged aborts the sync.

Fix: backfill proposalId, or make the finalized loaders proposalId-agnostic when the block has none.

4. Certified-root mismatch can fail to halt the node — internal/bft/client.go:561–582 (distinct from Gemini's now-fixed comment)
A certified root mismatch is a consensus-safety violation that must stop the node. The branch now calls abandonProposedBlockLocked first and return abandonErr on failure — before publishing TopicFatalError. A transient Mongo error in abandon (read + root-equality guard + txn write) downgrades a guaranteed-fatal event to a logged line, with luc/lastRootRound already advanced and proposedBlock left non-nil.
Fix: always Inc() + publish the fatal event + return ErrCertifiedRootMismatch, regardless of whether abandon succeeds.

5. ParentRoundManager WaitGroup race on leadership transition — internal/round/parent_round_manager.go:199 vs :549
StartNextRoundFromPrecollector reads activeCtx under roundMutex, releases it, then calls roundWG.Add(1) outside the lock. Deactivate flips activeCtx=nil under the lock then calls roundWG.Wait() without it. Interleaving: reader sees non-nil → Deactivate runs fully and Wait() returns at 0 → reader Add(1) after Wait(). This is sync.WaitGroup misuse (race-detector flag) and breaks Deactivate's "all round work joined" invariant during HA failover. The child RoundManager does this correctly under lock — mirror that pattern.

6. Abandoned-proposal records leak unbounded — internal/round/recovery.go:183 (AbandonDurableProposal)
The comment promises records are "left for lazy cleanup," but no cleanup exists (no TTL, no DeleteMany, no sweeper). Records are staged before every cert request and abandoned on repeat-UC / supersession / round-mismatch / newer-UC / root-mismatch → the largest collection grows forever, bloating both indexes and the join-based finalized lookups.
Fix: DeleteMany({blockNumber, proposalId}) inside the abandon txn (records are scoped to that attempt), or a TTL/sweeper; at minimum fix the misleading comment.

---

🟡 Medium

• Resumed proposal stall (bft/client.go:600–625): if FinalizeCertifiedProposal returns recovered=false on the resumed path, it returns a hard error without abandoning or starting a round — the round loop stalls until the next UC. The no-proposedBlock branch handles the same case benignly; make them consistent.
• FinalizeCertifiedProposal refuses a certified abandoned block (recovery.go:34–50): block is looked up by number only; if a prior attempt at that index is abandoned, the status gate returns false even when BFT certified an equivalent-root proposal. Confirm no path leaves the index abandoned while BFT certified it.
• Recovery methods absent from storage interfaces (recovery.go): *AnyFinalization / SetFinalizedWithCertificate / SetStatus are reached via runtime type-assertion with silent no-op/fallback — a missing impl compiles fine and silently degrades. Put them on the interfaces.
• Proof read-path regression (aggregator_record.go:188, hot caller service.go:345): replaced a covered unique-index FindOne with a per-request $lookup aggregation.
• RocksDB node CF is now grow-only (internal/smt/disk/persist/tree.go:829): rebuildOverlay no longer emits tombstone deletes.
• Blanket finalized=true status migration (block.go:243) can stamp a never-certified legacy block as finalized.
• Precollector snapshot leak if SetCommitTarget fails in StartNextRoundFromPrecollector (round_manager.go:1592).

---

Test coverage gaps

• The disk-mode crash window (the PR's central claim) is untested — Test04c uses a memory SMT, so the disk ordering branch of FinalizeCertifiedProposal never runs.
• Disk-startup tests exercise the interface fallback paths, not the production *AnyFinalization/SetFinalizedWithCertificate paths (disk_smt_startup_test.go fakes) — the new production code isn't actually covered.
• block.go status migration / SetStatus / SetFinalizedWithCertificate / new GetUnfinalized filter have no tests (block_test.go unchanged).
• Resumed-proposal wedge path and the index migration (lingering unique index) have no regression tests.

---

Already covered by Copilot / Gemini (cross-referenced, not repeated)

Source	Location	Note
Gemini	bft/client.go cert mismatch — abandon durable proposal	Already fixed in 06084bd; see #4 for the remaining gap
Copilot	block_syncer.go:260 — nextFinalizedBlock O(N²) GetRange	Confirmed
Copilot	aggregator_record.go:260 — empty-proposalId query misses old records	Confirmed; #3 extends impact to HA replay
Copilot	round_manager.go:682 — startup precollector wired with nil blockNumber (first handoff stages no durable records)	Confirmed at :137
Copilot	test_helpers_test.go:30/38/42 — testcontainer leak on Fatalf before cleanup	Valid (test hygiene)

Strengths

• Double root verification (stored vs recomputed vs certified) before finalize.
• Backend-asymmetric commit ordering is deliberate and the disk path is genuinely protected by startup replay (a tempting "disk masks SMT behind until restart" hypothesis was investigated and ruled out — the memory path Initial application #1 is the real risk).
• Non-equivocation UC check, finalizationMu discipline, and proposalId-joined proof visibility are solid; regression breadth is good even where depth is missing.

[jait91]

@MastaP
Fixed the non-disk finalize crash window by marking certified blocks as finalizing with the UC before SMT-node writes. Also fixed cert-root mismatch fatal publishing, parent activation roundWG race, precollector SetCommitTarget cleanup, abandoned-record scoped cleanup, and added fast-fail detection for the legacy unique stateId index.

This PR still intentionally assumes a clean DB; no in-place migration/backfill is included.

[MastaP]

Re-review: feedback addressed + regression check

Re-reviewed the four fix commits since the first pass (06084bd5..539c0bb4). Each "addressed" claim and each candidate regression was independently re-traced against the PR tip (multi-agent dimension finders → adversarial verification, then a manual re-trace of the core paths).

Recommendation: close to mergeable. All blocking items from the first review are genuinely fixed and well-tested; no blocking regressions were introduced by the fixes. Two non-trivial items remain (one is a footgun in the new guard's guidance).

---

1. Feedback status

Status	Items
✅ Addressed	memory-backend crash wedge, legacy unique stateId index, cert-mismatch fatal halt, parent WaitGroup race, abandoned-record leak, certified-abandoned handling, status migration, precollector snapshot leak, O(N²) catch-up, startup nil blockNumber, testcontainer cleanup
⚠️ Partial	proposalId backfill / guard wording; recovery methods on storage interfaces
❌ Not addressed	resumed-proposal recovered=false stall; proof $lookup hot path; grow-only RocksDB node CF

Spot-checks that confirm the criticals hold end-to-end:

• Crash wedge: non-disk FinalizeBlock now flips proposed → finalizing (persisting the UC, finalized=false) before the smt_nodes write; finalizing is in the unfinalized filter and startup recovery runs before the SMT root-verify, so a crash-in-the-window block is recovered and finalized first. Regression test added and it fails if the fix is reverted.
• Legacy index: ensureNoLegacyUniqueStateIDIndex hard-fails startup if a unique single-key stateId index exists. Test added.
• Cert mismatch: abandon failure is now only logged; the branch always increments the error metric, publishes the fatal event, and returns the mismatch error.

2. Regressions introduced by the fixes

No blocking regressions. Of the candidate issues surfaced, all but one were refuted under verification (not triggerable / actually an improvement / the new test does catch its regression). One survives, at low severity:

🟡 Low — asymmetric, un-interfaced finalizing marker (internal/round/recovery.go, setBlockFinalizingWithCertificate)
This helper is called first on every non-disk finalize and reaches the backend only via an optional type-assertion. On a miss it hard-errors with no fallback — unlike the sibling setBlockFinalizedWithCertificate, which falls back to SetFinalized. And SetFinalizingWithCertificate was not added to the interfaces.BlockStorage interface (the sibling commit did add GetNextFinalizedAfter there). Not a live bug — the only BlockStorage implementation has the method — but it's a compile-invisible landmine: existing test doubles satisfy the updated interface without it and still compile. Same root cause as the interface-gap item below.

(Trivial: README.md still documents SMT_MATERIALIZE_WORKERS default as 64 after the tuning commit changed it to 16 — benign doc drift.)

Two things worth doing before merge

1. Reword the legacy-index guard. Its message advises "drop the unique stateId index or recreate the database." Dropping the index alone gets an operator past the guard and into the still-unbackfilled proposalId corruption (GetByBlockNumber returns empty for pre-migration blocks; HA disk replay diverges). Only "recreate the database" is safe — drop the "drop the index" suggestion, or add the proposalId backfill so it's actually safe.

2. Promote recovery methods onto the storage interfaces. SetFinalizingWithCertificate, DeleteByBlockNumberAndProposalID, and SetStatus are reached by runtime type-assertion. Putting them on the interfaces turns the gaps into compile errors — this makes the new asymmetric helper fail loudly at build time and prevents the abandon-cleanup (the record-leak fix) from silently no-op'ing if a backend ever misses a method.

The remaining unaddressed items (resumed-proposal stall, proof $lookup per request, grow-only node CF) are genuine but non-blocking. Nice work on the list — the fixes are coherent and the regression coverage is solid.