Conversation
There was a problem hiding this comment.
Implementation looks good to me. @peterjah you know more about existing projects than me, even if this is the expected behaviour, do you think it could break any apps that rely on the no limit behaviour?
I assume if that's the case the fix is just to chunk the queries, probably easy to do?
Not that it's an issue, we just have to make a clear breaking changes list for builders like we've done before.
Actually yes its a breaking change, and quite dangerous one because it breaks "silently", just truncating the 500th first elements... |
I agree it's a bit scary. Any maintained project should be able to update easily, but any unmaintained project using this behaviour may break. WDYT @damip? |
|
I think it is worth the risk given the risk of DDoS.
|
damip
left a comment
damip
left a comment
There was a problem hiding this comment.
Review (F42 / #5059)
The fix is correct and consensus-safe for what it covers, and CI is fully green. I traced the wiring end to end: cleanup_datastore_key_range_query already computed the effective count (count.or(max_datastore_query_config)) for validation but discarded it, so JSON-RPC and gRPC forwarded the caller's None into scan_datastore for an unbounded enumeration. The PR propagates the effective value; both call sites pass the configured cap (max_datastore_keys_query = 500 by default for API and gRPC), so a count: None request is now clamped to 500. A request with count > max already errored before (pre-existing check, unchanged). The smart-contract path stays byte-identical (see inline), which is required on main.
Requesting changes for one process item and one documentation item — the code itself looks good:
-
This PR only fixes one of the three problems in #5059, but it is linked to auto-close it on merge. F42 also covers (a) the smart-contract path (
Context::get_keyshardcodes no limit, so SCs still reachscan_datastorewithcount = None), (b) thescan_datastoreinternals (theSpeculativeResetType::Setbranch still.collect()s the full matching speculative key set before truncation, and the merge path can over-fetch final-state batches), and (c) work-based pricing. Those are execution-semantics changes that need a versioned rollout ondev_breaking— out of scope here, rightly so. Please either unlink the issue from this PR or open a follow-up issue for the consensus-gated residuals before merging, so they don't silently disappear when GitHub closes #5059. -
Silent truncation is a client-visible semantics change and should be documented.
count: Noneused to mean "return all keys"; it now means "return up to the configured max (default 500)" with no truncation indicator in the response. This is exactly the error-vs-silent-drop interface decision that routed F42 to an issue. Silent clamping is defensible since pagination exists (start_key+ inclusive flags), but it needs an explicit changelog / API-docs note telling client authors to paginate — otherwise existing clients that relied on "None = everything" will silently see incomplete key sets.
Minor (no change needed, just noting): if an operator unsets max_datastore_keys_query, the fallback is None and the path is unbounded again — worth one line in the config comment saying the DoS bound relies on this setting.
| @@ -660,7 +660,7 @@ impl ExecutionContext { | |||
| let max_datastore_query = None; | |||
There was a problem hiding this comment.
Verified: keeping max_datastore_query = None here makes the new destructured count equal to count.or(None) — an identity — so the smart-contract path's behavior is byte-identical to before. That's the right call for a PR targeting main (bounding the ABI would change execution outcomes and needs a deterministic constant + versioned rollout, as the TODO says).
This is also the residual half of #5059: smart contracts can still trigger unbounded enumeration through this path, including the full speculative-key .collect() inside scan_datastore. Please make sure this residual is tracked (follow-up issue or unlink #5059 from this PR) since merging will auto-close the issue.
| ) -> Result<(Vec<u8>, Bound<Vec<u8>>, Bound<Vec<u8>>), ModelsError> { | ||
| ) -> Result<(Vec<u8>, Bound<Vec<u8>>, Bound<Vec<u8>>, Option<u32>), ModelsError> { | ||
| // check item count | ||
| let count = count.or(max_datastore_query_config); |
There was a problem hiding this comment.
This fallback is what makes the fix work — and it's also where the client-visible semantics change happens: count: None previously meant "return all keys" and now means "return up to max_datastore_query_config (default 500)", with no truncation indicator in the response. Clients that relied on the old behavior will silently get partial results.
Suggest (a) a changelog/API-docs note telling client authors to paginate via start_key/inclusive flags, and (b) mentioning in the function doc that when the operator config is None the returned count can still be None (unbounded), so the DoS bound relies on the config being set.
i will check |
…parsing to the scan
|
@damip @Leo-Besancon |
4286b53 to
d741b44
Compare
|
I also prepared a doc PR in massa docs |
… the requested key count
Leo-Besancon
left a comment
Leo-Besancon
left a comment
There was a problem hiding this comment.
LGTM on this PR and the followup issues you created, but I'd be more confortable with a second opinion as it's quite impactful, so don't merge this yet please :).
damip
left a comment
damip
left a comment
There was a problem hiding this comment.
Checked the new commit (b896454) since it touches scan_datastore which is shared with the SC path: on main this is only OK if the output is byte-identical. It is. The Set bounding is correct (among the first count + |updates| entry keys at most |updates| can be overridden, so the count-th surviving key is always within that bound, and updates still win via or_insert). With count = None the walk is unbounded so the SC path is unchanged. The replenish change is also output-preserving (sequential merge, refill happens as soon as the queue empties, .max(1) avoids a 0-size fetch being read as exhaustion). The prefix-of-unbounded test is the right property. Two nits inline.
@bilboquet #5241 and #5228 are follow-ups, not blockers. #5241 is consensus-observable and goes through a MIP on dev_breaking, that's why it was split out. #5228 is a nicety: massa-web3 already sends count = 500 explicitly so it already errors on a node with a lower cap, this PR doesn't change that, and everyone runs the default anyway.
@peterjah before merging massalabs/docs#461, double-check the "MAIN.6.0 / DEVN.31.0" claim matches the release this actually ships in.
| if let Some(last_k) = last_final_batch_key.take() { | ||
| // the last final item was consumed: replenish the queue by querying more | ||
| // only the keys still missing to reach `count` are worth fetching | ||
| let remaining = count.map(|cnt| { |
There was a problem hiding this comment.
Correct but low value: the old count batch was already bounded, so this saves at most one partial batch. And it has a downside: once speculative_keys.len() hits count - 1, every speculatively-deleted final key ahead costs one lock + RocksDB seek for a single key (up to ~500 single-key fetches per API call instead of one batch). Not a DoS (the deletes cost gas) but a regression in the adversarial case. I'd revert this part to count (also removes the need for a dedicated test). If you want to keep it, add a floor (e.g. .max(64)).
| /// the reset entry mean the scan has to look past the first `count` entry keys to | ||
| /// still produce `count` keys. | ||
| #[test] | ||
| fn test_scan_datastore_count_is_a_prefix_of_unbounded() { |
There was a problem hiding this comment.
Good test, exactly the property we need. It only covers the Set path though. If the remaining change in the replenish loop stays, add the same prefix-of-unbounded check for the merge path (mock get_datastore_keys honoring the start bound and the count, so the refill is actually exercised).
Implements the #5255 spec (phase 2 of #5057): bound the work per batch, never the lock. - ExecutionQueryRequest.max_event_count: batch-wide event budget, init from new ApiConfig/GrpcConfig max_events_per_query settings (default 7000). 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. - 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, TODO: measure real size). - Datastore keys counted against max_response_size (sum of key lengths). - Single-shot get_filtered_sc_output_event unchanged (unbounded limit). - 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.
Implements the #5255 spec (phase 2 of #5057): bound the work per batch, never the lock. - ExecutionQueryRequest.max_event_count: batch-wide event budget, init from new ApiConfig/GrpcConfig max_events_per_query settings (default 7000). 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. - 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, TODO: measure real size). - Datastore keys counted against max_response_size (sum of key lengths). - Single-shot get_filtered_sc_output_event unchanged (unbounded limit). - 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.
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.
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.
…#5266) * [F40] refactor(execution-worker): enforce atomic batch reads by borrowing 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. * [F40] feat(execution): batch-wide event budget in query_state (#5255) 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.
4 participants
…parsing to the scan
resync_checkflag