Don't query reorgable blocks before all chains reach reorg threshold - #680
Conversation
WalkthroughBlockLag is now a non-optional int threaded through FetchState.make/updateInternal; queue-based helper removed and replaced by FetchState.isReadyToEnterReorgThreshold; Prometheus gets a reorg-threshold gauge; ChainFetcher/ChainManager and GlobalState APIs and batch flows updated to use FetchState.t and config-driven blockLag; tests adapted. Changes
Sequence Diagram(s)sequenceDiagram
participant GS as GlobalState
participant CM as ChainManager
participant FS as FetchState
participant PM as Prometheus.ReorgThreshold
GS->>CM: getFetchStateWithData() → ChainMap<FetchState.t>
loop per chain
GS->>FS: isReadyToEnterReorgThreshold(~currentBlockHeight)
FS-->>GS: bool
end
alt any chain ready
GS->>GS: dispatch EnterReorgThreshold
GS->>PM: set(~isInReorgThreshold=true)
loop per chain
GS->>FS: updateInternal(~blockLag=..., ...)
FS-->>GS: updated FetchState.t
end
end
GS->>CM: createBatch(~maxBatchSize)
CM-->>GS: {items, fetchStates, ...}
sequenceDiagram
participant CF as ChainFetcher.make
participant Cfg as Config
participant FC as FetchState.make
CF->>Cfg: read shouldRollbackOnReorg & enableRawEvents
CF->>CF: compute isInReorgThreshold (param)
CF->>CF: blockLag = max((shouldRollbackOnReorg || isInReorgThreshold) ? 0 : chain.confirmedBlockThreshold, Env.indexingBlockLag|0)
CF->>FC: make(..., ~blockLag)
FC-->>CF: FetchState.t
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used📓 Path-based instructions (1)**/*.{res,resi}📄 CodeRabbit Inference Engine (.cursor/rules/rescript.mdc)
Files:
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (7)
✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
scenarios/test_codegen/test/lib_tests/SourceManager_test.res (1)
297-300: Tests aligned to concrete blockLagSwitching to blockLag: 0 in mockFetchState is correct given the new int field.
Consider adding one test where blockLag > currentBlockHeight to assert we don’t schedule queries and we wait for new blocks (especially if you adopt the headBlock clamp).
codegenerator/cli/npm/envio/src/Prometheus.res (1)
475-485: Global reorg-threshold gauge added; consider per-chain visibility (optional)The global envio_reorg_threshold gauge is fine if it represents “all chains have entered reorg threshold”. If you need per-chain introspection for debugging, add a labeled gauge variant keyed by chainId.
I can wire a ReorgThresholdPerChain gauge via SafeGauge with chainId labels, and set it alongside the global flag.
scenarios/test_codegen/test/lib_tests/FetchState_test.res (1)
2663-2794: Solid coverage for isReadyToEnterReorgThresholdGood boundary tests: endBlock reached, head - blockLag boundary, no endBlock, and queue non-empty. If you clamp threshold to non-negative, add a case with currentBlockHeight < blockLag to ensure readiness stays false until at least block 0 is fully fetched.
I can add that extra test if you adopt the clamp.
codegenerator/cli/templates/static/codegen/src/eventFetching/ChainFetcher.res (1)
137-142: Consider refactoring the blockLag calculation for better readability.The nested ternary operator makes the logic harder to follow. Consider extracting this into a more readable format:
- ~blockLag=Pervasives.max( - !(config->Config.shouldRollbackOnReorg) || isInReorgThreshold - ? 0 - : chainConfig.confirmedBlockThreshold, - Env.indexingBlockLag->Option.getWithDefault(0), - ), + ~blockLag={ + let reorgBlockLag = if !(config->Config.shouldRollbackOnReorg) || isInReorgThreshold { + 0 + } else { + chainConfig.confirmedBlockThreshold + } + Pervasives.max(reorgBlockLag, Env.indexingBlockLag->Option.getWithDefault(0)) + },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
codegenerator/cli/npm/envio/src/FetchState.res(7 hunks)codegenerator/cli/npm/envio/src/Prometheus.res(1 hunks)codegenerator/cli/templates/static/codegen/src/eventFetching/ChainFetcher.res(8 hunks)codegenerator/cli/templates/static/codegen/src/eventFetching/ChainManager.res(10 hunks)codegenerator/cli/templates/static/codegen/src/globalState/GlobalState.res(5 hunks)scenarios/test_codegen/test/ChainManager_test.res(3 hunks)scenarios/test_codegen/test/lib_tests/FetchState_test.res(10 hunks)scenarios/test_codegen/test/lib_tests/SourceManager_test.res(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{res,resi}
📄 CodeRabbit Inference Engine (.cursor/rules/rescript.mdc)
**/*.{res,resi}: Never use[| item |]to create an array. Use[ item ]instead.
Must always use=for setting value to a field. Use:=only for ref values created usingreffunction.
ReScript has record types which require a type definition before hand. You can access record fields by dot likefoo.myField.
It's also possible to define an inline object, it'll have quoted fields in this case.
Use records when working with structured data, and objects to conveniently pass payload data between functions.
Never use %raw to access object fields if you know the type.
Files:
codegenerator/cli/npm/envio/src/Prometheus.resscenarios/test_codegen/test/lib_tests/SourceManager_test.resscenarios/test_codegen/test/lib_tests/FetchState_test.rescodegenerator/cli/npm/envio/src/FetchState.rescodegenerator/cli/templates/static/codegen/src/globalState/GlobalState.rescodegenerator/cli/templates/static/codegen/src/eventFetching/ChainFetcher.resscenarios/test_codegen/test/ChainManager_test.rescodegenerator/cli/templates/static/codegen/src/eventFetching/ChainManager.res
codegenerator/cli/npm/envio/**
📄 CodeRabbit Inference Engine (.cursor/rules/navigation.mdc)
Library-fied runtime shared across indexers lives in codegenerator/cli/npm/envio.
Files:
codegenerator/cli/npm/envio/src/Prometheus.rescodegenerator/cli/npm/envio/src/FetchState.res
codegenerator/cli/templates/{dynamic/**/*.hbs,static/**}
📄 CodeRabbit Inference Engine (.cursor/rules/navigation.mdc)
Templates live under codegenerator/cli/templates: dynamic/ for Handlebars (.hbs), static/ for raw Rescript files copied verbatim.
Files:
codegenerator/cli/templates/static/codegen/src/globalState/GlobalState.rescodegenerator/cli/templates/static/codegen/src/eventFetching/ChainFetcher.rescodegenerator/cli/templates/static/codegen/src/eventFetching/ChainManager.res
{**/generated/src/**/*.res,codegenerator/cli/templates/static/codegen/src/**/*.res,codegenerator/cli/templates/dynamic/codegen/src/**/*.res}
📄 CodeRabbit Inference Engine (.cursor/rules/navigation.mdc)
Runtime code lives in each project’s generated/src, but template versions (good for editing) are under codegenerator/cli/templates/static/codegen/src or codegenerator/cli/templates/dynamic/codegen/src.
Files:
codegenerator/cli/templates/static/codegen/src/globalState/GlobalState.rescodegenerator/cli/templates/static/codegen/src/eventFetching/ChainFetcher.rescodegenerator/cli/templates/static/codegen/src/eventFetching/ChainManager.res
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build_and_test
🔇 Additional comments (11)
codegenerator/cli/npm/envio/src/FetchState.res (2)
232-233: Threading blockLag via updateInternal looks goodPropagating ~blockLag explicitly makes the state updates clearer and avoids stale reads. Once clamped as suggested, this path is robust.
Also applies to: 288-289
826-835: Validation complete: EndBlock({toBlock: …}) never receives a negative value
I searched all ReasonML files forEndBlock({toBlock:and found only:
- A type definition in scenarios/helpers/src/Indexer.res
- Constant instantiations in tests (8 and 0)
- The existing fallback at line 654 in codegenerator/cli/npm/envio/src/FetchState.res
Since
headBlockis clamped upstream and yourblockLaglogic safely boundsendBlock, no negativetoBlockcan occur. No further clamping or code changes are needed.scenarios/test_codegen/test/ChainManager_test.res (2)
280-286: blockLag set to 0 in test fetch statesSetting blockLag: 0 is consistent with the new non-optional field type.
157-171: All outdated references removed – no action needed
onlyBelowReorgThresholdis no longer present in the codebase.- Remaining
fetchStatesMapoccurrences in tests (ChainManager_test.res) and GlobalState.res reflect the intended updated API.No further updates are required.
codegenerator/cli/templates/static/codegen/src/eventFetching/ChainFetcher.res (1)
77-87: LGTM! Clear logic for event inclusion.The refactored logic properly uses the config to determine event inclusion and provides helpful logging when events won't be indexed.
codegenerator/cli/templates/static/codegen/src/globalState/GlobalState.res (3)
916-921: Verify the isBelowReorgThreshold logic when rollback is disabled.The current logic sets
isBelowReorgThresholdtofalsewhenshouldRollbackOnReorgisfalse. This means the reorg threshold check will never trigger when rollback is disabled. Is this intentional?If rollback is disabled, should the system still track when chains enter the reorg threshold for other purposes (e.g., metrics, history saving)?
654-677: Well-structured implementation of reorg threshold entry.The action properly updates all chain fetchers with the new blockLag value and sets the appropriate flags. The Prometheus metric update ensures proper monitoring.
939-1005: Excellent simplification of batch processing logic.The refactored code is much cleaner with:
- Direct batch handling without nested conditions
- Clear separation of concerns between reorg threshold checking and batch processing
- Proper error handling and rollback state management
codegenerator/cli/templates/static/codegen/src/eventFetching/ChainManager.res (3)
44-61: Good simplification of type system.Removing the intermediate
fetchStateWithDatatype and usingFetchState.tdirectly reduces complexity and improves code readability.
75-83: Consider improving isInReorgThreshold recovery logic.The comment indicates that the current approach might incorrectly recover the
isInReorgThresholdstate. While noted as "not a problem," this could lead to subtle issues if other parts of the system rely on this state being accurate after restart.Consider adding a more reliable way to persist and recover the
isInReorgThresholdstate, perhaps by storing it explicitly in the database rather than inferring it from the presence of history rows.
251-310: Well-implemented batch creation with proper state management.The updated batch creation properly:
- Makes deep copies of fetch states to avoid mutations
- Correctly chooses between ordered and unordered batch strategies
- Handles dynamic contracts storage appropriately
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
scenarios/test_codegen/test/lib_tests/FetchState_test.res (2)
2775-2793: Also gate on in-flight queries, not only queue emptiness.Right now this test ensures a non-empty queue blocks readiness. Consider adding a case where the queue is empty but a partition has fetchingStateId set (i.e., an in-flight query). If the implementation doesn’t already account for that, readiness could be reported too early.
Follow-up test suggestion to add within this describe block:
it("Returns false when a partition has an active fetch (in-flight query)", () => { let fs = makeInitial() // Simulate an in-flight query to set fetchingStateId let q: FetchState.query = { partitionId: "0", target: Head, selection: fs.normalSelection, addressesByContractName: Js.Dict.empty(), fromBlock: 0, indexingContracts: fs.indexingContracts, } fs->FetchState.startFetchingQueries(~queries=[q], ~stateId=0) Assert.equal(fs->FetchState.isReadyToEnterReorgThreshold(~currentBlockHeight=10), false) })
2795-2811: Test title is misleading; scenario duplicates the “endBlock reached & empty queue” path.This case passes because endBlock is reached and the queue is empty. The large blockLag doesn’t affect the outcome here. Consider:
- Renaming the test to reflect why it passes (endBlock reached with empty queue), or
- Adjusting the scenario to demonstrate that blockLag alone can gate readiness when endBlock is not reached.
Example rename:
- it("Returns true when the queue is empty and threshold is more than current block height", () => { + it("Returns true when endBlock is reached and the queue is empty (irrespective of blockLag)", () => {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
codegenerator/cli/templates/static/codegen/src/globalState/GlobalState.res(6 hunks)scenarios/test_codegen/test/lib_tests/FetchState_test.res(10 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{res,resi}
📄 CodeRabbit Inference Engine (.cursor/rules/rescript.mdc)
**/*.{res,resi}: Never use[| item |]to create an array. Use[ item ]instead.
Must always use=for setting value to a field. Use:=only for ref values created usingreffunction.
ReScript has record types which require a type definition before hand. You can access record fields by dot likefoo.myField.
It's also possible to define an inline object, it'll have quoted fields in this case.
Use records when working with structured data, and objects to conveniently pass payload data between functions.
Never use %raw to access object fields if you know the type.
Files:
scenarios/test_codegen/test/lib_tests/FetchState_test.rescodegenerator/cli/templates/static/codegen/src/globalState/GlobalState.res
codegenerator/cli/templates/{dynamic/**/*.hbs,static/**}
📄 CodeRabbit Inference Engine (.cursor/rules/navigation.mdc)
Templates live under codegenerator/cli/templates: dynamic/ for Handlebars (.hbs), static/ for raw Rescript files copied verbatim.
Files:
codegenerator/cli/templates/static/codegen/src/globalState/GlobalState.res
{**/generated/src/**/*.res,codegenerator/cli/templates/static/codegen/src/**/*.res,codegenerator/cli/templates/dynamic/codegen/src/**/*.res}
📄 CodeRabbit Inference Engine (.cursor/rules/navigation.mdc)
Runtime code lives in each project’s generated/src, but template versions (good for editing) are under codegenerator/cli/templates/static/codegen/src or codegenerator/cli/templates/dynamic/codegen/src.
Files:
codegenerator/cli/templates/static/codegen/src/globalState/GlobalState.res
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build_and_test
🔇 Additional comments (9)
scenarios/test_codegen/test/lib_tests/FetchState_test.res (2)
157-157: LGTM: blockLag assertions updated to non-optional int.Good job updating expected shapes to include blockLag: 0 across tests to reflect the new non-optional field. This keeps the test expectations aligned with the FetchState.t shape change.
Also applies to: 226-226, 289-289, 385-385, 905-905, 966-966, 1867-1867, 2181-2181
2663-2812: Comprehensive coverage for isReadyToEnterReorgThreshold.The new suite exercises key boundaries: endBlock reached, head - blockLag threshold (including equality), absence of endBlock, and queue non-emptiness gating. This aligns with the PR objective to avoid querying reorgable blocks prematurely.
codegenerator/cli/templates/static/codegen/src/globalState/GlobalState.res (7)
78-78: LGTM! Appropriate Prometheus metrics initialization.Good addition of the reorg threshold metric initialization to track when the indexer enters/exits the reorg threshold state.
610-647: Well-structured reorg threshold detection logic.The implementation correctly:
- Checks all chain fetchers are ready to enter reorg threshold
- Updates the
isInReorgThresholdstate appropriately- Only updates fetchState with blockLag when entering the threshold
The logic flow is clear and handles the state transitions properly.
680-703: Clean implementation of the EnterReorgThreshold action.The action handler properly:
- Logs the threshold entry
- Updates Prometheus metrics
- Applies blockLag to all chain fetchers
- Sets the global reorg threshold state
- Triggers re-querying of all chains
The separation of this logic into a dedicated action improves code organization.
733-733: Correct usage of FetchState.t type.The direct assignment of
fetchStatefrom thefetchStatesMapproperly reflects the updated type signature whereFetchState.tis now used directly instead of being wrapped in another type.
938-960: Effective pre-batch gating for reorg threshold entry.The logic correctly checks if all chains are ready to enter the reorg threshold before processing a batch, dispatching
EnterReorgThresholdwhen conditions are met. This prevents processing events while transitioning into the threshold state.
963-999: Simplified batch processing with proper metrics updates.The refactored batch processing logic is cleaner and more maintainable. The switch statement clearly handles empty vs non-empty batches, and the metrics updates are properly implemented per chain.
635-636: VerifyblockLagdefault fallbackThere’s a lingering FIXME in
Env.resnoting that falling back to0“broke HS grafana dashboard.” We currently uselet indexingBlockLag = envSafe->EnvSafe.get("ENVIO_INDEXING_BLOCK_LAG", S.option(S.int)) …->Option.getWithDefault(0)in both ChainFetcher and GlobalState without any external docs calling out the expected default.
Please confirm or adjust as needed:
- Ensure a default of
0(viaOption.getWithDefault(0)) is the correct semantic whenENVIO_INDEXING_BLOCK_LAGis unset.- If another fallback makes more sense (e.g. omitting the parameter, a positive lag, etc.), update the code and remove/fix the FIXME.
- Consider adding a note to your README or environment docs describing the behavior of
ENVIO_INDEXING_BLOCK_LAG.Key locations:
- codegenerator/cli/templates/static/codegen/src/Env.res
- codegenerator/cli/templates/static/codegen/src/eventFetching/ChainFetcher.res
- codegenerator/cli/templates/static/codegen/src/globalState/GlobalState.res
| | 0 => endBlock | ||
| | _ => |
There was a problem hiding this comment.
Nit: maybe an if is more appropriate here
| isInReorgThresholdRef := | ||
| isInReorgThresholdRef.contents || { | ||
| let {currentBlockHeight, highestBlockBelowThreshold} = | ||
| fetchStatesMap->ChainMap.get(chain) | ||
| earliestEvent->FetchState.queueItemIsInReorgThreshold( | ||
| ~currentBlockHeight, | ||
| ~highestBlockBelowThreshold, | ||
| ) | ||
| } | ||
|
|
There was a problem hiding this comment.
Nice, simplifies this stuff a lot
| | SetIsInReorgThreshold(isInReorgThreshold) => | ||
| if isInReorgThreshold { |
There was a problem hiding this comment.
Just checking we never used to exit reorg threshold right?
2 participants
Summary by CodeRabbit
New Features
Refactor
Tests