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.

Sign up for GitHub

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

Jump to bottom

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

Open
3 tasks
Leo-Besancon opened this issue Jul 8, 2024 · 9 comments · May be fixed by #4805
Open
3 tasks

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

Leo-Besancon opened this issue Jul 8, 2024 · 9 comments · May be fixed by #4805
Assignees
@modship
Labels
p1

Comments

@Leo-Besancon
Copy link
Collaborator

The JsonRPC API endpoint get_addresses lists, in the response, all the datastore keys of the given addresses.
One issue with this is that there is no limits in the number of datastore entries for a given address. So if the address has a lot of keys, the response may be too large (e.g. larger than the default 50Mo response limit of the API) which causes issues.

For example, the following explorer page errors out because of it: https://explorer.massa.net/mainnet/address/AS1Ba1T2mMpHvLEhTvsNkSDqm4djno2ezVojGrtLf1wRd8ThfZD3
A request / response to the mainnet API showing the issue:

{
    "jsonrpc": "2.0",
    "id": 1,
    "method": "get_addresses",
    "params": [["AS1Ba1T2mMpHvLEhTvsNkSDqm4djno2ezVojGrtLf1wRd8ThfZD3"]]
}
{
    "jsonrpc": "2.0",
    "error": {
        "code": -32008,
        "message": "Response is too big",
        "data": "Exceeded max limit of 52428800"
    },
    "id": 1
}

TODO:

  • List all the current ways we list datastore key (json rpc API, GRPC, assembly script ABI, WasmV1 ABIs, etc.)
  • Design a unified way to list the keys
    • Use (Bound, Bound) to define the range of keys to query. This is a way to generalize the optional prefix in some of the listing cases, with Bounds being Included, Excluded or Unbounded.
    • Limit the number of keys returned (max num would be a constant, or maybe a config parameter instead when applicable)
    • Best if non-breaking
  • Implement and test all the key listing cases
@SlnPons
Copy link
Collaborator

SlnPons commented Nov 21, 2024

Note priority N°1 is to address API JSONRPC+GRPC.

@SlnPons SlnPons added the p2 label Nov 21, 2024
@damip
Copy link
Member

damip commented Nov 21, 2024

This can be broken down in two steps:

  1. API JSONRPC/GRPC (non-breaking)
  2. ABI (breaking)

and for our purposes, for now we mostly need the API side (P1). The ABI side can be "left for later" (P2).

@damip
Copy link
Member

damip commented Dec 5, 2024

How it would work:

  • optional key_start parameter that can be either
    • absent (no lower bound)
    • INCLUDED(key)
    • EXCLUDED(key)
  • same for optional key_end parameter:
    • absent (no upper bound)
    • INCLUDED(key)
    • EXCLUDED(key)
  • optional LIMIT parameter (default 100) and error if the user tries to set it higher than 500

@peterjah
Copy link
Collaborator

peterjah commented Dec 13, 2024 •
edited
Loading

  • It would be nice to have the ability to provide a filter when quering keys
  • maybe the get_addresses endpoint is not the good place to get storage keys? maybe it would be cleaner to create a new one "get_datastore_keys"
  • the "get_datastore_entries" return should also be modified to return a key/value list. because today you provide a list of key in input and receive a list of key in output, but you have to pray that the ordering is kept unchanged and there is no missing values. having key/value pairs as result would be safer and cleaner

@SlnPons SlnPons added p1 and removed p2 labels Jan 3, 2025
@modship
Copy link
Member

modship commented Jan 3, 2025 •
edited
Loading

  • QueryState
    • AddressDatastoreKeysCandidate -> Execution.get_final_and_candidate_datastore_keys()
    • AddressDatastoreKeysFinal -> Execution.get_final_and_candidate_datastore_keys()
  • massa-api :
    • get_addresses() -> Execution.get_addresses_infos()
  • interfaceImpl :
    • get_keys() -> Context.get_keys()
    • get_keys_for() -> Context.get_keys()
    • get_ds_keys_wasmv1() -> Context.get_keys()
  • Context:
    • get_keys() -> SpeculativeLedger.get_key()
  • SpeculativeLedger
    • get_keys() -> Ledger.get_datastore_keys()
  • Execution
    • get_addresses_infos() -> Execution.get_final_and_candidate_datastore_keys()
    • get_final_and_candidate_datastore_keys() -> Ledger.get_datastore_keys()

@damip
Copy link
Member

damip commented Jan 4, 2025

@modship for now we focus only on the jsonrpc and grpc APIs. Let's not do the ABI part for now

@modship
Copy link
Member

modship commented Jan 6, 2025

@modship for now we focus only on the jsonrpc and grpc APIs. Let's not do the ABI part for now

So, why not just re-using https://github.com/massalabs/massa/blob/main/massa-api-exports/src/page.rs ?
Like

massa/massa-api/src/public.rs

Line 568 in 2a71797

async fn get_stakers(

@damip
Copy link
Member

damip commented Jan 6, 2025 •
edited
Loading

@modship we cannot know the total count nor list all matching keys internally for performance reasons

@modship modship linked a pull request Jan 6, 2025 that will close this issue
Limit datastore keys query #4805
Draft
8 tasks
@modship modship self-assigned this Jan 6, 2025
@damip
Copy link
Member

damip commented Jan 12, 2025

After merging the API work, in order to prepare the ABI work:

check TODOs in:

  • interface_impl: get_keys
  • interface_impl: get_keys_for
  • interface_impl: get_ds_keys_wasmv1
  • impl ExecutionContext: get_keys

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@modship modship

Labels
p1
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Limit datastore keys query massalabs/massa
5 participants
@modship @damip @Leo-Besancon @peterjah @SlnPons