[F40] bound query_state batch work: shared event budget, atomic reads - #5266
Open
bilboquet wants to merge 2 commits into
Conversation
This was
linked to
issues
Sep 14, 2026
bilboquet
marked this pull request as ready for review
September 14, 2026 16:43
bilboquet
changed the title
bilboquet
force-pushed
the
5057-f40-unbounded-query_state-batch-processing-can-cause-execution-lock-contention-and-node-dos
branch
from
September 14, 2026 18:19
cb57c02 to
7ae55c7
Compare
Leo-Besancon
left a comment
Leo-Besancon
left a comment
Member
There was a problem hiding this comment.
LGTM! Just have 1 comment inline
| self.execution_state | ||
| .read() | ||
| .get_filtered_sc_output_event(filter) | ||
| .get_filtered_sc_output_event(filter, usize::MAX) |
Member
There was a problem hiding this comment.
I know that would make a lot more changes to the interfaces, but I think it would be cleaner to have an option instead, and setting it explicitely to None in all the callers going through this path?
Member
There was a problem hiding this comment.
Unless I'm mistaken there is still usize::MAX limit here instead of None?
…wing Phase 1 of issue #5057. query_state and the other batch getters must read under a single execution_state read lock: the returned cursors and fingerprint describe the whole batch, so releasing the lock between items would silently corrupt reads while keeping a valid stamp. A comment-only invariant cannot hold that: drop + re-acquire compiles fine. Hence batch helpers (eval_query_item, *_under) take &ExecutionState borrowed from the guard: they receive a read capability for an acquisition they do not own and cannot release. A mid-batch drop is now a compile error (E0505, verified with a temporary drop test, then removed). Public trait signatures unchanged (acquire + delegate). Pure code move + reindent, no logic change. cargo check + clippy clean, crate suite green.
Implements the #5255 spec (phase 2 of #5057): bound the work per batch, never the lock. - ExecutionQueryRequest.max_event_count: Option<usize> batch budget, fed by the pre-existing [execution].max_event_per_query setting (no new knob; ApiConfig/GrpcConfig carry it to the transports). Each Events item takes min(remaining, per-item cap) and decrements what it returns; past zero it gets a per-item TooLargeResponse error (exists since #5236, no proto change) instead of fetching. None means unbounded (explicit opt-out for empty requests and tests, never for transport batches). - execution.rs get_filtered_sc_output_event takes a limit applied to the final-cache and active-history paths (take-after-fetch minimum: cost is proportional to the returned plus one truncated fetch). - Event bytes counted against max_response_size (data.len() + estimated per-event overhead const, marked ponytail: for later measurement). - Datastore keys counted against max_response_size (sum of key lengths). - Single-shot get_filtered_sc_output_event unchanged (explicit unbounded, marked ponytail: as known follow-up). - New tests: two Events items sharing a budget of 1 (first takes it, second errors); scan tripwire documenting the unbounded count=None hole left while #5189 is unmerged (breaks on purpose when it lands). - Observable behavior change (new per-item errors where success before): F42 breaking-changes list, same as #5189.
bilboquet
force-pushed
the
5057-f40-unbounded-query_state-batch-processing-can-cause-execution-lock-contention-and-node-dos
branch
from
September 15, 2026 09:26
5f11cca to
68fdd5b
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Attack
query_stateholdsexecution_state.read()for the whole batch (up to 128 items viagRPC
max_query_items_per_request).Eventsitems had no batch-wide bound: 7000 eventsper item on the final-cache path plus a completely uncapped active-history path
(
flat_map().collect()), i.e. up to ~896k events deserialized, collected and returnedunder a single read lock from one remote batch.
parking_lot::RwLockis fair, so theexecution writer (final/candidate slots) stalls behind the heaviest batch in flight.
Remote-triggerable node slowdown / stall with attacker-controlled gas upstream.
Fix (2 commits)
1. Atomic batch reads — Closes #5268 (F40 point 1, stronger than asked).
The spec asked for a comment; the PR enforces it by typing instead: batch helpers
(
eval_query_item,*_under) take&ExecutionStateborrowed from the guard, soreleasing the lock mid-batch is a compile error (E0505, verified), not a silent
corruption. Public trait signatures unchanged. Pure move, no behavior change.
2. Shared event budget — Closes #5255.
ExecutionQueryRequest.max_event_count, fed by newApiConfig/GrpcConfigmax_events_per_querysettings (default 7000): eachEventsitem takesmin(remaining, per-item cap)and decrements what it returns; past zero it gets aper-item
TooLargeResponseerror (exists since #5236, no proto change) instead offetching. The limit is applied to the final-cache and active-history paths
(take-after-fetch: cost proportional to the returned plus one truncated fetch).
Event bytes (
data.len()+ estimated 128 B overhead, marked for later measurement)and datastore-key bytes (sum of key lengths) now count against
max_response_size.Single-shot event queries are unchanged.
Left intentionally out: early-stop iterators (documented optimization), per-request
key-count forwarding (still #5189),
get_addresses_infoscap and slot deadline(phases 3-4 of #5057).
Why no MIP
Node-local API behavior only. Block validity rules, serialization, hashing, execution
semantics and PoS draws are untouched. Observable change: batches that used to return
up to ~896k events now get per-item errors past the budget — recorded on the F42
breaking-changes list for builders (same list as #5189).
Tests
test_query_state_events_budget_shared: two identicalEventsitems, budget of 1 —first takes it, second errors.
test_scan_datastore_count_none_is_unbounded: tripwire documenting the unboundedcount=Noneper-item scan left while [F42] propagate the configured datastore key query cap from RPC/gRPC … #5189 is unmerged (breaks on purpose when it lands).datastore_manipulations(has anEventsitem) green unchanged.cargo check --workspace,clippyclean.Closes #5255
Closes #5268
Relates #5057