Feat/fsmhooks - #489
Closed
ezeike wants to merge 28 commits into
Closed
Conversation
changedBlobKeys diffed accounts/pools/validators by building a map[string][]byte per side and comparing via map lookups - two full hash-map builds per indexer-blobs request at ~1.35M accounts. Add mergeChangedBlobKeys, a two-pointer merge walk over the already Pebble-sorted entry slices, and use it for accounts and validators, whose extracted keys (account/validator address bytes) match their Pebble storage-key order exactly. Pools stay on the map-based diff: poolEntryKey extracts Pool.Id's raw varint wire bytes, which diverges from KeyForPool's big-endian storage encoding at varint length boundaries (e.g. id 16383 vs 16384), so entry order there does not reliably match key order. Adds TestMergeChangedBlobKeys_MatchesMapBasedDiff, which asserts the merge-walk output equals the original map-based diff output on the same scenario, plus an empty-previous edge case.
cold_read_time was running ~3s higher than what current+previous IndexerBlob's own instrumented steps accounted for (even after weighting accounts_iterate/ validators_iterate/block_non_signers_get by the previous-reuse miss rate) -- DeltaIndexerBlobs (parsing every account/pool/validator entry into a map on both sides to diff them) and the final lib.Marshal call were both inside the coldStart timing window but had no ObserveIndexerBlobStep of their own, making them an invisible chunk of the total. Adds delta_compute and delta_marshal steps to canopy_indexer_blob_step_time so that gap is attributable instead of inferred.
Compact() shared one hardcoded 3-minute context timeout between two very different callers: MaybeCompact() (live, runs on every commit) and CompactAll() (one-time, background goroutine right after a node finishes syncing/restoring, covering all four prefixes including the indexer prefix - every block/tx/QC in chain history, never touched during normal operation). At 774k+ blocks the indexer prefix alone exceeds 3 minutes to compact, so CompactAll always failed outright on [i/] with 'context deadline exceeded' (observed on localnet-2: [s/] and [h/] compacted fine in 1m08s/1m47s, [i/] hit the timeout at exactly 3:00). Because CompactAll aborts on first error, the indexer prefix has effectively never finished a post-sync compaction, consistent with the store sitting at ~259GB across 2400+ SST files days after the fact. Give MaybeCompact's LSS/HSS calls (still time-sensitive, live path) liveCompactionTimeout (3m, unchanged) and CompactAll's calls postSyncCompactionTimeout (30m) - CompactAll runs off the hot path in a background goroutine, so a longer bound costs nothing. Adds TestCompactionTimeoutsDistinct (guards against the two budgets being collapsed back into one shared constant) and TestCompactAllSucceedsOnAllPrefixes (smoke test that every prefix, including indexerPrefix, compacts through the new signature).
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ndexerBlob Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…eCollector Also fixes entryFor()'s address extraction: KeyForAccount() encodes the address as a length-prefixed segment (lib.JoinLenPrefix), so stripping only AccountPrefix() left a stray length byte on the front of AccountChangeEntry.Address. Updated account_change_test.go's key fixtures to build keys via the real KeyForAccount() (previously hand- rolled AccountPrefix()+raw bytes, which masked the bug).
Remove indexerBlobCacheEntries and serveIndexerBlobsLive from docker config fixtures — they are intentionally partial and overlay on defaults via json.Unmarshal. These edits were unnecessary and introduced an undisclosed unrelated field (indexerBlobCacheEntries).
…nt read-only get contract
Adds a skipAccounts bool parameter to StateMachine.IndexerBlob so callers that source account deltas separately (IndexerBlobsCached, Task 9) can skip the expensive full IterateAndAppendWithThreshold (AccountPrefix()) scan over ~1.35M accounts. IndexerBlobs (plural) keeps full-scan behavior, passing false at both call sites. Also updates cmd/rpc/query.go's two IndexerBlob call sites to pass false, preserving today's behavior and keeping the repo building - those flip to true and wire the fast path in Task 9.
…in the account fast path The collector only reports accounts the block WROTE, but a reward or slash usually moves stake instead: a compounding validator's reward is routed to UpdateValidatorStake, a non-compounding validator's reward credits validator.Output rather than the operator address the event names, and a slash touches stake only. The old full-scan path force-included those accounts on both sides because its maps held every account in state, so the fast path was emitting a strictly smaller Accounts set on nearly every block paying or slashing a validator. Read the missing reward/slash addresses back individually -- O(block events), not O(all accounts). Also: emit each side sorted by address, since Results() ranges a map and deltaBytes is cached and served; copy account bytes so the response never aliases the shared tip cache; and fall back to the full scan at height 2, where there is no previous blob and the full account set is the documented output.
…es full-scan-and-diff Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Adds an integration-level test proving AccountChangeCollector captures only account-prefixed keys when a real applied block also writes POOL and VALIDATOR entries via BeginBlock/EndBlock committee-reward machinery (not a direct Set call, which TestStateMachine_SetDoesNotHookNonAccountKeys already covers). newTestPoolAndValidatorTouchingChain extends the newTestAccountDeltaChain pattern: validator #3 (the certificate's reward recipient) is flipped to Compound=true before block 4 commits, so block 5's committee reward routes through UpdateValidatorStake/SetValidator instead of the default non-compounding AccountAdd-to-output path. The unconditional DAO pool mint in BeginBlock supplies the pool write. The test verifies -- rather than assumes -- that the measured block writes both a pool and a validator entry (strict pre/post balance increases), then asserts every captured account entry is address-sized, is not validator #3's (proven-written) address, and round-trips to the live KeyForAccount value. Verified the validator-address check specifically catches a bytes.HasPrefix(k, ValidatorPrefix()) regression by temporarily broadening the collector hook and confirming the test fails, then reverting. Per re-scoping from the task-11 brief: the brief's other proposed test (reward/slash capture proving force-include redundancy) is intentionally NOT implemented -- that premise was found false by review; force-include remains necessary and is already covered by TestIndexerBlobsCached_ForceIncludesUnwrittenRewardSlashAccounts (cmd/rpc/query_test.go) and TestAccountDelta_MatchesOldFullScanAndDiff (fsm/indexer_test.go, Task 10).
MUST-FIX: bounds guard in AccountChangeCollector.entryFor before slicing the address out of a too-short account-prefixed key. This runs on the consensus write path (StateMachine.Set/Delete return this error before performing the store write), so a panic here would abort ApplyBlock and take the whole block commit down, not just fail indexing. Returns fsm.ErrInvalidKey (the existing constructor fsm/key.go already uses for malformed storage keys) instead of new error plumbing. MUST-FIX: gate cmd/rpc IndexerBlobsCached's account-delta fast path on ServeIndexerBlobsLive (previously only live capture read this flag), so disabling it is a config-only way back to the full-scan-and-diff path without a redeploy if GetAccountDelta misbehaves in production -- a failure there now fails the whole indexer response, not just accounts. Also fixes the previous blob's hardcoded skipAccounts=true, which was only equivalent to the fast path's predicate before this flag existed. RECOMMENDED: document the QC-normalization invariant GetAccountDelta's RootDexBatch restore depends on (controller/account_delta.go); save/restore ApplyBlock's accountCollector instead of unconditionally nil'ing it, so nested/reentrant ApplyBlock calls can't clobber an outer collector; deprecate the orphaned full-scan (*StateMachine).IndexerBlobs wrapper so a future caller doesn't silently reintroduce the regression this branch removed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Self-poison AccountChangeCollector instead of returning errors from RecordSet/RecordDelete: an internal failure (a baseline read error, a malformed key) now disables collection for the rest of the block without ever aborting the StateMachine.Set/Delete call it hooks, so a bug in the indexing hook can no longer stop a live block from committing. Callers check Err() before trusting Results(); a poisoned collector's delta is dropped rather than cached or served. Fix GetAccountDelta reading the block and QC through the live consensus store: every commit closes and swaps the live store's readers unsynchronized, so a concurrent RPC read raced Store.Reset(). Read through a TimeMachine(height) snapshot instead, matching IndexerBlob's existing pattern. Move accountSide/sortedAccountEntries/forceIncludeAccounts out of cmd/rpc into fsm.AssembleAccountDeltaSides so the differential regression test exercises the real production code instead of a hand-duplicated copy that could drift from it unnoticed. Consolidate the three positionally-threaded added/changed/removed slices into a single fsm.AccountDelta so they can't be transposed across a call boundary, and copy its slice headers on a cache hit so a caller's append can't write through into the shared tip cache. Hoist AccountPrefix() into a package-level accountKeyPrefix so the consensus-path hook doesn't allocate on every write. Log a warning node-side when the fast path fails: the error was previously only visible in the HTTP 400 body sent to the indexer client, giving an operator no signal to reach for the serveIndexerBlobsLive=false kill switch. Add StateMachine-level tests for the Delete hook (hooked, non-account key, nil collector) mirroring the existing Set hook coverage.
Add controller/account_delta_replay_test.go: full coverage of
GetAccountDelta's replay branch against a real committed chain --
happy path, the RootDexBatch restore for both a QC carrying a batch
and one with nil Results, a missing block, an above-tip height (the
TimeMachine clamp guard), a missing QC, and the height-2 boundary
where the QC fetch must be skipped entirely.
Add controller/block_test.go: table test for
accountCollectorForLiveCommit over {syncing, ServeIndexerBlobsLive},
and cacheAccountDelta coverage for nil collector, nil cache, a
healthy collector, and a poisoned one -- the live-commit gating that
decides whether the tip cache is ever populated.
Add AccountDeltaTipCacheHits/Misses metrics and a debug log on the
replay fall-through, so a cold-cache period (restart, backfill) is
visible instead of only showing up as an indirect step-time increase.
Extend the fsm differential test with an empty-block case (the only
account movement comes from the automatic reward payout) and cover
AccountEntriesAtVersion's skip-missing-account branch. Tighten
TestApplyBlock_CollectorCapturesTouchedAccounts from a vacuous
NotEmpty check to exact address/value assertions.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.