Skip to content

arkrpc: validate ancestry tree_depth at indexer trust boundary (#370) - #454

Closed
ellemouton wants to merge 1 commit into
mainfrom
fix/370-zero-tree-depth-strands-oor
Closed

arkrpc: validate ancestry tree_depth at indexer trust boundary (#370)#454
ellemouton wants to merge 1 commit into
mainfrom
fix/370-zero-tree-depth-strands-oor

Conversation

@ellemouton

Copy link
Copy Markdown
Member

Closes #370.

Summary

ancestryFromRPC (in both darepod/incoming_metadata.go and oor/incoming_metadata_query.go) copied the indexer-supplied AncestryPath.tree_depth into vtxo.Ancestry without validation. A malicious indexer could return:

  • tree_depth = 0 → descriptor persists; later unroll trips the frag.TreeDepth == 0 guard → stranded VTXO.
  • an under-reported non-zero depth → vtxo/expiry.go uses MaxTreeDepth() * TreeDepthMultiplier to size the safe-exit buffer → late refresh, missed CSV.

Fix

New arkrpc.ValidateAncestryPathDepth(claimed, reconstructed) at the trust boundary:

  • rejects claimed == 0
  • rejects claimed > MaxAncestryTreeWalkDepth (32 — reused existing documented constant)
  • rejects claimed != reconstructed.Depth() (mismatch with the actual TreePath)

Both ancestryFromRPC copies call it before materializing vtxo.Ancestry. oor.validateIncomingAncestry also calls it as defense-in-depth so in-process descriptor construction is gated too.

Test plan

  • new TestValidateAncestryPathDepth — table-driven: 0, 1 (leaf), at-cap, over-cap, mismatch, nil-tree
  • new TestBuildIncomingVTXODescriptorRejectsInvalidAncestry subcases
  • 4 new regression tests + 2 subcases fail when the production fix is reverted
  • 3 pre-existing test fixtures (testIncomingVTXO, testIncomingMetadataVTXO, three cases in incoming_metadata_query_test.go) updated — they previously built bogus zero-depth paths; now use a shared testValidAncestryPath helper
  • make lint-native — 0 issues
  • go test ./arkrpc/... ./oor/... ./darepod/... ./unroll/... ./vtxo/... — pass

Relationship to #372

Complementary, not conflicting. #370 (this PR) closes the indexer→client ingress: zero/under-reported TreeDepth never reaches persistence. #372 keeps the unroll boundary liberal about TreeDepth (the scalar is expiry-timing metadata, not proof material — the proof assembler walks TreePath.Root directly), so legacy/pre-fix poisoned data can still recover. The two fixes are mutually consistent.

@gemini-code-assist gemini-code-assist 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.

Code Review

This pull request introduces validation for the tree_depth field in ancestry paths to ensure consistency between indexer-supplied metadata and reconstructed tree paths. This change acts as a trust boundary to prevent malicious indexers from providing zero or mismatched depths that could result in fund-availability issues. A security concern was identified in the ValidateAncestryPathDepth function, where calling reconstructed.Depth() on untrusted data could potentially cause a stack overflow; it is recommended to use a bounded depth-checking helper instead.

Comment thread arkrpc/ancestry_path_convert.go Outdated
Comment on lines +104 to +108
actual := reconstructed.Depth()
if uint32(actual) != claimed {
return fmt.Errorf("ancestry tree_depth %d does not match "+
"reconstructed path depth %d", claimed, actual)
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

security-high high

The use of reconstructed.Depth() here may be unsafe if it performs an unbounded recursive walk on the tree structure. Since the tree is reconstructed from untrusted indexer data, an arbitrarily deep tree could cause a stack overflow.

You should use the treeMaxDepth helper defined later in this file (line 141), which explicitly bounds the walk depth to MaxAncestryTreeWalkDepth to prevent this risk. This is consistent with how depth is calculated in AncestryPathFromTree (line 41).

	actual, err := treeMaxDepth(reconstructed)
	if err != nil {
		return err
	}

	if uint32(actual) != claimed {
		return fmt.Errorf("ancestry tree_depth %d does not match "+
			"reconstructed path depth %d", claimed, actual)
	}

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Good catch, you are correct. tree.Tree.Depth() walks the recursive Node.Depth() without any bound, and TreePathToTree only enforces parent_idx < child_idx < len(nodes) per child entry; the overall chain length is uncapped at the proto layer, so an indexer can supply a linear chain of N nodes that crashes the goroutine on reconstructed.Depth() even when the claimed scalar is in range.

Fixed in 2f18d9d: ValidateAncestryPathDepth now uses the locally-bounded treeMaxDepth (capped at MaxAncestryTreeWalkDepth = 32) instead of reconstructed.Depth(). The bounded walker returns an error when traversal exceeds the cap, so a hostile path fails closed before the comparison. Added TestValidateAncestryPathDepthBoundsReconstructedWalk to lock the behaviour in. Lint clean (0 issues), ./arkrpc/... and ./oor/... pass.

The receive path copied AncestryPath.tree_depth directly from the
indexer into the persisted descriptor without validation. An untrusted
indexer could therefore return a matching VTXO with tree_depth = 0
(or a non-zero value that disagrees with the supplied tree_path) and
either silently strand the OOR VTXO at unroll time (the proof
assembler rejects zero-depth fragments as proof-unavailable) or
under-report MaxTreeDepth and delay refresh/unilateral-exit past the
safe CSV deadline. Both outcomes are fund-availability bugs.

Introduce arkrpc.ValidateAncestryPathDepth as the shared validator
at the indexer→client boundary. It rejects zero claims, claims above
MaxAncestryTreeWalkDepth (the same cap the receive-side tree walk
enforces, so any tree that survives decode also survives this gate),
and claims that disagree with the reconstructed tree path's actual
depth. Both ancestryFromRPC helpers (darepod + oor) call it before
materializing vtxo.Ancestry, and validateIncomingAncestry calls it
again as defense-in-depth so in-process descriptor construction is
also gated.

Closes #370.
@ellemouton
ellemouton force-pushed the fix/370-zero-tree-depth-strands-oor branch from 2bc0f3a to 2f18d9d Compare May 15, 2026 12:02
@ellemouton
ellemouton marked this pull request as ready for review May 15, 2026 12:31
@ellemouton

Copy link
Copy Markdown
Member Author

Superseded by consolidated PR #459. Closing to reduce CI load.

@ellemouton ellemouton closed this May 15, 2026
ellemouton added a commit that referenced this pull request Jun 30, 2026
…idated

This is the manager half of the reverse-dependency restore (epic #454,
F6): when a batch that provisionally forfeits a VTXO is later invalidated
by a finalized conflict, the VTXO's forfeit is reversed, so the VTXO must
be restored to a spendable state.

Add ForfeitedVTXOs to RegisterBatchRequest: the producer declares the
VTXOs a batch consumes via forfeit, and the manager records a
reverse-dependency edge (AddProvisionalConsumer) for each. In
deriveAndPersist, hook the consumer batch's canonicality transition:

  - StateConflictFinalized (permanently off the canonical chain): restore
    every forfeited VTXO via the new RestoreConsumedVTXO callback, then
    drop the edges so the restore fires at most once.
  - StateFinalized (canonical and final): the forfeit is now safe, so the
    restore window closes -- drop the edges without restoring.

Transient states (provisional / reorged-out / conflict-provisional) are
left untouched: a reorged-out batch may still reconfirm, so its forfeit
must not be reversed until the invalidation is final. The restore window
therefore matches the reorg-safety depth: the forfeit stays restorable
exactly as long as the consumer batch can still leave the canonical
chain.

RestoreConsumedVTXO is an optional callback (nil = data-model only,
leaving the persisted edges in place); the VTXO-FSM restore path and the
round producer that declares ForfeitedVTXOs are follow-up slices.
ellemouton added a commit that referenced this pull request Jun 30, 2026
This is the manager half of the reverse-dependency restore (epic
#454, F6): when a batch that provisionally forfeits a VTXO is later
invalidated by a finalized conflict, the VTXO's forfeit is reversed,
so the VTXO must be restored to a spendable state.

Add ForfeitedVTXOs to RegisterBatchRequest: the producer declares the
VTXOs a batch consumes via forfeit, and the manager records a
reverse-dependency edge (AddProvisionalConsumer) for each. In
deriveAndPersist, hook the consumer batch's canonicality transition:

  - StateConflictFinalized (permanently off the canonical chain):
    restore every forfeited VTXO via the new RestoreConsumedVTXO
    callback, then drop the edges so the restore fires at most once.
  - StateFinalized (canonical and final): the forfeit is now safe, so
    the restore window closes -- drop the edges without restoring.

Transient states (provisional / reorged-out / conflict-provisional)
are left untouched: a reorged-out batch may still reconfirm, so its
forfeit must not be reversed until the invalidation is final. The
restore window therefore matches the reorg-safety depth: the forfeit
stays restorable exactly as long as the consumer batch can still
leave the canonical chain.

RestoreConsumedVTXO is an optional callback (nil = data-model only,
leaving the persisted edges in place); the VTXO-FSM restore path and
the round producer that declares ForfeitedVTXOs are follow-up slices.
ellemouton added a commit that referenced this pull request Jun 30, 2026
Add RestoreForfeitedVTXORequest/Response to lib/actormsg (re-exported by
vtxo) and a manager handler that rolls a forfeited VTXO back to a
spendable state. This is the VTXO-manager half of the reverse-dependency
restore (epic #454, F6): when the batch that consumed a VTXO via forfeit
is invalidated, the forfeit is reversed and the VTXO must return.

The forfeit transition reaps the VTXO actor and persists
VTXOStatusForfeited, so the handler re-materializes a fresh LiveState
actor from the persisted descriptor and flips the status to Live --
mirroring recoverExitedVTXO's re-materialization for unilateral-exit
recovery. It is idempotent: a VTXO that is not currently forfeited (or
that already has a live actor) is left untouched. No FSM-lifecycle or
terminal-state change is needed, so the common forfeit path is
unaffected.
ellemouton added a commit that referenced this pull request Jun 30, 2026
Carry the round's forfeited VTXOs on VTXOCreatedNotification and forward
them as RegisterBatchRequest.ForfeitedVTXOs when registering the
round-born batch. The canonicality manager records a reverse-dependency
edge for each, so if this round's commitment is later invalidated (its
forfeit reversed by a reorg/conflict), the forfeited VTXOs are restored
to a spendable state (epic #454, F6).
ellemouton added a commit that referenced this pull request Jul 1, 2026
This is the manager half of the reverse-dependency restore (epic
#454, F6): when a batch that provisionally forfeits a VTXO is later
invalidated by a finalized conflict, the VTXO's forfeit is reversed,
so the VTXO must be restored to a spendable state.

Add ForfeitedVTXOs to RegisterBatchRequest: the producer declares the
VTXOs a batch consumes via forfeit, and the manager records a
reverse-dependency edge (AddProvisionalConsumer) for each. In
deriveAndPersist, hook the consumer batch's canonicality transition:

  - StateConflictFinalized (permanently off the canonical chain):
    restore every forfeited VTXO via the new RestoreConsumedVTXO
    callback, then drop the edges so the restore fires at most once.
  - StateFinalized (canonical and final): the forfeit is now safe, so
    the restore window closes -- drop the edges without restoring.

Transient states (provisional / reorged-out / conflict-provisional)
are left untouched: a reorged-out batch may still reconfirm, so its
forfeit must not be reversed until the invalidation is final. The
restore window therefore matches the reorg-safety depth: the forfeit
stays restorable exactly as long as the consumer batch can still
leave the canonical chain.

RestoreConsumedVTXO is an optional callback (nil = data-model only,
leaving the persisted edges in place); the VTXO-FSM restore path and
the round producer that declares ForfeitedVTXOs are follow-up slices.
ellemouton added a commit that referenced this pull request Jul 1, 2026
Add RestoreForfeitedVTXORequest/Response to lib/actormsg (re-exported by
vtxo) and a manager handler that rolls a forfeited VTXO back to a
spendable state. This is the VTXO-manager half of the reverse-dependency
restore (epic #454, F6): when the batch that consumed a VTXO via forfeit
is invalidated, the forfeit is reversed and the VTXO must return.

The forfeit transition reaps the VTXO actor and persists
VTXOStatusForfeited, so the handler re-materializes a fresh LiveState
actor from the persisted descriptor and flips the status to Live --
mirroring recoverExitedVTXO's re-materialization for unilateral-exit
recovery. It is idempotent: a VTXO that is not currently forfeited (or
that already has a live actor) is left untouched. No FSM-lifecycle or
terminal-state change is needed, so the common forfeit path is
unaffected.
ellemouton added a commit that referenced this pull request Jul 1, 2026
Carry the round's forfeited VTXOs on VTXOCreatedNotification and forward
them as RegisterBatchRequest.ForfeitedVTXOs when registering the
round-born batch. The canonicality manager records a reverse-dependency
edge for each, so if this round's commitment is later invalidated (its
forfeit reversed by a reorg/conflict), the forfeited VTXOs are restored
to a spendable state (epic #454, F6).
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.

[security][high] Indexer-supplied zero tree depth can strand OOR VTXOs

1 participant