Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion crates/rpc/src/server/api.rs
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ use miden_node_utils::limiter::{
QueryParamNoteTagLimit,
QueryParamNullifierPrefixLimit,
QueryParamStorageMapKeyTotalLimit,
QueryParamStorageMapSlotLimit,
};
use miden_node_utils::lru_cache::LruCache;
use miden_node_utils::retry::{self, Retryable};
Expand Down Expand Up @@ -386,6 +387,7 @@ static RPC_LIMITS: LazyLock<proto::rpc::RpcLimits> = LazyLock::new(|| {
use QueryParamNoteTagLimit as NoteTag;
use QueryParamNullifierPrefixLimit as NullifierPrefix;
use QueryParamStorageMapKeyTotalLimit as StorageMapKeyTotal;
use QueryParamStorageMapSlotLimit as StorageMapSlot;

proto::rpc::RpcLimits {
endpoints: std::collections::HashMap::from([
Expand All @@ -401,7 +403,10 @@ static RPC_LIMITS: LazyLock<proto::rpc::RpcLimits> = LazyLock::new(|| {
("GetNotesById".into(), endpoint_limits(&[(NoteId::PARAM_NAME, NoteId::LIMIT)])),
(
"GetAccount".into(),
endpoint_limits(&[(StorageMapKeyTotal::PARAM_NAME, StorageMapKeyTotal::LIMIT)]),
endpoint_limits(&[
(StorageMapKeyTotal::PARAM_NAME, StorageMapKeyTotal::LIMIT),
(StorageMapSlot::PARAM_NAME, StorageMapSlot::LIMIT),
]),
),
]),
}
Expand Down
129 changes: 116 additions & 13 deletions crates/rpc/src/server/api/get_account.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
use std::collections::HashSet;

use miden_node_proto::domain::account::{
AccountRequest,
AccountResponse,
Expand All @@ -6,7 +8,7 @@ use miden_node_proto::domain::account::{
};
use miden_node_proto::generated as proto;
use miden_node_store::GetAccountError;
use miden_node_utils::limiter::QueryParamStorageMapKeyTotalLimit;
use miden_node_utils::limiter::{QueryParamStorageMapKeyTotalLimit, QueryParamStorageMapSlotLimit};
use miden_node_utils::tracing::OpenTelemetrySpanExt;
use tonic::Status;
use tracing::{Span, debug, info_span};
Expand Down Expand Up @@ -35,20 +37,10 @@ impl proto::server::rpc_api::GetAccount for RpcService {
span.set_attribute("block.number", block);
}

// Validate total storage map key limit before forwarding to store
// Validate storage map request limits before forwarding to store.
if let Some(details) = &request.details {
let _span = info_span!(target: COMPONENT, "validate_storage_map_keys").entered();
let total_keys: usize = match &details.storage_request {
AccountStorageRequest::None | AccountStorageRequest::AllStorageMaps => 0,
AccountStorageRequest::Explicit(requests) => requests
.iter()
.filter_map(|request| match &request.slot_data {
SlotData::All => None,
SlotData::MapKeys(items) => Some(items.len()),
})
.sum(),
};
check::<QueryParamStorageMapKeyTotalLimit>(total_keys)?;
validate_storage_request(&details.storage_request)?;
}

let account_data =
Expand All @@ -60,6 +52,46 @@ impl proto::server::rpc_api::GetAccount for RpcService {
// HELPERS
// ================================================================================================

/// Validates the storage map request limits before forwarding it to the store.
///
/// Only the [`AccountStorageRequest::Explicit`] variant carries a user-controlled list of
/// slots. [`AccountStorageRequest::AllStorageMaps`] is expanded by the store from the account's
/// actual storage layout (and bounded by a response budget), so it needs no validation here.
fn validate_storage_request(storage_request: &AccountStorageRequest) -> Result<(), Status> {
let AccountStorageRequest::Explicit(requests) = storage_request else {
return Ok(());
};

// Bound the number of requested slots. `all-entries` requests are not counted by the per-key
// limit below, so without this an unbounded (or duplicated) list of `all-entries` slots could
// force the store into arbitrarily many forest lookups and database reconstructions, a
// denial-of-service vector.
check::<QueryParamStorageMapSlotLimit>(requests.len())?;

// Reject duplicate slots: requesting the same slot more than once is redundant and would
// otherwise multiply the store-side work for that slot.
let mut seen = HashSet::with_capacity(requests.len());
for request in requests {
if !seen.insert(&request.slot_name) {
return Err(Status::invalid_argument(format!(
"duplicate storage map slot in request: {}",
request.slot_name
)));
}
}

let total_keys: usize = requests
.iter()
.filter_map(|request| match &request.slot_data {
SlotData::All => None,
SlotData::MapKeys(items) => Some(items.len()),
})
.sum();
check::<QueryParamStorageMapKeyTotalLimit>(total_keys)?;

Ok(())
}

fn get_account_error_to_status(err: GetAccountError) -> Status {
let message = err.to_string();
match err {
Expand All @@ -71,3 +103,74 @@ fn get_account_error_to_status(err: GetAccountError) -> Status {
| GetAccountError::BlockPruned(_) => Status::invalid_argument(message),
}
}

// TESTS
// ================================================================================================

#[cfg(test)]
mod tests {
use miden_node_proto::domain::account::StorageMapRequest;
use miden_node_utils::limiter::QueryParamLimiter;
use miden_protocol::account::{StorageMapKey, StorageSlotName};
use tonic::Code;

use super::*;

fn slot_request(name: &str, slot_data: SlotData) -> StorageMapRequest {
StorageMapRequest {
slot_name: StorageSlotName::new(name).unwrap(),
slot_data,
}
}

#[test]
fn none_and_all_storage_maps_requests_are_always_valid() {
validate_storage_request(&AccountStorageRequest::None).unwrap();
validate_storage_request(&AccountStorageRequest::AllStorageMaps).unwrap();
}

#[test]
fn explicit_request_within_limits_is_valid() {
let requests = vec![
slot_request("a::0", SlotData::All),
slot_request("a::1", SlotData::MapKeys(vec![StorageMapKey::from_index(1)])),
];

validate_storage_request(&AccountStorageRequest::Explicit(requests)).unwrap();
}

#[test]
fn too_many_all_entries_slots_are_rejected() {
// `all-entries` requests are not counted by the per-key limit, so this must be caught by
// the per-slot limit.
let requests = (0..=QueryParamStorageMapSlotLimit::LIMIT)
.map(|index| slot_request(&format!("a::{index}"), SlotData::All))
.collect();

let status = validate_storage_request(&AccountStorageRequest::Explicit(requests))
.expect_err("request exceeding the slot limit must be rejected");
assert_eq!(status.code(), Code::OutOfRange);
}

#[test]
fn duplicate_slots_are_rejected() {
let requests =
vec![slot_request("a::0", SlotData::All), slot_request("a::0", SlotData::All)];

let status = validate_storage_request(&AccountStorageRequest::Explicit(requests))
.expect_err("duplicate slot must be rejected");
assert_eq!(status.code(), Code::InvalidArgument);
}

#[test]
fn too_many_map_keys_are_rejected() {
let keys: Vec<_> = (0..=QueryParamStorageMapKeyTotalLimit::LIMIT)
.map(|index| StorageMapKey::from_index(index as u32))
.collect();
let requests = vec![slot_request("a::0", SlotData::MapKeys(keys))];

let status = validate_storage_request(&AccountStorageRequest::Explicit(requests))
.expect_err("request exceeding the key limit must be rejected");
assert_eq!(status.code(), Code::OutOfRange);
}
}
19 changes: 19 additions & 0 deletions crates/rpc/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,8 @@ use miden_node_utils::limiter::{
QueryParamNoteIdLimit,
QueryParamNoteTagLimit,
QueryParamNullifierPrefixLimit,
QueryParamStorageMapKeyTotalLimit,
QueryParamStorageMapSlotLimit,
};
use miden_protocol::Word;
use miden_protocol::account::delta::AccountUpdateDetails;
Expand Down Expand Up @@ -869,6 +871,23 @@ async fn get_limits_endpoint() {
QueryParamNoteIdLimit::PARAM_NAME,
QueryParamNoteIdLimit::LIMIT
);

// Verify GetAccount endpoint advertises both the per-key and per-slot storage map limits.
let get_account = limits.endpoints.get("GetAccount").expect("GetAccount should exist");
assert_eq!(
get_account.parameters.get(QueryParamStorageMapKeyTotalLimit::PARAM_NAME),
Some(&(QueryParamStorageMapKeyTotalLimit::LIMIT as u32)),
"GetAccount {} limit should be {}",
QueryParamStorageMapKeyTotalLimit::PARAM_NAME,
QueryParamStorageMapKeyTotalLimit::LIMIT
);
assert_eq!(
get_account.parameters.get(QueryParamStorageMapSlotLimit::PARAM_NAME),
Some(&(QueryParamStorageMapSlotLimit::LIMIT as u32)),
"GetAccount {} limit should be {}",
QueryParamStorageMapSlotLimit::PARAM_NAME,
QueryParamStorageMapSlotLimit::LIMIT
);
}

#[tokio::test]
Expand Down
15 changes: 15 additions & 0 deletions crates/utils/src/limiter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -118,3 +118,18 @@ impl QueryParamLimiter for QueryParamStorageMapKeyTotalLimit {
const PARAM_NAME: &str = "storage_map_key";
const LIMIT: usize = 64;
}

/// Used for the following RPC endpoints
/// * `get_account`
///
/// Capped at the maximum number of storage slots an account can have
/// ([`AccountStorage::MAX_NUM_STORAGE_SLOTS`]). An explicit storage request can never legitimately
/// reference more distinct slots than that, so this bounds the per-request work (forest lookups and
/// database reconstructions) regardless of how many `all-entries` slots are requested. Without this
/// cap, `all-entries` requests are otherwise uncounted by
/// [`QueryParamStorageMapKeyTotalLimit`] and could be repeated without bound.
pub struct QueryParamStorageMapSlotLimit;
impl QueryParamLimiter for QueryParamStorageMapSlotLimit {
const PARAM_NAME: &str = "storage_maps";
const LIMIT: usize = miden_protocol::account::AccountStorage::MAX_NUM_STORAGE_SLOTS;
}
Loading