Skip to content

feat(state): stitch tree_aux root serving across the vct upgrade height#259

Draft
p0mvn wants to merge 1 commit into
perf-note-commit-treefrom
roman/vct-upgrade-height-root-serving
Draft

feat(state): stitch tree_aux root serving across the vct upgrade height#259
p0mvn wants to merge 1 commit into
perf-note-commit-treefrom
roman/vct-upgrade-height-root-serving

Conversation

@p0mvn

@p0mvn p0mvn commented Jun 25, 2026

Copy link
Copy Markdown

Motivation

ReadRequest::BlockRoots served per-block commitment roots with an either-or: if the commitment_roots_by_height index had anything at start_height it returned only the index's contiguous prefix, otherwise it derived from the per-height trees. It never stitched the two sources across a boundary.

Because the index is written on both commit paths, the only boundary that can exist is the upgrade height U — the first height the new binary commits: below U a DB written by the old binary has per-height trees but no index; at/above U the index is present. A peer request straddling U (e.g. [x, x+500] with U = x+100, default vct mode above so trees stop at U) returned only the ~100-root prefix, which the fetch client rejects under its minimum-progress threshold (¼ of count) — a permanent stall (state.vct.root.stalled.height). This addresses the TODO previously sitting in the BlockRoots handler.

Solution

  • New one-time vct_upgrade_height marker U in a vct_upgrade_metadata column family — the lowest height this binary commits (and the lowest height in the serving index). Written once on the first committed block (both commit paths) and never moved.
  • Serving stitch (serve_block_roots): per-height trees below U + serving index at/above U, concatenated into one gap-free run. A pre-index archive node (no U) still derives the whole range from the trees.
  • Band-aware tree availability: trees are absent only within [U, H) (H = checkpoint handoff), via a new vct_tree_absent helper. Trees are present below U (pre-upgrade) and at/above H (semantic sync). The z_gettreestate gate and the sapling/orchard_tree_by_height guards now use the band. For a genesis fast-sync (U = 0) this reduces exactly to the prior height < H behaviour.
    • When the upgrade is at/after the handoff (U >= H) the band is empty, so every height is servable — matching the design (semantic sync keeps writing trees above the checkpoint).

Tests

  • Unit tests for vct_tree_absent (the [U, H) band, plus the empty-band U >= H case) and an index-side serve_block_roots test.
  • End-to-end stitch assertion added to the DB-produced round-trip proptest: simulates a mid-chain upgrade by dropping the serving index below U, and asserts serve_block_roots still returns the full range gap-free, matching the all-trees reference.
  • U = 0 assertion in the genesis fast-sync proptest.
  • Green: new tests, zebra-state vct/commitment/tree suite (except one pre-existing failure, see below), zebra-network -- tree_aux, zebra-rpc -- treestate. cargo fmt/clippy clean for the changed code.

Specifications & References

verified-commitment-trees design (§4 serving index, §9 tree_aux serving).

Follow-up Work

  • Pre-existing (not from this PR): vct_frozen_frontier_survives_reopen fails on the base branch independently of these changes — the test's custom network has no embedded frontier, so reopening a frozen DB panics at FinalizedState::new. Confirmed it fails identically with this PR's marker write disabled, and for its genesis fast-sync (U = 0) vct_tree_absent is byte-identical to the prior guard.
  • Conservative over-rejection is intentional: toggling checkpoint_verify=false mid-run backfills trees only above the current tip, so [U, H) keeps reporting unavailable; full recovery is a fresh reindex from genesis.

AI Disclosure

  • AI tools were used: Claude Code for the analysis, implementation, and tests in this PR (reviewed by the author).

PR Checklist

  • The PR title follows conventional commits format.
  • The PR follows the contribution guidelines.
  • This change was discussed in an issue or with the team beforehand.
  • The solution is tested.
  • The documentation and changelogs are up to date.

Consensus-sensitive: touches the finalized commit and serve paths. Opened as a draft for human review.

@v12-auditor

v12-auditor Bot commented Jun 25, 2026

Copy link
Copy Markdown

Warning

Insufficient credits for auto-review. Keep at least $5.00 of available balance to start a run. Please add credits to continue.

Comment thread zebra-state/src/service.rs Outdated
Comment on lines -1578 to -1585
let indexed = state.db.commitment_roots_by_height_range(range.clone());
if !indexed.is_empty() || state.db.is_vct_synced() {
// Indexed roots (the common path), or a fast-synced node whose only
// possible source is the index — never the absent per-height trees.
indexed
} else {
// Pre-index archive database: derive from the per-height trees.
finalized_state::produce_block_roots(&state.db, range)

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: this was the main bug I suspect

Record a one-time `vct_upgrade_height` marker `U` (the lowest height this
binary commits, and the lowest height in the `commitment_roots_by_height`
serving index) in a new `vct_upgrade_metadata` column family. Written once on
the first committed block and never moved.

Root serving (`ReadRequest::BlockRoots`) now stitches the per-height trees
below `U` with the serving index at and above `U`, so a node that upgraded
mid-chain serves a range crossing `U` as one gap-free batch instead of the
short index-only prefix that stalled the fetch client's minimum-progress
check. A pre-index archive node (no `U`) still derives the whole range from
the trees.

Historical note-commitment tree availability is now the band `[U, H)` (H =
checkpoint handoff) via a new `vct_tree_absent` helper: trees are present
below `U` (pre-upgrade) and at/above `H` (semantic sync), absent only in
between. For a genesis fast-sync (`U = 0`) this reduces exactly to the prior
`height < H` behaviour.
@p0mvn p0mvn force-pushed the roman/vct-upgrade-height-root-serving branch from 987508b to 9626164 Compare June 25, 2026 18:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant