wallet2: trim stale transfer maps after output imports
What the developer wrote
Authored by selsta
wallet2: trim stale transfer maps after output imports
What changed, and why it matters
This Monero wallet patch cleans up internal lookup tables (key-image and public-key indexes) when the list of owned transaction outputs is shrunk, for example during an output import. Without the cleanup, those indexes could point to entries that no longer exist, which could cause the wallet to crash or behave incorrectly when it later tries to spend or display funds. The patch also repairs any stale indexes when loading an older wallet cache that has never been refreshed from a node.
Treat as a defensive correctness fix. Review whether stale m_pub_keys entries could affect transaction construction or output selection, and consider adding an explicit bounds check before any direct use of these map values. Backport to maintained releases because stale indexes in existing wallets are repaired on cache load only if the wallet has never refreshed from a node.
Security signals we found
Out-of-bounds index retained in wallet lookup maps after container shrink
Potential wallet crash or incorrect spend selection due to stale key-image / public-key mapping
Repair-on-load for legacy wallet caches that predate the fix
No explicit bounds check added to map lookups; relies on trimming and existing throw
Evidence from the diff
wallet2 maintains two maps, m_key_images and m_pub_keys, that map a key image / public key to an offset in m_transfers. When m_transfers is resized down (import_outputs shrinking, or loading a cache with fewer transfers than the maps reference), those offsets can become out-of-bounds or point to the wrong transfer. The new trim_transfer_maps() erases map entries whose index is >= the new m_transfers.size(). It is called before shrinking in both import_outputs overloads, and once during load_wallet_cache for wallets that have never refreshed from a node. A subsequent existing check throws if any stale key-image offset remains.
Changed components
src/wallet/wallet2.cppsrc/wallet/wallet2.hwallet2::import_outputswallet2::load_wallet_cachewallet2::m_key_imageswallet2::m_pub_keyswallet2::m_transfersInspect captured patch +29 / −0
### src/wallet/wallet2.cpp
@@ -6749,6 +6749,24 @@ void wallet2::load(const std::string& wallet_, const epee::wipeable_string& pass
}
}
//----------------------------------------------------------------------------------------------------
+void wallet2::trim_transfer_maps(size_t num_transfers)
+{
+ for (auto it = m_key_images.begin(); it != m_key_images.end(); )
+ {
+ if (it->second >= num_transfers)
+ it = m_key_images.erase(it);
+ else
+ ++it;
+ }
+ for (auto it = m_pub_keys.begin(); it != m_pub_keys.end(); )
+ {
+ if (it->second >= num_transfers)
+ it = m_pub_keys.erase(it);
+ else
+ ++it;
+ }
+}
+//----------------------------------------------------------------------------------------------------
void wallet2::load_wallet_cache(const bool use_fs, const std::string& cache_buf)
{
boost::system::error_code e;
@@ -6863,6 +6881,10 @@ void wallet2::load_wallet_cache(const bool use_fs, const std::string& cache_buf)
ar >> *this;
}
}
+ // Repair stale indices from older output imports.
+ if (!m_has_ever_refreshed_from_node)
+ trim_transfer_maps(m_transfers.size());
+
for (const auto &key_image : m_key_images)
THROW_WALLET_EXCEPTION_IF(key_image.second >= m_transfers.size(), error::wallet_internal_error,
std::string("Key images cache contains illegal transfer offset: ") + std::to_string(key_image.second)
@@ -14511,7 +14533,10 @@ size_t wallet2::import_outputs(const std::tuple<uint64_t, uint64_t, std::vector<
if (offset + output_array.size() > m_transfers.size())
m_transfers.resize(offset + output_array.size());
else if (num_outputs < m_transfers.size())
+ {
+ trim_transfer_maps(num_outputs);
m_transfers.resize(num_outputs);
+ }
for (size_t i = 0; i < output_array.size(); ++i)
{
@@ -14591,7 +14616,10 @@ size_t wallet2::import_outputs(const std::tuple<uint64_t, uint64_t, std::vector<
if (offset + output_array.size() > m_transfers.size())
m_transfers.resize(offset + output_array.size());
else if (num_outputs < m_transfers.size())
+ {
+ trim_transfer_maps(num_outputs);
m_transfers.resize(num_outputs);
+ }
for (size_t i = 0; i < output_array.size(); ++i)
{
### src/wallet/wallet2.h
@@ -1555,6 +1555,7 @@ namespace tools
bool load_keys_buf(const std::string& keys_buf, const epee::wipeable_string& password);
bool load_keys_buf(const std::string& keys_buf, const epee::wipeable_string& password, boost::optional<crypto::chacha_key>& keys_to_encrypt);
void load_wallet_cache(const bool use_fs, const std::string& cache_buf = "");
+ void trim_transfer_maps(size_t num_transfers);
void process_new_transaction(const crypto::hash &txid, const cryptonote::transaction& tx, const std::vector<uint64_t> &o_indices, uint64_t height, uint8_t block_version, uint64_t ts, bool miner_tx, bool pool, bool double_spend_seen, const tx_cache_data &tx_cache_data, std::map<std::pair<uint64_t, uint64_t>, size_t> *output_tracker_cache = NULL, bool ignore_callbacks = false);
bool should_skip_block(const cryptonote::block &b, uint64_t height) const;
void process_new_blockchain_entry(const cryptonote::block& b, const cryptonote::block_complete_entry& bche, const parsed_block &parsed_block, const crypto::hash& bl_id, uint64_t height, const std::vector<tx_cache_data> &tx_cache_data, size_t tx_cache_data_offset, std::map<std::pair<uint64_t, uint64_t>, size_t> *output_tracker_cache = NULL);Why this scored 42/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.