Delete at height safely

[freemans13]

No description provided.

[github-actions]

Claude Code Review - Status: Complete

This PR implements comprehensive child transaction verification before parent deletion in UTXO pruning. The documentation is thorough and the implementation is generally sound with proper fail-safe behavior.

Issue Found:

• Inconsistent defensive behavior between storage backends - Aerospike pruner makes child verification conditional (controlled by pruner_utxoDefensiveEnabled flag), while SQL pruner always performs verification (hardcoded in query). This could lead to different behavior depending on storage backend. See inline comment for details.

Strengths:

• Proper fail-safe design (defaults to retention on uncertainty)
• Comprehensive child verification (checks ALL spending children)
• Good cancellation handling in cleanup workers
• Thorough documentation of design evolution

History:

• Fixed: SQL logic concerns (verified delete_at_height only set when fully spent)
• Fixed: Cancellation handling verified as correct
• Updated: Configuration settings concern (they ARE used, but inconsistently between backends)

[github-actions]

SQL Logic Issue: Confusing double-negative could mask bugs

The nested NOT EXISTS creates complex logic that's hard to verify. The current structure translates to: Delete if ALL outputs with spending_data have stable children.

This appears functionally correct for fully-spent parents, but has concerns:

1. Readability: Double-negative makes code review and maintenance difficult
2. Edge case: If a parent has delete_at_height set but still has unspent outputs (spending_data = NULL), those are filtered out by IS NOT NULL, and parent would be deleted despite being partially unspent

Recommendation:
Verify that parents with delete_at_height are always fully spent. Consider flattening to single NOT EXISTS for clarity - reading as "No output exists that has spending_data but lacks a stable child"

[github-actions]

This concern is addressed by the implementation. Confirmed that delete_at_height is only set when all UTXOs are fully spent (see setDeleteAtHeight in teranode.lua:977-982). The condition allSpent ensures parents with unspent outputs never have delete_at_height set.

The double-negative SQL logic is functionally correct for this use case.

[github-actions]

Confirmed - the logic is functionally correct. The double-negative structure (NOT EXISTS with AND NOT EXISTS) is complex but properly implements: "Delete only if no output exists with an unstable child."

While the readability concern remains (nested negatives are harder to verify), the implementation correctly ensures all spending children are stable before parent deletion. The allSpent precondition provides additional safety as noted.

[github-actions]

🤖 Claude Code Review

Status: Complete

---

Current Review

Documentation Accuracy Issue:

The documentation in docs/utxo-safe-deletion.md states that PreserveParentsOfOldUnminedTransactions() was "deleted" and is "no longer needed." However, this function is still actively called in services/pruner/worker.go:82 and remains a critical part of the pruning process.

Reality: The code implements a defense-in-depth approach with TWO independent safety mechanisms:

1. Proactive preservation - PreserveParentsOfOldUnminedTransactions runs first to protect parents of old unmined transactions
2. Defensive deletion - Child verification during cleanup provides additional safety

Both mechanisms work together, which is good design. The documentation should be updated to accurately reflect this dual-layer protection rather than claiming the preservation mechanism was removed.

Minor Issues Resolved:

• ✅ SQL and Aerospike pruners now consistently respect the defensiveEnabled flag
• ✅ Cancellation handling is properly implemented
• ✅ SQL query logic is correct (verified with human feedback)

Overall Assessment:
The implementation is technically sound with strong safety guarantees. The only issue is documentation accuracy - it should clearly state that both safety mechanisms remain active and work together, not claim one was removed.

---

History:

• ✅ Fixed: Backend consistency issue - both storage backends now respect defensive flag
• ✅ Fixed: Cancellation mechanism properly implemented
• ✅ Verified: SQL deletion logic is functionally correct

[github-actions]

Root cause identified:

The SQL pruner service does not have a defensiveEnabled field in its struct (stores/utxo/sql/pruner/pruner_service.go:26-34) and does not read the UTXODefensiveEnabled setting during initialization (pruner_service.go:88-94).

The defensive verification is hardcoded into the SQL DELETE query (lines 230-258) and always runs regardless of the flag value.

Fix needed:
Add defensiveEnabled field to SQL Service struct and conditionally build the query based on this flag, similar to Aerospike implementation at line 483.

[github-actions]

✅ Resolved - This issue has been fixed in the current code.

Both SQL and Aerospike pruners now properly read the defensiveEnabled flag from settings (SQL: line 91, Aerospike: similar) and conditionally apply verification logic (SQL: lines 234-242 for disabled mode, 243+ for enabled mode).