feat(indexer): reconcile in-memory tree from DB on rollback

[kilianglas]

Summary

Replace per-leaf rollback history-based rollback mechanism of indexer in-memory tree with DB-based rollbacks. This makes the logic simpler and overall more robust.

[kilianglas]

/otto review

[agentotto]

I've kicked off a thorough review of PR #748 — the subagent is cloning the repo, analyzing the diff, and will post inline comments directly on the PR along with a summary. I'll let you know once it's done.

[agentotto]

Review posted with inline comments.

[agentotto]

Now that _event_id is unused in both TreeState::apply_leaf and VersionedTreeState::set_leaf_at_index, might be worth a follow-up to remove the parameter from the TreeApplier trait and all callers to reduce API surface noise. Not blocking for this PR — just a thought for cleanup.

[kilianglas]

/otto remove this

[agentotto]

Review Summary

Overall: Looks good 👍

This is a well-motivated simplification. Replacing the bounded per-leaf in-memory history with a DB-based reconciliation after rollback is strictly more robust — it eliminates the RollbackHistoryPruned failure mode and removes the TREE_MAX_BLOCK_AGE config knob. The DB is already the source of truth, so reading back from it after a rollback is the natural approach.

What I liked:

• Significant code reduction (~300 lines net removed) while making the system more correct.
• The new reconcile_tree_from_db is straightforward and easy to reason about.
• Good test coverage: the new test_reconcile_tree_matches_db_state_after_rollback integration test verifies end-to-end consistency against a freshly-built-from-DB tree, which is a strong assertion.
• Clean removal of the old history infrastructure (LeafVersion, LeafLog, VecDeque, AtomicU64, the Arc<VersionedTreeStateInner> indirection) and corresponding config/error variants.

One thing to verify: since VersionedTreeState now holds TreeState directly (not behind its own Arc), confirm that all clone-sites still get the shared-state semantics they expect. I checked and TreeState already wraps an Arc<TreeStateInner> internally, so .clone() on VersionedTreeState correctly shares the underlying tree. ✅

See inline comments for minor items (none blocking).

[agentotto]

The review is complete. Here's what was done on PR #748:

Verdict: Looks good — a solid net simplification (~300 lines removed) that makes the rollback mechanism more robust by eliminating the RollbackHistoryPruned failure mode.

5 inline comments were posted:

1. Perf suggestion — batch the individual get_account queries in reconcile_tree_from_db instead of issuing them one by one
2. Positive note on the robustness improvement of the DB-based approach
3. Observation — DB reads in reconcile_tree_from_db happen outside the serializable transaction; safe given current design but worth a comment
4. Bug — README step numbering has a duplicate 2. (should be 1.)
5. Cleanup suggestion — the now-unused event_id parameter in TreeApplier could be removed in a follow-up

A brief top-level summary was also posted on the PR.

[agentotto]

Fixed — pushed a commit that corrects the duplicate 1. to 2. for the "Run Postgres" step in the Developing Locally section of services/indexer/README.md.

[kilianglas]

/otto review again

[agentotto]

Re-review kicked off — the subagent is doing a fresh pass on the updated PR, skipping already-addressed comments. I'll update you when it's done.

[agentotto]

Nit/observability: since this is a pub function that could be called from different contexts, consider adding a tracing::info! at the top (and on completion) to make rollback debugging easier:

tracing::info!(affected_leaves = affected_leaf_indices.len(), ?target_event_id, "reconciling in-memory tree from DB");

rollback_to_event logs the affected count already, but those logs appear before the DB commit, while this function runs after — having both helps confirm the commit succeeded and reconciliation started/finished.

[agentotto]

Review Summary

This is a solid simplification. Replacing the bounded per-leaf history rollback with a DB-based reconciliation eliminates the RollbackHistoryPruned failure mode entirely — rollbacks now always succeed as long as the DB is consistent, regardless of how far back they need to go.

Correctness: I traced through the full rollback flow (rollback_to_event → commit → reconcile_tree_from_db) and the logic is sound. affected_leaf_indices correctly captures all leaves whose accounts table state differs from the in-memory tree post-rollback, and the reconciliation correctly reads from the committed DB state (or zeros out removed accounts). The VersionedTreeState simplification is safe since TreeState already wraps an Arc<TreeStateInner>, so the extra Arc layer was redundant.

Test coverage: The new test_reconcile_tree_matches_db_state_after_rollback test is well-designed — it covers an account update rollback, an unmodified account, and a fully removed account, and cross-checks the reconciled tree root against a fresh tree built entirely from the DB.

Two minor observations (non-blocking):

1. VersionedTreeState name is now slightly misleading — it no longer versions anything, its sole added value over TreeState is simulate_root. Could be worth a rename to something like SimulatingTreeState in a follow-up if you agree.

2. The relay/src/cli/mod.rs change (making log_wallet_status swallow errors) is a good resilience improvement but is unrelated to the indexer rollback scope — consider splitting it to a separate PR for cleaner git bisect history.

LGTM overall. 👍

[agentotto]

Re-review is done. Here's the summary:

Overall verdict: LGTM — the core rollback logic is correct. The subagent traced through the full rollback flow and confirmed:

• affected_leaf_indices correctly captures all leaves needing reconciliation
• The DB read-after-commit pattern is safe under the single-writer model
• The Arc removal from VersionedTreeState is safe since TreeState already wraps Arc<TreeStateInner>

New inline comment posted:

• Observability — suggested adding tracing::info! in reconcile_tree_from_db to log when reconciliation starts/finishes, which would aid rollback debugging

Non-blocking observations in summary:

• VersionedTreeState naming is now a misnomer — it no longer versions anything; its main role is simulate_root
• The log_wallet_status relay change is a good fix but is unrelated to the PR scope — could be split into a separate PR for a cleaner git history