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
| // A page and its persistent store are the same per-chain ecosystem, so | ||
| // the decoder is unaffected by the merge. Only the kind matters: the | ||
| // EVM checksum flag lives on the persistent store's decoder and may | ||
| // differ on a page built via `fromJs`. | ||
| debug_assert_eq!( | ||
| std::mem::discriminant(&self.ecosystem), | ||
| std::mem::discriminant(&page.ecosystem) | ||
| ); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Enforce ecosystem compatibility in release builds.
debug_assert_eq! disappears in release builds, but merge is public through N-API and accepts any BlockStore. Mixing ecosystems can compare unrelated ordinals or panic when a numeric column is treated as bytes. Return an error before accessing either table.
🤖 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/block_store.rs` around lines 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.
![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
|
|
||
| expect(result.exitCode, `[${name}] test failed (exit ${result.exitCode}):\n${result.stderr}\n${result.stdout}`).toBe(0); | ||
| }, config.timeouts.test + 10_000); | ||
| }, config.timeouts.test); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Keep a timeout cushion for the pnpm test command.
The child process already has config.timeouts.test at Line 187, but the enclosing test now has the same deadline. Restore a buffer, such as config.timeouts.test + 30_000, so the command can time out and settle cleanly before Vitest aborts the test.
🤖 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/e2e-tests/src/template-tests/templates.test.ts` at 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.
…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
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/envio/src/ChainState.res (2)
573-581: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winClamp an alignment-capped range cost before deriving the item budget.
maxTargetBlockcan clampchainTargetBlockbelowbufferBlockNumber. That makesrangeCostnegative and can pass a negativechainTargetItemsvalue intoFetchState.getNextQuery.Proposed fix
- let rangeCost = - density *. (chainTargetBlock - cs.fetchState->FetchState.bufferBlockNumber)->Int.toFloat + let remainingBlocks = Pervasives.max( + 0, + chainTargetBlock - cs.fetchState->FetchState.bufferBlockNumber, + ) + let rangeCost = density *. remainingBlocks->Int.toFloat Pervasives.min(chainTargetItems, Math.ceil(rangeCost) +. cs.pendingBudget)🤖 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/ChainState.res` around lines 573 - 581, Clamp the computed range cost in the effective-density branch of the chain target calculation to zero before applying Math.ceil and cs.pendingBudget. Preserve the existing minimum with chainTargetItems, ensuring FetchState.getNextQuery never receives a negative item budget when chainTargetBlock is below bufferBlockNumber.
528-536: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDo not report an unfetched one-block range as complete.
When
upper === lower, this returns1.even at the documented initialstartBlock - 1frontier. That makesfrontierProgresstreat a single remaining block as fetched, so cross-chain alignment can deprioritize it prematurely. Return0.until the frontier reaches that block; reserve unconditional1.for an empty range.Proposed fix
let progressAtBlock = (cs: t, ~blockNumber) => { let (lower, upper) = cs->progressRange - upper <= lower - ? 1. - : Pervasives.max( + if upper < lower { + 1. + } else if upper === lower { + blockNumber >= upper ? 1. : 0. + } else { + Pervasives.max( 0., Pervasives.min(1., (blockNumber - lower)->Int.toFloat /. (upper - lower)->Int.toFloat), ) + } }🤖 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/ChainState.res` around lines 528 - 536, Update progressAtBlock so an equal lower/upper range returns 0. until blockNumber reaches the remaining frontier block, then returns 1.; preserve the existing clamped calculation for non-empty ranges and reserve unconditional 1. for an actually empty range.
🤖 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.
Outside diff comments:
In `@packages/envio/src/ChainState.res`:
- Around line 573-581: Clamp the computed range cost in the effective-density
branch of the chain target calculation to zero before applying Math.ceil and
cs.pendingBudget. Preserve the existing minimum with chainTargetItems, ensuring
FetchState.getNextQuery never receives a negative item budget when
chainTargetBlock is below bufferBlockNumber.
- Around line 528-536: Update progressAtBlock so an equal lower/upper range
returns 0. until blockNumber reaches the remaining frontier block, then returns
1.; preserve the existing clamped calculation for non-empty ranges and reserve
unconditional 1. for an actually empty range.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 29305cb3-4c27-4e8c-81d2-3ecd76a320db
📒 Files selected for processing (9)
packages/envio/src/ChainState.respackages/envio/src/ChainState.resipackages/envio/src/sources/SourceManager.respackages/envio/src/sources/SourceManager.resiscenarios/test_codegen/test/IndexerState_test.resscenarios/test_codegen/test/helpers/MockIndexer.resscenarios/test_codegen/test/lib_tests/CrossChainState_test.resscenarios/test_codegen/test/lib_tests/SourceManager_test.resscenarios/test_codegen/test/rollback/Rollback_test.res
💤 Files with no reviewable changes (1)
- packages/envio/src/sources/SourceManager.resi
🚧 Files skipped from review as they are similar to previous changes (7)
- scenarios/test_codegen/test/IndexerState_test.res
- scenarios/test_codegen/test/helpers/MockIndexer.res
- packages/envio/src/ChainState.resi
- scenarios/test_codegen/test/lib_tests/CrossChainState_test.res
- scenarios/test_codegen/test/lib_tests/SourceManager_test.res
- packages/envio/src/sources/SourceManager.res
- scenarios/test_codegen/test/rollback/Rollback_test.res
…apshot, cleanups (#1448) - field_table: extract shared row-reduction/drop helpers from prune_keeping_field and rollback_keeping_field. - block_store: trim EvmBlockInput to the (number, timestamp, hash) trio the JS callers actually send, dropping the unreachable full-block conversion. - block_store: store the EVM hash as a fixed 32-byte column again; fromJsEvm left-pads shorter mock markers into that width (a no-op for real 32-byte observations), so storage stays fixed-width without input-fixture churn. - block_store: extract a ResponsePage type owning the response-only state (within-response conflict + SVM cursor coverage), so the persistent store no longer carries it and merge resets it explicitly instead of clearing fields. - request_stats/Source: mark the native-failure envelope with an explicit ENVIO_NATIVE_FAILURE: prefix so ReScript decodes only our own payloads and never a coincidental JSON error message. - Batch/ChainState: snapshot the in-threshold scanned hashes when the batch is assembled instead of reading the live block store per block; drops the live blockStore/maxReorgDepth off chainBeforeBatch and removes the per-block napi getHash calls from the checkpoint loop. - tests: migrate the getHash-derived EVM hash expectations to the padded 32-byte form via a shared MockIndexer.evmBlockHash helper. Claude-Session: https://claude.ai/code/session_01K3yMGs2o2DbQqu6LSTcHEd Co-authored-by: Claude <noreply@anthropic.com>
| .transpose() | ||
| .context("convert rollback guard") | ||
| .map_err(map_err)?, | ||
| rollback_guard, |
There was a problem hiding this comment.
I think we don't need to return back rollback guard anymore since all the purpose it served is now done on the Rust side.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cc63b82670
ℹ️ 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 cross.is_none() || report_only { | ||
| dst.table.append_from(&mut src.table); |
There was a problem hiding this comment.
When shouldRollbackOnReorg=false, this report_only merge overwrites only the hashes present in the current response, but it leaves any other in-threshold hashes from the old fork in the persistent store. The deleted ReorgDetection.registerReorgGuard reset its map after a detect-only reorg, so a partial page that detects a reorg at block N can now be followed by adjacent pages that compare against stale old-fork hashes and repeatedly log the same reorg at lower/other blocks. Clear the existing hash window (or otherwise reset detect-only state) when report_only is true and cross is Some(_).
Useful? React with 👍 / 👎.
| pub height: Option<i64>, | ||
| pub parent_slot: Option<i64>, | ||
| pub parent_hash: Option<String>, |
There was a problem hiding this comment.
I think we don't need these three.
| // rolled back alongside another chain's reorg) blocks above the target are | ||
| // reduced to hash-only rows: their data is refetched, but the scanned hashes | ||
| // stay valid for reorg detection. | ||
| @send external rollback: (t, int, ~keepHashes: bool) => unit = "rollback" |
There was a problem hiding this comment.
We should always drop hashes and all data when we roll back.
| // Drop blocks above the given block (rolled back). | ||
| @send external rollback: (t, int) => unit = "rollback" | ||
| // Hash of a stored block, if the store still holds it. | ||
| @send external getHash: (t, int) => Null.t<string> = "getHash" |
There was a problem hiding this comment.
Check whether we don't have dead code in this file and others.
| let backoffMillis = retry === 0 ? 100 : Pervasives.min(1000 * retry, 60_000) | ||
| let log = retry >= 2 ? Logging.childWarn : Logging.childTrace | ||
| logger->log({ | ||
| "msg": `Source returned an internally inconsistent ${method} response. Retrying the complete request.`, |
There was a problem hiding this comment.
This error message is confusing for end user. Let's update it to something more friendly, saying that we received a partial indicator of a reorg from the source, retrying the request to better identify whether a reorg happened or not.
2 participants
Summary
Reorg detection logic has been moved from ReScript into the Rust
BlockStore, where it can operate directly on stored block hashes during page merges. This eliminates the separateReorgDetectionmodule and integrates hash comparison into the core block storage mechanism.Key Changes
Rust BlockStore (
packages/cli/src/block_store.rs)FuelBlockFieldenum and support for Fuel block storage (height, id, time fields)HashMismatchstruct to report detected reorg events with block number and hash detailsmerge()method that compares block hashes between stored and incoming pages, detecting the lowest in-threshold mismatchget_hash()andgetHashedBlockNumbers()accessors for reading stored hashes and querying blocks within reorg thresholdfrom_js_evm(),from_js_svm(),from_js_fuel()factory methods to build pages from sparse JS block objectspending_mismatchtracking to detect hash conflicts within a single response (same block observed twice with different hashes)decode_hex_bytes()utility for strict 0x-prefixed hex validationReScript Runtime (
packages/envio/src/)ReorgDetection.ttype and all detection logic; kept only data shape types (blockData,reorgDetected)ChainStateto storeshouldRollbackOnReorgandmaxReorgDepthdirectly instead of aReorgDetectioninstanceBlockStorebindings to exposemerge()with hash comparison,getHash(), andgetHashedBlockNumbers()Batchto read block hashes fromBlockStoreinstead ofReorgDetectionRollbackto query stored hashes viaBlockStore.getHashedBlockNumbers()Test Updates
ReorgDetection_test.resto testBlockStoremerge behavior instead of the removed moduleBlockStore.fromJs()and verify hashes viaBlockStore.getHash()"0x0102"instead of"0x102") to pass validationNotable Implementation Details
fromBlockparameter); blocks below the threshold are merged without comparisonreport_onlyflag enables detect-only mode: mismatches are reported but the merge proceeds anyway, preventing re-reporting on subsequent responseshttps://claude.ai/code/session_01Aj6SS9KbG9mytdzuYnMs3a
Summary by CodeRabbit
blockStore.getBlockHashessupport returning BlockStore and request timing stats; introducedENVIO_MAX_SOURCE_RETRIESto cap retries.blockStoreinstead of a standaloneblockHashesarray; related source/update logic has been updated.