Handle Solana skipped slots in block fetching and unfinalized block t…#30
Conversation
…racking getBlock throws a SolanaError (block not available / slot skipped) for skipped slots instead of returning null, which was crashing the node. Convert these to BlockUnavailableError in fetchBlock and getHeaderByHeight, and allow non-consecutive heights when registering unfinalized blocks.
Coverage reportCaution Test run failed
Test suite run failedFailed tests: 4/57. Failed suites: 1/8.Report generated by 🧪jest coverage report action from 82c99e9 |
BLOCK_NOT_AVAILABLE and LONG_TERM_STORAGE_SLOT_SKIPPED don't reliably mean a slot was skipped (the former can be a transient node-lag signal, the latter an archival backfill gap), so silently converting them to BlockUnavailableError risked masking real blocks instead of just crashing on them.
![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: 1a3905c18c
ℹ️ 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 ( | ||
| lastUnfinalizedHeight !== undefined && | ||
| lastUnfinalizedHeight + 1 !== header.blockHeight | ||
| lastUnfinalizedHeight >= header.blockHeight |
There was a problem hiding this comment.
After a skipped slot, Solana's next block has parentSlot set to the last produced ancestor, while our solanaBlockToHeader still reports blockHeight as parentSlot + 1. For example, if slot 103 is skipped then fetching slot 104 produces a header at 103; this relaxed >= check then lets slot 105 register after that bogus 103, so unfinalized metadata drifts from the actual slot numbers instead of failing fast. Please carry the requested slot into the header before allowing non-consecutive heights.
Useful? React with 👍 / 👎.
| return ( | ||
| isSolanaError( | ||
| e, | ||
| SOLANA_ERROR__JSON_RPC__SERVER_ERROR_BLOCK_NOT_AVAILABLE, |
There was a problem hiding this comment.
When an RPC node is lagging or load-balanced, -32004 means the block is not retrievable from that node yet, not necessarily that the slot is permanently skipped; the existing getFinalizedBlockHeader comment even relies on retrying this case. Including BLOCK_NOT_AVAILABLE here converts those transient responses into BlockUnavailableError, bypassing handleError/backoff and letting callers treat a real slot as skipped. Restrict this helper to the permanent skipped-slot codes, or retry -32004 before mapping it to unavailable.
Useful? React with 👍 / 👎.
getBlock responses don't include their own slot, only the parent's. Deriving blockHeight as parentSlot + 1 is wrong whenever the previous slot was skipped, which let bogus heights into unfinalized block tracking. fetchBlock and getHeaderByHeight now pass the slot they actually requested through to the header.
BaseWorkerService.fetchBlock calls toBlockResponse with the unwrapped BlockContent, not the IBlock, so the requested slot couldn't be passed through the same way as fetchBlock/getHeaderByHeight. Track the requested slot per block in a WeakMap set in fetchChainBlock and read it back in toBlockResponse, so worker-threaded mode reports the same correct height as the non-worker path.
📝 WalkthroughWalkthroughAdds skipped-slot handling for Solana: ChangesSolana Skipped Slot Handling
Sequence Diagram(s)sequenceDiagram
participant Worker as WorkerService
participant API as SolanaApi
participant RPC as Solana RPC
participant Header as solanaBlockToHeader
participant Unfinalized as UnfinalizedBlocksService
Worker->>API: fetchChainBlock(heights)
API->>RPC: getBlock(slot)
alt slot skipped
RPC-->>API: JSON-RPC slot skipped error
API->>API: isSkippedSlotError(err)
API-->>Worker: throw BlockUnavailableError
else block returned
RPC-->>API: BlockResponse
API-->>Worker: BlockContent
Worker->>Worker: blockSlots.set(block, heights)
Worker->>Header: solanaBlockToHeader(block, slot)
Header-->>Worker: Header { blockHeight: slot }
Worker->>Unfinalized: registerUnfinalizedBlock(header)
alt height gap detected
Unfinalized->>API: backfill intermediate headers
API->>RPC: getBlock(missingHeight)
RPC-->>API: BlockResponse or skipped error
API-->>Unfinalized: Header or BlockUnavailableError
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed. For unrecoverable errors, disable the tool in CodeRabbit configuration. 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: 1
🤖 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/node/src/solana/block.solana.ts`:
- Around line 266-273: The formatBlockUtil function in solanaDictionaryV2.ts is
being called without providing the slot parameter, which prevents
solanaBlockToHeader from correctly calculating block heights when slots are
skipped. Locate the call to formatBlockUtil(parseBlock(...)) in
solanaDictionaryV2.ts and ensure the slot parameter is passed to formatBlockUtil
so that solanaBlockToHeader can properly determine the correct blockHeight
instead of incorrectly deriving it via parentSlot + 1 when prior slots are
skipped.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: ea694e58-52ec-4017-9d2b-1fed6d4debd3
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (5)
packages/node/package.jsonpackages/node/src/indexer/unfinalizedBlocks.service.tspackages/node/src/indexer/worker/worker.service.tspackages/node/src/solana/api.solana.tspackages/node/src/solana/block.solana.ts
| export function formatBlockUtil<B extends SolanaBlock = SolanaBlock>( | ||
| block: B, | ||
| slot?: number, | ||
| ): IBlock<B> { | ||
| return { | ||
| block, | ||
| getHeader: () => solanaBlockToHeader(block), | ||
| getHeader: () => solanaBlockToHeader(block, slot), | ||
| }; |
There was a problem hiding this comment.
Skipped-slot correctness is still bypassed in one known call path.
solanaBlockToHeader is only correct on skipped slots when slot is propagated, but packages/node/src/indexer/dictionary/v2/solanaDictionaryV2.ts still calls formatBlockUtil(parseBlock(...)) without slot and then uses getHeader().blockHeight for lastBufferedHeight. That path can still derive incorrect heights via parentSlot + 1 when prior slots are skipped.
Also applies to: 281-283
🤖 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/node/src/solana/block.solana.ts` around lines 266 - 273, The
formatBlockUtil function in solanaDictionaryV2.ts is being called without
providing the slot parameter, which prevents solanaBlockToHeader from correctly
calculating block heights when slots are skipped. Locate the call to
formatBlockUtil(parseBlock(...)) in solanaDictionaryV2.ts and ensure the slot
parameter is passed to formatBlockUtil so that solanaBlockToHeader can properly
determine the correct blockHeight instead of incorrectly deriving it via
parentSlot + 1 when prior slots are skipped.
…build-breaking expression removeUnfinalizedBelowSafeHeight pruned records using a fixed distance from the current head rather than the finalized height. When finalization lags more than UNFINALIZED_THRESHOLD behind the head (exactly the condition this service exists to guard against), it could delete the only record hasForked() needed at or below the finalized height, silently skipping fork detection. deleteFinalizedBlock already prunes safely once a fork check has passed, so the extra pruning is removed rather than re-targeted at finalizedBlockNumber. Also fixes a build failure: api.solana.spec.ts and decoder.spec.ts had `process.env.HTTP_ENDPOINT ?? '<literal>' ?? '<literal>'`, where the second `??` is always non-nullish, making the third operand dead code. The currently used TypeScript version flags this as an error (TS2881), which broke @subql/node-solana's build.
Covers: forks behind missed slots, rebuilding the parent chain across real and skipped slots, and rolling back when a backfilled or new block doesn't connect to the expected parent.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/node/src/solana/api.solana.spec.ts (2)
22-22: ⚡ Quick winClarify or update the API key comment.
The comment suggests an API key is required, but the endpoint on line 24 doesn't include one. If an API key should be added via
process.env.HTTP_ENDPOINT, consider updating the comment to clarify this (e.g., "// Set HTTP_ENDPOINT env var with API key to avoid rate limits"). If no API key is needed for basic testing, consider removing the comment to avoid confusion.🤖 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/node/src/solana/api.solana.spec.ts` at line 22, Update the comment on line 22 to clarify the actual approach for handling API keys. Either update it to explain that an API key should be provided via the HTTP_ENDPOINT environment variable (e.g., "Set HTTP_ENDPOINT env var with API key to avoid rate limits"), or remove the comment entirely if no API key is required for basic testing. Ensure the comment accurately reflects whether and how the HTTP_ENDPOINT endpoint on the following line uses authentication.
23-24: Endpoint used for general block tests; verify if standardization across test files is needed.This file's HTTP_ENDPOINT is used for general block and transaction parsing tests (e.g., fetching block 325922873), not for testing skipped-slot error handling. The codebase does use inconsistent endpoints across test files:
api.solana.spec.tsanddecoder.spec.tsuse mainnet-beta, whileblockchain.service.test.ts,api.service.solana.spec.ts, andsolanaDictionaryV2.spec.tsuse onfinality. If standardizing test endpoints is desired for maintainability, consider aligning them across all spec files.🤖 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/node/src/solana/api.solana.spec.ts` around lines 23 - 24, The HTTP_ENDPOINT constant in this file uses mainnet-beta, but other test files across the codebase use inconsistent endpoints (mainnet-beta in some files, onfinality in others like blockchain.service.test.ts and api.service.solana.spec.ts). Standardize the HTTP_ENDPOINT values by choosing a single endpoint configuration and updating all test spec files (api.solana.spec.ts, decoder.spec.ts, blockchain.service.test.ts, api.service.solana.spec.ts, and solanaDictionaryV2.spec.ts) to use the same endpoint value for consistency and maintainability across the test suite.
🤖 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.
Nitpick comments:
In `@packages/node/src/solana/api.solana.spec.ts`:
- Line 22: Update the comment on line 22 to clarify the actual approach for
handling API keys. Either update it to explain that an API key should be
provided via the HTTP_ENDPOINT environment variable (e.g., "Set HTTP_ENDPOINT
env var with API key to avoid rate limits"), or remove the comment entirely if
no API key is required for basic testing. Ensure the comment accurately reflects
whether and how the HTTP_ENDPOINT endpoint on the following line uses
authentication.
- Around line 23-24: The HTTP_ENDPOINT constant in this file uses mainnet-beta,
but other test files across the codebase use inconsistent endpoints
(mainnet-beta in some files, onfinality in others like
blockchain.service.test.ts and api.service.solana.spec.ts). Standardize the
HTTP_ENDPOINT values by choosing a single endpoint configuration and updating
all test spec files (api.solana.spec.ts, decoder.spec.ts,
blockchain.service.test.ts, api.service.solana.spec.ts, and
solanaDictionaryV2.spec.ts) to use the same endpoint value for consistency and
maintainability across the test suite.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 74bf70b0-1e55-4dc2-8bbb-ba11af4200fb
📒 Files selected for processing (4)
packages/node/src/indexer/unfinalizedBlocks.service.spec.tspackages/node/src/indexer/unfinalizedBlocks.service.tspackages/node/src/solana/api.solana.spec.tspackages/node/src/solana/decoder.spec.ts
✅ Files skipped from review due to trivial changes (1)
- packages/node/src/solana/decoder.spec.ts
1 participant
…racking
getBlock throws a SolanaError (block not available / slot skipped) for skipped slots instead of returning null, which was crashing the node. Convert these to BlockUnavailableError in fetchBlock and getHeaderByHeight, and allow non-consecutive heights when registering unfinalized blocks.
Description
Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.
Fixes # (issue)
Type of change
Please delete options that are not relevant.
Checklist
Summary by CodeRabbit
Release Notes
Bug Fixes
Chores