fix(tx_pool): dedupe key-image conflict list; make remove_tx idempotent#199
fix(tx_pool): dedupe key-image conflict list; make remove_tx idempotent#199raw391 wants to merge 1 commit into
Conversation
have_tx_keyimges_as_spent walks tx.vin and appends every conflicting txid per spent key image. If one mempool tx shares 2+ key images with the incoming tx, its txid is appended multiple times. remove_flash_conflicts iterates that vector and calls remove_tx per entry. The first call succeeds; the second on the same txid fails at the sorted-container check. remove_flash_conflicts treats failure as fatal and returns early, so the LockedTXN destructor aborts the DB batch. The LMDB row is restored, but in-memory mutations from the first call (m_txpool_weight decrement, remove_transaction_keyimages, sorted-container erase) are not rolled back. m_spent_key_images then no longer contains entries for that txs inputs, so subsequent txs spending the same outputs are not rejected by have_tx_keyimges_as_spent until the orphaned row is aged out or the daemon restarts. Triggered by a single signer producing two txs sharing 2+ inputs. Dedupes the conflict list at source in have_tx_keyimges_as_spent via unordered_set. Makes remove_tx idempotent when called without stc_it: a missing sorted-container entry is treated as already-removed with an MWARNING log so the path stays observable.
raw391
force-pushed
the
fix/tx-pool-dedupe-flash-conflicts
branch
from
75a6616 to
1769d5a
Compare
📝 WalkthroughSummary by CodeRabbit
WalkthroughTwo defensive fixes in Changestx_pool defensive fixes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~5 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)❌ Error creating Unit Test PR.
Comment |
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/cryptonote_core/tx_pool.cpp`:
- Around line 759-763: The early return in remove_tx treats a missing
m_txs_by_fee_and_receive_time entry as proof the tx is already gone, which can
skip the remaining txpool cleanup. Update remove_tx so that when the
sorted-container lookup fails it still falls back to the DB/meta/blob cleanup
path, and only returns idempotently after confirming the tx is absent there too;
keep the logging in place but do not let the miss short-circuit the removal
flow.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 41f71ab8-424d-4d71-bdd2-91aec41ed18b
📒 Files selected for processing (1)
src/cryptonote_core/tx_pool.cpp
| if (!stc_it) | ||
| { | ||
| MWARNING("remove_tx: tx " << txid << " not in sorted container, treating as already-removed"); | ||
| return true; | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Don't treat a sorted-container miss as proof the tx is gone.
remove_stuck_transactions() (Lines 1016-1024 and 1039-1052) and validate() (Lines 1919-1938) already have paths where a tx can disappear from m_txs_by_fee_and_receive_time before its txpool DB row and key-image state are actually cleared. This early return true now masks that inconsistency and skips the remaining cleanup, so remove_flash_conflicts() can believe a conflicting tx was removed when it is still live in the txpool backend. Fall back to the DB/meta/blob cleanup path when the sorted entry is missing, and only make this idempotent when the tx is absent there as well.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/cryptonote_core/tx_pool.cpp` around lines 759 - 763, The early return in
remove_tx treats a missing m_txs_by_fee_and_receive_time entry as proof the tx
is already gone, which can skip the remaining txpool cleanup. Update remove_tx
so that when the sorted-container lookup fails it still falls back to the
DB/meta/blob cleanup path, and only returns idempotently after confirming the tx
is absent there too; keep the logging in place but do not let the miss
short-circuit the removal flow.
|
Note Unit test generation is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Generating unit tests... This may take up to 20 minutes. |
|
❌ Failed to create PR with unit tests: AGENT_CHAT: Failed to open pull request |
3 participants
tx_memory_pool::have_tx_keyimges_as_spentatsrc/cryptonote_core/tx_pool.cpp:1447-1465appends every conflicting txid into theconflictingvector per spent key image, with no dedupe. If one mempool tx shares two or more key images with the incoming tx, its txid is appended multiple times.The approved-flash path in
tx_pool::add_tx(tx_pool.cpp:361-365) callsremove_flash_conflicts(which starts attx_pool.cpp:674).remove_flash_conflictsiterates that vector and callsremove_txper entry. The first call succeeds; the second call on the same txid fails at the sorted-container check at line 760.remove_flash_conflictstreats the failure as a hard error and returns early, so theLockedTXNdestructor aborts the DB batch. The LMDB row is restored, but the in-memory mutations from the first call (m_txpool_weightdecrement at line 787,remove_transaction_keyimagesat line 788, sorted-container erase at line 789) are not rolled back.m_spent_key_imagesthen no longer contains entries for that txs inputs, so a subsequent tx spending the same outputs is not rejected byhave_tx_keyimges_as_spentuntil the orphaned row is aged out or the daemon restarts.This is reachable when a single signer produces two transactions sharing 2+ key images (a normal mempool tx and an approved flash tx for the same inputs).
Patch dedupes the conflict list at source via
unordered_set, and makesremove_txidempotent for ad-hoc callers that passstc_it = nullptr(with an MWARNING log so the path stays observable):bool tx_memory_pool::have_tx_keyimges_as_spent(const transaction& tx, std::vector<crypto::hash> *conflicting) const { auto locks = tools::unique_locks(m_transactions_lock, m_blockchain); bool ret = false; + std::unordered_set<crypto::hash> seen; for(const auto& in: tx.vin) { CHECKED_GET_SPECIFIC_VARIANT(in, txin_to_key, tokey_in, true); auto it = m_spent_key_images.find(tokey_in.k_image); if (it != m_spent_key_images.end()) { if (!conflicting) return true; ret = true; - conflicting->insert(conflicting->end(), it->second.begin(), it->second.end()); + for (const auto &h : it->second) + if (seen.insert(h).second) + conflicting->push_back(h); } } return ret; }const auto it = stc_it ? *stc_it : find_tx_in_sorted_container(txid); if (it == m_txs_by_fee_and_receive_time.end()) { + if (!stc_it) + { + MWARNING("remove_tx: tx " << txid << " not in sorted container, treating as already-removed"); + return true; + } MERROR("Failed to find tx in txpool sorted list"); return false; }The bool-only fast path of
have_tx_keyimges_as_spent(conflicting == nullptr) is unchanged. The pruner at line 794 passes a known-goodstc_itand keeps the loud-fail semantics.