fix: compare reorg windows by l1 height#360
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
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. Comment |
There was a problem hiding this comment.
Pull request overview
This PR fixes reorg detection correctness during bootstrap / sparse via_l1_blocks coverage by ensuring comparisons are done by explicit Bitcoin L1 height (not vector position) and by seeding an empty via_l1_blocks table with the full configured reorg window.
Changes:
- Seed
via_l1_blockswith the fullreorg_windowending at the currentvia_btc_watchheight when the table is empty. - Compare DB vs canonical Bitcoin hashes by explicit height (and warn when the DB window is sparse), preventing sparse-window misalignment false positives.
- Preserve height ordering for concurrently fetched Bitcoin blocks by sorting fetch results before returning.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
core/node/via_main_node_reorg_detector/src/lib.rs |
Seeds full window on empty DB, sorts concurrent fetch results by height, and compares reorg windows by explicit L1 height (plus unit tests). |
via_verifier/node/via_reorg_detector/src/lib.rs |
Mirrors the main-node/external-node detector fixes for verifier: full-window seeding, height-ordered fetch results, explicit-height comparison (plus unit tests). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request enhances the reorg detection logic in both the main node and verifier by introducing helper functions to handle sparse L1 block windows and ensuring fetched blocks are sorted by height. It also updates the initialization process to populate the database with a window of blocks using transactions. Feedback was provided regarding the duplication of these new helper functions across different modules, suggesting they be moved to a shared utility crate to adhere to DRY principles.
| fn chain_hashes_by_height(start_height: i64, chain_blocks: &[Block]) -> Vec<(i64, String)> { | ||
| (start_height..) | ||
| .zip(chain_blocks) | ||
| .map(|(height, block)| (height, block.block_hash().to_string())) | ||
| .collect() | ||
| } | ||
|
|
||
| fn is_sparse_l1_block_window( | ||
| db_blocks: &[(i64, String)], | ||
| start_height: i64, | ||
| end_height: i64, | ||
| ) -> bool { | ||
| let expected_len = (end_height - start_height + 1) as usize; | ||
| db_blocks.len() != expected_len | ||
| || db_blocks | ||
| .windows(2) | ||
| .any(|blocks| blocks[1].0 != blocks[0].0 + 1) | ||
| || db_blocks | ||
| .first() | ||
| .is_some_and(|(height, _)| *height != start_height) | ||
| || db_blocks | ||
| .last() | ||
| .is_some_and(|(height, _)| *height != end_height) | ||
| } | ||
|
|
||
| fn find_reorg_start_block( | ||
| db_blocks: &[(i64, String)], | ||
| chain_hashes: &[(i64, String)], | ||
| ) -> anyhow::Result<Option<i64>> { | ||
| let Some((start_height, _)) = chain_hashes.first() else { | ||
| anyhow::bail!("Cannot detect reorg against an empty canonical chain window"); | ||
| }; | ||
| let Some((end_height, _)) = chain_hashes.last() else { | ||
| anyhow::bail!("Cannot detect reorg against an empty canonical chain window"); | ||
| }; | ||
|
|
||
| let mut previous_height = start_height - 1; | ||
| for (db_height, db_hash) in db_blocks { | ||
| if db_height < start_height || db_height > end_height { | ||
| anyhow::bail!( | ||
| "DB L1 block {db_height} is outside canonical comparison window {start_height}..={end_height}" | ||
| ); | ||
| } | ||
| if *db_height <= previous_height { | ||
| anyhow::bail!("DB L1 blocks are not strictly ordered by height"); | ||
| } | ||
|
|
||
| let chain_index = (*db_height - start_height) as usize; | ||
| let (chain_height, chain_hash) = &chain_hashes[chain_index]; | ||
| if chain_height != db_height { | ||
| anyhow::bail!( | ||
| "Canonical L1 block window is not indexed by expected height {db_height}; found {chain_height}" | ||
| ); | ||
| } | ||
| if chain_hash != db_hash { | ||
| return Ok(Some(*db_height)); | ||
| } | ||
|
|
||
| previous_height = *db_height; | ||
| } | ||
|
|
||
| Ok(None) | ||
| } |
There was a problem hiding this comment.
The helper functions chain_hashes_by_height, is_sparse_l1_block_window, and find_reorg_start_block are duplicated exactly in via_verifier/node/via_reorg_detector/src/lib.rs. To improve maintainability and adhere to the DRY (Don't Repeat Yourself) principle, consider moving these functions to a shared utility module (e.g., within via_btc_client or a common library crate) that both detectors can utilize.
![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: e7da86629c
ℹ️ 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".
| let start_height = block_height | ||
| .saturating_sub(self.config.reorg_window() - 1) | ||
| .max(1); | ||
| let blocks = self.fetch_blocks(start_height, block_height).await?; | ||
|
|
||
| let mut transaction = storage.start_transaction().await?; | ||
| for (height, block) in (start_height..=block_height).zip(blocks) { |
There was a problem hiding this comment.
If via_btc_watch metadata is absent or reset to 0, get_last_processed_l1_block() returns 0, so this path computes start_height = 1 and then zips (1..=0) with fetched blocks, inserting nothing into via_l1_blocks. On the next tick, detect_reorg() aborts with "No blocks found to detect reorg", which prevents sync_l1_blocks() from ever repopulating state; the service stays in a permanent error loop instead of recovering. The same regression pattern is present in the verifier detector init path as well.
Useful? React with 👍 / 👎.
| let start_height = block_height | ||
| .saturating_sub(self.config.reorg_window() - 1) | ||
| .max(1); | ||
| let blocks = self.fetch_blocks(start_height, block_height).await?; | ||
|
|
||
| let mut transaction = storage.start_transaction().await?; | ||
| for (height, block) in (start_height..=block_height).zip(blocks) { |
There was a problem hiding this comment.
When via_btc_watch last processed height is 0 (missing metadata or restart-to-zero), this computes start_height = 1 and calls fetch_blocks(1, 0), producing an empty set that inserts no via_l1_blocks rows. init() then succeeds, but every loop iteration fails in detect_reorg() with "No blocks found to detect reorg", so the verifier reorg detector never reaches sync_l1_blocks() to recover automatically.
Useful? React with 👍 / 👎.
| let Some((start_height, _)) = chain_hashes.first() else { | ||
| anyhow::bail!("Cannot detect reorg against an empty canonical chain window"); | ||
| }; |
There was a problem hiding this comment.
This new guard turns an empty canonical fetch window into a hard error, which breaks recovery when the last stored L1 block is 0 (a state the previous code tolerated). In that case detect_reorg() computes start_height = 1, fetches no chain blocks, then bails here before sync_l1_blocks() can append blocks 1..N; prior logic simply skipped comparison and allowed syncing to proceed. After upgrading with existing via_l1_blocks row 0, the detector can get stuck logging errors instead of self-healing.
Useful? React with 👍 / 👎.
Why
A fresh Via external-node or verifier database can contain
via_btc_watchbootstrap metadata beforevia_l1_blockshas dense local coverage. The existing reorg detector bootstrap inserted only the current watched Bitcoin block whenvia_l1_blockswas empty, thendetect_reorg()fetched a fixed reorg window from the DB and compared those rows to the canonical Bitcoin window by vector position.That position-based comparison is unsafe when the local DB window is sparse: a locally stored block at the bootstrap height can be compared against the first canonical block in the window instead of the canonical block with the same height. During fresh external-node bootstrap, this can falsely classify sparse local state as a Bitcoin reorg and trigger soft-reorg handling that rewinds
via_btc_watchbelow the bootstrap block. The runtime invariant should be that reorg detection compares Bitcoin block hashes by explicit L1 height, and sparse local coverage must not shift identities by row position.This source change addresses both the main-node/external-node detector and the verifier detector because they shared the same bootstrap and comparison pattern.
What changed
via_l1_blocksbootstrap now seeds the full configured reorg window ending at the currentvia_btc_watchblock instead of inserting only one row.100891from being compared against canonical block100792by position.Performance / Complexity Impact
wis the configured reorg window; this happens only whenvia_l1_blocksis empty.buffer_unorderedare sorted by height to preserve ordering.The runtime path allocates one
Vec<(height, hash)>for the canonical window comparison. The window is bounded byreorg_window, so memory growth remains bounded by existing configuration.Boundaries and non-goals
This PR changes source and tests only. No live infrastructure actions, database mutations, deployments, restarts, Kubernetes/Helm changes, secret reads, or wallet-state changes were run from this branch.
Merging this PR can affect future main-node, external-node, and verifier images built from
main; rollout to live environments remains a separate operator action after review and approval.How to review
Focus review on:
core/node/via_main_node_reorg_detector/src/lib.rsvia_verifier/node/via_reorg_detector/src/lib.rsCheck that the detector no longer relies on vector-position identity for sparse DB windows, that the bootstrap window insertion is bounded by
reorg_window, and that the main-node/external-node and verifier implementations preserve the same invariant.Checks run
git diff --check cargo fmt --package via_main_node_reorg_detector --package via_verifier_reorg_detector cargo test -p via_main_node_reorg_detector -p via_verifier_reorg_detectorzkstack dev lintand local secret scanning were not run.Author checklist
Live infrastructure and deployment impact
No live infrastructure actions were run.
Merging this PR changes source/tests only and is not expected to deploy live services by itself. Future rollout to external-node, main-node, or verifier environments requires separate deployment approval and verification.