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 2 commits 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.
75a6616 to
1769d5a
Compare
|
Warning Review limit reached
Next review available in: 59 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughTwo 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)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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
|
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 |
When remove_tx is called without a sorted-container iterator and the tx is not in the sorted container, it previously returned success unconditionally. A tx can be sorted-erased while still present in the backing store (the parse-failure window in remove_stuck_transactions leaves the DB row, key images, and counted weight). Check the backend: absent -> idempotent success; present and parseable -> complete the skipped cleanup; present but unparseable -> return failure so the orphan is surfaced rather than leaving orphaned key images.
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.