Public commit record
43/100 · Thin
What the developer wrote
Authored by Navid Rahimi
src: fix rpc limit
Co-authored-by: jeffro256 <jeffro256@tutanota.com>
✓ Subject identifies a change✓ Provides an explanatory body
What changed, and why it matters
This Monero update fixes a problem where the daemon's 'get blocks' RPC reply could grow too large and exceed network packet limits. The patch makes the mempool (pool of pending transactions) portion of the response respect a size budget, so it won't be added if the blocks part already fills most of the safe packet size. It also adds internal limits on how many transaction details and how much total transaction blob data are returned. The likely goal is to prevent oversized replies that could cause connection failures or denial-of-service issues for wallets and other nodes.
Recommended action
Treat as a hardening/denial-of-service mitigation patch. Nodes and wallets should upgrade to avoid RPC failures or resource exhaustion from oversized get_blocks responses. No immediate emergency response is indicated by the diff alone, but operators running public/restricted RPC endpoints should prioritize the update.
Security signals we found
01
RPC response size bounded against LEVIN_DEFAULT_MAX_PACKET_SIZE
02
Mempool transaction enumeration now limited by both count and cumulative blob size
03
Potential denial-of-service vector: unbounded mempool info in get_blocks response
04
Restricted RPC max transaction count already existed; this adds a size-based cap
05
Remaining txids returned so clients can fetch omitted transactions incrementally
Evidence from the diff
The commit modifies the tx_memory_pool and core RPC server to enforce a cumulative transaction blob size limit on mempool info returned by on_get_blocks. get_transactions_info now accepts cumul_txblob_size_limit and max_tx_count, skips entries that would exceed the cumulative blob size, and reserves output capacity safely. get_pool_info was refactored to compute a remaining byte budget (clawback for removed/remaining/added txid overhead), call the limited get_transactions_info, and populate remaining_added_txids for any txids not included. core_rpc_server.cpp now computes cumul_block_data_size while building the blocks response, then only calls get_pool_info with pool_limit = 0.9 * LEVIN_DEFAULT_MAX_PACKET_SIZE - cumul_block_data_size, and skips pool info entirely if blocks already exceed the budget. The change is defensive and appears intended to avoid generating RPC messages larger than the Levin protocol maximum packet size.
Changed components
src/cryptonote_core/tx_pool.cppsrc/cryptonote_core/tx_pool.hsrc/cryptonote_core/cryptonote_core.cppsrc/cryptonote_core/cryptonote_core.hsrc/rpc/core_rpc_server.cppInspect captured patch +155 / −94
diff --git a/src/cryptonote_core/cryptonote_core.cpp b/src/cryptonote_core/cryptonote_core.cpp
index 473afa4..f78a958 100644
--- a/src/cryptonote_core/cryptonote_core.cpp
+++ b/src/cryptonote_core/cryptonote_core.cpp
@@ -1546,9 +1546,9 @@ namespace cryptonote
return m_blockchain_storage.have_block(id, where);
}
//-----------------------------------------------------------------------------------------------
- bool core::get_pool_transactions_info(const std::vector<crypto::hash>& txids, std::vector<std::pair<crypto::hash, tx_memory_pool::tx_details>>& txs, bool include_sensitive_txes) const
+ bool core::get_pool_transactions_info(const std::vector<crypto::hash>& txids, std::vector<std::pair<crypto::hash, tx_memory_pool::tx_details>>& txs, bool include_sensitive_txes, size_t limit_size) const
{
- return m_mempool.get_transactions_info(txids, txs, include_sensitive_txes);
+ return m_mempool.get_transactions_info(epee::to_span(txids), txs, include_sensitive_txes, limit_size);
}
//-----------------------------------------------------------------------------------------------
bool core::get_pool_transactions(std::vector<transaction>& txs, bool include_sensitive_data) const
@@ -1563,9 +1563,9 @@ namespace cryptonote
return true;
}
//-----------------------------------------------------------------------------------------------
- bool core::get_pool_info(time_t start_time, bool include_sensitive_txes, size_t max_tx_count, std::vector<std::pair<crypto::hash, tx_memory_pool::tx_details>>& added_txs, std::vector<crypto::hash>& remaining_added_txids, std::vector<crypto::hash>& removed_txs, bool& incremental) const
+ bool core::get_pool_info(time_t start_time, bool include_sensitive_txes, size_t max_tx_count, std::vector<std::pair<crypto::hash, tx_memory_pool::tx_details>>& added_txs, std::vector<crypto::hash>& remaining_added_txids, std::vector<crypto::hash>& removed_txs, bool& incremental, size_t limit_size) const
{
- return m_mempool.get_pool_info(start_time, include_sensitive_txes, max_tx_count, added_txs, remaining_added_txids, removed_txs, incremental);
+ return m_mempool.get_pool_info(start_time, include_sensitive_txes, max_tx_count, added_txs, remaining_added_txids, removed_txs, incremental, limit_size);
}
//-----------------------------------------------------------------------------------------------
bool core::get_pool_transaction_stats(struct txpool_stats& stats, bool include_sensitive_data) const
diff --git a/src/cryptonote_core/cryptonote_core.h b/src/cryptonote_core/cryptonote_core.h
index e9b2571..773e154 100644
--- a/src/cryptonote_core/cryptonote_core.h
+++ b/src/cryptonote_core/cryptonote_core.h
@@ -511,7 +511,7 @@ namespace cryptonote
*
* @note see tx_memory_pool::get_pool_transactions_info
*/
- bool get_pool_transactions_info(const std::vector<crypto::hash>& txids, std::vector<std::pair<crypto::hash, tx_memory_pool::tx_details>>& txs, bool include_sensitive_txes = false) const;
+ bool get_pool_transactions_info(const std::vector<crypto::hash>& txids, std::vector<std::pair<crypto::hash, tx_memory_pool::tx_details>>& txs, bool include_sensitive_txes = false, size_t limit_size = 0) const;
/**
* @copydoc tx_memory_pool::get_pool_info
@@ -520,7 +520,7 @@ namespace cryptonote
*
* @note see tx_memory_pool::get_pool_info
*/
- bool get_pool_info(time_t start_time, bool include_sensitive_txes, size_t max_tx_count, std::vector<std::pair<crypto::hash, tx_memory_pool::tx_details>>& added_txs, std::vector<crypto::hash>& remaining_added_txids, std::vector<crypto::hash>& removed_txs, bool& incremental) const;
+ bool get_pool_info(time_t start_time, bool include_sensitive_txes, size_t max_tx_count, std::vector<std::pair<crypto::hash, tx_memory_pool::tx_details>>& added_txs, std::vector<crypto::hash>& remaining_added_txids, std::vector<crypto::hash>& removed_txs, bool& incremental, size_t limit_size = 0) const;
/**
* @copydoc tx_memory_pool::get_transactions
diff --git a/src/cryptonote_core/tx_pool.cpp b/src/cryptonote_core/tx_pool.cpp
index d682404..d72c92f 100644
--- a/src/cryptonote_core/tx_pool.cpp
+++ b/src/cryptonote_core/tx_pool.cpp
@@ -665,22 +665,35 @@ namespace cryptonote
return true;
}
//------------------------------------------------------------------
- bool tx_memory_pool::get_transactions_info(const std::vector<crypto::hash>& txids, std::vector<std::pair<crypto::hash, tx_details>>& txs, bool include_sensitive) const
+ bool tx_memory_pool::get_transactions_info(const epee::span<const crypto::hash> txids, std::vector<std::pair<crypto::hash, tx_details>>& txs, bool include_sensitive, size_t cumul_txblob_size_limit, size_t max_tx_count) const
{
CRITICAL_REGION_LOCAL(m_transactions_lock);
CRITICAL_REGION_LOCAL1(m_blockchain);
txs.clear();
- for (const auto &it: txids)
- {
- tx_details details;
- bool success = get_transaction_info(it, details, include_sensitive, true/*include_blob*/);
- if (success)
+ const size_t max_allowed =
+ max_tx_count ? std::min(max_tx_count, txids.size()) : txids.size();
+
+ txs.reserve(std::min<size_t>(max_allowed, 1000000)); // reserve limited to min(1 million, max_allowed)
+ size_t cumul_txblob_size = 0;
+
+ for (size_t i = 0; i < txids.size() && txs.size() < max_allowed; ++i)
{
- txs.push_back(std::make_pair(it, std::move(details)));
+ const crypto::hash &it{txids[i]};
+ tx_details details;
+ const bool success = get_transaction_info(it, details, include_sensitive, true /*include_blob*/);
+ if (!success)
+ continue;
+
+ if (cumul_txblob_size_limit &&
+ cumul_txblob_size + details.blob_size >= cumul_txblob_size_limit)
+ continue;
+
+ cumul_txblob_size += details.blob_size;
+ txs.emplace_back(it, std::move(details));
}
- }
+
return true;
}
//---------------------------------------------------------------------------------
@@ -952,7 +965,7 @@ namespace cryptonote
}, false, category);
}
//------------------------------------------------------------------
- bool tx_memory_pool::get_pool_info(time_t start_time, bool include_sensitive, size_t max_tx_count, std::vector<std::pair<crypto::hash, tx_details>>& added_txs, std::vector<crypto::hash>& remaining_added_txids, std::vector<crypto::hash>& removed_txs, bool& incremental) const
+ bool tx_memory_pool::get_pool_info(time_t start_time, bool include_sensitive, size_t max_tx_count, std::vector<std::pair<crypto::hash, tx_details>>& added_txs, std::vector<crypto::hash>& remaining_added_txids, std::vector<crypto::hash>& removed_txs, bool& incremental, size_t cumul_limit_size) const
{
CRITICAL_REGION_LOCAL(m_transactions_lock);
CRITICAL_REGION_LOCAL1(m_blockchain);
@@ -983,47 +996,76 @@ namespace cryptonote
remaining_added_txids.clear();
removed_txs.clear();
- std::vector<crypto::hash> txids;
if (!incremental)
- {
LOG_PRINT_L2("Giving back the whole pool");
- // Give back the whole pool in 'added_txs'; because calling 'get_transaction_info' right inside the
- // anonymous method somehow results in an LMDB error with transactions we have to build a list of
- // ids first and get the full info afterwards
- get_transaction_hashes(txids, include_sensitive);
- if (txids.size() > max_tx_count)
+
+ // If incremental, handle removed TXIDs first since it's important that txs are removed
+ // from synchronizers' pools, and we need need to estimate how much space we have left to
+ // request full-bodied txs
+ if (incremental)
+ {
+ std::multimap<time_t, removed_tx_info>::const_iterator rit = m_removed_txs_by_time.lower_bound(start_time);
+ while (rit != m_removed_txs_by_time.end())
{
- remaining_added_txids = std::vector<crypto::hash>(txids.begin() + max_tx_count, txids.end());
- txids.erase(txids.begin() + max_tx_count, txids.end());
+ if (include_sensitive || !rit->second.sensitive)
+ {
+ removed_txs.push_back(rit->second.txid);
+ }
+ ++rit;
}
- get_transactions_info(txids, added_txs, include_sensitive);
- return true;
}
- // Give back incrementally, based on time of entry into the map
+ // Build a list of TXIDs that we want information for eventually. When `!incremental`, we might
+ // collect TXIDs that have already left the pool which will cause a followup RPC call that
+ // fetches non-existent txs, but that's *okay* since a non-incremental caller probably doesn't
+ // care about being incredibly efficient. This problem is also unavoidable anyways if we need to
+ // ever return anything inside `remaining_added_txids`, since those txs might have left the mempool
+ // by the time the followup RPC call reaches the daemon
+ std::vector<crypto::hash> txids;
+ txids.reserve(m_added_txs_by_id.size());
for (const auto &pit : m_added_txs_by_id)
{
- if (pit.second >= start_time)
+ const bool relevant_txid{!incremental || pit.second >= start_time};
+ if (relevant_txid)
txids.push_back(pit.first);
}
- get_transactions_info(txids, added_txs, include_sensitive);
- if (added_txs.size() > max_tx_count)
+
+ // Estimate max cumulative size left for full tx blobs
+ const size_t removed_txids_clawback{32 * removed_txs.size()};
+ const size_t remaining_txids_clawback{32 * txids.size()};
+ const size_t added_tx_txid_clawback(32 * txids.size());
+ const size_t total_clawback{removed_txids_clawback + remaining_txids_clawback + added_tx_txid_clawback};
+ const size_t cumulative_txblob_size_limit{cumul_limit_size > total_clawback ? cumul_limit_size - total_clawback : 0};
+
+ // Perform TX info fetch, limited to max_tx_count and cumulative_txblob_size_limit
+ if (cumulative_txblob_size_limit && !txids.empty() && max_tx_count)
{
- remaining_added_txids.reserve(added_txs.size() - max_tx_count);
- for (size_t i = max_tx_count; i < added_txs.size(); ++i)
- remaining_added_txids.push_back(added_txs[i].first);
- added_txs.erase(added_txs.begin() + max_tx_count, added_txs.end());
+ if (!get_transactions_info(epee::to_span(txids),
+ added_txs,
+ include_sensitive,
+ cumulative_txblob_size_limit,
+ max_tx_count))
+ return false;
}
- std::multimap<time_t, removed_tx_info>::const_iterator rit = m_removed_txs_by_time.lower_bound(start_time);
- while (rit != m_removed_txs_by_time.end())
+ // Populate `remaining_added_txids` with all TXIDs in `txids` and not in `added_txs`
+ if (added_txs.size() < txids.size())
{
- if (include_sensitive || !rit->second.sensitive)
- {
- removed_txs.push_back(rit->second.txid);
+ // 1. Create a set of added IDs for fast lookup
+ std::unordered_set<crypto::hash> added_set;
+ for (const auto& tx : added_txs) {
+ added_set.insert(tx.first);
+ }
+
+ // 2. Iterate the original list. If not in set, it is "remaining".
+ remaining_added_txids.reserve(txids.size() - added_txs.size());
+ for (const auto& id : txids) {
+ if (added_set.find(id) == added_set.end()) {
+ remaining_added_txids.push_back(id);
+ }
}
- ++rit;
}
+
return true;
}
//------------------------------------------------------------------
diff --git a/src/cryptonote_core/tx_pool.h b/src/cryptonote_core/tx_pool.h
index 8528e63..fbdf7b2 100644
--- a/src/cryptonote_core/tx_pool.h
+++ b/src/cryptonote_core/tx_pool.h
@@ -492,14 +492,14 @@ namespace cryptonote
};
/**
- * @brief get infornation about a single transaction
+ * @brief get information about a single transaction
*/
bool get_transaction_info(const crypto::hash &txid, tx_details &td, bool include_sensitive_data, bool include_blob = false) const;
/**
* @brief get information about multiple transactions
*/
- bool get_transactions_info(const std::vector<crypto::hash>& txids, std::vector<std::pair<crypto::hash, tx_details>>& txs, bool include_sensitive_data = false) const;
+ bool get_transactions_info(const epee::span<const crypto::hash> txids, std::vector<std::pair<crypto::hash, tx_details>>& txs, bool include_sensitive_data = false, size_t cumul_txblob_size_limit = 0, size_t max_tx_count = 0) const;
/**
* @brief get transactions not in the passed set
@@ -511,7 +511,7 @@ namespace cryptonote
*
* @return true on success, false on error
*/
- bool get_pool_info(time_t start_time, bool include_sensitive, size_t max_tx_count, std::vector<std::pair<crypto::hash, tx_details>>& added_txs, std::vector<crypto::hash>& remaining_added_txids, std::vector<crypto::hash>& removed_txs, bool& incremental) const;
+ bool get_pool_info(time_t start_time, bool include_sensitive, size_t max_tx_count, std::vector<std::pair<crypto::hash, tx_details>>& added_txs, std::vector<crypto::hash>& remaining_added_txids, std::vector<crypto::hash>& removed_txs, bool& incremental, size_t cumul_limit_size = 0) const;
private:
diff --git a/src/rpc/core_rpc_server.cpp b/src/rpc/core_rpc_server.cpp
index bf4cdb4..68fb54a 100644
--- a/src/rpc/core_rpc_server.cpp
+++ b/src/rpc/core_rpc_server.cpp
@@ -673,53 +673,7 @@ namespace cryptonote
res.pool_info_extent = COMMAND_RPC_GET_BLOCKS_FAST::NONE;
- if (get_pool)
- {
- const bool restricted = m_restricted && ctx;
- const bool request_has_rpc_origin = ctx != NULL;
- const bool allow_sensitive = !request_has_rpc_origin || !restricted;
- const size_t max_tx_count = restricted ? RESTRICTED_TRANSACTIONS_COUNT : std::numeric_limits<size_t>::max();
-
- bool incremental;
- std::vector<std::pair<crypto::hash, tx_memory_pool::tx_details>> added_pool_txs;
- bool success = m_core.get_pool_info((time_t)req.pool_info_since, allow_sensitive, max_tx_count, added_pool_txs, res.remaining_added_pool_txids, res.removed_pool_txids, incremental);
- if (success)
- {
- res.added_pool_txs.clear();
- if (m_rpc_payment)
- {
- CHECK_PAYMENT_SAME_TS(req, res, added_pool_txs.size() * COST_PER_TX + (res.remaining_added_pool_txids.size() + res.removed_pool_txids.size()) * COST_PER_POOL_HASH);
- }
- for (const auto &added_pool_tx: added_pool_txs)
- {
- COMMAND_RPC_GET_BLOCKS_FAST::pool_tx_info info;
- info.tx_hash = added_pool_tx.first;
- std::stringstream oss;
- binary_archive<true> ar(oss);
- bool r = req.prune
- ? const_cast<cryptonote::transaction&>(added_pool_tx.second.tx).serialize_base(ar)
- : ::serialization::serialize(ar, const_cast<cryptonote::transaction&>(added_pool_tx.second.tx));
- if (!r)
- {
- res.status = "Failed to serialize transaction";
- return true;
- }
- info.tx_blob = oss.str();
- info.double_spend_seen = added_pool_tx.second.double_spend_seen;
- res.added_pool_txs.push_back(std::move(info));
- }
- }
- if (success)
- {
- res.pool_info_extent = incremental ? COMMAND_RPC_GET_BLOCKS_FAST::INCREMENTAL : COMMAND_RPC_GET_BLOCKS_FAST::FULL;
- }
- else
- {
- res.status = "Failed to get pool info";
- return true;
- }
- }
-
+ size_t cumul_block_data_size = 0;
if (get_blocks)
{
// quick check for noop
@@ -764,7 +718,7 @@ namespace cryptonote
CHECK_PAYMENT_SAME_TS(req, res, bs.size() * COST_PER_BLOCK);
- size_t size = 0, ntxes = 0;
+ size_t ntxes = 0;
res.blocks.reserve(bs.size());
res.output_indices.reserve(bs.size());
for(auto& bd: bs)
@@ -772,7 +726,7 @@ namespace cryptonote
res.blocks.resize(res.blocks.size()+1);
res.blocks.back().pruned = req.prune;
res.blocks.back().block = bd.first.first;
- size += bd.first.first.size();
+ cumul_block_data_size += bd.first.first.size();
res.output_indices.push_back(COMMAND_RPC_GET_BLOCKS_FAST::block_output_indices());
ntxes += bd.second.size();
res.output_indices.back().indices.reserve(1 + bd.second.size());
@@ -784,7 +738,7 @@ namespace cryptonote
res.blocks.back().txs.push_back({std::move(i->second), crypto::null_hash});
i->second.clear();
i->second.shrink_to_fit();
- size += res.blocks.back().txs.back().blob.size();
+ cumul_block_data_size += res.blocks.back().txs.back().blob.size();
}
const size_t n_txes_to_lookup = bd.second.size() + (req.no_miner_tx ? 0 : 1);
@@ -806,7 +760,72 @@ namespace cryptonote
res.output_indices.back().indices.push_back({std::move(indices[i])});
}
}
- MDEBUG("on_get_blocks: " << bs.size() << " blocks, " << ntxes << " txes, size " << size);
+ MDEBUG("on_get_blocks: " << bs.size() << " blocks, " << ntxes << " txes, size " << cumul_block_data_size);
+ }
+
+ if (get_pool)
+ {
+ const bool restricted = m_restricted && ctx;
+ const bool request_has_rpc_origin = ctx != NULL;
+ const bool allow_sensitive = !request_has_rpc_origin || !restricted;
+ const size_t max_tx_count = restricted ? RESTRICTED_TRANSACTIONS_COUNT : std::numeric_limits<size_t>::max();
+
+ const size_t limit = LEVIN_DEFAULT_MAX_PACKET_SIZE * 0.9;
+
+ // If the blocks alone already consume the safe packet budget, skip pool info
+ if (cumul_block_data_size >= limit)
+ {
+ LOG_ERROR("on_get_blocks: omitting pool info, response already "
+ << cumul_block_data_size << " bytes (limit " << limit << ")");
+ // res.pool_info_extent stays as NONE (set earlier), which tells wallets
+ // that there is no pool info in this response.
+ }
+ else
+ {
+ const size_t pool_limit = limit - cumul_block_data_size;
+
+ bool incremental;
+ std::vector<std::pair<crypto::hash, tx_memory_pool::tx_details>> added_pool_txs;
+ bool success = m_core.get_pool_info((time_t)req.pool_info_since, allow_sensitive, max_tx_count,
+ added_pool_txs, res.remaining_added_pool_txids, res.removed_pool_txids,
+ incremental, pool_limit);
+
+ if (!success)
+ {
+ res.status = "Failed to get pool info";
+ return true;
+ }
+
+ res.added_pool_txs.clear();
+ if (m_rpc_payment)
+ {
+ CHECK_PAYMENT_SAME_TS(req, res,
+ added_pool_txs.size() * COST_PER_TX +
+ (res.remaining_added_pool_txids.size() + res.removed_pool_txids.size()) * COST_PER_POOL_HASH);
+ }
+ for (const auto &added_pool_tx: added_pool_txs)
+ {
+ COMMAND_RPC_GET_BLOCKS_FAST::pool_tx_info info;
+ info.tx_hash = added_pool_tx.first;
+ std::stringstream oss;
+ binary_archive<true> ar(oss);
+ bool r = req.prune
+ ? const_cast<cryptonote::transaction&>(added_pool_tx.second.tx).serialize_base(ar)
+ : ::serialization::serialize(ar, const_cast<cryptonote::transaction&>(added_pool_tx.second.tx));
+ if (!r)
+ {
+ res.status = "Failed to serialize transaction";
+ return true;
+ }
+ info.tx_blob = oss.str();
+ info.double_spend_seen = added_pool_tx.second.double_spend_seen;
+ res.added_pool_txs.push_back(std::move(info));
+ }
+
+ res.pool_info_extent = incremental
+ ? COMMAND_RPC_GET_BLOCKS_FAST::INCREMENTAL
+ : COMMAND_RPC_GET_BLOCKS_FAST::FULL;
+ }
}
res.status = CORE_RPC_STATUS_OK;
Risk score
Our methodology →Why this scored 60/100
Human-validated context
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
No validated notes yet.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.