Skip to content

fix(data): require first-segment snapshot seeds - #129

Merged
proerror77 merged 4 commits into
mainfrom
codex/binance-verifier-snapshot-seed
Jul 18, 2026
Merged

proerror77 merged 4 commits into
mainfrom
codex/binance-verifier-snapshot-seed

Conversation

@proerror77

@proerror77 proerror77 commented Jul 18, 2026

Copy link
Copy Markdown
Owner

Change contract

Reject a first Binance market-tape segment whose replay state is established only by a checkpoint; every declared symbol must be seeded by a preceding snapshot in the first segment.

Out of scope

Dependency / merge order

None. This PR was originally stacked on #128; after #128 merged it was retargeted to main, updated, and revalidated at exact head 28bb82b06a3e156ef8576b69a93f08e4711358e4.

Focused validation

  • Review budget: 1 changed file, below the 25-file / 750-line split threshold.
  • Red GitHub/Linux counterexample: run 29638371502 failed only first_segment_checkpoint_cannot_replace_snapshot_seed; the old verifier incorrectly returned a verified handle for session_start + agg_trade + replay-safe checkpoint without a snapshot.
  • Green GitHub/Linux implementation run: 29638487525 passed rustfmt and all 8 focused verifier tests.
  • Exact-head GitHub/Linux focused run 29639024408 passed rustfmt and all verifier tests for 28bb82b06a3e156ef8576b69a93f08e4711358e4.
  • Exact-head standard CI run 29638735090 passed Rust Workspace, Rust HFT Engine Fast Lane, production image/Kubernetes contract, and the repository security lanes.
  • Safety boundary preserved: later segments may remain snapshot-free; the existing cross-segment replay test continues through to its intended sequence-gap assertion.

Rollout / rollback impact

Verifier-only fail-closed tightening. No runtime service changes. Rollback is reverting this PR; first segments without snapshot seeds remain rejected while active.

@coderabbitai

coderabbitai Bot commented Jul 18, 2026

Copy link
Copy Markdown

Warning

Review limit reached

@proerror77, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 35 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 5efc5f21-f8ef-4c85-a3dd-ab6443256c20

📥 Commits

Reviewing files that changed from the base of the PR and between f15dc2d and 28bb82b.

📒 Files selected for processing (1)
  • rust_hft/data-pipelines/core/src/binance_market_tape_artifact.rs
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch codex/binance-verifier-snapshot-seed

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@proerror77
proerror77 force-pushed the codex/binance-verifier-snapshot-seed branch from 43b41ac to f682005 Compare July 18, 2026 08:50
@proerror77
proerror77 force-pushed the codex/binance-verifier-snapshot-seed branch from f682005 to 877503d Compare July 18, 2026 08:52
@proerror77
proerror77 changed the base branch from codex/binance-verifier-aggtrade to main July 18, 2026 09:09
@proerror77
proerror77 marked this pull request as ready for review July 18, 2026 09:24
@proerror77

Copy link
Copy Markdown
Owner Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jul 18, 2026

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@proerror77
proerror77 merged commit e8b5506 into main Jul 18, 2026
17 checks passed

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 28bb82b06a

ℹ️ 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".

require_symbol(&symbols, &symbol)?;
if event_type == "checkpoint" {
if event_type == "snapshot" {
snapshot_seeds.insert(symbol.clone());

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Record only replay-accepted snapshot seeds

For first-segment tapes where a snapshot row uses non-canonical casing such as "btcusdt", this normalized insert marks BTCUSDT as seeded even though ReplaySequenceValidator::observe only accepts exact symbol matches for snapshots (binance_lob_replay.rs:136), so replay ignores that snapshot. A session_start + lowercase snapshot + agg_trade + replay-safe checkpoint segment with no diffs can therefore pass this new guard and still have the checkpoint establish the book, which is the counterexample this change is meant to reject; require canonical row symbols or record the seed only after replay accepts the snapshot.

Useful? React with 👍 / 👎.

@proerror77
proerror77 deleted the codex/binance-verifier-snapshot-seed branch July 19, 2026 04:27
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