Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Limit datastore keys query #4805

Draft
wants to merge 16 commits into
base: mainnet_2_3
Choose a base branch
from
Draft

Limit datastore keys query #4805

wants to merge 16 commits into from

Conversation

modship
Copy link
Member

@modship modship commented Jan 6, 2025

  • document all added functions
  • try in sandbox /simulation/labnet
    • if part of node-launch, checked using the resync_check flag
  • unit tests on the added/changed features
    • make tests compile
    • make tests pass
  • add logs allowing easy debugging in case the changes caused problems
  • if the API has changed, update the API specification

@modship modship changed the title add parameters Limit datastore keys query Jan 6, 2025
@@ -46,6 +46,8 @@ pub struct NodeStatus {
pub chain_id: u64,
/// minimal fees to include an operation in a block
pub minimal_fees: Amount,
/// current mip version
pub current_mip_version: u32,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this a rebase artifact ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cherry picked the commit, the difference will disappear when mip_versionning will be merged into main_2_3

@@ -39,6 +39,8 @@
enable_broadcast = false
# deferred credits delta (in milliseconds)
deferred_credits_delta = 7776000000 # ~ 3 months (90×24×60×60×1000) in milliseconds
# max datastore keys query (also used by GRPC API)
#max_datastore_keys_query = 500
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there should be 2 distinct config parameters:

  • max datastore keys queries: limits the number of simultaneous key queries in the api
  • max datastore keys limit : limits the number of keys returned by a single query

@@ -458,6 +463,12 @@ impl MassaRpcServer for API<Public> {
let config = CompactConfig::default();
let now = MassaTime::now();

let current_mip_version = self
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rebase artifact ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@@ -555,6 +566,7 @@ impl MassaRpcServer for API<Public> {
current_cycle,
chain_id: self.0.api_settings.chain_id,
minimal_fees: self.0.api_settings.minimal_fees,
current_mip_version,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rebase artifact ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

massa-api/src/public.rs Outdated Show resolved Hide resolved
massa-api/src/public.rs Outdated Show resolved Hide resolved
massa-api/src/public.rs Outdated Show resolved Hide resolved
}
_ => {
return Err(
ApiError::ExecutionError("unexpected response".to_string()).into()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note: this is also triggered when keys from a non-existing address are queried, which is not unexpected but could be quite common

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same comment elsewhere

Copy link
Member Author

@modship modship Jan 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here we match for the type of enum ExecutionQueryResponseItem, we can't have anything else than AddressDatastoreKeys type. So i return an Error "in case if"
For NotFound error, it will be triggered at line 996 with 'ExecutionQueryError'

&self,
addr: &Address,
prefix: &[u8],
offset: Option<Bound<Vec<u8>>>,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do not use Option<Bound>, use the variant Bound::Unbounded when we don't want a bound

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Re-design datastore key queries (unify between JSON RPC API / GRPC / ABI AS / ABI WASMV1)
2 participants