fix(reorg-detector): tolerate sparse via_l1_blocks during bootstrap#362
fix(reorg-detector): tolerate sparse via_l1_blocks during bootstrap#362romanornr wants to merge 1 commit into
Conversation
A freshly bootstrapped external node (and verifier) can have only the wallet bootstrap row in via_l1_blocks while the detector still compares a fixed reorg-window-sized canonical range. The previous code seeded one row in init() and then zipped DB rows against chain rows positionally in detect_reorg(), so a single row at the wallet bootstrap height was compared against the lowest canonical height in the window, producing a false reorg and triggering soft-reorg handling that rewound via_btc_watch below the wallet bootstrap block. Both ViaMainNodeReorgDetector and ViaVerifierReorgDetector now: - seed a full reorg-window-sized prefix ending at the wallet bootstrap block in init() inside a single transaction, so subsequent detector iterations always run against dense local state. - compare canonical chain hashes against DB rows by explicit block height via a new per-crate helper find_reorg_start_height, so a sparse DB window can never falsely flag a reorg by positional alignment. A shared comparison invariant is now covered by unit tests in each crate, including a regression test that reproduces the Hetzner 100792..100891 sparse-bootstrap window. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
![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 by implementing a height-aware comparison method in a new compare.rs module, which prevents false reorg reports on sparse databases. Additionally, the bootstrap process is updated to seed a complete reorg window. Reviewers identified a critical bug where the fetch_blocks function returns blocks out of order due to unordered buffering. This causes height misalignment during both the database seeding process and the reorg detection phase, potentially leading to data corruption or incorrect reorg triggers.
| let blocks = self.fetch_blocks(from_height, bootstrap_height).await?; | ||
|
|
||
| let mut transaction = storage.start_transaction().await?; | ||
| for (height, block) in (from_height..=bootstrap_height).zip(blocks) { |
There was a problem hiding this comment.
The zip operation here assumes that the blocks vector is ordered by height. However, the fetch_blocks method (defined at line 83) uses buffer_unordered (line 94) when fetching blocks from the Bitcoin client. This means the blocks are returned in the order they complete, which is not guaranteed to match the input height order. This will lead to incorrect block hashes being inserted into the database for the given heights during bootstrap.
| let chain_hashes: Vec<String> = chain_blocks | ||
| .iter() | ||
| .map(|b| b.block_hash().to_string()) | ||
| .collect(); |
There was a problem hiding this comment.
Similar to the issue in init(), chain_blocks is not guaranteed to be ordered by height because fetch_blocks uses buffer_unordered. Consequently, chain_hashes will be out of order, and the subsequent call to find_reorg_start_height will compare hashes against the wrong heights, leading to false reorg detections or missed real reorgs. You should update fetch_blocks to use buffer_ordered to ensure positional alignment.
| let blocks = self.fetch_blocks(from_height, bootstrap_height).await?; | ||
|
|
||
| let mut transaction = storage.start_transaction().await?; | ||
| for (height, block) in (from_height..=bootstrap_height).zip(blocks) { |
There was a problem hiding this comment.
The zip operation assumes blocks is ordered by height. However, fetch_blocks (defined at line 129) uses buffer_unordered (line 140), meaning the results are likely out of order. This will cause the database to be seeded with incorrect block hashes for the associated heights. Please update fetch_blocks to use buffer_ordered.
| let chain_hashes: Vec<String> = chain_blocks | ||
| .iter() | ||
| .map(|b| b.block_hash().to_string()) | ||
| .collect(); |
There was a problem hiding this comment.
Because fetch_blocks returns blocks in an arbitrary order (due to buffer_unordered), the chain_hashes vector will not be aligned with the expected heights starting from start_height. This breaks the logic in find_reorg_start_height, which assumes positional alignment. Changing fetch_blocks to use buffer_ordered is recommended to maintain correctness.
There was a problem hiding this comment.
Pull request overview
This PR fixes a correctness issue in both Via reorg detectors where a sparse/empty via_l1_blocks table during bootstrap could trigger a false-positive reorg due to positional (zip) comparison. The update makes bootstrap seeding produce a coherent local window and makes reorg detection compare by explicit block height (tolerating sparse DB windows).
Changes:
- Seed a full
reorg_window()prefix ending at thevia_btc_watchbootstrap height whenvia_l1_blocksis empty (in a single DB transaction). - Replace zip-by-position reorg comparison with a height-indexed helper (
find_reorg_start_height) that ignores missing DB heights. - Add unit tests for sparse/empty/dense/divergent cases in both crates.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
via_verifier/node/via_reorg_detector/src/lib.rs |
Seeds a full reorg window on init; updates reorg comparison to use height-based helper. |
via_verifier/node/via_reorg_detector/src/compare.rs |
Adds find_reorg_start_height helper + unit tests for sparse/dense/divergence scenarios. |
core/node/via_main_node_reorg_detector/src/lib.rs |
Mirrors verifier changes: init seeding and height-based reorg comparison. |
core/node/via_main_node_reorg_detector/src/compare.rs |
Adds the same helper + unit tests in the main-node detector crate. |
Comments suppressed due to low confidence (2)
via_verifier/node/via_reorg_detector/src/lib.rs:224
fetch_blocksusesbuffer_unordered, so the returnedchain_blocksVec is not guaranteed to be ordered by height.find_reorg_start_heightassumeschain_hashes[i]corresponds tostart_height + i; with out-of-order blocks this can yield incorrect (or missed) reorg detection. Consider switching to an ordered stream (buffered/buffer_ordered) or returning(height, block)pairs and sorting by height before buildingchain_hashes(also fixes init seeding and syncing which zip heights with blocks).
// Fetch chain blocks in batch
let chain_blocks = self.fetch_blocks(start_height, block_height).await?;
let chain_hashes: Vec<String> = chain_blocks
.iter()
.map(|b| b.block_hash().to_string())
.collect();
core/node/via_main_node_reorg_detector/src/lib.rs:217
fetch_blocksusesbuffer_unordered, so the returnedchain_blocksVec is not guaranteed to be ordered by height.find_reorg_start_heightassumeschain_hashes[i]corresponds tostart_height + i; with out-of-order blocks this can yield incorrect (or missed) reorg detection. Consider switching to an ordered stream (buffered/buffer_ordered) or returning(height, block)pairs and sorting by height before buildingchain_hashes(also fixes init seeding and syncing which zip heights with blocks).
// Fetch chain blocks in batch
let chain_blocks = self.fetch_blocks(start_height, block_height).await?;
let chain_hashes: Vec<String> = chain_blocks
.iter()
.map(|b| b.block_hash().to_string())
.collect();
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let bootstrap_height = storage | ||
| .via_indexer_dal() | ||
| .get_last_processed_l1_block("via_btc_watch") | ||
| .await? as i64; | ||
|
|
||
| let block = self.btc_client.fetch_block(block_height as u128).await?; | ||
|
|
||
| storage | ||
| .via_l1_block_dal() | ||
| .insert_l1_block(block_height, block.block_hash().to_string()) | ||
| .await?; | ||
| // Seed a coherent reorg-window-sized prefix ending at the wallet bootstrap | ||
| // block so subsequent `detect_reorg()` calls have dense local state and | ||
| // cannot misalign DB rows against canonical chain rows by zip position. | ||
| let window = self.config.reorg_window(); | ||
| let from_height = (bootstrap_height - window + 1).max(1); | ||
| let blocks = self.fetch_blocks(from_height, bootstrap_height).await?; | ||
|
|
| let bootstrap_height = storage | ||
| .via_indexer_dal() | ||
| .get_last_processed_l1_block("via_btc_watch") | ||
| .await? as i64; | ||
|
|
||
| let block = self.btc_client.fetch_block(block_height as u128).await?; | ||
|
|
||
| storage | ||
| .via_l1_block_dal() | ||
| .insert_l1_block(block_height, block.block_hash().to_string()) | ||
| .await?; | ||
| // Seed a coherent reorg-window-sized prefix ending at the wallet bootstrap | ||
| // block so subsequent `detect_reorg()` calls have dense local state and | ||
| // cannot misalign DB rows against canonical chain rows by zip position. | ||
| let window = self.config.reorg_window(); | ||
| let from_height = (bootstrap_height - window + 1).max(1); | ||
| let blocks = self.fetch_blocks(from_height, bootstrap_height).await?; | ||
|
|
![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: c4ae413beb
ℹ️ 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 blocks = self.fetch_blocks(from_height, bootstrap_height).await?; | ||
|
|
||
| let mut transaction = storage.start_transaction().await?; | ||
| for (height, block) in (from_height..=bootstrap_height).zip(blocks) { |
There was a problem hiding this comment.
Do not zip from_height..=bootstrap_height with blocks here unless fetch order is guaranteed. fetch_blocks() uses buffer_unordered, so the returned vector is completion-ordered rather than height-ordered; this can write valid hashes under the wrong heights during bootstrap, which then triggers false reorg detection and unnecessary rewinds on the next iteration. The same newly added pattern also appears in via_verifier/node/via_reorg_detector/src/lib.rs.
Useful? React with 👍 / 👎.
| let from_height = (bootstrap_height - window + 1).max(1); | ||
| let blocks = self.fetch_blocks(from_height, bootstrap_height).await?; |
There was a problem hiding this comment.
Guard this path when bootstrap_height == 0 (the default when via_indexer_metadata has no via_btc_watch row). With from_height = (bootstrap_height - window + 1).max(1), the first-run case becomes 1..=0, fetch_blocks() returns no blocks, and no row is inserted, so subsequent detect_reorg() calls keep failing with "No blocks found to detect reorg" instead of initializing state. The same regression exists in via_verifier/node/via_reorg_detector/src/lib.rs.
Useful? React with 👍 / 👎.
WalkthroughBoth the main node and verifier reorg detectors are refactored to use explicit height-based reorg detection. A new ChangesHeight-Explicit Reorg Detection
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
♻️ Duplicate comments (1)
via_verifier/node/via_reorg_detector/src/lib.rs (1)
106-117:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winSame critical
fetch_blocksordering bug as the main-node detector.
fetch_blockshere also usesbuffer_unordered, so the new init() bootstrap (line 111zip(blocks)) inserts mismatched (height, hash) rows, and detect_reorg'schain_hashesvector (lines 221-224) is no longer height-aligned forfind_reorg_start_height. Apply the samebuffer_unordered→bufferedfix to this crate'sfetch_blocksand verify both sibling detectors together.Also applies to: 129-146, 221-234
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@via_verifier/node/via_reorg_detector/src/lib.rs` around lines 106 - 117, The fetch_blocks implementation in this crate must stop using buffer_unordered (which breaks ordering) and switch to a buffered/ordered approach so returned blocks align with heights; update the fetch_blocks function to use a buffered stream (preserving input order) instead of buffer_unordered, then re-run the init/bootstrap code that zips (from_height..=bootstrap_height).zip(blocks) (the loop that inserts via_l1_block_dal.insert_l1_block) to ensure heights and block_hash() values match. Also update the sibling occurrences referenced in detect_reorg where chain_hashes is built (the detect_reorg function and the code that calls find_reorg_start_height) so chain_hashes remains height-aligned, and verify both detectors together after the change to ensure no ordering regressions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@via_verifier/node/via_reorg_detector/src/lib.rs`:
- Around line 106-117: The fetch_blocks implementation in this crate must stop
using buffer_unordered (which breaks ordering) and switch to a buffered/ordered
approach so returned blocks align with heights; update the fetch_blocks function
to use a buffered stream (preserving input order) instead of buffer_unordered,
then re-run the init/bootstrap code that zips
(from_height..=bootstrap_height).zip(blocks) (the loop that inserts
via_l1_block_dal.insert_l1_block) to ensure heights and block_hash() values
match. Also update the sibling occurrences referenced in detect_reorg where
chain_hashes is built (the detect_reorg function and the code that calls
find_reorg_start_height) so chain_hashes remains height-aligned, and verify both
detectors together after the change to ensure no ordering regressions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8538d12b-1703-4dc4-9b60-6d3f3955caf6
📒 Files selected for processing (4)
core/node/via_main_node_reorg_detector/src/compare.rscore/node/via_main_node_reorg_detector/src/lib.rsvia_verifier/node/via_reorg_detector/src/compare.rsvia_verifier/node/via_reorg_detector/src/lib.rs
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Agent
🧰 Additional context used
📓 Path-based instructions (9)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Preserve rich error context in production/library paths; prefer propagated errors andanyhow::Context/with_contextover flattening failures into generic strings
Prefer one-line expressions and short helper calls when they remain readable and rustfmt accepts them; avoid manual wrapping churn and do not reformat unrelated existing code just to change line shape
When changing async fetch/compare code, preserve or explicitly encode ordering assumptions; do not compare height-indexed data by vector position unless the ordering contract is proven and testedRun
cargo test -p <crate-or-package>before submitting a pull request to verify tests pass for affected crates
Files:
via_verifier/node/via_reorg_detector/src/compare.rscore/node/via_main_node_reorg_detector/src/compare.rsvia_verifier/node/via_reorg_detector/src/lib.rscore/node/via_main_node_reorg_detector/src/lib.rs
⚙️ CodeRabbit configuration file
**/*.rs: Focus on correctness, security, error handling, race conditions, async ordering,
DB semantics, height/hash/txid identity, and maintainability.
Do not request docstrings or unit tests unless tied to a concrete failure mode or invariant.
When changing a Via-specific path, consider the corresponding main-node or verifier sibling.
Files:
via_verifier/node/via_reorg_detector/src/compare.rscore/node/via_main_node_reorg_detector/src/compare.rsvia_verifier/node/via_reorg_detector/src/lib.rscore/node/via_main_node_reorg_detector/src/lib.rs
!(src/**/*.rs|tests/**/*.rs)
📄 CodeRabbit inference engine (AGENTS.md)
Avoid
unwrap()/expect()in non-test runtime paths
Files:
via_verifier/node/via_reorg_detector/src/compare.rscore/node/via_main_node_reorg_detector/src/compare.rsvia_verifier/node/via_reorg_detector/src/lib.rscore/node/via_main_node_reorg_detector/src/lib.rs
**/via_*/**/*.{rs,toml}
📄 CodeRabbit inference engine (.github/pull_request_template.md)
For Via-specific code in reorg detection/L1 sync, BTC sender/watch/client, DA/Celestia, and hot database paths (
via_*_dal), include a 'Performance / Complexity Impact' section describing performance characteristics, time complexity before vs. after, allocation behavior, and cache/memory access implications
Files:
via_verifier/node/via_reorg_detector/src/compare.rscore/node/via_main_node_reorg_detector/src/compare.rsvia_verifier/node/via_reorg_detector/src/lib.rscore/node/via_main_node_reorg_detector/src/lib.rs
**/{via_btc_watch,via_btc_client,*reorg_detector}/**/*.rs
📄 CodeRabbit inference engine (.github/ISSUE_TEMPLATE/l1_btc_reorg_sync_bug.yml)
For sparse DB rows, async fetches, batch RPC, zip, Vec index, heights, hashes, txids, batches, or timestamps: verify that the chain fetch preserves requested height order and document where.
Files:
via_verifier/node/via_reorg_detector/src/compare.rscore/node/via_main_node_reorg_detector/src/compare.rsvia_verifier/node/via_reorg_detector/src/lib.rscore/node/via_main_node_reorg_detector/src/lib.rs
**/{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)
**/{via_btc_watch,via_btc_client,via_btc_sender,*reorg_detector,*via_l1_block_dal}/**/*.rs: Do not use Vec/index position orzipto bind height/hash/tx identity without explicit verification that order is preserved.
Bitcoin block hash, txid, batch number, or DB row must remain bound to the exact height, transaction, batch, or row identity it was fetched or stored for.
Files:
via_verifier/node/via_reorg_detector/src/compare.rscore/node/via_main_node_reorg_detector/src/compare.rsvia_verifier/node/via_reorg_detector/src/lib.rscore/node/via_main_node_reorg_detector/src/lib.rs
via_verifier/node/via_reorg_detector/**
⚙️ CodeRabbit configuration file
via_verifier/node/via_reorg_detector/**: Verifier reorg detector. Treat as sibling of the main-node version; check both when either changes.
Preserve explicit height/hash identity and DB ordering assumptions.
Files:
via_verifier/node/via_reorg_detector/src/compare.rsvia_verifier/node/via_reorg_detector/src/lib.rs
**/via_*reorg_detector*/**
⚙️ CodeRabbit configuration file
**/via_*reorg_detector*/**: This is high-risk Via-specific code. Apply strong scrutiny across these review lenses:
- Identity and ordering assumptions (especially around heights, hashes, and async results)
- Main-node vs verifier sibling consistency
- Performance impact (time complexity, allocations, and cache behavior) when replacing algorithms or introducing new functions/data structures
- Clear distinction between DB state, canonical data, and live system behavior
Files:
via_verifier/node/via_reorg_detector/src/compare.rscore/node/via_main_node_reorg_detector/src/compare.rsvia_verifier/node/via_reorg_detector/src/lib.rscore/node/via_main_node_reorg_detector/src/lib.rs
**/via_*/**
⚙️ CodeRabbit configuration file
**/via_*/**: This is Via-specific code. When the change involves replacing an existing algorithm or introducing new functions or data structures, consider and describe the performance impact (time complexity, allocations, and cache behavior where relevant), in addition to correctness.
Files:
via_verifier/node/via_reorg_detector/src/compare.rscore/node/via_main_node_reorg_detector/src/compare.rsvia_verifier/node/via_reorg_detector/src/lib.rscore/node/via_main_node_reorg_detector/src/lib.rs
core/node/via_main_node_reorg_detector/**
⚙️ CodeRabbit configuration file
core/node/via_main_node_reorg_detector/**: Main-node reorg detector. Review together with via_verifier/node/via_reorg_detector when behavior overlaps.
Enforce explicit height/hash identity. Audit sparse rows, async fetch ordering, and Vec/zip usage.
Files:
core/node/via_main_node_reorg_detector/src/compare.rscore/node/via_main_node_reorg_detector/src/lib.rs
🔇 Additional comments (3)
core/node/via_main_node_reorg_detector/src/compare.rs (1)
1-90: LGTM!via_verifier/node/via_reorg_detector/src/compare.rs (1)
1-86: LGTM!core/node/via_main_node_reorg_detector/src/lib.rs (1)
83-100: ⚡ Quick winProvide the review comment to rewrite Paste the original
<review_comment>content (and the exact diff/context it references) so I can rewrite it in the required format.
Why
A freshly bootstrapped Via external node (and the verifier replica) can have system wallet rows and
via_btc_watchmetadata whilevia_l1_blocksis empty or sparse. The previous reorg-detector bootstrap path seeded one L1 block ininit()whenvia_l1_blockswas empty, and thendetect_reorg()compared a fixedreorg_window(100) of canonical chain blocks against the DB window usingdb_blocks.iter().zip(chain_blocks.iter()).With a single DB row at the wallet bootstrap height (e.g.
100891) and 100 canonical rows for100792..=100891, the zip paired DB row100891against canonical100792, producing:Soft-reorg handling then rewound
via_btc_watchto100890, hiding the wallet bootstrap row on the next restart and causing wallet-resource startup failures during the Hetzner private external-node rehearsal.The unsafe invariant in this repo was that
list_l1_blocks(start_height, window)does not assert dense coverage, but the comparison loop assumed dense, positionally-aligned rows. A freshly-bootstrapped detector therefore could not safely run a single iteration. The runtime correctness issue exists even though infra-side recovery automation was added separately.What changed
ViaMainNodeReorgDetector::init()andViaVerifierReorgDetector::init()now seed a coherent reorg-window-sized prefix ending at the wallet bootstrap block (via_btc_watchlast processed) inside a single DB transaction, so subsequent detector iterations always run against dense local state.detect_reorg()in both detectors no longer zips DB rows against canonical rows. A new per-crate helperfind_reorg_start_height(start_height, chain_hashes, db_blocks)compares by explicit block height; heights present on the canonical chain but absent in the DB window are ignored instead of being treated as divergences.core/node/via_main_node_reorg_detectorandvia_verifier/node/via_reorg_detector, which share the same invariant (and the same bug).100792..=100891regression case), empty DB, dense match, divergence reported by height (not position), and earliest-of-multiple divergences.Performance / Complexity Impact
detect_reorg()work is unchanged asymptotically. The new comparison builds aHashMap<i64, &str>over at mostreorg_window(100) DB rows once per iteration and then iterates the canonical hashes linearly.init()now performsreorg_windowconcurrent block fetches in the empty-DB case (still bounded bymax_concurrent_fetches, default 10) and one batched insert transaction.init()seed (empty DB)windowfetches + 1 txdetect_reorg()window comparisondetect_reorg()correctnessNo hot-path allocations beyond the window size were added. No DAL signatures, schema, metrics, or config keys changed.
Boundaries and non-goals
via_reorg_detector::reorg_window(); this PR does not retune it.via_main_node_reorg_detector,via_verifier_reorg_detector).list_l1_blocksincore/lib/dal/src/via_l1_block_dal.rsandvia_verifier/lib/verifier_dal/src/via_l1_block_dal.rsis left as-is; sparsity is now an expected input to the detector rather than something the DAL asserts away.Core, one forVerifier) instead of being shared because the surrounding code bases use different DAL types; a new shared workspace crate solely for a 15-line pure helper was deemed less reviewable than the per-crate duplicate. See AGENTS.mdReuse-firstnote on non-reuse decisions.list_l1_blockssemantics; no live cluster, kube-state, or helm-charts change is required to land this PR.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 --all-targetsAll ten new unit tests pass.
cargo fmt --checkis clean on the touched crates.cargo clippy --all-targetsis clean on the touched crates; pre-existing clippy diagnostics inzksync_typesandzksync_dalreproduce onmainand are out of scope here.Author checklist
Live infrastructure and deployment impact
main; rollout to a live environment remains a separate deployment approval and verification step, and the infra-side recovery automation referenced in the issue continues to apply to any pre-existing fresh/sparse external-node DBs.