Skip to content

tx_pool: fix early return in remove_transaction_keyimages#10332

Closed
DUQUEredes wants to merge 1 commit intomonero-project:masterfrom
DUQUEredes:fix-remove-keyimages-early-return
Closed

tx_pool: fix early return in remove_transaction_keyimages#10332
DUQUEredes wants to merge 1 commit intomonero-project:masterfrom
DUQUEredes:fix-remove-keyimages-early-return

Conversation

@DUQUEredes
Copy link

@DUQUEredes DUQUEredes commented Feb 23, 2026

Summary

  • Fix the FIXME at tx_pool.cpp:504 by replacing CHECK_AND_ASSERT_MES / CHECKED_GET_SPECIFIC_VARIANT macros with explicit error checks that continue instead of return false
  • The function now processes all transaction inputs even when errors occur, preventing orphaned key images in m_spent_key_images
  • Track success via an all_found flag so the return value still indicates success/failure
  • Clean up empty key_image sets instead of just failing on them

Context

The original code had an explicit FIXME from the authors:

FIXME: Can return early before removal of all of the key images.
       At the least, need to make sure that a false return here
       is treated properly.  Should probably not return early, however.

When CHECK_AND_ASSERT_MES triggers on the Nth input, inputs N+1 through end retain their key images in m_spent_key_images even though the transaction itself has already been removed by callers (prune(), take_tx(), remove_stuck_transactions()). These orphaned key images cause false double-spend rejections and are never cleaned up (memory leak).

The error conditions occur during node restart (#6175) and Dandelion++ relay failures (#6687).

Test plan

  • Build passes (make -j$(nproc))
  • All 6 txpool core tests pass (core_tests --generate_and_play_test_data --filter 'txpool_.*')
  • No new tests needed — this is defensive hardening of error handling for conditions that occur during node restart and Dandelion++ failures, which are impractical to reproduce in a test harness

@DUQUEredes DUQUEredes force-pushed the fix-remove-keyimages-early-return branch from d19999d to 0ecd98d Compare February 23, 2026 10:49
Comment on lines +512 to +518
if(vi.type() != typeid(txin_to_key))
{
MERROR("unexpected input variant type in remove_transaction_keyimages, transaction id = " << actual_hash);
all_found = false;
continue;
}
const txin_to_key& txin = boost::get<txin_to_key>(vi);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if(vi.type() != typeid(txin_to_key))
{
MERROR("unexpected input variant type in remove_transaction_keyimages, transaction id = " << actual_hash);
all_found = false;
continue;
}
const txin_to_key& txin = boost::get<txin_to_key>(vi);
const txin_to_key *txin = boost::get<txin_to_key>(&vi);
if (nullptr == txin)
{
MERROR("unexpected input variant type in remove_transaction_keyimages, transaction id = " << actual_hash);
all_found = false;
continue;
}

Replace CHECK_AND_ASSERT_MES macros with explicit error checks that
continue processing remaining inputs instead of returning early.
The previous behavior could leave orphaned key images in
m_spent_key_images when an error was encountered partway through
the input list, since callers already deleted the transaction from
the pool before calling this function.

Track success via an all_found flag so the return value still
indicates whether all key images were found and removed. Also clean
up empty key_image sets instead of just failing on them.

Resolves the FIXME at the top of the function that noted: "Should
probably not return early, however."
@DUQUEredes DUQUEredes force-pushed the fix-remove-keyimages-early-return branch from 0ecd98d to 8f9ad12 Compare February 23, 2026 19:31
@jeffro256
Copy link
Contributor

Please attribute this PR as machine generated code.

@selsta
Copy link
Collaborator

selsta commented Feb 26, 2026

Closing due to the fact that this is AI and not disclosed, in fact it appears to have been edited out of the description.

@selsta selsta closed this Feb 26, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants