fix: compare reorg windows by l1 height#355
Conversation
![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 reorganization detection logic in both the main node and the verifier node by introducing a new helper function, find_reorg_start_block_in_window, along with unit tests. The review identified a critical bug where the logic assumes blocks are ordered by height, but the asynchronous fetching mechanism used (buffer_unordered) does not guarantee this order, likely resulting in false-positive detections. It was also recommended to move the duplicated helper function into a shared module to improve maintainability.
| let chain_hashes = chain_blocks | ||
| .iter() | ||
| .map(|block| block.block_hash().to_string()) | ||
| .collect::<Vec<_>>(); |
There was a problem hiding this comment.
The logic here assumes that chain_blocks (and thus chain_hashes) is ordered by block height. However, the fetch_blocks implementation (lines 79-96) uses buffer_unordered, which returns blocks in the order they are received from the Bitcoin node, not necessarily in the order of the requested heights.
This will cause find_reorg_start_block_in_window to compare hashes for mismatched heights, leading to frequent false-positive reorg detections.
Fix: Change buffer_unordered to buffered (or buffer_ordered) in the fetch_blocks function to ensure the resulting Vec matches the order of the input heights.
| let chain_hashes = chain_blocks | ||
| .iter() | ||
| .map(|block| block.block_hash().to_string()) | ||
| .collect::<Vec<_>>(); |
There was a problem hiding this comment.
Similar to the main node reorg detector, this code assumes chain_blocks is ordered by height. Because fetch_blocks uses buffer_unordered, the order is non-deterministic. This will lead to incorrect reorg detection logic as hashes will be compared against the wrong heights.
Fix: Update fetch_blocks to use buffered() instead of buffer_unordered() to preserve the sequence of blocks.
| fn find_reorg_start_block_in_window( | ||
| start_height: i64, | ||
| db_blocks: &[(i64, String)], | ||
| chain_hashes: &[String], | ||
| ) -> Option<i64> { | ||
| for (db_number, db_hash) in db_blocks { | ||
| let Ok(chain_index) = usize::try_from(*db_number - start_height) else { | ||
| continue; | ||
| }; | ||
| let Some(chain_hash) = chain_hashes.get(chain_index) else { | ||
| continue; | ||
| }; | ||
|
|
||
| if chain_hash != db_hash { | ||
| return Some(*db_number); | ||
| } | ||
| } | ||
|
|
||
| None | ||
| } |
There was a problem hiding this comment.
The function find_reorg_start_block_in_window is identical to the one implemented in via_verifier/node/via_reorg_detector/src/lib.rs. To improve maintainability and avoid logic drift, consider moving this utility function to a shared internal crate or a common module accessible by both components.
There was a problem hiding this comment.
Pull request overview
This PR fixes false-positive reorg detection when via_l1_blocks is sparse by comparing DB rows to canonical Bitcoin blocks using explicit L1 block height (rather than zip position), and adds regression tests covering sparse-window scenarios in both the main-node/external-node detector and the verifier detector.
Changes:
- Replace zip-by-position DB-vs-chain comparisons with height-indexed comparisons.
- Add unit tests to cover sparse
via_l1_blockswindows (e.g., only end-height present). - Apply the same comparison logic to both reorg detector implementations.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
via_verifier/node/via_reorg_detector/src/lib.rs |
Switches reorg window comparison to be height-based and adds sparse-window regression tests. |
core/node/via_main_node_reorg_detector/src/lib.rs |
Mirrors the verifier change: height-based comparison plus sparse-window regression tests. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let chain_hashes = chain_blocks | ||
| .iter() | ||
| .map(|block| block.block_hash().to_string()) | ||
| .collect::<Vec<_>>(); | ||
|
|
||
| let reorg_start_block_height_opt = | ||
| find_reorg_start_block_in_window(start_height, &db_blocks, &chain_hashes); |
| let chain_hashes = chain_blocks | ||
| .iter() | ||
| .map(|block| block.block_hash().to_string()) | ||
| .collect::<Vec<_>>(); | ||
|
|
||
| for ((db_number, db_hash), chain_block) in db_blocks.iter().zip(chain_blocks.iter()) { | ||
| if chain_block.block_hash().to_string() != *db_hash { | ||
| tracing::warn!("Reorg detected at block {}", db_number); | ||
| reorg_start_block_height_opt = Some(*db_number); | ||
| break; | ||
| } | ||
| let reorg_start_block_height_opt = | ||
| find_reorg_start_block_in_window(start_height, &db_blocks, &chain_hashes); |
|
@codex review |
|
Codex Review: Didn't find any major issues. 🚀 ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
## Why `via-core` already had a small root `AGENTS.md` and a compact pull request template, but both were much thinner than the guidance now present in `kube-state` and `via-infra-hetzner`. That made protocol/runtime reviews easier to frame as generic code changes instead of reviews with explicit ownership boundaries, live-vs-source distinctions, validation expectations, and deployment impact notes. This repository owns runtime behavior that deployment repos consume through images, binaries, config keys, and docs. Reviewers and future operators benefit from a repo-local routing file and a PR template that clearly separate source behavior from live deployment proof, especially for external-node bootstrap, Bitcoin/L1 handling, verifier/prover paths, CLI-driven setup paths, and incident-response follow-up. The guidance was refined with GitNexus group context from the Via repository set (`vianetwork`, `via-platform-core`, and `via-infra-foundation`), direct GitNexus inspection of `via-core` component surfaces, and a full pass over the current upstream ZKsync `AGENTS.md`. Upstream ZKsync guidance was not copied wholesale; only the parts that match this Via checkout were adapted. ## What changed - Expanded the root `AGENTS.md` into a via-core-specific routing and safety guide. - Added explicit ownership boundaries between `via-core`, `kube-state`, `helm-charts`, `via-infra-hetzner`, and GCP infrastructure repos. - Added more specific via-core path guidance for: - BTC client / BTC watch / BTC sender paths - main-node and verifier reorg detectors - verifier-side BTC/DA sibling implementations - DA clients - prover, verifier, indexer, contracts, configs, Docker examples, and `zkstack_cli` - Added source-of-truth, safety, change-discipline, validation, GitNexus, and PR-description guidance for runtime/protocol work. - Adapted the upstream ZKsync `zkstack_cli` rebuild rule for Via: Rust changes under `zkstack_cli/` require rebuilding the installed CLI with `zkstackup --local` before CLI validation reflects source changes. - Adapted the upstream Forge-script lesson narrowly for this checkout: `zkstack_cli` commands that construct Forge script parameters or output paths should verify generated paths and fail clearly instead of masking missing deployments, wrong ordering, or bad paths with broad fallbacks. - Added a scoped fail-fast rule to `AGENTS.md` for deployment/upgrade orchestration scripts: avoid `try/catch` and low-level `staticcall` fallback probes when they would hide missing deployments, initialization-order bugs, or timing problems; explicit state checks are preferred. Static-call tests and precompile/EVM-behavior tests are explicitly excluded from that rule. The PR template keeps this as broader `zkstack_cli`/review-focus guidance rather than a mandatory checklist item. - Reworked `.github/pull_request_template.md` into reviewer/operator-focused sections: - `Why` - `What changed` - `Boundaries and non-goals` - `How to review` - `Checks run` - `Live infrastructure and deployment impact` ## Boundaries and non-goals - No runtime code, contracts, configs, Docker files, or workflows changed. - No live infrastructure actions were run. - No database rows, wallet state, Kubernetes resources, Helm releases, DNS records, cloud resources, or host services changed. - No secret values were added, moved, renamed, printed, or required by this PR. - The existing PR-title workflow and conventional-commit requirement remain unchanged. - This PR intentionally updates repo guidance only; it does not fix the open sparse reorg-window runtime bug in PR #355. - Upstream ZKsync's v31 ecosystem upgrade-script architecture was reviewed but not copied because this Via checkout does not contain the same `deploy-scripts/upgrade/...` files. - The upstream `try/catch` / `staticcall` rule was included only as a scoped orchestration-script rule, not as a blanket ban across all Solidity, because this checkout legitimately uses `staticcall` in test/precompile semantics code. ## How to review Focus on whether the guidance is useful for future `via-core` protocol/runtime reviews without overclaiming live deployment state. Review points: - `AGENTS.md` should route agents/reviewers to the right repo and avoid conflating source behavior with live desired state. - The listed paths should be specific enough to help with Via runtime review without becoming a brittle full source map. - The upstream ZKsync-derived guidance should be limited to parts that exist or clearly apply in this Via checkout. - `.github/pull_request_template.md` should preserve the useful checklist intent while asking for stronger context, validation, and rollout boundaries. - The template should remain broadly usable for code, docs, tests, configs, runtime safety PRs, and tooling changes. - The text should not imply that a source PR performs a deployment or live verification by itself. ## Checks run ```bash git diff --check git diff --cached --check gitleaks protect --staged --redact --no-banner curl -fsSL https://raw.githubusercontent.com/matter-labs/zksync-era/refs/heads/main/AGENTS.md -o /tmp/zksync-era-AGENTS.md NODE_NO_WARNINGS=1 gitnexus analyze --skip-agents-md "$PWD" NODE_NO_WARNINGS=1 gitnexus query -r via-core -c 'Improving via-core AGENTS.md and PR template with repo-specific runtime review guidance' -g 'Identify Via runtime components and review boundaries' -l 10 'via btc sender reorg detector external node verifier prover indexer config' NODE_NO_WARNINGS=1 gitnexus cypher -r via-core "MATCH (f:File) WHERE f.filePath CONTAINS 'via_btc_sender' OR f.filePath CONTAINS 'via_btc_client' OR f.filePath CONTAINS 'via_da_client' OR f.filePath CONTAINS 'via_btc_watch' OR f.filePath CONTAINS 'via_main_node_reorg_detector' RETURN f.name, f.filePath LIMIT 80" ``` ## Live infrastructure and deployment impact No live infrastructure actions were run. Merging this PR changes repository guidance and the default pull request body only. It is not expected to deploy live services by itself.
romanornr
force-pushed
the
fix/sparse-l1-reorg-window
branch
from
4f29b6b to
bffa3a7
Compare
Summary
via_l1_blockswindows where only the bootstrap/end height is present.Closes #354
Why
A fresh external-node database can have a sparse
via_l1_blockstable. The previous zip-by-position comparison could compare a DB row for a later height, such as100891, against the canonical hash for the first height in the reorg window, such as100792. That can false-detect a reorg and trigger soft-reorg handling even when the DB row is canonical for its actual height.Validation
Notes:
via_verifier_dalcurrently emits a pre-existing unused-variable warning inwithdrawals_dal.rs; this PR does not touch that file.Review notes
This PR intentionally keeps the change narrow: sparse DB windows are handled as a safe non-reorg path for rows whose explicit heights still match canonical chain hashes. It does not add historical backfilling; infra-side guarded recovery automation handles future fresh/sparse Hetzner EN DB repair separately.