fix: handle sparse L1 reorg detector windows#359
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 |
![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 refactors the reorg detection logic for both the main node and the verifier, introducing batch block fetching and a more robust reorg start height calculation. It also adds logic to backfill missing L1 block hashes in the database. Feedback focuses on improving maintainability by moving duplicated utility functions to a shared crate and removing a redundant validation loop in the reorg detection logic.
| fn reorg_window_start(block_height: i64, window: i64) -> i64 { | ||
| if block_height <= 1 { | ||
| block_height | ||
| } else { | ||
| block_height.saturating_sub(window.saturating_sub(1)).max(1) | ||
| } | ||
| } | ||
|
|
||
| fn find_reorg_start_height( | ||
| db_blocks: &[(i64, String)], | ||
| chain_blocks: &[(i64, String)], | ||
| start_height: i64, | ||
| end_height: i64, | ||
| ) -> anyhow::Result<Option<i64>> { | ||
| let expected_len = (end_height - start_height + 1) as usize; | ||
| if chain_blocks.len() != expected_len { | ||
| anyhow::bail!( | ||
| "Fetched {} canonical L1 blocks for expected window {}..={}", | ||
| chain_blocks.len(), | ||
| start_height, | ||
| end_height | ||
| ); | ||
| } | ||
|
|
||
| let chain_blocks_by_height = chain_blocks | ||
| .iter() | ||
| .map(|(height, hash)| (*height, hash.as_str())) | ||
| .collect::<HashMap<_, _>>(); | ||
|
|
||
| for height in start_height..=end_height { | ||
| if !chain_blocks_by_height.contains_key(&height) { | ||
| anyhow::bail!("Canonical L1 block window is missing height {}", height); | ||
| } | ||
| } | ||
|
|
||
| if db_blocks.len() != expected_len { | ||
| tracing::warn!( | ||
| "Sparse via_l1_blocks reorg window {}..={}: found {} of {} DB rows; comparing only stored heights", | ||
| start_height, | ||
| end_height, | ||
| db_blocks.len(), | ||
| expected_len | ||
| ); | ||
| } | ||
|
|
||
| for (db_number, db_hash) in db_blocks { | ||
| let Some(chain_hash) = chain_blocks_by_height.get(db_number) else { | ||
| anyhow::bail!( | ||
| "DB L1 block {} is outside canonical reorg window", | ||
| db_number | ||
| ); | ||
| }; | ||
|
|
||
| if *chain_hash != db_hash { | ||
| tracing::warn!("Reorg detected at block {}", db_number); | ||
| return Ok(Some(*db_number)); | ||
| } | ||
| } | ||
|
|
||
| Ok(None) | ||
| } |
There was a problem hiding this comment.
The functions reorg_window_start and find_reorg_start_height (and their associated tests) are duplicated exactly between the main node and verifier reorg detector implementations. To improve maintainability and ensure that logic changes are applied consistently, consider moving these shared utilities to a common crate or a shared module within the workspace.
| for height in start_height..=end_height { | ||
| if !chain_blocks_by_height.contains_key(&height) { | ||
| anyhow::bail!("Canonical L1 block window is missing height {}", height); | ||
| } | ||
| } |
There was a problem hiding this comment.
This loop is redundant because the length of chain_blocks is already verified against the expected window size on line 370. Since chain_blocks is constructed from a continuous range of heights in fetch_blocks, a correct length guarantees that all heights in the window are present.
There was a problem hiding this comment.
Pull request overview
This PR hardens Via’s Bitcoin L1 reorg detection logic against sparse/partially-populated via_l1_blocks tables (common on fresh external-node / verifier DBs) by ensuring comparisons are performed by explicit L1 height rather than by positional zipping of DB rows with canonical windows.
Changes:
- Carry
(height, block)through async Bitcoin block fetches and explicitly sort results afterbuffer_unordered. - Bootstrap an empty
via_l1_blockstable by inserting the full configured reorg window (ending at the currentvia_btc_watchheight) instead of a single row. - Compare stored DB block hashes to canonical hashes by height, and backfill missing-but-matching window entries via
ON CONFLICT DO NOTHINGinserts; add regression tests for sparse-window behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| via_verifier/node/via_reorg_detector/src/lib.rs | Verifier reorg detector now fetches/sorts blocks with heights, compares DB↔canonical by height, bootstraps/backfills full windows, and adds sparse-window regression tests. |
| core/node/via_main_node_reorg_detector/src/lib.rs | Main-node/external-node reorg detector applies the same height-keyed canonical comparison and full-window bootstrap/backfill behavior, plus matching regression tests. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| fn reorg_window_start(block_height: i64, window: i64) -> i64 { | ||
| if block_height <= 1 { | ||
| block_height | ||
| } else { | ||
| block_height.saturating_sub(window.saturating_sub(1)).max(1) | ||
| } | ||
| } |
Why
A fresh Via external-node or verifier database can start with
via_btc_watchmetadata and wallet-related state whilevia_l1_blocksis empty or only partially populated. The reorg detector used that table as the local Bitcoin-chain checkpoint source, but the bootstrap path inserted only the currentvia_btc_watchheight when the table was empty.That single-row bootstrap was unsafe because reorg detection later fetched a full fixed-size canonical Bitcoin window and compared DB rows to canonical rows by iterator position. With a sparse local window ending at block
100891, the first DB row for100891could be compared to the first canonical row for100792, producing a false reorg. On an external node, soft-reorg handling can then resetvia_btc_watchbelow the wallet bootstrap block, making wallet rows invisible after restart and causing startup failures.The runtime invariant enforced by this PR is that reorg detection must compare Bitcoin block hashes by explicit L1 height, not by positional assumptions about locally stored rows. Sparse local
via_l1_blockscoverage is treated as a bootstrap/sync gap unless a stored height actually disagrees with the canonical Bitcoin hash for the same height.What changed
via_l1_blocksbootstrap now inserts the full reorg window ending at the currentvia_btc_watchblock instead of inserting only one row.ON CONFLICT DO NOTHINGinserts.100891while the canonical comparison window is100792..=100891.Performance / Complexity Impact
buffer_unordered;nis bounded by the reorg window.nis bounded by the reorg window.via_l1_blocksis empty; creates a coherent detector window.ON CONFLICT DO NOTHING; only runs when DB coverage is sparse and no reorg is detected.The added allocations are bounded by the configured reorg window: one vector of
(height, hash)pairs and one height-to-hash lookup map during detection.Boundaries and non-goals
This PR updates source code and tests only. No live infrastructure actions, database writes, wallet state changes, Kubernetes/Helm changes, host restarts, or deployments were run from this branch.
Merging this PR can affect future main-node, external-node, and verifier images built from
main. Rollout to Hetzner external nodes, verifier nodes, or any other live environment remains a separate operator approval and verification step after review.Deployment desired state in sibling infrastructure repositories is not changed by this PR. The referenced Hetzner external-node rehearsal is operational evidence for the source bug, not proof of current live state.
How to review
Focus on the reorg detector invariants in:
core/node/via_main_node_reorg_detector/src/lib.rsvia_verifier/node/via_reorg_detector/src/lib.rsThe main review points are:
100891against canonical block100792by position;Checks run
cargo test -p via_verifier_reorg_detectoremitted an existing unrelated warning invia_verifier/lib/verifier_dal/src/withdrawals_dal.rsfor an unusedresultvariable.Author checklist
Live infrastructure and deployment impact
No live infrastructure actions were run.
Merging this PR changes source and tests only and is not expected to deploy live services by itself. Future images built from
mainwill include the corrected reorg detector behavior; live rollout remains a separate deployment approval and verification step.