feat(indexer): balance ledger and address activity log (CON-809) - #3588
Conversation
Add the L-4 write-path foundation: a 0002 migration adding the (height, event_index) idempotency key and tx_index to balance_changes, the account_txs address-activity table, and a staking balance reason. The block decoder now captures coin/transfer/mint/burn/slash events with base64-normalized attributes and derives per-tx signer addresses, the enabler for deriving balances from events. Genesis ledger rows move to initialHeight-1 so they never collide with block initialHeight's events.
Add the pure, DB-free derivation layer: parseCoins for coin strings, a module-address registry deriving Cosmos SDK module accounts by name (sha256(name)[:20]), an MVP reason classifier, and the balance deriver that turns coin_spent/coin_received into ordered movements with a deterministic block-wide event_index and correlated counterparties.
Add the account interner (resolve-or-create, no permanent cache) and the balance writer. The writer seeds each running balance from the ledger baseline (not the snapshot), appends balance_changes on the (height, event_index) unique key, and advances account_balances additively by only the rows it actually inserted — so overlapping sync/backfill writers apply every delta exactly once and re-commits are no-ops.
Wire balance derivation into the block committer: derive changes and address-activity rows per block, intern all touched addresses on the base connection, then inside the commit transaction write the balance ledger and account_txs after messages and before the checkpoint. Both sync and backfill flow through commitBatch, so both persist the ledger.
Add abci_query historical support to the RPC pool and a one-shot npm run reconcile entrypoint: at the sync checkpoint height it compares each sampled account's current balance against the node's bank balance and the ledger's per-denom totals against chain supply, exiting non-zero on any mismatch so it can gate a deploy.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Beyond the inline findings, I also looked at two other candidates: (1) AccountInterner not setting is_module_account for newly-interned addresses — module accounts are normally seeded with the correct flag at genesis, so this only affects a module account that first appears mid-chain, and nothing currently reads that flag off mid-chain-interned accounts, so it's not a correctness issue; (2) the ledger baseline query in balance-writer.service.ts selecting the accountId×denom cross product — it's filtered down to only the touched pairs in application code afterward, so it's a minor efficiency nit rather than a bug.
Extended reasoning...
Checked these two candidates raised during the automated pass beyond what's in the inline comments. Neither changes ledger correctness: the module-account flag gap only affects metadata on accounts that never appear in genesis (rare, and unused downstream in this PR), and the baseline query's cross-product shape is bounded by the batch's own touched accounts/denoms before being filtered, so it doesn't fetch unrelated data at scale in the way a true cross-product read would.
Scope the slash reason to the coincident burn leg so a validator slash no longer mislabels the block's routine inflation mint (and other block-level movements sharing the scope) as slash. Add the missing mint case to the reason classifier so a mint-module counterparty resolves to mint instead of falling through to transfer. Reject a non-numeric RECONCILE_SAMPLE_SIZE instead of coercing it to NaN, which slice() turned into an empty sample that could report a clean pass having verified zero accounts. Drop the unread "message" event type from the decoder whitelist and remove a dead branch in decodeIfBase64.
The reason classifier fell through to `transfer` for any coin movement involving the x/escrow module account, because that module wasn't in the address registry. It mislabeled every ACT (uact) flow — which only moves through escrow, never peer-to-peer — plus uakt escrow deposits, refunds, and lease settlements, as generic transfers. Add the escrow module to the registry and map it to the existing `escrow` reason. Verified against a mainnet day of blocks: uact movements now classify as escrow (previously 100% transfer) while per-account and total supply balances still reconcile exactly to the chain.
There was a problem hiding this comment.
Beyond the inline findings, I also checked whether hardcoding BME_VAULT_ADDRESS to a single mainnet address breaks BME classification on sandbox/testnet — it doesn't corrupt any balance, it just means a BME-vault movement on those networks falls through to the default transfer reason, consistent with the classifier's documented MVP scope.
Extended reasoning...
This is an informational note, not a full review. The bug-hunting system's main finding (concurrent sync+backfill writers seeding balance_after from an incomplete ledger baseline) is a real correctness gap in a core code path this PR is meant to guarantee, so I'm not approving. I additionally reviewed the ruled-out BME_VAULT_ADDRESS candidate from this run and confirmed it's not a distinct bug beyond what's already covered: an unmatched module address just falls back to the generic transfer reason (metadata only, no balance impact), the same non-corrupting failure mode already established for the other reason-classifier nits on this PR.
…ail over abci non-zero code
…rrency Read the checkpoint height and ledger balances inside one REPEATABLE READ read-only transaction so a concurrent block commit can't pair a stale height with post-commit balances and flag spurious mismatches. Fan out the per-account bank queries with bounded concurrency instead of one sequential RPC round-trip at a time, letting the RpcClientPool load-balance the sampled accounts.
There was a problem hiding this comment.
Beyond the reported nit finding, I also checked two other candidates the bug-hunting system raised and ruled out: ReconcileService's account sampling sums balances across unrelated denoms, but that's only a ranking heuristic for which accounts to check first — every sampled account still gets its full per-denom balance diffed against the chain, so it doesn't affect correctness. And BalanceWriter's baseline query filters accountIds/denoms as a cross-product, but the in-memory touched set afterward discards any (account, denom) pair not actually in the batch, so it's an over-fetch, not a correctness bug.
Extended reasoning...
This is a large (XL), financially-critical PR introducing a full balance ledger, address activity log, and reconciliation pipeline for the chain indexer — new coin-event decoding, reason classification, running-balance writes, module address derivation, and an RPC failover pool. Given the scope and the fact that a real (if minor) correctness bug was already found in this run (module-to-module reason misclassification) plus several bugs found and fixed in earlier rounds, this is well beyond what should be silently approved. I independently verified the two other candidate issues that were raised and ruled out this run (noted in the message) by reading the relevant code paths directly, and found the rebuttals sound.
… first For a module-to-module movement (fee_collector to distribution, mint to fee_collector) both sides carry a role, so preferring the counterparty tagged each leg with the other module's reason. Prefer the holder's own role and fall back to the counterparty, so every leg is classified by the module whose balance actually changed. Account-to-module movements are unaffected: the account side has no role and still falls through to the counterparty.
Chunk the account existence SELECT the same way the insert path already does, so a batch with tens of thousands of distinct addresses can't exceed postgres.js's bind-parameter limit and abort the commit transaction. Document the balance ledger, activity log, and the reconcile script in the app README alongside the existing feature sections.
Precompute the coin total per account before sorting the reconciliation sample instead of re-reducing both operands on every comparator call.
There was a problem hiding this comment.
Beyond the two inline nits, I also checked two other candidate concerns and ruled them out. AccountInterner never sets isModuleAccount for module accounts discovered mid-chain (e.g. fee_collector, distribution) — confirmed true, but nothing in the pipeline or reconcile logic reads that column; reason classification uses the independently-derived module address registry instead, so this is a data-completeness gap on an unused field, not a correctness bug. I also checked whether reconcile's checkpoint-height/balance-snapshot read could still race with a concurrent backfill commit — it can't, since both reads now happen inside the same REPEATABLE READ transaction.
Extended reasoning...
Checked whether AccountInterner should backfill isModuleAccount for mid-chain-discovered module accounts, and whether reconcile's snapshot read is still race-prone after the REPEATABLE READ fix. Neither is a real bug: isModuleAccount is write-only dead data (reason classification derives module roles independently via sha256-derived addresses), and the REPEATABLE READ transaction already makes the checkpoint/balance read atomic against concurrent commits.
…limit #readLedgerBaseline built accountIds/denoms from a whole commitBatch's intents and passed them into one unchunked inArray SELECT, unlike #insertChanges/#applyNetDeltas in the same class. A dense backfill batch near BACKFILL_BATCH_SIZE's 1000-block max can touch tens of thousands of distinct accounts, exceeding postgres.js's ~65k bind-parameter cap and aborting the whole commitBatch transaction. Chunk accountIds by INSERT_CHUNK_SIZE and merge per-chunk rows, matching AccountInterner's already-chunked select.
The reconcile CLI hand-rolled Number() parsing plus a bespoke Number.isInteger/positive check and its own CONFIG_INVALID log shape, duplicating the zod envSchema every other numeric env var already flows through. Add RECONCILE_SAMPLE_SIZE to envSchema as an optional coerced positive int (same idiom as SYNC_START_HEIGHT) and read it from parsed.data, so an invalid value fails at the existing safeParse path. The service keeps its own guard as defense in depth.
There was a problem hiding this comment.
I reviewed this PR and didn't find any new bugs in this pass — the two prior nit findings still open at my last look (the un-centralized RECONCILE_SAMPLE_SIZE parsing and the unchunked readLedgerBaseline select) have since been fixed. This is an XL, correctness-critical change to the chain-indexer's balance ledger, activity log, and reconciliation path, so a human look is still worthwhile before merging.
Beyond the reported findings, this run also checked two candidate issues and ruled them out: ReconcileService#sample summing balances across denoms to rank accounts — this only affects which accounts get sampled for checking, not the correctness of any individual balance comparison (that's per-denom in diffCoins). And deriveSignerAddresses not recording a multisig account's own address as a signer — correct, since only the individual member pubkeys are the cryptographic signers; the multisig account address itself never appears in SignerInfo.
Extended reasoning...
This run found no new bugs; the two nit-level findings open at the last review pass (RECONCILE_SAMPLE_SIZE validation duplication, and the unchunked readLedgerBaseline select) have both been fixed in the current code (commits 6c9cca8 and dcd61c9), matching the fix pattern already applied elsewhere in the PR (AccountInterner#selectInto chunking). Two additional candidate issues surfaced by finder agents this run were examined and refuted: the reconcile sampling heuristic summing across denoms only affects sample selection, not comparison correctness, and the signer-derivation behavior for multisig accounts matches the actual cryptographic signer set. Given the PR's size (XL, 47 files) and that it introduces the core write path for a financial balance ledger plus a new reconciliation gate, I'm deferring to a human reviewer for final sign-off despite the clean bug-hunting result.
35cfc61
into
feat/indexer-scaffold-chain-indexer-app
1 participant
Why
Closes CON-809. This is L-4 of the chain-indexer v2 stack. L-3 genesis (#3587) created and migrated the ledger schema and seeded genesis balances, and has now merged into the scaffold branch; L-4 is the write-path layer that makes every balance-affecting movement flow through the ledger, adds a queryable address activity log, and proves correctness with a reconciliation script.
What
Balances are derived from bank events, never from message bodies — the only source that can reconcile (fees, rewards, slashing, inflation, escrow/BME settlements, and failed-tx fees all emit coin events but not reconcilable message bodies).
coin_spent/coin_received(deltas) plustransfer/coinbase/burn/slash(classification only), with base64-normalized attributes and per-tx signer-address derivation.sha256(name)[:20], validated against known Cosmos module addresses), an MVP reason classifier (fee/reward/commission/mint/burn/slash/staking/gov/ibc/bme/transfer), and a balance deriver with a deterministic block-wideevent_index.(height, event_index)unique key is the serialization point:onConflictDoNothing().returning()means a re-committed block inserts 0 rows and applies 0 deltas. Running balances seed from the ledger baseline (not the snapshot), so a backfill batch overlapping the live-sync frontier can't corrupt running balances.account_balancesstays a pure, rebuildable projection of the ledger.account_txstable (address → every tx it signed/sent/received in), idempotent via a composite PK.npm run reconcilecompares each sampled account's current balance and the ledger's per-denom totals against the node's bank balance/supply at the sync checkpoint height (via a new historicalabci_query), exiting non-zero on any mismatch.Schema (migration
0002)balance_changes: addstx_index(nullable),event_index(not null), and a unique(height, event_index)index.event_index NOT NULLonly applies cleanly to an emptybalance_changes— a dev DB that already ran L-3 genesis must truncate the ledger tables and re-seed (greenfield; nothing consumes balances yet).account_txstable +account_tx_roleenum (signer/sender/receiver).stakingto thebalance_change_reasonenum.Verified end-to-end (sandbox + native Postgres)
Genesis seeds at height 0; block-level events (inflation/rewards/fees) derive through the ledger with correct reasons;
account_balancesis byte-for-byte consistent with the ledger's runningbalance_after; re-backfilling 300 already-committed blocks produced 0 new ledger rows and 0 balance drift (idempotency); the reconcile query round-trips real supply data against the live node.Known limitation: absolute reconcile at a from-genesis checkpoint can't pass against the public sandbox RPC because it prunes IAVL state below its recent window. In production the indexer reconciles near tip against a node serving recent state.