mirror of
https://github.com/block/buzz.git
synced 2026-10-27 04:30:52 +00:00
fix(relay): align NIP-11 max_limit with REQ ceiling (#3635)
Buzz's NIP-11 document advertised `limitation.max_limit: 10_000`, but the effective websocket REQ page ceiling was `1_000` — a 10x lie. The websocket REQ path never sets `EventQuery::max_limit`, so `query_events` applied its own `unwrap_or(1000)` clamp to every historical query. Only the COUNT fallback (`apply_count_fallback_limit`) ever raises that clamp. A client that trusts the advertised value asks for 10,000 events, silently receives 1,000, and — with no error and no continuation signal — reads that short page as exhaustion. Up to 9,000 events are dropped without anyone noticing. `MAX_HISTORICAL_LIMIT = 2_000` in `handlers/req.rs` was dead weight for the same reason: nothing clamped to 2,000 could survive the DB's 1,000 clamp one layer down. ## Change `buzz_db::DEFAULT_MAX_PAGE_LIMIT` (`1_000`) is now the single source of truth. It is the `query_events` clamp default, the value both REQ clamp sites use, and the value advertised as NIP-11 `max_limit`. `MAX_HISTORICAL_LIMIT` is removed rather than re-pointed — an alias for a constant used four lines away adds a name without adding meaning. The NIP-50 search path carries a second, independent bound. It clamps its emission target to the shared ceiling like any other REQ, but how many FTS candidates it will scan was bounded separately, by a bare 10-page loop over 100-hit pages. That product only coincidentally equalled the ceiling, so raising the ceiling — or shrinking a page — would shrink the scan relative to what clients may now request, degrading search quality while nothing in the code registered the change. The page count is now ceiling-divided from `DEFAULT_MAX_PAGE_LIMIT` over a named `SEARCH_PAGE_SIZE`, so the scan budget tracks the advertised ceiling by construction. That budget is a resource policy, not a delivery promise. It bounds candidates *scanned*, not events *emitted*: post-filtering (NIP-01 match, channel access, reader visibility, dedup) discards an unpredictable share of every page, so a search result smaller than the requested limit remains possible. This is not a NIP-11 violation — `max_limit` is defined as a clamp the relay applies to a requested `limit`, not a guaranteed count in the response. Two guards hold the pair together: - `req_filter_limit_clamps_to_advertised_nip11_max_limit` reads `max_limit` back out of a built `RelayInfo` and asserts the REQ path clamps to exactly that number. - `search_scan_capacity_covers_advertised_nip11_max_limit` asserts the scan budget covers exactly one advertised ceiling's worth of candidates — no less, and with no spare page of slack, so the derivation can't be quietly replaced by a hand-tuned constant that happens to pass today. ## Behavior Websocket behavior is unchanged: 1,000 was already the real ceiling on every path, including NIP-50. The advertisement now tells the truth about it. Raising the effective limit is a capacity decision and is deliberately not made here. The generic HTTP bridge's page-2+ offsets do change, as a consequence of the corrected clamp. `extract_page_offset` sizes a page from `query.limit` *before* the DB clamp applies, so an absent limit previously produced an offset of 2,000 and a requested 1,500 produced 1,500 — while the page actually returned held at most 1,000 rows. Both now produce 1,000. This corrects paging that had been skipping rows the previous page never returned; `extract_page_offset_sizes_pages_from_clamped_limit` locks it down. ## Scope note The bridge's per-endpoint ceilings — `BRIDGE_WINDOW_MAX_LIMIT` (200) for channel windows and `BRIDGE_THREAD_MAX_LIMIT` (500) for thread reads — are endpoint contracts on a non-NIP-01 transport, not values NIP-11 speaks for, and are unchanged. Fixes #3757 --------- Signed-off-by: Will Pfleger <pfleger.will@gmail.com> Co-authored-by: Duncan <dcfd242e557282d7a1e2cf2e6877522682f1e5c6156dc92ca7d90eaedd3b0f95@buzz.block.builderlab.xyz>
This commit is contained in:
@@ -17,6 +17,13 @@ use buzz_core::{CommunityId, StoredEvent};
|
||||
|
||||
use crate::error::{DbError, Result};
|
||||
|
||||
/// Largest page [`query_events`] will return when [`EventQuery::max_limit`] is
|
||||
/// unset — the effective ceiling on any client-requested `limit`.
|
||||
///
|
||||
/// This is the value the relay advertises as NIP-11 `limitation.max_limit`, so
|
||||
/// the advertised ceiling and the enforced one cannot drift.
|
||||
pub const DEFAULT_MAX_PAGE_LIMIT: i64 = 1_000;
|
||||
|
||||
/// Optional filters for [`query_events`].
|
||||
#[derive(Debug, Clone)]
|
||||
pub struct EventQuery {
|
||||
@@ -67,9 +74,9 @@ pub struct EventQuery {
|
||||
/// channel-less global events. Applied before SQL `LIMIT` so access-filtered
|
||||
/// historical pages have exact exhaustion semantics.
|
||||
pub channel_ids: Option<Vec<uuid::Uuid>>,
|
||||
/// Override the default limit clamp (1000). Used by COUNT fallback path
|
||||
/// which needs to fetch all matching events for post-filter counting.
|
||||
/// When None, the default clamp of 1000 applies.
|
||||
/// Override the default page clamp ([`DEFAULT_MAX_PAGE_LIMIT`]). Used by
|
||||
/// the COUNT fallback path, which needs to fetch all matching events for
|
||||
/// post-filter counting. When None, the default clamp applies.
|
||||
pub max_limit: Option<i64>,
|
||||
/// Shared-gated visibility reader: when set, append an SQL visibility
|
||||
/// clause for every kind in [`SHARED_GATED_KINDS`] before ORDER/LIMIT so
|
||||
@@ -357,7 +364,7 @@ pub(crate) async fn query_events_on(
|
||||
return Ok(vec![]);
|
||||
}
|
||||
|
||||
let clamp = q.max_limit.unwrap_or(1000);
|
||||
let clamp = q.max_limit.unwrap_or(DEFAULT_MAX_PAGE_LIMIT);
|
||||
let limit_val = q.limit.unwrap_or(100).min(clamp);
|
||||
let offset_val = q.offset.unwrap_or(0);
|
||||
|
||||
|
||||
@@ -55,7 +55,7 @@ pub mod user;
|
||||
pub mod workflow;
|
||||
|
||||
pub use error::{DbError, Result};
|
||||
pub use event::{EventQuery, ReactionEventInsertOutcome};
|
||||
pub use event::{EventQuery, ReactionEventInsertOutcome, DEFAULT_MAX_PAGE_LIMIT};
|
||||
|
||||
use chrono::{DateTime, Utc};
|
||||
use sqlx::postgres::{PgConnection, PgPoolOptions};
|
||||
|
||||
@@ -3042,6 +3042,27 @@ mod tests {
|
||||
assert_eq!(extract_page_offset(&raw, None), None);
|
||||
}
|
||||
|
||||
/// Offsets are sized from the *clamped* limit the DB will honor, not from
|
||||
/// what the client asked for. `filter_to_query_params` clamps an absent or
|
||||
/// over-ceiling `limit` to `DEFAULT_MAX_PAGE_LIMIT` (guarded in
|
||||
/// `handlers::req::tests::req_filter_limit_clamps_to_advertised_nip11_max_limit`)
|
||||
/// and that clamped value is what arrives here — so page N starts exactly
|
||||
/// N-1 full pages in. Sizing from an unclamped limit would step past rows
|
||||
/// the previous page never returned.
|
||||
#[test]
|
||||
fn extract_page_offset_sizes_pages_from_clamped_limit() {
|
||||
let clamped = buzz_db::DEFAULT_MAX_PAGE_LIMIT;
|
||||
|
||||
assert_eq!(
|
||||
extract_page_offset(&serde_json::json!({ "page": 2 }), Some(clamped)),
|
||||
Some(clamped)
|
||||
);
|
||||
assert_eq!(
|
||||
extract_page_offset(&serde_json::json!({ "page": 3 }), Some(clamped)),
|
||||
Some(clamped * 2)
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn extract_depth_limit_valid() {
|
||||
let raw = serde_json::json!({ "depth_limit": 3 });
|
||||
|
||||
@@ -22,7 +22,6 @@ use crate::connection::{AuthState, ConnectionState};
|
||||
use crate::protocol::RelayMessage;
|
||||
use crate::state::AppState;
|
||||
|
||||
const MAX_HISTORICAL_LIMIT: i64 = 2_000;
|
||||
const MAX_SUBSCRIPTIONS: usize = 1024;
|
||||
|
||||
/// Maximum `query_events` calls in flight per multi-filter REQ / bridge query.
|
||||
@@ -416,10 +415,24 @@ pub async fn handle_req(
|
||||
);
|
||||
}
|
||||
|
||||
/// Handle a NIP-50 search REQ: query Postgres FTS, fetch full events, deliver results, EOSE.
|
||||
/// Search subscriptions are one-shot — no persistent subscription is registered.
|
||||
/// FTS candidate hits fetched per page. Pages are always full regardless of
|
||||
/// the requested limit — post-filtering discards an unpredictable share of
|
||||
/// hits, so the scan fetches candidates in full pages rather than sizing
|
||||
/// pages to the request.
|
||||
const SEARCH_PAGE_SIZE: u32 = 100;
|
||||
|
||||
/// Maximum FTS pages to fetch per filter (prevents unbounded loops).
|
||||
const MAX_SEARCH_PAGES: u32 = 10;
|
||||
///
|
||||
/// Derived from the advertised page ceiling rather than fixed: the scan
|
||||
/// budget is a resource policy — at most one advertised page ceiling's worth
|
||||
/// of candidates per filter — and deriving it keeps the budget tracking the
|
||||
/// ceiling if the ceiling ever moves. This bounds candidates *scanned*, not
|
||||
/// events *emitted*: post-filtering (NIP-01 match, channel access, reader
|
||||
/// visibility, dedup) can discard any number of candidates, so a result
|
||||
/// smaller than the requested limit remains possible and is not a NIP-11
|
||||
/// violation — `max_limit` promises a clamp on the request, not a count in
|
||||
/// the response.
|
||||
const MAX_SEARCH_PAGES: u32 = (buzz_db::DEFAULT_MAX_PAGE_LIMIT as u32).div_ceil(SEARCH_PAGE_SIZE);
|
||||
|
||||
/// Resolve request-local channel access, repairing a stale cache-negative.
|
||||
///
|
||||
@@ -501,6 +514,8 @@ pub(crate) fn build_search_channel_scope_filter(
|
||||
})
|
||||
}
|
||||
|
||||
/// Handle a NIP-50 search REQ: query Postgres FTS, fetch full events, deliver results, EOSE.
|
||||
/// Search subscriptions are one-shot — no persistent subscription is registered.
|
||||
#[allow(clippy::too_many_arguments)]
|
||||
async fn handle_search_req(
|
||||
sub_id: &str,
|
||||
@@ -535,8 +550,8 @@ async fn handle_search_req(
|
||||
|
||||
let limit = filter
|
||||
.limit
|
||||
.map(|l| (l as u32).min(MAX_HISTORICAL_LIMIT as u32))
|
||||
.unwrap_or(MAX_HISTORICAL_LIMIT as u32);
|
||||
.map(|l| (l as u32).min(buzz_db::DEFAULT_MAX_PAGE_LIMIT as u32))
|
||||
.unwrap_or(buzz_db::DEFAULT_MAX_PAGE_LIMIT as u32);
|
||||
|
||||
if limit == 0 {
|
||||
continue; // NIP-01: limit 0 means "no results from this filter"
|
||||
@@ -583,13 +598,11 @@ async fn handle_search_req(
|
||||
let since = filter.since.map(|s| s.as_secs() as i64);
|
||||
let until = filter.until.map(|u| u.as_secs() as i64);
|
||||
|
||||
// Paginate: keep fetching pages until we've emitted `limit` results
|
||||
// or exhausted the search result set. This ensures post-filtering
|
||||
// doesn't silently reduce the result count below the requested limit.
|
||||
// Paginate: keep fetching pages until we've emitted `limit` results or
|
||||
// exhausted the search result set. Post-filtering discards an unpredictable
|
||||
// share of each page, so continuing past short yields gives the scan a
|
||||
// chance — not a guarantee — of filling the requested limit.
|
||||
let mut emitted: u32 = 0;
|
||||
// Always fetch full pages (100) regardless of limit — post-filtering
|
||||
// may discard many hits, so we need headroom to fill the requested limit.
|
||||
let per_page: u32 = 100;
|
||||
|
||||
for page in 1..=MAX_SEARCH_PAGES {
|
||||
if emitted >= limit {
|
||||
@@ -605,7 +618,7 @@ async fn handle_search_req(
|
||||
since,
|
||||
until,
|
||||
page,
|
||||
per_page,
|
||||
per_page: SEARCH_PAGE_SIZE,
|
||||
mode: buzz_search::SearchMode::FullText,
|
||||
};
|
||||
|
||||
@@ -617,9 +630,9 @@ async fn handle_search_req(
|
||||
}
|
||||
};
|
||||
|
||||
// A short page is the last page: FTS returns up to `per_page` hits,
|
||||
// so fewer than that means the result set is exhausted.
|
||||
let exhausted = search_result.hits.len() < per_page as usize;
|
||||
// A short page is the last page: FTS returns up to a full page of
|
||||
// hits, so fewer than that means the result set is exhausted.
|
||||
let exhausted = search_result.hits.len() < SEARCH_PAGE_SIZE as usize;
|
||||
let page_empty = search_result.hits.is_empty();
|
||||
|
||||
let hit_ids: Vec<[u8; 32]> =
|
||||
@@ -878,8 +891,8 @@ fn filter_to_query_params(
|
||||
.and_then(|u| chrono::DateTime::from_timestamp(u.as_secs() as i64, 0));
|
||||
let limit = filter
|
||||
.limit
|
||||
.map(|l| (l as i64).min(MAX_HISTORICAL_LIMIT))
|
||||
.unwrap_or(MAX_HISTORICAL_LIMIT);
|
||||
.map(|l| (l as i64).min(buzz_db::DEFAULT_MAX_PAGE_LIMIT))
|
||||
.unwrap_or(buzz_db::DEFAULT_MAX_PAGE_LIMIT);
|
||||
|
||||
// Push author filter into SQL. Single-author uses the indexed `pubkey` column;
|
||||
// multi-author uses the `authors` IN-list pushdown added in the pure-nostr PR.
|
||||
@@ -1418,6 +1431,83 @@ mod tests {
|
||||
)
|
||||
}
|
||||
|
||||
/// NIP-11 `limitation.max_limit` as this relay actually advertises it.
|
||||
fn advertised_max_limit() -> i64 {
|
||||
crate::nip11::RelayInfo::build(
|
||||
None,
|
||||
None,
|
||||
false,
|
||||
crate::config::DEFAULT_MAX_FRAME_BYTES,
|
||||
None,
|
||||
)
|
||||
.limitation
|
||||
.expect("limitation")
|
||||
.max_limit
|
||||
.expect("max_limit") as i64
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn req_filter_limit_clamps_to_advertised_nip11_max_limit() {
|
||||
let advertised = advertised_max_limit();
|
||||
|
||||
let community = buzz_core::tenant::CommunityId::from_uuid(uuid::Uuid::new_v4());
|
||||
|
||||
// A filter asking for more than the relay advertises is clamped down to
|
||||
// exactly the advertised ceiling — the NIP-11 document is the promise,
|
||||
// this is the enforcement.
|
||||
let greedy = filter_to_query_params(
|
||||
&Filter::new().limit(advertised as usize * 10),
|
||||
None,
|
||||
community,
|
||||
);
|
||||
assert_eq!(greedy.limit, Some(advertised));
|
||||
|
||||
// A filter with no `limit` gets the same ceiling, not something larger.
|
||||
let unbounded = filter_to_query_params(&Filter::new(), None, community);
|
||||
assert_eq!(unbounded.limit, Some(advertised));
|
||||
|
||||
// Neither sets `max_limit`, so `query_events` applies its own default
|
||||
// clamp. That default must equal the advertised value too, or the
|
||||
// clamp above would be undone one layer down.
|
||||
assert_eq!(greedy.max_limit, None);
|
||||
assert_eq!(unbounded.max_limit, None);
|
||||
assert_eq!(buzz_db::DEFAULT_MAX_PAGE_LIMIT, advertised);
|
||||
|
||||
// Under-ceiling requests are honored verbatim.
|
||||
let modest = filter_to_query_params(&Filter::new().limit(10), None, community);
|
||||
assert_eq!(modest.limit, Some(10));
|
||||
}
|
||||
|
||||
/// The NIP-50 search path clamps its emission target to the advertised
|
||||
/// ceiling like every other REQ, but the number of candidates it will scan
|
||||
/// is bounded a second time by the page budget. This pins the resource
|
||||
/// policy: the budget covers exactly one advertised page ceiling's worth of
|
||||
/// candidates — no less (a ceiling raise must not silently shrink the scan
|
||||
/// relative to what clients may request) and no hand-tuned spare (the budget
|
||||
/// must stay derived, not drift back into a magic number). It deliberately
|
||||
/// does NOT claim search fills the emitted limit — post-filtering can
|
||||
/// discard any number of candidates.
|
||||
#[test]
|
||||
fn search_scan_capacity_covers_advertised_nip11_max_limit() {
|
||||
let advertised = advertised_max_limit();
|
||||
let capacity = i64::from(MAX_SEARCH_PAGES) * i64::from(SEARCH_PAGE_SIZE);
|
||||
|
||||
assert!(
|
||||
capacity >= advertised,
|
||||
"NIP-50 scans at most {capacity} candidates ({MAX_SEARCH_PAGES} pages of \
|
||||
{SEARCH_PAGE_SIZE}) but NIP-11 advertises {advertised} — the scan budget \
|
||||
no longer covers the advertised ceiling"
|
||||
);
|
||||
|
||||
// The budget is derived, not hand-tuned: one page under the derived
|
||||
// count must be insufficient, or the ceiling could rise without the
|
||||
// page count following it.
|
||||
assert!(
|
||||
capacity - i64::from(SEARCH_PAGE_SIZE) < advertised,
|
||||
"scan budget has a spare page of slack — derive it from the ceiling"
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn count_fallback_fetches_one_extra_candidate() {
|
||||
let mut query =
|
||||
|
||||
@@ -89,6 +89,11 @@ pub struct RelayLimitation {
|
||||
|
||||
/// Canonical `RelayLimitation` advertised by this relay.
|
||||
///
|
||||
/// `max_limit` is [`buzz_db::DEFAULT_MAX_PAGE_LIMIT`], the same constant the
|
||||
/// REQ path clamps filter limits to, so the advertised ceiling and the
|
||||
/// enforced one cannot drift (see
|
||||
/// `handlers::req::tests::req_filter_limit_clamps_to_advertised_nip11_max_limit`).
|
||||
///
|
||||
/// `auth_required` is always `true`: the REQ, EVENT, and COUNT handlers
|
||||
/// unconditionally reject connections that are not in
|
||||
/// `AuthState::Authenticated`. This is independent of the REST API token
|
||||
@@ -103,7 +108,7 @@ fn relay_limitation(max_message_length: usize) -> RelayLimitation {
|
||||
max_message_length: Some(max_message_length as u64),
|
||||
max_subscriptions: Some(1024),
|
||||
max_filters: Some(10),
|
||||
max_limit: Some(10_000),
|
||||
max_limit: Some(buzz_db::DEFAULT_MAX_PAGE_LIMIT as u32),
|
||||
max_subid_length: Some(256),
|
||||
min_pow_difficulty: None,
|
||||
auth_required: true,
|
||||
|
||||
Reference in New Issue
Block a user