fix(utxo): mempool atomicity BUG-1 + BUG-4 (PR 6146 review)#1
Open
crowniteto wants to merge 5 commits into
Open
fix(utxo): mempool atomicity BUG-1 + BUG-4 (PR 6146 review)#1crowniteto wants to merge 5 commits into
crowniteto wants to merge 5 commits into
Conversation
…on (BUG-4) BUG-1: mempool_remove() now uses BEGIN IMMEDIATE to ensure both DELETE operations are atomic. Previously, a crash between the two deletes could leave orphan utxo_mempool_inputs rows, causing a persistent UTXO lock / mempool DoS. Impact clarified per @MolhamHamwi review: this is not a double-spend vector (box_id PK + mempool_check_double_spend still prevent that), but an availability issue. BUG-4: New _evict_stale_data_input_txs() method proactively removes mempool txs whose data_inputs were just spent by apply_transaction(). Previously, stale txs held their normal inputs reserved in utxo_mempool_inputs until candidate selection or expiry caught them. 5 regression tests passing. Addresses review feedback on PR Scottcjn#6146.
Code review fixes: - Move _spent_ids computation before COMMIT (consistent with original flow) - Wrap eviction in try/except inside the try block (after COMMIT) - If eviction fails, the already-committed transaction is unaffected - Swallow exceptions to prevent rollbacks on committed data Regression test improvements: - Fix all output values to respect DUST_THRESHOLD (1000 nRTC) - Fix conservation law: output + fee = input for all test transactions - Use real minting + mempool_add flow (not direct DB inserts) for integration-level BUG-4 regression tests - Add test_apply_transaction_evicts_stale_mempool_tx_via_input: verifies mempool tx is evicted when its input box is spent on-chain - Add test_apply_transaction_preserves_unrelated_mempool_txs: verifies that spending box_a does NOT evict mempool tx claiming box_b - Fix method signature typo (fulg,obj -> self) - Fix fetchone/fetchall typos - 7/7 tests passing, 46/46 UTXO tests passing
…tx_data_json scan
Reviewer (ayskobtw-lil) found that _evict_stale_data_input_txs only
searched utxo_mempool_inputs, but mempool_add() does NOT record
data_inputs there — only regular inputs. This means spending a box
used as a data_input by a mempool tx would NOT evict that tx.
Changes:
- _evict_stale_data_input_txs now uses a two-phase search:
1. Check utxo_mempool_inputs for txs claiming spent boxes as
regular inputs (fast path)
2. Scan utxo_mempool.tx_data_json for txs whose data_inputs
reference any spent box (covers the gap)
- Added try/except wrapper in apply_transaction() for best-effort
eviction after COMMIT — failure does not affect committed tx
- New regression tests:
- test_evict_finds_tx_via_data_input_in_tx_data_json:
unit test verifying tx_data_json scanning path works
- test_apply_transaction_evicts_mempool_tx_with_data_input:
integration test using real mempool_add() with data_inputs +
apply_transaction() spending the data_input box
- test_apply_transaction_evicts_stale_mempool_tx_via_input:
integration test for regular input eviction
- test_apply_transaction_preserves_unrelated_mempool_txs:
verifies spending box_a does not evict tx claiming box_b
All 48 UTXO tests passing (9 regression + 39 existing).
…eview v3) The stale data-input eviction helper opened its own SQLite connection, which fails silently when the outer caller holds BEGIN IMMEDIATE (the utxo_endpoints.py transfer pattern). The DELETE is blocked by SQLite's single-writer lock, the exception is swallowed, and stale mempool txs survive after the outer COMMIT. Fix: - _evict_stale_data_input_txs() now accepts optional conn= parameter - When manage_tx=False (external connection), eviction runs on the caller's connection inside their transaction (before their COMMIT) - When manage_tx=True (own transaction), eviction runs after COMMIT on a separate connection (same as before, failure is safe) - own_conn flag controls commit/rollback/close in the helper - 3 new regression tests using real external-connection shape Addresses: ayskobtw-lil review on PR Scottcjn#6306
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes UTXO mempool atomicity issues identified in PR 6146 review.
BUG-1: mempool_remove() must atomically remove from both utxo_mempool and utxo_mempool_inputs within a single BEGIN IMMEDIATE transaction. Without this, a crash between the two operations could leave orphan input-claim rows that block future spends of the same box.
BUG-4: apply_transaction() must proactively evict mempool transactions whose inputs (including data_inputs) reference boxes that were just spent on-chain. Without this, stale mempool txs hold their normal inputs reserved in utxo_mempool_inputs until candidate selection catches them.
Changes:
All 46 UTXO-related tests pass.