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

RPC method state_nextKey #4717

Closed
xlc opened this issue Jan 23, 2020 · 9 comments
Closed

RPC method state_nextKey #4717

xlc opened this issue Jan 23, 2020 · 9 comments

Comments

@xlc
Copy link
Contributor

@xlc xlc commented Jan 23, 2020

Currently we have RPC state_getKeys which returns all keys with a same prefix, this may cause trouble for example trying to iterate all the account free balances.

Can we have state_nextKey that is basically sp_io::storage::next_key so SDK can control the iteration process.

@xlc

This comment has been minimized.

Copy link
Contributor Author

@xlc xlc commented Jan 23, 2020

And maybe we should eventually remove state_getKeys because it can be a trouble maker.

@bkchr

This comment has been minimized.

Copy link
Contributor

@bkchr bkchr commented Jan 23, 2020

I'm not sure this is the right way, before we had huge latency problems because the ui needed to query each key. Now you are saying that query all at once is a problem? Why? Because the result set could be to big?

I think we should not introduce getNextKey. We should make getKeys iteratable, probably by returning batches of the final result.

@xlc

This comment has been minimized.

Copy link
Contributor Author

@xlc xlc commented Jan 23, 2020

Without enforce some limitation on state_getKeys, it can make the node returns every keys in the state, which I think will be an issue later.
Batch return will work, it will basically be state_getNextKeys that takes a key, a number, and return that many number of keys. Then the node can add a max keys to return to ensure the RPC method doesn't take forever to return. This plays nicely with paging because the last key of previous call will be the input of next call. Otherwise you will need a way to perform paging for getKeys.
With get keys, the UI have to wait for the node to return all the keys and then start displaying them, with getNextKey, the UI can start showing things much faster.

@tomusdrw

This comment has been minimized.

Copy link
Contributor

@tomusdrw tomusdrw commented Jan 23, 2020

I'm with @bkchr here, the idea of state_getNextKeys is quite nice, but it seems that the only way we can implement that is to iterate over all past keys to find the one we should start sending, which makes the whole method quite inefficient (i.e. The further you go, the slower the RPC will respond).
Actually, NVM me. Looking at the PR I realised it's definitely possible to seek to a key in the trie.

@bkchr do you have an idea how the API could look like? We could have additional optional parameter from: Option<Key>, but it seems to me that a separate RPC method state_getKeysAfter()/state_getNextKeys() seems like a good idea. I'd consider deprecating state_geyKeys as indeed it may kill the node if a too large query is sent.

@xlc

This comment has been minimized.

Copy link
Contributor Author

@xlc xlc commented Jan 27, 2020

Can we agree a RPC interface before I start working on #4718 again?

My proposal is:

  • Change arguments of state_getKeys to key_prefix: StroageKey, hash: Option<Hash>, start_key: Option<StorageKey>, count: Option<u32>

    • start_key and count are used for paging / batching purpose
    • the iteration will starts from start_key, and stops when the next key of it does not have prefix of key_prefix
    • if start_key is None, key_prefix will be used
    • up to count keys will be returned, None means no limit, later maybe we want to make it default to some sensible value and enforce a max value, but no need to happen in this PR
    • this is compatible with the existing interface
  • I think state_getNextKey is still a useful primitive method, but let me know if I should keep this or not. I see no downside of having this other than more code to be maintained and documented.

@bkchr @tomusdrw

@tomusdrw

This comment has been minimized.

Copy link
Contributor

@tomusdrw tomusdrw commented Jan 29, 2020

@xlc I'd keep only one method long term. I think the best option would be to:

  1. Create a new method: state_getKeysPaged(prefix: StorageKey, count: u32, start_key: Option<StorageKey>, hash: Option<Hash>), reasoning for parameters order: count should be mandatory and we should return an error on big values (say above 10k), hash should stay as the last argument to be aligned with other methods.
  2. I'd avoid state_getNextKey to avoid endorsing iteration one-by-one. The same can be done via new state_getKeysPaged if really needed.
  3. Deprecate existing state_getKeys, implement it using the new method and keep for couple of versions, then remove entirely.

I'm not sold on state_getKeysPaged, but don't have a better idea for now. After we remove state_getKeys we can probably go back to the old name.

@bkchr

This comment has been minimized.

Copy link
Contributor

@bkchr bkchr commented Jan 29, 2020

Sounds good, however I also don't like the idea of state_getKeysPaged. Ideally the RPC could be some stream that only returns a new page when being "polled". I think currently the jsonrpc-crate doesn't support Streams and I'm not sure if that even is feasible.

@xlc

This comment has been minimized.

Copy link
Contributor Author

@xlc xlc commented Jan 29, 2020

This isn't subscripting future events, so server can only return data upon request. Then the request is basically just calling state_getKeysPaged. I can't think a better way. Also it is good to keep the server as stateless as possible (i.e. do not store paging details on server side), so load balancing won't cause any troubles.

@tomusdrw

This comment has been minimized.

Copy link
Contributor

@tomusdrw tomusdrw commented Jan 29, 2020

@bkchr Yeah, it's more a limitation of a stateless JSON-RPC protocol that we are using, we already bended the rules a bit to support subscriptions :)
I agree with @xlc that given current way we use the protocol (request-response HTTP especially) it's best to keep it stateless and within the bounds of what's currently possible and what users are used to.

@bkchr bkchr closed this in #4718 Feb 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

3 participants
You can’t perform that action at this time.