feat(repair): skip credit balances rebuild at boot when screens find no drift - #1226
feat(repair): skip credit balances rebuild at boot when screens find no drift#1226odesenfans wants to merge 7 commits into
Conversation
foxpatch-aleph
left a comment
There was a problem hiding this comment.
Well-designed and thoroughly tested PR that replaces unconditional per-boot credit_balances rebuild with cheap set-based screening. All three screens (structural, conservation, order-inversion) have sound logic verified against the actual writer semantics, with 19 tests covering edge cases including expiry bounce, overdraft, backdated expenses, and grant/grant inversions. The orchestration is crash-safe (state row advances only after rebuilds commit), raw SQL uses parameterized binds, and the migration follows the hex-ID convention. The residual blind spot (total-preserving misattribution) is honestly documented. No blocking concerns.
src/aleph/repair.py (line 261): The inversion screen's WHERE late.last_update > :watermark requires a sequential scan of credit_history since there is no index on last_update. For large deployments this could be slow at boot. Consider adding an index on credit_history.last_update in a follow-up migration (the existing get_updated_credit_balance_accounts accessor would also benefit). Non-blocking — this is still cheaper than the old unconditional full rebuild.
src/aleph/repair.py (line 174): The grant.amount < 0 condition catches lots pointing at a drain (negative) history row, but there is no explicit test for this specific case (a lot pointing at its own address's drain row). The cross-address test covers grant.address != AlephCreditBalanceDb.address, and the over-grant test covers amount_remaining > grant.amount (which would also fire for a negative grant), but a dedicated test for the grant.amount < 0 branch would make the coverage explicit.
src/aleph/repair.py (line 212): The conservation screen's min_running_sum window function scans the entire per-address history (necessary for correctness — the running sum must start from the first row). For very active addresses this is O(n) per address at every boot. Acceptable given the alternative was a full delete-and-replay, but worth noting if performance becomes a concern.
foxpatch-aleph
left a comment
There was a problem hiding this comment.
Well-architected PR that replaces an unconditional every-boot credit cache rebuild with cheap set-based screening. The three screens (structural, conservation, order-inversion) are sound: the conservation screen correctly handles the eager-writer vs. replay lower-bound discrepancy via min_running_sum, the inversion screen correctly uses last_update as an insertion-order proxy with strict watermark comparison, and the crash-safety property (state row advances only after all rebuilds commit) is correctly implemented. The migration chain is valid, the model and accessors are clean, and 19 tests cover each screen's flag/no-flag cases plus all orchestration paths. No bugs or security issues found. The only gap is a missing dedicated test for the backdated-expense conservation scenario (the soundness claim at lines 198-200 is tested only indirectly via the overdraft test).
src/aleph/repair.py (line 198): The comment claims min_running_sum < 0 covers backdated expenses, but there is no dedicated test in test_credit_repair.py that seeds a backdated expense (message_timestamp before the grant) and asserts the conservation screen does NOT flag it. The overdraft test (test_conservation_screen_allows_overdraft) exercises min_running_sum < 0 but for a different reason (overdraw). A dedicated test would directly validate this soundness claim.
src/aleph/repair.py (line 364): This line is long: session.execute(select(func.max(AlephCreditHistoryDb.last_update))).scalar(). PR states linting passes so it's within the project's configured limit, but wrapping it would improve readability.
Problem
_repair_credit_balancesunconditionally rebuilds thecredit_balanceslot cache for every address with credit history on every startup — the log line "Repairing credit_balances for 213 address(es)" means "rebuilding all 213 from scratch", not "found 213 problems". Slow boots, and genuine drift is silently overwritten instead of reported.Approach
The cache is a pure function of the append-only
credit_history, and both are written in the same transaction — so drift can only come from three causes, each detectable with a cheap set-based screen:REPAIR_POLICY_VERSIONstamp in a new single-rowcredit_repair_statetable → full rebuild only when it changes (or on bootstrap)last_updatewatermark, tuple comparison against replay order (credit_history.idis never populated, solast_updateis the insertion-order proxy)SUM(cache) == SUM(ledger)exactly where sound; one-sided>=where expiry-bounce/overdraft makes equality illegitimate)A clean boot is now 3 read-only SELECTs and logs
Credit balances clean, nothing to repair; detected drift logs at WARNING with per-screen counts and rebuilds only the flagged addresses. Crash-safe: the state row advances only after all rebuilds commit, so an interrupted repair re-screens idempotently next boot.Known residual (documented in the docstring): a total-preserving misattribution bug (right total drained from the wrong lot) is invisible to the screens until its effects change a total; only a full replay can see it.
Expiration needs no repair at all — reads already filter expired lots at query time.
Changes
deployment/migrations/versions/0065_c5e1a9d3f7b2_credit_repair_state.py— single-row bookkeeping table (policy version + history watermark)src/aleph/db/models/balances.py,src/aleph/db/accessors/balances.py—CreditRepairStateDbmodel + get/upsert accessorssrc/aleph/repair.py—REPAIR_POLICY_VERSION, the three screens, rewired_repair_credit_balances(same signature;repair_node/commands.pyuntouched)tests/db/test_credit_repair.py— 19 new tests against real PG: each screen's flag/no-flag cases (incl. expiry bounce, overdraft, backdated expenses, grant/grant inversions, watermark strictness) and the orchestration paths (skip-when-clean, bootstrap, policy bump, rebuild-only-flagged)Testing
tests/db/test_credit_repair.py: 19 passed;tests/db/test_credit_balances.py: 55 passed (regression)hatch run linting:all: ruff, black, isort, yamlfix, mypy all pass🤖 Generated with Claude Code