retry solana blocks if amount of transactions is suspicious #424
Conversation
| let validator = getResultValidator(nullable(GetBlock)) | ||
| let block = validator(result) | ||
| if (this.txThreshold && block != null && block.transactions != null) { | ||
| let transactions = block.transactions.filter(tx => !isVoteTransaction(tx)) |
There was a problem hiding this comment.
my bad i didn't understand my measurements at first. it doesn't take ~30ms to filter it's just ~0.030ms in average
There was a problem hiding this comment.
Pull request overview
This PR adds retry logic for Solana getBlock RPC requests when the number of non-vote transactions in a block is suspiciously low (below a configurable threshold). The feature helps handle cases where RPC nodes may return incomplete block data.
Changes:
- Adds transaction threshold validation to retry
getBlockrequests with fewer non-vote transactions than expected - Introduces
ThresholdRequestsclass to track retry attempts per slot (max 3 retries) - Adds optional
txThresholdparameter throughout the stack (CLI, RPC client, worker)
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 11 comments.
| File | Description |
|---|---|
| solana/solana-rpc/src/rpc.ts | Core implementation: adds validateGetBlockResult method, ThresholdRequests class, and constructor changes for threshold support |
| solana/solana-rpc/src/rpc-worker.ts | Passes txThreshold from worker options to Rpc constructor |
| solana/solana-rpc/src/rpc-remote.ts | Adds txThreshold to RemoteRpcOptions type definition |
| solana/solana-dump/src/dumper.ts | Adds CLI option --tx-threshold and passes it to RemoteRpcPool |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| inc(slot: number) { | ||
| if (this.inner.size > 100) { | ||
| let keys = this.inner.keys() | ||
| for (let i = 0; i < 20; i++) { | ||
| let res = keys.next() | ||
| this.inner.delete(assertNotNull(res.value)) | ||
| } | ||
| } |
There was a problem hiding this comment.
The ThresholdRequests.inc method uses a non-deterministic cleanup strategy that deletes the first 20 entries from the Map when size exceeds 100. Since Map iteration order is insertion order, this means older slots get removed. However, if batch requests are processed out-of-order or slots are retried non-sequentially, this could incorrectly delete entries for slots that are still being retried. Consider using a more robust cleanup strategy, such as removing entries for slots that are outside the current processing window, or using a timestamp-based eviction policy.
| program.option('--max-confirmation-attempts <N>', 'Maximum number of confirmation attempts', positiveInt, 10) | ||
| program.option('--assert-log-messages-not-null', 'Check if tx.meta.logMessages is not null', false) | ||
| program.option('--validate-chain-continuity', 'Check if block parent hash matches previous block hash', false) | ||
| program.option('--tx-threshold <N>', 'Retry getBlock call if transactions count is less than threshold') |
There was a problem hiding this comment.
The CLI option for tx-threshold does not specify a parser function (like positiveInt) to validate the input. This means the value will be parsed as a string rather than a number, which will cause issues when passed to the Rpc constructor. Add positiveInt as the parser function to ensure proper validation and type conversion.
| program.option('--tx-threshold <N>', 'Retry getBlock call if transactions count is less than threshold') | |
| program.option('--tx-threshold <N>', 'Retry getBlock call if transactions count is less than threshold', positiveInt) |
| if (retries < 3) { | ||
| this.requests.inc(slot) | ||
| throw new RetryError(`transactions count is less than threshold: ${transactions.length} < ${this.txThreshold}`) | ||
| } |
There was a problem hiding this comment.
The retry counting logic increments the counter before throwing the RetryError. This creates an off-by-one issue where the actual number of retries will be one less than expected. When retries reaches 2, it increments to 3, but then the next attempt (which should be the 4th total attempt) won't retry because retries < 3 will be false. The condition should check retries < 2 if you want to allow 3 total attempts (1 initial + 2 retries), or increment after throwing if you want the current behavior to match the intended semantics.
|
|
||
|
|
||
| class ThresholdRequests { | ||
| inner: Map<number, number> |
There was a problem hiding this comment.
The 'inner' field of ThresholdRequests is not marked as private, making it accessible from outside the class. This breaks encapsulation and allows external code to directly modify the internal state. Consider making this field private to maintain proper encapsulation.
| inner: Map<number, number> | |
| private inner: Map<number, number> |
| validateGetBlockResult(result: unknown, req: RpcRequest) { | ||
| let validator = getResultValidator(nullable(GetBlock)) | ||
| let block = validator(result) | ||
| if (this.txThreshold && block != null && block.transactions != null) { | ||
| let transactions = block.transactions.filter(tx => !isVoteTransaction(tx)) | ||
| if (transactions.length < this.txThreshold) { | ||
| let slot = req.params![0] as any as number | ||
| let retries = this.requests.get(slot) | ||
| if (retries < 3) { | ||
| this.requests.inc(slot) | ||
| throw new RetryError(`transactions count is less than threshold: ${transactions.length} < ${this.txThreshold}`) | ||
| } | ||
| } | ||
| } | ||
| return block | ||
| } |
There was a problem hiding this comment.
The validateGetBlockResult method lacks documentation explaining the retry logic and the threshold behavior. Consider adding a JSDoc comment that explains: (1) what txThreshold represents, (2) why vote transactions are filtered out before checking the threshold, (3) the maximum number of retries (currently 3), and (4) what happens when retries are exhausted (the block is returned as-is).
| @@ -18,6 +18,7 @@ export type RemoteRpcOptions = Pick< | |||
| * Remove vote transactions from all relevant responses | |||
| */ | |||
| noVotes?: boolean | |||
There was a problem hiding this comment.
The txThreshold field in RemoteRpcOptions lacks documentation. Consider adding a JSDoc comment explaining what this parameter controls, similar to how the noVotes field is documented. This helps users of the API understand the purpose and usage of this option.
| noVotes?: boolean | |
| noVotes?: boolean | |
| /** | |
| * Limit the number of transactions returned for a block or response. | |
| * If set, blocks with more transactions than this threshold may be truncated. | |
| */ |
| if (this.txThreshold != null) { | ||
| assert(this.txThreshold > 0) | ||
| } |
There was a problem hiding this comment.
The assertion checks that txThreshold > 0, but there's no check for what happens when txThreshold is 0. While the condition 'if (this.txThreshold && ...)' on line 148 would treat 0 as falsy and skip the validation, it would be clearer to either: (1) explicitly document that 0 disables the feature, or (2) include 0 in the assertion if it's not a valid value. This improves code clarity and prevents confusion about the intended behavior.
| if (this.txThreshold && block != null && block.transactions != null) { | ||
| let transactions = block.transactions.filter(tx => !isVoteTransaction(tx)) | ||
| if (transactions.length < this.txThreshold) { | ||
| let slot = req.params![0] as any as number |
There was a problem hiding this comment.
The type cast 'as any as number' is used to extract the slot from req.params. While this works, it's fragile and bypasses TypeScript's type safety. Consider using a more type-safe approach, such as checking that req.params is defined and that the first element is a number before casting, or defining a proper type for the getBlock request parameters.
| let slot = req.params![0] as any as number | |
| if (!Array.isArray(req.params) || typeof req.params[0] !== 'number') { | |
| throw new DataValidationError('invalid getBlock request parameters: expected numeric slot as first parameter') | |
| } | |
| let slot = req.params[0] |
| let retries = this.requests.get(slot) | ||
| if (retries < 3) { | ||
| this.requests.inc(slot) | ||
| throw new RetryError(`transactions count is less than threshold: ${transactions.length} < ${this.txThreshold}`) |
There was a problem hiding this comment.
The error message uses 'transactions count' which is grammatically awkward. Consider changing it to 'transaction count' (singular) for better readability, as 'count' is already a measure of quantity.
| program.option('--max-confirmation-attempts <N>', 'Maximum number of confirmation attempts', positiveInt, 10) | ||
| program.option('--assert-log-messages-not-null', 'Check if tx.meta.logMessages is not null', false) | ||
| program.option('--validate-chain-continuity', 'Check if block parent hash matches previous block hash', false) | ||
| program.option('--tx-threshold <N>', 'Retry getBlock call if transactions count is less than threshold') |
There was a problem hiding this comment.
The description for the tx-threshold option mentions 'transactions count' which is grammatically awkward (similar to the error message in rpc.ts). Consider changing it to 'transaction count' for consistency and better readability.
this change supposed to retry
getBlockrequest if amount of transactions is suspiciously small