Repository navigation
Move reorg detection into Rust BlockStore - #1405
Conversation
The per-chain BlockStore now owns reorg detection: merging a fetch-response page compares block hashes and reports the lowest in-threshold mismatch (discarding the page in rollback mode, overwriting in detect-only mode), pruning keeps in-threshold hashes as hash-only rows, and rollback reads (getHash, getHashedBlockNumbers, latestValidBlock) replace the JS-side ReorgDetection registry. - Fuel gets a first-class store (height/time/id) built by the Rust client; Fuel blocks are materialised from the store instead of carried inline. - BlockStore.fromJs builds pages from sparse JS blocks: RPC contributes hash-only observations, simulate an empty page, and stored reorg checkpoints seed the store on resume. - The EVM HyperSync rollback-guard blocks are inserted into the page store on the Rust side; sources no longer return a separate blockHashes array. - ReorgDetection.res shrinks to the shared data types and log params. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Aj6SS9KbG9mytdzuYnMs3a
- Store detection hashes as their JS string form for every ecosystem, so pages built from JS observations compare byte-for-byte with fetched blocks (and mismatch reports return the original strings). - Record within-page hash conflicts (the same block observed twice with different hashes in one response) while a page is built and report them from merge, matching the old duplicate-collision detection. - Rollback keeps hash-only rows on non-reorg chains — their scanned hashes stay valid while refetch repopulates the data — and drops everything above the target on the reorg chain. - Checkpoint block hashes are gated by the chain's own reorg threshold (sourceBlockNumber - maxReorgDepth) instead of the global flag. - Port ReorgDetection/SourceBlockHashes/ChainState/rollback tests to the store-backed API and pin Fuel.blockFields against the Rust ordering. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Aj6SS9KbG9mytdzuYnMs3a
- FuelBlockField orders id first (Fuel.res blockFields matches). - The within-page hash conflict lives inside the store's single Mutex alongside the table instead of a second lock; it stays on the struct because a page is built across several insert calls (response blocks, then guard rows) and merge reads it later. - Clarify why the EVM hash column is filled outside evm_block_col. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Aj6SS9KbG9mytdzuYnMs3a
EVM/Fuel block hashes are hex-validated and stored as bytes again: fromJs pages reject non-hex hashes (e.g. arbitrary marker strings) with a validation error instead of storing them opaquely. The hash column is variable-width — 32 bytes for fetched blocks — so hex test fixtures can stay short. Test mocks now use valid even-length hex hashes. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Aj6SS9KbG9mytdzuYnMs3a
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis change adds Fuel block storage, moves reorg detection into BlockStore merges, replaces ReorgDetection state with BlockStore-backed APIs, updates source responses and native request handling, and adds bounded retries, validation, materialization changes, and related tests. ChangesBlockStore and source pipeline
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
scenarios/test_codegen/test/ReorgDetection_test.res (2)
3-5: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the refactor narration.
This describes where reorg detection moved rather than a non-obvious behavioral constraint.
As per coding guidelines,
**/*.res: “Never narrate the refactor itself.”Proposed change
-// Reorg detection now lives in the Rust BlockStore: merging a page compares -// block hashes and reports the lowest in-threshold mismatch; pruning keeps -// in-threshold hashes; rollback reads find the last valid block. describe("Block store reorg detection", () => {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scenarios/test_codegen/test/ReorgDetection_test.res` around lines 3 - 5, Remove the refactor-narration comment at the top of the test file; leave the test implementation unchanged.Source: Coding guidelines
35-59: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse one aggregate assertion per test.
scenarios/test_codegen/test/ReorgDetection_test.res#L35-L59: collect threshold-query results into one record assertion.scenarios/test_codegen/test/ReorgDetection_test.res#L88-L122: collect rollback and report-only outcomes into one final assertion, or split the modes into separate tests.scenarios/test_codegen/test/ReorgDetection_test.res#L141-L164: compare conflicting and identical duplicate outcomes together.scenarios/test_codegen/test/ReorgDetection_test.res#L191-L218: compare alllatestValidBlockcases in one array or record.scenarios/test_codegen/test/SourceBlockHashes_test.res#L195-L217: aggregate item count and both hash-presence checks.scenarios/test_codegen/test/SourceBlockHashes_test.res#L253-L271: aggregate item count, fetched block number, and hashes.As per coding guidelines,
**/*_test.res: “Always use single assert to check the whole value instead of multiple asserts for every field.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scenarios/test_codegen/test/ReorgDetection_test.res` around lines 35 - 59, Replace the multiple assertions in the affected tests with one aggregate assertion per test: in scenarios/test_codegen/test/ReorgDetection_test.res ranges 35-59, 88-122, 141-164, and 191-218, collect the threshold, rollback/report-only, duplicate, and latestValidBlock outcomes into a single record or array assertion; in scenarios/test_codegen/test/SourceBlockHashes_test.res ranges 195-217 and 253-271, aggregate the item count, fetched block number, and hash-presence results into one assertion while preserving all expected values.Source: Coding guidelines
packages/envio/src/Batch.res (1)
15-16: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the caller-oriented field comment.
The comment only explains where
blockStoreis consumed rather than a non-obvious constraint.As per coding guidelines, “Don't write a comment that restates what the code already says — … which callers use a value.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/envio/src/Batch.res` around lines 15 - 16, Remove the caller-oriented comment above the chain block store field in Batch.res, leaving the field declaration and surrounding code unchanged.Source: Coding guidelines
packages/envio/src/sources/RpcSource.res (1)
1118-1122: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winDedupe
observedBlocksentries per block number before pushing per-log.This loop pushes one
{blockNumber, blockHash}entry per log item. For a block containing many matched logs (common for popular contracts), the same(blockNumber, blockHash)pair gets pushed once per log, inflating the page passed toBlockStore.fromJs/fromJsEvmacross the napi boundary for no benefit — the block's hash doesn't change between logs.♻️ Proposed dedup
+ let seenLogBlocks = Utils.Set.make() items->Array.forEach(({log}) => - observedBlocks - ->Array.push({BlockStore.blockNumber: log.blockNumber, blockHash: log.blockHash}) - ->ignore + if !(seenLogBlocks->Utils.Set.has(log.blockNumber)) { + seenLogBlocks->Utils.Set.add(log.blockNumber)->ignore + observedBlocks + ->Array.push({BlockStore.blockNumber: log.blockNumber, blockHash: log.blockHash}) + ->ignore + } )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/envio/src/sources/RpcSource.res` around lines 1118 - 1122, Deduplicate observedBlocks by block number before adding entries in the items iteration around the log handling flow. Ensure each block contributes only one {BlockStore.blockNumber, blockHash} pair, while preserving the existing block hash and the downstream BlockStore.fromJs/fromJsEvm input behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/cli/src/block_store.rs`:
- Around line 658-665: Update the merge method containing the ecosystem
assertion to enforce compatibility in all builds: replace the debug-only check
with a runtime discriminant comparison, return the method’s existing error type
on mismatch, and perform this validation before accessing either table. Preserve
the current merge behavior for matching ecosystems.
- Around line 995-999: Update the Height and ParentSlot conversions in the
block-store field factory to propagate u64 conversion failures like the existing
slot handling. Reject negative values as errors instead of converting them to
missing fields, while preserving successful conversion behavior for valid
values.
In `@packages/envio/src/ChainState.res`:
- Around line 25-26: Use cs.maxReorgDepth as the authoritative runtime value
across reorg detection, pruning, rollback, and checkpoint retention. In
packages/envio/src/ChainState.res:25-26 and getHighestBlockBelowThreshold, use
cs.maxReorgDepth; at packages/envio/src/ChainState.res:743-744, include
maxReorgDepth: cs.maxReorgDepth in Batch.chainBeforeBatch; and in
packages/envio/src/Batch.res:208-209, calculate the retention threshold from
chainBeforeBatch.maxReorgDepth instead of chainConfig.maxReorgDepth.
In `@scenarios/test_codegen/test/rollback/Rollback_test.res`:
- Around line 2084-2089: The comments in the rollback test still reference
removed reorg helper methods. Update the affected comments near the second reorg
and the additional referenced sections to describe BlockStore’s current hash
comparison and rollback behavior, including the relevant stored-block and
threshold outcomes without naming getThresholdBlockNumbersBelowBlock or
registerReorgGuard.
---
Nitpick comments:
In `@packages/envio/src/Batch.res`:
- Around line 15-16: Remove the caller-oriented comment above the chain block
store field in Batch.res, leaving the field declaration and surrounding code
unchanged.
In `@packages/envio/src/sources/RpcSource.res`:
- Around line 1118-1122: Deduplicate observedBlocks by block number before
adding entries in the items iteration around the log handling flow. Ensure each
block contributes only one {BlockStore.blockNumber, blockHash} pair, while
preserving the existing block hash and the downstream
BlockStore.fromJs/fromJsEvm input behavior.
In `@scenarios/test_codegen/test/ReorgDetection_test.res`:
- Around line 3-5: Remove the refactor-narration comment at the top of the test
file; leave the test implementation unchanged.
- Around line 35-59: Replace the multiple assertions in the affected tests with
one aggregate assertion per test: in
scenarios/test_codegen/test/ReorgDetection_test.res ranges 35-59, 88-122,
141-164, and 191-218, collect the threshold, rollback/report-only, duplicate,
and latestValidBlock outcomes into a single record or array assertion; in
scenarios/test_codegen/test/SourceBlockHashes_test.res ranges 195-217 and
253-271, aggregate the item count, fetched block number, and hash-presence
results into one assertion while preserving all expected values.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2cc44269-3e3e-4112-aaa9-fd867381eeb0
📒 Files selected for processing (37)
packages/cli/src/block_store.rspackages/cli/src/evm_hypersync_source/mod.rspackages/cli/src/field_table.rspackages/cli/src/fuel_hypersync_source/mod.rspackages/envio/src/Batch.respackages/envio/src/ChainFetching.respackages/envio/src/ChainState.respackages/envio/src/ChainState.resipackages/envio/src/Core.respackages/envio/src/EventConfigBuilder.respackages/envio/src/EventProcessing.respackages/envio/src/ReorgDetection.respackages/envio/src/Rollback.respackages/envio/src/SimulateItems.respackages/envio/src/sources/BlockStore.respackages/envio/src/sources/Fuel.respackages/envio/src/sources/HyperFuel.respackages/envio/src/sources/HyperFuel.resipackages/envio/src/sources/HyperFuelClient.respackages/envio/src/sources/HyperFuelSource.respackages/envio/src/sources/HyperSyncSource.respackages/envio/src/sources/RpcSource.respackages/envio/src/sources/SimulateSource.respackages/envio/src/sources/Source.respackages/envio/src/sources/SvmHyperSyncSource.resscenarios/fuel_test/src/Indexer.resscenarios/test_codegen/test/BlockStore_test.resscenarios/test_codegen/test/IndexerState_test.resscenarios/test_codegen/test/ReorgDetection_test.resscenarios/test_codegen/test/RpcSource_test.resscenarios/test_codegen/test/SourceBlockHashes_test.resscenarios/test_codegen/test/helpers/MockIndexer.resscenarios/test_codegen/test/lib_tests/ChainState_materialize_test.resscenarios/test_codegen/test/lib_tests/CrossChainState_test.resscenarios/test_codegen/test/lib_tests/IndexerLoop_test.resscenarios/test_codegen/test/rollback/ChainMocking.resscenarios/test_codegen/test/rollback/Rollback_test.res
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7e4d2ad4a1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| switch cs.blockStore->BlockStore.merge( | ||
| blockStore, | ||
| ~fromBlock=Pervasives.max(knownHeight - cs.maxReorgDepth, 0), | ||
| ~reportOnly=!cs.shouldRollbackOnReorg, | ||
| ) { |
There was a problem hiding this comment.
When a response page is merged here, fromBlock is only used as the comparison lower bound; rows below that threshold are still appended to cs.blockStore and are only removed later by applyBatchProgress. For sparse/RPC ranges with no parsedQueueItems, no batch progress runs, so each empty fetch can leave hash-only observations (for example the latest block and parent) in the store indefinitely, whereas the old ReorgDetection.registerReorgGuard rebuilt its map from only in-threshold rows on every response. This lets long no-event backfills or polling grow the block store unboundedly; drop/prune hash-only observations outside the current reorg threshold when merging reorg pages.
Useful? React with 👍 / 👎.
| cs.blockStore->BlockStore.rollback(newProgressBlockNumber) | ||
| // A non-reorg chain's scanned hashes above the target are still valid, so | ||
| // keep them for reorg detection while the refetch repopulates the data. | ||
| cs.blockStore->BlockStore.rollback(newProgressBlockNumber, ~keepHashes=!isReorgChain) |
There was a problem hiding this comment.
When the rollback target checkpoint is below newProgressBlockNumber (for example there were no events/checkpoints in the intervening blocks), rolling the reorg chain's block store only to newProgressBlockNumber leaves orphaned hashes between the last valid block and that progress point. The next refetch can include parent/guard observations for those blocks and compare them against stale hashes, immediately reporting another reorg or choosing the wrong depth. For the reorg chain, clear hashes above rollbackTargetBlockNumber even if fetch/progress are restored to newProgressBlockNumber.
Useful? React with 👍 / 👎.
…org-tracking-2ucqi4 # Conflicts: # packages/envio/src/ChainState.res # packages/envio/src/sources/SimulateSource.res
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 52fa0321e7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ~ecosystem=config.ecosystem.name, | ||
| ~shouldChecksum=!lowercaseAddresses, | ||
| ) | ||
| blockStore->BlockStore.merge(seedPage, ~fromBlock=0, ~reportOnly=false)->ignore |
There was a problem hiding this comment.
When a restarted indexer has checkpoint rows from both before and after a prior rollback, chainReorgCheckpoints can contain the same block number with different hashes because the checkpoint query does not dedupe by block. BlockStore.fromJs records that as a within-page mismatch, and this merge(..., reportOnly=false) then returns the mismatch without appending any of the seed page; since the result is ignored, the chain resumes with no persisted reorg hashes at all until new pages rebuild them. Deduplicate/order the checkpoints before building the page, or merge the seed in a mode that still preserves usable hashes.
Useful? React with 👍 / 👎.
| .pending_mismatch | ||
| .as_ref() | ||
| .is_none_or(|p| (key as i64) < p.block_number) | ||
| { | ||
| inner.pending_mismatch = Some(HashMismatch { |
There was a problem hiding this comment.
When a page records more than one same-block conflict, this keeps only the lowest block before merge applies fromBlock. If the lowest conflict is below the reorg threshold but a higher conflict is inside it (for example conflicts at blocks 50 and 100 with fromBlock=75), merge filters out the saved block 50 conflict and the block 100 conflict has already been discarded, so the self-conflicting page is appended without reporting a reorg. Keep all pending conflicts or choose the lowest conflict after applying the threshold.
Useful? React with 👍 / 👎.
| inner.table.rollback_keeping_field(target, self.hash_field()) | ||
| } | ||
| Ok(target) => inner.table.rollback(target), | ||
| Err(_) => inner.table.clear(), |
There was a problem hiding this comment.
When a non-reorg chain is rolled back alongside another chain to a checkpoint before its first block, newProgressBlockNumber can be -1; with keepHashes=true the intent is to reduce all buffered rows to hash-only rows, but this branch clears the whole store instead. That drops valid in-threshold hashes for the non-reorg chain and creates a reorg-detection gap until those blocks are fetched again; handle negative targets specially when keep_hashes is set.
Useful? React with 👍 / 👎.
* Validate block-store responses before retry * Address block hash query review feedback * Track SVM cursor coverage in block stores * Fix SVM parent validation and metrics
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0db045d918
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if !shouldSwitch { | ||
| await Utils.delay(backoffMillis) | ||
| } |
There was a problem hiding this comment.
When this source is the only usable source, or all alternatives are still inside recoveryTimeout, setting lastFailedAt does not prevent the next loop from selecting it again because getNextSources falls back to primaries that are still in recovery. This branch then skips the delay on every even retry, so repeated internally inconsistent responses immediately reissue the same request against the same drifting provider instead of backing off; keep the delay when no alternative source is actually available.
Useful? React with 👍 / 👎.
| retryRef := retryRef.contents + 1 | ||
|
|
||
| | Source.InconsistentResponse(_) as err => { | ||
| await retryInconsistentResponse( |
There was a problem hiding this comment.
When an RPC-backed source returns an internally inconsistent page, its onReorg hook is what clears cached block/transaction/receipt loaders. Because this await runs before invoking that hook, any other partition that runs during the 100ms/backoff window can still read orphaned cached data from the same source; invalidate the source state before waiting to retry.
Useful? React with 👍 / 👎.
…org-tracking-2ucqi4 # Conflicts: # packages/envio/src/SimulateItems.res # packages/envio/src/sources/SimulateSource.res
…g-2ucqi4' into claude/block-store-reorg-tracking-2ucqi4
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
packages/envio/src/sources/HyperSyncClient.res (1)
305-306: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove these implementation-location comments.
packages/envio/src/sources/HyperSyncClient.res#L305-L306: remove the comment; the method signature already expresses the boundary.packages/envio/src/sources/SvmHyperSyncClient.res#L208-L209: remove the equivalent comment.As per coding guidelines,
.rescomments must not restate code or point to where behavior is defined.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/envio/src/sources/HyperSyncClient.res` around lines 305 - 306, Remove the implementation-location comment near the relevant method in packages/envio/src/sources/HyperSyncClient.res (lines 305-306) and remove the equivalent comment in packages/envio/src/sources/SvmHyperSyncClient.res (lines 208-209); leave both method implementations unchanged.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/cli/src/evm_hypersync_source/mod.rs`:
- Around line 177-180: Update the EVM source polling loop around the
next_block/cursor check to return the same structured failure used by the SVM
path when next_block does not advance beyond cursor. Remove the
sleep-and-continue retry behavior for this stalled response so the source
manager receives an error instead of growing request_stats indefinitely.
In `@packages/envio/src/sources/HyperSync.res`:
- Around line 1-17: The rate-limit exception mapping currently discards native
request statistics. Update Source.res lines 112-113 to extend rateLimited with
the requestStats payload, then update mapRateLimitedExn in
packages/envio/src/sources/HyperSync.res lines 1-17 to pass failure.requestStats
when constructing Source.RateLimited, preserving those timings for retry
metrics.
In `@scenarios/test_codegen/test/helpers/RpcSourcePins.res`:
- Around line 1-3: In scenarios/test_codegen/test/helpers/RpcSourcePins.res
lines 1-3, remove the module-purpose comment; at lines 85-87, replace the
migration narrative with only the invariant that BlockStore keys hashes by block
number, making the projection deduplicated and ascending.
In `@scenarios/test_codegen/test/RateLimit_test.res`:
- Around line 107-110: Replace the multiple field-level assertions with one
whole-value assertion per test. In
scenarios/test_codegen/test/RateLimit_test.res lines 107-110 and 125-126,
combine the hash-range and wait-time results; in
scenarios/test_codegen/test/SvmHyperSyncSource_test.res lines 208-209, combine
forwarded blocks and request statistics; in
scenarios/test_codegen/test/lib_tests/SourceManager_test.res lines 101-107,
combine projected timing and mapped reset delay; and in lines 1489-1504, combine
pre/post-reorg state with the final response. Build a single record or tuple
containing each expected value and compare it once in each affected test.
---
Nitpick comments:
In `@packages/envio/src/sources/HyperSyncClient.res`:
- Around line 305-306: Remove the implementation-location comment near the
relevant method in packages/envio/src/sources/HyperSyncClient.res (lines
305-306) and remove the equivalent comment in
packages/envio/src/sources/SvmHyperSyncClient.res (lines 208-209); leave both
method implementations unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e6648dca-d679-4287-8e3c-386456625565
📒 Files selected for processing (39)
packages/cli/src/block_store.rspackages/cli/src/evm_hypersync_source/mod.rspackages/cli/src/evm_rpc_source/mod.rspackages/cli/src/field_table.rspackages/cli/src/lib.rspackages/cli/src/request_stats.rspackages/cli/src/svm_hypersync_source/mod.rspackages/cli/src/svm_hypersync_source/query.rspackages/envio/src/ChainFetching.respackages/envio/src/ChainState.respackages/envio/src/ChainState.resipackages/envio/src/Core.respackages/envio/src/ReorgDetection.respackages/envio/src/Rollback.respackages/envio/src/SimulateItems.respackages/envio/src/sources/BlockStore.respackages/envio/src/sources/HyperSync.respackages/envio/src/sources/HyperSync.resipackages/envio/src/sources/HyperSyncClient.respackages/envio/src/sources/HyperSyncSource.respackages/envio/src/sources/RpcSource.respackages/envio/src/sources/SimulateSource.respackages/envio/src/sources/Source.respackages/envio/src/sources/SourceManager.respackages/envio/src/sources/SourceManager.resipackages/envio/src/sources/SvmHyperSyncClient.respackages/envio/src/sources/SvmHyperSyncSource.resscenarios/test_codegen/test/IndexerState_test.resscenarios/test_codegen/test/RateLimit_test.resscenarios/test_codegen/test/ReorgDetection_test.resscenarios/test_codegen/test/RpcSourceContract_test.resscenarios/test_codegen/test/RpcSource_test.resscenarios/test_codegen/test/SvmHyperSyncSource_test.resscenarios/test_codegen/test/helpers/MockIndexer.resscenarios/test_codegen/test/helpers/RpcSourcePins.resscenarios/test_codegen/test/lib_tests/CrossChainState_test.resscenarios/test_codegen/test/lib_tests/SourceManager_test.resscenarios/test_codegen/test/rollback/ChainMocking.resscenarios/test_codegen/test/rollback/Rollback_test.res
🚧 Files skipped from review as they are similar to previous changes (14)
- packages/envio/src/Core.res
- scenarios/test_codegen/test/lib_tests/CrossChainState_test.res
- packages/envio/src/SimulateItems.res
- packages/envio/src/Rollback.res
- scenarios/test_codegen/test/IndexerState_test.res
- packages/envio/src/sources/SimulateSource.res
- packages/envio/src/sources/HyperSyncSource.res
- packages/envio/src/sources/RpcSource.res
- packages/envio/src/ChainFetching.res
- packages/envio/src/sources/BlockStore.res
- scenarios/test_codegen/test/ReorgDetection_test.res
- packages/envio/src/ChainState.res
- packages/cli/src/block_store.rs
- scenarios/test_codegen/test/rollback/Rollback_test.res
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 4
🧹 Nitpick comments (1)
packages/envio/src/sources/HyperSyncClient.res (1)
305-306: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove these implementation-location comments.
packages/envio/src/sources/HyperSyncClient.res#L305-L306: remove the comment; the method signature already expresses the boundary.packages/envio/src/sources/SvmHyperSyncClient.res#L208-L209: remove the equivalent comment.As per coding guidelines,
.rescomments must not restate code or point to where behavior is defined.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/envio/src/sources/HyperSyncClient.res` around lines 305 - 306, Remove the implementation-location comment near the relevant method in packages/envio/src/sources/HyperSyncClient.res (lines 305-306) and remove the equivalent comment in packages/envio/src/sources/SvmHyperSyncClient.res (lines 208-209); leave both method implementations unchanged.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/cli/src/evm_hypersync_source/mod.rs`:
- Around line 177-180: Update the EVM source polling loop around the
next_block/cursor check to return the same structured failure used by the SVM
path when next_block does not advance beyond cursor. Remove the
sleep-and-continue retry behavior for this stalled response so the source
manager receives an error instead of growing request_stats indefinitely.
In `@packages/envio/src/sources/HyperSync.res`:
- Around line 1-17: The rate-limit exception mapping currently discards native
request statistics. Update Source.res lines 112-113 to extend rateLimited with
the requestStats payload, then update mapRateLimitedExn in
packages/envio/src/sources/HyperSync.res lines 1-17 to pass failure.requestStats
when constructing Source.RateLimited, preserving those timings for retry
metrics.
In `@scenarios/test_codegen/test/helpers/RpcSourcePins.res`:
- Around line 1-3: In scenarios/test_codegen/test/helpers/RpcSourcePins.res
lines 1-3, remove the module-purpose comment; at lines 85-87, replace the
migration narrative with only the invariant that BlockStore keys hashes by block
number, making the projection deduplicated and ascending.
In `@scenarios/test_codegen/test/RateLimit_test.res`:
- Around line 107-110: Replace the multiple field-level assertions with one
whole-value assertion per test. In
scenarios/test_codegen/test/RateLimit_test.res lines 107-110 and 125-126,
combine the hash-range and wait-time results; in
scenarios/test_codegen/test/SvmHyperSyncSource_test.res lines 208-209, combine
forwarded blocks and request statistics; in
scenarios/test_codegen/test/lib_tests/SourceManager_test.res lines 101-107,
combine projected timing and mapped reset delay; and in lines 1489-1504, combine
pre/post-reorg state with the final response. Build a single record or tuple
containing each expected value and compare it once in each affected test.
---
Nitpick comments:
In `@packages/envio/src/sources/HyperSyncClient.res`:
- Around line 305-306: Remove the implementation-location comment near the
relevant method in packages/envio/src/sources/HyperSyncClient.res (lines
305-306) and remove the equivalent comment in
packages/envio/src/sources/SvmHyperSyncClient.res (lines 208-209); leave both
method implementations unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e6648dca-d679-4287-8e3c-386456625565
📒 Files selected for processing (39)
packages/cli/src/block_store.rspackages/cli/src/evm_hypersync_source/mod.rspackages/cli/src/evm_rpc_source/mod.rspackages/cli/src/field_table.rspackages/cli/src/lib.rspackages/cli/src/request_stats.rspackages/cli/src/svm_hypersync_source/mod.rspackages/cli/src/svm_hypersync_source/query.rspackages/envio/src/ChainFetching.respackages/envio/src/ChainState.respackages/envio/src/ChainState.resipackages/envio/src/Core.respackages/envio/src/ReorgDetection.respackages/envio/src/Rollback.respackages/envio/src/SimulateItems.respackages/envio/src/sources/BlockStore.respackages/envio/src/sources/HyperSync.respackages/envio/src/sources/HyperSync.resipackages/envio/src/sources/HyperSyncClient.respackages/envio/src/sources/HyperSyncSource.respackages/envio/src/sources/RpcSource.respackages/envio/src/sources/SimulateSource.respackages/envio/src/sources/Source.respackages/envio/src/sources/SourceManager.respackages/envio/src/sources/SourceManager.resipackages/envio/src/sources/SvmHyperSyncClient.respackages/envio/src/sources/SvmHyperSyncSource.resscenarios/test_codegen/test/IndexerState_test.resscenarios/test_codegen/test/RateLimit_test.resscenarios/test_codegen/test/ReorgDetection_test.resscenarios/test_codegen/test/RpcSourceContract_test.resscenarios/test_codegen/test/RpcSource_test.resscenarios/test_codegen/test/SvmHyperSyncSource_test.resscenarios/test_codegen/test/helpers/MockIndexer.resscenarios/test_codegen/test/helpers/RpcSourcePins.resscenarios/test_codegen/test/lib_tests/CrossChainState_test.resscenarios/test_codegen/test/lib_tests/SourceManager_test.resscenarios/test_codegen/test/rollback/ChainMocking.resscenarios/test_codegen/test/rollback/Rollback_test.res
🚧 Files skipped from review as they are similar to previous changes (14)
- packages/envio/src/Core.res
- scenarios/test_codegen/test/lib_tests/CrossChainState_test.res
- packages/envio/src/SimulateItems.res
- packages/envio/src/Rollback.res
- scenarios/test_codegen/test/IndexerState_test.res
- packages/envio/src/sources/SimulateSource.res
- packages/envio/src/sources/HyperSyncSource.res
- packages/envio/src/sources/RpcSource.res
- packages/envio/src/ChainFetching.res
- packages/envio/src/sources/BlockStore.res
- scenarios/test_codegen/test/ReorgDetection_test.res
- packages/envio/src/ChainState.res
- packages/cli/src/block_store.rs
- scenarios/test_codegen/test/rollback/Rollback_test.res
🛑 Comments failed to post (4)
packages/cli/src/evm_hypersync_source/mod.rs (1)
177-180: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Stop retrying an unadvanced EVM cursor indefinitely.
A stalled backend response keeps this loop alive forever and continuously grows
request_stats; the source manager never receives an error to retry. Return a structured failure here, as the SVM path does.Proposed fix
if next_block <= cursor { - tokio::time::sleep(Duration::from_millis(100)).await; - continue; + let error = map_err(anyhow::anyhow!( + "EVM block hash query made no progress: cursor={cursor}, next_block={next_block}" + )); + return Err(error_with_request_stats(error, &request_stats)); }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.if next_block <= cursor { let error = map_err(anyhow::anyhow!( "EVM block hash query made no progress: cursor={cursor}, next_block={next_block}" )); return Err(error_with_request_stats(error, &request_stats)); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/evm_hypersync_source/mod.rs` around lines 177 - 180, Update the EVM source polling loop around the next_block/cursor check to return the same structured failure used by the SVM path when next_block does not advance beyond cursor. Remove the sleep-and-continue retry behavior for this stalled response so the source manager receives an error instead of growing request_stats indefinitely.packages/envio/src/sources/HyperSync.res (1)
1-17: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Preserve native request timings through rate-limit retries.
The native payload includes
requestStats, but the ReScript exception mapping drops them beforeSourceManagercan update its metrics.
packages/envio/src/sources/HyperSync.res#L1-L17: propagatefailure.requestStatswhen constructing the rate-limit exception.packages/envio/src/sources/Source.res#L112-L113: extendrateLimitedto carry those statistics so the retry handler can record them.📍 Affects 2 files
packages/envio/src/sources/HyperSync.res#L1-L17(this comment)packages/envio/src/sources/Source.res#L112-L113🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/envio/src/sources/HyperSync.res` around lines 1 - 17, The rate-limit exception mapping currently discards native request statistics. Update Source.res lines 112-113 to extend rateLimited with the requestStats payload, then update mapRateLimitedExn in packages/envio/src/sources/HyperSync.res lines 1-17 to pass failure.requestStats when constructing Source.RateLimited, preserving those timings for retry metrics.scenarios/test_codegen/test/helpers/RpcSourcePins.res (1)
1-3: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove refactor narration from comments.
scenarios/test_codegen/test/helpers/RpcSourcePins.res#L1-L3: remove the module-purpose comment.scenarios/test_codegen/test/helpers/RpcSourcePins.res#L85-L87: replace the migration narrative with only the invariant: BlockStore keys hashes by block number, so the projection is deduplicated and ascending.As per coding guidelines,
.rescomments must not restate module purpose or narrate refactors.📍 Affects 1 file
scenarios/test_codegen/test/helpers/RpcSourcePins.res#L1-L3(this comment)scenarios/test_codegen/test/helpers/RpcSourcePins.res#L85-L87🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scenarios/test_codegen/test/helpers/RpcSourcePins.res` around lines 1 - 3, In scenarios/test_codegen/test/helpers/RpcSourcePins.res lines 1-3, remove the module-purpose comment; at lines 85-87, replace the migration narrative with only the invariant that BlockStore keys hashes by block number, making the projection deduplicated and ascending.Source: Coding guidelines
scenarios/test_codegen/test/RateLimit_test.res (1)
107-110: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use one whole-value assertion per test.
scenarios/test_codegen/test/RateLimit_test.res#L107-L110: combine hash-range and wait-time checks into one record assertion.scenarios/test_codegen/test/RateLimit_test.res#L125-L126: combine hash-range and zero-wait checks.scenarios/test_codegen/test/SvmHyperSyncSource_test.res#L208-L209: assert forwarded blocks and request stats together.scenarios/test_codegen/test/lib_tests/SourceManager_test.res#L101-L107: project timing and mapped reset delay into one result.scenarios/test_codegen/test/lib_tests/SourceManager_test.res#L1489-L1504: collect pre/post-reorg state and final response into one assertion.As per coding guidelines,
**/*_test.res: “Always use single assert to check the whole value instead of multiple asserts for every field.”📍 Affects 3 files
scenarios/test_codegen/test/RateLimit_test.res#L107-L110(this comment)scenarios/test_codegen/test/RateLimit_test.res#L125-L126scenarios/test_codegen/test/SvmHyperSyncSource_test.res#L208-L209scenarios/test_codegen/test/lib_tests/SourceManager_test.res#L101-L107scenarios/test_codegen/test/lib_tests/SourceManager_test.res#L1489-L1504🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scenarios/test_codegen/test/RateLimit_test.res` around lines 107 - 110, Replace the multiple field-level assertions with one whole-value assertion per test. In scenarios/test_codegen/test/RateLimit_test.res lines 107-110 and 125-126, combine the hash-range and wait-time results; in scenarios/test_codegen/test/SvmHyperSyncSource_test.res lines 208-209, combine forwarded blocks and request statistics; in scenarios/test_codegen/test/lib_tests/SourceManager_test.res lines 101-107, combine projected timing and mapped reset delay; and in lines 1489-1504, combine pre/post-reorg state with the final response. Build a single record or tuple containing each expected value and compare it once in each affected test.Source: Coding guidelines
…ilience, seam validation (#1442) * Address review findings: threshold authority, EVM parent-link check, test coverage - Use the resumed-from-DB maxReorgDepth and per-chain shouldRollbackOnReorg everywhere (Batch checkpoints, getHighestBlockBelowThreshold, reorg logging/rollback decision) instead of mixing them with config values - Validate EVM parent links in response stores: block N's parentHash must match block N-1's hash, within a page and across page seams; select parentHash in the EVM getBlockHashes re-fetch (Fuel has no parent-id field, so the check stays EVM/SVM-only) - Make shouldRollbackOnReorg/maxReorgDepth required in ChainState.make - Cover ChainState threshold arithmetic (depth changes across restarts, clamping, registerReorgGuard boundary) and applyBatchProgress hash retention with new tests - Harden field_table: hard width checks on fixed columns, 64-field cap - Drop stale ReorgDetection comments, dedupe native-failure unpacking, map rate-limited errors in the SVM getBlockHashes path too, fix test indentation Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019RcUijSCoEY19MCEbdh6fL * Surface EVM block-hash no-progress as an error for SourceManager to retry Matches the SVM path: instead of silently sleeping 100ms in-process (unbounded, no logging, no failover), the paginator returns a RequestFailed error carrying the accumulated request stats. SourceManager logs each retry with backoff and switches to another source on repeated failures. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019RcUijSCoEY19MCEbdh6fL * Friendlier no-progress message and a fast first retry for block-hash fetches The replica-drift error now explains itself (routing to a replica slightly behind the head, safe to continue after a retry), and SourceManager's first block-hash retry backs off only 100ms to match how quickly a lagging replica usually catches up. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019RcUijSCoEY19MCEbdh6fL * Polish block-hash retry: doubling backoff, message-first logging - Backoff doubles from 100ms (capped at 60s) instead of stepping by 1s - The native failure's own message becomes the retry log's msg (the replica-drift text is self-explanatory); generic failures keep the err payload - Native failure causes are plain JS errors now, so logs no longer show a NativeRequestFailed wrapper - Drop the logType field from the block-hash query logger and reword the replica-drift message Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019RcUijSCoEY19MCEbdh6fL * Shorten the replica-drift message Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019RcUijSCoEY19MCEbdh6fL * Use the resumed reorg depth for the pre-threshold fetch lag Codex review: with a reduced configured depth, blockLag from the config value would let fetching enter the stored rollback window without history while detection still compares the resumed window. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019RcUijSCoEY19MCEbdh6fL * Fail test runs with the source error instead of retrying forever In production a failing source is retried indefinitely (backoff and failover keep the indexer alive), which in tests turns an unreachable endpoint into a bare test-runner timeout with no context. SourceManager now accepts a maxRetries cap (ENVIO_MAX_SOURCE_RETRIES) enforced across the height, getItems, and getBlockHashes retry loops; when exhausted the run fails with the underlying error. The test indexer worker defaults the cap to 1, and generated templates give vitest 60s so the real error surfaces before the runner's axe. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019RcUijSCoEY19MCEbdh6fL * Make live-endpoint tests resilient to hung connections The test indexer worker now caps the HyperSync request timeout at 10s (production default is 120s, which outlives every test timeout and turns a hung connection into a context-free failure) so a hang fails fast and retries on a fresh connection. The scenarios suite retries a failed test once on CI - tests run sequentially with a fresh indexer per test. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019RcUijSCoEY19MCEbdh6fL * Reject EVM block-hash pages that don't cover their range densely Codex review: the parent-link check skips absent neighbours (event pages are sparse by design), so an include_all_blocks page omitting an interior block or a parentHash could hide a mixed-fork seam. Each page is now validated to carry every block in its covered range with hash and parent hash before it joins the aggregate. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019RcUijSCoEY19MCEbdh6fL * Honor the retry cap in the subscription fallback poller Codex review: the stale-subscription fallback swallowed every getHeightOrThrow failure, so with a subscription installed the cap never fired and a dead endpoint could still hang a test run. The fallback's rejection is separately observed since it can lose the height race. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019RcUijSCoEY19MCEbdh6fL * Retry a template smoke test once before failing the job The template suites index real blocks through live HyperSync; one hung connection on the runner shouldn't fail the job now that a failed attempt surfaces quickly with a real error. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019RcUijSCoEY19MCEbdh6fL * Retry e2e smoke tests once on CI Same rationale as the scenarios suite: the smoke tests hit live HyperSync, and a hung runner connection now fails fast with a real error, so a single retry absorbs it without hiding deterministic failures. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019RcUijSCoEY19MCEbdh6fL * Replace the global CI retry with per-test retries on live-endpoint tests Only the tests that hit live HyperSync/RPC endpoints retry (following the existing SourceBlockHashes pattern of {retry: 3}): the HyperSync client live tests, the RPC height check, the createTestIndexer tests that fetch mainnet, and the e2e smoke test. Deterministic tests fail on the first attempt again. The Vitest binding options gained a timeout field so the corrupted-token test keeps its 60s budget alongside the retry. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019RcUijSCoEY19MCEbdh6fL * Keep the test-worker HyperSync timeout at 30s Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019RcUijSCoEY19MCEbdh6fL * Widen template harness timeout padding from 10s to 30s Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019RcUijSCoEY19MCEbdh6fL * Simplify test-run resilience knobs Drop the template harness re-run hack and the derived outer timeout (the config value is the single budget now), run template suites with a 30s per-test vitest timeout, and rely on ENVIO_MAX_SOURCE_RETRIES=3 in test workers instead of overriding the HyperSync client timeout. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019RcUijSCoEY19MCEbdh6fL * Replace parent-hash validation with hash-collision checks A single HyperSync response is internally consistent, so parent-link validation only guarded the seams between paginated block-hash requests. Those seams are now covered directly: each follow-up page re-requests the last returned block, and a fork switch between requests surfaces as a hash collision on the overlapping block via the existing duplicate detection. The EVM/SVM parent-link checks, the dense-range validation, and the parent fields in block-hash queries are gone. RPC responses can mix forks since every block is fetched separately, so getBlockHashes now derives a minimal (number-1, parentHash) row from each block - like the items path already did - letting the page's existing collision check cross-validate separately fetched blocks. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019RcUijSCoEY19MCEbdh6fL --------- Co-authored-by: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 20a8fc4066
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| cursor = next_slot; | ||
| overlap_slot = last_slot.or(overlap_slot); |
There was a problem hiding this comment.
When a paginated SVM block-hash request advances through a range containing only skipped slots, last_slot is None but overlap_slot retains the last block from an earlier page. The next request then starts at that old slot rather than cursor; once it receives the same cursor boundary again, next_slot <= cursor treats normal progress as a replica failure and retries indefinitely. Clear the overlap anchor when the current page has no block, while retaining it only for the immediately preceding page.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
scenarios/test_codegen/test/SvmHyperSyncSource_test.res (1)
208-209: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse one assertion for the complete response contract.
Combine the forwarded range and request statistics into a single expected value.
Suggested adjustment
- t.expect(capturedBlockHashRequests->Utils.Array.lastUnsafe).toEqual(blockNumbers) - t.expect(response.requestStats).toEqual([{Source.method: "getBlockHashes", seconds: 0.25}]) + t.expect({ + "blockNumbers": capturedBlockHashRequests->Utils.Array.lastUnsafe, + "requestStats": response.requestStats, + }).toEqual({ + "blockNumbers": blockNumbers, + "requestStats": [{Source.method: "getBlockHashes", seconds: 0.25}], + })As per coding guidelines,
**/*_test.res: Always use single assert to check the whole value instead of multiple asserts for every field.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scenarios/test_codegen/test/SvmHyperSyncSource_test.res` around lines 208 - 209, Update the test assertions around the captured block hash request and response statistics to use one assertion against the complete expected response contract. Combine the forwarded blockNumbers range and requestStats into the single expected value, removing the separate field-level assertions while preserving both expected values.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/e2e-tests/src/template-tests/templates.test.ts`:
- Line 191: Update the enclosing test timeout in the template test around the
child process invocation to exceed config.timeouts.test by a settling buffer,
such as 30 seconds. Keep the child process timeout unchanged, and apply the
cushioned deadline only to the outer test.
---
Nitpick comments:
In `@scenarios/test_codegen/test/SvmHyperSyncSource_test.res`:
- Around line 208-209: Update the test assertions around the captured block hash
request and response statistics to use one assertion against the complete
expected response contract. Combine the forwarded blockNumbers range and
requestStats into the single expected value, removing the separate field-level
assertions while preserving both expected values.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: dd3e85ea-17af-4ac5-996b-5d084157e97d
📒 Files selected for processing (32)
packages/cli/src/block_store.rspackages/cli/src/evm_hypersync_source/mod.rspackages/cli/src/field_table.rspackages/cli/src/svm_hypersync_source/mod.rspackages/cli/templates/dynamic/init_templates/shared/package.json.hbspackages/e2e-tests/src/template-tests/templates.test.tspackages/envio/src/Batch.respackages/envio/src/ChainFetching.respackages/envio/src/ChainState.respackages/envio/src/ChainState.resipackages/envio/src/Env.respackages/envio/src/ReorgDetection.respackages/envio/src/TestIndexer.respackages/envio/src/bindings/Vitest.respackages/envio/src/sources/HyperSync.respackages/envio/src/sources/HyperSync.resipackages/envio/src/sources/HyperSyncSource.respackages/envio/src/sources/RpcSource.respackages/envio/src/sources/Source.respackages/envio/src/sources/SourceManager.respackages/envio/src/sources/SourceManager.resipackages/envio/src/sources/SvmHyperSyncSource.resscenarios/e2e_test/src/indexer.test.tsscenarios/test_codegen/test/EventHandler.test.tsscenarios/test_codegen/test/HyperSyncClient_test.resscenarios/test_codegen/test/IndexerState_test.resscenarios/test_codegen/test/ReorgDetection_test.resscenarios/test_codegen/test/RpcSource_test.resscenarios/test_codegen/test/SvmHyperSyncSource_test.resscenarios/test_codegen/test/lib_tests/CrossChainState_test.resscenarios/test_codegen/test/lib_tests/IndexerLoop_test.resscenarios/test_codegen/test/lib_tests/SourceManager_test.res
💤 Files with no reviewable changes (1)
- packages/envio/src/ReorgDetection.res
🚧 Files skipped from review as they are similar to previous changes (20)
- packages/envio/src/sources/SourceManager.resi
- scenarios/test_codegen/test/IndexerState_test.res
- scenarios/test_codegen/test/lib_tests/SourceManager_test.res
- scenarios/test_codegen/test/RpcSource_test.res
- packages/envio/src/ChainState.resi
- packages/envio/src/sources/SvmHyperSyncSource.res
- scenarios/test_codegen/test/lib_tests/IndexerLoop_test.res
- scenarios/test_codegen/test/lib_tests/CrossChainState_test.res
- packages/envio/src/Batch.res
- packages/envio/src/sources/RpcSource.res
- packages/envio/src/sources/HyperSync.res
- packages/envio/src/ChainFetching.res
- packages/cli/src/field_table.rs
- packages/envio/src/sources/Source.res
- packages/cli/src/evm_hypersync_source/mod.rs
- packages/envio/src/sources/HyperSyncSource.res
- packages/cli/src/svm_hypersync_source/mod.rs
- packages/envio/src/ChainState.res
- packages/cli/src/block_store.rs
- packages/envio/src/sources/HyperSync.resi
…org-tracking-2ucqi4 # Conflicts: # packages/envio/src/ChainState.res # scenarios/test_codegen/test/rollback/Rollback_test.res
…g-2ucqi4' into claude/block-store-reorg-tracking-2ucqi4
The review-findings commit made ~shouldRollbackOnReorg/~maxReorgDepth required; the CrossChainState_test call site introduced by the query-sizing merge needed ~shouldRollbackOnReorg added. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Aj6SS9KbG9mytdzuYnMs3a
A page returning no rows kept the previous page's last block as the overlap anchor. On SVM that anchor sits behind the cursor whenever a window is all skipped slots, so the next request replayed the same empty window, the cursor never advanced, and the source-behind-head error reproduced on every retry. The anchor is now the block the page itself ended on, and a rewound request that covers no new ground drops the anchor and resumes from the cursor instead of being reported as a stalled source — an overlap page capped at the block it re-requested is this loop's own doing, not an instance behind the head. The SVM caller no longer marks coverage for a range the cursor did not advance past, so that case surfaces through the paginator's own error rather than a malformed coverage range. Covers the paginator with unit tests over an injected backend: the overlap seam, the empty page, the rewound page, a genuinely stalled source, and an empty range. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JscwnEcKUbmcQbPtotGZ48
The snapshot is ascending, so the gap's first block is a bisection away. Scanning it whole for every gap made checkpoint building quadratic in the number of blocks a wide batch spans. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JscwnEcKUbmcQbPtotGZ48
| ~requiredBlockNumbers: array<int>=[], | ||
| ) => { | ||
| let conflict = blockStore->BlockStore.responseConflict->Null.toOption | ||
| let missingBlockNumbers = blockStore->BlockStore.missingHashes(requiredBlockNumbers) |
There was a problem hiding this comment.
Let's calculate this only when there are no conflicts.
There was a problem hiding this comment.
Should we even group the two functions into one and call it something like validateInconsistentResponse?
| let backoffMillis = retry === 0 ? 100 : 500 * retry | ||
| let log = retry >= 4 ? Logging.childWarn : Logging.childTrace | ||
| logger->log({ | ||
| "msg": `Block #${blockNumber->Int.toString} is not available on the ${sourceState.source.name} source yet. Instances of a load-balanced backend drift slightly around the head, so this is expected — indexing continues after an automatic retry.`, |
There was a problem hiding this comment.
Use - instead of — everywhere
| // Floor for the retries driven by a condition the source reported itself | ||
| // (behind the head, inconsistent response). Unlike a caller-supplied backoff of | ||
| // 0, these must never busy-loop when there is no other source to move to. | ||
| let minRecoverableBackoffMillis = 50 |
| ~err: exn, | ||
| ~excludedSources=?, | ||
| ) => { | ||
| let backoffMillis = retry === 0 ? 100 : 500 * retry |
There was a problem hiding this comment.
Let's reuse minRecoverableBackoffMillis for retry 0?
| ~err: exn, | ||
| ~excludedSources=?, | ||
| ) => { | ||
| let backoffMillis = retry === 0 ? 100 : 500 * retry |
There was a problem hiding this comment.
Should we even get the value from backoffBeforeRetry
| chainState->ChainState.sourceManager->SourceManager.onReorg | ||
|
|
||
| let rollbackTargetBlockNumber = await chainState->getLastKnownValidBlock( |
There was a problem hiding this comment.
Let's move
chainState->ChainState.sourceManager->SourceManager.onReorg
inside of getLastKnownValidBlock?
| let getFreshBlockInfo = blockNumber => | ||
| blockLoader.contents | ||
| ->LazyLoader.set( |
There was a problem hiding this comment.
Let's revert it - it's fine that a request might be read from cache, what's important is that the next block will be read from rpc (not in cache yet) and we'll use parentHash for reorg detection.
| WHERE cp."${(#block_hash: field :> string)}" IS NOT NULL | ||
| AND cp."${(#block_number: field :> string)}" >= rc.safe_block;` // Include safe_block checkpoint to use it for safe checkpoint tracking | ||
| AND cp."${(#block_number: field :> string)}" >= rc.safe_block | ||
| ORDER BY cp."${(#id: field :> string)}";` // Include safe_block checkpoint to use it for safe checkpoint tracking |
There was a problem hiding this comment.
Why is this changed? Not a critic, just curiosity
| if request_from < cursor { | ||
| // Rewinding to the overlap block covered no ground the cursor | ||
| // had not already passed. That is this loop's own anchor | ||
| // failing to advance, not an instance stuck behind the head, so | ||
| // drop the anchor and ask again from the cursor. | ||
| overlap = None; | ||
| continue; | ||
| } |
There was a problem hiding this comment.
I don't understand the check - investigate whether it's correct and shouldn't be fixed.
| continue; | ||
| } | ||
| return Err(error_with_request_stats( | ||
| source_behind_head_err(cursor), |
There was a problem hiding this comment.
Check how the logic is handled for getLogs queries and give me overview whether we should apply the same source_behind_head_err to getLogs for all sources we have?
- block_store merge: reject cross-ecosystem pages at runtime (was debug-only), reset stale old-fork hashes above the divergence on detect-only reorgs, and prune hash-only observations below the reorg threshold so long no-event ranges stay bounded. - EVM/SVM sources: keep a hash-only row for every returned block/slot header whose logs/instructions were all dropped by client-side routing, so a fork on such a block can still be detected. - SourceManager: compute missing hashes only when there is no response conflict; replace em-dashes with hyphens in the reorg runtime files. - e2e template test: give the outer `pnpm test` case a timeout cushion over the command timeout. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Aj6SS9KbG9mytdzuYnMs3a
…g-2ucqi4' into claude/block-store-reorg-tracking-2ucqi4 # Conflicts: # packages/cli/src/evm_hypersync_source/mod.rs # packages/envio/src/sources/SourceManager.res
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ac9adeb390
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if let Some(mismatch) = &cross { | ||
| if let Ok(target) = u64::try_from(mismatch.block_number - 1) { | ||
| dst.table.rollback(target); |
There was a problem hiding this comment.
When rollbackOnReorg is disabled and a mismatch is detected, this rolls back the entire table above the divergence, including full block rows already merged by other completed partitions whose items remain in the fetch buffer. Detect-only mode performs no corresponding item rollback/refetch, so those buffered events are later materialized with missing selected block fields. Reset only the stale reorg hashes here rather than deleting unrelated buffered block data.
Useful? React with 👍 / 👎.
| // The guard carries the head block's timestamp, and the head is | ||
| // often outside the queried range's returned blocks — keeping it | ||
| // makes the row materialisable rather than hash-only. | ||
| timestamp: Some( | ||
| Quantity::try_from(guard.timestamp).context("guard timestamp negative")?, |
There was a problem hiding this comment.
On EVM HyperSync ranges with no parsed items and therefore no batch-progress pruning, each advancing rollback guard can leave its head row behind indefinitely. Fresh evidence in the current code is that this row now stores both hash and timestamp, while merge only calls prune_field_only_below, which explicitly retains any row containing fields besides the hash; consequently the attempted stale-observation fix does not prune these old guard heads and long empty backfills or polling can still grow the store without bound.
Useful? React with 👍 / 👎.
- block_store detect-only reset: clear only the stale hash field above the divergence instead of dropping the rows, so full block data other partitions buffered still materialises (regression from the previous reset). - SourceManager: reduce the inconsistent-response stall threshold from ~10 to ~5 minutes. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Aj6SS9KbG9mytdzuYnMs3a
- Revert "Read the range's own blocks for detection, not the block cache": the block cache read is fine, since the next block comes from the RPC and its parentHash carries the reorg detection. Drops `LazyLoader.set` with it. - Rollback now drops every block above its target, hashes included, on every chain. A non-reorg chain therefore replays without its hashes until it refetches, so its checkpoints in that range carry no hash. - Move the source-cache drop into `getLastKnownValidBlock`, so the depth search cannot be run against a cache filled on the orphaned fork. - FetchState: replace the `blockRef` record with a plain `latestFetchedBlock: int`. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Aj6SS9KbG9mytdzuYnMs3a
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
- merge takes `pruneHashesBelow`, so a hash for a block the chain has not processed yet survives even when it falls outside the reorg threshold. A backfill sits far below the threshold, and those blocks are still to be read. - SVM get_event_items reports a replica that has not reached the queried range through `source_behind_head_err`, like the paginator and the EVM/Fuel item paths already do, and the source re-raises the recoverable markers instead of burying them in a generic retry. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Aj6SS9KbG9mytdzuYnMs3a
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
- Detect-only reorgs no longer reset anything: the page overwrites what it observed and later pages converge the rest. - Drop the reorg-checkpoint dedup and its ORDER BY. One block number carrying two hashes needs a rollback torn between its delete and its insert, and both run in the same transaction. - One backoff schedule for every same-request retry, replacing the two linear ones and the exponential one. - The block-hash paginator reports a behind-head instance from the overlap request instead of spending a second request from the cursor. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Aj6SS9KbG9mytdzuYnMs3a
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
A real source returns the header of every block a matched log came from, but the mock only reported the range's seam and its last block. Reorg detection therefore never saw the blocks events actually landed on: checkpoints written on them carried no hash, and the rollback depth search skipped them. The rollback tests now search those blocks too, so they answer for them - with a differing hash where the block belongs to the reorg they set up. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Aj6SS9KbG9mytdzuYnMs3a
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Merging a page no longer prunes. The progress-time prune already drops every processed row and reduces the ones still inside the reorg depth to their hash, and a response with no items still advances progress to the fetched frontier, so it runs on empty ranges too. Dropping the merge-time prune removes the need to bound it by the processed progress, and the rollback-guard rows it could never recognise are trimmed by position like any other processed row. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Aj6SS9KbG9mytdzuYnMs3a
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
`makeGetBlockHashes(~query=client.getBlockHashes)` passed a napi class method as a value, so calling it later lost the instance and every block-hash query threw "Illegal invocation" — the reorg rollback depth search on any HyperSync source, EVM or SVM, since #1405. Every existing test of that path mocks `Source`, so nothing called the real client. The mock server can now answer at the HTTP level, which is what rate limiting and payload-too-large need. New tests cover the block-hash query and its pagination, the 429 -> retry wait mapping, a page that made no progress, a partial page, range halving on 413, logs that route to no registration, address checksumming, and a page that withholds a selected block field. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EQnf4SXnntuNhEiZW5CBLr
* Add a mock HyperSync server for source-level tests The HyperSync query is built, sent, and decoded inside the native addon, so nothing on the JS side could observe or answer it — the source was only ever exercised against the real service. MockHyperSyncServer is a local HTTP server speaking the same protocol: it captures the JSON query bodies the client POSTs and answers them with a page encoded the way HyperSync does, a packed Cap'n Proto message carrying one Arrow IPC file per table. Tests supply pages as JSON rows keyed by HyperSync field names. HyperSyncSourceContract_test drives a real EvmHyperSyncSource against it and pins the query's field selection and log selection, the fields each registration's item carries after materialisation, the height stream, the rollback guard's blocks, and the error a page that withholds a selected field produces. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EQnf4SXnntuNhEiZW5CBLr * Fix detached block-hash client method, cover more source responses `makeGetBlockHashes(~query=client.getBlockHashes)` passed a napi class method as a value, so calling it later lost the instance and every block-hash query threw "Illegal invocation" — the reorg rollback depth search on any HyperSync source, EVM or SVM, since #1405. Every existing test of that path mocks `Source`, so nothing called the real client. The mock server can now answer at the HTTP level, which is what rate limiting and payload-too-large need. New tests cover the block-hash query and its pagination, the 429 -> retry wait mapping, a page that made no progress, a partial page, range halving on 413, logs that route to no registration, address checksumming, and a page that withholds a selected block field. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EQnf4SXnntuNhEiZW5CBLr * Remove TODO.md Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EQnf4SXnntuNhEiZW5CBLr * Register the source test's handlers through the user API The test built its registrations by calling HandlerRegister.setHandler, which is what `indexer.onEvent` calls — so it reproduced the API instead of using it, and skipped the type check that makes an inline `fields` selection meaningful. fromUserApi grows `~registerHandlers`: it runs the handlers module the way a project does and exposes what registered as `parsed.registrations`, readable from a test body since the module is imported while vitest collects. The `~test` path is unchanged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EQnf4SXnntuNhEiZW5CBLr * Reject ~registerHandlers without a handlers source Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EQnf4SXnntuNhEiZW5CBLr --------- Co-authored-by: Claude <noreply@anthropic.com>
This branch was previously deployed
2 participants
Summary
Reorg detection moves out of the ReScript
ReorgDetectionmodule and into the RustBlockStore, where hash comparison happens directly on stored block hashes as a fetch-response page is merged into the per-chain store. The separate detection module is gone; what remains ofReorgDetection.resis the data shapes and the log formatting.Two things fall out of that. Response validation becomes a distinct concept from reorg detection: a response that contradicts itself (the same block twice with different hashes, a requested hash missing) is now a bad response to retry, not a chain reorg to roll back for. And Fuel gets a real block store, so it participates in reorg detection (detect-only) and materialises its block from the store like EVM/SVM.
Key changes
Rust
BlockStore(packages/cli/src/block_store.rs)merge()compares hashes while draining a page into the persistent store and reports the lowest in-threshold mismatch. On a mismatch nothing merges, so the scanned hashes survive for the rollback comparison — unlessreport_only(detect-only mode), which merges anyway so the same mismatch doesn't re-report on every response.responseConflict, on the page rather than the chain store, and reset on merge so response-only state can't become persistent chain data.prune(up_to, keep_hashes_from)reduces processed rows to hash-only instead of dropping them, keeping detection working inside the reorg threshold.FuelBlockField(height, id, time) plus Fuel materialisation;from_js_evm/svm/fuelbuild pages from sparse JS blocks (RPC observations, seeded checkpoints).latest_valid_block_from_storeandmissing_hashesmove the rollback comparison and coverage check into Rust; SVM accepts a missing slot when the HyperSync cursor proved its range was fully processed.Native block-hash queries
getBlockHasheson the EVM and SVM clients: query construction, pagination and overlap checks live in Rust, so block-only response data never crosses into ReScript. ReplacesHyperSync.BlockData.ENVIO_NATIVE_FAILURE:envelope, so a retried operation still reports what it spent.Retry policy (one place, every ecosystem)
Source.SourceBehindHead— raised by EVM/FuelgetItemsand by the Rust paginators via aSOURCE_BEHIND_HEAD:<block>marker (alongside the existingRATE_LIMITED:one). EVM and Fuel each used to build their own backoff schedule and message for this; SourceManager now owns it forgetItemsandgetBlockHashesalike.backoffBeforeRetrycentralises the failover decision that only theWithBackoffpath implemented. MarkinglastFailedAtonly demotes a source in the selection order, so when there is no alternative to move to the retry lands right back on the same source — those retries now carry a 50ms floor instead of spinning.ReScript runtime
ChainStateholdsshouldRollbackOnReorg/maxReorgDepthdirectly and derives every merge/read/prune boundary from(knownHeight, maxReorgDepth), using the resumed depth rather than the config one.Batchtakes an immutable snapshot of in-threshold hashes at batch assembly, so checkpoint hashes can't shift under a concurrent store mutation.blockStorepage (no moreoption): inline sources contribute hash-only rows built from the hashes they observed.Notes for reviewers
fromJshash shorter than that is left-padded to the canonical width, so a stored hash reads back padded —MockIndexer.evmBlockHashmirrors this for assertions.blockLagon resume uses the resumedmaxReorgDepth, not the config value, so a reduced config depth can't let fetching enter the stored rollback window without history.ENVIO_MAX_SOURCE_RETRIESwas introduced earlier in this branch and has been removed again: nothing set it, and its call sites turned a routine head-of-chain condition into a run-ending error.Tests
packages/cli480 Rust tests,packages/envio-tests408,scenarios/test_codegen711 — all passing. New coverage: block-store merge/prune/rollback and conflict detection in Rust,ChainStateReorgThreshold_testfor the threshold arithmetic across a changedmaxReorgDepth,SourceManagercases for the behind-head retry and foronReorgfiring before an inconsistent-response retry, and the rollback E2E suite reworked onto the new hash storage.https://claude.ai/code/session_01Aj6SS9KbG9mytdzuYnMs3a