wallet2: avoid linear scans in pool state updates
What changed, and why it matters
This commit is a performance optimization in the Monero wallet. It replaces repeated linear list scans with hash-set lookups when checking which transactions are in the memory pool. The change should make wallet refresh faster when there are many pending transactions, but it does not appear to fix a security vulnerability or change behavior in a way that is directly exploitable.
No security action required. Treat as a normal performance improvement. Standard review and testing for correctness are sufficient.
Security signals we found
Performance optimization only: replaces linear scans with hash-set lookups
No change to transaction validation, cryptography, or consensus rules
No new trust boundaries or RPC interfaces introduced
No input sanitization or memory-safety fixes present
Logic in accept_pool_tx_for_processing remains semantically equivalent
Evidence from the diff
The patch refactors wallet2 pool-state handling. It introduces std::unordered_set
Changed components
src/wallet/wallet2.cppsrc/wallet/wallet2.hwallet2::remove_obsolete_pool_txswallet2::accept_pool_tx_for_processingwallet2::update_pool_state_by_pool_querywallet2::update_pool_state_from_pool_dataread_pool_txs helperInspect captured patch +50 / −71
diff --git a/src/wallet/wallet2.cpp b/src/wallet/wallet2.cpp
index b89370f..adcfd07 100644
--- a/src/wallet/wallet2.cpp
+++ b/src/wallet/wallet2.cpp
@@ -3089,6 +3089,7 @@ void read_pool_txs(const cryptonote::COMMAND_RPC_GET_TRANSACTIONS::request &req,
MDEBUG("Reading pool txs");
if (res.txs.size() == req.txs_hashes.size())
{
+ const std::unordered_set<crypto::hash> txid_set(txids.begin(), txids.end());
for (const auto &tx_entry: res.txs)
{
if (tx_entry.in_pool)
@@ -3099,9 +3100,7 @@ void read_pool_txs(const cryptonote::COMMAND_RPC_GET_TRANSACTIONS::request &req,
if (get_pruned_tx(tx_entry, tx, tx_hash))
{
- const std::vector<crypto::hash>::const_iterator i = std::find_if(txids.begin(), txids.end(),
- [tx_hash](const crypto::hash &e) { return e == tx_hash; });
- if (i != txids.end())
+ if (txid_set.count(tx_hash) > 0)
{
txs.emplace_back(std::move(tx), tx_hash, tx_entry.double_spend_seen);
}
@@ -3561,6 +3560,11 @@ void wallet2::pull_and_parse_next_blocks(bool first, bool try_incremental, uint6
}
void wallet2::remove_obsolete_pool_txs(const std::vector<crypto::hash> &tx_hashes, bool remove_if_found)
+{
+ remove_obsolete_pool_txs(std::unordered_set<crypto::hash>(tx_hashes.begin(), tx_hashes.end()), remove_if_found);
+}
+
+void wallet2::remove_obsolete_pool_txs(const std::unordered_set<crypto::hash> &tx_hashes, bool remove_if_found)
{
// remove pool txes to us that aren't in the pool anymore (remove_if_found = false),
// or remove pool txes to us that were reported as removed (remove_if_found = true)
@@ -3568,15 +3572,7 @@ void wallet2::remove_obsolete_pool_txs(const std::vector<crypto::hash> &tx_hashe
while (uit != m_unconfirmed_payments.end())
{
const crypto::hash &txid = uit->second.m_pd.m_tx_hash;
- bool found = false;
- for (const auto &it2: tx_hashes)
- {
- if (it2 == txid)
- {
- found = true;
- break;
- }
- }
+ const bool found = tx_hashes.count(txid) > 0;
auto pit = uit++;
if ((!remove_if_found && !found) || (remove_if_found && found))
{
@@ -3592,17 +3588,9 @@ void wallet2::remove_obsolete_pool_txs(const std::vector<crypto::hash> &tx_hashe
// Code that is common to 'update_pool_state_by_pool_query' and 'update_pool_state_from_pool_data':
// Check wether a tx in the pool is worthy of processing because we did not see it
// yet or because it is "interesting" out of special circumstances
-bool wallet2::accept_pool_tx_for_processing(const crypto::hash &txid)
+bool wallet2::accept_pool_tx_for_processing(const crypto::hash &txid, const std::unordered_set<crypto::hash> &payments_tx_hashes)
{
- bool txid_found_in_up = false;
- for (const auto &up: m_unconfirmed_payments)
- {
- if (up.second.m_pd.m_tx_hash == txid)
- {
- txid_found_in_up = true;
- break;
- }
- }
+ const bool txid_found_in_up = payments_tx_hashes.count(txid) > 0;
if (m_scanned_pool_txs[0].find(txid) != m_scanned_pool_txs[0].end() || m_scanned_pool_txs[1].find(txid) != m_scanned_pool_txs[1].end())
{
// if it's for us, we want to keep track of whether we saw a double spend, so don't bail out
@@ -3615,30 +3603,25 @@ bool wallet2::accept_pool_tx_for_processing(const crypto::hash &txid)
if (!txid_found_in_up)
{
LOG_PRINT_L1("Found new pool tx: " << txid);
- bool found = false;
- for (const auto &i: m_unconfirmed_txs)
+ const auto i = m_unconfirmed_txs.find(txid);
+ bool sent_by_us = i != m_unconfirmed_txs.end();
+ if (sent_by_us)
{
- if (i.first == txid)
+ const unconfirmed_transfer_details& utd = i->second;
+ for (const auto& dst : utd.m_dests)
{
- found = true;
- // if this is a payment to yourself at a different subaddress account, don't skip it
- // so that you can see the incoming pool tx with 'show_transfers' on that receiving subaddress account
- const unconfirmed_transfer_details& utd = i.second;
- for (const auto& dst : utd.m_dests)
+ auto subaddr_index = m_subaddresses.find(dst.addr.m_spend_public_key);
+ if (subaddr_index != m_subaddresses.end() && subaddr_index->second.major != utd.m_subaddr_account)
{
- auto subaddr_index = m_subaddresses.find(dst.addr.m_spend_public_key);
- if (subaddr_index != m_subaddresses.end() && subaddr_index->second.major != utd.m_subaddr_account)
- {
- found = false;
- break;
- }
+ // Payment to ourselves at a different subaddress account:
+ // process it so the receiving account can show the incoming pool tx.
+ sent_by_us = false;
+ break;
}
- break;
}
}
- if (!found)
+ if (!sent_by_us)
{
- // not one of those we sent ourselves
return true;
}
else
@@ -3802,19 +3785,14 @@ void wallet2::update_pool_state_by_pool_query(std::vector<std::tuple<cryptonote:
// remove any pending tx that's not in the pool
const auto now = std::chrono::system_clock::now();
std::unordered_map<crypto::hash, wallet2::unconfirmed_transfer_details>::iterator it = m_unconfirmed_txs.begin();
+
+ const std::unordered_set<crypto::hash> pool_set(res.tx_hashes.begin(), res.tx_hashes.end());
+
while (it != m_unconfirmed_txs.end())
{
const crypto::hash &txid = it->first;
MDEBUG("Checking m_unconfirmed_txs entry " << txid);
- bool found = false;
- for (const auto &it2: res.tx_hashes)
- {
- if (it2 == txid)
- {
- found = true;
- break;
- }
- }
+ const bool found = pool_set.count(txid) > 0;
auto pit = it++;
process_unconfirmed_transfer(false, txid, pit->second, found, now, refreshed);
MDEBUG("New state of that entry: " << pit->second.m_state);
@@ -3826,15 +3804,21 @@ void wallet2::update_pool_state_by_pool_query(std::vector<std::tuple<cryptonote:
// the in transfers list instead (or nowhere if it just
// disappeared without being mined)
if (refreshed)
- remove_obsolete_pool_txs(res.tx_hashes, false);
+ remove_obsolete_pool_txs(pool_set, false);
MTRACE("update_pool_state_by_pool_query done second loop");
+ std::unordered_set<crypto::hash> payments_tx_hashes;
+ payments_tx_hashes.reserve(m_unconfirmed_payments.size());
+ for (const auto &p: m_unconfirmed_payments)
+ payments_tx_hashes.insert(p.second.m_pd.m_tx_hash);
+
// gather txids of new pool txes to us
std::vector<crypto::hash> txids;
+ txids.reserve(res.tx_hashes.size());
for (const auto &txid: res.tx_hashes)
{
- if (accept_pool_tx_for_processing(txid))
+ if (accept_pool_tx_for_processing(txid, payments_tx_hashes))
txids.push_back(txid);
}
@@ -3861,6 +3845,11 @@ void wallet2::update_pool_state_from_pool_data(bool incremental, const std::vect
m_encrypt_keys_after_refresh.reset();
});
+ std::unordered_set<crypto::hash> added_pool_txids;
+ added_pool_txids.reserve(added_pool_txs.size());
+ for (const auto &pool_tx: added_pool_txs)
+ added_pool_txids.insert(std::get<1>(pool_tx));
+
if (refreshed)
{
if (incremental)
@@ -3873,16 +3862,8 @@ void wallet2::update_pool_state_from_pool_data(bool incremental, const std::vect
}
else
{
- // Delete from the list of unconfirmed payments what we don't find anymore in the pool; a bit
- // unfortunate that we have to build a new vector with ids first, but better than copying and
- // modifying the code of 'remove_obsolete_pool_txs' here
- std::vector<crypto::hash> txids;
- txids.reserve(added_pool_txs.size());
- for (const auto &pool_tx: added_pool_txs)
- {
- txids.push_back(std::get<1>(pool_tx));
- }
- remove_obsolete_pool_txs(txids, false);
+ // Delete from the list of unconfirmed payments what we don't find anymore in the pool
+ remove_obsolete_pool_txs(added_pool_txids, false);
}
}
@@ -3893,15 +3874,7 @@ void wallet2::update_pool_state_from_pool_data(bool incremental, const std::vect
{
const crypto::hash &txid = it->first;
MDEBUG("Checking m_unconfirmed_txs entry " << txid);
- bool found = false;
- for (const auto &pool_tx: added_pool_txs)
- {
- if (std::get<1>(pool_tx) == txid)
- {
- found = true;
- break;
- }
- }
+ const bool found = added_pool_txids.count(txid) > 0;
auto pit = it++;
process_unconfirmed_transfer(incremental, txid, pit->second, found, now, refreshed);
MDEBUG("Resulting state of that entry: " << pit->second.m_state);
@@ -3911,9 +3884,13 @@ void wallet2::update_pool_state_from_pool_data(bool incremental, const std::vect
// if we work incrementally and thus see only new pool txs since last time we asked it should
// be rare that we know already about one of those, but check nevertheless
process_txs.clear();
+ std::unordered_set<crypto::hash> payments_tx_hashes;
+ payments_tx_hashes.reserve(m_unconfirmed_payments.size());
+ for (const auto &p: m_unconfirmed_payments)
+ payments_tx_hashes.insert(p.second.m_pd.m_tx_hash);
for (const auto &pool_tx: added_pool_txs)
{
- if (accept_pool_tx_for_processing(std::get<1>(pool_tx)))
+ if (accept_pool_tx_for_processing(std::get<1>(pool_tx), payments_tx_hashes))
{
process_txs.push_back(pool_tx);
}
diff --git a/src/wallet/wallet2.h b/src/wallet/wallet2.h
index 36e5082..673a6d2 100644
--- a/src/wallet/wallet2.h
+++ b/src/wallet/wallet2.h
@@ -31,6 +31,7 @@
#pragma once
#include <memory>
+#include <unordered_set>
#include <boost/program_options/options_description.hpp>
#include <boost/program_options/variables_map.hpp>
@@ -1637,6 +1638,7 @@ private:
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);
+ void remove_obsolete_pool_txs(const std::unordered_set<crypto::hash> &tx_hashes, bool remove_if_found);
std::string encrypt(const char *plaintext, size_t len, const crypto::secret_key &skey, bool authenticated = true) const;
std::string encrypt(const epee::span<char> &span, const crypto::secret_key &skey, bool authenticated = true) const;
@@ -1829,7 +1831,7 @@ private:
void fast_refresh(uint64_t stop_height, uint64_t &blocks_start_height, std::list<crypto::hash> &short_chain_history, bool force = false);
void pull_and_parse_next_blocks(bool first, bool try_incremental, uint64_t start_height, uint64_t &blocks_start_height, std::list<crypto::hash> &short_chain_history, const std::vector<cryptonote::block_complete_entry> &prev_blocks, const std::vector<parsed_block> &prev_parsed_blocks, std::vector<cryptonote::block_complete_entry> &blocks, std::vector<parsed_block> &parsed_blocks, std::vector<std::tuple<cryptonote::transaction, crypto::hash, bool>>& process_pool_txs, bool &last, bool &error, std::exception_ptr &exception);
void process_parsed_blocks(const uint64_t start_height, const std::vector<cryptonote::block_complete_entry> &blocks, const std::vector<parsed_block> &parsed_blocks, uint64_t& blocks_added, std::map<std::pair<uint64_t, uint64_t>, size_t> *output_tracker_cache = NULL);
- bool accept_pool_tx_for_processing(const crypto::hash &txid);
+ bool accept_pool_tx_for_processing(const crypto::hash &txid, const std::unordered_set<crypto::hash> &payments_tx_hashes);
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);
void update_pool_state_by_pool_query(std::vector<std::tuple<cryptonote::transaction, crypto::hash, bool>> &process_txs, bool refreshed = false);
Why this scored 19/100
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.