← Watch feed
rpc: replace get_blocks.bin Levin pool budget
Public commit record
68/100 · Adequate
What the developer wrote
Authored by selsta
rpc: replace get_blocks.bin Levin pool budget
/get_blocks.bin is HTTP binary RPC serialized with portable storage, not a
P2P Levin message. The previously added pool limiter used
LEVIN_DEFAULT_MAX_PACKET_SIZE as a response budget, but that constant does
not apply to the RPC path.
✓ Descriptive subject✓ Names a concrete action or component✓ Provides detailed explanatory context
What changed, and why it matters
This Monero code change fixes how the /get_blocks.bin RPC endpoint limits the size of transaction-pool data it returns. Previously, the code reused a P2P Levin packet-size constant that does not apply to HTTP RPC responses, and it tried to budget pool data against the size of the block data already sent. The patch removes that size-budget plumbing and instead caps the number of added pool transactions at a fixed maximum (20,000). This is a correctness fix for response-size limiting, but it also removes a cumulative byte-size cap that could, in edge cases, allow very large individual transactions to make responses bigger than intended.
Recommended action
Treat this as a hardening/correctness patch rather than a confirmed vulnerability. Operators should upgrade to avoid RPC response-size anomalies. Reviewers should verify that the new 20,000 transaction count cap, combined with existing HTTP server limits, provides adequate DoS protection for the /get_blocks.bin endpoint, especially against large or maliciously crafted transactions.
Security signals we found
01
Incorrect transport-layer size constant reused for HTTP RPC response limiting
02
Removal of cumulative byte-size cap on returned transaction pool blobs
03
Replacement with a count-based cap (20,000 transactions)
04
Potential for oversized RPC responses if individual transactions are very large
05
Fixes a logic bug where block-data size could zero out pool-info budget
Evidence from the diff
The commit removes the cumul_txblob_size_limit / cumul_limit_size parameters from tx_memory_pool::get_transactions_info, tx_memory_pool::get_pool_info, core::get_pool_transactions_info, and core::get_pool_info. It also removes the cumul_block_data_size output from core_rpc_server::handle_get_blocks. In core_rpc_server::on_get_blocks, the previous logic computed a 90% fraction of LEVIN_DEFAULT_MAX_PACKET_SIZE, subtracted the serialized block-data size, and passed the remainder as a byte budget for pool transactions. That budget is now gone; the code simply passes a max_tx_count capped at GET_BLOCKS_BIN_MAX_ADDED_POOL_TX_BODIES (20,000). The pool layer no longer skips transactions whose blob_size would exceed a cumulative byte limit. The commit message explicitly states the old constant was being used in the wrong path (HTTP binary RPC vs. P2P Levin).
Changed components
src/rpc/core_rpc_server.cppsrc/rpc/core_rpc_server.hsrc/cryptonote_core/tx_pool.cppsrc/cryptonote_core/tx_pool.hsrc/cryptonote_core/cryptonote_core.cppsrc/cryptonote_core/cryptonote_core.hInspect captured patch +49 / −77
diff --git a/src/cryptonote_core/cryptonote_core.cpp b/src/cryptonote_core/cryptonote_core.cpp
index df9a34e..05049a9 100644
--- a/src/cryptonote_core/cryptonote_core.cpp
+++ b/src/cryptonote_core/cryptonote_core.cpp
@@ -1520,9 +1520,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, size_t limit_size) 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) const
{
- return m_mempool.get_transactions_info(epee::to_span(txids), txs, include_sensitive_txes, limit_size);
+ return m_mempool.get_transactions_info(epee::to_span(txids), txs, include_sensitive_txes);
}
//-----------------------------------------------------------------------------------------------
bool core::get_pool_transactions(std::vector<transaction>& txs, bool include_sensitive_data) const
@@ -1537,9 +1537,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, size_t limit_size) 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) const
{
- return m_mempool.get_pool_info(start_time, include_sensitive_txes, max_tx_count, added_txs, remaining_added_txids, removed_txs, incremental, limit_size);
+ return m_mempool.get_pool_info(start_time, include_sensitive_txes, max_tx_count, added_txs, remaining_added_txids, removed_txs, incremental);
}
//-----------------------------------------------------------------------------------------------
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 ed4e81f..505e52f 100644
--- a/src/cryptonote_core/cryptonote_core.h
+++ b/src/cryptonote_core/cryptonote_core.h
@@ -491,7 +491,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, size_t limit_size = 0) 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) const;
/**
* @copydoc tx_memory_pool::get_pool_info
@@ -500,7 +500,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, size_t limit_size = 0) 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) const;
/**
* @copydoc tx_memory_pool::get_transactions
diff --git a/src/cryptonote_core/tx_pool.cpp b/src/cryptonote_core/tx_pool.cpp
index 0be5107..4bc723a 100644
--- a/src/cryptonote_core/tx_pool.cpp
+++ b/src/cryptonote_core/tx_pool.cpp
@@ -665,7 +665,7 @@ namespace cryptonote
return true;
}
//------------------------------------------------------------------
- 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
+ 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 max_tx_count) const
{
CRITICAL_REGION_LOCAL(m_transactions_lock);
CRITICAL_REGION_LOCAL1(m_blockchain);
@@ -676,7 +676,6 @@ namespace cryptonote
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)
{
@@ -686,11 +685,6 @@ namespace cryptonote
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));
}
@@ -972,7 +966,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, size_t cumul_limit_size) 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) const
{
CRITICAL_REGION_LOCAL(m_transactions_lock);
CRITICAL_REGION_LOCAL1(m_blockchain);
@@ -1007,8 +1001,7 @@ namespace cryptonote
LOG_PRINT_L2("Giving back the whole pool");
// If incremental, handle removed TXIDs first since it's important that txs are removed
- // from synchronizers' pools, and we need to estimate how much space we have left to
- // request full-bodied txs
+ // from synchronizers' pools.
if (incremental)
{
std::multimap<time_t, removed_tx_info>::const_iterator rit = m_removed_txs_by_time.lower_bound(start_time);
@@ -1039,20 +1032,12 @@ namespace cryptonote
txids.push_back(pit.first);
}
- // 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)
+ // Perform TX info fetch, limited to max_tx_count.
+ if (!txids.empty())
{
if (!get_transactions_info(epee::to_span(txids),
added_txs,
include_sensitive,
- cumulative_txblob_size_limit,
max_tx_count))
return false;
}
diff --git a/src/cryptonote_core/tx_pool.h b/src/cryptonote_core/tx_pool.h
index 2cba87d..a5f86d2 100644
--- a/src/cryptonote_core/tx_pool.h
+++ b/src/cryptonote_core/tx_pool.h
@@ -503,7 +503,7 @@ namespace cryptonote
/**
* @brief get information about multiple transactions
*/
- 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;
+ 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 max_tx_count = 0) const;
/**
* @brief get transactions not in the passed set
@@ -515,7 +515,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, size_t cumul_limit_size = 0) 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) const;
private:
diff --git a/src/rpc/core_rpc_server.cpp b/src/rpc/core_rpc_server.cpp
index 2b12463..3cd1314 100644
--- a/src/rpc/core_rpc_server.cpp
+++ b/src/rpc/core_rpc_server.cpp
@@ -75,6 +75,8 @@ using namespace epee;
#define RESTRICTED_SPENT_KEY_IMAGES_COUNT 5000
#define RESTRICTED_BLOCK_COUNT 1000
+static constexpr size_t GET_BLOCKS_BIN_MAX_ADDED_POOL_TX_BODIES = 20000;
+
#define RPC_TRACKER(rpc) \
PERF_TIMER(rpc); \
RPCTracker tracker(#rpc, PERF_TIMER_NAME(rpc))
@@ -519,10 +521,8 @@ namespace cryptonote
return true;
}
//------------------------------------------------------------------------------------------------------------------------------
- bool core_rpc_server::handle_get_blocks(const COMMAND_RPC_GET_BLOCKS_FAST::request& req, COMMAND_RPC_GET_BLOCKS_FAST::response& res, const connection_context *ctx, std::size_t &cumul_block_data_size)
+ bool core_rpc_server::handle_get_blocks(const COMMAND_RPC_GET_BLOCKS_FAST::request& req, COMMAND_RPC_GET_BLOCKS_FAST::response& res, const connection_context *ctx)
{
- cumul_block_data_size = 0;
-
// quick check for noop
if (req.start_height > 0 || !req.block_ids.empty())
{
@@ -552,6 +552,7 @@ namespace cryptonote
return false;
}
+ size_t cumul_block_data_size = 0;
size_t ntxes = 0;
res.blocks.reserve(bs.size());
res.output_indices.reserve(bs.size());
@@ -647,10 +648,9 @@ namespace cryptonote
res.pool_info_extent = COMMAND_RPC_GET_BLOCKS_FAST::NONE;
- size_t cumul_block_data_size = 0;
if (get_blocks)
{
- if (!handle_get_blocks(req, res, ctx, cumul_block_data_size))
+ if (!handle_get_blocks(req, res, ctx))
{
res.status = "Failed";
return true;
@@ -662,58 +662,45 @@ namespace cryptonote
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 max_tx_count = restricted
+ ? std::min<size_t>(RESTRICTED_TRANSACTIONS_COUNT, GET_BLOCKS_BIN_MAX_ADDED_POOL_TX_BODIES)
+ : GET_BLOCKS_BIN_MAX_ADDED_POOL_TX_BODIES;
- const size_t limit = LEVIN_DEFAULT_MAX_PACKET_SIZE * 0.9;
+ 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 the blocks alone already consume the safe packet budget, skip pool info
- if (cumul_block_data_size >= limit)
+ if (!success)
{
- 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.
+ res.status = "Failed to get pool info";
+ return true;
}
- 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.added_pool_txs.clear();
+ 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 get pool info";
+ res.status = "Failed to serialize transaction";
return true;
}
-
- res.added_pool_txs.clear();
- 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;
+ 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;
diff --git a/src/rpc/core_rpc_server.h b/src/rpc/core_rpc_server.h
index 5fe669d..6ca8e2b 100644
--- a/src/rpc/core_rpc_server.h
+++ b/src/rpc/core_rpc_server.h
@@ -271,7 +271,7 @@ private:
template <typename COMMAND_TYPE>
bool use_bootstrap_daemon_if_necessary(const invoke_http_mode &mode, const std::string &command_name, const typename COMMAND_TYPE::request& req, typename COMMAND_TYPE::response& res, bool &r);
bool get_block_template(const account_public_address &address, const crypto::hash *prev_block, const cryptonote::blobdata &extra_nonce, size_t &reserved_offset, cryptonote::difficulty_type &difficulty, uint64_t &height, uint64_t &expected_reward, uint64_t& cumulative_weight, block &b, uint64_t &seed_height, crypto::hash &seed_hash, crypto::hash &next_seed_hash, epee::json_rpc::error &error_resp);
- bool handle_get_blocks(const COMMAND_RPC_GET_BLOCKS_FAST::request& req, COMMAND_RPC_GET_BLOCKS_FAST::response& res, const connection_context *ctx, std::size_t &cumul_block_data_size_out);
+ bool handle_get_blocks(const COMMAND_RPC_GET_BLOCKS_FAST::request& req, COMMAND_RPC_GET_BLOCKS_FAST::response& res, const connection_context *ctx);
core& m_core;
nodetool::node_server<cryptonote::t_cryptonote_protocol_handler<cryptonote::core> >& m_p2p;
Risk score
Our methodology →Why this scored 49/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.