fix(reorg-detector): compare L1 blocks by explicit height on sparse via_l1_blocks#358
fix(reorg-detector): compare L1 blocks by explicit height on sparse via_l1_blocks#358romanornr wants to merge 1 commit into
Conversation
…ia_l1_blocks Both the main-node/external-node reorg detector and the verifier reorg detector fetched the canonical reorg window with futures::buffer_unordered and then compared local DB rows to canonical rows by positional zip. With a freshly bootstrapped via_l1_blocks containing only the wallet/bootstrap block (for example 100891), the 100-block detector window started at 100792, so the single DB row at 100891 was compared against the canonical block at index 0 (height 100792). The hashes did not match and a false reorg at the wallet bootstrap block was reported. On the main node, soft-reorg handling then reset the via_btc_watch cursor below the wallet bootstrap block and broke wallet-resource startup on the next restart. Changes (apply to both detectors): - fetch_blocks now returns Vec<(i64, Block)> with explicit heights and ascending sort, so callers no longer depend on buffer_unordered preserving request order. This also removes the latent ordering bug in sync_l1_blocks where new blocks were inserted under heights derived from the request range rather than the actually-fetched block. - detect_reorg builds a HashMap<i64, String> of canonical hashes keyed by height and delegates to a pure scan_for_reorg helper that compares each DB row by its explicit number, never positionally. - New ReorgScan::SparseAt(height) variant: when the canonical fetch lacks a height the local DB knows about, the detector logs and returns without acting instead of falsely demoting the BtcWatch cursor. Regression coverage: six unit tests per crate, including sparse_db_does_not_falsely_compare_against_window_start, which reproduces the exact 100891-vs-100792 scenario observed during the Hetzner private external-node rehearsal. No DAL signatures, schema, config keys, metric names, or deployment desired state were changed. init() still seeds a single bootstrap row; the height-keyed comparison makes that safe rather than requiring a coherent full-window backfill. Amp-Thread-ID: https://ampcode.com/threads/T-019e3766-dbfa-71c9-865a-bd24243ae5e6 Co-authored-by: Amp <amp@ampcode.com>
There was a problem hiding this comment.
Pull request overview
This PR fixes Via L1 reorg detection so sparse via_l1_blocks state is compared against canonical BTC blocks by explicit height rather than positional order, preventing false reorg detection during bootstrap-like states.
Changes:
- Pairs fetched BTC blocks with their requested heights and sorts after
buffer_unordered. - Replaces positional
zipreorg comparison with a height-keyedHashMapscan helper. - Adds mirrored unit coverage for main-node and verifier sparse-window regression cases.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
core/node/via_main_node_reorg_detector/src/lib.rs |
Updates main-node fetch, sync, and reorg detection logic to use explicit block heights. |
core/node/via_main_node_reorg_detector/src/tests.rs |
Adds unit tests for sparse DB and height-keyed reorg scanning behavior. |
via_verifier/node/via_reorg_detector/src/lib.rs |
Applies the same height-keyed fetch and reorg scan behavior to the verifier detector. |
via_verifier/node/via_reorg_detector/src/tests.rs |
Adds verifier-side unit tests mirroring main-node reorg scan coverage. |
💡 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 reorg detection for both the main node and verifier by implementing height-keyed block comparison, replacing the previous positional approach. It introduces a ReorgScan enum and a scan_for_reorg utility to explicitly handle cases where the local database or canonical chain data is sparse, preventing false reorg reports. Review feedback highlights that the fetch_blocks implementation is currently too strict because it fails the entire operation on any single RPC error. This prevents the newly added SparseAt logic from handling transient failures gracefully, so it is recommended to allow partial results in the fetch logic.
| let mut blocks: Vec<(i64, Block)> = results | ||
| .into_iter() | ||
| .collect::<Result<_, _>>() | ||
| .context("Failed to fetch blocks")?; |
There was a problem hiding this comment.
The current implementation of fetch_blocks is strict and will fail the entire iteration if a single block fetch fails due to the use of collect::<Result<Vec<_>, _>>(). This effectively makes the newly introduced ReorgScan::SparseAt logic in detect_reorg unreachable, as scan_for_reorg will only ever receive a complete set of blocks or not be called at all if an error occurs.
Consider making fetch_blocks more lenient by allowing partial results. This would allow the reorg detector to gracefully skip the check (via the SparseAt variant) when the RPC is temporarily missing some blocks, rather than logging a high-level error and failing the whole iteration.
let mut blocks: Vec<(i64, Block)> = results
.into_iter()
.filter_map(|res| match res {
Ok(b) => Some(b),
Err(err) => {
tracing::warn!("Failed to fetch block from canonical chain: {err}");
None
}
})
.collect();
if blocks.is_empty() && from_block_height <= to_block_height {
anyhow::bail!("Failed to fetch any blocks from the canonical chain");
}| let mut blocks: Vec<(i64, Block)> = results | ||
| .into_iter() | ||
| .collect::<Result<_, _>>() | ||
| .context("Failed to fetch blocks")?; |
There was a problem hiding this comment.
Similar to the main node detector, fetch_blocks here is strict, which prevents the ReorgScan::SparseAt logic from being utilized. Making it lenient by allowing partial results would enable the intended behavior of skipping the reorg check during transient RPC issues instead of failing the iteration with an error.
let mut blocks: Vec<(i64, Block)> = results
.into_iter()
.filter_map(|res| match res {
Ok(b) => Some(b),
Err(err) => {
tracing::warn!("Failed to fetch block from canonical chain: {err}");
None
}
})
.collect();
if blocks.is_empty() && from_block_height <= to_block_height {
anyhow::bail!("Failed to fetch any blocks from the canonical chain");
}
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
📜 Recent review details🧰 Additional context used📓 Path-based instructions (10)**/*.rs📄 CodeRabbit inference engine (AGENTS.md)
Files:
⚙️ CodeRabbit configuration file
Files:
!(src/**/*.rs|tests/**/*.rs)📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/via_*/**/*.{rs,toml}📄 CodeRabbit inference engine (.github/pull_request_template.md)
Files:
**/{via_btc_watch,via_btc_client,*reorg_detector}/**/*.rs📄 CodeRabbit inference engine (.github/ISSUE_TEMPLATE/l1_btc_reorg_sync_bug.yml)
Files:
**/{via_btc_watch,via_btc_client,via_btc_sender,*reorg_detector,*via_l1_block_dal}/**/*.rs📄 CodeRabbit inference engine (.github/ISSUE_TEMPLATE/l1_btc_reorg_sync_bug.yml)
Files:
via_verifier/node/via_reorg_detector/**⚙️ CodeRabbit configuration file
Files:
**/via_*reorg_detector*/**⚙️ CodeRabbit configuration file
Files:
**/via_*/**⚙️ CodeRabbit configuration file
Files:
core/node/via_main_node_reorg_detector/**⚙️ CodeRabbit configuration file
Files:
**/*{test,spec}*.rs📄 CodeRabbit inference engine (.github/ISSUE_TEMPLATE/l1_btc_reorg_sync_bug.yml)
Files:
🔇 Additional comments (4)
WalkthroughBoth reorg detectors refactor block fetching and comparison logic to use explicit height-keyed matching instead of positional iteration. A new pure ChangesReorg Detector Height-Keyed Refactoring
Possibly Related Issues
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
Why
A Via external-node bootstrap observed during the Hetzner private external-node
rehearsal produced a false soft reorg at the wallet bootstrap block and broke
wallet-resource startup on the next restart. The failure mode was rooted in
the L1 reorg detector and was not specific to that operator run; any fresh or
sparse
via_l1_blockstable on a node that already has avia_btc_watchcursor at the wallet bootstrap height could trigger the same path.
Two source defects combined to produce the false positive:
core/node/via_main_node_reorg_detectorandvia_verifier/node/via_reorg_detectorfetched the 100-block canonicalreorg window with
futures::buffer_unordered, which yields results incompletion order, not request order. The fetched
Vec<Block>was thenused as if it were ordered by ascending height.
detect_reorgcompared the local DB rows to the canonical chain rows bypositional
zip, not by explicit height. With a freshly bootstrappedvia_l1_blockscontaining only one row at the wallet bootstrap block(for example
100891), the detector window started at100792, so thesingle DB row at
100891was compared against the canonical chainelement at index
0(height100792). The hashes did not match and thedetector logged
Reorg detected at block 100891.The main-node soft-reorg handler then reset
via_btc_watch.last_processedbelow the wallet bootstrap block. On the next restart, the wallet-resource
bootstrap path could not find its rows, because
via_btc_watchno longerconsidered the wallet bootstrap block processed. This breaks the invariant
that a node booted with
VIA_BTC_WATCH_START_L1_BLOCK_NUMBERandVIA_BTC_WATCH_RESTART_INDEXING=falseshould not regress its BTC-watchcursor purely because local
via_l1_blocksstate is sparse.The same positional-zip and unordered-fetch pattern existed in the verifier
detector, so the invariant must be enforced on both sides.
What changed
Applied to both
core/node/via_main_node_reorg_detector/src/lib.rsandvia_verifier/node/via_reorg_detector/src/lib.rs:fetch_blocksnow returnsVec<(i64, Block)>, pairing each block with itsrequested height and re-sorting ascending after
buffer_unordered. Thisremoves the silent ordering dependency.
sync_l1_blockswas also relyingon positional zip with a height range; it now consumes the explicit
(height, block)pairs directly.detect_reorgbuilds aHashMap<i64, String>of canonical block hasheskeyed by height and delegates to a pure
scan_for_reorghelper that walksthe DB rows in ascending order and compares each row by its explicit
numberagainstcanonical_by_height[number]. Positionalzipis gone.ReorgScan::SparseAt(height)variant signals that the canonical fetchis missing a height that the local DB knows about (for example, a stale
RPC or a partially failed batch fetch). The detector logs the height and
returns without acting, so a sparse local window cannot cause a false
reorg event or a BtcWatch demotion.
sparse_db_does_not_falsely_compare_against_window_start, which pins theexact
100891-vs-100792regression.Performance / Complexity Impact
This change touches reorg detection and L1 sync (a Via-specific area called
out as requiring this section).
fetch_blocks(per detector iteration)O(W)RPC +O(W)collectO(W)RPC +O(W log W)final sortW = reorg_window = 100. Adds one in-memory sort over up to 100 elements. RPC fan-out (max_concurrent_fetches = 10) is unchanged.O(min(D, W))positional zipO(W)HashMap build +O(D)scanD = rows returned by list_l1_blocks(<= W). Same asymptotic cost.Vec<Block>+Vec<height>HashMap<i64, String>of sizeWsync_l1_blocksinsert loopO(N)zip over rangeO(N)direct iterationWis fixed at 100 byViaReorgDetectorConfig::reorg_window, so the extrasort and the HashMap are bounded and run at most once per poll interval. No
hot DAL path or BTC-client path changed.
Boundaries and non-goals
run from this branch.
names, or chain-state were changed.
init()still seeds exactly onebootstrap row when
via_l1_blocksis empty; the height-keyed comparisonmakes that safe rather than requiring a coherent full-window backfill.
kube-state,helm-charts, or any Hetzner / GCPinfrastructure repo. Rollout of the new behavior to
via-main-testnet,Hetzner external nodes, or verifier nodes remains a separate operator
step after review and approval.
How to review
Suggested review focus:
core/node/via_main_node_reorg_detector/src/lib.rsandvia_verifier/node/via_reorg_detector/src/lib.rs: confirm both detectorsnow route through
scan_for_reorgand that theSparseAtbranch returnswithout inserting reorg metadata or touching
via_btc_watch.fetch_blocks: confirm the height pairing + sort is correct and that thesignature change is propagated to
sync_l1_blocksandis_canonical_chainin both crates.
core/node/via_main_node_reorg_detector/src/tests.rsandvia_verifier/node/via_reorg_detector/src/tests.rs: thesparse_db_does_not_falsely_compare_against_window_starttest is thenamed regression for the Hetzner observation; the
missing_canonical_height_for_present_db_row_returns_sparsetest pins thesafe non-reorg path when the canonical fetch is stale or incomplete.
and verifier detectors received the same fix in the same PR; there are no
remaining positional-zip comparisons against canonical chain blocks in
these two files.
No upstream ZKsync reference is needed; the affected code is Via-specific.
Checks run
cargo build -p via_main_node_reorg_detector -p via_verifier_reorg_detector cargo test -p via_main_node_reorg_detector -p via_verifier_reorg_detector cargo fmt -p via_main_node_reorg_detector -p via_verifier_reorg_detector --check cargo clippy -p via_main_node_reorg_detector -p via_verifier_reorg_detector --testsAll 12 new unit tests (6 per crate) passed. Clippy emitted no new warnings in
the touched crates (pre-existing warnings in
core/lib/dalare unrelated).zkstack dev fmt/zkstack dev lintwere not run in this environment; thenarrower
cargo fmt --checkandcargo clippyabove were used in theirplace for the two affected crates.
Author checklist
Live infrastructure and deployment impact
deploy live services by itself.
main; rollout to a liveenvironment (Hetzner external nodes,
via-main-testnet, verifier nodes)remains a separate deployment approval and verification step. Operators
rolling this out on a node that has already been reset by the previous
buggy soft-reorg path may also need to restore the wallet bootstrap
cursor; that recovery is out of scope for this PR.