Skip to content

fix(discovery-service): reject zero-size history pages, and correct the cursor docs - #991

Open
Yoni-Starkware wants to merge 3 commits into
mainfrom
yoni/history-preliminary-cleanups
Open

fix(discovery-service): reject zero-size history pages, and correct the cursor docs#991
Yoni-Starkware wants to merge 3 commits into
mainfrom
yoni/history-preliminary-cleanups

Conversation

@Yoni-Starkware

@Yoni-Starkware Yoni-Starkware commented Sep 10, 2026

Copy link
Copy Markdown
Collaborator

Extracted from #866 so that changes unrelated to that PR's gap-scan bug fix can land on their own. #866 is now stacked on top of this branch.

Each commit is independent of the follow-up fix and of the other two.

1. fix(discovery-service): reject max_transactions == 0 with 400

validate_bound only rejects values above the maximum, so a zero page size reached the scan. There it breaks out of the loop before any work, and can never satisfy the completion condition (result.len() < max_transactions is false at 0), so every page returns an empty 200 with an unchanged cursor. A client paginating until history_complete — which the spec instructs, and which both e2e loops do — never terminates.

Verified empirically rather than by reading: a probe calling fetch_transactions with max_transactions = 0 and a deliberately huge budget returns Ok(empty) with history_complete: false and an untouched cursor on every attempt.

No caller can regress: the field is required (no serde default), the SDK defaults to 50, and the demo/e2e use 1, 5 and 10. A client sending 0 today is already stuck in that loop.

2. docs(discovery-service): the first history request omits begin_block_number

begin_block_number is an Option<u64> and the SDK sends it as undefined, but the spec told clients to set it to 0 on the first request. Following that literally pins the scan's upper bound at block 0: every note is skipped as above the bound, the page comes back empty and marked complete, and the cursor is burned so a retry cannot recover.

Also corrects "resolves from chain head" — the code resolves from the pinned snapshot block (block_ref, or the current head when omitted), which for Tag(PreConfirmed) is cached_head + 1.

3. test(discovery-core): name the one-block budget shared by history tests

Three history tests each recomputed the same budget expression inline. Bound once as ONE_BLOCK_BUDGET so the arithmetic lives in one place and the cost constants can change without touching each test. Test-only; no production code.

Testing

cargo fmt --check clean · cargo clippy -p discovery-core -p discovery-service --all-targets 0 warnings · cargo test 156 (discovery-core) + 58 (discovery-service) + all integration suites pass.

The new validator test was proven to fail when its guard is disabled (if max_transactions == 0if falsetest_history_cursor_zero_max_transactions_rejected fails on its own unwrap_err), per the repo's "prove a new test can fail" rule. A boundary test pins that max_transactions == 1 is still accepted.

🤖 Generated with Claude Code


This change is Reviewable

Yoni-Starkware and others added 3 commits September 10, 2026 16:28
validate_bound only rejects values above the maximum, so a zero page size reached the scan. It breaks out of the loop before any work and can never satisfy the history_complete condition (result.len() < max_transactions is false at 0), so every page returns an empty 200 with an unchanged cursor and a client paginating until history_complete loops forever.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…number

begin_block_number is an Option<u64> and the SDK sends it as undefined, but the spec told clients to set it to 0 on the first request. A client following that literally pins the scan upper bound at block 0: every note is skipped as above the bound and the page comes back empty and marked complete, with the cursor burned so a retry cannot recover.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Three history tests each recomputed the same budget expression inline. Bind it once as ONE_BLOCK_BUDGET so the arithmetic lives in one place and the cost constants can change without touching each test.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant