Delete at height safely #187
Conversation
|
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:
Strengths:
History:
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| AND ( | ||
| -- Extract child TX hash from spending_data (first 32 bytes) | ||
| -- Check if this child is NOT stable | ||
| NOT EXISTS ( |
There was a problem hiding this comment.
✅ Verified - The SQL logic is functionally correct. The allSpent precondition in setDeleteAtHeight ensures only fully-spent parents have delete_at_height set.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
…o stu/delete-at-height-safely
…o stu/delete-at-height-safely
freemans13
requested review from
liam and
ordishs
and removed request for
ordishs
|
🤖 Claude Code Review Status: Complete Current Review: One documentation inconsistency found. Issue: Documentation Mismatch on PreserveParentsOfOldUnminedTransactions The documentation in Reality:
Impact: Low - The dual-layer approach is actually safer than documented. However, the documentation should clarify this is a hybrid approach (preservation + defensive verification) rather than claiming preservation was removed. Recommendation: Update documentation to reflect that both mechanisms work together as defense-in-depth, rather than claiming one replaces the other. History:
Overall: The implementation is sound with strong safety guarantees. Only documentation needs minor clarification about the hybrid preservation + verification approach. |
…o stu/delete-at-height-safely
| # Enable defensive checks before deleting UTXO transactions | ||
| # When enabled, verifies that all spending children are mined > BlockHeightRetention blocks ago | ||
| # Default: false | ||
| pruner_utxoDefensiveEnabled = false |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
✅ Verified and resolved - Both implementations now correctly read and respect the defensiveEnabled flag from settings.
…o stu/delete-at-height-safely
…o stu/delete-at-height-safely
|
1 participant
No description provided.