Skip to content

fix: avoid recursive teardown of deep AVL node chains - #26

Open
a-shannon wants to merge 2 commits into
ergoplatform:mainfrom
a-shannon:fix/iterative-node-drop-upstream
Open

fix: avoid recursive teardown of deep AVL node chains#26
a-shannon wants to merge 2 commits into
ergoplatform:mainfrom
a-shannon:fix/iterative-node-drop-upstream

Conversation

@a-shannon

@a-shannon a-shannon commented Aug 3, 2026

Copy link
Copy Markdown

Summary

  • tear down last-owner InternalNode chains with an explicit worklist instead of recursive Rc destruction
  • preserve shared subtrees and live RefCell borrows
  • skip worklist allocation when two distinct children remain shared after the parent releases them
  • cover construction errors, ownership orderings, shared and aliased children, allocation behavior, and verifier-acceptance compatibility

Dependency

#24 has merged. This branch is rebased onto its resulting main at 9231458 and now contains only the two iterative-teardown commits:

  • a306d57 - iterative teardown and regression matrix
  • 1ede0fc - shared-child fast path and allocation/alias regressions

The validation below was rerun on the rebased head.

Problem

#24 makes subtree labeling iterative, but releasing the resulting Rc tree can still recursively destroy a last-owner chain of InternalNode values. A sufficiently deep proof spine can therefore exhaust the process stack during cleanup, including after constructor rejection.

Imposing a proof-depth cap is not acceptance-compatible: hash-matching explicit spines at depths 255, 256, and 300 are accepted by both this verifier and the JVM reference when operation limits are absent.

Implementation

Drop for InternalNode detaches internal child edges into a worklist and descends only when Rc::try_unwrap proves unique ownership. Shared allocations are released normally and remain controlled by their other owners.

A fast path returns before allocating the worklist when both children are distinct and each has another strong owner. The Rc::ptr_eq condition is required: if both fields alias one allocation with a strong count of two, releasing both edges can still reach zero and must use iterative teardown.

Adding Drop to the public InternalNode type is source-breaking for downstream Rust code that moves its fields out by value. The known sigma-rust and node call sites do not do so, but unknown external consumers remain possible; this compatibility tradeoff is intentionally exposed for review in a separate PR.

No parser predicate, proof-depth bound, verifier acceptance rule, serialization, operation limit, or public function signature is changed.

Validation

  • Base behavior: the deep invalid-digest case aborts with stack overflow during cleanup.
  • Candidate: all 10 isolated depth-50,000 teardown scenarios pass on a 512 KiB worker stack.
  • Aliasing mutant: removing only Rc::ptr_eq reproduces STATUS_STACK_OVERFLOW; restoring it passes.
  • Allocation regression: dropping the tree.copy ownership shape goes from two allocations to zero with the fast path.
  • Compatibility: hash-matching Lookup cases at depths 255, 256, and 300 remain accepted.
  • cargo check --lib and cargo test --all-targets pass.
  • Targeted formatting passes; Clippy exits successfully with only inherited warnings outside the changed lines.
  • Independent review found no correctness or blocking quality issue.

The release benchmark evidence and review history are recorded in mwaddip#2.

Scope boundary

This closes recursive ownership teardown. It does not claim that every recursive lookup, modification, or deletion operation is stack-safe.

@a-shannon
a-shannon force-pushed the fix/iterative-node-drop-upstream branch from a48703f to 1ede0fc Compare August 19, 2026 19:48
@a-shannon
a-shannon marked this pull request as ready for review August 20, 2026 21:03
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