Skip to content

feat(state): verify supplied commitment roots against header commitments#411

Merged
ValarDragon merged 14 commits into
ironwood-mainfrom
split/vct-verify-core
Jul 2, 2026
Merged

feat(state): verify supplied commitment roots against header commitments#411
ValarDragon merged 14 commits into
ironwood-mainfrom
split/vct-verify-core

Conversation

@p0mvn

@p0mvn p0mvn commented Jul 2, 2026

Copy link
Copy Markdown

Motivation

Next slice of the verified-commitment-trees (VCT) split series (#402 merged, #410 open, #408 draft/reference). This is the verification core — the single invariant the whole design rests on: no peer-supplied root influences consensus state until it has been authenticated against a header commitment. Landing it alone, with its own tests, lets that consensus-critical logic be reviewed without the ~5k lines of plumbing around it.

Stacked on #410 (split/vct-root-serving); the diff collapses to this slice once #410 merges.

Solution

Add commitment_aux_verify.rs, read-only verification of supplied per-block Sapling/Orchard roots against the node's own checkpoint-committed block headers:

  • verify_commitment_roots: for each block, validate its header commitment against the running ZIP-221 ChainHistory MMR (i.e. against every root already folded in — the one-block-lag check), then fold the block's supplied roots into the MMR via HistoryTree::push. Returns the candidate tree only if every step verifies; any failure reports the failing height and leaves no state touched.
  • verify_supplied_sapling_root_below_heartwood / verify_supplied_orchard_root_below_nu5: direct checks where the MMR does not constrain a pool's root — below Heartwood the header's hashFinalSaplingRoot pins the Sapling root; below NU5 the Orchard root must be the empty-tree root (new CommitmentError::InvalidPreNu5OrchardRoot in zebra-chain).
  • CommitmentRootVerification items carry an optional precomputed ZIP-244 auth-data root (byte-identical to recomputing; see the new comment in check.rs) and a header_only form for confirming a predecessor's roots from the successor header.

There is no new crypto: the module reuses the existing consensus check block_commitment_is_valid_for_chain_history and HistoryTree::push, which build the V1/V2 leaf from the block body and the supplied roots.

@v12-auditor

v12-auditor Bot commented Jul 2, 2026

Copy link
Copy Markdown

Note

Complete: Audit complete. V12 found one issue worth reviewing.

Open the full results here.

FindingSeverityDetails
F-95121 🟠 High
Mutable cached roots bypass commitment binding

The checkpoint-finalization path validates NU5+ block commitments against a cached AuthDataRoot that public zebra-state callers can mutate after the block wrapper is constructed. SemanticallyVerifiedBlock.auth_data_root is public, CheckpointVerifiedBlock exposes mutable access to that inner struct through DerefMut, and the crate publicly re-exports both the wrapper and the Request type. A caller can submit Request::CommitCheckpointVerifiedBlock with auth_data_root = Some(forged_root) while the wrapped block's actual block.auth_data_root() is different. FinalizedState::commit_finalized_direct copies that supplied value and block_commitment_is_valid_for_chain_history uses it directly, only recomputing from the block when the option is None. If the block header commitment is computed from the forged root and parent history root, the commitment check succeeds and the block is written as finalized without proving that the header binds the block's actual authorizing data.

Analyzed four files, diff e326c5d...711c2ad.

@p0mvn
p0mvn marked this pull request as draft July 2, 2026 07:22
@p0mvn
p0mvn marked this pull request as ready for review July 2, 2026 07:32
Comment on lines +185 to +187
// Validate this block's header commitment against the current (parent) tree,
// i.e. against every root already folded in.
if !skip_parent_check {

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 to reviewer why this is needed

@p0mvn p0mvn Jul 2, 2026

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.

The skip is only a dedup for the VCT one-block look-ahead, not a trust shortcut.

When committing fast block H, we first check H’s header against T(H-1), then push H’s supplied roots into a candidate T(H). If H+1 is buffered, we immediately check H+1’s header against T(H), which authenticates H’s supplied roots before they are persisted.

When H+1 later becomes the block being committed, its parent-tree check is the exact same H+1 header against T(H) check that already succeeded as H’s look-ahead. skip_parent_check only fires when the cached prevalidation matches the exact (height, hash), so it just avoids repeating that same computation. The current block’s supplied roots are still pushed and authenticated by the successor check.

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.

@p0mvn
p0mvn force-pushed the split/vct-verify-core branch from 711c2ad to c293612 Compare July 2, 2026 18:32
Base automatically changed from split/vct-root-serving to ironwood-main July 2, 2026 19:05
Comment thread zebra-state/src/service/check.rs Outdated
@p0mvn

p0mvn commented Jul 2, 2026

Copy link
Copy Markdown
Author

@v12-auditor review

@v12-auditor

v12-auditor Bot commented Jul 2, 2026

Copy link
Copy Markdown

Note

Complete: Audit complete. No review-worthy issues remain after automatic triage. Three findings were auto-invalidated.

Open the full results here.

Analyzed four files, diff c046a43...cc199db.

@p0mvn

p0mvn commented Jul 2, 2026

Copy link
Copy Markdown
Author

@cursor review

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

Reviewed by Cursor Bugbot for commit cc199db. Configure here.

/// Verify this block's parent-history commitment, then fold the supplied
/// per-block roots into the running history tree for the next block.
pub(crate) fn with_roots(
block: Arc<Block>,

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Later on this will no longer require block correct?

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.

Yes, that's the end plan

@ValarDragon
ValarDragon merged commit 2a3a028 into ironwood-main Jul 2, 2026
38 of 39 checks passed
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.

2 participants