2518 - Importer Cursor Pagination

[gventino-cw]

PR Type

Enhancement

---

Description

• Add cursor-based pagination to importer RPC methods

• Implement client fetcher and page reducers

• Preserve backward compatibility for legacy clients

• Expose config flag and error for oversized items

---

Diagram Walkthrough

flowchart LR
  A["Importer Client"] -- "opt-in / send cursor" --> B["RPC Server"]
  B -- "paginated response (block, receipts/changes, pagination)" --> A
  A -- "fetch nextCursor until none" --> B

Loading

File Walkthrough

	Relevant files
Enhancement	6 files importer_pagination.rs Add pagination engine for importer RPC methods                      +1437/-0 importer_pagination.rs Implement client-side paginated page fetcher                          +803/-0  external_block.rs Add merge and length methods for ExternalBlock                      +147/-1  pagination.rs Introduce generic pagination traits and fetcher                    +189/-0  server.rs Integrate pagination into RPC handlers                                      +24/-14  blockchain_client.rs Use PaginationClient in fetch methods                                        +3/-22
Configuration changes	1 files config.rs Add pagination_enabled configuration flag                                +11/-0
Miscellaneous	2 files mod.rs Export new pagination types and pagination request              +11/-0    mod.rs Include importer_pagination module                                              +1/-0
Formatting	1 files importer_supervisor.rs Fix enum arm formatting for FakeLeader case                            +3/-2
Error handling	1 files error.rs Add error for oversized pagination item                                    +6/-0
Additional files	6 files e2e-leader-follower.yml +1/-1      e2e-test.yml +9/-0      leader-follower-pagination.test.ts +169/-0  e2e-pagination-budget.test.ts +149/-0  e2e-pagination.test.ts +176/-0  justfile +23/-0

---

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit bae6dfb)

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis ✅ 2518 - Fully compliant Compliant requirements: Implement mechanism to paginate responses larger than max_response_size_bytes. Add cursor-based pagination to importer RPC methods. Implement client fetcher and page reducers. Preserve backward compatibility for legacy clients. Expose config flag and error for oversized items.

⏱️ Estimated effort to review: 5 🔵🔵🔵🔵🔵

🧪 PR contains tests

🔒 No security concerns identified

⚡ No major issues detected

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to bae6dfb
Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Fix item-count vs byte-size mismatch The pagination.limit value is an item count, not a byte size, so comparing it against RESPONSE_CAP_BYTES mixes units and will fail. Replace this check with a meaningful assertion about item counts or remove the byte-based lower bound. e2e/test/external/e2e-pagination-budget.test.ts [30-34] expect(page.pagination.limit) - .to.be.greaterThan(RESPONSE_CAP_BYTES / 2) - .and.to.be.at.most( - RESPONSE_CAP_BYTES, - ); + .to.be.greaterThan(0) + .and.to.be.a('number'); Suggestion importance[1-10]: 8 __ Why: The test incorrectly compares page.pagination.limit (an item count) against bytes, so fixing this assertion prevents unit mismatch and test failures.	Medium
General	Simplify cursor decoding You can simplify and harden cursor parsing by collecting the three expected parts in one step and validating their count. Using collect_tuple makes it clear you expect exactly three segments and reduces manual error cases. src/eth/rpc/types/importer_pagination.rs [652-667] fn decode_cursor(cursor: &str) -> Result<(Self, usize), Self::Error> { - let mut parts = cursor.split(':'); - let version = parts.next().ok_or(RpcError::ParameterInvalid)?; - let block_hash = parts.next().ok_or(RpcError::ParameterInvalid)?; - let next_index = parts.next().ok_or(RpcError::ParameterInvalid)?; - if version != IMPORTER_CURSOR_VERSION || parts.next().is_some() { + let (version, hash_str, idx_str) = cursor + .split(':') + .collect_tuple() + .ok_or(RpcError::ParameterInvalid)?; + if version != IMPORTER_CURSOR_VERSION { return Err(RpcError::ParameterInvalid); } - Ok(( - Self { - block_hash: block_hash.parse().map_err(|_| RpcError::ParameterInvalid)?, - }, - next_index.parse().map_err(|_| RpcError::ParameterInvalid)?, - )) + let block_hash = hash_str.parse().map_err(|_| RpcError::ParameterInvalid)?; + let next_index = idx_str.parse().map_err(|_| RpcError::ParameterInvalid)?; + Ok((Self { block_hash }, next_index)) } Suggestion importance[1-10]: 6 __ Why: Using collect_tuple from itertools ensures exactly three segments are parsed and reduces boilerplate, improving robustness.	Low
	Validate receipt–transaction alignment In addition to checking blockHash, verify that each receipt’s transactionHash matches one of the fetched transaction hashes to ensure receipts line up with the corresponding transactions. e2e/test/external/e2e-pagination.test.ts [56-58] for (const receipt of allReceipts) { expect(receipt.blockHash).to.equal(blockHash); + expect(receipt.transactionHash).to.be.oneOf(reassembledTxHashes); } Suggestion importance[1-10]: 6 __ Why: Adding a check that each receipt.transactionHash is one of the fetched hashes strengthens the test by ensuring receipts align to the correct transactions.	Low
	Simplify cursor formatting Use explicit positional format arguments to avoid reliance on named capture and improve clarity. This makes the template easier to read and less error-prone. src/eth/rpc/types/importer_pagination.rs [648-650] impl CursorCodec for BlockHashCursor { fn encode_cursor(&self, next_index: usize) -> String { - format!("{IMPORTER_CURSOR_VERSION}:{}:{next_index}", self.block_hash) + format!("{}:{}:{}", IMPORTER_CURSOR_VERSION, self.block_hash, next_index) } } Suggestion importance[1-10]: 5 __ Why: This stylistic change refactors format! to positional arguments, improving clarity with minimal impact on functionality.	Low
	Clarify page-validation guard Split the combined if let … && … into nested if let and then an inner if to improve readability and compatibility with older Rust editions. src/eth/rpc/blockchain_client/importer_pagination.rs [88-91] -if let Some(last) = *last_cursor_index - && next_index <= last -{ - bail!("paginated {context} did not advance: cursor index went from {last} to {next_index}"); +if let Some(last) = *last_cursor_index { + if next_index <= last { + bail!( + "paginated {context} did not advance: cursor index went from {last} to {next_index}" + ); + } } Suggestion importance[1-10]: 3 __ Why: Splitting the combined if let … && … into nested if statements marginally enhances readability without changing logic.	Low

---

Previous suggestions

Suggestions up to commit 681c886

Category	Suggestion	Impact
Possible issue	Fix bail! formatting placeholders Replace the named placeholders in the bail! macros with positional {} placeholders and pass the corresponding variables as arguments. This ensures the format strings compile correctly and include the intended context and values. src/eth/rpc/blockchain_client/importer_pagination.rs [66-80] fn validate_progress(page: &ImporterPageInfo, expected_total: &mut Option<usize>, context: &str) -> anyhow::Result<Option<String>> { if page.returned == 0 && page.next_cursor.is_some() { - bail!("paginated {context} returned no items but provided a next cursor"); + bail!( + "paginated {} returned no items but provided a next cursor", + context + ); } match expected_total { - Some(expected_total) if *expected_total != page.total => { - bail!("paginated {context} changed total from {expected_total} to {}", page.total); + Some(total) if *total != page.total => { + bail!( + "paginated {} changed total from {} to {}", + context, + total, + page.total + ); } Some(_) => {} None => *expected_total = Some(page.total), } Ok(page.next_cursor.clone()) } Suggestion importance[1-10]: 9 __ Why: This fixes compile errors in validate_progress by replacing unsupported named placeholders with positional {} formatting, enabling the bail! macros to work correctly.	High
	Use positional placeholders in bail! Correct the format string in the bail!macro by using {} placeholders for both values and passing page_block_number and self.block_number as positional arguments to avoid compile errors. src/eth/rpc/blockchain_client/importer_pagination.rs [178-184] impl BlockWithChangesPages { fn push_block(&mut self, page_block: BlockRocksdb) -> anyhow::Result<()> { let page_block_number = BlockNumber::from(page_block.header.number); if page_block_number != self.block_number { bail!( - "paginated block with changes returned unexpected block number {page_block_number} instead of {}", + "paginated block with changes returned unexpected block number {} instead of {}", + page_block_number, self.block_number ); } // ... } } Suggestion importance[1-10]: 8 __ Why: Changing the bail! macro to use {} placeholders and positional arguments corrects the format string error in push_block, ensuring it compiles and displays the values properly.	Medium
	Correct bail! placeholders in finish Replace the named placeholders in the bail! call with positional {} and supply total and expected_total as arguments so the error message formats correctly. src/eth/rpc/blockchain_client/importer_pagination.rs [236-244] impl PageReducer<BlockWithChangesPageResponse> for BlockWithChangesPages { fn finish(self) -> anyhow::Result<Option<Self::Output>> { let Some(block) = self.block else { return Ok(None); }; let expected_total = self.expected_total.unwrap_or_default(); let total = block.transactions.len() + self.changes.account_changes.len() + self.changes.slot_changes.len(); if total != expected_total { - bail!("paginated block with changes assembled {total} items but expected {expected_total}"); + bail!( + "paginated block with changes assembled {} items but expected {}", + total, + expected_total + ); } Ok(Some((block, self.changes))) } } Suggestion importance[1-10]: 8 __ Why: Updating the bail! call in finish to use positional formatting fixes a compilation issue and ensures the error message includes the correct total and expected_total values.	Medium

[cloudwalk-review-agent]

Summary

Great change set. The pagination design is cohesive across server slicing (ImporterPagination), client reassembly (PaginatedPageFetcher + reducers), and domain validation (ExternalBlock transaction merge helpers). I reviewed for correctness/safety around partial data assembly, cursor integrity, and mismatch detection; the implementation has strong defensive checks (stable total, block identity/hash checks, duplicate state-change guards, and not-found-after-partial failure behavior).

Test coverage is substantial and targeted at failure modes that matter for importer safety (cursor parsing, bounds handling, multi-section slicing, mismatch/error paths, and reducer finalization). I did not find any concrete blocking issues in the provided diff/context.

[cloudwalk-review-agent]

Summary

Solid implementation of cursor pagination across server and importer sides, with strong defensive checks for consistency (stable totals, block identity/hash continuity, duplicate change detection, and partial-not-found failure). The generic paginator/fetcher abstractions are coherent, and test coverage is meaningful across happy paths and important failure modes.

I did not find any concrete blocking issues in the provided diff/context.

[carneiro-cw]

Did not fully review yet, but add an e2e test for: requests that need pagination, requests that dont but ask for pagination, legacy requests without the pagination param.

edit: i guess the last one already exists

[gventino-cw]

Did not fully review yet, but add an e2e test for: requests that need pagination, requests that dont but ask for pagination, legacy requests without the pagination param.

edit: i guess the last one already exists

ok, will do it

[carneiro-cw]

Also would be good to understand the behavior if say, a follower is deployed with this new feature, but leader does not have it yet. Is that follower prepared to deal with that?
This is important for when we first deploy this. Either make the follower deal with either response version or add a config to enable/disable pagination.

[cloudwalk-review-agent]

Summary

Nice progress on introducing generic cursor pagination and importer-side reassembly; the reducer validation and tests are generally strong. I found one blocking deployment-safety issue related to mixed-version leader/follower compatibility.

Blocking

• The importer client now always calls stratus_getBlockAndReceipts / stratus_getBlockWithChanges with a second pagination param and expects paginated response shapes. If follower is upgraded before leader (raised in PR discussion), older leaders that only support legacy signatures/tuple responses can fail decode or reject params, causing importer sync interruption during rollout.

Please add backward compatibility in the client fetch path (e.g., fallback to legacy one-param call and legacy response decode when paginated call fails with method/params/shape mismatch), or gate pagination usage behind a config/feature flag for staged deploys.

Other than that, coverage and invariants look good.

[gventino-cw]

Also would be good to understand the behavior if say, a follower is deployed with this new feature, but leader does not have it yet. Is that follower prepared to deal with that? This is important for when we first deploy this. Either make the follower deal with either response version or add a config to enable/disable pagination.

currently old follower -> new leader works fine. But old leader <- new follower it will not work, because the follower at this version always send the request expecting a paginated response.

[cloudwalk-review-agent]

Summary

Solid implementation overall: pagination reducers, cursor decoding, reassembly invariants, and test coverage are strong. However, there is one blocking deploy-safety issue still present in the importer client path.

Blocking

The follower importer now always sends the paginated 2-param RPC form and expects paginated/object-compatible responses, but there is no request-level fallback when talking to an older leader that only supports legacy signatures/response shapes. In a mixed-version rollout (new follower -> old leader), this can fail RPC decode/params validation and interrupt syncing.

[gventino-cw]

Also would be good to understand the behavior if say, a follower is deployed with this new feature, but leader does not have it yet. Is that follower prepared to deal with that? This is important for when we first deploy this. Either make the follower deal with either response version or add a config to enable/disable pagination.

currently old follower -> new leader works fine. But old leader <- new follower it will not work, because the follower at this version always send the request expecting a paginated response.

fixed it

[stratus-benchmark]

Benchmark:
Run ID: bench-3e4f02af

Git Info:

• Branch: feat/2518-pagination
• Commit: d3085fd
• PR: 2518 - Importer Cursor Pagination #2626

Leader Stats:
RPS Stats: Max: 8394.00, Min: 1978.00, Avg: 3045.58, StdDev: 438.74
TPS Stats: Max: 3351.00, Min: 17.00, Avg: 2975.75, StdDev: 334.69

Follower Stats:
Imported Blocks/s: Max: 5.00, Min: 1.00, Avg: 2.06, StdDev: 0.86
Imported Transactions/s: Max: 11575.00, Min: 2744.00, Avg: 6132.46, StdDev: 2519.63

Plots:

• View Leader Plot
• View Follower Plot

[cloudwalk-review-agent]

Summary

Great work — this PR introduces a cohesive pagination model across server and importer client with strong compatibility handling and meaningful guardrails.

What I verified in the provided diff/context:

• Server now supports optional cursor pagination for both stratus_getBlockAndReceipts and stratus_getBlockWithChanges, while preserving legacy one-shot shapes when pagination is not needed.
• Importer client fetch path now supports both paginated and legacy response forms (BlockWithChangesPageResponse custom deserializer + optional pagination), addressing mixed-version rollout risk.
• Reducers include solid consistency checks (stable total, cursor monotonicity, block identity/hash continuity, duplicate change detection, tx/receipt count matching, partial-not-found fail-fast).
• Test coverage is substantial and includes e2e round-trips plus targeted unit tests for cursor decoding, clamping, section slicing, byte-budget behavior, backward compatibility, and reducer invariants.

I did not find any concrete blocking issues in the shown changes.

[stratus-benchmark]

Benchmark:
Run ID: bench-2b42bcfa

Git Info:

• Branch: feat/2518-pagination
• Commit: 15962ab
• PR: 2518 - Importer Cursor Pagination #2626

Leader Stats:
RPS Stats: Max: 10084.00, Min: 2559.00, Avg: 3051.53, StdDev: 425.93
TPS Stats: Max: 3380.00, Min: 450.00, Avg: 2989.00, StdDev: 292.72

Follower Stats:
Imported Blocks/s: Max: 7.00, Min: 1.00, Avg: 4.61, StdDev: 1.01
Imported Transactions/s: Max: 17864.00, Min: 3325.00, Avg: 13767.50, StdDev: 2895.16

Plots:

• View Leader Plot
• View Follower Plot

[stratus-benchmark]

Benchmark:
Run ID: bench-436fffc2

Git Info:

• Branch: feat/2518-pagination
• Commit: a9cd119
• PR: 2518 - Importer Cursor Pagination #2626

Leader Stats:
RPS Stats: Max: 8433.00, Min: 1525.00, Avg: 2970.45, StdDev: 374.99
TPS Stats: Max: 3424.00, Min: 264.00, Avg: 2928.45, StdDev: 320.40

Follower Stats:
Imported Blocks/s: Max: 8.00, Min: 2.00, Avg: 4.75, StdDev: 1.08
Imported Transactions/s: Max: 23744.00, Min: 873.00, Avg: 13910.16, StdDev: 3443.14

Plots:

• View Leader Plot
• View Follower Plot

[cloudwalk-review-agent]

Summary

Strong change set overall: pagination is implemented end-to-end (RPC handlers + importer client), with explicit backward-compat handling for legacy one-shot shapes, cursor validation, and consistency checks during reassembly. I also verified meaningful coverage was added across unit and e2e paths (legacy behavior, multi-page assembly, byte-budget constraints, and leader/follower round-trips).

I did not find concrete blocking issues in the provided diff/context.

[github-actions]

Persistent review updated to latest commit bae6dfb

[cloudwalk-review-agent]

Rescue Review — Re-approval

Retry on the same head (bae6dfb, no new commits since my earlier platform-deep approval). Re-verified the diff and open threads: nothing new to flag. The explicit limit override is already gated behind #[cfg(any(test, feature = "dev"))], addressing the remaining member feedback. Sticking with APPROVE. 🦉

[gventino-cw]

extract to should paginate tuple (bool)

[gventino-cw]

maybe change this from a naive json response and implement a string stream

[gventino-cw]

the PR code it's well structured and feature rich. But we don't need all of that, a simple text stream should get the job done. I'll put this and draft and try to do it in the simpler way possible.