Bug: unidentified spend tx in pool on wallet restore

[j-berman]

If you send a Monero tx, it enters the pool, and then restore the wallet, then the wallet won't identify the spend tx in the pool. If a user then attempts to construct another tx, the wallet may attempt to re-spend an output already spent in the pool, causing the node to reject the tx as a double spend. Note: this bug requires the pool tx to remain in the pool (as soon as it enters the chain, the wallet will identify the tx as a spend correctly).

Expected behavior: the pool tx should be identified as a spend, and any spent outputs should no longer be usable. The wallet state on restore should match the wallet state of the wallet that originally sent the tx.

Identified by @nahuhh in testing the FCMP++/Carrot integration (the bug is unrelated to the integration, I reproduced the issue on current master).

[0xFFFC0000]

This should effectively solve this issue. Once confirmed, I will send a PR.

$ >> git diff 
diff --git a/src/wallet/wallet2.cpp b/src/wallet/wallet2.cpp
index 432b5d5c7..8c6f7e4e0 100644
--- a/src/wallet/wallet2.cpp
+++ b/src/wallet/wallet2.cpp
@@ -3714,6 +3714,19 @@ void wallet2::update_pool_state(std::vector<std::tuple<cryptonote::transaction,
   process_txs.clear();
   if (m_background_syncing)
     return;
+  // Ensure we can detect spends from mempool after restore by making key images known
+  if (!m_watch_only && !m_multisig && has_unknown_key_images())
+  {
+    MDEBUG("Unknown key images detected; rescanning spent status before processing pool");
+    try
+    {
+      rescan_spent(); // updates spent status for both pool and chain
+    }
+    catch (const std::exception &e)
+    {
+      MWARNING("rescan_spent() failed before pool update: " << e.what());
+    }
+  }
   bool updated = false;
   if (m_pool_info_query_time != 0 && try_incremental)
   {
@@ -3755,7 +3768,19 @@ void wallet2::update_pool_state_by_pool_query(std::vector<std::tuple<cryptonote:
 {
   MTRACE("update_pool_state_by_pool_query start");
   process_txs.clear();
-
+  // Ensure we can detect spends from mempool after restore by making key images known
+  if (!m_watch_only && !m_multisig && has_unknown_key_images())
+  {
+    MDEBUG("Unknown key images detected; rescanning spent status before processing pool (by_pool_query)");
+    try
+    {
+      rescan_spent(); // updates spent status for both pool and chain
+    }
+    catch (const std::exception &e)
+    {
+      MWARNING("rescan_spent() failed before pool update (by_pool_query): " << e.what());
+    }
+  }
   auto keys_reencryptor = epee::misc_utils::create_scope_leave_handler([&, this]() {
     m_encrypt_keys_after_refresh.reset();
   });

[j-berman]

rescan_spent will query the daemon with the wallet's key images (both spent and unspent), which we should be able to avoid here because the wallet is already fetching the whole pool and scanning it (i.e. the bug and fix should be in the pool scanning logic). Avoiding rescan_spent is worthwhile here (some users may be pointing to untrusted daemons and also we shouldn't need the extra trip to the daemon)

[0xFFFC0000]

rescan_spent will query the daemon with the wallet's key images (both spent and unspent), which we should be able to avoid here because the wallet is already fetching the whole pool and scanning it (i.e. the bug and fix should be in the pool scanning logic). Avoiding rescan_spent is worthwhile here (some users may be pointing to untrusted daemons and also we shouldn't need the extra trip to the daemon)

Yes. You are correct. I was going to update the patch:

diff --git a/src/wallet/wallet2.cpp b/src/wallet/wallet2.cpp
index 432b5d5c7..71356b5d7 100644
--- a/src/wallet/wallet2.cpp
+++ b/src/wallet/wallet2.cpp
@@ -3699,6 +3699,51 @@ void wallet2::process_unconfirmed_transfer(bool incremental, const crypto::hash
     }
   }
 }
+void wallet2::compute_unknown_key_images_locally()
+{
+  if (m_watch_only || m_multisig) return;
+
+  hw::device &hwdev = m_account.get_device();
+  // If AskPasswordToDecrypt, ensure keys are temporarily decrypted
+  std::shared_ptr<wallet_keys_unlocker> unlocker;
+  if (m_ask_password == AskPasswordToDecrypt && !m_unattended && !m_background_syncing) {
+    // IMPORTANT: create an actual unlocker here (adapt ctor to real type)
+    try {
+      unlocker = std::make_shared<wallet_keys_unlocker>(*this /* or correct args */);
+      m_encrypt_keys_after_refresh = unlocker; // hold so keys re-encrypt later
+    } catch (const std::exception &e) {
+      MWARNING("Failed to create wallet_keys_unlocker: " << e.what());
+      // Continue — may still work if keys not encrypted, but likely will fail below.
+    }
+  }
+
+  hwdev.computing_key_images(true);
+  auto hw_guard = epee::misc_utils::create_scope_leave_handler([&]{ hwdev.computing_key_images(false); });
+
+  const auto &keys = m_account.get_keys();
+
+  for (size_t i = 0; i < m_transfers.size(); ++i)
+  {
+    auto &td = m_transfers[i];
+    if (td.m_key_image_known || td.m_key_image_partial) continue;
+
+    const crypto::public_key out_pub = td.get_public_key();
+    const crypto::public_key tx_pub = get_tx_pub_key_from_received_outs(td);
+
+    crypto::key_derivation derivation{};
+    if (!hwdev.generate_key_derivation(tx_pub, keys.m_view_secret_key, derivation))
+      continue;
+
+    cryptonote::keypair in_ephemeral{};
+    crypto::key_image ki{};
+    if (!cryptonote::generate_key_image_helper_precomp(keys, out_pub, derivation, td.m_internal_output_index, td.m_subaddr_index, in_ephemeral, ki, hwdev))
+      continue;
+
+    td.m_key_image = ki;
+    td.m_key_image_known = true;
+    m_key_images[ki] = i;
+  }
+}
 //----------------------------------------------------------------------------------------------------
 // This public method is typically called to make sure that the wallet's pool state is up-to-date by
 // clients like simplewallet and the RPC daemon. Before incremental update this was the same method
@@ -3714,6 +3759,20 @@ void wallet2::update_pool_state(std::vector<std::tuple<cryptonote::transaction,
   process_txs.clear();
   if (m_background_syncing)
     return;
+  if (!m_watch_only && !m_multisig && has_unknown_key_images())
+  {
+    if (is_trusted_daemon())
+    {
+      MDEBUG("Unknown key images detected; rescanning spent status (trusted daemon)");
+      try { rescan_spent(); }
+      catch (const std::exception &e) { MWARNING("rescan_spent() failed: " << e.what()); }
+    }
+    else
+    {
+      MDEBUG("Unknown key images detected; computing key images locally (untrusted daemon)");
+      compute_unknown_key_images_locally();
+    }
+  }
   bool updated = false;
   if (m_pool_info_query_time != 0 && try_incremental)
   {
@@ -3755,7 +3814,20 @@ void wallet2::update_pool_state_by_pool_query(std::vector<std::tuple<cryptonote:
 {
   MTRACE("update_pool_state_by_pool_query start");
   process_txs.clear();
-
+  if (!m_watch_only && !m_multisig && has_unknown_key_images())
+  {
+    if (is_trusted_daemon())
+    {
+      MDEBUG("Unknown key images; rescanning spent status (trusted daemon, by_pool_query)");
+      try { rescan_spent(); }
+      catch (const std::exception &e) { MWARNING("rescan_spent() failed: " << e.what()); }
+    }
+    else
+    {
+      MDEBUG("Unknown key images; computing key images locally (untrusted daemon, by_pool_query)");
+      compute_unknown_key_images_locally();
+    }
+  }
   auto keys_reencryptor = epee::misc_utils::create_scope_leave_handler([&, this]() {
     m_encrypt_keys_after_refresh.reset();
   });
diff --git a/src/wallet/wallet2.h b/src/wallet/wallet2.h
index 9efeee0ea..e731cb977 100644
--- a/src/wallet/wallet2.h
+++ b/src/wallet/wallet2.h
@@ -1636,6 +1636,7 @@ private:
     bool import_key_images(signed_tx_set & signed_tx, size_t offset=0, bool only_selected_transfers=false);
     crypto::public_key get_tx_pub_key_from_received_outs(const tools::wallet2::transfer_details &td) const;
 
+    void compute_unknown_key_images_locally();
     void update_pool_state(std::vector<std::tuple<cryptonote::transaction, crypto::hash, bool>> &process_txs, bool refreshed = false, bool try_incremental = false);
     void process_pool_state(const std::vector<std::tuple<cryptonote::transaction, crypto::hash, bool>> &txs);
     void remove_obsolete_pool_txs(const std::vector<crypto::hash> &tx_hashes, bool remove_if_found);

[j-berman]

The wallet should already be computing key images during the chain scan on restore, and identifying the spend when reading the pool txs. So rescan_spent shouldn't be necessary (nor the additional call to compute_unknown_key_images_locally)

IIRC ... the scanner scans the pool first and then scans the chain, which would explain why it's missing spends in the pool (needs key images from receives when scanning the chain first before being able to identify the spend in the pool). The wallet also uses m_scanned_pool_txs to cache already scanned pool txs, so perhaps the reason it's missing these spends is because it already cached them as scanned before it had identified the received outputs (that those spend txs spent them in). There is also the whole incremental approach to pool scanning (give me all new pool txs since I last asked) where the daemon may not be returning the same pool txs it already returned.

If above is the case, which should be tested/confirmed before implementing a fix, maybe it makes sense for the wallet to take the extra steps necessary to re-scan the pool if it had just scanned >= 10 blocks (clear local cached variables preventing it, and then just re-executing refresh). I say 10 blocks because that's the minimum necessary for pool txs to be able to spend outputs in the chain (EDIT: double check off-by-1).

[0xFFFC0000]

The wallet should already be computing key images during the chain scan on restore, and identifying the spend when reading the pool txs. So rescan_spent shouldn't be necessary (nor the additional call to compute_unknown_key_images_locally)

IIRC ... the scanner scans the pool first and then scans the chain, which would explain why it's missing spends in the pool (needs key images from receives when scanning the chain first before being able to identify the spend in the pool). The wallet also uses m_scanned_pool_txs to cache already scanned pool txs, so perhaps the reason it's missing these spends is because it already cached them as scanned before it had identified the received outputs (that those spend txs spent them in). There is also the whole incremental approach to pool scanning (give me all new pool txs since I last asked) where the daemon may not be returning the same pool txs it already returned.

If above is the case, which should be tested/confirmed before implementing a fix, maybe it makes sense for the wallet to take the extra steps necessary to re-scan the pool if it had just scanned >= 10 blocks (clear local cached variables preventing it, and then just re-executing refresh). I say 10 blocks because that's the minimum necessary for pool txs to be able to spend outputs in the chain (EDIT: double check off-by-1).

This does not directly address your concern. But fixes the issue from another angle. This should work without any need for extra daemon communication. Basically comparing key images to txis in the tx_pool.

I am thinking about what you proposed. Will try to evaluate it.

diff --git a/src/wallet/wallet2.cpp b/src/wallet/wallet2.cpp
index 432b5d5c7..9c1caf6cf 100644
--- a/src/wallet/wallet2.cpp
+++ b/src/wallet/wallet2.cpp
@@ -3821,6 +3821,24 @@ void wallet2::update_pool_state_by_pool_query(std::vector<std::tuple<cryptonote:
 
   MTRACE("update_pool_state_by_pool_query end");
 }
+bool wallet2::tx_spends_ours(const cryptonote::transaction& tx) const
+{
+  std::unordered_set<crypto::key_image> ours;
+  ours.reserve(m_transfers.size());
+  for (const transfer_details &td : m_transfers)
+    ours.insert(td.m_key_image);
+
+  for (const auto &in : tx.vin)
+  {
+    if (in.type() == typeid(cryptonote::txin_to_key))
+    {
+      const cryptonote::txin_to_key &itk = boost::get<cryptonote::txin_to_key>(in);
+      if (ours.find(itk.k_image) != ours.end())
+        return true;
+    }
+  }
+  return false;
+}
 //----------------------------------------------------------------------------------------------------
 // Update pool state from pool data we got together with block data, either incremental data with
 // txs that are new in the pool since the last time we queried and the ids of txs that were
@@ -3885,7 +3903,9 @@ void wallet2::update_pool_state_from_pool_data(bool incremental, const std::vect
   process_txs.clear();
   for (const auto &pool_tx: added_pool_txs)
   {
-    if (accept_pool_tx_for_processing(std::get<1>(pool_tx)))
+    const auto &tx    = std::get<0>(pool_tx);
+    const auto &txid  = std::get<1>(pool_tx);
+    if (accept_pool_tx_for_processing(txid) || tx_spends_ours(tx))
     {
       process_txs.push_back(pool_tx);
     }
diff --git a/src/wallet/wallet2.h b/src/wallet/wallet2.h
index 9efeee0ea..887213264 100644
--- a/src/wallet/wallet2.h
+++ b/src/wallet/wallet2.h
@@ -1834,6 +1834,7 @@ private:
     bool accept_pool_tx_for_processing(const crypto::hash &txid);
     void process_unconfirmed_transfer(bool incremental, const crypto::hash &txid, wallet2::unconfirmed_transfer_details &tx_details, bool seen_in_pool, std::chrono::system_clock::time_point now, bool refreshed);
     void process_pool_info_extent(const cryptonote::COMMAND_RPC_GET_BLOCKS_FAST::response &res, std::vector<std::tuple<cryptonote::transaction, crypto::hash, bool>> &process_txs, bool refreshed);
+    bool tx_spends_ours(const cryptonote::transaction& tx) const;
     void update_pool_state_by_pool_query(std::vector<std::tuple<cryptonote::transaction, crypto::hash, bool>> &process_txs, bool refreshed = false);
     void update_pool_state_from_pool_data(bool incremental, const std::vector<crypto::hash> &removed_pool_txids, const std::vector<std::tuple<cryptonote::transaction, crypto::hash, bool>> &added_pool_txs, std::vector<std::tuple<cryptonote::transaction, crypto::hash, bool>> &process_txs, bool refreshed);
     uint64_t select_transfers(uint64_t needed_money, std::vector<size_t> unused_transfers_indices, std::vector<size_t>& selected_transfers) const;

This also sidesteps the m_scanned_pool_txs cache for this path, because we add such txs to process_txs even if the cache says “already seen.” Which makes sense to me. If tx is ours (tx_spends_ours), I think it is reasonable to make it special.